From 3055431e9667ff6b93cac0b4ba5f64843c10566a Mon Sep 17 00:00:00 2001 From: Nathan Fudenberg Date: Tue, 20 Dec 2022 13:52:46 -0500 Subject: [PATCH] Feat/k8s no clustername (#402) * k8s1.25 * interface * GetAnnotations() replaces ClusterName field in ClusterResourceId interface * actually support k8s v1.25 (with controller-runtime v0.13.1) * update go templates; changelog; dep updates * put changelog in folder * update from generated code * add annotations field to ClusterObjectRef proto to match ClusterResourceId interface * remove annotations field from ClusterObjectRef proto * Adding changelog file to new location * Deleting changelog file from old location * add function to allow ClusterObjectRef struct to satisfy the ClusterResourceId interface * handle possibility of clusterName being set by annotation or field * pr feedback during working session with eitan * Update contrib/pkg/sets/sets.go Co-authored-by: J. Conrad Hanson <27842017+conradhanson@users.noreply.github.com> * contrib/reconciler: Update to converting to new interface. Not stored directly as its not exported. * Adding changelog file to new location * Deleting changelog file from old location * remove extraneous separators * mod: Revert the comingled k8s1.25 changes * changelog: Update to reference what is being done correctly * add back * codegen * fix input reconciler * register: Update to ensure that sa secret exists. Note: Not handled on SA creation but seems to be the only place where we need to ensure secrets exist * register: Make sure to have a namespace * register: Make sure wait for token population * reigster: Longer delay now that extra controller layer is needed for secrets * mod: skv2 tocreate on the correct cluster * secret-type change * templ/input: Set clusters in a novel way * templ/input: Set clusters consistently given new novel approach * kubeconfig: Update comment * ezkube: Method for converting deprecated objects will no longer blow away other annotations * register: harden service account getting * test/registration: Change expectations given serviceaccount changes * template/inputs/hybrid: Clusternaming Co-authored-by: Eitan Yarmush Co-authored-by: Conrad Hanson <27842017+conradhanson@users.noreply.github.com> Co-authored-by: changelog-bot Co-authored-by: Jenny Shu --- .../v0.26.0/clustername-via-annotations.yaml | 7 ++ .../input/hybrid_input_reconciler.gotmpl | 2 +- .../templates/input/input_reconciler.gotmpl | 5 +- .../templates/input/input_snapshot.gotmpl | 8 +- .../templates/output/output_snapshot.gotmpl | 6 +- contrib/pkg/input/input_reconciler.go | 2 +- contrib/pkg/output/snapshot.go | 4 +- contrib/pkg/sets/sets.go | 20 +++-- pkg/ezkube/object_ref.go | 4 +- pkg/ezkube/resource_id.go | 74 ++++++++++++++++++- .../kubeconfig/kubeconfig_secret.go | 3 +- pkg/multicluster/register/registrant.go | 60 ++++++++++++--- pkg/multicluster/register/registrant_test.go | 10 +-- 13 files changed, 162 insertions(+), 43 deletions(-) create mode 100644 changelog/v0.26.0/clustername-via-annotations.yaml diff --git a/changelog/v0.26.0/clustername-via-annotations.yaml b/changelog/v0.26.0/clustername-via-annotations.yaml new file mode 100644 index 000000000..5bd575a4b --- /dev/null +++ b/changelog/v0.26.0/clustername-via-annotations.yaml @@ -0,0 +1,7 @@ +changelog: + - type: BREAKING_CHANGE + issueLink: https://github.com/solo-io/gloo-mesh-enterprise/issues/5158 + description: > + In the ClusterResourceId interface, ClusterName field is replaced with GetAnnotations method, + since k8s v1.24+ removes the ClusterName field from resources. + Now, a resource annotation of "cluster.solo.io/cluster" is expected to hold a string of the cluster name. \ No newline at end of file diff --git a/contrib/codegen/templates/input/hybrid_input_reconciler.gotmpl b/contrib/codegen/templates/input/hybrid_input_reconciler.gotmpl index 6f3736688..0306b969c 100644 --- a/contrib/codegen/templates/input/hybrid_input_reconciler.gotmpl +++ b/contrib/codegen/templates/input/hybrid_input_reconciler.gotmpl @@ -159,7 +159,7 @@ type remote{{ $snapshotName }}InputReconciler struct { {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }} func (r *remote{{ $snapshotName }}InputReconciler) Reconcile{{ $resource.Kind }}(clusterName string, obj *{{ $type_import_prefix }}.{{ $resource.Kind }}) (reconcile.Result, error) { - obj.ClusterName = clusterName + ezkube.SetClusterName(obj, clusterName) return r.base.ReconcileRemoteGeneric(obj) } diff --git a/contrib/codegen/templates/input/input_reconciler.gotmpl b/contrib/codegen/templates/input/input_reconciler.gotmpl index 874897bcc..b3ddce0a7 100644 --- a/contrib/codegen/templates/input/input_reconciler.gotmpl +++ b/contrib/codegen/templates/input/input_reconciler.gotmpl @@ -26,6 +26,7 @@ import ( multicluster_v2 "github.com/solo-io/skv2/pkg/multicluster/v2" reconcile "github.com/solo-io/skv2/pkg/reconcile" reconcile_v2 "github.com/solo-io/skv2/pkg/reconcile/v2" + "github.com/solo-io/skv2/pkg/ezkube" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -88,7 +89,7 @@ func RegisterMultiCluster{{ $snapshotName }}Reconciler( multicluster_reconcile_v2.NewLoop("das", clusters, &{{ $types_import_prefix }}.{{ $resource.Kind }}{}, reconcile_v2.Options{}). AddReconciler(ctx, &multicluster_v2.ReconcilerFuncs[*{{ $types_import_prefix }}.{{ $resource.Kind }}]{ ReconcileFunc: func(ctx context.Context, clusterName string, obj *{{ $types_import_prefix }}.{{ $resource.Kind }}) (reconcile.Result, error) { - obj.ClusterName = clusterName + ezkube.SetClusterName(obj, clusterName) return r.base.ReconcileRemoteGeneric(obj) } , ReconcileDeletionFunc: func(ctx context.Context, clusterName string, obj reconcile_v2.Request) error { @@ -97,7 +98,7 @@ func RegisterMultiCluster{{ $snapshotName }}Reconciler( Namespace: obj.Namespace, ClusterName: clusterName, } - _, err := r.base.ReconcileRemoteGeneric(ref) + _, err := r.base.ReconcileRemoteGeneric(ezkube.ConvertRefToId(ref)) return err }, }, predicates...) diff --git a/contrib/codegen/templates/input/input_snapshot.gotmpl b/contrib/codegen/templates/input/input_snapshot.gotmpl index 9c869874e..fea35bf7c 100644 --- a/contrib/codegen/templates/input/input_snapshot.gotmpl +++ b/contrib/codegen/templates/input/input_snapshot.gotmpl @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" + "github.com/solo-io/skv2/pkg/ezkube" "github.com/solo-io/skv2/pkg/resource" "github.com/solo-io/skv2/pkg/verifier" "github.com/solo-io/go-utils/contextutils" @@ -335,7 +336,7 @@ func (s *snapshot{{ $snapshotName }}) ForEachObject(handleObject func(cluster st {{- $kindLowerCamel := lower_camel $resource.Kind }} {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }} for _, obj := range s.{{ $kindLowerCamelPlural }}.List() { - cluster := obj.GetClusterName() + cluster := ezkube.GetClusterName(obj) gvk := schema.GroupVersionKind{ Group: "{{ $resource.Group.Group }}", Version: "{{ $resource.Version }}", @@ -487,7 +488,8 @@ func (b *multiCluster{{ $snapshotName }}Builder) insert{{ $kindPlural }}FromClus for _, item := range {{ $kindLowerCamel }}List.Items { item := item.DeepCopy() // pike + own - item.ClusterName = cluster // set cluster for in-memory processing + + ezkube.SetClusterName(item, cluster) // set cluster for what was in-memory processing {{ $kindLowerCamelPlural }}.Insert(item) } @@ -601,7 +603,7 @@ func (b *singleCluster{{ $snapshotName }}Builder) insert{{ $kindPlural }}(ctx co for _, item := range {{ $kindLowerCamel }}List.Items { item := item.DeepCopy() // pike + own the item. - item.ClusterName = b.clusterName + ezkube.SetClusterName(item, b.clusterName) // set cluster for what was in-memory processing {{ $kindLowerCamelPlural }}.Insert(item) } diff --git a/contrib/codegen/templates/output/output_snapshot.gotmpl b/contrib/codegen/templates/output/output_snapshot.gotmpl index f2125e2bf..14588b811 100644 --- a/contrib/codegen/templates/output/output_snapshot.gotmpl +++ b/contrib/codegen/templates/output/output_snapshot.gotmpl @@ -269,7 +269,7 @@ func (s *snapshot) ForEachObject(handleObject func(cluster string, gvk schema.Gr {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }} for _, set := range s.{{ $kindLowerCamelPlural }} { for _, obj := range set.Set().List() { - cluster := obj.GetClusterName() + cluster := ezkube.GetClusterName(obj) gvk := schema.GroupVersionKind{ Group: "{{ $resource.Group.Group }}", Version: "{{ $resource.Version }}", @@ -668,7 +668,7 @@ func (b *builder) Generic() resource.ClusterSnapshot { {{- $kindLowerCamel := lower_camel $resource.Kind }} {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }} for _, obj := range b.Get{{ $kindPlural }}().List() { - cluster := obj.GetClusterName() + cluster := ezkube.GetClusterName(obj) gvk := schema.GroupVersionKind{ Group: "{{ $resource.Group.Group }}", Version: "{{ $resource.Version }}", @@ -697,7 +697,7 @@ func (b *builder) ForEachObject(handleObject func(cluster string, gvk schema.Gro {{- $kindLowerCamel := lower_camel $resource.Kind }} {{- $kindLowerCamelPlural := pluralize $kindLowerCamel }} for _, obj := range b.Get{{ $kindPlural }}().List() { - cluster := obj.GetClusterName() + cluster := ezkube.GetClusterName(obj) gvk := schema.GroupVersionKind{ Group: "{{ $resource.Group.Group }}", Version: "{{ $resource.Version }}", diff --git a/contrib/pkg/input/input_reconciler.go b/contrib/pkg/input/input_reconciler.go index ee0bd99ee..baa3d01e8 100644 --- a/contrib/pkg/input/input_reconciler.go +++ b/contrib/pkg/input/input_reconciler.go @@ -121,7 +121,7 @@ func (r *inputReconciler) processNextWorkItem() bool { // determine whether the resource has been read from a remote cluster // based on whether its ClusterName field is set if clusterResource, ok := key.(ezkube.ClusterResourceId); ok { - if clusterResource.GetClusterName() != "" { + if ezkube.GetClusterName(clusterResource) != "" { isRemoteCluster = true } } diff --git a/contrib/pkg/output/snapshot.go b/contrib/pkg/output/snapshot.go index ba6e36c71..b281346a3 100644 --- a/contrib/pkg/output/snapshot.go +++ b/contrib/pkg/output/snapshot.go @@ -180,7 +180,7 @@ type ResourceList struct { func (l ResourceList) SplitByClusterName() map[string][]ezkube.Object { listsByCluster := map[string][]ezkube.Object{} for _, resource := range l.Resources { - clusterName := resource.GetClusterName() + clusterName := ezkube.GetClusterName(resource) listForCluster := listsByCluster[clusterName] // list func (i.e. garbage collection labels) shared across clusters @@ -354,7 +354,7 @@ func (s Snapshot) syncList( func (s Snapshot) upsert(ctx context.Context, cli client.Client, obj ezkube.Object, statusUpdate bool) error { // add cluster label to object obj.SetLabels( - AddClusterLabels(obj.GetClusterName(), obj.GetLabels()), + AddClusterLabels(ezkube.GetClusterName(obj), obj.GetLabels()), ) var ( diff --git a/contrib/pkg/sets/sets.go b/contrib/pkg/sets/sets.go index f2c5b4339..5a2d29c57 100644 --- a/contrib/pkg/sets/sets.go +++ b/contrib/pkg/sets/sets.go @@ -34,18 +34,22 @@ func Key(id ezkube.ResourceId) string { if id == nil { return "" } - if clusterId, ok := id.(ezkube.ClusterResourceId); ok { - b.WriteString(clusterId.GetName()) - b.WriteString(separator) - b.WriteString(clusterId.GetNamespace()) - b.WriteString(separator) - b.WriteString(clusterId.GetClusterName()) - return b.String() - } b.WriteString(id.GetName()) b.WriteString(separator) b.WriteString(id.GetNamespace()) b.WriteString(separator) + // handle the possibility that clusterName could be set either as an annotation (new way) + // or as a field (old way pre-k8s 1.25) + if clusterId, ok := id.(ezkube.ClusterResourceId); ok { + clusterNameByAnnotation := ezkube.GetClusterName(clusterId) + if clusterNameByAnnotation != "" { + b.WriteString(clusterNameByAnnotation) + return b.String() + } + } + if deprecatedClusterId, ok := id.(interface{ GetClusterName() string }); ok { + b.WriteString(deprecatedClusterId.GetClusterName()) + } return b.String() } diff --git a/pkg/ezkube/object_ref.go b/pkg/ezkube/object_ref.go index 715935722..17eee5b86 100644 --- a/pkg/ezkube/object_ref.go +++ b/pkg/ezkube/object_ref.go @@ -22,7 +22,7 @@ func MakeClusterObjectRef(resource ClusterResourceId) *v1.ClusterObjectRef { return &v1.ClusterObjectRef{ Name: resource.GetName(), Namespace: resource.GetNamespace(), - ClusterName: resource.GetClusterName(), + ClusterName: GetClusterName(resource), } } @@ -42,7 +42,7 @@ func ClusterRefsMatch(ref1, ref2 ClusterResourceId) bool { } return ref1.GetNamespace() == ref2.GetNamespace() && ref1.GetName() == ref2.GetName() && - ref1.GetClusterName() == ref2.GetClusterName() + GetClusterName(ref1) == GetClusterName(ref2) } func MakeClientObjectKey(ref ResourceId) client.ObjectKey { diff --git a/pkg/ezkube/resource_id.go b/pkg/ezkube/resource_id.go index dabaaee35..f26dfda44 100644 --- a/pkg/ezkube/resource_id.go +++ b/pkg/ezkube/resource_id.go @@ -1,14 +1,86 @@ package ezkube +import ( + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ClusterAnnotation = "cluster.solo.io/cluster" + // ResourceId represents a global identifier for a k8s resource. type ResourceId interface { GetName() string GetNamespace() string } -// ResourceId represents a global identifier for a k8s resource. +// ClusterResourceId represents a global identifier for a k8s resource. type ClusterResourceId interface { + GetName() string + GetNamespace() string + GetAnnotations() map[string]string +} + +// internal struct needed to create helper func that converts ref to struct that satisfies ClusterResourceId interface +type clusterResourceId struct { + name, namespace string + annotations map[string]string +} + +func (c clusterResourceId) GetName() string { + return c.name +} + +func (c clusterResourceId) GetNamespace() string { + return c.namespace +} + +func (c clusterResourceId) GetAnnotations() map[string]string { + return c.annotations +} + +type deprecatedClusterResourceId interface { GetName() string GetNamespace() string GetClusterName() string } + +// ConvertRefToId converts a ClusterObjectRef to a struct that implements the ClusterResourceId interface +// Will not set an empty cluster name over an existing cluster name +func ConvertRefToId(ref deprecatedClusterResourceId) ClusterResourceId { + // if ref is already stores annotations then we need to store the updates + anno := map[string]string{} + + if cri, ok := ref.(ClusterResourceId); ok { + anno = cri.GetAnnotations() + } + cn := ref.GetClusterName() + if cn != "" { + anno[ClusterAnnotation] = cn + } + + return clusterResourceId{ + name: ref.GetName(), + namespace: ref.GetNamespace(), + annotations: anno, + } +} + +func GetDeprecatedClusterName(id ResourceId) string { + if id, ok := id.(deprecatedClusterResourceId); ok { + return id.GetClusterName() + } + return "" +} + +func GetClusterName(id ClusterResourceId) string { + if id.GetAnnotations() == nil { + return "" + } + return id.GetAnnotations()[ClusterAnnotation] +} + +func SetClusterName(obj client.Object, cluster string) { + if obj.GetAnnotations() == nil { + obj.SetAnnotations(map[string]string{}) + } + obj.GetAnnotations()[ClusterAnnotation] = cluster +} diff --git a/pkg/multicluster/kubeconfig/kubeconfig_secret.go b/pkg/multicluster/kubeconfig/kubeconfig_secret.go index 5738a6768..03b42f1c2 100644 --- a/pkg/multicluster/kubeconfig/kubeconfig_secret.go +++ b/pkg/multicluster/kubeconfig/kubeconfig_secret.go @@ -26,7 +26,8 @@ var ( // TODO settle on how to canonicalize cluster names: https://github.com/solo-io/skv2/issues/15 -// ToSecret converts a kubernetes api.Config to a secret with the provided name and namespace. +// ToSecret converts a kubernetes api.Config to a secret with the provided +// cluster as the name and the provided namespace. func ToSecret(namespace string, cluster string, resourceLabels map[string]string, kc api.Config) (*kubev1.Secret, error) { err := convertCertFilesToInline(kc) if err != nil { diff --git a/pkg/multicluster/register/registrant.go b/pkg/multicluster/register/registrant.go index 6dea3c62a..b5a9444cc 100644 --- a/pkg/multicluster/register/registrant.go +++ b/pkg/multicluster/register/registrant.go @@ -42,8 +42,8 @@ var ( // exponential backoff retry with an initial period of 0.1s for 7 iterations, which will mean a cumulative retry period of ~6s // visible for testing SecretLookupOpts = []retry.Option{ - retry.Delay(time.Millisecond * 100), - retry.Attempts(7), + retry.Delay(time.Millisecond * 200), + retry.Attempts(10), retry.DelayType(retry.BackOffDelay), } ) @@ -594,26 +594,64 @@ func (c *clusterRegistrant) getTokenForSa( if err != nil { return "", err } - var foundSecret *corev1.Secret + + var sa *corev1.ServiceAccount + if err = retry.Do(func() error { - sa, err := saClient.GetServiceAccount(ctx, saRef) + sa, err = saClient.GetServiceAccount(ctx, saRef) if err != nil { return err } - if len(sa.Secrets) == 0 { + return nil + }, SecretLookupOpts...); err != nil { + return "", err + } + + // create a secret name format. This is similar to what was previously done + // but after the k8s upgrade we no longer have knowledge of what was previously + // autogenerated by k8s. + // TODO(nfuden): move away from these long lived connections as they are + // likely to be deprecated even more in the future. + secretName := fmt.Sprintf("%s-secret", sa.Name) + + // see if there is a secret + _, err = remoteSecretClient.GetSecret(ctx, client.ObjectKey{Name: secretName, Namespace: saRef.Namespace}) + if err != nil && k8s_errs.IsNotFound(err) { + + saSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: saRef.Namespace, + Annotations: map[string]string{"kubernetes.io/service-account.name": saRef.Name}, + }, + Type: "kubernetes.io/service-account-token", + } + + // make secret + if err = remoteSecretClient.UpsertSecret(ctx, saSecret); err != nil { + return "", err + } + + } + + var foundSecret *corev1.Secret + if err = retry.Do(func() error { + secret, err := remoteSecretClient.GetSecret(ctx, client.ObjectKey{Name: secretName, Namespace: saRef.Namespace}) + if err != nil { return eris.Errorf( "service account %s.%s does not have a token secret associated with it", saRef.Name, saRef.Namespace, ) } - secretName := sa.Secrets[0].Name - secret, err := remoteSecretClient.GetSecret(ctx, client.ObjectKey{Name: secretName, Namespace: saRef.Namespace}) - if err != nil { - return err - } - foundSecret = secret + if _, ok := foundSecret.Data[SecretTokenKey]; !ok { + return eris.Errorf( + "service account %s.%s has not populated its token secret", + saRef.Name, + saRef.Namespace, + ) + } return nil }, SecretLookupOpts...); err != nil { return "", SecretNotReady(err) diff --git a/pkg/multicluster/register/registrant_test.go b/pkg/multicluster/register/registrant_test.go index 35ef5e6cf..595fc2705 100644 --- a/pkg/multicluster/register/registrant_test.go +++ b/pkg/multicluster/register/registrant_test.go @@ -283,7 +283,7 @@ var _ = Describe("Registrant", func() { token := "hello" saSecret := &k8s_core_types.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "sa-secret", + Name: "sa-name-secret", Namespace: namespace, }, Data: map[string][]byte{ @@ -295,18 +295,12 @@ var _ = Describe("Registrant", func() { Name: sa.Name, Namespace: sa.Namespace, }, - Secrets: []k8s_core_types.ObjectReference{ - { - Namespace: namespace, - Name: saSecret.GetName(), - }, - }, } saClient.EXPECT(). GetServiceAccount(ctx, sa). Return(expectedSa, nil). - Times(2) + Times(1) secretClient.EXPECT(). GetSecret(ctx, client.ObjectKey{