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

kubeadm: skip ipv4 check if the cluster is using IPv6 address #115420

Merged
merged 1 commit into from Feb 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
73 changes: 61 additions & 12 deletions cmd/kubeadm/app/preflight/checks.go
Expand Up @@ -883,15 +883,11 @@ func (MemCheck) Name() string {
return "Mem"
}

// RunInitNodeChecks executes all individual, applicable to control-plane node checks.
// The boolean flag 'isSecondaryControlPlane' controls whether we are running checks in a --join-control-plane scenario.
// The boolean flag 'downloadCerts' controls whether we should skip checks on certificates because we are downloading them.
// If the flag is set to true we should skip checks already executed by RunJoinNodeChecks.
func RunInitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguration, ignorePreflightErrors sets.Set[string], isSecondaryControlPlane bool, downloadCerts bool) error {
func InitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguration, ignorePreflightErrors sets.Set[string], isSecondaryControlPlane bool, downloadCerts bool) ([]Checker, error) {
if !isSecondaryControlPlane {
// First, check if we're root separately from the other preflight checks and fail fast
if err := RunRootCheckOnly(ignorePreflightErrors); err != nil {
return err
return nil, err
}
}

Expand All @@ -912,23 +908,56 @@ func RunInitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfigura
FileAvailableCheck{Path: kubeadmconstants.GetStaticPodFilepath(kubeadmconstants.Etcd, manifestsDir)},
HTTPProxyCheck{Proto: "https", Host: cfg.LocalAPIEndpoint.AdvertiseAddress},
}

// File content check for IPV4 and IPV6 are needed if it is:
// (dual stack) `--service-cidr` or `--pod-network-cidr` is set with an IPV4 and IPV6 CIDR, `--apiserver-advertise-address` is optional as it can be auto detected.
// (single stack) which is decided by the `--apiserver-advertise-address`.
// Note that for the case of dual stack, user might only give IPV6 CIDR for `--service-cidr` and leave the `--apiserver-advertise-address` a default value which will be
// auto detected and properly bound to an IPV4 address, this will make the cluster non-functional eventually. The case like this should be avoided by the validation instead,
// i.e. We don't care whether the input values for those parameters are set correctly here but if it's an IPV4 scoped CIDR or address we will add the file content check for IPV4,
// as does the IPV6.
IPV4Check := false
IPV6Check := false
cidrs := strings.Split(cfg.Networking.ServiceSubnet, ",")
for _, cidr := range cidrs {
checks = append(checks, HTTPProxyCIDRCheck{Proto: "https", CIDR: cidr})
if !IPV4Check && netutils.IsIPv4CIDRString(cidr) {
IPV4Check = true
}
if !IPV6Check && netutils.IsIPv6CIDRString(cidr) {
IPV6Check = true
}

}
cidrs = strings.Split(cfg.Networking.PodSubnet, ",")
for _, cidr := range cidrs {
checks = append(checks, HTTPProxyCIDRCheck{Proto: "https", CIDR: cidr})
if !IPV4Check && netutils.IsIPv4CIDRString(cidr) {
IPV4Check = true
}
if !IPV6Check && netutils.IsIPv6CIDRString(cidr) {
IPV6Check = true
}
}

if !isSecondaryControlPlane {
checks = addCommonChecks(execer, cfg.KubernetesVersion, &cfg.NodeRegistration, checks)

// Check if Bridge-netfilter and IPv6 relevant flags are set
if ip := netutils.ParseIPSloppy(cfg.LocalAPIEndpoint.AdvertiseAddress); ip != nil {
if netutils.IsIPv6(ip) {
checks = addIPv6Checks(checks)
if !IPV4Check && netutils.IsIPv4(ip) {
IPV4Check = true
}
if !IPV6Check && netutils.IsIPv6(ip) {
IPV6Check = true
}
}

if IPV4Check {
checks = addIPv4Checks(checks)
}
if IPV6Check {
checks = addIPv6Checks(checks)
}

// if using an external etcd
Expand Down Expand Up @@ -959,15 +988,25 @@ func RunInitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfigura
checks = append(checks, FileExistingCheck{Path: cfg.Etcd.External.KeyFile, Label: "ExternalEtcdClientCertificates"})
}
}
return checks, nil
}

// RunInitNodeChecks executes all individual, applicable to control-plane node checks.
// The boolean flag 'isSecondaryControlPlane' controls whether we are running checks in a --join-control-plane scenario.
// The boolean flag 'downloadCerts' controls whether we should skip checks on certificates because we are downloading them.
// If the flag is set to true we should skip checks already executed by RunJoinNodeChecks.
func RunInitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguration, ignorePreflightErrors sets.Set[string], isSecondaryControlPlane bool, downloadCerts bool) error {
checks, err := InitNodeChecks(execer, cfg, ignorePreflightErrors, isSecondaryControlPlane, downloadCerts)
if err != nil {
return err
}
return RunChecks(checks, os.Stderr, ignorePreflightErrors)
}

// RunJoinNodeChecks executes all individual, applicable to node checks.
func RunJoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.JoinConfiguration, ignorePreflightErrors sets.Set[string]) error {
func JoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.JoinConfiguration, ignorePreflightErrors sets.Set[string]) ([]Checker, error) {
// First, check if we're root separately from the other preflight checks and fail fast
if err := RunRootCheckOnly(ignorePreflightErrors); err != nil {
return err
return nil, err
}

checks := []Checker{
Expand All @@ -986,13 +1025,24 @@ func RunJoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.JoinConfigura
HTTPProxyCheck{Proto: "https", Host: ipstr},
)
if ip := netutils.ParseIPSloppy(ipstr); ip != nil {
if netutils.IsIPv4(ip) {
checks = addIPv4Checks(checks)
}
Comment on lines +1028 to +1030
Copy link
Member

@SataQiu SataQiu Jan 31, 2023

Choose a reason for hiding this comment

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

Emm... How about DualStack? We need to check both IPv4 and IPv6.
Maybe we need a way to check if it's SingleStack v6, SingleStack v4 or DualStack.

Copy link

Choose a reason for hiding this comment

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

The current logic should take care of it. as the checks get appended and they don't overwrite.
So if you have IPv4 single stack "IsIPv4" is true and "IsIPv6" is false. So you only get the IPv4 checks.
If you have IPv6 single stack "IsIPv4" is false and "IsIPv6" is true. So you only get the IPv6 checks.
If you have dualstack "IsIPv4" is true and the IPv4 checks are added. But also "IsIPv6" is true and the IPv6 checks are added as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks @agowa338 for the comments, I think @SataQiu 's comments is valid when it comes to the case of dual-stack, as it originally checks the cfg.LocalAPIEndpoint.AdvertiseAddress only which is not enough.

Comment on lines +1028 to +1030
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we can only do IPv4 check or IPv6 check for RunJoinNodeChecks, since the APIServerEndpoint can only be an IPv4 or IPv6 IP. How about dual stack check for RunJoinNodeChecks? Can we do it just like RunInitNodeChecks?

I +1 for this feature, but it seems we need to do more :)
And we need some e2e tests for this feature.

/approve
/hold
free to cancel hold if needed

if netutils.IsIPv6(ip) {
checks = addIPv6Checks(checks)
}
}
}
}
return checks, nil
}

// RunJoinNodeChecks executes all individual, applicable to node checks.
func RunJoinNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.JoinConfiguration, ignorePreflightErrors sets.Set[string]) error {
checks, err := JoinNodeChecks(execer, cfg, ignorePreflightErrors)
if err != nil {
return err
}
return RunChecks(checks, os.Stderr, ignorePreflightErrors)
}

Expand All @@ -1007,7 +1057,6 @@ func addCommonChecks(execer utilsexec.Interface, k8sVersion string, nodeReg *kub
}

// non-windows checks
checks = addIPv4Checks(checks)
checks = addSwapCheck(checks)
checks = addExecChecks(checks, execer)
checks = append(checks,
Expand Down
134 changes: 134 additions & 0 deletions cmd/kubeadm/app/preflight/checks_test.go
Expand Up @@ -26,6 +26,7 @@ import (
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/lithammer/dedent"
"github.com/pkg/errors"

Expand All @@ -36,7 +37,9 @@ import (
fakeexec "k8s.io/utils/exec/testing"

kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmapiv1 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta3"
"k8s.io/kubernetes/cmd/kubeadm/app/constants"
configutil "k8s.io/kubernetes/cmd/kubeadm/app/util/config"
utilruntime "k8s.io/kubernetes/cmd/kubeadm/app/util/runtime"
)

Expand Down Expand Up @@ -1005,3 +1008,134 @@ func TestMemCheck(t *testing.T) {
})
}
}

func TestInitIPCheck(t *testing.T) {
// skip this test, if OS in not Linux, since it will ONLY pass on Linux.
if runtime.GOOS != "linux" {
t.Skip("unsupported OS")
}
// should be a privileged user for the `init` command, otherwise just skip it.
isPrivileged := IsPrivilegedUserCheck{}
if _, err := isPrivileged.Check(); err != nil {
t.Skip("not a privileged user")
}
internalcfg, err := configutil.DefaultedStaticInitConfiguration()
if err != nil {
t.Fatalf("unexpected failure when defaulting InitConfiguration: %v", err)
}
internalcfg.LocalAPIEndpoint.AdvertiseAddress = "" // AdvertiseAddress is optional, it could be auto-detected.
ipv4File := "FileContent--proc-sys-net-ipv4-ip_forward"
ipv6File := "FileContent--proc-sys-net-ipv6-conf-default-forwarding"
var tests = []struct {
testName string
PodSubnet string
serviceCidr string
expStr []string
}{
{
testName: "dual stack",
PodSubnet: "fda9:d324:354d:0::/56",
serviceCidr: "10.96.0.0/16,fda9:d324:354d:1::/112",
expStr: []string{"FileContent--proc-sys-net-ipv4-ip_forward", "FileContent--proc-sys-net-ipv6-conf-default-forwarding"},
},
{
testName: "single stack ipv4",
PodSubnet: "10.244.0.0/16",
serviceCidr: "10.96.0.0/16",
expStr: []string{"FileContent--proc-sys-net-ipv4-ip_forward"},
},
{
testName: "single stack ipv6",
PodSubnet: "fda9:d324:354d:0::/56",
serviceCidr: "fda9:d324:354d:1::/112",
expStr: []string{"FileContent--proc-sys-net-ipv6-conf-default-forwarding"},
},
}

for _, rt := range tests {
t.Run(rt.testName, func(t *testing.T) {
checkList := []string{}
internalcfg.Networking.ServiceSubnet = rt.serviceCidr
internalcfg.Networking.PodSubnet = rt.PodSubnet
checks, err := InitNodeChecks(exec.New(), internalcfg, nil, false, false)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
for _, check := range checks {
if check.Name() == ipv4File {
checkList = append(checkList, ipv4File)
}
if check.Name() == ipv6File {
checkList = append(checkList, ipv6File)
}
}
if diff := cmp.Diff(checkList, rt.expStr); diff != "" {
t.Fatalf("unexpected file content check (-want,+got):\n%s", diff)
}
})
}
}

func TestJoinIPCheck(t *testing.T) {
// skip this test, if OS in not Linux, since it will ONLY pass on Linux.
if runtime.GOOS != "linux" {
t.Skip("unsupported OS")
}
// should be a privileged user for the `join` command, otherwise just skip it.
isPrivileged := IsPrivilegedUserCheck{}
if _, err := isPrivileged.Check(); err != nil {
t.Skip("not a privileged user")
}
internalcfg, err := configutil.DefaultedJoinConfiguration(&kubeadmapiv1.JoinConfiguration{
Discovery: kubeadmapiv1.Discovery{
BootstrapToken: &kubeadmapiv1.BootstrapTokenDiscovery{
Token: configutil.PlaceholderToken.Token.String(),
APIServerEndpoint: "kube-apiserver:6443",
UnsafeSkipCAVerification: true,
},
},
})
if err != nil {
t.Fatalf("unexpected failure when defaulting JoinConfiguration: %v", err)
}
ipv4File := "FileContent--proc-sys-net-ipv4-ip_forward"
ipv6File := "FileContent--proc-sys-net-ipv6-conf-default-forwarding"
var tests = []struct {
testName string
endpoint string
expStr []string
}{
{
testName: "single stack ipv4",
endpoint: "10.244.0.0:1234",
expStr: []string{"FileContent--proc-sys-net-ipv4-ip_forward"},
},
{
testName: "single stack ipv6",
endpoint: "[fda9:d324:354d:0::]:1234",
expStr: []string{"FileContent--proc-sys-net-ipv6-conf-default-forwarding"},
},
}

for _, rt := range tests {
t.Run(rt.testName, func(t *testing.T) {
checkList := []string{}
internalcfg.Discovery.BootstrapToken.APIServerEndpoint = rt.endpoint
checks, err := JoinNodeChecks(exec.New(), internalcfg, nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
for _, check := range checks {
if check.Name() == ipv4File {
checkList = append(checkList, ipv4File)
}
if check.Name() == ipv6File {
checkList = append(checkList, ipv6File)
}
}
if diff := cmp.Diff(checkList, rt.expStr); diff != "" {
t.Fatalf("unexpected file content check (-want,+got):\n%s", diff)
}
})
}
}