-
Notifications
You must be signed in to change notification settings - Fork 486
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
Enhancement proposal for OVN secondary networks #811
Conversation
Skipping CI for Draft Pull Request. |
9432582
to
a2dbbe3
Compare
4c8370c
to
78e7600
Compare
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Also add an example featuring external network connectivity. Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
I believe we had talked about a CRD for defining OVN-Kubernetes networks, rather than pulling them from the NAD. Do you still want to work on that? |
aa04fd2
to
042522e
Compare
Sure @squeed, just pushed a version with that I had half-prepared. |
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
042522e
to
61e3034
Compare
My comment is pretty close to cosmetic, so I'll make it here rather than in a review. In the I skimmed a bit at https://github.com/containernetworking/cni, but I could not locate explicit guidance. |
|
||
type OVNSecondaryNetworkSpec struct { | ||
// Subnet is a RFC 4632/4291-style string that represents an IP address and prefix length in CIDR notation | ||
Subnet string `json:"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.
How are subnets validated? Do we prevent overlapping subnets with other things in the cluster? (other secondary networks, node CIDRs for the primary network, ClusterCIDR, serviceCIDR, etc)
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.
Hm, I guess that it only matters if the overlay has external connectivity.
We could ensure that we only render NADs for disconnected subnets, or for subnets with external connectivities that do not clash with the subnets you've listed.
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.
Two things:
1 - Can you explain what this field does? Does it provide IPAM, or is it just for routing? Does it cause a gateway to be created (at the .1)? Too much magic in this field...
2 - This should always be an array for dual-stack (or just disjoint ranges).
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.
Two things:
1 - Can you explain what this field does? Does it provide IPAM, or is it just for routing? Does it cause a gateway to be created (at the .1)? Too much magic in this field...
I'll explain better.
2 - This should always be an array for dual-stack (or just disjoint ranges).
I am proposing a very simple model here; one IP per interface, and 1 subnet per logical switch. If the user desires dual stack, the option would be to create 2 OverlayNetwork
s, each with an IP version.
Is this model too narrow minded ?
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.
Starting simple is fine. However, let's just make this dual-stack from the get-go.
A convention is to have arrays for all IPs, and reject as invalid if there is more than one ip / cidr per family.
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.
Done.
type OVNSecondaryNetworkSpec struct { | ||
// Subnet is a RFC 4632/4291-style string that represents an IP address and prefix length in CIDR notation | ||
Subnet string `json:"subnet"` | ||
HasExternalConnectivity bool `json:"hasExternalConnectivity,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.
Should spell out the guarantees around this. Is traffic going to be NAT-ed? Directly dumped onto the host's underlay? If NAT then what IP does it get NAT-ed to? Are there any guarantees around ingress to this network, or is only reply traffic that is allowed?
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.
No ingress at all is in scope of this feature; We might eventually desire it, but it would be only via kubernetes service
s, which still need to be made available for secondary networks. I can add this to the non-goals section of this enhancement proposal, for clarity.
Regarding how egress is implemented, I would prefer to avoid NAT - using a localnet port on the overlay logical switch seems preferable.
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.
Perhaps ExternalConnectivity should be its own struct, to allow for expansion? What potential options are there?
- no external access
- using the node's gateway router
- A centralized gateway
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.
Using the node's gateway router, afaiu, implies that we will be sharing the OVN db between OVN-K and whatever implements these overlays, right ?
That would only be an option if this feature is implemented in OVN-Kubernetes - and, imo, is one of the advantages of implementing it there - i.e. we wouldn't need to implement a centralized gateway to provide external connectivity to the pods.
Never-the-less, I think having ExternalConnectivity
as a separate struct is a good suggestion, I'll model it; thanks!
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
/remove-lifecycle rotten |
/close Since this isn't being actively developed, let's close the PR to keep the review queue clear for work that is more urgent. We can reopen this PR or submit a new one when work resumes. |
@dhellmann: Closed this PR. 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. |
/reopen We will resume work on this enhancement in the following days / weeks. |
@maiqueb: Reopened this PR. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@maiqueb: The following test failed, say
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. |
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
Stale enhancement proposals rot after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle rotten |
Rotten enhancement proposals close after 7d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Reopen the proposal by commenting /close |
@openshift-bot: Closed this PR. 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. |
No description provided.