From 8fa558492c10473dd62e7403e4974b9511400a4f Mon Sep 17 00:00:00 2001 From: Ekaterina Kazakova Date: Thu, 19 Sep 2024 12:22:14 +0400 Subject: [PATCH] Allow Templates deletion by the controller only The admission controller will block the removal of any Template managed by HMC or from the system namespace if the request is not made by the HMC controller. --- internal/controller/suite_test.go | 24 ++++- internal/controller/template_controller.go | 22 ++--- .../controller/template_controller_test.go | 2 +- internal/templateutil/interface.go | 36 ++++++++ internal/webhook/template_webhook.go | 91 ++++++++++++++++--- internal/webhook/template_webhook_test.go | 61 +++++++++++-- internal/webhook/userinfo.go | 30 ++++++ .../provider/hmc/templates/deployment.yaml | 2 + test/objects/template/template.go | 9 ++ 9 files changed, 232 insertions(+), 45 deletions(-) create mode 100644 internal/templateutil/interface.go create mode 100644 internal/webhook/userinfo.go diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index d6627aae1..967ad5d7c 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -29,10 +29,12 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" admissionv1 "k8s.io/api/admissionregistration/v1" + authenticationv1 "k8s.io/api/authentication/v1" utilyaml "sigs.k8s.io/cluster-api/util/yaml" ctrl "sigs.k8s.io/controller-runtime" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" helmcontrollerv2 "github.com/fluxcd/helm-controller/api/v2" sourcev1 "github.com/fluxcd/source-controller/api/v1" @@ -54,6 +56,8 @@ import ( const ( mutatingWebhookKind = "MutatingWebhookConfiguration" validatingWebhookKind = "ValidatingWebhookConfiguration" + + hmcServiceAccountName = "hmc-controller-manager" ) var ( @@ -62,6 +66,8 @@ var ( testEnv *envtest.Environment ctx context.Context cancel context.CancelFunc + + userInfo = authenticationv1.UserInfo{Username: fmt.Sprintf("system:serviceaccount:hmc-system:%s", hmcServiceAccountName)} ) func TestControllers(t *testing.T) { @@ -82,6 +88,9 @@ var _ = BeforeSuite(func() { ) Expect(err).NotTo(HaveOccurred()) + err = os.Setenv(hmcwebhook.ServiceAccountEnvName, hmcServiceAccountName) + Expect(err).To(Succeed()) + testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{ filepath.Join("..", "..", "templates", "provider", "hmc", "templates", "crds"), @@ -153,13 +162,19 @@ var _ = BeforeSuite(func() { err = (&hmcwebhook.ServiceTemplateChainValidator{}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) - err = (&hmcwebhook.ClusterTemplateValidator{}).SetupWebhookWithManager(mgr) + templateValidator := hmcwebhook.TemplateValidator{ + InjectUserInfo: func(req *admission.Request) { + req.UserInfo = userInfo + }, + } + + err = (&hmcwebhook.ClusterTemplateValidator{TemplateValidator: templateValidator}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) - err = (&hmcwebhook.ServiceTemplateValidator{}).SetupWebhookWithManager(mgr) + err = (&hmcwebhook.ServiceTemplateValidator{TemplateValidator: templateValidator}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) - err = (&hmcwebhook.ProviderTemplateValidator{}).SetupWebhookWithManager(mgr) + err = (&hmcwebhook.ProviderTemplateValidator{TemplateValidator: templateValidator}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) go func() { @@ -185,6 +200,9 @@ var _ = AfterSuite(func() { cancel() err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) + + err = os.Unsetenv(hmcwebhook.ServiceAccountEnvName) + Expect(err).To(Succeed()) }) func loadWebhooks(path string) ([]*admissionv1.ValidatingWebhookConfiguration, []*admissionv1.MutatingWebhookConfiguration, error) { diff --git a/internal/controller/template_controller.go b/internal/controller/template_controller.go index b2171e886..56465a4b5 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -36,6 +36,7 @@ import ( hmc "github.com/Mirantis/hmc/api/v1alpha1" "github.com/Mirantis/hmc/internal/helm" + "github.com/Mirantis/hmc/internal/templateutil" ) const ( @@ -123,14 +124,7 @@ func (r *ProviderTemplateReconciler) Reconcile(ctx context.Context, req ctrl.Req return r.ReconcileTemplate(ctx, providerTemplate) } -// Template is the interface defining a list of methods to interact with templates -type Template interface { - client.Object - GetSpec() *hmc.TemplateSpecCommon - GetStatus() *hmc.TemplateStatusCommon -} - -func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template Template) (ctrl.Result, error) { +func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template templateutil.Template) (ctrl.Result, error) { l := log.FromContext(ctx) spec := template.GetSpec() @@ -149,7 +143,7 @@ func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template Tem l.Error(err, "invalid helm chart reference") return ctrl.Result{}, err } - if template.GetNamespace() == r.SystemNamespace || !templateManagedByHMC(template) { + if template.GetNamespace() == r.SystemNamespace || !templateutil.IsManagedByHMC(template) { err := r.reconcileDefaultHelmRepository(ctx, template.GetNamespace()) if err != nil { l.Error(err, "Failed to reconcile default HelmRepository", "namespace", template.GetNamespace()) @@ -222,11 +216,7 @@ func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template Tem return ctrl.Result{}, r.updateStatus(ctx, template, "") } -func templateManagedByHMC(template Template) bool { - return template.GetLabels()[hmc.HMCManagedLabelKey] == hmc.HMCManagedLabelValue -} - -func parseChartMetadata(template Template, inChart *chart.Chart) error { +func parseChartMetadata(template templateutil.Template, inChart *chart.Chart) error { if inChart.Metadata == nil { return fmt.Errorf("chart metadata is empty") } @@ -261,7 +251,7 @@ func parseChartMetadata(template Template, inChart *chart.Chart) error { return nil } -func (r *TemplateReconciler) updateStatus(ctx context.Context, template Template, validationError string) error { +func (r *TemplateReconciler) updateStatus(ctx context.Context, template templateutil.Template, validationError string) error { status := template.GetStatus() status.ObservedGeneration = template.GetGeneration() status.ValidationError = validationError @@ -312,7 +302,7 @@ func (r *TemplateReconciler) reconcileDefaultHelmRepository(ctx context.Context, return nil } -func (r *TemplateReconciler) reconcileHelmChart(ctx context.Context, template Template) (*sourcev1.HelmChart, error) { +func (r *TemplateReconciler) reconcileHelmChart(ctx context.Context, template templateutil.Template) (*sourcev1.HelmChart, error) { spec := template.GetSpec() namespace := template.GetNamespace() if namespace == "" { diff --git a/internal/controller/template_controller_test.go b/internal/controller/template_controller_test.go index 8a95ed2e8..45f51c70a 100644 --- a/internal/controller/template_controller_test.go +++ b/internal/controller/template_controller_test.go @@ -170,7 +170,7 @@ var _ = Describe("Template Controller", func() { err = k8sClient.Get(ctx, typeNamespacedName, providerTemplateResource) Expect(err).NotTo(HaveOccurred()) - By("Cleanup the specific resource instance ClusterTemplate") + By("Cleanup the specific resource instance ProviderTemplate") Expect(k8sClient.Delete(ctx, providerTemplateResource)).To(Succeed()) }) It("should successfully reconcile the resource", func() { diff --git a/internal/templateutil/interface.go b/internal/templateutil/interface.go new file mode 100644 index 000000000..a1a700d56 --- /dev/null +++ b/internal/templateutil/interface.go @@ -0,0 +1,36 @@ +// Copyright 2024 +// +// 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 +// +// http://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 templateutil + +import ( + "sigs.k8s.io/controller-runtime/pkg/client" + + hmc "github.com/Mirantis/hmc/api/v1alpha1" +) + +// Template is the interface defining a list of methods to interact with templates +type Template interface { + client.Object + GetSpec() *hmc.TemplateSpecCommon + GetStatus() *hmc.TemplateStatusCommon +} + +func IsManagedByHMC(template Template) bool { + labels := template.GetLabels() + if labels == nil { + return false + } + return labels[hmc.HMCManagedLabelKey] == hmc.HMCManagedLabelValue +} diff --git a/internal/webhook/template_webhook.go b/internal/webhook/template_webhook.go index da075a04f..1c5da57fc 100644 --- a/internal/webhook/template_webhook.go +++ b/internal/webhook/template_webhook.go @@ -18,6 +18,7 @@ import ( "context" "errors" "fmt" + "os" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/fields" @@ -28,13 +29,21 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/templateutil" ) -type ClusterTemplateValidator struct { +var errTemplateDeletionForbidden = errors.New("template deletion is forbidden") + +type TemplateValidator struct { client.Client + + SystemNamespace string + InjectUserInfo func(*admission.Request) } -var errTemplateDeletionForbidden = errors.New("template deletion is forbidden") +type ClusterTemplateValidator struct { + TemplateValidator +} func (v *ClusterTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { v.Client = mgr.GetClient() @@ -66,6 +75,13 @@ func (v *ClusterTemplateValidator) ValidateDelete(ctx context.Context, obj runti if !ok { return admission.Warnings{"Wrong object"}, apierrors.NewBadRequest(fmt.Sprintf("expected ClusterTemplate but got a %T", obj)) } + deletionAllowed, err := v.isTemplateDeletionAllowed(ctx, template) + if err != nil { + return nil, fmt.Errorf("failed to check if the ClusterTemplate %s/%s is allowed to be deleted: %v", template.Namespace, template.Name, err) + } + if !deletionAllowed { + return nil, errTemplateDeletionForbidden + } managedClusters := &v1alpha1.ManagedClusterList{} listOptions := client.ListOptions{ @@ -73,7 +89,7 @@ func (v *ClusterTemplateValidator) ValidateDelete(ctx context.Context, obj runti Limit: 1, Namespace: template.Namespace, } - err := v.Client.List(ctx, managedClusters, &listOptions) + err = v.Client.List(ctx, managedClusters, &listOptions) if err != nil { return nil, err } @@ -91,15 +107,15 @@ func (*ClusterTemplateValidator) Default(context.Context, runtime.Object) error } type ServiceTemplateValidator struct { - client.Client + TemplateValidator } -func (in *ServiceTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { - in.Client = mgr.GetClient() +func (v *ServiceTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + v.Client = mgr.GetClient() return ctrl.NewWebhookManagedBy(mgr). For(&v1alpha1.ServiceTemplate{}). - WithValidator(in). - WithDefaulter(in). + WithValidator(v). + WithDefaulter(v). Complete() } @@ -119,7 +135,18 @@ func (*ServiceTemplateValidator) ValidateUpdate(_ context.Context, _ runtime.Obj } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (*ServiceTemplateValidator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { +func (v *ServiceTemplateValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + template, ok := obj.(*v1alpha1.ServiceTemplate) + if !ok { + return admission.Warnings{"Wrong object"}, apierrors.NewBadRequest(fmt.Sprintf("expected ServiceTemplate but got a %T", obj)) + } + deletionAllowed, err := v.isTemplateDeletionAllowed(ctx, template) + if err != nil { + return nil, fmt.Errorf("failed to check if the ServiceTemplate %s/%s is allowed to be deleted: %v", template.Namespace, template.Name, err) + } + if !deletionAllowed { + return nil, errTemplateDeletionForbidden + } return nil, nil } @@ -129,15 +156,15 @@ func (*ServiceTemplateValidator) Default(_ context.Context, _ runtime.Object) er } type ProviderTemplateValidator struct { - client.Client + TemplateValidator } -func (in *ProviderTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { - in.Client = mgr.GetClient() +func (v *ProviderTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + v.Client = mgr.GetClient() return ctrl.NewWebhookManagedBy(mgr). For(&v1alpha1.ProviderTemplate{}). - WithValidator(in). - WithDefaulter(in). + WithValidator(v). + WithDefaulter(v). Complete() } @@ -157,7 +184,18 @@ func (*ProviderTemplateValidator) ValidateUpdate(_ context.Context, _ runtime.Ob } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. -func (*ProviderTemplateValidator) ValidateDelete(_ context.Context, _ runtime.Object) (admission.Warnings, error) { +func (v *ProviderTemplateValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + template, ok := obj.(*v1alpha1.ProviderTemplate) + if !ok { + return admission.Warnings{"Wrong object"}, apierrors.NewBadRequest(fmt.Sprintf("expected ProviderTemplate but got a %T", obj)) + } + deletionAllowed, err := v.isTemplateDeletionAllowed(ctx, template) + if err != nil { + return nil, fmt.Errorf("failed to check if the ProviderTemplate %s is allowed to be deleted: %v", template.Name, err) + } + if !deletionAllowed { + return nil, errTemplateDeletionForbidden + } return nil, nil } @@ -165,3 +203,26 @@ func (*ProviderTemplateValidator) ValidateDelete(_ context.Context, _ runtime.Ob func (*ProviderTemplateValidator) Default(_ context.Context, _ runtime.Object) error { return nil } + +func (v TemplateValidator) isTemplateDeletionAllowed(ctx context.Context, template templateutil.Template) (bool, error) { + req, err := admission.RequestFromContext(ctx) + if err != nil { + return false, err + } + if v.InjectUserInfo != nil { + v.InjectUserInfo(&req) + } + // Allow all templates' deletion for the HMC controller + if serviceAccountIsEqual(req, os.Getenv(ServiceAccountEnvName)) { + return true, nil + } + // Cluster-scoped ProviderTemplates and Templates from the system namespace are not allowed to be deleted + if template.GetNamespace() == "" || template.GetNamespace() == v.SystemNamespace { + return false, nil + } + // Allow template deletion if template is not managed by the TemplateManagement + if !templateutil.IsManagedByHMC(template) { + return true, nil + } + return false, nil +} diff --git a/internal/webhook/template_webhook_test.go b/internal/webhook/template_webhook_test.go index d8291b3a7..ea59271a9 100644 --- a/internal/webhook/template_webhook_test.go +++ b/internal/webhook/template_webhook_test.go @@ -16,9 +16,12 @@ package webhook import ( "context" + "fmt" "testing" . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -26,25 +29,44 @@ import ( "github.com/Mirantis/hmc/api/v1alpha1" "github.com/Mirantis/hmc/test/objects/managedcluster" "github.com/Mirantis/hmc/test/objects/template" + tm "github.com/Mirantis/hmc/test/objects/templatemanagement" "github.com/Mirantis/hmc/test/scheme" ) func TestClusterTemplateValidateDelete(t *testing.T) { g := NewWithT(t) ctx := context.Background() + namespace := "test" + hmcServiceAccountName := "hmc-controller-manager" + + tmName := "test" + systemNamespace := "hmc" + tpl := template.NewClusterTemplate(template.WithName("testTemplateFail"), template.WithNamespace(namespace)) - tplTest := template.NewClusterTemplate(template.WithName("testTemplate"), template.WithNamespace(namespace)) tests := []struct { name string template *v1alpha1.ClusterTemplate existingObjects []runtime.Object + userInfo authenticationv1.UserInfo err string warnings admission.Warnings }{ { - name: "should fail if ManagedCluster objects exist in the same namespace", + name: "should fail if the template is managed by HMC and the user triggered the deletion", + template: template.NewClusterTemplate(template.ManagedByHMC()), + existingObjects: []runtime.Object{tm.NewTemplateManagement(tm.WithName(tmName))}, + err: "template deletion is forbidden", + }, + { + name: "should fail if the template is in the system namespace", + template: template.NewClusterTemplate(template.WithNamespace(systemNamespace)), + existingObjects: []runtime.Object{}, + err: "template deletion is forbidden", + }, + { + name: "should fail if ManagedCluster object referencing the template exists in the same namespace", template: tpl, existingObjects: []runtime.Object{managedcluster.NewManagedCluster( managedcluster.WithNamespace(namespace), @@ -62,14 +84,20 @@ func TestClusterTemplateValidateDelete(t *testing.T) { )}, }, { - name: "should be OK because of a different cluster", + name: "should succeed if the template is managed by HMC and the controller triggered the deletion", + template: template.NewClusterTemplate(template.ManagedByHMC()), + userInfo: authenticationv1.UserInfo{Username: fmt.Sprintf("system:serviceaccount:hmc-system:%s", hmcServiceAccountName)}, + existingObjects: []runtime.Object{tm.NewTemplateManagement(tm.WithName(tmName))}, + }, + { + name: "should succeed if the template is not managed by HMC", template: tpl, - existingObjects: []runtime.Object{managedcluster.NewManagedCluster()}, + existingObjects: []runtime.Object{tm.NewTemplateManagement(tm.WithName(tmName))}, }, { - name: "should succeed", - template: template.NewClusterTemplate(), - existingObjects: []runtime.Object{managedcluster.NewManagedCluster(managedcluster.WithTemplate(tplTest.Name))}, + name: "should succeed because no cluster references the template", + template: tpl, + existingObjects: []runtime.Object{managedcluster.NewManagedCluster()}, }, } @@ -78,10 +106,23 @@ func TestClusterTemplateValidateDelete(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme.Scheme). WithRuntimeObjects(tt.existingObjects...). - WithIndex(tt.existingObjects[0], v1alpha1.TemplateKey, v1alpha1.ExtractTemplateName). + WithIndex(&v1alpha1.ManagedCluster{}, v1alpha1.TemplateKey, v1alpha1.ExtractTemplateName). Build() - validator := &ClusterTemplateValidator{Client: c} - warn, err := validator.ValidateDelete(ctx, tt.template) + validator := &ClusterTemplateValidator{ + TemplateValidator: TemplateValidator{ + Client: c, + SystemNamespace: systemNamespace, + }, + } + + t.Setenv(ServiceAccountEnvName, hmcServiceAccountName) + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: tt.userInfo, + }, + } + warn, err := validator.ValidateDelete(admission.NewContextWithRequest(ctx, req), tt.template) if tt.err != "" { g.Expect(err).To(HaveOccurred()) if err.Error() != tt.err { diff --git a/internal/webhook/userinfo.go b/internal/webhook/userinfo.go new file mode 100644 index 000000000..534f9c04e --- /dev/null +++ b/internal/webhook/userinfo.go @@ -0,0 +1,30 @@ +// Copyright 2024 +// +// 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 +// +// http://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 webhook + +import ( + "strings" + + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +const ( + ServiceAccountEnvName = "SERVICE_ACCOUNT" +) + +func serviceAccountIsEqual(req admission.Request, serviceAccount string) bool { + return strings.HasPrefix(req.UserInfo.Username, "system:serviceaccount:") && + strings.HasSuffix(req.UserInfo.Username, ":"+serviceAccount) +} diff --git a/templates/provider/hmc/templates/deployment.yaml b/templates/provider/hmc/templates/deployment.yaml index 925c9d491..2c04e8f3c 100644 --- a/templates/provider/hmc/templates/deployment.yaml +++ b/templates/provider/hmc/templates/deployment.yaml @@ -37,6 +37,8 @@ spec: env: - name: KUBERNETES_CLUSTER_DOMAIN value: {{ quote .Values.kubernetesClusterDomain }} + - name: SERVICE_ACCOUNT + value: {{ include "hmc.fullname" . }}-controller-manager image: {{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }} imagePullPolicy: {{ .Values.image.pullPolicy }} diff --git a/test/objects/template/template.go b/test/objects/template/template.go index 668f40191..f3184be39 100644 --- a/test/objects/template/template.go +++ b/test/objects/template/template.go @@ -92,6 +92,15 @@ func WithLabels(labels map[string]string) Opt { } } +func ManagedByHMC() Opt { + return func(t *Template) { + if t.Labels == nil { + t.Labels = make(map[string]string) + } + t.Labels[v1alpha1.HMCManagedLabelKey] = v1alpha1.HMCManagedLabelValue + } +} + func WithHelmSpec(helmSpec v1alpha1.HelmSpec) Opt { return func(t *Template) { t.Spec.Helm = helmSpec