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

Validate pod CIDR and service CIDR #695

Open
wants to merge 9 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
61 changes: 61 additions & 0 deletions src/k8s/cmd/k8s/k8s_bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"bytes"
"fmt"
"io"
"net"
"os"
"slices"
"strings"
Expand Down Expand Up @@ -127,6 +128,14 @@ func newBootstrapCmd(env cmdutil.ExecutionEnvironment) *cobra.Command {
}
}

if bootstrapConfig.PodCIDR != nil && bootstrapConfig.ServiceCIDR != nil {
eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
if err = validateCIDROverlapAndSize(*bootstrapConfig.PodCIDR, *bootstrapConfig.ServiceCIDR); err != nil {
cmd.PrintErrf("Error: Failed to validate the CIDR configuration.\n\nThe error was: %v\n", err)
env.Exit(1)
return
}
}

eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
cmd.PrintErrln("Bootstrapping the cluster. This may take a few seconds, please wait.")

response, err := client.BootstrapCluster(cmd.Context(), apiv1.BootstrapClusterRequest{
Expand Down Expand Up @@ -269,3 +278,55 @@ func askQuestion(stdin io.Reader, stdout io.Writer, stderr io.Writer, question s
return s
}
}

// validateCIDROverlapAndSize checks for overlap and size constraints between pod and service CIDRs.
// It parses the provided podCIDR and serviceCIDR strings, checks for IPv4 and IPv6 overlaps, and ensures
// that the service IPv6 CIDR does not have a prefix length of 64 or more.
func validateCIDROverlapAndSize(podCIDR string, serviceCIDR string) error {
// Parse the CIDRs
podIPv4CIDR, podIPv6CIDR, err := utils.ParseCIDRs(podCIDR)
if err != nil {
return err
eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
}

svcIPv4CIDR, svcIPv6CIDR, err := utils.ParseCIDRs(serviceCIDR)
if err != nil {
return err
}

// Check for IPv4 overlap
if podIPv4CIDR != "" && svcIPv4CIDR != "" {
if overlap, err := utils.CIDRsOverlap(podIPv4CIDR, svcIPv4CIDR); err != nil {
return err
} else if overlap {
return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR)
}
}

// Check for IPv6 overlap
if podIPv6CIDR != "" && svcIPv6CIDR != "" {
if overlap, err := utils.CIDRsOverlap(podIPv6CIDR, svcIPv6CIDR); err != nil {
return err
} else if overlap {
return fmt.Errorf("pod CIDR %q and service CIDR %q overlap", podCIDR, serviceCIDR)
}
}

// Check CIDR size
// Ref: https://documentation.ubuntu.com/canonical-kubernetes/latest/snap/howto/networking/dualstack/#cidr-size-limitations
Copy link
Contributor

Choose a reason for hiding this comment

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

/64 is too large for the Service CIDR: Using a /64 CIDR for services may cause issues like failure to initialize the IPv6 allocator.

This makes me feel that the problem might not only be with the IPv6 but that IPv6 problem is just an example, WDYT? Do we need to change the docs or check for IPv4 here as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a limitation of the Kubernetes controller. I understand your point, but IMHO we should describe the limitation for a user (-> use smaller CIDR size) as they normally don't care about the internals of some Kubernetes controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did some tests with @berkayoz and we will be going with /108. Cluster fails to bootstrap at /64 and /96.

if svcIPv6CIDR != "" {
_, ipv6Net, err := net.ParseCIDR(svcIPv6CIDR)
if err != nil {
// Should not happen, as we already parsed the CIDR
return fmt.Errorf("invalid service CIDR %q: %w", svcIPv6CIDR, err)
}

prefixLength, _ := ipv6Net.Mask.Size()

if prefixLength >= 64 {
return fmt.Errorf("service CIDR %q cannot have a prefix length of 64 or more", svcIPv6CIDR)
}
}

return nil
}
36 changes: 36 additions & 0 deletions src/k8s/cmd/k8s/k8s_bootstrap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,3 +155,39 @@ func TestGetConfigFromYaml_Stdin(t *testing.T) {
expectedConfig := apiv1.BootstrapConfig{SecurePort: utils.Pointer(5000)}
g.Expect(config).To(Equal(expectedConfig))
}

func TestValidateCIDROverlapAndSize(t *testing.T) {
tests := []struct {
name string
podCIDR string
serviceCIDR string
expectErr bool
}{
{"Empty", "", "", true},
{"EmptyServiceCIDR", "192.168.1.0/24", "", true},
{"EmptyPodCIDR", "", "192.168.1.0/24", true},
//IPv4
{"SameIPv4CIDRs", "192.168.100.0/24", "192.168.100.0/24", true},
eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
{"OverlappingIPv4CIDRs", "10.2.0.13/24", "10.2.0.0/24", true},
{"IPv4CIDRMinimumSize", "192.168.1.1/32", "10.0.0.0/24", false},
{"InvalidIPv4CIDRFormat", "192.168.1.1/33", "10.0.0.0/24", true},
{"MaxSizeIPv4CIDRs", "0.0.0.0/0", "0.0.0.0/0", true},
//IPv6
{"SameIPv6CIDRs", "fe80::1/32", "fe80::1/32", true},
{"OverlappingIPv6CIDRs", "fe80::/48", "fe80::dead/48", true},
{"IPv6CIDRMinimumSize", "2001:db8::1/128", "fc00::/32", false},
{"IPv6CIDRPrefixAtLimit", "192.168.1.0/24", "192.168.2.0/24,fe80::/64", true},
{"IPv6CIDRPrefixGreaterThanLimit", "192.168.1.0/24", "fe80::/68", true},
{"InvalidIPv6CIDRFormat", "2001:db8::1/129", "fc00::/64", true},
{"MaxSizeIPv6CIDRs", "::/0", "::/0", true},
//Mixed
{"IPv4AndIPv6MixedCIDRs", "192.168.0.0/16", "2001:db8::/32", false},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if err := validateCIDROverlapAndSize(tc.podCIDR, tc.serviceCIDR); (err != nil) != tc.expectErr {
t.Errorf("validateCIDROverlapAndSize() error = %v, expectErr %v", err, tc.expectErr)
}
})
}
}
18 changes: 18 additions & 0 deletions src/k8s/pkg/utils/cidr.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,21 @@ func ToIPString(ip net.IP) string {
}
return "[" + ip.String() + "]"
}

// CIDRsOverlap checks if two given CIDR blocks overlap.
// It takes two strings representing the CIDR blocks as input and returns a boolean indicating
// whether they overlap and an error if any of the CIDR blocks are invalid.
func CIDRsOverlap(cidr1, cidr2 string) (bool, error) {
eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
_, ipNet1, err1 := net.ParseCIDR(cidr1)
_, ipNet2, err2 := net.ParseCIDR(cidr2)

if err1 != nil || err2 != nil {
return false, fmt.Errorf("invalid CIDR blocks, %v, %v", err1, err2)
eaudetcobello marked this conversation as resolved.
Show resolved Hide resolved
}

if ipNet1.Contains(ipNet2.IP) || ipNet2.Contains(ipNet1.IP) {
return true, nil
}

return false, nil
}
Loading