Dual-stack Handling #612
Dual-stack Handling #612
Conversation
| @@ -213,14 +213,14 @@ func TestConfigureKubernetes_ArtefactDownloaderErrorRKE2(t *testing.T) { | |||
| assert.Nil(t, scripts) | |||
| } | |||
|
|
|||
| func TestConfigureKubernetes_SuccessfulSingleNodeK3sCluster(t *testing.T) { | |||
| func TestConfigureKubernetes_SuccessfulSingleNodeK3sClusterIPv4(t *testing.T) { | |||
There was a problem hiding this comment.
I don't think we can afford to stretch these test cases that much. It's arguably similar to how we don't replicate these tests for all different CNIs. Do you mind discussing this offline?
pkg/combustion/kubernetes.go
Outdated
| "registryMirrors": prependArtefactPath(filepath.Join(k8sDir, registryMirrorsFileName)), | ||
| "configFilePath": prependArtefactPath(K8sDir), | ||
| "registryMirrors": prependArtefactPath(filepath.Join(K8sDir, registryMirrorsFileName)), | ||
| "setNodeIPScript": setNodeIPScript, |
There was a problem hiding this comment.
Can we get this value from somewhere else? It's better to "tie" it with the rest of the code, where this is a variable that's returned by the function which has created it. Not an arbitrary constant.
| switch { | ||
| case ip6 != nil && prioritizeIPv6: | ||
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) | ||
| case ip6 != nil && ip4 == nil: | ||
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) | ||
| default: | ||
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip4, port).String()) | ||
| } |
There was a problem hiding this comment.
If I had to improve, I'd go with this. But I'm not convinced it should be part of this function. The code is the same for both so the checks could be extracted out of this and only a single address could be passed.
| switch { | |
| case ip6 != nil && prioritizeIPv6: | |
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) | |
| case ip6 != nil && ip4 == nil: | |
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) | |
| default: | |
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip4, port).String()) | |
| } | |
| if ip6 != nil && (prioritizeIPv6 || ip4 == nil) { | |
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) | |
| return | |
| } | |
| config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip4, port).String()) |
pkg/image/validation/kubernetes.go
Outdated
| return failures | ||
| } | ||
|
|
||
| func checkCIDR(cidrs []string) string { |
There was a problem hiding this comment.
Do we handle single-stack IPv6?
pkg/image/validation/kubernetes.go
Outdated
| if len(parsedClusterCIDRs) == 0 && len(parsedServiceCIDRs) == 0 { | ||
| return failures | ||
| } |
There was a problem hiding this comment.
This essentially means that we're unable to catch bootstrap dual-stack config issues.
pkg/image/validation/kubernetes.go
Outdated
| if len(parsedServiceCIDRs) != 0 && len(parsedClusterCIDRs) == 0 { | ||
| failures = append(failures, FailedValidation{ | ||
| UserMessage: "Kubernetes server config cluster-cidr must be defined when service-cidr is defined", | ||
| }) | ||
|
|
||
| return failures | ||
| } else if len(parsedServiceCIDRs) == 0 && len(parsedClusterCIDRs) != 0 { | ||
| failures = append(failures, FailedValidation{ | ||
| UserMessage: "Kubernetes server config service-cidr must be defined when cluster-cidr is defined", | ||
| }) | ||
|
|
||
| return failures | ||
| } |
There was a problem hiding this comment.
In general (outside of dual-stack context), either of those should be configurable.
No description provided.