From 0a0bc222d09be745fe2fe66e8adab31436c10f67 Mon Sep 17 00:00:00 2001 From: Hang Yan Date: Fri, 7 Apr 2023 10:14:32 +0800 Subject: [PATCH 1/5] Add missing labels for antreaconfig from clusterbootstrap Signed-off-by: Hang Yan --- .../clusterbootstrapclone.go | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index bc8b5d69eb..fcce72f225 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -444,6 +444,12 @@ 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(), @@ -964,6 +970,31 @@ func (h *Helper) AddClusterOwnerRefToExistingProviders(cluster *clusterapiv1beta return nil } +// AddPackageLabelForCNIConfig 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{ @@ -995,6 +1026,32 @@ 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 + } + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + // We need to get and update, otherwise there could have concurrency issue: ["the object has been modified; please + // apply your changes to the latest version and try again"] + newChild, errGetProvider := h.DynamicClient.Resource(*gvr).Namespace(child.GetNamespace()).Get(h.Ctx, child.GetName(), metav1.GetOptions{}) + if errGetProvider != nil { + h.Logger.Error(errGetProvider, fmt.Sprintf("unable to get %s %s/%s", child.GetKind(), child.GetNamespace(), child.GetName())) + return errGetProvider + } + newChild = newChild.DeepCopy() + newChild.SetLabels(labels) + _, errUpdateProvider := h.DynamicClient.Resource(*gvr).Namespace(newChild.GetNamespace()).Update(h.Ctx, newChild, metav1.UpdateOptions{}) + if errUpdateProvider != nil { + h.Logger.Error(errUpdateProvider, fmt.Sprintf("unable to update %s %s/%s", child.GetKind(), child.GetNamespace(), child.GetName())) + return errUpdateProvider + } + return nil + }) + 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()) From 5ba32fdfdcda90b8baca37f34ecc2e5eb8347355 Mon Sep 17 00:00:00 2001 From: Hang Yan Date: Fri, 7 Apr 2023 10:32:14 +0800 Subject: [PATCH 2/5] add test update Signed-off-by: Hang Yan --- addons/controllers/calicoconfig_controller_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/addons/controllers/calicoconfig_controller_test.go b/addons/controllers/calicoconfig_controller_test.go index 4a651f3122..a658781549 100644 --- a/addons/controllers/calicoconfig_controller_test.go +++ b/addons/controllers/calicoconfig_controller_test.go @@ -478,6 +478,7 @@ var _ = Describe("CalicoConfig Reconciler and Webhooks", func() { Expect(k8sClient.Get(ctx, configKey, calicoConfig)).To(Succeed()) Expect(calicoConfig.Spec.Calico.Config.VethMTU).Should(Equal(int64(1420))) + Expect(calicoConfig.ObjectMeta.Labels["tkg.tanzu.vmware.com/package-name"]).Should(Equal("calico.tanzu.vmware.com.1.2.5--vmware.12-tkg.1")) // why int64?? that would make it architecture specific, as oposed to why not just int? // (defined in apis/addonconfigs/cni/v1alpha1/calicoconfig_types.go) // seems to also have forced part of this fix: https://github.com/vmware-tanzu/tanzu-framework/pull/2164 From 14ee1e8968c111a2f891176277c789870bd0945e Mon Sep 17 00:00:00 2001 From: Hang Yan Date: Fri, 7 Apr 2023 15:06:32 +0800 Subject: [PATCH 3/5] use patch Signed-off-by: Hang Yan --- .../clusterbootstrapclone.go | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index fcce72f225..3da896a60f 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -1032,24 +1032,20 @@ func (h *Helper) setLabels(labels map[string]string, child *unstructured.Unstruc h.Logger.Error(err, fmt.Sprintf("unable to get GVR of %s/%s", child.GetNamespace(), child.GetName())) return err } - err = retry.RetryOnConflict(retry.DefaultRetry, func() error { - // We need to get and update, otherwise there could have concurrency issue: ["the object has been modified; please - // apply your changes to the latest version and try again"] - newChild, errGetProvider := h.DynamicClient.Resource(*gvr).Namespace(child.GetNamespace()).Get(h.Ctx, child.GetName(), metav1.GetOptions{}) - if errGetProvider != nil { - h.Logger.Error(errGetProvider, fmt.Sprintf("unable to get %s %s/%s", child.GetKind(), child.GetNamespace(), child.GetName())) - return errGetProvider - } - newChild = newChild.DeepCopy() - newChild.SetLabels(labels) - _, errUpdateProvider := h.DynamicClient.Resource(*gvr).Namespace(newChild.GetNamespace()).Update(h.Ctx, newChild, metav1.UpdateOptions{}) - if errUpdateProvider != nil { - h.Logger.Error(errUpdateProvider, fmt.Sprintf("unable to update %s %s/%s", child.GetKind(), child.GetNamespace(), child.GetName())) - return errUpdateProvider - } - return nil - }) + + 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(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 { From 26959fc5d7bb7abaaedeee7408fe277b7adf29cb Mon Sep 17 00:00:00 2001 From: Hang Yan Date: Fri, 7 Apr 2023 15:10:54 +0800 Subject: [PATCH 4/5] update Signed-off-by: Hang Yan --- addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go index 3da896a60f..dfa2a9583c 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -1042,7 +1042,7 @@ func (h *Helper) setLabels(labels map[string]string, child *unstructured.Unstruc } _, err = h.DynamicClient.Resource(*gvr).Namespace(child.GetNamespace()).Patch(h.Ctx, child.GetName(), types.MergePatchType, patchData, metav1.PatchOptions{}) if err != nil { - h.Logger.Error(fmt.Sprintf("unable to patch provider %s/%s", child.GetNamespace(), child.GetName()), "gvr", gvr) + h.Logger.Error(err, fmt.Sprintf("unable to patch provider %s/%s", child.GetNamespace(), child.GetName()), "gvr", gvr) } return err From 82e8782f302ffe4afb350313c77f992b84937cfb Mon Sep 17 00:00:00 2001 From: Hang Yan Date: Fri, 7 Apr 2023 19:21:56 +0800 Subject: [PATCH 5/5] also fix the upgrade case Signed-off-by: Hang Yan --- addons/controllers/clusterbootstrap_controller.go | 5 +++++ .../pkg/util/clusterbootstrapclone/clusterbootstrapclone.go | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/addons/controllers/clusterbootstrap_controller.go b/addons/controllers/clusterbootstrap_controller.go index 017580b688..0d30e1d85a 100644 --- a/addons/controllers/clusterbootstrap_controller.go +++ b/addons/controllers/clusterbootstrap_controller.go @@ -481,6 +481,11 @@ 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 dfa2a9583c..7884541863 100644 --- a/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go +++ b/addons/pkg/util/clusterbootstrapclone/clusterbootstrapclone.go @@ -444,7 +444,7 @@ 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 { + 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 } @@ -970,9 +970,9 @@ func (h *Helper) AddClusterOwnerRefToExistingProviders(cluster *clusterapiv1beta return nil } -// AddPackageLabelForCNIConfig patch the missing labels for AntreaConfig(cni config) when it's related to +// 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 { +func (h *Helper) AddPackageLabelForCNIProvider(clusterName string, clusterBootstrap *runtanzuv1alpha3.ClusterBootstrap) error { cni := clusterBootstrap.Spec.CNI if cni != nil { providers, err := h.getListOfExistingProviders(clusterBootstrap)