From b5b2d6b684988dbef77a306d979a54359f07b8fc Mon Sep 17 00:00:00 2001 From: Ludovic Cleroux Date: Tue, 2 Apr 2024 17:45:05 +0200 Subject: [PATCH] ROX-20581: probe labels (#1730) --- .../pkg/central/reconciler/reconciler.go | 140 +++++++----- .../pkg/central/reconciler/reconciler_test.go | 210 ++++++++++++++++++ 2 files changed, 300 insertions(+), 50 deletions(-) diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index a13ac70e73..d7fe6254c8 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -32,7 +32,6 @@ import ( "github.com/stackrox/rox/operator/apis/platform/v1alpha1" "github.com/stackrox/rox/pkg/declarativeconfig" "github.com/stackrox/rox/pkg/random" - "golang.org/x/exp/maps" "gopkg.in/yaml.v2" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" @@ -61,10 +60,14 @@ const ( envAnnotationKey = "rhacs.redhat.com/environment" clusterNameAnnotationKey = "rhacs.redhat.com/cluster-name" orgNameAnnotationKey = "rhacs.redhat.com/org-name" - instanceTypeLabelKey = "rhacs.redhat.com/instance-type" - orgIDLabelKey = "rhacs.redhat.com/org-id" - tenantIDLabelKey = "rhacs.redhat.com/tenant" - centralExpiredAtKey = "rhacs.redhat.com/expired-at" + + labelManagedByFleetshardValue = "rhacs-fleetshard" + instanceLabelKey = "app.kubernetes.io/instance" + instanceTypeLabelKey = "rhacs.redhat.com/instance-type" + managedByLabelKey = "app.kubernetes.io/managed-by" + orgIDLabelKey = "rhacs.redhat.com/org-id" + tenantIDLabelKey = "rhacs.redhat.com/tenant" + centralExpiredAtKey = "rhacs.redhat.com/expired-at" auditLogNotifierKey = "com.redhat.rhacs.auditLogNotifier" auditLogNotifierName = "Platform Audit Logs" @@ -196,17 +199,7 @@ func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private return status, err } - namespaceLabels := map[string]string{ - orgIDLabelKey: remoteCentral.Spec.Auth.OwnerOrgId, - tenantIDLabelKey: remoteCentral.Id, - } - namespaceAnnotations := map[string]string{ - orgNameAnnotationKey: remoteCentral.Spec.Auth.OwnerOrgName, - } - if remoteCentral.Metadata.ExpiredAt != nil { - namespaceAnnotations[centralExpiredAtKey] = remoteCentral.Metadata.ExpiredAt.Format(time.RFC3339) - } - if err := r.reconcileNamespace(ctx, remoteCentralNamespace, namespaceLabels, namespaceAnnotations); err != nil { + if err := r.reconcileNamespace(ctx, remoteCentral); err != nil { return nil, errors.Wrapf(err, "unable to ensure that namespace %s exists", remoteCentralNamespace) } @@ -1133,17 +1126,11 @@ func (r *CentralReconciler) computeCentralHash(central private.ManagedCentral) ( } func (r *CentralReconciler) getNamespace(name string) (*corev1.Namespace, error) { - namespace := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - } - err := r.client.Get(context.Background(), ctrlClient.ObjectKey{Name: name}, namespace) - if err != nil { - // Propagate corev1.Namespace to the caller so that the namespace can be easily created - return namespace, fmt.Errorf("retrieving resource for namespace %q from Kubernetes: %w", name, err) + var namespace corev1.Namespace + if err := r.client.Get(context.Background(), ctrlClient.ObjectKey{Name: name}, &namespace); err != nil { + return nil, fmt.Errorf("getting namespace %q: %w", name, err) } - return namespace, nil + return &namespace, nil } func (r *CentralReconciler) getSecret(namespaceName string, secretName string) (*corev1.Secret, error) { @@ -1178,36 +1165,54 @@ func (r *CentralReconciler) createImagePullSecret(ctx context.Context, namespace return nil } -func (r *CentralReconciler) createTenantNamespace(ctx context.Context, namespace *corev1.Namespace) error { - err := r.client.Create(ctx, namespace) - if err != nil { - return fmt.Errorf("creating namespace %q: %w", namespace.ObjectMeta.Name, err) - } - return nil -} +func (r *CentralReconciler) reconcileNamespace(ctx context.Context, c private.ManagedCentral) error { + desiredNamespace := getDesiredNamespace(c) -func (r *CentralReconciler) reconcileNamespace(ctx context.Context, name string, labels map[string]string, annotations map[string]string) error { - namespace, err := r.getNamespace(name) + existingNamespace, err := r.getNamespace(desiredNamespace.Name) if err != nil { if apiErrors.IsNotFound(err) { - namespace.Annotations = annotations - namespace.Labels = labels - return r.createTenantNamespace(ctx, namespace) - } - return fmt.Errorf("getting namespace %s: %w", name, err) - } else if !maps.Equal(labels, namespace.Labels) || - !maps.Equal(annotations, namespace.Annotations) { - namespace.Annotations = annotations - namespace.Labels = labels - if err = r.client.Update(ctx, namespace, &ctrlClient.UpdateOptions{ + if err := r.client.Create(ctx, desiredNamespace); err != nil { + return fmt.Errorf("creating namespace %q: %w", desiredNamespace.Name, err) + } + return nil + } + return fmt.Errorf("getting namespace %q: %w", desiredNamespace.Name, err) + } + + if stringMapNeedsUpdating(desiredNamespace.Annotations, existingNamespace.Annotations) || stringMapNeedsUpdating(desiredNamespace.Labels, existingNamespace.Labels) { + glog.Infof("Updating namespace %q", desiredNamespace.Name) + if existingNamespace.Annotations == nil { + existingNamespace.Annotations = map[string]string{} + } + for k, v := range desiredNamespace.Annotations { + existingNamespace.Annotations[k] = v + } + if existingNamespace.Labels == nil { + existingNamespace.Labels = map[string]string{} + } + for k, v := range desiredNamespace.Labels { + existingNamespace.Labels[k] = v + } + if err = r.client.Update(ctx, existingNamespace, &ctrlClient.UpdateOptions{ FieldManager: "fleetshard-sync", }); err != nil { - return fmt.Errorf("updating namespace %s: %w", name, err) + return fmt.Errorf("updating namespace %q: %w", desiredNamespace.Name, err) } } + return nil } +func getDesiredNamespace(c private.ManagedCentral) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: c.Metadata.Namespace, + Annotations: getNamespaceAnnotations(c), + Labels: getNamespaceLabels(c), + }, + } +} + func (r *CentralReconciler) ensureImagePullSecretConfigured(ctx context.Context, namespaceName string, secretName string, imagePullSecret []byte) error { // Ensure that the secret exists. _, err := r.getSecret(namespaceName, secretName) @@ -1682,15 +1687,42 @@ func (r *CentralReconciler) ensureRouteDeleted(ctx context.Context, routeSupplie return false, nil } -func (r *CentralReconciler) chartValues(_ private.ManagedCentral) (chartutil.Values, error) { +func getTenantLabels(c private.ManagedCentral) map[string]string { + return map[string]string{ + managedByLabelKey: labelManagedByFleetshardValue, + instanceLabelKey: c.Metadata.Name, + orgIDLabelKey: c.Spec.Auth.OwnerOrgId, + tenantIDLabelKey: c.Id, + instanceTypeLabelKey: c.Spec.InstanceType, + } +} + +func getTenantAnnotations(c private.ManagedCentral) map[string]string { + return map[string]string{ + orgNameAnnotationKey: c.Spec.Auth.OwnerOrgName, + } +} + +func getNamespaceLabels(c private.ManagedCentral) map[string]string { + return getTenantLabels(c) +} + +func getNamespaceAnnotations(c private.ManagedCentral) map[string]string { + namespaceAnnotations := getTenantAnnotations(c) + if c.Metadata.ExpiredAt != nil { + namespaceAnnotations[centralExpiredAtKey] = c.Metadata.ExpiredAt.Format(time.RFC3339) + } + return namespaceAnnotations +} + +func (r *CentralReconciler) chartValues(c private.ManagedCentral) (chartutil.Values, error) { if r.resourcesChart == nil { return nil, errors.New("resources chart is not set") } src := r.resourcesChart.Values dst := map[string]interface{}{ - "labels": map[string]interface{}{ - k8s.ManagedByLabelKey: k8s.ManagedByFleetshardValue, - }, + "labels": stringMapToMapInterface(getTenantLabels(c)), + "annotations": stringMapToMapInterface(getTenantAnnotations(c)), } if r.egressProxyImage != "" { dst["egressProxy"] = map[string]interface{}{ @@ -1700,6 +1732,14 @@ func (r *CentralReconciler) chartValues(_ private.ManagedCentral) (chartutil.Val return chartutil.CoalesceTables(dst, src), nil } +func stringMapToMapInterface(m map[string]string) map[string]interface{} { + result := make(map[string]interface{}, len(m)) + for k, v := range m { + result[k] = v + } + return result +} + func (r *CentralReconciler) shouldSkipReadyCentral(remoteCentral private.ManagedCentral) bool { return r.wantsAuthProvider == r.hasAuthProvider && isRemoteCentralReady(&remoteCentral) diff --git a/fleetshard/pkg/central/reconciler/reconciler_test.go b/fleetshard/pkg/central/reconciler/reconciler_test.go index 7648eb14ee..ed19668600 100644 --- a/fleetshard/pkg/central/reconciler/reconciler_test.go +++ b/fleetshard/pkg/central/reconciler/reconciler_test.go @@ -47,6 +47,7 @@ import ( "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" yaml2 "sigs.k8s.io/yaml" ) @@ -2287,6 +2288,215 @@ metadata: } +func TestReconciler_reconcileNamespace(t *testing.T) { + tests := []struct { + name string + existingNamespace *v1.Namespace + wantErr bool + wantNamespace *v1.Namespace + expectUpdate bool + expectCreate bool + }{ + { + name: "namespace should be created if it doesn't exist", + expectCreate: true, + wantNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + }, + }, + }, + }, + { + name: "namespace with wrong labels or annotations should be updated", + expectUpdate: true, + existingNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "wrong", + "app.kubernetes.io/managed-by": "wrong", + "rhacs.redhat.com/instance-type": "wrong", + "rhacs.redhat.com/org-id": "wrong", + "rhacs.redhat.com/tenant": "wrong", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "wrong", + }, + }, + }, + wantNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + }, + }, + }, + }, + { + name: "extra labels/annotations should remain untouched", + existingNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + "extra": "extra", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + "extra": "extra", + }, + }, + }, + wantNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + "extra": "extra", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + "extra": "extra", + }, + }, + }, + }, + { + name: "namespace should not be updated if it's already correct", + existingNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + }, + }, + }, + wantNamespace: &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: simpleManagedCentral.Metadata.Namespace, + Labels: map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + }, + Annotations: map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + }, + }, + }, + }, + } + + managedCentral := simpleManagedCentral + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient, _, r := getClientTrackerAndReconciler(t, simpleManagedCentral, nil, defaultReconcilerOptions) + if tt.existingNamespace != nil { + require.NoError(t, fakeClient.Create(context.Background(), tt.existingNamespace)) + } + updateCount := 0 + createCount := 0 + r.client = interceptor.NewClient(fakeClient, interceptor.Funcs{ + Update: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updateCount++ + return client.Update(ctx, obj, opts...) + }, + Create: func(ctx context.Context, client client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + createCount++ + return client.Create(ctx, obj, opts...) + }, + }) + err := r.reconcileNamespace(context.Background(), managedCentral) + if tt.wantErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + var gotNamespace v1.Namespace + err := fakeClient.Get(context.Background(), client.ObjectKey{Name: simpleManagedCentral.Metadata.Namespace}, &gotNamespace) + require.NoError(t, err) + assert.Equal(t, tt.wantNamespace.Name, gotNamespace.Name) + assert.Equal(t, tt.wantNamespace.Labels, gotNamespace.Labels) + assert.Equal(t, tt.wantNamespace.Annotations, gotNamespace.Annotations) + if tt.expectUpdate { + assert.Equal(t, 1, updateCount, "update should be called") + } else { + assert.Equal(t, 0, updateCount, "update should not be called") + } + + if tt.expectCreate { + assert.Equal(t, 1, createCount, "create should be called") + } else { + assert.Equal(t, 0, createCount, "create should not be called") + } + } + }) + } +} + +func TestReconciler_ensureChartResourcesExist_labelsAndAnnotations(t *testing.T) { + fakeClient, _, r := getClientTrackerAndReconciler(t, simpleManagedCentral, nil, defaultReconcilerOptions) + require.NoError(t, r.ensureChartResourcesExist(context.Background(), simpleManagedCentral)) + + var egressProxyDeployment appsv1.Deployment + err := fakeClient.Get(context.Background(), client.ObjectKey{ + Namespace: simpleManagedCentral.Metadata.Namespace, + Name: "egress-proxy", + }, &egressProxyDeployment) + require.NoError(t, err) + + assert.Equal(t, map[string]string{ + "app.kubernetes.io/instance": "test-central", + "app.kubernetes.io/managed-by": "rhacs-fleetshard", + "rhacs.redhat.com/instance-type": "standard", + "rhacs.redhat.com/org-id": "12345", + "rhacs.redhat.com/tenant": "cb45idheg5ip6dq1jo4g", + "app.kubernetes.io/component": "egress-proxy", + "app.kubernetes.io/name": "central-tenant-resources", + "helm.sh/chart": "central-tenant-resources-0.0.0", + }, egressProxyDeployment.ObjectMeta.Labels) + + assert.Equal(t, map[string]string{ + "rhacs.redhat.com/org-name": "org-name", + }, egressProxyDeployment.ObjectMeta.Annotations) + +} + func TestReconciler_needsReconcile(t *testing.T) { tests := []struct { name string