Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle routing destinations and static addresses in API like the CLI [specific ci=Group23-VIC-Machine-Service] #6743

Merged

Conversation

zjs
Copy link
Member

@zjs zjs commented Nov 10, 2017

The vic-machine CLI has differing requirements for gateway routing information, depending on the type of network.

According to the CLI help:

  • a client gateway must specify one or more routing destinations
  • a public gateway must not specify any routing destinations
  • a management gateway must specify one or more routing destinations
  • a container gateway must specify exactly one routing destination

This does not seem to be enforced in code, and may simply be more of a suggestion about how these gateways should be used than a requirement.

Update the parsing for client, public, and management to support all zero or more routing destinations in all cases; defer to the existing ProcessNetwork code to ensure consistent validation behavior now and in the future.

Additionally update the parsing for container to provide a clear error message if the expected routing destination is not supplied.

Fixes #6715


The vic-machine CLI requires that static addresses are specified as a CIDR, which allows the static address and subnet mask to be supplied in a compact way on the command line. This pattern does not allow for static addresses to be expressed in terms of a hostname.

Update the API to match this convention. We may wish to allow for more flexibility in the API in the future, but there's value in at least starting with consistent behavior.

zjs added 2 commits November 9, 2017 16:19
The vic-machine CLI has differing requirements for gateway routing
information, depending on the type of network.

According to the CLI help:
 - a client gateway must specify one or more routing destinations
 - a public gateway must not specify any routing destinations
 - a management gateway must specify one or more routing destinations
 - a container gateway must specify exactly one routing destination

This does not seem to be enforced in code, and may simply be more of a
suggestion about how these gateways should be used than a requirement.

Update the parsing for client, public, and management to support all
zero or more routing destinations in all cases; defer to the existing
ProcessNetwork code to ensure consistent validation behavior now and
in the future.

Additionally update the parsing for container to provide a clear error
message if the expected routing destination is not supplied.
The vic-machine CLI requires that static addresses are specified as a
CIDR, which allows the static address and subnet mask to be supplied
in a compact way on the command line. This pattern does not allow for
static addresses to be expressed in terms of a hostname.

Update the API to match this convention. We may wish to allow for more
flexibility in the API in the future, but there's value in at least
starting with consistent behavior.
@zjs zjs self-assigned this Nov 10, 2017
@zjs zjs requested review from jzt, hickeng and mdharamadas1 November 10, 2017 00:21
@zjs
Copy link
Member Author

zjs commented Nov 10, 2017

Note that until #6559 is addressed, it is not feasible to write a comprehensive positive test case for this functionality.

Copy link
Contributor

@mdharamadas1 mdharamadas1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@zjs zjs changed the title Handle routing destinations and static addresses in API like the CLI Handle routing destinations and static addresses in API like the CLI [specific ci=Group23-VIC-Machine-Service] Nov 10, 2017
@zjs zjs merged commit 4a2b795 into vmware:feature/vic-machine-service Nov 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants