Merge pull request #3918 from jrvaldes/OCPBUGS-38401

OCPBUGS-38401: Mirror MAPI userData to openshift-cluster-api namespace for CAPI support

Author: openshift-merge-bot[bot]

controllers/secret_controller.go

@@ -34,6 +34,7 @@ import (
 
 //+kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;patch
 //+kubebuilder:rbac:groups="",resources=secrets,verbs=create;get;list;watch;update
+//+kubebuilder:rbac:groups="",resources=namespaces,verbs=get
 
 const (
 	// SecretController is the name of this controller in logs and other outputs.
@@ -113,9 +114,14 @@ func (r *SecretReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manage
 		Complete(r)
 }
 
-// isUserDataSecret returns true if the provided object is the userData Secret
+// isUserDataSecret returns true if the provided object is the userData Secret in either the
+// Machine API or Cluster API namespace.
 func isUserDataSecret(obj client.Object) bool {
-	return obj.GetName() == secrets.UserDataSecret && obj.GetNamespace() == cluster.MachineAPINamespace
+	if obj.GetName() != secrets.UserDataSecret {
+		return false
+	}
+	return obj.GetNamespace() == cluster.MachineAPINamespace ||
+		obj.GetNamespace() == cluster.ClusterAPINamespace
 }
 
 // isPrivateKeySecret returns true if the provided object is the private key secret
@@ -192,25 +198,80 @@ func (r *SecretReconciler) reconcileUserDataSecret(ctx context.Context) error {
 	// Fetch UserData instance
 	err = r.client.Get(ctx,
 		kubeTypes.NamespacedName{Name: secrets.UserDataSecret, Namespace: cluster.MachineAPINamespace}, userData)
-	if err != nil && k8sapierrors.IsNotFound(err) {
+	if err != nil {
+		if !k8sapierrors.IsNotFound(err) {
+			r.log.Error(err, "error retrieving the secret", "name", secrets.UserDataSecret)
+			return err
+		}
 		// Secret is deleted
 		r.log.Info("secret not found, creating the secret", "name", secrets.UserDataSecret)
 		err = r.client.Create(ctx, validUserData)
 		if err != nil {
 			return err
 		}
-		// Secret created successfully - don't requeue
-		return nil
-	} else if err != nil {
-		r.log.Error(err, "error retrieving the secret", "name", secrets.UserDataSecret)
-		return err
-	} else if string(userData.Data["userData"][:]) == string(validUserData.Data["userData"][:]) {
-		// valid userData secret already exists
-		return nil
 	} else {
-		// userdata secret data does not match what is expected
-		return r.updateUserData(ctx, keySigner, validUserData)
+		if string(userData.Data["userData"][:]) != string(validUserData.Data["userData"][:]) {
+			// userdata secret data does not match what is expected
+			if err = r.updateUserData(ctx, keySigner, validUserData); err != nil {
+				return err
+			}
+		}
+	}
+
+	// If the ClusterAPIMachineManagement feature gate is enabled, mirror the userData secret
+	// to the openshift-cluster-api namespace for CAPI-provisioned Windows machines
+	capiEnabled, err := r.isClusterAPIEnabled(ctx)
+	if err != nil {
+		return fmt.Errorf("error checking for Cluster API namespace: %w", err)
+	}
+	if capiEnabled {
+		return r.ensureCAPIUserDataSecret(ctx, validUserData)
+	}
+
+	// reconciliation successful, no need to requeue
+	return nil
+}
+
+// isClusterAPIEnabled returns true when the openshift-cluster-api namespace exist which indicate that the
+// ClusterAPIMachineManagement feature gate is active
+func (r *SecretReconciler) isClusterAPIEnabled(ctx context.Context) (bool, error) {
+	ns := &core.Namespace{}
+	err := r.client.Get(ctx, kubeTypes.NamespacedName{Name: cluster.ClusterAPINamespace}, ns)
+	if err != nil {
+		if k8sapierrors.IsNotFound(err) {
+			return false, nil
+		}
+		return false, fmt.Errorf("error getting namespace %s: %w", cluster.ClusterAPINamespace, err)
+	}
+	return true, nil
+}
+
+// ensureCAPIUserDataSecret creates or updates a copy of the windows userData secret in the
+// openshift-cluster-api namespace so that CAPI-based MachineSets can reference it.
+func (r *SecretReconciler) ensureCAPIUserDataSecret(ctx context.Context, mapiExpectedSecret *core.Secret) error {
+	capiSecret := mapiExpectedSecret.DeepCopy()
+	capiSecret.Namespace = cluster.ClusterAPINamespace
+	// ensure fields are clear
+	capiSecret.ResourceVersion = ""
+	capiSecret.UID = ""
+
+	existing := &core.Secret{}
+	err := r.client.Get(ctx,
+		kubeTypes.NamespacedName{Name: secrets.UserDataSecret, Namespace: cluster.ClusterAPINamespace}, existing)
+	if err != nil {
+		if k8sapierrors.IsNotFound(err) {
+			r.log.Info("creating user data secret in Cluster API namespace", "name", secrets.UserDataSecret)
+			return r.client.Create(ctx, capiSecret)
+		}
+		return fmt.Errorf("error getting user data secret in Cluster API namespace: %w", err)
+	}
+	if string(existing.Data["userData"]) == string(capiSecret.Data["userData"]) {
+		// secrets match, nothing to do.
+		return nil
 	}
+	r.log.Info("updating user data secret in Cluster API namespace", "name", secrets.UserDataSecret)
+	capiSecret.ResourceVersion = existing.ResourceVersion
+	return r.client.Update(ctx, capiSecret)
 }
 
 func (r *SecretReconciler) reconcileTLSSecret(ctx context.Context) error {

controllers/secret_controller_test.go

@@ -0,0 +1,146 @@
+package controllers
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	core "k8s.io/api/core/v1"
+	meta "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/openshift/windows-machine-config-operator/pkg/cluster"
+	"github.com/openshift/windows-machine-config-operator/pkg/secrets"
+)
+
+// newTestSecretReconciler returns a SecretReconciler backed by a fake client pre-seeded with initObjs.
+func newTestSecretReconciler(initObjs ...client.Object) *SecretReconciler {
+	scheme := runtime.NewScheme()
+	_ = core.AddToScheme(scheme)
+
+	fc := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjs...).Build()
+	return &SecretReconciler{
+		instanceReconciler: instanceReconciler{
+			client: fc,
+			log:    ctrl.Log.WithName("test"),
+		},
+	}
+}
+
+// newUserDataSecret creates a minimal windows-user-data Secret in the given namespace.
+func newUserDataSecret(namespace, userData string) *core.Secret {
+	return &core.Secret{
+		ObjectMeta: meta.ObjectMeta{
+			Name:      secrets.UserDataSecret,
+			Namespace: namespace,
+		},
+		Data: map[string][]byte{
+			"userData": []byte(userData),
+		},
+	}
+}
+
+// TestIsUserDataSecret verifies that isUserDataSecret matches secrets in both the Machine API and
+// Cluster API namespaces, but not in unrelated namespaces.
+func TestIsUserDataSecret(t *testing.T) {
+	tests := []struct {
+		name     string
+		ns       string
+		expected bool
+	}{
+		{
+			name:     "Machine API namespace matches",
+			ns:       cluster.MachineAPINamespace,
+			expected: true,
+		},
+		{
+			name:     "Cluster API namespace matches",
+			ns:       cluster.ClusterAPINamespace,
+			expected: true,
+		},
+		{
+			name:     "Unrelated namespace does not match",
+			ns:       "kube-system",
+			expected: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := newUserDataSecret(tt.ns, "data")
+			assert.Equal(t, tt.expected, isUserDataSecret(s))
+		})
+	}
+}
+
+// TestIsClusterAPIEnabled verifies that isClusterAPIEnabled returns true only when the
+// openshift-cluster-api namespace exists.
+func TestIsClusterAPIEnabled(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("Namespace absent returns false", func(t *testing.T) {
+		r := newTestSecretReconciler()
+		enabled, err := r.isClusterAPIEnabled(ctx)
+		require.NoError(t, err)
+		assert.False(t, enabled)
+	})
+
+	t.Run("Namespace present returns true", func(t *testing.T) {
+		ns := &core.Namespace{ObjectMeta: meta.ObjectMeta{Name: cluster.ClusterAPINamespace}}
+		r := newTestSecretReconciler(ns)
+		enabled, err := r.isClusterAPIEnabled(ctx)
+		require.NoError(t, err)
+		assert.True(t, enabled)
+	})
+}
+
+// TestEnsureCAPIUserDataSecret verifies the create / no-op / update behaviour of ensureCAPIUserDataSecret.
+func TestEnsureCAPIUserDataSecret(t *testing.T) {
+	ctx := context.Background()
+	const content = "<powershell>ssh-rsa AAAA...</powershell>"
+
+	t.Run("Creates secret when absent in CAPI namespace", func(t *testing.T) {
+		r := newTestSecretReconciler()
+		src := newUserDataSecret(cluster.MachineAPINamespace, content)
+
+		require.NoError(t, r.ensureCAPIUserDataSecret(ctx, src))
+
+		got := &core.Secret{}
+		require.NoError(t, r.client.Get(ctx,
+			client.ObjectKey{Name: secrets.UserDataSecret, Namespace: cluster.ClusterAPINamespace}, got))
+		assert.Equal(t, content, string(got.Data["userData"]))
+		assert.Equal(t, cluster.ClusterAPINamespace, got.Namespace)
+	})
+
+	t.Run("No-op when CAPI secret already matches", func(t *testing.T) {
+		existing := newUserDataSecret(cluster.ClusterAPINamespace, content)
+		existing.ResourceVersion = "42"
+		r := newTestSecretReconciler(existing)
+		src := newUserDataSecret(cluster.MachineAPINamespace, content)
+
+		require.NoError(t, r.ensureCAPIUserDataSecret(ctx, src))
+
+		got := &core.Secret{}
+		require.NoError(t, r.client.Get(ctx,
+			client.ObjectKey{Name: secrets.UserDataSecret, Namespace: cluster.ClusterAPINamespace}, got))
+		// ResourceVersion must be unchanged (no update was issued).
+		assert.Equal(t, "42", got.ResourceVersion)
+	})
+
+	t.Run("Updates CAPI secret when content differs", func(t *testing.T) {
+		existing := newUserDataSecret(cluster.ClusterAPINamespace, "old-data")
+		existing.ResourceVersion = "1"
+		r := newTestSecretReconciler(existing)
+		src := newUserDataSecret(cluster.MachineAPINamespace, content)
+
+		require.NoError(t, r.ensureCAPIUserDataSecret(ctx, src))
+
+		got := &core.Secret{}
+		require.NoError(t, r.client.Get(ctx,
+			client.ObjectKey{Name: secrets.UserDataSecret, Namespace: cluster.ClusterAPINamespace}, got))
+		assert.Equal(t, content, string(got.Data["userData"]))
+	})
+}

pkg/cluster/config.go

@@ -27,6 +27,9 @@ const (
 	baseK8sVersion = "v1.35"
 	// MachineAPINamespace is the name of the namespace in which machine objects and userData secret is created.
 	MachineAPINamespace = "openshift-machine-api"
+	// ClusterAPINamespace is the namespace used by the Cluster API components when the
+	// ClusterAPIMachineManagement feature gate is enabled.
+	ClusterAPINamespace = "openshift-cluster-api"
 )
 
 var (