Skip to content
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.

Sign up for GitHub

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

Make a certain ipv4-vs-ipv6 config error non-fatal, for backward compat #121008

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Make a certain ipv4-vs-ipv6 config error non-fatal, for backward compat #121008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/kube-proxy/app/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) {
clusterCIDRs := strings.Split(s.Config.ClusterCIDR, ",")
if badCIDRs(clusterCIDRs, badFamily) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function badCidrs only 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:

  • cidrs is 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.
  • cidrs is 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.
  • cidrs is 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks for clarifying

errors = append(errors, fmt.Errorf("cluster is %s but clusterCIDRs contains only IPv%s addresses", clusterType, badFamily))
if s.Config.DetectLocalMode == kubeproxyconfig.LocalModeClusterCIDR {
if s.Config.DetectLocalMode == kubeproxyconfig.LocalModeClusterCIDR && !dualStackSupported {
// This has always been a fatal error
fatal = true
}
Expand Down
66 changes: 36 additions & 30 deletions cmd/kube-proxy/app/server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,11 +817,12 @@ func Test_detectNodeIPs(t *testing.T) {

func Test_checkIPConfig(t *testing.T) {
cases := []struct {
name string
proxy *ProxyServer
ssErr bool
dsErr bool
fatal bool
name string
proxy *ProxyServer
ssErr bool
ssFatal bool
dsErr bool
dsFatal bool
}{
{
name: "empty config",
Expand Down Expand Up @@ -874,9 +875,10 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv4Protocol,
},
ssErr: true,
dsErr: true,
fatal: false,
ssErr: true,
ssFatal: false,
dsErr: true,
dsFatal: false,
},
{
name: "wrong-family clusterCIDR when using ClusterCIDR LocalDetector",
Expand All @@ -887,9 +889,10 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv4Protocol,
},
ssErr: true,
dsErr: true,
fatal: true,
ssErr: true,
ssFatal: true,
dsErr: true,
dsFatal: false,
},

{
Expand Down Expand Up @@ -933,9 +936,10 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv6Protocol,
},
ssErr: true,
dsErr: true,
fatal: false,
ssErr: true,
ssFatal: false,
dsErr: true,
dsFatal: false,
},

{
Expand Down Expand Up @@ -983,9 +987,10 @@ func Test_checkIPConfig(t *testing.T) {
PrimaryIPFamily: v1.IPv4Protocol,
podCIDRs: []string{"fd01:2345::/64"},
},
ssErr: true,
dsErr: true,
fatal: true,
ssErr: true,
ssFatal: true,
dsErr: true,
dsFatal: true,
},

{
Expand All @@ -1011,9 +1016,10 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv4Protocol,
},
ssErr: true,
dsErr: true,
fatal: false,
ssErr: true,
ssFatal: false,
dsErr: true,
dsFatal: false,
},

{
Expand Down Expand Up @@ -1057,9 +1063,9 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv6Protocol,
},
ssErr: true,
dsErr: false,
fatal: false,
ssErr: true,
ssFatal: false,
dsErr: false,
},

{
Expand All @@ -1085,9 +1091,9 @@ func Test_checkIPConfig(t *testing.T) {
},
PrimaryIPFamily: v1.IPv6Protocol,
},
ssErr: true,
dsErr: false,
fatal: false,
ssErr: true,
ssFatal: false,
dsErr: false,
},
}

Expand All @@ -1098,17 +1104,17 @@ func Test_checkIPConfig(t *testing.T) {
t.Errorf("unexpected error in single-stack case: %v", err)
} else if err == nil && c.ssErr {
t.Errorf("unexpected lack of error in single-stack case")
} else if fatal != c.fatal {
t.Errorf("expected fatal=%v, got %v", c.fatal, fatal)
} else if fatal != c.ssFatal {
t.Errorf("expected fatal=%v, got %v", c.ssFatal, fatal)
}

err, fatal = checkIPConfig(c.proxy, true)
if err != nil && !c.dsErr {
t.Errorf("unexpected error in dual-stack case: %v", err)
} else if err == nil && c.dsErr {
t.Errorf("unexpected lack of error in dual-stack case")
} else if fatal != c.fatal {
t.Errorf("expected fatal=%v, got %v", c.fatal, fatal)
} else if fatal != c.dsFatal {
t.Errorf("expected fatal=%v, got %v", c.dsFatal, fatal)
}
})
}
Expand Down