XMGMT-15704 - Bugfix for local cluster import (openshift#5484)

During the QE process for this feature, it was discovered that there are some bugs preventing the proper creation of a ClusterDeployment in some environments. The owner of the ClusterDeployment has been changed to the AgentServiceConfig as it should be and a permission has been added to permit "ManagedClusterSet/join" as this is required to be able to create a ClusterDeployment.
danielerez · Oct 15, 2023 · 769fcd1 · 769fcd1
1 parent 697fb4a
commit 769fcd1
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 75 deletions.
diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -258,6 +258,12 @@ rules:
   - subjectaccessreviews
   verbs:
   - create
+- apiGroups:
+  - cluster.open-cluster-management.io
+  resources:
+  - managedclustersets/join
+  verbs:
+  - create
 - apiGroups:
   - config.openshift.io
   resources:

diff --git a/deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml b/deploy/olm-catalog/manifests/assisted-service-operator.clusterserviceversion.yaml
@@ -603,6 +603,12 @@ spec:
           - subjectaccessreviews
           verbs:
           - create
+        - apiGroups:
+          - cluster.open-cluster-management.io
+          resources:
+          - managedclustersets/join
+          verbs:
+          - create
         - apiGroups:
           - config.openshift.io
           resources:

diff --git a/internal/controller/controllers/agentserviceconfig_controller.go b/internal/controller/controllers/agentserviceconfig_controller.go
@@ -67,7 +67,7 @@ import (
 const (
 	// agentServiceConfigName is the one and only name for an AgentServiceConfig
 	// supported in the cluster. Any others will be ignored.
-	agentServiceConfigName        = "agent"
+	AgentServiceConfigName        = "agent"
 	serviceName            string = "assisted-service"
 	imageServiceName       string = "assisted-image-service"
 	webhookServiceName     string = "agentinstalladmission"
@@ -229,9 +229,9 @@ func (r *AgentServiceConfigReconciler) Reconcile(origCtx context.Context, req ct
 
 	// We only support one AgentServiceConfig per cluster, and it must be called "agent". This prevents installing
 	// AgentService more than once in the cluster.
-	if instance.Name != agentServiceConfigName {
+	if instance.Name != AgentServiceConfigName {
 		reason := fmt.Sprintf("Invalid name (%s)", instance.Name)
-		msg := fmt.Sprintf("Only one AgentServiceConfig supported per cluster and must be named '%s'", agentServiceConfigName)
+		msg := fmt.Sprintf("Only one AgentServiceConfig supported per cluster and must be named '%s'", AgentServiceConfigName)
 		log.Info(fmt.Sprintf("%s: %s", reason, msg), req.NamespacedName)
 		r.Recorder.Event(instance, "Warning", reason, msg)
 		return reconcile.Result{}, nil
@@ -460,7 +460,7 @@ func (r *AgentServiceConfigReconciler) SetupWithManager(mgr ctrl.Manager) error
 	})
 	ingressCMHandler := handler.EnqueueRequestsFromMapFunc(
 		func(_ client.Object) []reconcile.Request {
-			return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: agentServiceConfigName}}}
+			return []reconcile.Request{{NamespacedName: types.NamespacedName{Name: AgentServiceConfigName}}}
 		},
 	)
 

diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go
@@ -116,6 +116,7 @@ const minimalOpenShiftVersionForDefaultNetworkTypeOVNKubernetes = "4.12.0-0.0"
 // +kubebuilder:rbac:groups=extensions.hive.openshift.io,resources=agentclusterinstalls/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=extensions.hive.openshift.io,resources=agentclusterinstalls/finalizers,verbs=update
 // +kubebuilder:rbac:groups=config.openshift.io,resources=dnses,verbs=get
+// +kubebuilder:rbac:groups=cluster.open-cluster-management.io,resources=managedclustersets/join,verbs=create
 
 func (r *ClusterDeploymentsReconciler) Reconcile(origCtx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	ctx := addRequestIdIfNeeded(origCtx)

diff --git a/pkg/localclusterimport/import_local_cluster.go b/pkg/localclusterimport/import_local_cluster.go
@@ -8,6 +8,7 @@ import (
 	multierror "github.com/hashicorp/go-multierror"
 	configv1 "github.com/openshift/api/config/v1"
 	hiveext "github.com/openshift/assisted-service/api/hiveextension/v1beta1"
+	aiv1beta1 "github.com/openshift/assisted-service/api/v1beta1"
 	hivev1 "github.com/openshift/hive/apis/hive/v1"
 	"github.com/openshift/hive/apis/hive/v1/agent"
 	"github.com/sirupsen/logrus"
@@ -37,7 +38,7 @@ func (i *LocalClusterImport) createClusterImageSet(release_image string) error {
 		},
 	}
 	err = i.clusterImportOperations.CreateClusterImageSet(&clusterImageSet)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
+	if err != nil {
 		i.log.Errorf("unable to create ClusterImageSet due to error: %s", err.Error())
 		return err
 	}
@@ -53,7 +54,7 @@ func (i *LocalClusterImport) createAdminKubeConfig(kubeConfigSecret *v1.Secret)
 	localClusterSecret.Data = make(map[string][]byte)
 	localClusterSecret.Data["kubeconfig"] = kubeConfigSecret.Data["lb-ext.kubeconfig"]
 	err = i.clusterImportOperations.CreateSecret(i.localClusterNamespace, &localClusterSecret)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
+	if err != nil {
 		i.log.Errorf("to store secret due to error %s", err.Error())
 		return err
 	}
@@ -77,14 +78,14 @@ func (i *LocalClusterImport) createLocalClusterPullSecret(sourceSecret *v1.Secre
 	}
 	hubPullSecret.OwnerReferences = []metav1.OwnerReference{}
 	err = i.clusterImportOperations.CreateSecret(i.localClusterNamespace, &hubPullSecret)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
+	if err != nil {
 		i.log.Errorf("unable to store hub pull secret due to error %s", err.Error())
 		return err
 	}
 	return nil
 }
 
-func (i *LocalClusterImport) createAgentClusterInstall(numberOfControlPlaneNodes int) (*hiveext.AgentClusterInstall, error) {
+func (i *LocalClusterImport) createAgentClusterInstall(numberOfControlPlaneNodes int) error {
 	//Create an AgentClusterInstall in the local cluster namespace
 	userManagedNetworkingActive := true
 	agentClusterInstall := &hiveext.AgentClusterInstall{
@@ -106,21 +107,28 @@ func (i *LocalClusterImport) createAgentClusterInstall(numberOfControlPlaneNodes
 	agentClusterInstall.Namespace = i.localClusterNamespace
 	agentClusterInstall.Name = i.localClusterNamespace + "-cluster-install"
 	err := i.clusterImportOperations.CreateAgentClusterInstall(agentClusterInstall)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
-		i.log.Errorf("could not create AgentClusterInstall due to error %s", err.Error())
-		return nil, err
-	}
-	// Fetch the recently stored AgentClusterInstall so that we can obtain the UID
-	aci, err := i.clusterImportOperations.GetAgentClusterInstall(i.localClusterNamespace, i.localClusterNamespace+"-cluster-install")
 	if err != nil {
-		i.log.Errorf("failed to fetch created AgentClusterInstall due to error %s", err.Error())
-		return nil, err
+		i.log.Errorf("could not create AgentClusterInstall due to error %s", err.Error())
+		return err
 	}
-	return aci, nil
+	return nil
 }
 
-func (i *LocalClusterImport) createClusterDeployment(pullSecret *v1.Secret, dns *configv1.DNS, kubeConfigSecret *v1.Secret, agentClusterInstall *hiveext.AgentClusterInstall) error {
-	if pullSecret == nil || dns == nil || kubeConfigSecret == nil || agentClusterInstall == nil {
+func (i *LocalClusterImport) createClusterDeployment(pullSecret *v1.Secret, dns *configv1.DNS, kubeConfigSecret *v1.Secret, agentServiceConfig *aiv1beta1.AgentServiceConfig) error {
+	if pullSecret == nil {
+		i.log.Errorf("Skipping creation of cluster deployment due to nil pullsecret")
+		return nil
+	}
+	if dns == nil {
+		i.log.Errorf("Skipping creation of cluster deployment due to nil dns")
+		return nil
+	}
+	if kubeConfigSecret == nil {
+		i.log.Errorf("Skipping creation of cluster deployment due to nil kubeconfigsecret")
+		return nil
+	}
+	if agentServiceConfig == nil {
+		i.log.Errorf("Skipping creation of cluster deployment due to nil agentServiceConfig")
 		return nil
 	}
 	// Create a cluster deployment in the local cluster namespace
@@ -154,20 +162,15 @@ func (i *LocalClusterImport) createClusterDeployment(pullSecret *v1.Secret, dns
 	clusterDeployment.Namespace = i.localClusterNamespace
 	clusterDeployment.Spec.ClusterName = i.localClusterNamespace + "-cluster-deployment"
 	clusterDeployment.Spec.BaseDomain = dns.Spec.BaseDomain
-	//
-	// Adding this ownership reference to ensure we can submit clusterDeployment without ManagedClusterSet/join permission
-	//
-	// Must match https://github.com/stolostron/multicloud-operators-foundation/blob/0001f46a5115fe43c606a068e5e7ee00abec3b68/pkg/webhook/clusterset/validatingWebhook.go#L44
-	// or the ClusterDeployment will be rejected during admission.
 	agentClusterInstallOwnerRef := metav1.OwnerReference{
-		Kind:       "AgentCluster",
-		APIVersion: "capi-provider.agent-install.openshift.io/v1alpha1",
-		Name:       i.localClusterNamespace + "-cluster-install",
-		UID:        agentClusterInstall.UID,
+		Kind:       "AgentServiceConfig",
+		APIVersion: "agent-install.openshift.io/v1beta1",
+		Name:       agentServiceConfig.Name,
+		UID:        agentServiceConfig.UID,
 	}
 	clusterDeployment.OwnerReferences = []metav1.OwnerReference{agentClusterInstallOwnerRef}
 	err := i.clusterImportOperations.CreateClusterDeployment(clusterDeployment)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
+	if err != nil {
 		i.log.Errorf("could not create ClusterDeployment due to error %s", err.Error())
 		return err
 	}
@@ -176,7 +179,7 @@ func (i *LocalClusterImport) createClusterDeployment(pullSecret *v1.Secret, dns
 
 func (i *LocalClusterImport) createNamespace(name string) error {
 	err := i.clusterImportOperations.CreateNamespace(name)
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
+	if err != nil {
 		i.log.Errorf("could not create Namespace due to error %s", err.Error())
 		return err
 	}
@@ -239,7 +242,7 @@ func (i *LocalClusterImport) ImportLocalCluster() error {
 	release_image = clusterVersion.Status.History[0].Image
 
 	err = i.createClusterImageSet(release_image)
-	if err != nil {
+	if err != nil && !k8serrors.IsAlreadyExists(err) {
 		errorList = multierror.Append(errorList, err)
 	}
 
@@ -263,11 +266,18 @@ func (i *LocalClusterImport) ImportLocalCluster() error {
 	}
 
 	if numberOfControlPlaneNodes > 0 {
-		agentClusterInstall, err := i.createAgentClusterInstall(numberOfControlPlaneNodes)
+		err := i.createAgentClusterInstall(numberOfControlPlaneNodes)
 		if err != nil && !k8serrors.IsAlreadyExists(err) {
 			errorList = multierror.Append(errorList, err)
 		}
-		err = i.createClusterDeployment(pullSecret, dns, kubeConfigSecret, agentClusterInstall)
+
+		asc, err := i.clusterImportOperations.GetAgentServiceConfig()
+		if err != nil {
+			i.log.Errorf("failed to fetch AgentServiceConfig due to error %s", err.Error())
+			errorList = multierror.Append(errorList, err)
+		}
+
+		err = i.createClusterDeployment(pullSecret, dns, kubeConfigSecret, asc)
 		if err != nil && !k8serrors.IsAlreadyExists(err) {
 			errorList = multierror.Append(errorList, err)
 		}

diff --git a/pkg/localclusterimport/import_local_cluster_test.go b/pkg/localclusterimport/import_local_cluster_test.go
@@ -12,6 +12,7 @@ import (
 	. "github.com/onsi/gomega"
 	configv1 "github.com/openshift/api/config/v1"
 	hiveext "github.com/openshift/assisted-service/api/hiveextension/v1beta1"
+	aiv1beta1 "github.com/openshift/assisted-service/api/v1beta1"
 	hivev1 "github.com/openshift/hive/apis/hive/v1"
 	"github.com/openshift/hive/apis/hive/v1/agent"
 	"github.com/sirupsen/logrus"
@@ -32,7 +33,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		nodeConfigsKubeConfig   string
 		pullSecret              []byte
 		pullSecretBase64Encoded []byte
-		agentClusterInstallUID  types.UID
+		agentServiceConfigUID   types.UID
 		releaseImage            string
 		localClusterNamespace   string
 	)
@@ -49,11 +50,23 @@ var _ = Describe("ImportLocalCluster", func() {
 		// The final layer needs to be encoded/decoded
 		pullSecret = []byte("pullSecret")
 		pullSecretBase64Encoded = []byte(base64.StdEncoding.EncodeToString(pullSecret))
-		agentClusterInstallUID = types.UID(uuid.NewString())
+		agentServiceConfigUID = types.UID(uuid.NewString())
 		releaseImage = "quay.io/openshift-release-dev/ocp-release@sha256:a266d3d65c433b460cdef7ab5d6531580f5391adbe85d9c475208a56452e4c0b"
 
 	})
 
+	var mockGetAgentServiceConfig = func() *gomock.Call {
+		agentServiceConfig := &aiv1beta1.AgentServiceConfig{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "agent",
+				UID:  agentServiceConfigUID,
+			},
+		}
+		return clusterImportOperations.EXPECT().
+			GetAgentServiceConfig().
+			Return(agentServiceConfig, nil)
+	}
+
 	var mockCreateNamespace = func() *gomock.Call {
 		namespace := &v1.Namespace{}
 		namespace.Name = localClusterNamespace
@@ -62,17 +75,6 @@ var _ = Describe("ImportLocalCluster", func() {
 			Return(nil)
 	}
 
-	var mockAgentClusterInstallPresent = func() *gomock.Call {
-		agentClusterInstall := &hiveext.AgentClusterInstall{
-			ObjectMeta: metav1.ObjectMeta{
-				UID: agentClusterInstallUID,
-			},
-		}
-		return clusterImportOperations.EXPECT().
-			GetAgentClusterInstall(localClusterNamespace, fmt.Sprintf("%s-cluster-install", localClusterNamespace)).
-			Return(agentClusterInstall, nil)
-	}
-
 	var mockNoClusterVersionFound = func() *gomock.Call {
 		return clusterImportOperations.EXPECT().
 			GetClusterVersion("version").
@@ -321,10 +323,10 @@ var _ = Describe("ImportLocalCluster", func() {
 		// Adding this ownership reference to ensure we can submit clusterDeployment without ManagedClusterSet/join permission
 		//
 		clusterDeployment.OwnerReferences = []metav1.OwnerReference{{
-			Kind:       "AgentCluster",
-			APIVersion: "capi-provider.agent-install.openshift.io/v1alpha1",
-			Name:       localClusterNamespace + "-cluster-install",
-			UID:        agentClusterInstallUID,
+			Kind:       "AgentServiceConfig",
+			APIVersion: "agent-install.openshift.io/v1beta1",
+			Name:       "agent",
+			UID:        agentServiceConfigUID,
 		}}
 		return clusterDeployment
 	}
@@ -480,7 +482,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockCreateLocalClusterPullSecret()
 		mockCreateLocalClusterDeployment()
 		mockCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		multiErrors := result.(*multierror.Error)
 		apiStatus := multiErrors.Errors[0].(k8serrors.APIStatus)
@@ -500,7 +502,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockCreateLocalClusterPullSecret()
 		mockCreateLocalClusterDeployment()
 		mockCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		multiErrors := result.(*multierror.Error)
 		apiStatus := multiErrors.Errors[0].(k8serrors.APIStatus)
@@ -520,7 +522,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockFailedToCreateLocalClusterPullSecret()
 		mockCreateLocalClusterDeployment()
 		mockCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		multiErrors := result.(*multierror.Error)
 		apiStatus := multiErrors.Errors[0].(k8serrors.APIStatus)
@@ -540,7 +542,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockCreateLocalClusterPullSecret()
 		mockCreateLocalClusterDeployment()
 		mockFailedToCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		multiErrors := result.(*multierror.Error)
 		apiStatus := multiErrors.Errors[0].(k8serrors.APIStatus)
@@ -560,7 +562,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockCreateLocalClusterPullSecret()
 		mockFailedToCreateLocalClusterDeployment()
 		mockCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		multiErrors := result.(*multierror.Error)
 		apiStatus := multiErrors.Errors[0].(k8serrors.APIStatus)
@@ -579,7 +581,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockCreateLocalClusterAdminKubeConfig()
 		mockCreateLocalClusterPullSecret()
 		mockCreateAgentClusterInstall()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		mockCreateLocalClusterDeployment()
 		result := localClusterImport.ImportLocalCluster()
 		Expect(result).To(BeNil())
@@ -597,7 +599,7 @@ var _ = Describe("ImportLocalCluster", func() {
 		mockAlreadyExistsOnCreateLocalClusterPullSecret()
 		mockAlreadyExistsOnCreateAgentClusterInstall()
 		mockAlreadyExistsOnCreateLocalClusterDeployment()
-		mockAgentClusterInstallPresent()
+		mockGetAgentServiceConfig()
 		result := localClusterImport.ImportLocalCluster()
 		Expect(result).To(BeNil())
 	})