From f27b9ce1515059daa8a251392a09fe3e117cbfd2 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 In case the TemplateManagement object exists on the environment the admission controller will block the removal of any Template managed by HMC. --- internal/controller/suite_test.go | 38 +++++-- internal/controller/template_controller.go | 26 +---- .../controller/template_controller_test.go | 2 +- internal/templateutil/interface.go | 36 ++++++ internal/webhook/template_webhook.go | 106 +++++++++++++++--- internal/webhook/template_webhook_test.go | 70 ++++++++++-- internal/webhook/userinfo.go | 30 +++++ .../provider/hmc/templates/deployment.yaml | 2 + test/objects/template/template.go | 9 ++ 9 files changed, 262 insertions(+), 57 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 aeb6e1057..7a3fa557a 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" @@ -55,13 +57,19 @@ import ( const ( mutatingWebhookKind = "MutatingWebhookConfiguration" validatingWebhookKind = "ValidatingWebhookConfiguration" + + hmcServiceAccountName = "hmc-controller-manager" ) -var cfg *rest.Config -var k8sClient client.Client -var testEnv *envtest.Environment -var ctx context.Context -var cancel context.CancelFunc +var ( + cfg *rest.Config + k8sClient client.Client + 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) { RegisterFailHandler(Fail) @@ -81,6 +89,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"), @@ -147,13 +158,21 @@ var _ = BeforeSuite(func() { err = (&hmcwebhook.TemplateManagementValidator{}).SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) - err = (&hmcwebhook.ClusterTemplateValidator{}).SetupWebhookWithManager(mgr) + injectUserInfo := func(req *admission.Request) { + req.UserInfo = userInfo + } + + templateValidator := hmcwebhook.TemplateValidator{ + InjectUserInfo: injectUserInfo, + } + + 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() { @@ -179,6 +198,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 4d5b909f6..bc5cf4f02 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -37,6 +37,7 @@ import ( hmc "github.com/Mirantis/hmc/api/v1alpha1" "github.com/Mirantis/hmc/internal/helm" + "github.com/Mirantis/hmc/internal/templateutil" ) const ( @@ -124,14 +125,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.TemplateSpecMixin - GetStatus() *hmc.TemplateStatusMixin -} - -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() @@ -150,7 +144,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()) @@ -223,15 +217,7 @@ func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template Tem return ctrl.Result{}, r.updateStatus(ctx, template, "") } -func templateManagedByHMC(template Template) bool { - labels := template.GetLabels() - if labels == nil { - return false - } - return labels[hmc.HMCManagedLabelKey] == hmc.HMCManagedLabelValue -} - -func (r *TemplateReconciler) parseChartMetadata(template Template, chart *chart.Chart) error { +func (r *TemplateReconciler) parseChartMetadata(template templateutil.Template, chart *chart.Chart) error { if chart.Metadata == nil { return fmt.Errorf("chart metadata is empty") } @@ -266,7 +252,7 @@ func (r *TemplateReconciler) parseChartMetadata(template Template, chart *chart. 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 @@ -317,7 +303,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 8433e192c..6210e8c2c 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..d99d91cd3 --- /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.TemplateSpecMixin + GetStatus() *hmc.TemplateStatusMixin +} + +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 9812c788a..83cb3f35f 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,22 +29,30 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/templateutil" ) -type ClusterTemplateValidator struct { +type TemplateValidator struct { client.Client + + SystemNamespace string + InjectUserInfo func(*admission.Request) +} + +type ClusterTemplateValidator struct { + TemplateValidator } var ( ErrTemplateDeletionForbidden = errors.New("template deletion is forbidden") ) -func (in *ClusterTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { - in.Client = mgr.GetClient() +func (v *ClusterTemplateValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { + v.Client = mgr.GetClient() return ctrl.NewWebhookManagedBy(mgr). For(&v1alpha1.ClusterTemplate{}). - WithValidator(in). - WithDefaulter(in). + WithValidator(v). + WithDefaulter(v). Complete() } @@ -68,13 +77,20 @@ 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{ FieldSelector: fields.SelectorFromSet(fields.Set{v1alpha1.TemplateKey: template.Name}), Limit: 1, } - err := v.Client.List(ctx, managedClusters, &listOptions) + err = v.Client.List(ctx, managedClusters, &listOptions) if err != nil { return nil, err } @@ -92,15 +108,15 @@ func (*ClusterTemplateValidator) Default(_ context.Context, _ runtime.Object) er } 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() } @@ -120,7 +136,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 } @@ -130,15 +157,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() } @@ -158,7 +185,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.ServiceTemplate) + 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 } @@ -166,3 +204,35 @@ 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 + } + // Templates in the system namespace are not allowed to be deleted + if template.GetNamespace() == v.SystemNamespace { + return false, nil + } + tmList := &v1alpha1.TemplateManagementList{} + err = v.List(ctx, tmList) + if err != nil { + return false, err + } + // Allow template deletion if TemplateManagement object is not created + if len(tmList.Items) == 0 { + return true, 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 c02d9ef20..592afdc91 100644 --- a/internal/webhook/template_webhook_test.go +++ b/internal/webhook/template_webhook_test.go @@ -16,9 +16,13 @@ package webhook import ( "context" + "fmt" + "os" "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,6 +30,7 @@ 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" ) @@ -33,31 +38,58 @@ func TestClusterTemplateValidateDelete(t *testing.T) { g := NewWithT(t) ctx := context.Background() tpl := template.NewClusterTemplate(template.WithName("testTemplateFail")) - tplTest := template.NewClusterTemplate(template.WithName("testTemplate")) + + hmcServiceAccountName := "hmc-controller-manager" + + tmName := "test" + systemNamespace := "hmc" 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", + 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", template: tpl, existingObjects: []runtime.Object{managedcluster.NewManagedCluster(managedcluster.WithTemplate(tpl.Name))}, warnings: admission.Warnings{"The ClusterTemplate object can't be removed if ManagedCluster objects referencing it still exist"}, err: "template deletion is forbidden", }, { - 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 if the template is managed by HMC but no TemplateManagement object found", + template: template.NewClusterTemplate(template.ManagedByHMC()), }, { - 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()}, }, } @@ -66,10 +98,28 @@ 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, + }, + } + + err := os.Setenv(ServiceAccountEnvName, hmcServiceAccountName) + g.Expect(err).To(Succeed()) + defer func() { + err = os.Unsetenv(ServiceAccountEnvName) + g.Expect(err).To(Succeed()) + }() + + 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 b97f8e818..541304e24 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