fix: kube-vip admin.conf pre/postKubeadmCommands

Author: dkoshkin

charts/cluster-api-runtime-extensions-nutanix/defaultclusterclasses/nutanix-cluster-class.yaml

@@ -147,14 +147,6 @@ spec:
               tls-cipher-suites: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
         postKubeadmCommands:
         - echo export KUBECONFIG=/etc/kubernetes/admin.conf >> /root/.bashrc
-        - |
-          KUBERNETES_VERSION_NO_V=${KUBERNETES_VERSION#v}
-          VERSION_TO_COMPARE=1.29.0
-          if [ "$(printf '%s\n' "$KUBERNETES_VERSION_NO_V" "$VERSION_TO_COMPARE" | sort -V | head -n1)" != "$KUBERNETES_VERSION_NO_V" ]; then
-            if [ -f /run/kubeadm/kubeadm.yaml ]; then
-              sed -i 's#path: /etc/kubernetes/super-admin.conf#path: /etc/kubernetes/admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
-            fi
-          fi
         - echo "after kubeadm call" > /var/log/postkubeadm.log
         preKubeadmCommands:
         - echo "before kubeadm call" > /var/log/prekubeadm.log
@@ -163,14 +155,6 @@ spec:
         - echo "127.0.0.1   localhost" >>/etc/hosts
         - echo "127.0.0.1   kubernetes" >>/etc/hosts
         - echo "127.0.0.1   {{ ds.meta_data.hostname }}" >> /etc/hosts
-        - |
-          KUBERNETES_VERSION_NO_V=${KUBERNETES_VERSION#v}
-          VERSION_TO_COMPARE=1.29.0
-          if [ "$(printf '%s\n' "$KUBERNETES_VERSION_NO_V" "$VERSION_TO_COMPARE" | sort -V | head -n1)" != "$KUBERNETES_VERSION_NO_V" ]; then
-            if [ -f /run/kubeadm/kubeadm.yaml ]; then
-              sed -i 's#path: /etc/kubernetes/admin.conf#path: /etc/kubernetes/super-admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
-            fi
-          fi
         useExperimentalRetryJoin: true
         verbosity: 10
 ---

docs/content/customization/nutanix/control-plane-endpoint.md

@@ -55,4 +55,16 @@ spec:
         owner: root:root
         path: /etc/kubernetes/manifests/kube-vip.yaml
         permissions: "0600"
+      postKubeadmCommands:
+        # Only added for clusters version >=v1.29.0
+        - |-
+          if [ -f /run/kubeadm/kubeadm.yaml ]; then
+            sed -i 's#path: /etc/kubernetes/super-admin.conf#path: /etc/kubernetes/admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
+          fi
+      preKubeadmCommands:
+        # Only added for clusters version >=v1.29.0
+        - |-
+          if [ -f /run/kubeadm/kubeadm.yaml ]; then
+            sed -i 's#path: /etc/kubernetes/admin.conf#path: /etc/kubernetes/super-admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
+          fi
 ```

hack/examples/bases/nutanix/clusterclass/kustomization.yaml.tmpl

@@ -49,6 +49,17 @@ patches:
     - op: "remove"
       path: "/spec/template/spec/kubeadmConfigSpec/files/0"
 
+# Delete the kube-vip related pre and postKubeadmCommands.
+# Will be added back in the handler if enabled.
+# If the index of these changes upstream this will need to change, but will show up as a git diff.
+- target:
+    kind: KubeadmControlPlaneTemplate
+  patch: |-
+    - op: "remove"
+      path: "/spec/template/spec/kubeadmConfigSpec/preKubeadmCommands/6"
+    - op: "remove"
+      path: "/spec/template/spec/kubeadmConfigSpec/postKubeadmCommands/1"
+
 # FIXME: Debug why some of the patches are needed.
 # When the handler runs, it sends back multiple patches for individual fields.
 # But CAPI fails applying them because of missing value.

pkg/handlers/generic/mutation/controlplanevirtualip/inject.go

@@ -75,7 +75,7 @@ func (h *ControlPlaneVirtualIP) Mutate(
 	vars map[string]apiextensionsv1.JSON,
 	holderRef runtimehooksv1.HolderReference,
 	_ client.ObjectKey,
-	_ mutation.ClusterGetter,
+	clusterGetter mutation.ClusterGetter,
 ) error {
 	log := ctrl.LoggerFrom(ctx).WithValues(
 		"holderRef", holderRef,
@@ -108,6 +108,15 @@ func (h *ControlPlaneVirtualIP) Mutate(
 		return nil
 	}
 
+	cluster, err := clusterGetter(ctx)
+	if err != nil {
+		log.Error(
+			err,
+			"failed to get cluster from extraAPIServerCertSANs mutation handler",
+		)
+		return err
+	}
+
 	// only kube-vip is supported, but more providers can be added in the future
 	virtualIPProvider := providers.NewKubeVIPFromConfigMapProvider(
 		h.client,
@@ -138,6 +147,40 @@ func (h *ControlPlaneVirtualIP) Mutate(
 				obj.Spec.Template.Spec.KubeadmConfigSpec.Files,
 				*virtualIPProviderFile,
 			)
+
+			preKubeadmCommands, postKubeadmCommands, getCommandsErr := virtualIPProvider.GetCommands(cluster)
+			if getCommandsErr != nil {
+				return getCommandsErr
+			}
+
+			if len(preKubeadmCommands) > 0 {
+				log.WithValues(
+					"patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(),
+					"patchedObjectName", client.ObjectKeyFromObject(obj),
+				).Info(fmt.Sprintf(
+					"adding %s preKubeadmCommands to control plane kubeadm config spec",
+					virtualIPProvider.Name(),
+				))
+				obj.Spec.Template.Spec.KubeadmConfigSpec.PreKubeadmCommands = append(
+					obj.Spec.Template.Spec.KubeadmConfigSpec.PreKubeadmCommands,
+					preKubeadmCommands...,
+				)
+			}
+
+			if len(postKubeadmCommands) > 0 {
+				log.WithValues(
+					"patchedObjectKind", obj.GetObjectKind().GroupVersionKind().String(),
+					"patchedObjectName", client.ObjectKeyFromObject(obj),
+				).Info(fmt.Sprintf(
+					"adding %s postKubeadmCommands to control plane kubeadm config spec",
+					virtualIPProvider.Name(),
+				))
+				obj.Spec.Template.Spec.KubeadmConfigSpec.PostKubeadmCommands = append(
+					obj.Spec.Template.Spec.KubeadmConfigSpec.PostKubeadmCommands,
+					postKubeadmCommands...,
+				)
+			}
+
 			return nil
 		},
 	)

pkg/handlers/generic/mutation/controlplanevirtualip/inject_test.go

@@ -11,14 +11,18 @@ import (
 	"github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/capi/clustertopology/handlers/mutation"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/testutils/capitest"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/common/pkg/testutils/capitest/request"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/clusterconfig"
-	nutanixclusterconfig "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/nutanix/clusterconfig"
+	virtuialipproviders "github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/generic/mutation/controlplanevirtualip/providers"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/pkg/handlers/options"
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/test/helpers"
 )
@@ -32,10 +36,11 @@ var _ = Describe("Generate ControlPlane virtual IP patches", func() {
 	testDefs := []struct {
 		capitest.PatchTestDef
 		virtualIPTemplate string
+		cluster           *clusterv1.Cluster
 	}{
 		{
 			PatchTestDef: capitest.PatchTestDef{
-				Name: "host and port should be templated in a new file",
+				Name: "host and port should be templated in a new file and no pre/post commands",
 				Vars: []runtimehooksv1.Variable{
 					capitest.VariableWithValue(
 						clusterconfig.MetaVariableName,
@@ -69,24 +74,120 @@ var _ = Describe("Generate ControlPlane virtual IP patches", func() {
 						),
 					},
 				},
+				UnexpectedPatchMatchers: []capitest.JSONPatchMatcher{
+					{
+						Operation: "add",
+						Path:      "/spec/template/spec/kubeadmConfigSpec/preKubeadmCommands",
+						ValueMatcher: gomega.ContainElements(
+							virtuialipproviders.KubeVipPreKubeadmCommands,
+						),
+					},
+					{
+						Operation: "add",
+						Path:      "/spec/template/spec/kubeadmConfigSpec/postKubeadmCommands",
+						ValueMatcher: gomega.ContainElements(
+							virtuialipproviders.KubeVipPostKubeadmCommands,
+						),
+					},
+				},
+			},
+			virtualIPTemplate: validKubeVIPTemplate,
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      request.ClusterName,
+					Namespace: metav1.NamespaceDefault,
+				},
+				Spec: clusterv1.ClusterSpec{
+					Topology: &clusterv1.Topology{
+						Version: "v1.28.100",
+					},
+				},
+			},
+		},
+		{
+			PatchTestDef: capitest.PatchTestDef{
+				Name: "host and port should be templated in a new file with pre/post commands",
+				Vars: []runtimehooksv1.Variable{
+					capitest.VariableWithValue(
+						clusterconfig.MetaVariableName,
+						v1alpha1.ControlPlaneEndpointSpec{
+							Host:          "10.20.100.10",
+							Port:          6443,
+							VirtualIPSpec: &v1alpha1.ControlPlaneVirtualIPSpec{},
+						},
+						VariableName,
+					),
+				},
+				RequestItem: request.NewKubeadmControlPlaneTemplateRequestItem(
+					"",
+				),
+				ExpectedPatchMatchers: []capitest.JSONPatchMatcher{
+					{
+						Operation: "add",
+						Path:      "/spec/template/spec/kubeadmConfigSpec/files",
+						ValueMatcher: gomega.ContainElements(
+							gomega.SatisfyAll(
+								gomega.HaveKeyWithValue(
+									"content",
+									gomega.ContainSubstring("value: \"10.20.100.10\""),
+								),
+								gomega.HaveKeyWithValue(
+									"content",
+									gomega.ContainSubstring("value: \"6443\""),
+								),
+								gomega.HaveKey("owner"),
+								gomega.HaveKeyWithValue("path", gomega.ContainSubstring("kube-vip")),
+								gomega.HaveKey("permissions"),
+							),
+						),
+					},
+					{
+						Operation: "add",
+						Path:      "/spec/template/spec/kubeadmConfigSpec/preKubeadmCommands",
+						ValueMatcher: gomega.ContainElements(
+							virtuialipproviders.KubeVipPreKubeadmCommands,
+						),
+					},
+					{
+						Operation: "add",
+						Path:      "/spec/template/spec/kubeadmConfigSpec/postKubeadmCommands",
+						ValueMatcher: gomega.ContainElements(
+							virtuialipproviders.KubeVipPostKubeadmCommands,
+						),
+					},
+				},
 			},
 			virtualIPTemplate: validKubeVIPTemplate,
+			cluster: &clusterv1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      request.ClusterName,
+					Namespace: metav1.NamespaceDefault,
+				},
+				Spec: clusterv1.ClusterSpec{
+					Topology: &clusterv1.Topology{
+						Version: "v1.29.0",
+					},
+				},
+			},
 		},
 	}
 
 	// create test node for each case
-	for _, tt := range testDefs {
+	for idx := range testDefs {
+		tt := testDefs[idx]
 		It(tt.Name, func() {
+			clientScheme := runtime.NewScheme()
+			utilruntime.Must(clientgoscheme.AddToScheme(clientScheme))
+			utilruntime.Must(clusterv1.AddToScheme(clientScheme))
 			// Always initialize the testEnv variable in the closure.
 			// This will allow ginkgo to initialize testEnv variable during test execution time.
 			testEnv := helpers.TestEnv
 			// use direct client instead of controller client. This will allow the patch handler to read k8s object
 			// that are written by the tests.
 			// Test cases writes credentials secret that the mutator handler reads.
 			// Using direct client will enable reading it immediately.
-			client, err := testEnv.GetK8sClient()
+			client, err := testEnv.GetK8sClientWithScheme(clientScheme)
 			gomega.Expect(err).To(gomega.BeNil())
-
 			// setup a test ConfigMap to be used by the handler
 			cm := &corev1.ConfigMap{
 				TypeMeta: metav1.TypeMeta{
@@ -104,6 +205,15 @@ var _ = Describe("Generate ControlPlane virtual IP patches", func() {
 			err = client.Create(context.Background(), cm)
 			gomega.Expect(err).ToNot(gomega.HaveOccurred())
 
+			if tt.cluster != nil {
+				err = client.Create(context.Background(), tt.cluster)
+				gomega.Expect(err).To(gomega.BeNil())
+				defer func() {
+					err = client.Delete(context.Background(), tt.cluster)
+					gomega.Expect(err).To(gomega.BeNil())
+				}()
+			}
+
 			cfg := &Config{
 				GlobalOptions:               options.NewGlobalOptions(),
 				defaultKubeVipConfigMapName: cm.Name,

pkg/handlers/generic/mutation/controlplanevirtualip/providers/kubevip.go

@@ -7,13 +7,26 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/blang/semver/v4"
 	corev1 "k8s.io/api/core/v1"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
 )
 
+var (
+	//nolint:lll // for readability prefer to keep the long line
+	KubeVipPreKubeadmCommands = []string{`if [ -f /run/kubeadm/kubeadm.yaml ]; then
+  sed -i 's#path: /etc/kubernetes/admin.conf#path: /etc/kubernetes/super-admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
+fi`}
+	//nolint:lll // for readability prefer to keep the long line
+	KubeVipPostKubeadmCommands = []string{`if [ -f /run/kubeadm/kubeadm.yaml ]; then
+  sed -i 's#path: /etc/kubernetes/super-admin.conf#path: /etc/kubernetes/admin.conf#' /etc/kubernetes/manifests/kube-vip.yaml;
+fi`}
+)
+
 type kubeVIPFromConfigMapProvider struct {
 	client client.Reader
 
@@ -61,6 +74,29 @@ func (p *kubeVIPFromConfigMapProvider) GetFile(
 	}, nil
 }
 
+//nolint:gocritic // No need for named return values
+func (p *kubeVIPFromConfigMapProvider) GetCommands(cluster *clusterv1.Cluster) ([]string, []string, error) {
+	// The kube-vip static Pod uses admin.conf on the host to connect to the API server.
+	// But, starting with Kubernetes 1.29, admin.conf first gets created with no RBAC permissions.
+	// At the same time, 'kubeadm init' command waits for the API server to be reachable on the kube-vip IP.
+	// And since the kube-vip Pod is crashlooping with a permissions error, 'kubeadm init' fails.
+	// To work around this:
+	// 1. return a preKubeadmCommand to change the kube-vip Pod to use the new super-admin.conf file.
+	// 2. return a postKubeadmCommand to change the kube-vip Pod back to use admin.conf,
+	// after kubeadm has assigned it the necessary RBAC permissions.
+	//
+	// See https://github.com/kube-vip/kube-vip/issues/684
+	needCommands, err := needHackCommands(cluster)
+	if err != nil {
+		return nil, nil, fmt.Errorf("failed to determine if kube-vip commands are needed: %w", err)
+	}
+	if !needCommands {
+		return nil, nil, nil
+	}
+
+	return KubeVipPreKubeadmCommands, KubeVipPostKubeadmCommands, nil
+}
+
 type multipleKeysError struct {
 	configMapKey client.ObjectKey
 }
@@ -101,3 +137,12 @@ func getTemplateFromConfigMap(
 
 	return "", emptyValuesError{configMapKey: configMapKey}
 }
+
+func needHackCommands(cluster *clusterv1.Cluster) (bool, error) {
+	version, err := semver.ParseTolerant(cluster.Spec.Topology.Version)
+	if err != nil {
+		return false, fmt.Errorf("failed to parse version from cluster %w", err)
+	}
+
+	return version.Minor >= 29, nil
+}

pkg/handlers/generic/mutation/controlplanevirtualip/providers/providers.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"text/template"
 
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	bootstrapv1 "sigs.k8s.io/cluster-api/bootstrap/kubeadm/api/v1beta1"
 
 	"github.com/nutanix-cloud-native/cluster-api-runtime-extensions-nutanix/api/v1alpha1"
@@ -24,6 +25,7 @@ const (
 type Provider interface {
 	Name() string
 	GetFile(ctx context.Context, spec v1alpha1.ControlPlaneEndpointSpec) (*bootstrapv1.File, error)
+	GetCommands(cluster *clusterv1.Cluster) ([]string, []string, error)
 }
 
 func templateValues(controlPlaneEndpoint v1alpha1.ControlPlaneEndpointSpec, text string) (string, error) {