From bcafb098633923f427b54f4ee140dcc266f63f4d Mon Sep 17 00:00:00 2001 From: dbw7 Date: Tue, 15 Oct 2024 15:54:27 -0400 Subject: [PATCH 01/10] pre-testing-updates --- pkg/combustion/helm.go | 2 +- pkg/combustion/kubernetes.go | 22 ++++---- pkg/combustion/kubernetes_test.go | 10 ++-- .../templates/k3s-multi-node-installer.sh.tpl | 8 ++- .../k3s-single-node-installer.sh.tpl | 8 ++- pkg/combustion/templates/k8s-vip.yaml.tpl | 7 ++- .../rke2-multi-node-installer.sh.tpl | 8 ++- .../rke2-single-node-installer.sh.tpl | 8 ++- pkg/image/definition.go | 3 +- pkg/image/definition_test.go | 3 +- pkg/image/testdata/full-valid-example.yaml | 1 + pkg/image/validation/kubernetes.go | 2 +- pkg/image/validation/kubernetes_test.go | 2 +- pkg/kubernetes/cluster.go | 54 +++++++++++++++---- pkg/kubernetes/cluster_test.go | 17 +++--- 15 files changed, 110 insertions(+), 45 deletions(-) diff --git a/pkg/combustion/helm.go b/pkg/combustion/helm.go index e37d3241..458394cc 100644 --- a/pkg/combustion/helm.go +++ b/pkg/combustion/helm.go @@ -22,7 +22,7 @@ func ComponentHelmCharts(ctx *image.Context) ([]image.HelmChart, []image.HelmRep var charts []image.HelmChart var repos []image.HelmRepository - if ctx.ImageDefinition.Kubernetes.Network.APIVIP != "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 != "" || ctx.ImageDefinition.Kubernetes.Network.APIVIP6 != "" { metalLBChart := image.HelmChart{ Name: ctx.ArtifactSources.MetalLB.Chart, RepositoryName: metallbRepositoryName, diff --git a/pkg/combustion/kubernetes.go b/pkg/combustion/kubernetes.go index e8308506..c87140ef 100644 --- a/pkg/combustion/kubernetes.go +++ b/pkg/combustion/kubernetes.go @@ -147,7 +147,8 @@ func (c *Combustion) configureK3S(ctx *image.Context, cluster *kubernetes.Cluste templateValues := map[string]any{ "installScript": installScript, - "apiVIP": ctx.ImageDefinition.Kubernetes.Network.APIVIP, + "apiVIP4": ctx.ImageDefinition.Kubernetes.Network.APIVIP4, + "apiVIP6": ctx.ImageDefinition.Kubernetes.Network.APIVIP6, "apiHost": ctx.ImageDefinition.Kubernetes.Network.APIHost, "binaryPath": binaryPath, "imagesPath": imagesPath, @@ -158,7 +159,7 @@ func (c *Combustion) configureK3S(ctx *image.Context, cluster *kubernetes.Cluste singleNode := len(ctx.ImageDefinition.Kubernetes.Nodes) < 2 if singleNode { - if ctx.ImageDefinition.Kubernetes.Network.APIVIP == "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 == "" && ctx.ImageDefinition.Kubernetes.Network.APIVIP6 == "" { zap.S().Info("Virtual IP address for k3s cluster is not provided and will not be configured") } else { log.Audit("WARNING: A Virtual IP address for the k3s cluster has been provided. " + @@ -242,7 +243,8 @@ func (c *Combustion) configureRKE2(ctx *image.Context, cluster *kubernetes.Clust templateValues := map[string]any{ "installScript": installScript, - "apiVIP": ctx.ImageDefinition.Kubernetes.Network.APIVIP, + "apiVIP4": ctx.ImageDefinition.Kubernetes.Network.APIVIP4, + "apiVIP6": ctx.ImageDefinition.Kubernetes.Network.APIVIP6, "apiHost": ctx.ImageDefinition.Kubernetes.Network.APIHost, "installPath": installPath, "imagesPath": imagesPath, @@ -253,7 +255,7 @@ func (c *Combustion) configureRKE2(ctx *image.Context, cluster *kubernetes.Clust singleNode := len(ctx.ImageDefinition.Kubernetes.Nodes) < 2 if singleNode { - if ctx.ImageDefinition.Kubernetes.Network.APIVIP == "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 == "" && ctx.ImageDefinition.Kubernetes.Network.APIVIP6 == "" { zap.S().Info("Virtual IP address for RKE2 cluster is not provided and will not be configured") } @@ -317,11 +319,13 @@ func (c *Combustion) downloadRKE2Artefacts(ctx *image.Context, cluster *kubernet func kubernetesVIPManifest(k *image.Kubernetes) (string, error) { manifest := struct { - APIAddress string - RKE2 bool + APIAddress4 string + APIAddress6 string + RKE2 bool }{ - APIAddress: k.Network.APIVIP, - RKE2: strings.Contains(k.Version, image.KubernetesDistroRKE2), + APIAddress4: k.Network.APIVIP4, + APIAddress6: k.Network.APIVIP6, + RKE2: strings.Contains(k.Version, image.KubernetesDistroRKE2), } return template.Parse("k8s-vip", k8sVIPManifest, &manifest) @@ -367,7 +371,7 @@ func (c *Combustion) configureManifests(ctx *image.Context) (string, error) { manifestsPath := localKubernetesManifestsPath() manifestDestDir := filepath.Join(ctx.ArtefactsDir, manifestsPath) - if ctx.ImageDefinition.Kubernetes.Network.APIVIP != "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 != "" || ctx.ImageDefinition.Kubernetes.Network.APIVIP6 != "" { if err := os.MkdirAll(manifestDestDir, os.ModePerm); err != nil { return "", fmt.Errorf("creating manifests destination dir: %w", err) } diff --git a/pkg/combustion/kubernetes_test.go b/pkg/combustion/kubernetes_test.go index 700d4f73..854f8128 100644 --- a/pkg/combustion/kubernetes_test.go +++ b/pkg/combustion/kubernetes_test.go @@ -220,7 +220,7 @@ func TestConfigureKubernetes_SuccessfulSingleNodeK3sCluster(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+k3s1", Network: image.Network{ - APIVIP: "192.168.122.100", + APIVIP4: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } @@ -293,7 +293,7 @@ func TestConfigureKubernetes_SuccessfulMultiNodeK3sCluster(t *testing.T) { Version: "v1.30.3+k3s1", Network: image.Network{ APIHost: "api.cluster01.hosted.on.edge.suse.com", - APIVIP: "192.168.122.100", + APIVIP4: "192.168.122.100", }, Nodes: []image.Node{ { @@ -423,7 +423,7 @@ func TestConfigureKubernetes_SuccessfulSingleNodeRKE2Cluster(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+rke2r1", Network: image.Network{ - APIVIP: "192.168.122.100", + APIVIP4: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } @@ -492,7 +492,7 @@ func TestConfigureKubernetes_SuccessfulMultiNodeRKE2Cluster(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.cluster01.hosted.on.edge.suse.com", - APIVIP: "192.168.122.100", + APIVIP4: "192.168.122.100", }, Nodes: []image.Node{ { @@ -734,7 +734,7 @@ func TestConfigureKubernetes_SuccessfulRKE2ServerWithManifests(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+rke2r1", Network: image.Network{ - APIVIP: "192.168.122.100", + APIVIP4: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } diff --git a/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl b/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl index 31794eb6..864962e3 100644 --- a/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl +++ b/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl @@ -89,8 +89,12 @@ systemctl enable kubernetes-resources-install.service {{- end }} fi -{{- if and .apiVIP .apiHost }} -echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP4 .apiHost }} +echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts +{{- end }} + +{{- if and .apiVIP6 .apiHost }} +echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/k3s/ diff --git a/pkg/combustion/templates/k3s-single-node-installer.sh.tpl b/pkg/combustion/templates/k3s-single-node-installer.sh.tpl index f30e6a39..3b762246 100644 --- a/pkg/combustion/templates/k3s-single-node-installer.sh.tpl +++ b/pkg/combustion/templates/k3s-single-node-installer.sh.tpl @@ -62,8 +62,12 @@ EOF systemctl enable kubernetes-resources-install.service {{- end }} -{{- if and .apiVIP .apiHost }} -echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP4 .apiHost }} +echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts +{{- end }} + +{{- if and .apiVIP6 .apiHost }} +echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/k3s/ diff --git a/pkg/combustion/templates/k8s-vip.yaml.tpl b/pkg/combustion/templates/k8s-vip.yaml.tpl index dc75546f..3ca6df1c 100644 --- a/pkg/combustion/templates/k8s-vip.yaml.tpl +++ b/pkg/combustion/templates/k8s-vip.yaml.tpl @@ -6,7 +6,12 @@ metadata: namespace: metallb-system spec: addresses: - - {{ .APIAddress }}/32 + {{- if .APIAddress4 }} + - {{ .APIAddress4 }}/32 + {{- end }} + {{- if .APIAddress6 }} + - {{ .APIAddress6 }}/128 + {{- end }} avoidBuggyIPs: true serviceAllocation: namespaces: diff --git a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl index 91a29d97..cf882363 100644 --- a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl +++ b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl @@ -91,8 +91,12 @@ systemctl enable kubernetes-resources-install.service {{- end }} fi -{{- if .apiHost }} -echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP4 .apiHost }} +echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts +{{- end }} + +{{- if and .apiVIP6 .apiHost }} +echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/rke2/ diff --git a/pkg/combustion/templates/rke2-single-node-installer.sh.tpl b/pkg/combustion/templates/rke2-single-node-installer.sh.tpl index 3d144010..3e3c7b27 100644 --- a/pkg/combustion/templates/rke2-single-node-installer.sh.tpl +++ b/pkg/combustion/templates/rke2-single-node-installer.sh.tpl @@ -64,8 +64,12 @@ EOF systemctl enable kubernetes-resources-install.service {{- end }} -{{- if and .apiVIP .apiHost }} -echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP4 .apiHost }} +echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts +{{- end }} + +{{- if and .apiVIP6 .apiHost }} +echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/rke2/ diff --git a/pkg/image/definition.go b/pkg/image/definition.go index acf4ce8a..a5fcb11e 100644 --- a/pkg/image/definition.go +++ b/pkg/image/definition.go @@ -194,7 +194,8 @@ type Kubernetes struct { type Network struct { APIHost string `yaml:"apiHost"` - APIVIP string `yaml:"apiVIP"` + APIVIP4 string `yaml:"apiVIP"` + APIVIP6 string `yaml:"apiVIP6"` } type Node struct { diff --git a/pkg/image/definition_test.go b/pkg/image/definition_test.go index 1cc10eed..f620ff92 100644 --- a/pkg/image/definition_test.go +++ b/pkg/image/definition_test.go @@ -162,7 +162,8 @@ func TestParse(t *testing.T) { assert.Equal(t, "v1.30.3+rke2r1", kubernetes.Version) // Network - assert.Equal(t, "192.168.122.100", kubernetes.Network.APIVIP) + assert.Equal(t, "192.168.122.100", kubernetes.Network.APIVIP4) + assert.Equal(t, "fd12:3456:789a::21", kubernetes.Network.APIVIP6) assert.Equal(t, "api.cluster01.hosted.on.edge.suse.com", kubernetes.Network.APIHost) // Nodes diff --git a/pkg/image/testdata/full-valid-example.yaml b/pkg/image/testdata/full-valid-example.yaml index 90ad9d28..65b15c8b 100644 --- a/pkg/image/testdata/full-valid-example.yaml +++ b/pkg/image/testdata/full-valid-example.yaml @@ -83,6 +83,7 @@ kubernetes: version: v1.30.3+rke2r1 network: apiVIP: 192.168.122.100 + apiVIP6: fd12:3456:789a::21 apiHost: api.cluster01.hosted.on.edge.suse.com nodes: - hostname: node1.suse.com diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index 647a6326..206f9af0 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -52,7 +52,7 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation { return failures } - if k8s.Network.APIVIP == "" { + if k8s.Network.APIVIP4 == "" && k8s.Network.APIVIP6 == "" { failures = append(failures, FailedValidation{ UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", }) diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index 82406ddb..4bd03195 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -15,7 +15,7 @@ import ( var validNetwork = image.Network{ APIHost: "host.com", - APIVIP: "127.0.0.1", + APIVIP4: "127.0.0.1", } func TestValidateKubernetes(t *testing.T) { diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index 7c9cd909..0f715511 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io/fs" + "net/netip" "os" "path/filepath" "strings" @@ -54,7 +55,24 @@ func NewCluster(kubernetes *image.Kubernetes, configPath string) (*Cluster, erro return &Cluster{ServerConfig: serverConfig}, nil } - setMultiNodeConfigDefaults(kubernetes, serverConfig) + var ip4 *netip.Addr + if kubernetes.Network.APIVIP4 != "" { + ip4Holder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP4) + if ipErr != nil { + return nil, fmt.Errorf("parsing kubernetes ipv4 address: %w", ipErr) + } + ip4 = &ip4Holder + } + + var ip6 *netip.Addr + if kubernetes.Network.APIVIP6 != "" { + ip6Holder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP6) + if ipErr != nil { + return nil, fmt.Errorf("parsing kubernetes ipv6 address: %w", ipErr) + } + ip6 = &ip6Holder + } + setMultiNodeConfigDefaults(kubernetes, serverConfig, ip4, ip6) agentConfigPath := filepath.Join(configPath, agentConfigFile) agentConfig, err := ParseKubernetesConfig(agentConfigPath) @@ -137,8 +155,14 @@ func setSingleNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string if strings.Contains(kubernetes.Version, image.KubernetesDistroRKE2) { setClusterCNI(config) } - if kubernetes.Network.APIVIP != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP) + if kubernetes.Network.APIVIP4 != "" || kubernetes.Network.APIVIP6 != "" { + if kubernetes.Network.APIVIP4 != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP4) + } + + if kubernetes.Network.APIVIP6 != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP6) + } if strings.Contains(kubernetes.Version, image.KubernetesDistroK3S) { appendDisabledServices(config, "servicelb") @@ -150,22 +174,27 @@ func setSingleNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string delete(config, serverKey) } -func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string]any) { +func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string]any, ip4, ip6 *netip.Addr) { const ( k3sServerPort = 6443 rke2ServerPort = 9345 ) if strings.Contains(kubernetes.Version, image.KubernetesDistroRKE2) { - setClusterAPIAddress(config, kubernetes.Network.APIVIP, rke2ServerPort) + setClusterAPIAddress(config, ip4, ip6, rke2ServerPort) setClusterCNI(config) } else { - setClusterAPIAddress(config, kubernetes.Network.APIVIP, k3sServerPort) + setClusterAPIAddress(config, ip4, ip6, k3sServerPort) appendDisabledServices(config, "servicelb") } setClusterToken(config) - appendClusterTLSSAN(config, kubernetes.Network.APIVIP) + if kubernetes.Network.APIVIP4 != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP4) + } + if kubernetes.Network.APIVIP6 != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP6) + } setSELinux(config) if kubernetes.Network.APIHost != "" { appendClusterTLSSAN(config, kubernetes.Network.APIHost) @@ -196,13 +225,18 @@ func setClusterCNI(config map[string]any) { config[cniKey] = cniDefaultValue } -func setClusterAPIAddress(config map[string]any, apiAddress string, port int) { - if apiAddress == "" { +func setClusterAPIAddress(config map[string]any, ip4, ip6 *netip.Addr, port uint16) { + if ip4 == nil && ip6 == nil { zap.S().Warn("Attempted to set an empty cluster API address") return } - config[serverKey] = fmt.Sprintf("https://%s:%d", apiAddress, port) + switch { + case ip4 != nil: + config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip4, port).String()) + case ip6 != nil: + config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) + } } func setSELinux(config map[string]any) { diff --git a/pkg/kubernetes/cluster_test.go b/pkg/kubernetes/cluster_test.go index e1e5a950..31d4574b 100644 --- a/pkg/kubernetes/cluster_test.go +++ b/pkg/kubernetes/cluster_test.go @@ -1,6 +1,7 @@ package kubernetes import ( + "net/netip" "testing" "github.com/google/uuid" @@ -14,7 +15,7 @@ func TestNewCluster_SingleNodeRKE2_MissingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP: "192.168.122.50", + APIVIP4: "192.168.122.50", }, } @@ -39,7 +40,7 @@ func TestNewCluster_SingleNodeK3s_MissingConfig(t *testing.T) { Version: "v1.30.3+k3s1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP: "192.168.122.50", + APIVIP4: "192.168.122.50", }, } @@ -63,7 +64,7 @@ func TestNewCluster_SingleNode_ExistingConfig(t *testing.T) { kubernetes := &image.Kubernetes{ Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP: "192.168.122.50", + APIVIP4: "192.168.122.50", }, } @@ -87,7 +88,7 @@ func TestNewCluster_MultiNodeRKE2_MissingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP: "192.168.122.50", + APIVIP4: "192.168.122.50", }, Nodes: []image.Node{ { @@ -137,7 +138,7 @@ func TestNewCluster_MultiNodeRKE2_ExistingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP: "192.168.122.50", + APIVIP4: "192.168.122.50", }, Nodes: []image.Node{ { @@ -276,10 +277,12 @@ func TestIdentifyInitialiserNode(t *testing.T) { func TestSetClusterAPIAddress(t *testing.T) { config := map[string]any{} - setClusterAPIAddress(config, "", 9345) + setClusterAPIAddress(config, nil, nil, 9345) assert.NotContains(t, config, "server") + ip4, err := netip.ParseAddr("192.168.122.50") + assert.NoError(t, err) - setClusterAPIAddress(config, "192.168.122.50", 9345) + setClusterAPIAddress(config, &ip4, nil, 9345) assert.Equal(t, "https://192.168.122.50:9345", config["server"]) } From 26e872526ffb46db9c7a74f568ba853e41b06930 Mon Sep 17 00:00:00 2001 From: dbw7 Date: Wed, 16 Oct 2024 15:56:24 -0400 Subject: [PATCH 02/10] beginning to write tests --- pkg/image/validation/kubernetes.go | 75 +++++++- pkg/image/validation/kubernetes_test.go | 245 ++++++++++++++++++++++-- 2 files changed, 294 insertions(+), 26 deletions(-) diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index 206f9af0..fa3a8860 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -3,6 +3,7 @@ package validation import ( "errors" "fmt" + "net/netip" "net/url" "os" "path/filepath" @@ -32,6 +33,7 @@ func validateKubernetes(ctx *image.Context) []FailedValidation { return failures } + failures = append(failures, validateNetwork(&def.Kubernetes)...) failures = append(failures, validateNodes(&def.Kubernetes)...) failures = append(failures, validateManifestURLs(&def.Kubernetes)...) failures = append(failures, validateHelm(&def.Kubernetes, combustion.HelmValuesPath(ctx), combustion.HelmCertsPath(ctx))...) @@ -52,12 +54,6 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation { return failures } - if k8s.Network.APIVIP4 == "" && k8s.Network.APIVIP6 == "" { - failures = append(failures, FailedValidation{ - UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", - }) - } - var nodeTypes []string var nodeNames []string var initialisers []*image.Node @@ -117,6 +113,73 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation { return failures } +func validateNetwork(k8s *image.Kubernetes) []FailedValidation { + var failures []FailedValidation + + numNodes := len(k8s.Nodes) + if numNodes <= 1 { + // Single node cluster, node configurations are not required + return failures + } + + ip4 := k8s.Network.APIVIP4 + ip6 := k8s.Network.APIVIP6 + + if ip4 == "" && ip6 == "" { + failures = append(failures, FailedValidation{ + UserMessage: "The 'apiVIP' or 'apiVIP6' field is required in the 'network' section when defining entries under 'nodes'.", + }) + } + + if ip4 != "" { + ip, err := netip.ParseAddr(ip4) + if err != nil { + failures = append(failures, FailedValidation{ + UserMessage: fmt.Sprintf("Invalid IPV4 address %q for field 'apiVIP'.", ip4), + Error: err, + }) + } + + if !ip.Is4() { + failures = append(failures, FailedValidation{ + UserMessage: fmt.Sprintf("%q is not an IPV4 address, only IPV4 addresses are valid for field 'apiVIP'.", ip4), + }) + } + + if !ip.IsGlobalUnicast() { + msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", ip4) + failures = append(failures, FailedValidation{ + UserMessage: msg, + }) + } + } + + if ip6 != "" { + ip, err := netip.ParseAddr(ip6) + if err != nil { + failures = append(failures, FailedValidation{ + UserMessage: fmt.Sprintf("Invalid IPV6 address %q for field 'apiVIP6'.", ip6), + Error: err, + }) + } + + if !ip.Is6() { + failures = append(failures, FailedValidation{ + UserMessage: fmt.Sprintf("%q is not an IPV6 address, only IPV6 addresses are valid for field 'apiVIP6'.", ip6), + }) + } + + if !ip.IsGlobalUnicast() { + msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP6'.", ip6) + failures = append(failures, FailedValidation{ + UserMessage: msg, + }) + } + } + + return failures +} + func validateManifestURLs(k8s *image.Kubernetes) []FailedValidation { var failures []FailedValidation diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index 4bd03195..e828c683 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -15,7 +15,7 @@ import ( var validNetwork = image.Network{ APIHost: "host.com", - APIVIP4: "127.0.0.1", + APIVIP4: "192.168.1.1", } func TestValidateKubernetes(t *testing.T) { @@ -78,7 +78,10 @@ func TestValidateKubernetes(t *testing.T) { `failures all sections`: { K8s: image.Kubernetes{ Version: "v1.30.3", - Network: validNetwork, + Network: image.Network{ + APIHost: "host.com", + APIVIP4: "127.0.0.1", + }, Nodes: []image.Node{ { Type: image.KubernetesNodeTypeServer, @@ -116,6 +119,7 @@ func TestValidateKubernetes(t *testing.T) { "Helm chart 'name' field must be defined.", "Helm repository 'name' field for \"apache-repo\" must match the 'repositoryName' field in at least one defined Helm chart.", "Helm chart 'repositoryName' \"another-apache-repo\" for Helm chart \"\" does not match the name of any defined repository.", + "Invalid non-unicast cluster API address (127.0.0.1) for field 'apiVIP'.", }, }, } @@ -185,24 +189,24 @@ func TestValidateNodes(t *testing.T) { Nodes: []image.Node{}, }, }, - `with nodes - no network config`: { - K8s: image.Kubernetes{ - Network: image.Network{}, - Nodes: []image.Node{ - { - Hostname: "host1", - Type: image.KubernetesNodeTypeServer, - }, - { - Hostname: "host2", - Type: image.KubernetesNodeTypeAgent, - }, - }, - }, - ExpectedFailedMessages: []string{ - "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", - }, - }, + //`with nodes - no network config`: { + // K8s: image.Kubernetes{ + // Network: image.Network{}, + // Nodes: []image.Node{ + // { + // Hostname: "host1", + // Type: image.KubernetesNodeTypeServer, + // }, + // { + // Hostname: "host2", + // Type: image.KubernetesNodeTypeAgent, + // }, + // }, + // }, + // ExpectedFailedMessages: []string{ + // "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + // }, + //}, `no hostname`: { K8s: image.Kubernetes{ Network: validNetwork, @@ -1079,3 +1083,204 @@ func TestValidateAdditionalArtifacts(t *testing.T) { }) } } + +func TestValidateNodes(t *testing.T) { + tests := map[string]struct { + K8s image.Kubernetes + ExpectedFailedMessages []string + }{ + `valid`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "agent1", + Type: image.KubernetesNodeTypeAgent, + }, + { + Hostname: "server", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, + }, + }, + }, + `no nodes`: { + K8s: image.Kubernetes{ + Nodes: []image.Node{}, + }, + }, + //`with nodes - no network config`: { + // K8s: image.Kubernetes{ + // Network: image.Network{}, + // Nodes: []image.Node{ + // { + // Hostname: "host1", + // Type: image.KubernetesNodeTypeServer, + // }, + // { + // Hostname: "host2", + // Type: image.KubernetesNodeTypeAgent, + // }, + // }, + // }, + // ExpectedFailedMessages: []string{ + // "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + // }, + //}, + `no hostname`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "host1", + Type: image.KubernetesNodeTypeServer, + }, + { + Type: image.KubernetesNodeTypeServer, + }, + }, + }, + ExpectedFailedMessages: []string{ + "The 'hostname' field is required for entries in the 'nodes' section.", + }, + }, + `missing type`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "host1", + Type: image.KubernetesNodeTypeServer, + }, + { + Hostname: "valid", + }, + }, + }, + ExpectedFailedMessages: []string{ + fmt.Sprintf("The 'type' field for entries in the 'nodes' section must be one of: %s", strings.Join(validNodeTypes, ", ")), + }, + }, + `invalid type`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "valid", + Type: image.KubernetesNodeTypeServer, + }, + { + Hostname: "invalid", + Type: "abnormal", + }, + }, + }, + ExpectedFailedMessages: []string{ + fmt.Sprintf("The 'type' field for entries in the 'nodes' section must be one of: %s", strings.Join(validNodeTypes, ", ")), + }, + }, + `incorrect initialiser type`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "valid", + Type: image.KubernetesNodeTypeServer, + }, + { + Hostname: "invalid", + Initialiser: true, + Type: image.KubernetesNodeTypeAgent, + }, + }, + }, + ExpectedFailedMessages: []string{ + fmt.Sprintf("The node labeled with 'initialiser' must be of type '%s'.", image.KubernetesNodeTypeServer), + }, + }, + `duplicate entries`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "foo", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, + { + Hostname: "bar", + Type: image.KubernetesNodeTypeAgent, + }, + { + Hostname: "bar", + Type: image.KubernetesNodeTypeAgent, + }, + { + Hostname: "foo", + Type: image.KubernetesNodeTypeAgent, + }, + }, + }, + ExpectedFailedMessages: []string{ + "The 'nodes' section contains duplicate entries: bar, foo", + }, + }, + `no server node`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "foo", + Type: image.KubernetesNodeTypeAgent, + }, + { + Hostname: "bar", + Type: image.KubernetesNodeTypeAgent, + }, + }, + }, + ExpectedFailedMessages: []string{ + fmt.Sprintf("There must be at least one node of type '%s' defined.", image.KubernetesNodeTypeServer), + }, + }, + `multiple initialisers`: { + K8s: image.Kubernetes{ + Network: validNetwork, + Nodes: []image.Node{ + { + Hostname: "foo", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, + { + Hostname: "bar", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, + }, + }, + ExpectedFailedMessages: []string{ + "Only one node may be specified as the cluster initializer.", + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + k := test.K8s + failures := validateNodes(&k) + assert.Len(t, failures, len(test.ExpectedFailedMessages)) + + var foundMessages []string + for _, foundValidation := range failures { + foundMessages = append(foundMessages, foundValidation.UserMessage) + } + + for _, expectedMessage := range test.ExpectedFailedMessages { + assert.Contains(t, foundMessages, expectedMessage) + } + + }) + } +} From 1e82f2235c4777746547d6a927c2fbf8d3133d30 Mon Sep 17 00:00:00 2001 From: dbw7 Date: Thu, 17 Oct 2024 23:54:09 -0400 Subject: [PATCH 03/10] ipv6 handling --- pkg/combustion/helm.go | 2 +- pkg/combustion/kubernetes.go | 32 +-- pkg/combustion/kubernetes_test.go | 54 ++++- .../templates/k3s-multi-node-installer.sh.tpl | 8 +- .../k3s-single-node-installer.sh.tpl | 8 +- pkg/combustion/templates/k8s-vip.yaml.tpl | 8 +- .../rke2-multi-node-installer.sh.tpl | 9 +- .../rke2-single-node-installer.sh.tpl | 8 +- pkg/image/definition.go | 3 +- pkg/image/definition_test.go | 3 +- pkg/image/testdata/full-valid-example.yaml | 1 - pkg/image/validation/kubernetes.go | 45 +--- pkg/image/validation/kubernetes_test.go | 213 +++++------------- pkg/kubernetes/cluster.go | 38 ++-- pkg/kubernetes/cluster_test.go | 10 +- 15 files changed, 165 insertions(+), 277 deletions(-) diff --git a/pkg/combustion/helm.go b/pkg/combustion/helm.go index 458394cc..e37d3241 100644 --- a/pkg/combustion/helm.go +++ b/pkg/combustion/helm.go @@ -22,7 +22,7 @@ func ComponentHelmCharts(ctx *image.Context) ([]image.HelmChart, []image.HelmRep var charts []image.HelmChart var repos []image.HelmRepository - if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 != "" || ctx.ImageDefinition.Kubernetes.Network.APIVIP6 != "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP != "" { metalLBChart := image.HelmChart{ Name: ctx.ArtifactSources.MetalLB.Chart, RepositoryName: metallbRepositoryName, diff --git a/pkg/combustion/kubernetes.go b/pkg/combustion/kubernetes.go index c87140ef..e7bb82a6 100644 --- a/pkg/combustion/kubernetes.go +++ b/pkg/combustion/kubernetes.go @@ -3,6 +3,7 @@ package combustion import ( _ "embed" "fmt" + "net/netip" "os" "path/filepath" "strings" @@ -147,8 +148,7 @@ func (c *Combustion) configureK3S(ctx *image.Context, cluster *kubernetes.Cluste templateValues := map[string]any{ "installScript": installScript, - "apiVIP4": ctx.ImageDefinition.Kubernetes.Network.APIVIP4, - "apiVIP6": ctx.ImageDefinition.Kubernetes.Network.APIVIP6, + "apiVIP": ctx.ImageDefinition.Kubernetes.Network.APIVIP, "apiHost": ctx.ImageDefinition.Kubernetes.Network.APIHost, "binaryPath": binaryPath, "imagesPath": imagesPath, @@ -159,7 +159,7 @@ func (c *Combustion) configureK3S(ctx *image.Context, cluster *kubernetes.Cluste singleNode := len(ctx.ImageDefinition.Kubernetes.Nodes) < 2 if singleNode { - if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 == "" && ctx.ImageDefinition.Kubernetes.Network.APIVIP6 == "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP == "" { zap.S().Info("Virtual IP address for k3s cluster is not provided and will not be configured") } else { log.Audit("WARNING: A Virtual IP address for the k3s cluster has been provided. " + @@ -243,8 +243,7 @@ func (c *Combustion) configureRKE2(ctx *image.Context, cluster *kubernetes.Clust templateValues := map[string]any{ "installScript": installScript, - "apiVIP4": ctx.ImageDefinition.Kubernetes.Network.APIVIP4, - "apiVIP6": ctx.ImageDefinition.Kubernetes.Network.APIVIP6, + "apiVIP": ctx.ImageDefinition.Kubernetes.Network.APIVIP, "apiHost": ctx.ImageDefinition.Kubernetes.Network.APIHost, "installPath": installPath, "imagesPath": imagesPath, @@ -255,7 +254,7 @@ func (c *Combustion) configureRKE2(ctx *image.Context, cluster *kubernetes.Clust singleNode := len(ctx.ImageDefinition.Kubernetes.Nodes) < 2 if singleNode { - if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 == "" && ctx.ImageDefinition.Kubernetes.Network.APIVIP6 == "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP == "" { zap.S().Info("Virtual IP address for RKE2 cluster is not provided and will not be configured") } @@ -318,14 +317,21 @@ func (c *Combustion) downloadRKE2Artefacts(ctx *image.Context, cluster *kubernet } func kubernetesVIPManifest(k *image.Kubernetes) (string, error) { + ip, err := netip.ParseAddr(k.Network.APIVIP) + if err != nil { + return "", fmt.Errorf("parsing kubernetes APIVIP address: %w", err) + } + manifest := struct { - APIAddress4 string - APIAddress6 string - RKE2 bool + APIAddress string + IsIPV4 bool + IsIPV6 bool + RKE2 bool }{ - APIAddress4: k.Network.APIVIP4, - APIAddress6: k.Network.APIVIP6, - RKE2: strings.Contains(k.Version, image.KubernetesDistroRKE2), + APIAddress: k.Network.APIVIP, + IsIPV4: ip.Is4(), + IsIPV6: ip.Is6(), + RKE2: strings.Contains(k.Version, image.KubernetesDistroRKE2), } return template.Parse("k8s-vip", k8sVIPManifest, &manifest) @@ -371,7 +377,7 @@ func (c *Combustion) configureManifests(ctx *image.Context) (string, error) { manifestsPath := localKubernetesManifestsPath() manifestDestDir := filepath.Join(ctx.ArtefactsDir, manifestsPath) - if ctx.ImageDefinition.Kubernetes.Network.APIVIP4 != "" || ctx.ImageDefinition.Kubernetes.Network.APIVIP6 != "" { + if ctx.ImageDefinition.Kubernetes.Network.APIVIP != "" { if err := os.MkdirAll(manifestDestDir, os.ModePerm); err != nil { return "", fmt.Errorf("creating manifests destination dir: %w", err) } diff --git a/pkg/combustion/kubernetes_test.go b/pkg/combustion/kubernetes_test.go index 854f8128..020dd5c0 100644 --- a/pkg/combustion/kubernetes_test.go +++ b/pkg/combustion/kubernetes_test.go @@ -220,7 +220,7 @@ func TestConfigureKubernetes_SuccessfulSingleNodeK3sCluster(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+k3s1", Network: image.Network{ - APIVIP4: "192.168.122.100", + APIVIP: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } @@ -293,7 +293,7 @@ func TestConfigureKubernetes_SuccessfulMultiNodeK3sCluster(t *testing.T) { Version: "v1.30.3+k3s1", Network: image.Network{ APIHost: "api.cluster01.hosted.on.edge.suse.com", - APIVIP4: "192.168.122.100", + APIVIP: "192.168.122.100", }, Nodes: []image.Node{ { @@ -423,7 +423,7 @@ func TestConfigureKubernetes_SuccessfulSingleNodeRKE2Cluster(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+rke2r1", Network: image.Network{ - APIVIP4: "192.168.122.100", + APIVIP: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } @@ -492,7 +492,7 @@ func TestConfigureKubernetes_SuccessfulMultiNodeRKE2Cluster(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.cluster01.hosted.on.edge.suse.com", - APIVIP4: "192.168.122.100", + APIVIP: "192.168.122.100", }, Nodes: []image.Node{ { @@ -734,7 +734,7 @@ func TestConfigureKubernetes_SuccessfulRKE2ServerWithManifests(t *testing.T) { ctx.ImageDefinition.Kubernetes = image.Kubernetes{ Version: "v1.30.3+rke2r1", Network: image.Network{ - APIVIP4: "192.168.122.100", + APIVIP: "192.168.122.100", APIHost: "api.cluster01.hosted.on.edge.suse.com", }, } @@ -821,3 +821,47 @@ func TestConfigureKubernetes_SuccessfulRKE2ServerWithManifests(t *testing.T) { assert.Contains(t, contents, "name: my-nginx") assert.Contains(t, contents, "image: nginx:1.14.2") } + +func TestKubernetesVIPManifestValidIPV4(t *testing.T) { + k8s := &image.Kubernetes{ + Version: "v1.30.3+rke2r1", + Network: image.Network{ + APIVIP: "192.168.1.1", + }, + } + + manifest, err := kubernetesVIPManifest(k8s) + require.NoError(t, err) + + assert.Contains(t, manifest, "- 192.168.1.1/32") + assert.Contains(t, manifest, "- name: rke2-api") +} + +func TestKubernetesVIPManifestValidIPV6(t *testing.T) { + k8s := &image.Kubernetes{ + Version: "v1.30.3+k3s1", + Network: image.Network{ + APIVIP: "fd12:3456:789a::21", + }, + } + + manifest, err := kubernetesVIPManifest(k8s) + require.NoError(t, err) + fmt.Println(manifest) + + assert.Contains(t, manifest, "- fd12:3456:789a::21/128") + assert.Contains(t, manifest, "- name: k8s-api") + assert.NotContains(t, manifest, "rke2") +} + +func TestKubernetesVIPManifestInvalidIP(t *testing.T) { + k8s := &image.Kubernetes{ + Version: "v1.30.3+k3s1", + Network: image.Network{ + APIVIP: "1111", + }, + } + + _, err := kubernetesVIPManifest(k8s) + require.ErrorContains(t, err, "parsing kubernetes APIVIP address: ParseAddr(\"1111\"): unable to parse IP") +} diff --git a/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl b/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl index 864962e3..31794eb6 100644 --- a/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl +++ b/pkg/combustion/templates/k3s-multi-node-installer.sh.tpl @@ -89,12 +89,8 @@ systemctl enable kubernetes-resources-install.service {{- end }} fi -{{- if and .apiVIP4 .apiHost }} -echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts -{{- end }} - -{{- if and .apiVIP6 .apiHost }} -echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP .apiHost }} +echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/k3s/ diff --git a/pkg/combustion/templates/k3s-single-node-installer.sh.tpl b/pkg/combustion/templates/k3s-single-node-installer.sh.tpl index 3b762246..f30e6a39 100644 --- a/pkg/combustion/templates/k3s-single-node-installer.sh.tpl +++ b/pkg/combustion/templates/k3s-single-node-installer.sh.tpl @@ -62,12 +62,8 @@ EOF systemctl enable kubernetes-resources-install.service {{- end }} -{{- if and .apiVIP4 .apiHost }} -echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts -{{- end }} - -{{- if and .apiVIP6 .apiHost }} -echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP .apiHost }} +echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/k3s/ diff --git a/pkg/combustion/templates/k8s-vip.yaml.tpl b/pkg/combustion/templates/k8s-vip.yaml.tpl index 3ca6df1c..7dcc67bf 100644 --- a/pkg/combustion/templates/k8s-vip.yaml.tpl +++ b/pkg/combustion/templates/k8s-vip.yaml.tpl @@ -6,11 +6,11 @@ metadata: namespace: metallb-system spec: addresses: - {{- if .APIAddress4 }} - - {{ .APIAddress4 }}/32 + {{- if .IsIPV4 }} + - {{ .APIAddress }}/32 {{- end }} - {{- if .APIAddress6 }} - - {{ .APIAddress6 }}/128 + {{- if .IsIPV6 }} + - {{ .APIAddress }}/128 {{- end }} avoidBuggyIPs: true serviceAllocation: diff --git a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl index cf882363..2d633d6e 100644 --- a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl +++ b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl @@ -91,14 +91,9 @@ systemctl enable kubernetes-resources-install.service {{- end }} fi -{{- if and .apiVIP4 .apiHost }} -echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP .apiHost }} +echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts {{- end }} - -{{- if and .apiVIP6 .apiHost }} -echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts -{{- end }} - mkdir -p /etc/rancher/rke2/ cp $CONFIGFILE /etc/rancher/rke2/config.yaml diff --git a/pkg/combustion/templates/rke2-single-node-installer.sh.tpl b/pkg/combustion/templates/rke2-single-node-installer.sh.tpl index 3e3c7b27..3d144010 100644 --- a/pkg/combustion/templates/rke2-single-node-installer.sh.tpl +++ b/pkg/combustion/templates/rke2-single-node-installer.sh.tpl @@ -64,12 +64,8 @@ EOF systemctl enable kubernetes-resources-install.service {{- end }} -{{- if and .apiVIP4 .apiHost }} -echo "{{ .apiVIP4 }} {{ .apiHost }}" >> /etc/hosts -{{- end }} - -{{- if and .apiVIP6 .apiHost }} -echo "{{ .apiVIP6 }} {{ .apiHost }}" >> /etc/hosts +{{- if and .apiVIP .apiHost }} +echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts {{- end }} mkdir -p /etc/rancher/rke2/ diff --git a/pkg/image/definition.go b/pkg/image/definition.go index a5fcb11e..acf4ce8a 100644 --- a/pkg/image/definition.go +++ b/pkg/image/definition.go @@ -194,8 +194,7 @@ type Kubernetes struct { type Network struct { APIHost string `yaml:"apiHost"` - APIVIP4 string `yaml:"apiVIP"` - APIVIP6 string `yaml:"apiVIP6"` + APIVIP string `yaml:"apiVIP"` } type Node struct { diff --git a/pkg/image/definition_test.go b/pkg/image/definition_test.go index f620ff92..1cc10eed 100644 --- a/pkg/image/definition_test.go +++ b/pkg/image/definition_test.go @@ -162,8 +162,7 @@ func TestParse(t *testing.T) { assert.Equal(t, "v1.30.3+rke2r1", kubernetes.Version) // Network - assert.Equal(t, "192.168.122.100", kubernetes.Network.APIVIP4) - assert.Equal(t, "fd12:3456:789a::21", kubernetes.Network.APIVIP6) + assert.Equal(t, "192.168.122.100", kubernetes.Network.APIVIP) assert.Equal(t, "api.cluster01.hosted.on.edge.suse.com", kubernetes.Network.APIHost) // Nodes diff --git a/pkg/image/testdata/full-valid-example.yaml b/pkg/image/testdata/full-valid-example.yaml index 65b15c8b..90ad9d28 100644 --- a/pkg/image/testdata/full-valid-example.yaml +++ b/pkg/image/testdata/full-valid-example.yaml @@ -83,7 +83,6 @@ kubernetes: version: v1.30.3+rke2r1 network: apiVIP: 192.168.122.100 - apiVIP6: fd12:3456:789a::21 apiHost: api.cluster01.hosted.on.edge.suse.com nodes: - hostname: node1.suse.com diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index fa3a8860..ec969b0f 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -115,6 +115,7 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation { func validateNetwork(k8s *image.Kubernetes) []FailedValidation { var failures []FailedValidation + ip := k8s.Network.APIVIP numNodes := len(k8s.Nodes) if numNodes <= 1 { @@ -122,55 +123,29 @@ func validateNetwork(k8s *image.Kubernetes) []FailedValidation { return failures } - ip4 := k8s.Network.APIVIP4 - ip6 := k8s.Network.APIVIP6 - - if ip4 == "" && ip6 == "" { + if ip == "" { failures = append(failures, FailedValidation{ - UserMessage: "The 'apiVIP' or 'apiVIP6' field is required in the 'network' section when defining entries under 'nodes'.", + UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", }) } - if ip4 != "" { - ip, err := netip.ParseAddr(ip4) - if err != nil { - failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("Invalid IPV4 address %q for field 'apiVIP'.", ip4), - Error: err, - }) - } - - if !ip.Is4() { - failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("%q is not an IPV4 address, only IPV4 addresses are valid for field 'apiVIP'.", ip4), - }) - } - - if !ip.IsGlobalUnicast() { - msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", ip4) - failures = append(failures, FailedValidation{ - UserMessage: msg, - }) - } - } - - if ip6 != "" { - ip, err := netip.ParseAddr(ip6) + if ip != "" { + parsedIP, err := netip.ParseAddr(ip) if err != nil { failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("Invalid IPV6 address %q for field 'apiVIP6'.", ip6), + UserMessage: fmt.Sprintf("Invalid APIVIP address %q for field 'apiVIP'.", ip), Error: err, }) } - if !ip.Is6() { + if !parsedIP.Is4() && !parsedIP.Is6() { failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("%q is not an IPV6 address, only IPV6 addresses are valid for field 'apiVIP6'.", ip6), + UserMessage: fmt.Sprintf("%q is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", ip), }) } - if !ip.IsGlobalUnicast() { - msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP6'.", ip6) + if !parsedIP.IsGlobalUnicast() { + msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", ip) failures = append(failures, FailedValidation{ UserMessage: msg, }) diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index e828c683..b9d5c1f7 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -15,7 +15,20 @@ import ( var validNetwork = image.Network{ APIHost: "host.com", - APIVIP4: "192.168.1.1", + APIVIP: "192.168.1.1", +} + +var multiNode = []image.Node{ + { + Hostname: "foo", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, + { + Hostname: "bar", + Type: image.KubernetesNodeTypeServer, + Initialiser: true, + }, } func TestValidateKubernetes(t *testing.T) { @@ -80,7 +93,7 @@ func TestValidateKubernetes(t *testing.T) { Version: "v1.30.3", Network: image.Network{ APIHost: "host.com", - APIVIP4: "127.0.0.1", + APIVIP: "127.0.0.1", }, Nodes: []image.Node{ { @@ -189,24 +202,6 @@ func TestValidateNodes(t *testing.T) { Nodes: []image.Node{}, }, }, - //`with nodes - no network config`: { - // K8s: image.Kubernetes{ - // Network: image.Network{}, - // Nodes: []image.Node{ - // { - // Hostname: "host1", - // Type: image.KubernetesNodeTypeServer, - // }, - // { - // Hostname: "host2", - // Type: image.KubernetesNodeTypeAgent, - // }, - // }, - // }, - // ExpectedFailedMessages: []string{ - // "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", - // }, - //}, `no hostname`: { K8s: image.Kubernetes{ Network: validNetwork, @@ -1084,184 +1079,82 @@ func TestValidateAdditionalArtifacts(t *testing.T) { } } -func TestValidateNodes(t *testing.T) { +func TestValidateNetwork(t *testing.T) { tests := map[string]struct { K8s image.Kubernetes ExpectedFailedMessages []string }{ - `valid`: { + `no network defined`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "agent1", - Type: image.KubernetesNodeTypeAgent, - }, - { - Hostname: "server", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, - }, - }, - }, - `no nodes`: { - K8s: image.Kubernetes{ - Nodes: []image.Node{}, - }, - }, - //`with nodes - no network config`: { - // K8s: image.Kubernetes{ - // Network: image.Network{}, - // Nodes: []image.Node{ - // { - // Hostname: "host1", - // Type: image.KubernetesNodeTypeServer, - // }, - // { - // Hostname: "host2", - // Type: image.KubernetesNodeTypeAgent, - // }, - // }, - // }, - // ExpectedFailedMessages: []string{ - // "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", - // }, - //}, - `no hostname`: { - K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "host1", - Type: image.KubernetesNodeTypeServer, - }, - { - Type: image.KubernetesNodeTypeServer, - }, - }, + Network: image.Network{}, + Nodes: multiNode, }, ExpectedFailedMessages: []string{ - "The 'hostname' field is required for entries in the 'nodes' section.", + "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", }, }, - `missing type`: { + `valid ipv4`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "host1", - Type: image.KubernetesNodeTypeServer, - }, - { - Hostname: "valid", - }, + Network: image.Network{ + APIVIP: "192.168.1.1", }, - }, - ExpectedFailedMessages: []string{ - fmt.Sprintf("The 'type' field for entries in the 'nodes' section must be one of: %s", strings.Join(validNodeTypes, ", ")), + Nodes: multiNode, }, }, - `invalid type`: { + `valid ipv6`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "valid", - Type: image.KubernetesNodeTypeServer, - }, - { - Hostname: "invalid", - Type: "abnormal", - }, + Network: image.Network{ + APIVIP: "fd12:3456:789a::21", }, - }, - ExpectedFailedMessages: []string{ - fmt.Sprintf("The 'type' field for entries in the 'nodes' section must be one of: %s", strings.Join(validNodeTypes, ", ")), + Nodes: multiNode, }, }, - `incorrect initialiser type`: { + `invalid ipv4`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "valid", - Type: image.KubernetesNodeTypeServer, - }, - { - Hostname: "invalid", - Initialiser: true, - Type: image.KubernetesNodeTypeAgent, - }, + Network: image.Network{ + APIVIP: "500.168.1.1", }, + Nodes: multiNode, }, ExpectedFailedMessages: []string{ - fmt.Sprintf("The node labeled with 'initialiser' must be of type '%s'.", image.KubernetesNodeTypeServer), + "Invalid APIVIP address \"500.168.1.1\" for field 'apiVIP'.", + "\"500.168.1.1\" is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", + "Invalid non-unicast cluster API address (500.168.1.1) for field 'apiVIP'.", }, }, - `duplicate entries`: { + `non-unicast ipv4`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "foo", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, - { - Hostname: "bar", - Type: image.KubernetesNodeTypeAgent, - }, - { - Hostname: "bar", - Type: image.KubernetesNodeTypeAgent, - }, - { - Hostname: "foo", - Type: image.KubernetesNodeTypeAgent, - }, + Network: image.Network{ + APIVIP: "127.0.0.1", }, + Nodes: multiNode, }, ExpectedFailedMessages: []string{ - "The 'nodes' section contains duplicate entries: bar, foo", + "Invalid non-unicast cluster API address (127.0.0.1) for field 'apiVIP'.", }, }, - `no server node`: { + `invalid ipv6`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "foo", - Type: image.KubernetesNodeTypeAgent, - }, - { - Hostname: "bar", - Type: image.KubernetesNodeTypeAgent, - }, + Network: image.Network{ + APIVIP: "xxxx:3456:789a::21", }, + Nodes: multiNode, }, ExpectedFailedMessages: []string{ - fmt.Sprintf("There must be at least one node of type '%s' defined.", image.KubernetesNodeTypeServer), + "Invalid APIVIP address \"xxxx:3456:789a::21\" for field 'apiVIP'.", + "\"xxxx:3456:789a::21\" is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", + "Invalid non-unicast cluster API address (xxxx:3456:789a::21) for field 'apiVIP'.", }, }, - `multiple initialisers`: { + `non-unicast ipv6`: { K8s: image.Kubernetes{ - Network: validNetwork, - Nodes: []image.Node{ - { - Hostname: "foo", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, - { - Hostname: "bar", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, + Network: image.Network{ + APIVIP: "ff02::1", }, + Nodes: multiNode, }, ExpectedFailedMessages: []string{ - "Only one node may be specified as the cluster initializer.", + "Invalid non-unicast cluster API address (ff02::1) for field 'apiVIP'.", }, }, } @@ -1269,7 +1162,7 @@ func TestValidateNodes(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { k := test.K8s - failures := validateNodes(&k) + failures := validateNetwork(&k) assert.Len(t, failures, len(test.ExpectedFailedMessages)) var foundMessages []string diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index 0f715511..5e49efad 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -56,22 +56,20 @@ func NewCluster(kubernetes *image.Kubernetes, configPath string) (*Cluster, erro } var ip4 *netip.Addr - if kubernetes.Network.APIVIP4 != "" { - ip4Holder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP4) + var ip6 *netip.Addr + if kubernetes.Network.APIVIP != "" { + ipHolder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP) if ipErr != nil { - return nil, fmt.Errorf("parsing kubernetes ipv4 address: %w", ipErr) + return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", ipErr) } - ip4 = &ip4Holder - } - var ip6 *netip.Addr - if kubernetes.Network.APIVIP6 != "" { - ip6Holder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP6) - if ipErr != nil { - return nil, fmt.Errorf("parsing kubernetes ipv6 address: %w", ipErr) + if ipHolder.Is4() { + ip4 = &ipHolder + } else { + ip6 = &ipHolder } - ip6 = &ip6Holder } + setMultiNodeConfigDefaults(kubernetes, serverConfig, ip4, ip6) agentConfigPath := filepath.Join(configPath, agentConfigFile) @@ -155,14 +153,8 @@ func setSingleNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string if strings.Contains(kubernetes.Version, image.KubernetesDistroRKE2) { setClusterCNI(config) } - if kubernetes.Network.APIVIP4 != "" || kubernetes.Network.APIVIP6 != "" { - if kubernetes.Network.APIVIP4 != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP4) - } - - if kubernetes.Network.APIVIP6 != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP6) - } + if kubernetes.Network.APIVIP != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP) if strings.Contains(kubernetes.Version, image.KubernetesDistroK3S) { appendDisabledServices(config, "servicelb") @@ -189,12 +181,10 @@ func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string] } setClusterToken(config) - if kubernetes.Network.APIVIP4 != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP4) - } - if kubernetes.Network.APIVIP6 != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP6) + if kubernetes.Network.APIVIP != "" { + appendClusterTLSSAN(config, kubernetes.Network.APIVIP) } + setSELinux(config) if kubernetes.Network.APIHost != "" { appendClusterTLSSAN(config, kubernetes.Network.APIHost) diff --git a/pkg/kubernetes/cluster_test.go b/pkg/kubernetes/cluster_test.go index 31d4574b..1b44b172 100644 --- a/pkg/kubernetes/cluster_test.go +++ b/pkg/kubernetes/cluster_test.go @@ -15,7 +15,7 @@ func TestNewCluster_SingleNodeRKE2_MissingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP4: "192.168.122.50", + APIVIP: "192.168.122.50", }, } @@ -40,7 +40,7 @@ func TestNewCluster_SingleNodeK3s_MissingConfig(t *testing.T) { Version: "v1.30.3+k3s1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP4: "192.168.122.50", + APIVIP: "192.168.122.50", }, } @@ -64,7 +64,7 @@ func TestNewCluster_SingleNode_ExistingConfig(t *testing.T) { kubernetes := &image.Kubernetes{ Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP4: "192.168.122.50", + APIVIP: "192.168.122.50", }, } @@ -88,7 +88,7 @@ func TestNewCluster_MultiNodeRKE2_MissingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP4: "192.168.122.50", + APIVIP: "192.168.122.50", }, Nodes: []image.Node{ { @@ -138,7 +138,7 @@ func TestNewCluster_MultiNodeRKE2_ExistingConfig(t *testing.T) { Version: "v1.30.3+rke2r1", Network: image.Network{ APIHost: "api.suse.edge.com", - APIVIP4: "192.168.122.50", + APIVIP: "192.168.122.50", }, Nodes: []image.Node{ { From 9a6cd099a014a6234263c89a4f01316704b3e60b Mon Sep 17 00:00:00 2001 From: dbw7 Date: Fri, 18 Oct 2024 00:00:58 -0400 Subject: [PATCH 04/10] remove unnecessary if --- pkg/kubernetes/cluster.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index 5e49efad..74a58184 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -181,9 +181,7 @@ func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string] } setClusterToken(config) - if kubernetes.Network.APIVIP != "" { - appendClusterTLSSAN(config, kubernetes.Network.APIVIP) - } + appendClusterTLSSAN(config, kubernetes.Network.APIVIP) setSELinux(config) if kubernetes.Network.APIHost != "" { From 090d38517fbf20b3ac83fc5c2eac0d5361ba6abb Mon Sep 17 00:00:00 2001 From: dbw7 Date: Fri, 18 Oct 2024 00:18:03 -0400 Subject: [PATCH 05/10] update cluster_test --- pkg/kubernetes/cluster_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/kubernetes/cluster_test.go b/pkg/kubernetes/cluster_test.go index 1b44b172..1ca21df2 100644 --- a/pkg/kubernetes/cluster_test.go +++ b/pkg/kubernetes/cluster_test.go @@ -279,11 +279,18 @@ func TestSetClusterAPIAddress(t *testing.T) { setClusterAPIAddress(config, nil, nil, 9345) assert.NotContains(t, config, "server") + ip4, err := netip.ParseAddr("192.168.122.50") assert.NoError(t, err) setClusterAPIAddress(config, &ip4, nil, 9345) assert.Equal(t, "https://192.168.122.50:9345", config["server"]) + + ip6, err := netip.ParseAddr("fd12:3456:789a::21") + assert.NoError(t, err) + + setClusterAPIAddress(config, nil, &ip6, 9345) + assert.Equal(t, "https://[fd12:3456:789a::21]:9345", config["server"]) } func TestAppendClusterTLSSAN(t *testing.T) { From d66ad0b5fd9a06bdd2c14e872a5f702bcb6e73f0 Mon Sep 17 00:00:00 2001 From: dbw7 Date: Fri, 18 Oct 2024 13:40:59 -0400 Subject: [PATCH 06/10] feedback updates --- pkg/combustion/kubernetes.go | 2 - pkg/combustion/kubernetes_test.go | 1 - pkg/combustion/templates/k8s-vip.yaml.tpl | 3 +- .../rke2-multi-node-installer.sh.tpl | 1 + pkg/image/validation/kubernetes.go | 47 +++++++++---------- pkg/image/validation/kubernetes_test.go | 28 +---------- pkg/kubernetes/cluster.go | 35 ++++---------- pkg/kubernetes/cluster_test.go | 6 +-- 8 files changed, 39 insertions(+), 84 deletions(-) diff --git a/pkg/combustion/kubernetes.go b/pkg/combustion/kubernetes.go index e7bb82a6..86b0e8d0 100644 --- a/pkg/combustion/kubernetes.go +++ b/pkg/combustion/kubernetes.go @@ -325,12 +325,10 @@ func kubernetesVIPManifest(k *image.Kubernetes) (string, error) { manifest := struct { APIAddress string IsIPV4 bool - IsIPV6 bool RKE2 bool }{ APIAddress: k.Network.APIVIP, IsIPV4: ip.Is4(), - IsIPV6: ip.Is6(), RKE2: strings.Contains(k.Version, image.KubernetesDistroRKE2), } diff --git a/pkg/combustion/kubernetes_test.go b/pkg/combustion/kubernetes_test.go index 020dd5c0..ffe7562d 100644 --- a/pkg/combustion/kubernetes_test.go +++ b/pkg/combustion/kubernetes_test.go @@ -847,7 +847,6 @@ func TestKubernetesVIPManifestValidIPV6(t *testing.T) { manifest, err := kubernetesVIPManifest(k8s) require.NoError(t, err) - fmt.Println(manifest) assert.Contains(t, manifest, "- fd12:3456:789a::21/128") assert.Contains(t, manifest, "- name: k8s-api") diff --git a/pkg/combustion/templates/k8s-vip.yaml.tpl b/pkg/combustion/templates/k8s-vip.yaml.tpl index 7dcc67bf..28c8484c 100644 --- a/pkg/combustion/templates/k8s-vip.yaml.tpl +++ b/pkg/combustion/templates/k8s-vip.yaml.tpl @@ -8,8 +8,7 @@ spec: addresses: {{- if .IsIPV4 }} - {{ .APIAddress }}/32 - {{- end }} - {{- if .IsIPV6 }} + {{- else }} - {{ .APIAddress }}/128 {{- end }} avoidBuggyIPs: true diff --git a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl index 2d633d6e..0efec761 100644 --- a/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl +++ b/pkg/combustion/templates/rke2-multi-node-installer.sh.tpl @@ -94,6 +94,7 @@ fi {{- if and .apiVIP .apiHost }} echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts {{- end }} + mkdir -p /etc/rancher/rke2/ cp $CONFIGFILE /etc/rancher/rke2/config.yaml diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index ec969b0f..4a60fbb7 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -115,41 +115,38 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation { func validateNetwork(k8s *image.Kubernetes) []FailedValidation { var failures []FailedValidation - ip := k8s.Network.APIVIP - numNodes := len(k8s.Nodes) - if numNodes <= 1 { - // Single node cluster, node configurations are not required + if k8s.Network.APIVIP == "" { + failures = append(failures, FailedValidation{ + UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + }) + return failures } - if ip == "" { + parsedIP, err := netip.ParseAddr(k8s.Network.APIVIP) + if err != nil { failures = append(failures, FailedValidation{ - UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + UserMessage: fmt.Sprintf("Invalid address value %q for field 'apiVIP'.", k8s.Network.APIVIP), + Error: err, }) + + return failures } - if ip != "" { - parsedIP, err := netip.ParseAddr(ip) - if err != nil { - failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("Invalid APIVIP address %q for field 'apiVIP'.", ip), - Error: err, - }) - } + if !parsedIP.Is4() && !parsedIP.Is6() { + failures = append(failures, FailedValidation{ + UserMessage: "Only IPv4 and IPv6 addresses are valid values for field 'apiVIP'.", + }) - if !parsedIP.Is4() && !parsedIP.Is6() { - failures = append(failures, FailedValidation{ - UserMessage: fmt.Sprintf("%q is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", ip), - }) - } + return failures + } - if !parsedIP.IsGlobalUnicast() { - msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", ip) - failures = append(failures, FailedValidation{ - UserMessage: msg, - }) - } + if !parsedIP.IsGlobalUnicast() { + msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", k8s.Network.APIVIP) + failures = append(failures, FailedValidation{ + UserMessage: msg, + }) } return failures diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index b9d5c1f7..ed650c9a 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -18,19 +18,6 @@ var validNetwork = image.Network{ APIVIP: "192.168.1.1", } -var multiNode = []image.Node{ - { - Hostname: "foo", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, - { - Hostname: "bar", - Type: image.KubernetesNodeTypeServer, - Initialiser: true, - }, -} - func TestValidateKubernetes(t *testing.T) { configDir, err := os.MkdirTemp("", "eib-config-") require.NoError(t, err) @@ -1087,7 +1074,6 @@ func TestValidateNetwork(t *testing.T) { `no network defined`: { K8s: image.Kubernetes{ Network: image.Network{}, - Nodes: multiNode, }, ExpectedFailedMessages: []string{ "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", @@ -1098,7 +1084,6 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "192.168.1.1", }, - Nodes: multiNode, }, }, `valid ipv6`: { @@ -1106,7 +1091,6 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "fd12:3456:789a::21", }, - Nodes: multiNode, }, }, `invalid ipv4`: { @@ -1114,12 +1098,9 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "500.168.1.1", }, - Nodes: multiNode, }, ExpectedFailedMessages: []string{ - "Invalid APIVIP address \"500.168.1.1\" for field 'apiVIP'.", - "\"500.168.1.1\" is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", - "Invalid non-unicast cluster API address (500.168.1.1) for field 'apiVIP'.", + "Invalid address value \"500.168.1.1\" for field 'apiVIP'.", }, }, `non-unicast ipv4`: { @@ -1127,7 +1108,6 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "127.0.0.1", }, - Nodes: multiNode, }, ExpectedFailedMessages: []string{ "Invalid non-unicast cluster API address (127.0.0.1) for field 'apiVIP'.", @@ -1138,12 +1118,9 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "xxxx:3456:789a::21", }, - Nodes: multiNode, }, ExpectedFailedMessages: []string{ - "Invalid APIVIP address \"xxxx:3456:789a::21\" for field 'apiVIP'.", - "\"xxxx:3456:789a::21\" is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", - "Invalid non-unicast cluster API address (xxxx:3456:789a::21) for field 'apiVIP'.", + "Invalid address value \"xxxx:3456:789a::21\" for field 'apiVIP'.", }, }, `non-unicast ipv6`: { @@ -1151,7 +1128,6 @@ func TestValidateNetwork(t *testing.T) { Network: image.Network{ APIVIP: "ff02::1", }, - Nodes: multiNode, }, ExpectedFailedMessages: []string{ "Invalid non-unicast cluster API address (ff02::1) for field 'apiVIP'.", diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index 74a58184..25a920f4 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -55,22 +55,12 @@ func NewCluster(kubernetes *image.Kubernetes, configPath string) (*Cluster, erro return &Cluster{ServerConfig: serverConfig}, nil } - var ip4 *netip.Addr - var ip6 *netip.Addr - if kubernetes.Network.APIVIP != "" { - ipHolder, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP) - if ipErr != nil { - return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", ipErr) - } - - if ipHolder.Is4() { - ip4 = &ipHolder - } else { - ip6 = &ipHolder - } + ip, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP) + if ipErr != nil { + return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", ipErr) } - setMultiNodeConfigDefaults(kubernetes, serverConfig, ip4, ip6) + setMultiNodeConfigDefaults(kubernetes, serverConfig, &ip) agentConfigPath := filepath.Join(configPath, agentConfigFile) agentConfig, err := ParseKubernetesConfig(agentConfigPath) @@ -166,17 +156,17 @@ func setSingleNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string delete(config, serverKey) } -func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string]any, ip4, ip6 *netip.Addr) { +func setMultiNodeConfigDefaults(kubernetes *image.Kubernetes, config map[string]any, ip *netip.Addr) { const ( k3sServerPort = 6443 rke2ServerPort = 9345 ) if strings.Contains(kubernetes.Version, image.KubernetesDistroRKE2) { - setClusterAPIAddress(config, ip4, ip6, rke2ServerPort) + setClusterAPIAddress(config, ip, rke2ServerPort) setClusterCNI(config) } else { - setClusterAPIAddress(config, ip4, ip6, k3sServerPort) + setClusterAPIAddress(config, ip, k3sServerPort) appendDisabledServices(config, "servicelb") } @@ -213,18 +203,13 @@ func setClusterCNI(config map[string]any) { config[cniKey] = cniDefaultValue } -func setClusterAPIAddress(config map[string]any, ip4, ip6 *netip.Addr, port uint16) { - if ip4 == nil && ip6 == nil { +func setClusterAPIAddress(config map[string]any, ip *netip.Addr, port uint16) { + if ip == nil { zap.S().Warn("Attempted to set an empty cluster API address") return } - switch { - case ip4 != nil: - config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip4, port).String()) - case ip6 != nil: - config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip6, port).String()) - } + config[serverKey] = fmt.Sprintf("https://%s", netip.AddrPortFrom(*ip, port).String()) } func setSELinux(config map[string]any) { diff --git a/pkg/kubernetes/cluster_test.go b/pkg/kubernetes/cluster_test.go index 1ca21df2..debb2ef7 100644 --- a/pkg/kubernetes/cluster_test.go +++ b/pkg/kubernetes/cluster_test.go @@ -277,19 +277,19 @@ func TestIdentifyInitialiserNode(t *testing.T) { func TestSetClusterAPIAddress(t *testing.T) { config := map[string]any{} - setClusterAPIAddress(config, nil, nil, 9345) + setClusterAPIAddress(config, nil, 9345) assert.NotContains(t, config, "server") ip4, err := netip.ParseAddr("192.168.122.50") assert.NoError(t, err) - setClusterAPIAddress(config, &ip4, nil, 9345) + setClusterAPIAddress(config, &ip4, 9345) assert.Equal(t, "https://192.168.122.50:9345", config["server"]) ip6, err := netip.ParseAddr("fd12:3456:789a::21") assert.NoError(t, err) - setClusterAPIAddress(config, nil, &ip6, 9345) + setClusterAPIAddress(config, &ip6, 9345) assert.Equal(t, "https://[fd12:3456:789a::21]:9345", config["server"]) } From 38dc6b7ddb2b126dd82fbfa9111f11c36952bf03 Mon Sep 17 00:00:00 2001 From: dbw7 Date: Fri, 18 Oct 2024 15:16:09 -0400 Subject: [PATCH 07/10] add release notes and doc --- RELEASE_NOTES.md | 2 ++ docs/building-images.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 91595d7b..43dc95d8 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -4,6 +4,8 @@ ## General +* Added support for IPV6 addresses in the Kubernetes 'apiVIP' field + ## API ### Image Definition Changes diff --git a/docs/building-images.md b/docs/building-images.md index 23690ea1..f1ca374f 100644 --- a/docs/building-images.md +++ b/docs/building-images.md @@ -261,7 +261,7 @@ kubernetes: * `network` - Required for multi-node clusters, optional for single-node clusters; Defines the network configuration for bootstrapping a cluster. * `apiVIP` - Required for multi-node clusters, optional for single-node clusters; Specifies the IP address which - will serve as the cluster LoadBalancer, backed by MetalLB. + will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPV4 and IPV6 addresses. * `apiHost` - Optional; Specifies the domain address for accessing the cluster. * `nodes` - Required for multi-node clusters; Defines a list of all nodes that form the cluster. * `hostname` - Required; Indicates the fully qualified domain name (FQDN) to identify the particular node on which From cdca128a8bdc2d6c88e9e7214e0a4bd3d0b799c4 Mon Sep 17 00:00:00 2001 From: dbw7 Date: Fri, 18 Oct 2024 15:32:41 -0400 Subject: [PATCH 08/10] fix validation bug --- pkg/image/validation/kubernetes.go | 9 ++++++--- pkg/image/validation/kubernetes_test.go | 14 +++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index 4a60fbb7..c469a245 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -117,9 +117,12 @@ func validateNetwork(k8s *image.Kubernetes) []FailedValidation { var failures []FailedValidation if k8s.Network.APIVIP == "" { - failures = append(failures, FailedValidation{ - UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", - }) + if len(k8s.Nodes) >= 1 { + failures = append(failures, FailedValidation{ + UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + }) + return failures + } return failures } diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index ed650c9a..58313297 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -1071,10 +1071,22 @@ func TestValidateNetwork(t *testing.T) { K8s image.Kubernetes ExpectedFailedMessages []string }{ - `no network defined`: { + `no network defined, no nodes defined`: { K8s: image.Kubernetes{ Network: image.Network{}, }, + }, + `no network defined, nodes defined`: { + K8s: image.Kubernetes{ + Network: image.Network{}, + Nodes: []image.Node{ + { + Hostname: "node1", + Type: "server", + Initialiser: false, + }, + }, + }, ExpectedFailedMessages: []string{ "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", }, From 7582178f2c91040391a3114e0e105bf2c4f592de Mon Sep 17 00:00:00 2001 From: dbw7 Date: Mon, 21 Oct 2024 09:20:28 -0400 Subject: [PATCH 09/10] feedback updates --- pkg/image/validation/kubernetes.go | 5 ++--- pkg/image/validation/kubernetes_test.go | 7 ++++++- pkg/kubernetes/cluster.go | 6 +++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/image/validation/kubernetes.go b/pkg/image/validation/kubernetes.go index c469a245..8a01b5b4 100644 --- a/pkg/image/validation/kubernetes.go +++ b/pkg/image/validation/kubernetes.go @@ -117,11 +117,10 @@ func validateNetwork(k8s *image.Kubernetes) []FailedValidation { var failures []FailedValidation if k8s.Network.APIVIP == "" { - if len(k8s.Nodes) >= 1 { + if len(k8s.Nodes) > 1 { failures = append(failures, FailedValidation{ - UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + UserMessage: "The 'apiVIP' field is required in the 'network' section for multi node clusters.", }) - return failures } return failures diff --git a/pkg/image/validation/kubernetes_test.go b/pkg/image/validation/kubernetes_test.go index 58313297..b2e0a691 100644 --- a/pkg/image/validation/kubernetes_test.go +++ b/pkg/image/validation/kubernetes_test.go @@ -1085,10 +1085,15 @@ func TestValidateNetwork(t *testing.T) { Type: "server", Initialiser: false, }, + { + Hostname: "node2", + Type: "server", + Initialiser: false, + }, }, }, ExpectedFailedMessages: []string{ - "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.", + "The 'apiVIP' field is required in the 'network' section for multi node clusters.", }, }, `valid ipv4`: { diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index 25a920f4..d0bc6b3a 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -55,9 +55,9 @@ func NewCluster(kubernetes *image.Kubernetes, configPath string) (*Cluster, erro return &Cluster{ServerConfig: serverConfig}, nil } - ip, ipErr := netip.ParseAddr(kubernetes.Network.APIVIP) - if ipErr != nil { - return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", ipErr) + ip, err := netip.ParseAddr(kubernetes.Network.APIVIP) + if err != nil { + return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", err) } setMultiNodeConfigDefaults(kubernetes, serverConfig, &ip) From 86843c276540c6f0140a24f901e2c50a8744aaed Mon Sep 17 00:00:00 2001 From: dbw7 Date: Mon, 21 Oct 2024 10:36:30 -0400 Subject: [PATCH 10/10] feedback updates --- RELEASE_NOTES.md | 2 +- docs/building-images.md | 2 +- pkg/combustion/kubernetes.go | 2 +- pkg/combustion/kubernetes_test.go | 2 +- pkg/kubernetes/cluster.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 43dc95d8..c87487f3 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -4,7 +4,7 @@ ## General -* Added support for IPV6 addresses in the Kubernetes 'apiVIP' field +* Added support for IPv6 addresses in the Kubernetes 'apiVIP' field ## API diff --git a/docs/building-images.md b/docs/building-images.md index f1ca374f..71600f56 100644 --- a/docs/building-images.md +++ b/docs/building-images.md @@ -261,7 +261,7 @@ kubernetes: * `network` - Required for multi-node clusters, optional for single-node clusters; Defines the network configuration for bootstrapping a cluster. * `apiVIP` - Required for multi-node clusters, optional for single-node clusters; Specifies the IP address which - will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPV4 and IPV6 addresses. + will serve as the cluster LoadBalancer, backed by MetalLB. Supports IPv4 and IPv6 addresses. * `apiHost` - Optional; Specifies the domain address for accessing the cluster. * `nodes` - Required for multi-node clusters; Defines a list of all nodes that form the cluster. * `hostname` - Required; Indicates the fully qualified domain name (FQDN) to identify the particular node on which diff --git a/pkg/combustion/kubernetes.go b/pkg/combustion/kubernetes.go index 86b0e8d0..da48f045 100644 --- a/pkg/combustion/kubernetes.go +++ b/pkg/combustion/kubernetes.go @@ -319,7 +319,7 @@ func (c *Combustion) downloadRKE2Artefacts(ctx *image.Context, cluster *kubernet func kubernetesVIPManifest(k *image.Kubernetes) (string, error) { ip, err := netip.ParseAddr(k.Network.APIVIP) if err != nil { - return "", fmt.Errorf("parsing kubernetes APIVIP address: %w", err) + return "", fmt.Errorf("parsing kubernetes apiVIP address: %w", err) } manifest := struct { diff --git a/pkg/combustion/kubernetes_test.go b/pkg/combustion/kubernetes_test.go index ffe7562d..f0412642 100644 --- a/pkg/combustion/kubernetes_test.go +++ b/pkg/combustion/kubernetes_test.go @@ -862,5 +862,5 @@ func TestKubernetesVIPManifestInvalidIP(t *testing.T) { } _, err := kubernetesVIPManifest(k8s) - require.ErrorContains(t, err, "parsing kubernetes APIVIP address: ParseAddr(\"1111\"): unable to parse IP") + require.ErrorContains(t, err, "parsing kubernetes apiVIP address: ParseAddr(\"1111\"): unable to parse IP") } diff --git a/pkg/kubernetes/cluster.go b/pkg/kubernetes/cluster.go index d0bc6b3a..2b5eaf57 100644 --- a/pkg/kubernetes/cluster.go +++ b/pkg/kubernetes/cluster.go @@ -57,7 +57,7 @@ func NewCluster(kubernetes *image.Kubernetes, configPath string) (*Cluster, erro ip, err := netip.ParseAddr(kubernetes.Network.APIVIP) if err != nil { - return nil, fmt.Errorf("parsing kubernetes APIVIP address: %w", err) + return nil, fmt.Errorf("parsing kubernetes apiVIP address: %w", err) } setMultiNodeConfigDefaults(kubernetes, serverConfig, &ip)