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.

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

IPV6 APIVIP Handling #589

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
10 changes: 10 additions & 0 deletions pkg/combustion/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package combustion
import (
_ "embed"
"fmt"
"net/netip"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -316,11 +317,20 @@ 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should read as apiVIP instead of all caps, since that's what the field is named in the definition. But I don't feel particularly strongly if you decide to keep it like this.

}

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),
}

Expand Down
44 changes: 44 additions & 0 deletions pkg/combustion/kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

@mchiappero mchiappero Oct 24, 2024

Choose a reason for hiding this comment

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

Nit-picking, but I'd align these two names from "IPV4"/"IPV6" to IPv4 too, as the rest of the code/prints.

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)
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved

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")
}
7 changes: 6 additions & 1 deletion pkg/combustion/templates/k8s-vip.yaml.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ metadata:
namespace: metallb-system
spec:
addresses:
- {{ .APIAddress }}/32
{{- if .IsIPV4 }}
- {{ .APIAddress }}/32
{{- end }}
{{- if .IsIPV6 }}
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
- {{ .APIAddress }}/128
{{- end }}
avoidBuggyIPs: true
serviceAllocation:
namespaces:
Expand Down
3 changes: 1 addition & 2 deletions pkg/combustion/templates/rke2-multi-node-installer.sh.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,9 @@ systemctl enable kubernetes-resources-install.service
{{- end }}
fi

{{- if .apiHost }}
{{- if and .apiVIP .apiHost }}
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
echo "{{ .apiVIP }} {{ .apiHost }}" >> /etc/hosts
{{- end }}

atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
mkdir -p /etc/rancher/rke2/
cp $CONFIGFILE /etc/rancher/rke2/config.yaml

Expand Down
50 changes: 44 additions & 6 deletions pkg/image/validation/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package validation
import (
"errors"
"fmt"
"net/netip"
"net/url"
"os"
"path/filepath"
Expand Down Expand Up @@ -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))...)
Expand All @@ -52,12 +54,6 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation {
return failures
}

if k8s.Network.APIVIP == "" {
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
Expand Down Expand Up @@ -117,6 +113,48 @@ func validateNodes(k8s *image.Kubernetes) []FailedValidation {
return failures
}

func validateNetwork(k8s *image.Kubernetes) []FailedValidation {
var failures []FailedValidation
ip := k8s.Network.APIVIP
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved

numNodes := len(k8s.Nodes)
if numNodes <= 1 {
// Single node cluster, node configurations are not required
return failures
}

if ip == "" {
failures = append(failures, FailedValidation{
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
UserMessage: "The 'apiVIP' field is required in the 'network' section when defining entries under 'nodes'.",
})
}

if ip != "" {
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
parsedIP, err := netip.ParseAddr(ip)
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
failures = append(failures, FailedValidation{
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
UserMessage: fmt.Sprintf("Invalid APIVIP address %q for field 'apiVIP'.", ip),
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
Error: err,
})
}

if !parsedIP.Is4() && !parsedIP.Is6() {
failures = append(failures, FailedValidation{
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
UserMessage: fmt.Sprintf("%q is not an IPV4 or IPV6 address, only IPV4 and IPV6 addresses are valid for field 'apiVIP'.", ip),
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
})
}

if !parsedIP.IsGlobalUnicast() {
msg := fmt.Sprintf("Invalid non-unicast cluster API address (%s) for field 'apiVIP'.", ip)
failures = append(failures, FailedValidation{
UserMessage: msg,
})
}
}

return failures
}

func validateManifestURLs(k8s *image.Kubernetes) []FailedValidation {
var failures []FailedValidation

Expand Down
138 changes: 118 additions & 20 deletions pkg/image/validation/kubernetes_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,20 @@ import (

var validNetwork = image.Network{
APIHost: "host.com",
APIVIP: "127.0.0.1",
APIVIP: "192.168.1.1",
}

var multiNode = []image.Node{
atanasdinov marked this conversation as resolved.
Show resolved Hide resolved
{
Hostname: "foo",
Type: image.KubernetesNodeTypeServer,
Initialiser: true,
},
{
Hostname: "bar",
Type: image.KubernetesNodeTypeServer,
Initialiser: true,
},
}

func TestValidateKubernetes(t *testing.T) {
Expand Down Expand Up @@ -78,7 +91,10 @@ func TestValidateKubernetes(t *testing.T) {
`failures all sections`: {
K8s: image.Kubernetes{
Version: "v1.30.3",
Network: validNetwork,
Network: image.Network{
APIHost: "host.com",
APIVIP: "127.0.0.1",
},
Nodes: []image.Node{
{
Type: image.KubernetesNodeTypeServer,
Expand Down Expand Up @@ -116,6 +132,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'.",
},
},
}
Expand Down Expand Up @@ -185,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,
Expand Down Expand Up @@ -1079,3 +1078,102 @@ func TestValidateAdditionalArtifacts(t *testing.T) {
})
}
}

func TestValidateNetwork(t *testing.T) {
Copy link

@mchiappero mchiappero Oct 24, 2024

Choose a reason for hiding this comment

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

Hey Danial, glad to see that most of my code has been reused :)
I saw you re-wrote and simplified this function, which on the one hand is good, but ends having a much smaller coverage. The one I wrote was carefully calibrated around the IsGlobalUnicast() check (and internal sub-checks), so that the different cases of non-global unicast values were sufficiently tested. I think it make sense to include a couple of additional malformed IPs (e.g. 500.168.1.1), but I'd bring back the cases from my initial work as it's just a matter of copying and pasting.

tests := map[string]struct {
K8s image.Kubernetes
ExpectedFailedMessages []string
}{
`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'.",
},
},
`valid ipv4`: {
K8s: image.Kubernetes{
Network: image.Network{
APIVIP: "192.168.1.1",
},
Nodes: multiNode,
},
},
`valid ipv6`: {
K8s: image.Kubernetes{
Network: image.Network{
APIVIP: "fd12:3456:789a::21",
},
Nodes: multiNode,
},
},
`invalid ipv4`: {
K8s: image.Kubernetes{
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'.",
},
},
`non-unicast ipv4`: {
K8s: image.Kubernetes{
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'.",
},
},
`invalid ipv6`: {
K8s: image.Kubernetes{
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'.",
},
},
`non-unicast ipv6`: {
K8s: image.Kubernetes{
Network: image.Network{
APIVIP: "ff02::1",
},
Nodes: multiNode,
},
ExpectedFailedMessages: []string{
"Invalid non-unicast cluster API address (ff02::1) for field 'apiVIP'.",
},
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
k := test.K8s
failures := validateNetwork(&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)
}

})
}
}
Loading