Merge pull request #11948 from chrischdi/pr-ensure-v1beta2-conditions-are-true

🐛 ensure kubeadm controller always sets all v1beta2 conditions

Author: k8s-ci-robot

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -312,9 +312,30 @@ func (r *KubeadmConfigReconciler) reconcile(ctx context.Context, scope *Scope, c
 			Status: metav1.ConditionTrue,
 			Reason: bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Reason,
 		})
+		v1beta2conditions.Set(scope.Config, metav1.Condition{
+			Type:   bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Condition,
+			Status: metav1.ConditionTrue,
+			Reason: bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Reason,
+		})
 		return ctrl.Result{}, nil
 	// Status is ready means a config has been generated.
+	// This also solves the upgrade scenario to a version which includes v1beta2 to ensure v1beta2 conditions are properly set.
 	case config.Status.Ready:
+		// Based on existing code paths status.Ready is only true if status.dataSecretName is set
+		// So we can assume that the DataSecret is available.
+		conditions.MarkTrue(config, bootstrapv1.DataSecretAvailableCondition)
+		v1beta2conditions.Set(scope.Config, metav1.Condition{
+			Type:   bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Condition,
+			Status: metav1.ConditionTrue,
+			Reason: bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Reason,
+		})
+		// Same applies for the CertificatesAvailable, which must have been the case to generate
+		// the DataSecret.
+		v1beta2conditions.Set(scope.Config, metav1.Condition{
+			Type:   bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Condition,
+			Status: metav1.ConditionTrue,
+			Reason: bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Reason,
+		})
 		if config.Spec.JoinConfiguration != nil && config.Spec.JoinConfiguration.Discovery.BootstrapToken != nil {
 			if !configOwner.HasNodeRefs() {
 				// If the BootstrapToken has been generated for a join but the config owner has no nodeRefs,

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller_test.go

@@ -46,6 +46,7 @@ import (
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/certs"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/secret"
 	"sigs.k8s.io/cluster-api/util/test/builder"
@@ -2744,3 +2745,96 @@ func assertHasTrueCondition(g *WithT, myclient client.Client, req ctrl.Request,
 	g.Expect(c).ToNot(BeNil())
 	g.Expect(c.Status).To(Equal(corev1.ConditionTrue))
 }
+
+func TestKubeadmConfigReconciler_Reconcile_v1beta2_conditions(t *testing.T) {
+	// Setup work for an initialized cluster
+	clusterName := "my-cluster"
+	cluster := builder.Cluster(metav1.NamespaceDefault, clusterName).Build()
+	conditions.MarkTrue(cluster, clusterv1.ControlPlaneInitializedCondition)
+	cluster.Status.InfrastructureReady = true
+	cluster.Spec.ControlPlaneEndpoint = clusterv1.APIEndpoint{
+		Host: "example.com",
+		Port: 6443,
+	}
+
+	machine := builder.Machine(metav1.NamespaceDefault, "my-machine").
+		WithVersion("v1.19.1").
+		WithClusterName(cluster.Name).
+		WithBootstrapTemplate(bootstrapbuilder.KubeadmConfig(metav1.NamespaceDefault, "").Unstructured()).
+		Build()
+
+	kubeadmConfig := newKubeadmConfig(metav1.NamespaceDefault, "kubeadmconfig")
+
+	tests := []struct {
+		name    string
+		config  *bootstrapv1.KubeadmConfig
+		machine *clusterv1.Machine
+	}{
+		{
+			name:    "conditions should be true again after reconciling",
+			config:  kubeadmConfig.DeepCopy(),
+			machine: machine.DeepCopy(),
+		},
+		{
+			name:   "conditions should be true again after status got emptied out",
+			config: kubeadmConfig.DeepCopy(),
+			machine: func() *clusterv1.Machine {
+				m := machine.DeepCopy()
+				m.Spec.Bootstrap.DataSecretName = ptr.To("foo")
+				return m
+			}(),
+		},
+		{
+			name: "conditions should be true after upgrading to v1beta2",
+			config: func() *bootstrapv1.KubeadmConfig {
+				c := kubeadmConfig.DeepCopy()
+				c.Status.Ready = true
+				return c
+			}(),
+			machine: machine.DeepCopy(),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+			cluster := cluster.DeepCopy()
+			tt.config.SetOwnerReferences([]metav1.OwnerReference{{
+				APIVersion: clusterv1.GroupVersion.String(),
+				Kind:       "Machine",
+				Name:       tt.machine.Name,
+			}})
+
+			objects := []client.Object{cluster, tt.machine, tt.config}
+			objects = append(objects, createSecrets(t, cluster, tt.config)...)
+
+			myclient := fake.NewClientBuilder().WithObjects(objects...).WithStatusSubresource(&bootstrapv1.KubeadmConfig{}).Build()
+
+			r := &KubeadmConfigReconciler{
+				Client:              myclient,
+				SecretCachingClient: myclient,
+				ClusterCache:        clustercache.NewFakeClusterCache(myclient, client.ObjectKey{Name: cluster.Name, Namespace: cluster.Namespace}),
+				KubeadmInitLock:     &myInitLocker{},
+			}
+
+			key := client.ObjectKey{Namespace: tt.config.Namespace, Name: tt.config.Name}
+			_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key})
+			g.Expect(err).ToNot(HaveOccurred())
+
+			newConfig := &bootstrapv1.KubeadmConfig{}
+			g.Expect(myclient.Get(ctx, key, newConfig)).To(Succeed())
+
+			for _, conditionType := range []string{bootstrapv1.KubeadmConfigReadyV1Beta2Condition, bootstrapv1.KubeadmConfigCertificatesAvailableV1Beta2Condition, bootstrapv1.KubeadmConfigDataSecretAvailableV1Beta2Condition} {
+				condition := v1beta2conditions.Get(newConfig, conditionType)
+				g.Expect(condition).ToNot(BeNil(), "condition %s is missing", conditionType)
+				g.Expect(condition.Status).To(Equal(metav1.ConditionTrue))
+				g.Expect(condition.Message).To(BeEmpty())
+			}
+			for _, conditionType := range []string{clusterv1.PausedV1Beta2Condition} {
+				condition := v1beta2conditions.Get(newConfig, conditionType)
+				g.Expect(condition).ToNot(BeNil(), "condition %s is missing", conditionType)
+				g.Expect(condition.Status).To(Equal(metav1.ConditionFalse))
+				g.Expect(condition.Message).To(BeEmpty())
+			}
+		})
+	}
+}

test/e2e/clusterctl_upgrade.go

@@ -53,6 +53,7 @@ import (
 	"sigs.k8s.io/cluster-api/test/framework/bootstrap"
 	"sigs.k8s.io/cluster-api/test/framework/clusterctl"
 	"sigs.k8s.io/cluster-api/util"
+	v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2"
 )
 
 // ClusterctlUpgradeSpecInput is the input for ClusterctlUpgradeSpec.
@@ -705,6 +706,10 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg
 				upgrade.PostUpgrade(managementClusterProxy, workloadCluster.Namespace, workloadCluster.Name)
 			}
 
+			Byf("[%d] Verify v1beta2 Available and Ready conditions (if exist) to be true for Cluster and Machines", i)
+			verifyV1Beta2ConditionsTrue(ctx, managementClusterProxy.GetClient(), workloadCluster.Name, workloadCluster.Namespace,
+				[]string{clusterv1.AvailableV1Beta2Condition, clusterv1.ReadyV1Beta2Condition})
+
 			Byf("[%d] Verify client-side SSA still works", i)
 			clusterUpdate := &unstructured.Unstructured{}
 			clusterUpdate.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("Cluster"))
@@ -767,6 +772,38 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg
 	})
 }
 
+// verifyV1Beta2ConditionsTrue checks the Cluster and Machines of a Cluster that
+// the given v1beta2 condition types are set to true without a message, if they exist.
+func verifyV1Beta2ConditionsTrue(ctx context.Context, c client.Client, clusterName, clusterNamespace string, v1beta2conditionTypes []string) {
+	cluster := framework.GetClusterByName(ctx, framework.GetClusterByNameInput{
+		Getter:    c,
+		Name:      clusterName,
+		Namespace: clusterNamespace,
+	})
+	for _, conditionType := range v1beta2conditionTypes {
+		if v1beta2conditions.Has(cluster, conditionType) {
+			condition := v1beta2conditions.Get(cluster, conditionType)
+			Expect(condition.Status).To(Equal(metav1.ConditionTrue), "The v1beta2 condition %q on the Cluster should be set to true", conditionType)
+			Expect(condition.Message).To(BeEmpty(), "The v1beta2 condition %q on the Cluster should have an empty message", conditionType)
+		}
+	}
+
+	machines := framework.GetMachinesByCluster(ctx, framework.GetMachinesByClusterInput{
+		Lister:      c,
+		ClusterName: clusterName,
+		Namespace:   clusterNamespace,
+	})
+	for _, machine := range machines {
+		for _, conditionType := range v1beta2conditionTypes {
+			if v1beta2conditions.Has(&machine, conditionType) {
+				condition := v1beta2conditions.Get(&machine, conditionType)
+				Expect(condition.Status).To(Equal(metav1.ConditionTrue), "The v1beta2 condition %q on the Machine %q should be set to true", conditionType, machine.Name)
+				Expect(condition.Message).To(BeEmpty(), "The v1beta2 condition %q on the Machine %q should have an empty message", conditionType, machine.Name)
+			}
+		}
+	}
+}
+
 func setupClusterctl(ctx context.Context, clusterctlBinaryURL, clusterctlConfigPath string) (string, string) {
 	clusterctlBinaryPath := downloadToTmpFile(ctx, clusterctlBinaryURL)
 

test/infrastructure/docker/internal/controllers/backends/docker/dockermachine_backend.go

@@ -126,6 +126,26 @@ func (r *MachineBackendReconciler) ReconcileNormal(ctx context.Context, cluster
 				Status: metav1.ConditionTrue,
 				Reason: infrav1.DevMachineDockerContainerProvisionedV1Beta2Reason,
 			})
+			// In case of upgrades the v1beta2 condition for BootstrapExecSucceeded does not exist.
+			// In this case recover the information from the existing v1beta1 condition, because we do not know if
+			// all commands succeeded.
+			if !v1beta2conditions.Has(dockerMachine, infrav1.DevMachineDockerContainerBootstrapExecSucceededV1Beta2Condition) {
+				condition := conditions.Get(dockerMachine, infrav1.BootstrapExecSucceededCondition)
+				if condition == nil || condition.Status == corev1.ConditionTrue {
+					v1beta2conditions.Set(dockerMachine, metav1.Condition{
+						Type:   infrav1.DevMachineDockerContainerBootstrapExecSucceededV1Beta2Condition,
+						Status: metav1.ConditionTrue,
+						Reason: infrav1.DevMachineDockerContainerBootstrapExecSucceededV1Beta2Reason,
+					})
+				} else {
+					v1beta2conditions.Set(dockerMachine, metav1.Condition{
+						Type:    infrav1.DevMachineDockerContainerBootstrapExecSucceededV1Beta2Condition,
+						Status:  metav1.ConditionFalse,
+						Message: condition.Message,
+						Reason:  infrav1.DevMachineDockerContainerBootstrapExecNotSucceededV1Beta2Reason,
+					})
+				}
+			}
 
 			// Setting machine address is required after move, because status.Address field is not retained during move.
 			if err := setMachineAddress(ctx, dockerMachine, externalMachine); err != nil {