From d2e6f42b8493c640e57af49fdd16c543b9085176 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Mon, 3 Apr 2023 15:34:56 -0700 Subject: [PATCH 01/10] Add logic to put cluster labels and package labels on resources for custom cluster bootstrap scenario --- .../clusterbootstrap_controller_test.go | 25 +-- .../clusterbootstrapclone.go | 205 +++++++++--------- 2 files changed, 109 insertions(+), 121 deletions(-) diff --git a/addons/controllers/clusterbootstrap_controller_test.go b/addons/controllers/clusterbootstrap_controller_test.go index 59b09bdee7..e93177cf46 100644 --- a/addons/controllers/clusterbootstrap_controller_test.go +++ b/addons/controllers/clusterbootstrap_controller_test.go @@ -1295,25 +1295,16 @@ var _ = Describe("ClusterBootstrap Reconciler", func() { if err := k8sClient.Get(ctx, key, config); err != nil { return false } - - if len(config.OwnerReferences) > 0 { - return false + ownerRef := metav1.OwnerReference{ + APIVersion: clusterapiv1beta1.GroupVersion.String(), + Kind: "Cluster", + Name: cluster.Name, + UID: cluster.UID, } - - Expect(len(config.OwnerReferences)).Should(Equal(0)) - return true + return clusterapiutil.HasOwnerRef(config.OwnerReferences, ownerRef) }, waitTimeout, pollingInterval).Should(BeTrue()) - patchedSecret := config.DeepCopy() - ownerRef := metav1.OwnerReference{ - APIVersion: clusterapiv1beta1.GroupVersion.String(), - Kind: "Cluster", - Name: cluster.Name, - UID: cluster.UID, - } - - patchedSecret.OwnerReferences = clusterapiutil.EnsureOwnerRef(patchedSecret.OwnerReferences, ownerRef) - Expect(k8sClient.Patch(ctx, patchedSecret, client.MergeFrom(config))).ShouldNot(HaveOccurred()) + Expect(config.ObjectMeta.Labels["tkg.tanzu.vmware.com/package-name"]).Should(Equal("load-balancer-and-ingress-service.tanzu.vmware.com.0.0.4")) By("ClusterBootstrap CR is created with correct ownerReference added") // Verify ownerReference for cluster in cloned object @@ -1350,7 +1341,7 @@ var _ = Describe("ClusterBootstrap Reconciler", func() { deleteOptions := client.DeleteOptions{PropagationPolicy: &deletePropagation} Expect(k8sClient.Delete(ctx, cluster, &deleteOptions)).To(Succeed()) - By("instacllpackages for additional packages should have been removed.") + By("installpackages for additional packages should have been removed.") Eventually(func() bool { return hasPackageInstalls(ctx, k8sClient, cluster, constants.TKGSystemNS, clusterBootstrap.Spec.AdditionalPackages, clusterBootstrap.Annotations, setupLog) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 7884541863..71e470cb95 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -355,24 +355,6 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph return nil, err } - // Update the packages slice to make it only contains the ones we want the rest of the code to do the cloning. - // For cloning, we only care about the ClusterBootstrap packages which have no valuesFrom originally. The missing - // valuesFrom field will be added by AddMissingSpecFieldsFromTemplate() in few lines below, and we want to clone - // those packages from system namespace. - var nilValuesFromPackages []*runtanzuv1alpha3.ClusterBootstrapPackage - for _, cbPackage := range packages { - if cbPackage != nil && cbPackage.ValuesFrom == nil { - nilValuesFromPackages = append(nilValuesFromPackages, cbPackage) - } - } - - var nonEmptyInlinePackages []*runtanzuv1alpha3.ClusterBootstrapPackage - for _, cbPackage := range packages { - if cbPackage != nil && cbPackage.ValuesFrom != nil && cbPackage.ValuesFrom.Inline != nil { - nonEmptyInlinePackages = append(nonEmptyInlinePackages, cbPackage) - } - } - if _, ok := clusterBootstrap.Annotations[constants.UnmanagedCNI]; ok { clusterBootstrapTemplate.Spec.CNI = clusterBootstrap.Spec.CNI } @@ -393,39 +375,7 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph clusterBootstrap.Spec.Kapp, }, clusterBootstrap.Spec.AdditionalPackages...) - var packagesToBeCloned []*runtanzuv1alpha3.ClusterBootstrapPackage - for _, nilValuesFromPackage := range nilValuesFromPackages { - for _, updatedCBPackage := range updatedCBPackages { - // If the nilValuesFromPackage has been filled by AddMissingSpecFieldsFromTemplate(), it is the package - // we want to record and do cloning. - if updatedCBPackage != nil && updatedCBPackage.ValuesFrom != nil && updatedCBPackage.RefName == nilValuesFromPackage.RefName { - packagesToBeCloned = append(packagesToBeCloned, updatedCBPackage) - } - } - } - h.Logger.Info("Updating packagesToBeCloned with inline packages") - packagesToBeCloned = append(packagesToBeCloned, nonEmptyInlinePackages...) - packages = packagesToBeCloned - } else { - // packages with values from inline need to be cloned for two cases: - // - // 1. When no Clusterbootstrap is given by user and clusterbootstraptemplate - // is cloned as clusterbootstrap but resolved tkr is not set - In this - // case the providers are already cloned so providers do not have to be cloned again - // - // 2. When full clusterbootstrap without tkr annotation is given by user. - // In this case we only want to handle inline packages for creating - // secret. The providers are expected to be present in the - // clusternamespace, created by user before clusterbootstrap is created. - var nonEmptyInlinePackages []*runtanzuv1alpha3.ClusterBootstrapPackage - for _, cbPackage := range packages { - if cbPackage != nil && cbPackage.ValuesFrom != nil && cbPackage.ValuesFrom.Inline != nil { - nonEmptyInlinePackages = append(nonEmptyInlinePackages, cbPackage) - } - } - var packagesToBeCloned []*runtanzuv1alpha3.ClusterBootstrapPackage - packagesToBeCloned = append(packagesToBeCloned, nonEmptyInlinePackages...) - packages = packagesToBeCloned + packages = updatedCBPackages } secrets, _, err := h.CloneReferencedObjectsFromCBPackages(cluster, packages, systemNamespace) @@ -434,7 +384,7 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph return nil, err } - // We need to verify that all the listed prvoiders in the clusterbootstrap with fromRef exist + // We need to verify that all the listed providers in the clusterbootstrap with fromRef exist providers, err := h.verifyProvidersFromRefExist(clusterBootstrap) if err != nil { h.Logger.Error(err, "could not verify all providers listed with providerRef in clusterbootstrap exists") @@ -468,7 +418,7 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph return nil, err } - // all providers have the clusterboostrap added as controller owner reference. + // all providers have the clusterbootstrap added as controller owner reference. if err := h.EnsureOwnerRef(clusterBootstrap, secrets, providers); err != nil { h.Logger.Error(err, fmt.Sprintf("unable to ensure ClusterBootstrap %s/%s as a ownerRef on created secrets and providers", clusterBootstrap.Namespace, clusterBootstrap.Name)) return nil, err @@ -729,20 +679,33 @@ func (h *Helper) cloneSecretRef( return nil, nil } + customSecret := false secret := &corev1.Secret{} key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { - h.Logger.Error(err, "unable to fetch secret", "objectkey", key) - return nil, err + // Look for secret in cluster namespace for customer cluster bootstrap + customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { + h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) + return nil, err + } else { + customSecret = true + } } - newSecret := secret.DeepCopy() - newSecret.ObjectMeta.Reset() - newSecret.Name = util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName) - newSecret.Namespace = cluster.Namespace + createOrPatchSecret := &corev1.Secret{} + if customSecret { + createOrPatchSecret = secret + } else { + newSecret := secret.DeepCopy() + newSecret.ObjectMeta.Reset() + newSecret.Name = util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName) + newSecret.Namespace = cluster.Namespace + createOrPatchSecret = newSecret + } - opResult, createOrPatchErr := controllerutil.CreateOrPatch(h.Ctx, h.K8sClient, newSecret, func() error { - newSecret.OwnerReferences = []metav1.OwnerReference{ + opResult, createOrPatchErr := controllerutil.CreateOrPatch(h.Ctx, h.K8sClient, createOrPatchSecret, func() error { + createOrPatchSecret.OwnerReferences = []metav1.OwnerReference{ { APIVersion: clusterapiv1beta1.GroupVersion.String(), Kind: cluster.Kind, @@ -752,22 +715,25 @@ func (h *Helper) cloneSecretRef( } // Add cluster and package labels to cloned secrets - if newSecret.Labels == nil { - newSecret.Labels = map[string]string{} + if createOrPatchSecret.Labels == nil { + createOrPatchSecret.Labels = map[string]string{} + } + createOrPatchSecret.Labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cbPkg.RefName) + createOrPatchSecret.Labels[addontypes.ClusterNameLabel] = cluster.Name + + if !customSecret { + // Set secret.Type to ClusterBootstrapManagedSecret to enable clusterbootstrap_controller to Watch these secrets + createOrPatchSecret.Type = constants.ClusterBootstrapManagedSecret } - newSecret.Labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cbPkg.RefName) - newSecret.Labels[addontypes.ClusterNameLabel] = cluster.Name - // Set secret.Type to ClusterBootstrapManagedSecret to enable clusterbootstrap_controller to Watch these secrets - newSecret.Type = constants.ClusterBootstrapManagedSecret return nil }) if createOrPatchErr != nil { return nil, createOrPatchErr } - h.Logger.Info(fmt.Sprintf("Secret %s/%s %s", newSecret.Namespace, newSecret.Name, opResult)) - cbPkg.ValuesFrom.SecretRef = newSecret.Name + h.Logger.Info(fmt.Sprintf("Secret %s/%s %s", createOrPatchSecret.Namespace, createOrPatchSecret.Name, opResult)) + cbPkg.ValuesFrom.SecretRef = createOrPatchSecret.Name - return newSecret, nil + return createOrPatchSecret, nil } // cloneProviderRef is an internal function clones the referenced objects of a single ClusterBootstrapPackage.ValuesFrom.ProviderRef @@ -789,11 +755,74 @@ func (h *Helper) cloneProviderRef( h.Logger.Error(err, "failed to getGVR") return nil, err } + isCustomCB := false provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s/%s", sourceNamespace, cbPkg.ValuesFrom.ProviderRef.Name), "gvr", gvr) + // Look in cluster namespace for custom bootstrap configs + provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) + return nil, err + } else { + isCustomCB = true + } + } + + newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) + patchExistingProvider := false + if !isCustomCB { + h.Logger.Info(fmt.Sprintf("cloning provider %s/%s to namespace %s", sourceNamespace, newProvider.GetName(), cluster.Namespace), "gvr", gvr) + createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Create(h.Ctx, newProvider, metav1.CreateOptions{}) + if err != nil { + if apierrors.IsAlreadyExists(err) { + h.Logger.Info(fmt.Sprintf("provider %s/%s already exist, patching its Labels and OwnerReferences fields", newProvider.GetNamespace(), newProvider.GetName())) + patchExistingProvider = true + } else { + h.Logger.Error(err, fmt.Sprintf("unable to clone provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) + return nil, err + } + } + } + if isCustomCB || patchExistingProvider { + // Instantiate an empty unstructured and only set ownerReferences and Labels for patching + patchObj := unstructured.Unstructured{} + patchObj.SetLabels(newProvider.GetLabels()) + patchObj.SetOwnerReferences(newProvider.GetOwnerReferences()) + patchData, err := patchObj.MarshalJSON() + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) + return nil, err + } + var providerName string + if isCustomCB { + providerName = provider.GetName() + } else { + providerName = newProvider.GetName() + } + createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Patch(h.Ctx, providerName, types.MergePatchType, patchData, metav1.PatchOptions{}) + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to update provider %s/%s", cluster.Namespace, providerName), "gvr", gvr) + return nil, err + } + } + + cbPkg.ValuesFrom.ProviderRef.Name = createdOrUpdatedProvider.GetName() + h.Logger.Info(fmt.Sprintf("cloned provider %s/%s to namespace %s", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName(), cluster.Namespace), "gvr", gvr) + + if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { return nil, err } + + return createdOrUpdatedProvider, nil +} + +func (h *Helper) createNewProviderToClone( + cluster *clusterapiv1beta1.Cluster, + provider *unstructured.Unstructured, + cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage, + carvelPkgRefName string) *unstructured.Unstructured { + + var newProvider *unstructured.Unstructured newProvider = provider.DeepCopy() unstructured.RemoveNestedField(newProvider.Object, "metadata") newProvider.SetOwnerReferences([]metav1.OwnerReference{ @@ -823,39 +852,7 @@ func (h *Helper) cloneProviderRef( newProvider.SetName(util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName)) newProvider.SetNamespace(cluster.Namespace) - h.Logger.Info(fmt.Sprintf("cloning provider %s/%s to namespace %s", sourceNamespace, newProvider.GetName(), cluster.Namespace), "gvr", gvr) - createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Create(h.Ctx, newProvider, metav1.CreateOptions{}) - if err != nil { - if apierrors.IsAlreadyExists(err) { - h.Logger.Info(fmt.Sprintf("provider %s/%s already exist, patching its Labels and OwnerReferences fields", newProvider.GetNamespace(), newProvider.GetName())) - // Instantiate an empty unstructured and only set ownerReferences and Labels for patching - patchObj := unstructured.Unstructured{} - patchObj.SetLabels(newProvider.GetLabels()) - patchObj.SetOwnerReferences(newProvider.GetOwnerReferences()) - patchData, err := patchObj.MarshalJSON() - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) - return nil, err - } - createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Patch(h.Ctx, newProvider.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{}) - if err != nil { - h.Logger.Info(fmt.Sprintf("unable to update provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) - return nil, err - } - } else { - h.Logger.Error(err, fmt.Sprintf("unable to clone provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) - return nil, err - } - } - - cbPkg.ValuesFrom.ProviderRef.Name = createdOrUpdatedProvider.GetName() - h.Logger.Info(fmt.Sprintf("cloned provider %s/%s to namespace %s", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName(), cluster.Namespace), "gvr", gvr) - - if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { - return nil, err - } - - return createdOrUpdatedProvider, nil + return newProvider } // cloneEmbeddedLocalObjectRef is an internal function attempts to clone the embedded local object references from provider's namespace to cluster's From e4af225534aae047eee343f74895fd1615b9bd63 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Tue, 4 Apr 2023 22:29:43 -0700 Subject: [PATCH 02/10] Fixed review comments --- .../clusterbootstrapclone.go | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 71e470cb95..2a7c041622 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -683,13 +683,18 @@ func (h *Helper) cloneSecretRef( secret := &corev1.Secret{} key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { - // Look for secret in cluster namespace for customer cluster bootstrap - customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} - if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { - h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) - return nil, err + if apierrors.IsNotFound(err) { + // Look for secret in cluster namespace for custom cluster bootstrap + customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { + h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) + return nil, err + } else { + customSecret = true + } } else { - customSecret = true + h.Logger.Error(err, "unable to fetch secret %s", key) + return nil, err } } @@ -758,13 +763,18 @@ func (h *Helper) cloneProviderRef( isCustomCB := false provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { - // Look in cluster namespace for custom bootstrap configs - provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) - return nil, err + if apierrors.IsNotFound(err) { + // Look in cluster namespace for custom bootstrap configs + provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) + return nil, err + } else { + isCustomCB = true + } } else { - isCustomCB = true + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) + return nil, err } } From b260f3ea19dc29f195dd5bb98d7c21dd7099688b Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Thu, 6 Apr 2023 15:23:44 -0700 Subject: [PATCH 03/10] Trying out pipeline with first checking in cluster namespace for custom cluster bootstrap case --- .../clusterbootstrapclone.go | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 2a7c041622..bff90444cd 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -681,21 +681,21 @@ func (h *Helper) cloneSecretRef( customSecret := false secret := &corev1.Secret{} - key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} - if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { + // Look for secret in cluster namespace for custom cluster bootstrap + customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + if err := h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { if apierrors.IsNotFound(err) { - // Look for secret in cluster namespace for custom cluster bootstrap - customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} - if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { + key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} + if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) return nil, err - } else { - customSecret = true } } else { - h.Logger.Error(err, "unable to fetch secret %s", key) + h.Logger.Error(err, "unable to fetch secret %s", customSecretKey) return nil, err } + } else { + customSecret = true } createOrPatchSecret := &corev1.Secret{} @@ -761,21 +761,21 @@ func (h *Helper) cloneProviderRef( return nil, err } isCustomCB := false - provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + // Look in cluster namespace for custom bootstrap configs + provider, err := h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - // Look in cluster namespace for custom bootstrap configs - provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + provider, err = h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) return nil, err - } else { - isCustomCB = true } } else { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, cluster.Namespace), "gvr", gvr) return nil, err } + } else { + isCustomCB = true } newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) From e8af2a731ffac159de2ff0079d59ab3a2b8d8ab9 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Thu, 6 Apr 2023 22:17:17 -0700 Subject: [PATCH 04/10] Reverting recent change --- .../clusterbootstrapclone.go | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index bff90444cd..2a7c041622 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -681,21 +681,21 @@ func (h *Helper) cloneSecretRef( customSecret := false secret := &corev1.Secret{} - // Look for secret in cluster namespace for custom cluster bootstrap - customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} - if err := h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { + key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} + if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { if apierrors.IsNotFound(err) { - key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} - if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { + // Look for secret in cluster namespace for custom cluster bootstrap + customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) return nil, err + } else { + customSecret = true } } else { - h.Logger.Error(err, "unable to fetch secret %s", customSecretKey) + h.Logger.Error(err, "unable to fetch secret %s", key) return nil, err } - } else { - customSecret = true } createOrPatchSecret := &corev1.Secret{} @@ -761,21 +761,21 @@ func (h *Helper) cloneProviderRef( return nil, err } isCustomCB := false - // Look in cluster namespace for custom bootstrap configs - provider, err := h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { - provider, err = h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + // Look in cluster namespace for custom bootstrap configs + provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) return nil, err + } else { + isCustomCB = true } } else { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, cluster.Namespace), "gvr", gvr) + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) return nil, err } - } else { - isCustomCB = true } newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) From c0be264c4a230a3464094309b628307b89e53ee4 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Fri, 7 Apr 2023 08:42:20 -0700 Subject: [PATCH 05/10] Trying out new changes for custom CB --- .../testdata/test-cluster-bootstrap-9.yaml | 11 --- .../clusterbootstrapclone.go | 85 +++++++++++++------ 2 files changed, 61 insertions(+), 35 deletions(-) diff --git a/addons/controllers/testdata/test-cluster-bootstrap-9.yaml b/addons/controllers/testdata/test-cluster-bootstrap-9.yaml index 06e672c163..42be345352 100644 --- a/addons/controllers/testdata/test-cluster-bootstrap-9.yaml +++ b/addons/controllers/testdata/test-cluster-bootstrap-9.yaml @@ -1044,17 +1044,6 @@ stringData: identity_management_type: none tkg_cluster_role: workload --- -apiVersion: v1 -kind: Secret -metadata: - name: default-pinniped-config-v1.23.5---vmware.1-tkg.1 - namespace: cluster-namespace-9 -stringData: - values.yaml: | - infrastructure_provider: vsphere - identity_management_type: none - tkg_cluster_role: workload ---- apiVersion: cni.tanzu.vmware.com/v1alpha1 kind: CalicoConfig metadata: diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 2a7c041622..5723d688dd 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -680,27 +680,45 @@ func (h *Helper) cloneSecretRef( } customSecret := false + secretInSrcNs, secretInClusterNs := false, false secret := &corev1.Secret{} + clusterNsSecret := &corev1.Secret{} + var clusterNsSecretKey client.ObjectKey key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} - if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { - if apierrors.IsNotFound(err) { - // Look for secret in cluster namespace for custom cluster bootstrap - customSecretKey := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} - if err = h.K8sClient.Get(h.Ctx, customSecretKey, secret); err != nil { - h.Logger.Error(err, "unable to fetch secret %s or %s", key, customSecretKey) + err := h.K8sClient.Get(h.Ctx, key, secret) + if err != nil { + if !apierrors.IsNotFound(err) { + h.Logger.Error(err, "unable to fetch secret %s", key) + return nil, err + } + } else { + secretInSrcNs = true + } + if sourceNamespace != cluster.Namespace { + // Look for secret in cluster namespace for custom cluster bootstrap + clusterNsSecretKey = client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + err = h.K8sClient.Get(h.Ctx, clusterNsSecretKey, clusterNsSecret) + if err != nil { + if !apierrors.IsNotFound(err) { + h.Logger.Error(err, "unable to fetch secret %s", clusterNsSecretKey) return nil, err - } else { - customSecret = true } } else { - h.Logger.Error(err, "unable to fetch secret %s", key) - return nil, err + secretInClusterNs = true } } + if !secretInSrcNs && !secretInClusterNs { + h.Logger.Error(err, "unable to fetch secret %s or %s", key, clusterNsSecretKey) + return nil, err + } else if secretInClusterNs { + // custom clusterbootstrap case + customSecret = true + } + createOrPatchSecret := &corev1.Secret{} if customSecret { - createOrPatchSecret = secret + createOrPatchSecret = clusterNsSecret } else { newSecret := secret.DeepCopy() newSecret.ObjectMeta.Reset() @@ -754,6 +772,7 @@ func (h *Helper) cloneProviderRef( } var newProvider *unstructured.Unstructured + var clusterProvider *unstructured.Unstructured var createdOrUpdatedProvider *unstructured.Unstructured gvr, err := h.GVRHelper.GetGVR(schema.GroupKind{Group: *cbPkg.ValuesFrom.ProviderRef.APIGroup, Kind: cbPkg.ValuesFrom.ProviderRef.Kind}) if err != nil { @@ -761,24 +780,40 @@ func (h *Helper) cloneProviderRef( return nil, err } isCustomCB := false + providerInSrcNs, providerInClusterNs := false, false + provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { - if apierrors.IsNotFound(err) { - // Look in cluster namespace for custom bootstrap configs - provider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) + if !apierrors.IsNotFound(err) { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) + return nil, err + } + } else { + providerInSrcNs = true + } + if sourceNamespace != cluster.Namespace { + // Look in cluster namespace for custom bootstrap configs + clusterProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + if err != nil { + if !apierrors.IsNotFound(err) { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, cluster.Namespace), "gvr", gvr) return nil, err - } else { - isCustomCB = true } } else { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) - return nil, err + providerInClusterNs = true } } + if !providerInSrcNs && !providerInClusterNs { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) + return nil, err + } else if providerInClusterNs { + // custom clusterbootstrap case + isCustomCB = true + newProvider = h.createNewProviderToClone(cluster, clusterProvider, cbPkg, carvelPkgRefName) + } else { + newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) + } - newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) patchExistingProvider := false if !isCustomCB { h.Logger.Info(fmt.Sprintf("cloning provider %s/%s to namespace %s", sourceNamespace, newProvider.GetName(), cluster.Namespace), "gvr", gvr) @@ -805,7 +840,7 @@ func (h *Helper) cloneProviderRef( } var providerName string if isCustomCB { - providerName = provider.GetName() + providerName = clusterProvider.GetName() } else { providerName = newProvider.GetName() } @@ -819,8 +854,10 @@ func (h *Helper) cloneProviderRef( cbPkg.ValuesFrom.ProviderRef.Name = createdOrUpdatedProvider.GetName() h.Logger.Info(fmt.Sprintf("cloned provider %s/%s to namespace %s", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName(), cluster.Namespace), "gvr", gvr) - if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { - return nil, err + if !isCustomCB { + if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { + return nil, err + } } return createdOrUpdatedProvider, nil From 6bfa446363918c0fef3c1f09bd00e236c0bdf7d8 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Fri, 7 Apr 2023 16:59:38 -0700 Subject: [PATCH 06/10] Take 2 updating labels on existing providers --- .../clusterbootstrapclone.go | 378 +++++++++++------- 1 file changed, 230 insertions(+), 148 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 5723d688dd..658eab24bd 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -346,6 +346,9 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph clusterBootstrap.Spec.Kapp, }, clusterBootstrap.Spec.AdditionalPackages...) + var packagesToBeCloned []*runtanzuv1alpha3.ClusterBootstrapPackage + var packagesToBeUpdated []*runtanzuv1alpha3.ClusterBootstrapPackage + if annotationValue, annotationExist := clusterBootstrap.Annotations[constants.AddCBMissingFieldsAnnotationKey]; annotationExist { tkrName = annotationValue clusterBootstrapTemplateName := annotationValue // TanzuKubernetesRelease and ClusterBootstrapTemplate share the same name @@ -355,6 +358,24 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph return nil, err } + // Update the packages slice to make it only contains the ones we want the rest of the code to do the cloning. + // For cloning, we only care about the ClusterBootstrap packages which have no valuesFrom originally. The missing + // valuesFrom field will be added by AddMissingSpecFieldsFromTemplate() in few lines below, and we want to clone + // those packages from system namespace. + var nilValuesFromPackages []*runtanzuv1alpha3.ClusterBootstrapPackage + for _, cbPackage := range packages { + if cbPackage != nil && cbPackage.ValuesFrom == nil { + nilValuesFromPackages = append(nilValuesFromPackages, cbPackage) + } + } + + var nonEmptyInlinePackages []*runtanzuv1alpha3.ClusterBootstrapPackage + for _, cbPackage := range packages { + if cbPackage != nil && cbPackage.ValuesFrom != nil && cbPackage.ValuesFrom.Inline != nil { + nonEmptyInlinePackages = append(nonEmptyInlinePackages, cbPackage) + } + } + if _, ok := clusterBootstrap.Annotations[constants.UnmanagedCNI]; ok { clusterBootstrapTemplate.Spec.CNI = clusterBootstrap.Spec.CNI } @@ -375,16 +396,67 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph clusterBootstrap.Spec.Kapp, }, clusterBootstrap.Spec.AdditionalPackages...) + for _, nilValuesFromPackage := range nilValuesFromPackages { + for _, updatedCBPackage := range updatedCBPackages { + // If the nilValuesFromPackage has been filled by AddMissingSpecFieldsFromTemplate(), it is the package + // we want to record and do cloning. + if updatedCBPackage != nil && updatedCBPackage.ValuesFrom != nil && updatedCBPackage.RefName == nilValuesFromPackage.RefName { + packagesToBeCloned = append(packagesToBeCloned, updatedCBPackage) + } + } + } + h.Logger.Info("Updating packagesToBeCloned with inline packages") + packagesToBeCloned = append(packagesToBeCloned, nonEmptyInlinePackages...) packages = updatedCBPackages + } else { + // packages with values from inline need to be cloned for two cases: + // + // 1. When no Clusterbootstrap is given by user and clusterbootstraptemplate + // is cloned as clusterbootstrap but resolved tkr is not set - In this + // case the providers are already cloned so providers do not have to be cloned again + // + // 2. When full clusterbootstrap without tkr annotation is given by user. + // In this case we only want to handle inline packages for creating + // secret. The providers are expected to be present in the + // clusternamespace, created by user before clusterbootstrap is created. + var nonEmptyInlinePackages []*runtanzuv1alpha3.ClusterBootstrapPackage + for _, cbPackage := range packages { + if cbPackage != nil && cbPackage.ValuesFrom != nil && cbPackage.ValuesFrom.Inline != nil { + nonEmptyInlinePackages = append(nonEmptyInlinePackages, cbPackage) + } + } + packagesToBeCloned = append(packagesToBeCloned, nonEmptyInlinePackages...) + } + + // find the packages that don't need to be cloned by updated only + var toBeCloned bool + for _, cbPackage := range packages { + toBeCloned = false + for _, clonePackage := range packagesToBeCloned { + if cbPackage == clonePackage { + toBeCloned = true + } + } + if !toBeCloned { + packagesToBeUpdated = append(packagesToBeUpdated, cbPackage) + } } - secrets, _, err := h.CloneReferencedObjectsFromCBPackages(cluster, packages, systemNamespace) + // Clone the required objects + secrets, _, err := h.CloneReferencedObjectsFromCBPackages(cluster, packagesToBeCloned, systemNamespace) if err != nil { h.Logger.Error(err, "unable to clone secrets or providers") return nil, err } - // We need to verify that all the listed providers in the clusterbootstrap with fromRef exist + // Update the existing objects to ensure labels are present for cluster bootstrap + err = h.updateProvidersAndSecretsLabels(cluster, packagesToBeUpdated) + if err != nil { + h.Logger.Error(err, "unable to update labels on secrets or providers") + return nil, err + } + + // We need to verify that all the listed prvoiders in the clusterbootstrap with fromRef exist providers, err := h.verifyProvidersFromRefExist(clusterBootstrap) if err != nil { h.Logger.Error(err, "could not verify all providers listed with providerRef in clusterbootstrap exists") @@ -418,7 +490,7 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph return nil, err } - // all providers have the clusterbootstrap added as controller owner reference. + // all providers have the clusterboostrap added as controller owner reference. if err := h.EnsureOwnerRef(clusterBootstrap, secrets, providers); err != nil { h.Logger.Error(err, fmt.Sprintf("unable to ensure ClusterBootstrap %s/%s as a ownerRef on created secrets and providers", clusterBootstrap.Namespace, clusterBootstrap.Name)) return nil, err @@ -679,56 +751,20 @@ func (h *Helper) cloneSecretRef( return nil, nil } - customSecret := false - secretInSrcNs, secretInClusterNs := false, false secret := &corev1.Secret{} - clusterNsSecret := &corev1.Secret{} - var clusterNsSecretKey client.ObjectKey key := client.ObjectKey{Namespace: sourceNamespace, Name: cbPkg.ValuesFrom.SecretRef} - err := h.K8sClient.Get(h.Ctx, key, secret) - if err != nil { - if !apierrors.IsNotFound(err) { - h.Logger.Error(err, "unable to fetch secret %s", key) - return nil, err - } - } else { - secretInSrcNs = true - } - if sourceNamespace != cluster.Namespace { - // Look for secret in cluster namespace for custom cluster bootstrap - clusterNsSecretKey = client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} - err = h.K8sClient.Get(h.Ctx, clusterNsSecretKey, clusterNsSecret) - if err != nil { - if !apierrors.IsNotFound(err) { - h.Logger.Error(err, "unable to fetch secret %s", clusterNsSecretKey) - return nil, err - } - } else { - secretInClusterNs = true - } - } - - if !secretInSrcNs && !secretInClusterNs { - h.Logger.Error(err, "unable to fetch secret %s or %s", key, clusterNsSecretKey) + if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { + h.Logger.Error(err, "unable to fetch secret", "objectkey", key) return nil, err - } else if secretInClusterNs { - // custom clusterbootstrap case - customSecret = true } - createOrPatchSecret := &corev1.Secret{} - if customSecret { - createOrPatchSecret = clusterNsSecret - } else { - newSecret := secret.DeepCopy() - newSecret.ObjectMeta.Reset() - newSecret.Name = util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName) - newSecret.Namespace = cluster.Namespace - createOrPatchSecret = newSecret - } + newSecret := secret.DeepCopy() + newSecret.ObjectMeta.Reset() + newSecret.Name = util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName) + newSecret.Namespace = cluster.Namespace - opResult, createOrPatchErr := controllerutil.CreateOrPatch(h.Ctx, h.K8sClient, createOrPatchSecret, func() error { - createOrPatchSecret.OwnerReferences = []metav1.OwnerReference{ + opResult, createOrPatchErr := controllerutil.CreateOrPatch(h.Ctx, h.K8sClient, newSecret, func() error { + newSecret.OwnerReferences = []metav1.OwnerReference{ { APIVersion: clusterapiv1beta1.GroupVersion.String(), Kind: cluster.Kind, @@ -738,25 +774,22 @@ func (h *Helper) cloneSecretRef( } // Add cluster and package labels to cloned secrets - if createOrPatchSecret.Labels == nil { - createOrPatchSecret.Labels = map[string]string{} - } - createOrPatchSecret.Labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cbPkg.RefName) - createOrPatchSecret.Labels[addontypes.ClusterNameLabel] = cluster.Name - - if !customSecret { - // Set secret.Type to ClusterBootstrapManagedSecret to enable clusterbootstrap_controller to Watch these secrets - createOrPatchSecret.Type = constants.ClusterBootstrapManagedSecret + if newSecret.Labels == nil { + newSecret.Labels = map[string]string{} } + newSecret.Labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cbPkg.RefName) + newSecret.Labels[addontypes.ClusterNameLabel] = cluster.Name + // Set secret.Type to ClusterBootstrapManagedSecret to enable clusterbootstrap_controller to Watch these secrets + newSecret.Type = constants.ClusterBootstrapManagedSecret return nil }) if createOrPatchErr != nil { return nil, createOrPatchErr } - h.Logger.Info(fmt.Sprintf("Secret %s/%s %s", createOrPatchSecret.Namespace, createOrPatchSecret.Name, opResult)) - cbPkg.ValuesFrom.SecretRef = createOrPatchSecret.Name + h.Logger.Info(fmt.Sprintf("Secret %s/%s %s", newSecret.Namespace, newSecret.Name, opResult)) + cbPkg.ValuesFrom.SecretRef = newSecret.Name - return createOrPatchSecret, nil + return newSecret, nil } // cloneProviderRef is an internal function clones the referenced objects of a single ClusterBootstrapPackage.ValuesFrom.ProviderRef @@ -772,104 +805,17 @@ func (h *Helper) cloneProviderRef( } var newProvider *unstructured.Unstructured - var clusterProvider *unstructured.Unstructured var createdOrUpdatedProvider *unstructured.Unstructured gvr, err := h.GVRHelper.GetGVR(schema.GroupKind{Group: *cbPkg.ValuesFrom.ProviderRef.APIGroup, Kind: cbPkg.ValuesFrom.ProviderRef.Kind}) if err != nil { h.Logger.Error(err, "failed to getGVR") return nil, err } - isCustomCB := false - providerInSrcNs, providerInClusterNs := false, false - provider, err := h.DynamicClient.Resource(*gvr).Namespace(sourceNamespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) if err != nil { - if !apierrors.IsNotFound(err) { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace), "gvr", gvr) - return nil, err - } - } else { - providerInSrcNs = true - } - if sourceNamespace != cluster.Namespace { - // Look in cluster namespace for custom bootstrap configs - clusterProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) - if err != nil { - if !apierrors.IsNotFound(err) { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespace %s", cbPkg.ValuesFrom.ProviderRef.Name, cluster.Namespace), "gvr", gvr) - return nil, err - } - } else { - providerInClusterNs = true - } - } - if !providerInSrcNs && !providerInClusterNs { - h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s in namespaces %s or %s", cbPkg.ValuesFrom.ProviderRef.Name, sourceNamespace, cluster.Namespace), "gvr", gvr) + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s/%s", sourceNamespace, cbPkg.ValuesFrom.ProviderRef.Name), "gvr", gvr) return nil, err - } else if providerInClusterNs { - // custom clusterbootstrap case - isCustomCB = true - newProvider = h.createNewProviderToClone(cluster, clusterProvider, cbPkg, carvelPkgRefName) - } else { - newProvider = h.createNewProviderToClone(cluster, provider, cbPkg, carvelPkgRefName) } - - patchExistingProvider := false - if !isCustomCB { - h.Logger.Info(fmt.Sprintf("cloning provider %s/%s to namespace %s", sourceNamespace, newProvider.GetName(), cluster.Namespace), "gvr", gvr) - createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Create(h.Ctx, newProvider, metav1.CreateOptions{}) - if err != nil { - if apierrors.IsAlreadyExists(err) { - h.Logger.Info(fmt.Sprintf("provider %s/%s already exist, patching its Labels and OwnerReferences fields", newProvider.GetNamespace(), newProvider.GetName())) - patchExistingProvider = true - } else { - h.Logger.Error(err, fmt.Sprintf("unable to clone provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) - return nil, err - } - } - } - if isCustomCB || patchExistingProvider { - // Instantiate an empty unstructured and only set ownerReferences and Labels for patching - patchObj := unstructured.Unstructured{} - patchObj.SetLabels(newProvider.GetLabels()) - patchObj.SetOwnerReferences(newProvider.GetOwnerReferences()) - patchData, err := patchObj.MarshalJSON() - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) - return nil, err - } - var providerName string - if isCustomCB { - providerName = clusterProvider.GetName() - } else { - providerName = newProvider.GetName() - } - createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Patch(h.Ctx, providerName, types.MergePatchType, patchData, metav1.PatchOptions{}) - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to update provider %s/%s", cluster.Namespace, providerName), "gvr", gvr) - return nil, err - } - } - - cbPkg.ValuesFrom.ProviderRef.Name = createdOrUpdatedProvider.GetName() - h.Logger.Info(fmt.Sprintf("cloned provider %s/%s to namespace %s", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName(), cluster.Namespace), "gvr", gvr) - - if !isCustomCB { - if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { - return nil, err - } - } - - return createdOrUpdatedProvider, nil -} - -func (h *Helper) createNewProviderToClone( - cluster *clusterapiv1beta1.Cluster, - provider *unstructured.Unstructured, - cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage, - carvelPkgRefName string) *unstructured.Unstructured { - - var newProvider *unstructured.Unstructured newProvider = provider.DeepCopy() unstructured.RemoveNestedField(newProvider.Object, "metadata") newProvider.SetOwnerReferences([]metav1.OwnerReference{ @@ -899,7 +845,143 @@ func (h *Helper) createNewProviderToClone( newProvider.SetName(util.GeneratePackageSecretName(cluster.Name, carvelPkgRefName)) newProvider.SetNamespace(cluster.Namespace) - return newProvider + h.Logger.Info(fmt.Sprintf("cloning provider %s/%s to namespace %s", sourceNamespace, newProvider.GetName(), cluster.Namespace), "gvr", gvr) + createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Create(h.Ctx, newProvider, metav1.CreateOptions{}) + if err != nil { + if apierrors.IsAlreadyExists(err) { + h.Logger.Info(fmt.Sprintf("provider %s/%s already exist, patching its Labels and OwnerReferences fields", newProvider.GetNamespace(), newProvider.GetName())) + // Instantiate an empty unstructured and only set ownerReferences and Labels for patching + patchObj := unstructured.Unstructured{} + patchObj.SetLabels(newProvider.GetLabels()) + patchObj.SetOwnerReferences(newProvider.GetOwnerReferences()) + patchData, err := patchObj.MarshalJSON() + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) + return nil, err + } + createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Patch(h.Ctx, newProvider.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{}) + if err != nil { + h.Logger.Info(fmt.Sprintf("unable to update provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) + return nil, err + } + } else { + h.Logger.Error(err, fmt.Sprintf("unable to clone provider %s/%s", newProvider.GetNamespace(), newProvider.GetName()), "gvr", gvr) + return nil, err + } + } + + cbPkg.ValuesFrom.ProviderRef.Name = createdOrUpdatedProvider.GetName() + h.Logger.Info(fmt.Sprintf("cloned provider %s/%s to namespace %s", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName(), cluster.Namespace), "gvr", gvr) + + if err := h.cloneEmbeddedLocalObjectRef(cluster, provider); err != nil { + return nil, err + } + + return createdOrUpdatedProvider, nil +} + +// updateProvidersAndSecretsLabels ensure that all referenced providers and secrets in the given packages have appropriate labels +// as required for cluster bootstrap +func (h *Helper) updateProvidersAndSecretsLabels( + cluster *clusterapiv1beta1.Cluster, + cbPackages []*runtanzuv1alpha3.ClusterBootstrapPackage) error { + + for _, cbPackage := range cbPackages { + if cbPackage == nil { + continue + } + + if cbPackage.ValuesFrom.SecretRef != "" { + err := h.updateSecretLabels(cluster, cbPackage) + if err != nil { + return err + } + } + + if cbPackage.ValuesFrom.ProviderRef != nil { + err := h.updateProviderLabels(cluster, cbPackage) + if err != nil { + return err + } + } + } + return nil +} + +// updateSecretLabels is an internal function that updates the labels on the provided secret +func (h *Helper) updateSecretLabels( + cluster *clusterapiv1beta1.Cluster, + cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage) error { + + if cbPkg.ValuesFrom.SecretRef == "" { + return nil + } + secret := &corev1.Secret{} + key := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} + if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { + h.Logger.Error(err, "unable to fetch secret to update labels", "objectkey", key) + return err + } + + opResult, createOrPatchErr := controllerutil.CreateOrPatch(h.Ctx, h.K8sClient, secret, func() error { + // Add cluster and package labels to secrets + if secret.Labels == nil { + secret.Labels = map[string]string{} + } + secret.Labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cbPkg.RefName) + secret.Labels[addontypes.ClusterNameLabel] = cluster.Name + return nil + }) + if createOrPatchErr != nil { + return createOrPatchErr + } + h.Logger.Info(fmt.Sprintf("Secret %s/%s %s with labels", secret.Namespace, secret.Name, opResult)) + return nil +} + +// updateProviderLabels is an internal function clones update the labels on the given provider +func (h *Helper) updateProviderLabels( + cluster *clusterapiv1beta1.Cluster, + cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage) error { + + if cbPkg.ValuesFrom == nil { + return nil + } + var createdOrUpdatedProvider *unstructured.Unstructured + gvr, err := h.GVRHelper.GetGVR(schema.GroupKind{Group: *cbPkg.ValuesFrom.ProviderRef.APIGroup, Kind: cbPkg.ValuesFrom.ProviderRef.Kind}) + if err != nil { + h.Logger.Error(err, "failed to getGVR") + return err + } + provider, err := h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Get(h.Ctx, cbPkg.ValuesFrom.ProviderRef.Name, metav1.GetOptions{}) + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to fetch provider %s/%s to update labels", cluster.Namespace, cbPkg.ValuesFrom.ProviderRef.Name), "gvr", gvr) + return err + } + + h.Logger.Info(fmt.Sprintf("provider %s/%s already exist, patching its Labels", provider.GetNamespace(), provider.GetName())) + // Instantiate an empty unstructured and only set Labels for patching + patchObj := unstructured.Unstructured{} + patchObj.SetLabels(map[string]string{ + // A package refName could contain characters that K8S does not like as a label value. + // For example, kapp-controller.tanzu.vmware.com.0.30.0+vmware.1-tkg.1 is a + // valid package refName, but it contains "+" that K8S complains. We parse the refName by replacing + // + to ---. + addontypes.PackageNameLabel: util.ParseStringForLabel(cbPkg.RefName), + addontypes.ClusterNameLabel: cluster.Name, + }) + patchData, err := patchObj.MarshalJSON() + if err != nil { + h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s with labels", provider.GetNamespace(), provider.GetName()), "gvr", gvr) + return err + } + createdOrUpdatedProvider, err = h.DynamicClient.Resource(*gvr).Namespace(cluster.Namespace).Patch(h.Ctx, provider.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{}) + if err != nil { + h.Logger.Info(fmt.Sprintf("unable to update provider %s/%s with labels", provider.GetNamespace(), provider.GetName()), "gvr", gvr) + return err + } + h.Logger.Info(fmt.Sprintf("updated provider %s/%s with labels", createdOrUpdatedProvider.GetNamespace(), createdOrUpdatedProvider.GetName()), "gvr", gvr) + return nil } // cloneEmbeddedLocalObjectRef is an internal function attempts to clone the embedded local object references from provider's namespace to cluster's From 2aaa735770858c317fe719c82e7fe001857a5f68 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Mon, 10 Apr 2023 14:44:12 -0700 Subject: [PATCH 07/10] Take 3: trying code for labels on custom CB providers and secrets --- .../clusterbootstrapclone/clusterbootstrapclone.go | 10 ++-------- tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go | 2 +- tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go | 2 +- .../aws_cc/aws_cc_tkrresolver_and_upgrade_test.go | 2 +- tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go | 2 +- 5 files changed, 6 insertions(+), 12 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 658eab24bd..eb2a01924c 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -428,7 +428,7 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph packagesToBeCloned = append(packagesToBeCloned, nonEmptyInlinePackages...) } - // find the packages that don't need to be cloned by updated only + // find the packages that don't need to be cloned but updated only var toBeCloned bool for _, cbPackage := range packages { toBeCloned = false @@ -887,7 +887,7 @@ func (h *Helper) updateProvidersAndSecretsLabels( cbPackages []*runtanzuv1alpha3.ClusterBootstrapPackage) error { for _, cbPackage := range cbPackages { - if cbPackage == nil { + if cbPackage == nil || cbPackage.ValuesFrom == nil { continue } @@ -913,9 +913,6 @@ func (h *Helper) updateSecretLabels( cluster *clusterapiv1beta1.Cluster, cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage) error { - if cbPkg.ValuesFrom.SecretRef == "" { - return nil - } secret := &corev1.Secret{} key := client.ObjectKey{Namespace: cluster.Namespace, Name: cbPkg.ValuesFrom.SecretRef} if err := h.K8sClient.Get(h.Ctx, key, secret); err != nil { @@ -944,9 +941,6 @@ func (h *Helper) updateProviderLabels( cluster *clusterapiv1beta1.Cluster, cbPkg *runtanzuv1alpha3.ClusterBootstrapPackage) error { - if cbPkg.ValuesFrom == nil { - return nil - } var createdOrUpdatedProvider *unstructured.Unstructured gvr, err := h.GVRHelper.GetGVR(schema.GroupKind{Group: *cbPkg.ValuesFrom.ProviderRef.APIGroup, Kind: cbPkg.ValuesFrom.ProviderRef.Kind}) if err != nil { diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go index 0a018c7cb4..d9c633a2a7 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = Describe("Functional tests for aws (clusterclass) - Calico", func() { +var _ = FDescribe("Functional tests for aws (clusterclass) - Calico", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go index 9445dd66e0..5c03b150f7 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = Describe("Functional tests to create & upgrade AWS cluster with custom ClusterBootstrap", func() { +var _ = FDescribe("Functional tests to create & upgrade AWS cluster with custom ClusterBootstrap", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go index 734cc4f887..eae08e79db 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = Describe("Functional tests for aws - TKRResolver and cluster upgrade with CNI Antrea", func() { +var _ = FDescribe("Functional tests for aws - TKRResolver and cluster upgrade with CNI Antrea", func() { E2ETKRResolverValidationForClusterCRUDSpec(context.TODO(), func() E2ETKRResolverValidationForClusterCRUDSpecInput { return E2ETKRResolverValidationForClusterCRUDSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go index 85146a8785..4a75514078 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = Describe("Functional tests for aws - cluster upgrade with CNI Calico", func() { +var _ = FDescribe("Functional tests for aws - cluster upgrade with CNI Calico", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, From 1c26b04598cd0257c621a078c494961e96abbcdd Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Tue, 11 Apr 2023 09:32:37 -0700 Subject: [PATCH 08/10] Changes for generically adding labels to secrets and providers and running all the integration tests --- tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go | 2 +- tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go | 2 +- tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go | 2 +- tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go index d9c633a2a7..0a018c7cb4 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_calico_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = FDescribe("Functional tests for aws (clusterclass) - Calico", func() { +var _ = Describe("Functional tests for aws (clusterclass) - Calico", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go index 5c03b150f7..9445dd66e0 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_custom_cb_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = FDescribe("Functional tests to create & upgrade AWS cluster with custom ClusterBootstrap", func() { +var _ = Describe("Functional tests to create & upgrade AWS cluster with custom ClusterBootstrap", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go index eae08e79db..734cc4f887 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_tkrresolver_and_upgrade_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = FDescribe("Functional tests for aws - TKRResolver and cluster upgrade with CNI Antrea", func() { +var _ = Describe("Functional tests for aws - TKRResolver and cluster upgrade with CNI Antrea", func() { E2ETKRResolverValidationForClusterCRUDSpec(context.TODO(), func() E2ETKRResolverValidationForClusterCRUDSpecInput { return E2ETKRResolverValidationForClusterCRUDSpecInput{ E2EConfig: e2eConfig, diff --git a/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go b/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go index 4a75514078..85146a8785 100644 --- a/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go +++ b/tkg/test/tkgctl/aws_cc/aws_cc_upgrade_calico_test.go @@ -12,7 +12,7 @@ import ( . "github.com/vmware-tanzu/tanzu-framework/tkg/test/tkgctl/shared" ) -var _ = FDescribe("Functional tests for aws - cluster upgrade with CNI Calico", func() { +var _ = Describe("Functional tests for aws - cluster upgrade with CNI Calico", func() { E2ECommonCCSpec(context.TODO(), func() E2ECommonCCSpecInput { return E2ECommonCCSpecInput{ E2EConfig: e2eConfig, From 9df99647f2b0cab75e5ec1f2a7107ffd721f6253 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Tue, 11 Apr 2023 13:06:19 -0700 Subject: [PATCH 09/10] Reverting back changes for earlier trials for fixing unit tests --- .../clusterbootstrap_controller_test.go | 23 ++++++++++++++----- .../testdata/test-cluster-bootstrap-9.yaml | 11 +++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/addons/controllers/clusterbootstrap_controller_test.go b/addons/controllers/clusterbootstrap_controller_test.go index e93177cf46..e7d45d1700 100644 --- a/addons/controllers/clusterbootstrap_controller_test.go +++ b/addons/controllers/clusterbootstrap_controller_test.go @@ -1295,15 +1295,26 @@ var _ = Describe("ClusterBootstrap Reconciler", func() { if err := k8sClient.Get(ctx, key, config); err != nil { return false } - ownerRef := metav1.OwnerReference{ - APIVersion: clusterapiv1beta1.GroupVersion.String(), - Kind: "Cluster", - Name: cluster.Name, - UID: cluster.UID, + + if len(config.OwnerReferences) > 0 { + return false } - return clusterapiutil.HasOwnerRef(config.OwnerReferences, ownerRef) + + Expect(len(config.OwnerReferences)).Should(Equal(0)) + return true }, waitTimeout, pollingInterval).Should(BeTrue()) + patchedSecret := config.DeepCopy() + ownerRef := metav1.OwnerReference{ + APIVersion: clusterapiv1beta1.GroupVersion.String(), + Kind: "Cluster", + Name: cluster.Name, + UID: cluster.UID, + } + + patchedSecret.OwnerReferences = clusterapiutil.EnsureOwnerRef(patchedSecret.OwnerReferences, ownerRef) + Expect(k8sClient.Patch(ctx, patchedSecret, client.MergeFrom(config))).ShouldNot(HaveOccurred()) + Expect(config.ObjectMeta.Labels["tkg.tanzu.vmware.com/package-name"]).Should(Equal("load-balancer-and-ingress-service.tanzu.vmware.com.0.0.4")) By("ClusterBootstrap CR is created with correct ownerReference added") diff --git a/addons/controllers/testdata/test-cluster-bootstrap-9.yaml b/addons/controllers/testdata/test-cluster-bootstrap-9.yaml index 42be345352..06e672c163 100644 --- a/addons/controllers/testdata/test-cluster-bootstrap-9.yaml +++ b/addons/controllers/testdata/test-cluster-bootstrap-9.yaml @@ -1044,6 +1044,17 @@ stringData: identity_management_type: none tkg_cluster_role: workload --- +apiVersion: v1 +kind: Secret +metadata: + name: default-pinniped-config-v1.23.5---vmware.1-tkg.1 + namespace: cluster-namespace-9 +stringData: + values.yaml: | + infrastructure_provider: vsphere + identity_management_type: none + tkg_cluster_role: workload +--- apiVersion: cni.tanzu.vmware.com/v1alpha1 kind: CalicoConfig metadata: From c45cc8c60b90ddcc16944b735c30fa39924bc113 Mon Sep 17 00:00:00 2001 From: Shivaani Gupta Date: Tue, 11 Apr 2023 15:38:15 -0700 Subject: [PATCH 10/10] Rebasing from latest main --- .../clusterbootstrap_controller.go | 5 -- .../clusterbootstrapclone.go | 57 +------------------ 2 files changed, 2 insertions(+), 60 deletions(-) diff --git a/addons/controllers/clusterbootstrap_controller.go b/addons/controllers/clusterbootstrap_controller.go index 0d30e1d85a..017580b688 100644 --- a/addons/controllers/clusterbootstrap_controller.go +++ b/addons/controllers/clusterbootstrap_controller.go @@ -481,11 +481,6 @@ func (r *ClusterBootstrapReconciler) patchClusterBootstrapFromTemplate( return nil, err } - if err := clusterBootstrapHelper.AddPackageLabelForCNIProvider(cluster.Name, updatedClusterBootstrap); err != nil { - r.Log.Error(err, fmt.Sprintf("unable to add labels to cni provider")) - return nil, err - } - r.Log.Info("updated clusterBootstrap", "clusterBootstrap", updatedClusterBootstrap) return updatedClusterBootstrap, nil } diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index eb2a01924c..30a28a5577 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -466,12 +466,6 @@ func (h *Helper) HandleExistingClusterBootstrap(clusterBootstrap *runtanzuv1alph h.Logger.Error(err, fmt.Sprintf("unable to add cluster %s/%s as owner reference to providers", cluster.Namespace, cluster.Name)) } - if err := h.AddPackageLabelForCNIProvider(cluster.Name, clusterBootstrap); err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to add labels to cni provider")) - return nil, err - } - h.Logger.Info("patched labels for cni provider.") - clusterBootstrap.OwnerReferences = []metav1.OwnerReference{ { APIVersion: clusterapiv1beta1.GroupVersion.String(), @@ -1080,41 +1074,16 @@ func (h *Helper) getListOfExistingProviders(clusterBootstrap *runtanzuv1alpha3.C func (h *Helper) AddClusterOwnerRefToExistingProviders(cluster *clusterapiv1beta1.Cluster, clusterBootstrap *runtanzuv1alpha3.ClusterBootstrap) error { - exsitingProviders, err := h.getListOfExistingProviders(clusterBootstrap) + existingProviders, err := h.getListOfExistingProviders(clusterBootstrap) if err != nil { return err } - if err = h.AddClusterOwnerRef(cluster, exsitingProviders, nil, nil); err != nil { + if err = h.AddClusterOwnerRef(cluster, existingProviders, nil, nil); err != nil { return err } return nil } -// AddPackageLabelForCNIProvider patch the missing labels for AntreaConfig(cni config) when it's related to -// an existing clusterboostrap. To make antreaconfig works with old versions, it relies on the package label now. -func (h *Helper) AddPackageLabelForCNIProvider(clusterName string, clusterBootstrap *runtanzuv1alpha3.ClusterBootstrap) error { - cni := clusterBootstrap.Spec.CNI - if cni != nil { - providers, err := h.getListOfExistingProviders(clusterBootstrap) - if err != nil { - return err - } - for _, p := range providers { - if cni.ValuesFrom != nil && cni.ValuesFrom.ProviderRef != nil && p.GetKind() == cni.ValuesFrom.ProviderRef.Kind { - labels := p.GetLabels() - if labels == nil { - labels = map[string]string{} - } - labels[addontypes.PackageNameLabel] = util.ParseStringForLabel(cni.RefName) - labels[addontypes.ClusterNameLabel] = clusterName - return h.setLabels(labels, p) - } - } - - } - return nil -} - // AddClusterOwnerRef adds cluster as an owner reference to the children with given controller and blockownerdeletion settings func (h *Helper) AddClusterOwnerRef(cluster *clusterapiv1beta1.Cluster, children []*unstructured.Unstructured, controller, blockownerdeletion *bool) error { ownerRef := metav1.OwnerReference{ @@ -1146,28 +1115,6 @@ func ownedByDifferentCluster(k8SObject *unstructured.Unstructured, cluster *clus return "" } -func (h *Helper) setLabels(labels map[string]string, child *unstructured.Unstructured) error { - gvr, err := h.GVRHelper.GetGVR(child.GroupVersionKind().GroupKind()) - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to get GVR of %s/%s", child.GetNamespace(), child.GetName())) - return err - } - - patchObj := unstructured.Unstructured{} - patchObj.SetLabels(labels) - patchData, err := patchObj.MarshalJSON() - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", child.GetNamespace(), child.GetName()), "gvr", gvr) - return err - } - _, err = h.DynamicClient.Resource(*gvr).Namespace(child.GetNamespace()).Patch(h.Ctx, child.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{}) - if err != nil { - h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", child.GetNamespace(), child.GetName()), "gvr", gvr) - } - return err - -} - func (h *Helper) setOwnerRef(ownerRef *metav1.OwnerReference, children []*unstructured.Unstructured) error { for _, child := range children { gvr, err := h.GVRHelper.GetGVR(child.GroupVersionKind().GroupKind())