-
Notifications
You must be signed in to change notification settings - Fork 582
OCPBUGS-18093:Allow users to specify Gateway Subnet #1410
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
OCPBUGS-18093:Allow users to specify Gateway Subnet #1410
Conversation
|
Hello @bpickard22! Some important instructions when contributing to openshift/api: |
|
LGTM |
|
need to do a codegen since the verify job is failing? |
tssurya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a few comments, but I know we are waiting for the bits from the generated code..
operator/v1/types_network.go
Outdated
| //The host is configured with these addresses, as well as the shared gateway | ||
| //bridge interface. The values can be changed after installation. | ||
| //The defaults are 169.254.169.1 - 169.254.169.4 and fd69::1 - fd69::4 | ||
| //+optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: These are kubebuilder formats, please follow convention and put the comment above the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example start the comment with gatewaySubnet contains.. blah blah...
| v6InternalSubnet: | ||
| description: v6InternalSubnet is a v6 subnet used internally by ovn-kubernetes in case the default one is being already used by something else. It must not overlap with any other subnet being used by OpenShift or by the node network. The size of the subnet must be larger than the number of nodes. The value cannot be changed after installation. Default is fd98::/48 | ||
| type: string | ||
| GatewaySubnet: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you manually edit this or did this get auto-generated? gatewaySubnet is what I'd expect...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that manually
operator/v1/types_network.go
Outdated
| // egressIPConfig holds the configuration for EgressIP options. | ||
| // +optional | ||
| EgressIPConfig EgressIPConfig `json:"egressIPConfig,omitempty"` | ||
| GatewaySubnet string `json:"GatewaySubnet,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see why now..
| GatewaySubnet string `json:"GatewaySubnet,omitempty"` | |
| GatewaySubnet string `json:"gatewaySubnet,omitempty"` |
please pay attention to semantics, notice how other fields are being done..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will correct
operator/v1/types_network.go
Outdated
| // egressIPConfig holds the configuration for EgressIP options. | ||
| // +optional | ||
| EgressIPConfig EgressIPConfig `json:"egressIPConfig,omitempty"` | ||
| GatewaySubnet string `json:"GatewaySubnet,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions:
Should we put these new options inside the GatewayConfig?
And shouldn't we have 2 subnet options one for v4 and another for v6 ? How do you plan to have both of them in this single field?
169.254.169.0/16 isn't exactly a Gateway Subnet, if we move this into GatewayConfig then we could call them V4InternalMasqueradeSubnet and V6InternalMasqueradeSubnet..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I think this makes more sense, will use this approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @trozet if he is ok with this.
b5a8524 to
2b49375
Compare
tssurya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there.
BTW this can't merge without the OVNK PR right? Probably best to put a hold until then?
operator/v1/types_network.go
Outdated
| // ovn-kubernetes to enable host to service traffic. The host is configured with these | ||
| // addresses, as well as the shared gateway bridge interface.The values can be changed after | ||
| // installation. | ||
| // The defaults are 169.254.169.1 - 169.254.169.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also use the 169.254.169.5 btw for the hairpin service traffic. Since its a subnet, its enough to say
Default is 169.254.169.0/16 or whatever is the default "subnet".
Doesn't make sense to quote the IPs within the subnet as defaults right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. BTW this can't merge without the OVNK PR right? Probably best to put a hold until then?
right, just wanted this up for reivew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tssurya should we just call it a range then. If the user needs to use those addresses for their infra, are we going to shift the masq addresses to a different subnet or just silde them down in the same subnet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a subnet. The defaults right now are:
V4MasqueradeSubnet = "169.254.169.0/29"
V6MasqueradeSubnet = "fd69::/125"
We can specify that the minimum cidr needs to be at least /29 for v4 and at least /125 for v6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trozet is there a reason that we need to specify that min cidr?
operator/v1/types_network.go
Outdated
| // ovn-kubernetes to enable host to service traffic. The host is configured with these | ||
| // addresses, as well as the shared gateway bridge interface.The values can be changed after | ||
| // installation. | ||
| // The defaults are fd69::1 - fd69::4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
operator/v1/types_network.go
Outdated
| // egressIPConfig holds the configuration for EgressIP options. | ||
| // +optional | ||
| EgressIPConfig EgressIPConfig `json:"egressIPConfig,omitempty"` | ||
| GatewaySubnet string `json:"GatewaySubnet,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @trozet if he is ok with this.
|
I thought we also need the ability to specify the join subnet? |
2b49375 to
3e7785f
Compare
@bpickard22 pointed out it already exists via: #1207 |
|
/lgtm |
|
/hold until OVNK bits land |
3e7785f to
680fd20
Compare
|
/hold cancel |
|
/lgtm |
|
/approve |
operator/v1/types_network.go
Outdated
| // +optional | ||
| IPForwarding IPForwardingMode `json:"ipForwarding,omitempty"` | ||
| // V4InternalMasqueradeSubnet contains the v4 masquerade addresses used internally by | ||
| // ovn-kubernetes to enable host to service traffic. The host is configured with these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These addresses would be configured on all hosts on the network, as they all use the same masq subnet to enable the host to service traffic flow described here https://github.com/ovn-org/ovn-kubernetes/blob/master/docs/design/host_to_services_OpenFlow.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifically, addresses from this subnet are configured on every OCP node, including as secondary IP addresses on host linux interfaces and as well as addresses assigned to internal OVN interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the description, perhaps Each host in the cluster is configured... covers this?
operator/v1/types_network.go
Outdated
| // The supported values are "Restricted" and "Global". | ||
| // +optional | ||
| IPForwarding IPForwardingMode `json:"ipForwarding,omitempty"` | ||
| // V4InternalMasqueradeSubnet contains the v4 masquerade addresses used internally by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what conditions would a cluster-admin want to change this and how would they know if they were choosing a "good" value? How would they know if their choice was bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cluster admin would want to change this if they want to addresses that we currently use here in their local infrastructure. I think "good" may be subjective, but any address range that neither we nor the cloud provider reserve should be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just say: The subnet chosen should not overlap with other networks specified for OVN-Kubernetes as well as other networks used on the host. Additionally the subnet must be large enough to accommodate 6 IPs (maximum prefix length /29).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description with this addition
operator/v1/types_network.go
Outdated
| // ovn-kubernetes to enable host to service traffic. The host is configured with these | ||
| // addresses, as well as the shared gateway bridge interface.The values can be changed after | ||
| // installation. | ||
| // The default subnet is 169.254.169.0/29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a regex that can validate this. I bet @Miciah knows for this and for the ipv6 one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont currently use regex to validate the other configurable subnet in ovn-k (the internal join subnet), and any invalid value passed in would be caught by the cno or in ovnk, do you think we need that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added regex taken from
api/operator/v1/types_ingress.go
Lines 378 to 386 in 3e3f07a
| // CIDR is an IP address range in CIDR notation (for example, "10.0.0.0/8" | |
| // or "fd00::/8"). | |
| // +kubebuilder:validation:Pattern=`(^(([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])\.){3}([0-9]|[0-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])/([0-9]|[12][0-9]|3[0-2])$)|(^s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]d|1dd|[1-9]?d)(.(25[0-5]|2[0-4]d|1dd|[1-9]?d)){3}))|:)))(%.+)?s*(\/(12[0-8]|1[0-1][0-9]|[1-9][0-9]|[0-9]))$)` | |
| // + --- | |
| // + The regex for the IPv4 CIDR range was taken from other CIDR fields in the OpenShift API | |
| // + and the one for the IPv6 CIDR range was taken from | |
| // + https://blog.markhatton.co.uk/2011/03/15/regular-expressions-for-ip-addresses-cidr-ranges-and-hostnames/ | |
| // + The resulting regex is an OR of both regexes. | |
| type CIDR string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
split into v4 and v6 respectively
JoelSpeed
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of nits to fix, but otherwise LGTM. I would appreciate if the networking team could ack the integration tests for the validations here, especially for the v6, we need to be sure we are validating the requirements of a v6 address correctly as we won't be able to change this once released.
| gatewayConfig: | ||
| ipv6: | ||
| internalMasqueradeSubnet: "abcd:ef01:2345:6789:abcd:ef01:2345::/125" | ||
| expectedError: "Invalid value: \"string\": a valid IPv6 address must contain 8 segments unless elided (::), in which case it must contain at most 6 non-empty segments" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, missing new line at the end of this file
operator/v1/types_network.go
Outdated
| // +kubebuilder:validation:XValidation:rule="[self.findAll('[0-9]+')[0]].all(x, x != '0' && int(x) <= 255 && !x.startsWith('0'))",message="first IP address octet must not contain leading zeros, must be greater than 0 and less or equal to 255" | ||
| // +kubebuilder:validation:XValidation:rule="[self.findAll('[0-9]+')[1], self.findAll('[0-9]+')[2], self.findAll('[0-9]+')[3]].all(x, int(x) <= 255 && (x == '0' || !x.startsWith('0')))",message="IP address octets must not contain leading zeros, and must be less or equal to 255" | ||
| // +optional | ||
| InternalMasqueradeSubnet string `json:"internalMasqueradeSubnet"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid issues with Go marshalling this to the empty string, In this case, I think it's safer to add omitempty than to update all validations to account for the empty string.
This would be an exception to our normal guidance for configuration APIs, and I appreciate I told you to remove this earlier. But in this case, because the validation will fail when the empty string is present, we want to ensure that unset Go types don't accidentally add the empty string for this value.
The alternative is to prefix every validation rule with self != '' || which IMO, does make things a little more complicated and then allows users to set the value to the empty string and believe that's a valid value, I would expect we want to prevent users setting the empty string here.
The same is true for the v6 address too.
Adds tests for KEL expressions for internalMasqueradeSubnet fields Remove redundant ipv4 and ipv6 name for internalMasqueradeSubnet fields inside their respective structs Signed-off-by: Ben Pickard <bpickard@redhat.com>
6ee3950 to
aa0120a
Compare
|
@JoelSpeed thank you for the thorough review. Is this PR good to go from your perspective? Looks like @bpickard22 has addressed most of the comments and has the integration tests in place as well. cc @trozet |
|
Yep, I'm good to label this, is the implementation PR ready to merge once this is done? Are we planning to merge during the bug window that's currently in place for 4.14? |
|
@JoelSpeed the implementation is ready. We can merge during the bug window (either way we will need a jira bug) as it is not a risky change to CNO and the OVNK bits have already landed downstream. |
|
Ack, @bpickard22 Can you link this and the implementation up to a bug so they are tied together, than we can merge if everyone is happy? @trozet You happy to add LGTM? I'll add approve once that and bug link are done |
trozet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
/retitle OCPBUGS-18093:Allow users to specify Gateway Subnet |
|
@bpickard22: This pull request references Jira Issue OCPBUGS-18093, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/jira refresh |
|
@bpickard22: This pull request references Jira Issue OCPBUGS-18093, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bpickard22, JoelSpeed, trozet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@bpickard22: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@bpickard22: Jira Issue OCPBUGS-18093: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-18093 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
…way-subnet-api"" This reverts commit 29d364b.
We currently reserve a range of addresses to configure host to service traffic internally in ovn. We need to allow users to specify this range to avoid conflicting with addresses they use in their local infra