CORS-4220: pass dualstack cluster cidrs as an comma-separated list argument #889
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references CORS-4220 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.21.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
|
@tthvo: This pull request references CORS-4220 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @sadasu @mkowalski |
| service-cluster-ip-range: {{range .ServiceClusterIPRange}} | ||
| - {{.}}{{end}} | ||
| service-cluster-ip-range: | ||
| - {{range $i, $ipRange := .ServiceClusterIPRange}}{{if $i}},{{end}}{{$ipRange}}{{end}} |
There was a problem hiding this comment.
FWIW, cluster-kube-apiserver-operator just does:
{{- if .ServiceCIDR | len}}
servicesSubnet: {{index .ServiceCIDR 0 -}}
{{- if .ServiceCIDR | len | eq 2}},{{index .ServiceCIDR 1}}{{end -}}
{{- end}}
both cluster-cidr and service-cluster-ip-range are required to be no more than 2 elements.
There was a problem hiding this comment.
Will this always be the case? We thought similarly about VIP, that there will be always only 1; then we thought similarly about machine networks, that there will be always maximum 2...
There was a problem hiding this comment.
Sure, but when we thought there was only going to be 1 before, we didn't know what the API was going to be for specifying 2, so we couldn't have pre-emptively supported 2 CIDRs here anyway. Same with if it gets extended to support more than 2 later. Probably it would work by just adding more commas and CIDRs, but we don't actually know that.
Also, it's possible we change the OCP config to allow more than 2 but the kcm arguments don't change, in which case adding all of the OCP config CIDRs would break the kcm config.
There was a problem hiding this comment.
Thanks guys for the discussion 🙏
IIUC, we know, as of now, cluster-cidr and service-cluster-ip-range cannot be more than 2 elements. So, we want to handle the max of 2 CIDRs each to ensure that we won't break kcm arguments in the future?
I updated the PR to address the discussion. It is similar to cluster-kube-apiserver-operator, but with a slight modification to make sure if multiple CIDRs are defined, kcm is passed the first 2 CIDRs that it can understand.
Please lmk if that's OK...
…d lists kube-controller-manager expects the dualstack cluster and service CIDRs to be passed as comma-separated list arguments [0]. This ensures the dualstack cluster and service cidrs are indeed passed as comma-separated lists. References [0] https://kubernetes.io/docs/concepts/services-networking/dual-stack/#configure-ipv4-ipv6-dual-stack
|
@tthvo: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski, tthvo 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 |
4 participants
kube-controller-managerexpects the dualstack cluster and service CIDRs to be passed as comma-separated list arguments according to the kubernetes IPv4/IPv6 documentation.This ensures the dualstack cluster and service cidrs are indeed passed as comma-separated lists.
Notes
There was previous unmerged work due to inactivity: #745, which has more details on why we need this.
Related Information
Tested with openshift/installer#9930
kube-controller-managerpod logs:KubeControllerManagerCR is rendered:$ oc get kubecontrollermanager cluster -o yaml | yq .spec logLevel: Normal managementState: Managed observedConfig: extendedArguments: cluster-cidr: - 10.128.0.0/14,fd01::/48 service-cluster-ip-range: - 172.30.0.0/16,fd02::/112 ...output-omitted...