in-cluster DNS and load balancers on more platforms #1666
Conversation
proposes adding in-cluster DNS and load balancers as an option for more platform types than just the current `baremetal` and `openstack`.
mhrivnak
force-pushed
the
in-cluster-network-services
branch
from
159b7fd to
1e8dbd5
Compare
| CloudControllerManager CloudControllerManager `json:"cloudControllerManager,omitempty"` | ||
|
|
||
| // InClusterLoadBalancer is an optional feature that uses haproxy and | ||
| // keepalived as loadbalancers running in the cluster. Is is useful in |
There was a problem hiding this comment.
| // keepalived as loadbalancers running in the cluster. Is is useful in | |
| // keepalived as loadbalancers running in the cluster. This is useful in |
| type InClusterLoadBalancer struct { | ||
| // APIVIPs contains the VIP(s) to use for internal API communication. In | ||
| // dual stack clusters it contains an IPv4 and IPv6 address, otherwise only | ||
| // one VIP | ||
| // | ||
| // +kubebuilder:validation:MaxItems=2 | ||
| // +kubebuilder:validation:UniqueItems=true | ||
| // +kubebuilder:validation:Format=ip | ||
| APIVIPs []string `json:"apiVIPs,omitempty"` | ||
|
|
||
| // IngressVIPs contains the VIP(s) to use for ingress traffic. In dual stack | ||
| // clusters it contains an IPv4 and IPv6 address, otherwise only one VIP | ||
| // | ||
| // +kubebuilder:validation:MaxItems=2 | ||
| // +kubebuilder:validation:UniqueItems=true | ||
| // +kubebuilder:validation:Format=ip | ||
| IngressVIPs []string `json:"ingressVIPs,omitempty"` | ||
| } |
There was a problem hiding this comment.
are the existing API/ingress VIP fields going to be deprecated/moved to this type?
There was a problem hiding this comment.
Hopefully that's not necessary. I proposed an API here just to have a standing point for conversation, but I'm happy for the team to weigh in on what would make the most sense. There needs to be some balance of fitting in with the existing APIs, but also making it obvious and simple to enable or disable the feature. And for this proposal, the default should continue to be "disabled" since that matches existing behavior.
There was a problem hiding this comment.
Following external-lb-vips enhancement a loadBalancer property was added to the install config [here].(https://github.com/openshift/installer/pull/6812/files)
I think this enhancement is the counterpart of external-lb-vips enhancement and it could probably use the same API
There was a problem hiding this comment.
There's prior art for controlling loadbalancer deployment based on either the existence of the VIPs or an explicit field (as in the feature @eranco74 linked). The existence of the VIPs was something of a kludge because the VSphere UPI implementation was done before we had these components and we needed to be able to differentiate UPI and IPI without breaking existing logic.
Given that, I'd mildly prefer to use the explicit field (which would default to the opposite of what the existing platforms do), in part because the existing VSphere UPI logic is already a headache that semi-regularly trips us up. It's probably not a big deal as long as we pick a method and stick to it for these new platforms, but if we implicitly enable the LB when VIPs are provided, then we'll have three different sets of logic: Based only on the loadBalancer property (non-vsphere on-prem platforms), based on both the loadBalancer property and the existence of VIPs (vsphere), and only the existence of VIPs (none and external). We're less likely to accidentally break this in the future if it works the same as the other on-prem platforms and VSphere is our only outlier.
enhancements/network/in-cluster-dns-and-loadbalancers-on-more-platforms.md
Outdated
Show resolved
Hide resolved
|
cc @2uasimojo you might be interested in the install-config implications here |
Thanks @rvanderp3. I think these changes will be transparent to hive -- in fact, I'm not even sure if they're applicable to a hive-provisioned cluster -- but I'll keep an eye on this. |
Co-authored-by: Richard Vanderpool <[email protected]>
|
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 |
openshift-ci
bot
added
the
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 |
openshift-ci
bot
added
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-sigs/prow repository. |
|
/reopen |
|
@eranco74: 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-sigs/prow 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 |
|
/remove-lifecycle rotten |
openshift-ci
bot
removed
the
lifecycle/rotten
|
@mhrivnak: 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-sigs/prow repository. I understand the commands that are listed here. |
| // +kubebuilder:validation:UniqueItems=true | ||
| // +kubebuilder:validation:Format=ip |
There was a problem hiding this comment.
Nit, for real APIs we use CEL for both of these now, these validations don't actually work as intended
| `InternalDNS`. They are shown below, added to the existing settings for the | ||
| External platform type. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
| ``` | |
| ```go |
| The in-cluster network infrastructure has a limitation that it requires nodes | ||
| to be on the same subnet. This proposal does not seek to change or remove that |
There was a problem hiding this comment.
limitation that it requires nodes to be on the same subnet
Is this a hard limitation? In general BYO infra in cloud providers using publish strategy External they create public and private subnets, placing nodes in private subnets.
I see in the EP provide in-cluster implementations of network services that the the Load Balancer service is created, initially in Bootstrap node then moved to Control Plane nodes when InClusterLoadBalancer is set, would be possible to have certain flexibility to configure which node it will be deployed? Is it makes sense? I am wondering if we could advise "non-integrated" cloud providers which does not met LB requirements (eg hairpin), or does not offer cloud-based LBs to isolate the nodes where the Load Balancer is deployed in public installations, allowing them to also opt-in this feature, keeping that network standard: keeping control planes in private subnets, and haproxy deployed in nodes where they can expose to internet, in general public subnets.
There was a problem hiding this comment.
It is already possible to exercise at least some control over where the loadbalancer will run, which is how we deploy clusters with remote workers today. However, I am going to agree with @mhrivnak that this is out of scope in terms of simply enabling the DNS and LB infra on platform None. If changes to how that infra works are desired that should be a separate discussion
| The in-cluster network infrastructure has a limitation that it requires nodes | ||
| to be on the same subnet. This proposal does not seek to change or remove that |
There was a problem hiding this comment.
It is already possible to exercise at least some control over where the loadbalancer will run, which is how we deploy clusters with remote workers today. However, I am going to agree with @mhrivnak that this is out of scope in terms of simply enabling the DNS and LB infra on platform None. If changes to how that infra works are desired that should be a separate discussion
| type InClusterLoadBalancer struct { | ||
| // APIVIPs contains the VIP(s) to use for internal API communication. In | ||
| // dual stack clusters it contains an IPv4 and IPv6 address, otherwise only | ||
| // one VIP | ||
| // | ||
| // +kubebuilder:validation:MaxItems=2 | ||
| // +kubebuilder:validation:UniqueItems=true | ||
| // +kubebuilder:validation:Format=ip | ||
| APIVIPs []string `json:"apiVIPs,omitempty"` | ||
|
|
||
| // IngressVIPs contains the VIP(s) to use for ingress traffic. In dual stack | ||
| // clusters it contains an IPv4 and IPv6 address, otherwise only one VIP | ||
| // | ||
| // +kubebuilder:validation:MaxItems=2 | ||
| // +kubebuilder:validation:UniqueItems=true | ||
| // +kubebuilder:validation:Format=ip | ||
| IngressVIPs []string `json:"ingressVIPs,omitempty"` | ||
| } |
There was a problem hiding this comment.
There's prior art for controlling loadbalancer deployment based on either the existence of the VIPs or an explicit field (as in the feature @eranco74 linked). The existence of the VIPs was something of a kludge because the VSphere UPI implementation was done before we had these components and we needed to be able to differentiate UPI and IPI without breaking existing logic.
Given that, I'd mildly prefer to use the explicit field (which would default to the opposite of what the existing platforms do), in part because the existing VSphere UPI logic is already a headache that semi-regularly trips us up. It's probably not a big deal as long as we pick a method and stick to it for these new platforms, but if we implicitly enable the LB when VIPs are provided, then we'll have three different sets of logic: Based only on the loadBalancer property (non-vsphere on-prem platforms), based on both the loadBalancer property and the existence of VIPs (vsphere), and only the existence of VIPs (none and external). We're less likely to accidentally break this in the future if it works the same as the other on-prem platforms and VSphere is our only outlier.
|
|
||
| ## Alternatives | ||
|
|
||
| ### Move In-Cluster Network Settings Out of the Platform Spec |
There was a problem hiding this comment.
FWIW, in a perfect world I'd like to see this happen, for a couple of reasons:
- There's a ton of duplication in the API already, and we're probably only going to add support for more platforms over time. Each platform requires us to duplicate the entire in-cluster network API again.
- We anticipate having more cross-platform configuration settings in the near future and it would be nice to have a logical place to put them already present.
That said, I recognize this could complicate things quite a lot, as discussed in the cons section. I guess you can consider this a vote in favor of a common config section, but not a vote against the proposal above.
|
|
||
| const ( | ||
| // CoreDNS is the default service used to implement internal DNS within a cluster. | ||
| CoreDNS InternalDNS = "CoreDNS" |
There was a problem hiding this comment.
If we go with the loadBalancer field discussed above, we probably want this to have a default value of "None" or "UserManaged" or something like that to match the loadBalancer behavior.
We also may not want to use CoreDNS in the option name. There have been discussions about changing the implementation of the on-prem DNS to something lighter weight and it would be confusing if the option CoreDNS actually deployed dnsmasq. That's also why the loadBalancer parameter names are "OpenShiftManaged" and "UserManaged" instead of "keepalived" and "none".
proposes adding in-cluster DNS and load balancers as an option for more platform types than just the current
baremetalandopenstack.