From 198df861e7354ffc3cf3cdbcc8202a92edc9e724 Mon Sep 17 00:00:00 2001 From: hsmatulis Date: Mon, 27 Oct 2025 18:33:32 -0400 Subject: [PATCH] chore: remove operator cleanup logic b/422809019 Signed-off-by: Henrique Matulis --- cmd/datasource-syncer/datasource-syncer.yaml | 2 +- e2e/webhook_test.go | 100 ----------- manifests/operator.yaml | 2 +- manifests/rule-evaluator.yaml | 2 +- pkg/operator/operator.go | 69 -------- pkg/operator/operator_test.go | 164 ------------------- 6 files changed, 3 insertions(+), 336 deletions(-) delete mode 100644 e2e/webhook_test.go delete mode 100644 pkg/operator/operator_test.go diff --git a/cmd/datasource-syncer/datasource-syncer.yaml b/cmd/datasource-syncer/datasource-syncer.yaml index 783d35df05..044c7414f2 100644 --- a/cmd/datasource-syncer/datasource-syncer.yaml +++ b/cmd/datasource-syncer/datasource-syncer.yaml @@ -1,4 +1,4 @@ -# Copyright 2025 Google LLC +# Copyright 2026 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/e2e/webhook_test.go b/e2e/webhook_test.go deleted file mode 100644 index c0324b4335..0000000000 --- a/e2e/webhook_test.go +++ /dev/null @@ -1,100 +0,0 @@ -// Copyright 2024 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package e2e - -import ( - "context" - "strings" - "testing" - "time" - - "github.com/GoogleCloudPlatform/prometheus-engine/e2e/deploy" - "github.com/GoogleCloudPlatform/prometheus-engine/pkg/operator" - appsv1 "k8s.io/api/apps/v1" - rbacv1 "k8s.io/api/rbac/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/wait" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// TestWebhooksNoRBAC validates that the operator works without any webhook RBAC policies. -func TestWebhooksNoRBAC(t *testing.T) { - ctx := contextWithDeadline(t) - kubeClient, restConfig, err := setupCluster(ctx, t) - if err != nil { - t.Fatalf("error instantiating clients. err: %s", err) - } - - if err := kubeClient.Delete(ctx, &rbacv1.ClusterRole{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gmp-system:operator:webhook-admin", - }, - }); err != nil { - t.Fatalf("error deleting cluster role: %s", err) - } - if err := kubeClient.Delete(ctx, &rbacv1.ClusterRoleBinding{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gmp-system:operator:webhook-admin", - }, - }); err != nil { - t.Fatalf("error deleting cluster role binding: %s", err) - } - - // Restart the GMP operator since it is already healthy before we delete the RBAC policies. - t.Log("restarting operator") - if err := deploymentRestart(ctx, kubeClient, operator.DefaultOperatorNamespace, operator.NameOperator); err != nil { - t.Fatalf("error restarting operator. err: %s", err) - } - - t.Log("waiting for operator to be deployed") - if err := deploy.WaitForOperatorReady(ctx, kubeClient); err != nil { - t.Fatalf("error waiting for operator deployment to be ready: %s", err) - } - - if err := wait.PollUntilContextCancel(ctx, 3*time.Second, true, func(ctx context.Context) (bool, error) { - logs, err := deploy.OperatorLogs(ctx, restConfig, kubeClient, operator.DefaultOperatorNamespace) - if err != nil { - t.Logf("unable to get operator logs: %s", err) - return false, nil - } - - t.Logf("waiting for operator logs to contain RBAC message") - return strings.Contains(logs, "delete legacy ValidatingWebHookConfiguration was not allowed"), nil - }); err != nil { - t.Fatalf("unable to check operator logs: %s", err) - } -} - -func deploymentRestart(ctx context.Context, kubeClient client.Client, namespace, name string) error { - deploy := appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: namespace, - Name: name, - }, - } - if err := kubeClient.Get(ctx, client.ObjectKeyFromObject(&deploy), &deploy); err != nil { - return err - } - deployPatch := deploy.DeepCopy() - if deployPatch.Spec.Template.Annotations == nil { - deployPatch.Spec.Template.Annotations = make(map[string]string) - } - deployPatch.Spec.Template.Annotations["kubectl.kubernetes.io/restartedAt"] = time.Now().Format(time.RFC3339) - if err := kubeClient.Patch(ctx, deployPatch, client.MergeFrom(&deploy)); err != nil { - return err - } - - return nil -} diff --git a/manifests/operator.yaml b/manifests/operator.yaml index f67028c909..318c47fcf2 100644 --- a/manifests/operator.yaml +++ b/manifests/operator.yaml @@ -1,4 +1,4 @@ -# Copyright 2025 Google LLC +# Copyright 2026 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/manifests/rule-evaluator.yaml b/manifests/rule-evaluator.yaml index dd14ce2607..01659cf1ea 100644 --- a/manifests/rule-evaluator.yaml +++ b/manifests/rule-evaluator.yaml @@ -1,4 +1,4 @@ -# Copyright 2025 Google LLC +# Copyright 2026 Google LLC # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index 9d6d665316..5d6b8cbe07 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -26,11 +26,9 @@ import ( "github.com/go-logr/logr" "github.com/prometheus/client_golang/api" "github.com/prometheus/client_golang/prometheus" - arv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" @@ -338,9 +336,6 @@ func New(logger logr.Logger, clientConfig *rest.Config, opts Options) (*Operator func (o *Operator) Run(ctx context.Context, registry prometheus.Registerer) error { defer runtimeutil.HandleCrash() - if err := o.cleanupOldResources(ctx); err != nil { - return fmt.Errorf("cleanup old resources: %w", err) - } if err := setupAdmissionWebhooks(ctx, o.logger, o.client, o.manager.GetWebhookServer().(*webhook.DefaultServer), &o.opts, o.vpaAvailable); err != nil { return fmt.Errorf("init admission resources: %w", err) } @@ -366,70 +361,6 @@ func (o *Operator) Run(ctx context.Context, registry prometheus.Registerer) erro return o.manager.Start(ctx) } -func (o *Operator) cleanupOldResources(ctx context.Context) error { - // Delete old ValidatingWebhookConfiguration that was installed directly by the operator. - // in previous versions. - validatingWebhookConfig := arv1.ValidatingWebhookConfiguration{ - ObjectMeta: metav1.ObjectMeta{Name: "gmp-operator"}, - } - if err := o.client.Delete(ctx, &validatingWebhookConfig); err != nil { - switch { - case apierrors.IsForbidden(err): - o.logger.Info("delete legacy ValidatingWebHookConfiguration was not allowed. Please remove it manually") - case !apierrors.IsNotFound(err): - return fmt.Errorf("delete legacy ValidatingWebHookConfiguration failed: %w", err) - } - } - - // If cleanup annotations are not provided, do not clean up any further. - if o.opts.CleanupAnnotKey == "" { - return nil - } - - // Cleanup resources without the provided annotation. - // Check the collector DaemonSet. - dsKey := client.ObjectKey{ - Name: NameCollector, - Namespace: o.opts.OperatorNamespace, - } - var ds appsv1.DaemonSet - if err := o.client.Get(ctx, dsKey, &ds); apierrors.IsNotFound(err) { - return fmt.Errorf("get collector DaemonSet: %w", err) - } - if _, ok := ds.Annotations[o.opts.CleanupAnnotKey]; !ok { - if err := o.client.Delete(ctx, &ds); err != nil { - switch { - case apierrors.IsForbidden(err): - o.logger.Info("delete collector was not allowed. Please remove it manually", "err", err) - case !apierrors.IsNotFound(err): - return fmt.Errorf("cleanup collector failed: %w", err) - } - } - } - - // Check the rule-evaluator Deployment. - deployKey := client.ObjectKey{ - Name: NameRuleEvaluator, - Namespace: o.opts.OperatorNamespace, - } - var deploy appsv1.Deployment - if err := o.client.Get(ctx, deployKey, &deploy); apierrors.IsNotFound(err) { - return fmt.Errorf("get rule-evaluator Deployment: %w", err) - } - if _, ok := deploy.Annotations[o.opts.CleanupAnnotKey]; !ok { - if err := o.client.Delete(ctx, &deploy); err != nil { - switch { - case apierrors.IsForbidden(err): - o.logger.Info("delete rule-evaluator was not allowed. Please remove it manually", "err", err) - case !apierrors.IsNotFound(err): - return fmt.Errorf("cleanup rule-evaluator failed: %w", err) - } - } - } - - return nil -} - // namespacedNamePredicate is an event filter predicate that only allows events with // a single object. type namespacedNamePredicate struct { diff --git a/pkg/operator/operator_test.go b/pkg/operator/operator_test.go deleted file mode 100644 index ac2d99ee53..0000000000 --- a/pkg/operator/operator_test.go +++ /dev/null @@ -1,164 +0,0 @@ -// Copyright 2022 Google LLC -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package operator - -import ( - "testing" - - appsv1 "k8s.io/api/apps/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - "github.com/go-logr/logr/testr" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" -) - -func TestCleanupOldResources(t *testing.T) { - var cases = []struct { - desc string - cleanupAnnotKey string - collectorAnnots map[string]string - evaluatorAnnots map[string]string - collectorDeleted bool - evaluatorDeleted bool - }{ - { - desc: "keep both", - cleanupAnnotKey: "dont-cleanme", - collectorAnnots: map[string]string{ - "dont-cleanme": "true", - }, - evaluatorAnnots: map[string]string{ - "dont-cleanme": "true", - }, - collectorDeleted: false, - evaluatorDeleted: false, - }, - { - desc: "delete both", - cleanupAnnotKey: "dont-cleanme", - collectorAnnots: map[string]string{ - "cleanme": "true", - }, - evaluatorAnnots: map[string]string{ - "cleanme": "true", - }, - collectorDeleted: true, - evaluatorDeleted: true, - }, - { - desc: "delete collector", - cleanupAnnotKey: "dont-cleanme", - collectorAnnots: map[string]string{ - "cleanme": "true", - }, - evaluatorAnnots: map[string]string{ - "dont-cleanme": "true", - }, - collectorDeleted: true, - evaluatorDeleted: false, - }, - { - desc: "delete rule-evaluator", - cleanupAnnotKey: "dont-cleanme", - collectorAnnots: map[string]string{ - "dont-cleanme": "true", - }, - evaluatorAnnots: map[string]string{ - "cleanme": "true", - }, - collectorDeleted: false, - evaluatorDeleted: true, - }, - { - desc: "keep both", - cleanupAnnotKey: "", - collectorAnnots: map[string]string{ - "dont-cleanme": "true", - }, - evaluatorAnnots: map[string]string{ - "cleanme": "true", - }, - collectorDeleted: false, - evaluatorDeleted: false, - }, - } - - for _, c := range cases { - t.Run(c.desc, func(t *testing.T) { - ds := &appsv1.DaemonSet{ - ObjectMeta: v1.ObjectMeta{ - Name: NameCollector, - Namespace: "gmp-system", - Annotations: c.collectorAnnots, - }, - } - - deploy := &appsv1.Deployment{ - ObjectMeta: v1.ObjectMeta{ - Name: NameRuleEvaluator, - Namespace: "gmp-system", - Annotations: c.evaluatorAnnots, - }, - } - opts := Options{ - ProjectID: "test-proj", - Location: "test-loc", - Cluster: "test-cluster", - OperatorNamespace: "gmp-system", - CleanupAnnotKey: c.cleanupAnnotKey, - } - cl := fake.NewClientBuilder().WithObjects(ds, deploy).Build() - - op := &Operator{ - logger: testr.New(t), - opts: opts, - client: cl, - } - if err := op.cleanupOldResources(t.Context()); err != nil { - t.Fatal(err) - } - - // Check if collector DaemonSet was preserved. - var gotDS appsv1.DaemonSet - dsErr := cl.Get(t.Context(), client.ObjectKey{ - Name: NameCollector, - Namespace: "gmp-system", - }, &gotDS) - if c.collectorDeleted { - if !apierrors.IsNotFound(dsErr) { - t.Errorf("collector should be deleted but found: %+v", gotDS) - } - } else if gotDS.Name != ds.Name || gotDS.Namespace != ds.Namespace { - t.Errorf("collector DaemonSet differs") - } - - // Check if rule-evaluator Deployment was preserved. - var gotDeploy appsv1.Deployment - deployErr := cl.Get(t.Context(), client.ObjectKey{ - Name: NameRuleEvaluator, - Namespace: "gmp-system", - }, &gotDeploy) - if c.evaluatorDeleted { - if !apierrors.IsNotFound(deployErr) { - t.Errorf("rule-evaluator should be deleted but found: %+v", gotDeploy) - } - } else if gotDeploy.Name != deploy.Name || gotDeploy.Namespace != deploy.Namespace { - t.Errorf("rule-evaluator Deployment differs") - } - }) - } -}