Make a certain ipv4-vs-ipv6 config error non-fatal, for backward compat #121008
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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 APPROVED This pull-request has been approved by: danwinship 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 |
44b1b35
to
b2f0052
Compare
|
/lgtm |
|
LGTM label has been added. Git tree hash: 63fd0d1014bda6f45e8b34d204b99c01e7d7ab19
|
| @@ -678,7 +678,7 @@ func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) { | |||
| clusterCIDRs := strings.Split(s.Config.ClusterCIDR, ",") | |||
| if badCIDRs(clusterCIDRs, badFamily) { | |||
There was a problem hiding this comment.
this function badCidrs only checks that the first cidr in the array is different than the badFamily, but the comment of the function seems to assume all the cidrs are not badFamily
// badCIDRs returns true if cidrs is a non-empty list of CIDRs, all of wrongFamily.
just for my curiosity, the comment is wrong, right?
There was a problem hiding this comment.
I think the comment wording is weird but should be
// badCIDRs returns true if cidrs is a non-empty list of CIDRs and all are of wrongFamily.
essentially if the list is empty, or as soon as it finds a "good" CIDR that's not of wrongFamily it returns false.
There was a problem hiding this comment.
this function
badCidrsonly checks that the first cidr in the array is different than the badFamily
No, it loops over all of the CIDRs, but bails out early in some cases. The semantics of the function are sort of confusing, so it's hard for the comment to be both concise and non-confusing...
There are three possibilities:
cidrsis empty, meaning the admin isn't claiming anything about what the primary IP family is, so we don't learn anything about whether there is a config problem or not.cidrsis non-empty and contains at least one CIDR of the same family as nodeIP, meaning the admin is expecting that IP family to be in use, meaning it's OK that we detected nodeIP to be of that family.cidrsis non-empty and contains only CIDRs of the opposite IP family from nodeIP, meaning the admin is asserting that this is a single-stack cluster of that family, meaning our detected nodeIP is wrong.
badCIDRs returns false in the first two cases and true in the third.
yes, it is |
|
/retest seems like the failures are unrelated (flakes?) |
…121008-origin-release-1.28 Automated cherry pick of #121008: Make a certain ipv4-vs-ipv6 config error non-fatal, for
What type of PR is this?
/kind bug
/kind regression
/sig network
/priority important-soon
What this PR does / why we need it:
kube-proxy historically allowed badly-mismatched bits of configuration (e.g., specifying an IPv6
--cluster-cidrvalue on a seemingly dual-stack-IPv4-primary node). In 1.28 I consolidated the existing checks and added a bunch more, but I accidentally made one case fatal which had not been fatal in prior releases. This makes it just a warning again.(It is still fatal to specify an IPv6
--cluster-cidron a necessarily single-stack-IPv4 node (e.g., when kernel IPv6 is disabled).)Which issue(s) this PR fixes:
Fixes #120999
Does this PR introduce a user-facing change?