From bb925b06bffb97716916bd8e404db80211e101cf Mon Sep 17 00:00:00 2001 From: kerenlahav <45451976+kerenlahav@users.noreply.github.com> Date: Thu, 22 Feb 2024 14:39:59 +0200 Subject: [PATCH] Bug fix - block user updates on spec.userInfo (#399) --- .../servicebinding_mutating_webhook.go | 6 +-- .../serviceinstance_mutating_webhook.go | 52 +++++++++---------- controllers/servicebinding_controller_test.go | 14 +++++ .../serviceinstance_controller_test.go | 14 +++++ 4 files changed, 56 insertions(+), 30 deletions(-) diff --git a/api/v1/webhooks/servicebinding_mutating_webhook.go b/api/v1/webhooks/servicebinding_mutating_webhook.go index 4c8f6e2f..23edbe95 100644 --- a/api/v1/webhooks/servicebinding_mutating_webhook.go +++ b/api/v1/webhooks/servicebinding_mutating_webhook.go @@ -48,7 +48,7 @@ func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Reques } } - if req.Operation == v1admission.Create || req.Operation == v1admission.Delete { + if req.Operation == v1admission.Create { binding.Spec.UserInfo = &v1.UserInfo{ Username: req.UserInfo.Username, UID: req.UserInfo.UID, @@ -57,9 +57,9 @@ func (s *ServiceBindingDefaulter) Handle(_ context.Context, req admission.Reques } } - marshaledInstance, err := json.Marshal(binding) + marshaledBinding, err := json.Marshal(binding) if err != nil { return admission.Errored(http.StatusInternalServerError, err) } - return admission.PatchResponseFromRaw(req.Object.Raw, marshaledInstance) + return admission.PatchResponseFromRaw(req.Object.Raw, marshaledBinding) } diff --git a/api/v1/webhooks/serviceinstance_mutating_webhook.go b/api/v1/webhooks/serviceinstance_mutating_webhook.go index ee889a4b..c490fd7c 100644 --- a/api/v1/webhooks/serviceinstance_mutating_webhook.go +++ b/api/v1/webhooks/serviceinstance_mutating_webhook.go @@ -30,15 +30,36 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque return admission.Errored(http.StatusBadRequest, err) } + if req.Operation == v1admission.Create { + instance.Spec.UserInfo = &v1.UserInfo{ + Username: req.UserInfo.Username, + UID: req.UserInfo.UID, + Groups: req.UserInfo.Groups, + Extra: req.UserInfo.Extra, + } + } else { + oldInstance := &servicesv1.ServiceInstance{} + err := s.Decoder.DecodeRaw(req.OldObject, oldInstance) + if err != nil { + return admission.Errored(http.StatusInternalServerError, err) + } + + if !reflect.DeepEqual(instance.Spec.UserInfo, oldInstance.Spec.UserInfo) { + return admission.Errored(http.StatusBadRequest, fmt.Errorf("modifying spec.userInfo is not allowed")) + } else if !reflect.DeepEqual(oldInstance.Spec, instance.Spec) { //UserInfo is updated only when spec is changed + instance.Spec.UserInfo = &v1.UserInfo{ + Username: req.UserInfo.Username, + UID: req.UserInfo.UID, + Groups: req.UserInfo.Groups, + Extra: req.UserInfo.Extra, + } + } + } if len(instance.Spec.ExternalName) == 0 { instancelog.Info(fmt.Sprintf("externalName not provided, defaulting to k8s name: %s", instance.Name)) instance.Spec.ExternalName = instance.Name } - if err := s.setServiceInstanceUserInfo(req, instance); err != nil { - return admission.Errored(http.StatusInternalServerError, err) - } - marshaledInstance, err := json.Marshal(instance) if err != nil { return admission.Errored(http.StatusInternalServerError, err) @@ -46,26 +67,3 @@ func (s *ServiceInstanceDefaulter) Handle(_ context.Context, req admission.Reque return admission.PatchResponseFromRaw(req.Object.Raw, marshaledInstance) } - -func (s *ServiceInstanceDefaulter) setServiceInstanceUserInfo(req admission.Request, instance *servicesv1.ServiceInstance) error { - userInfo := &v1.UserInfo{ - Username: req.UserInfo.Username, - UID: req.UserInfo.UID, - Groups: req.UserInfo.Groups, - Extra: req.UserInfo.Extra, - } - - if req.Operation == v1admission.Create || req.Operation == v1admission.Delete { - instance.Spec.UserInfo = userInfo - } else if req.Operation == v1admission.Update { - oldInstance := &servicesv1.ServiceInstance{} - err := s.Decoder.DecodeRaw(req.OldObject, oldInstance) - if err != nil { - return err - } - if !reflect.DeepEqual(oldInstance.Spec, instance.Spec) { - instance.Spec.UserInfo = userInfo - } - } - return nil -} diff --git a/controllers/servicebinding_controller_test.go b/controllers/servicebinding_controller_test.go index d91f17a5..7abf388c 100644 --- a/controllers/servicebinding_controller_test.go +++ b/controllers/servicebinding_controller_test.go @@ -7,6 +7,8 @@ import ( "net/http" "strings" + authv1 "k8s.io/api/authentication/v1" + "github.com/SAP/sap-btp-service-operator/api/common" "github.com/SAP/sap-btp-service-operator/internal/utils" "k8s.io/utils/pointer" @@ -641,6 +643,18 @@ var _ = Describe("ServiceBinding controller", func() { Expect(err.Error()).To(ContainSubstring("updating service bindings is not supported")) }) }) + + When("UserInfo changed", func() { + It("should fail", func() { + createdBinding.Spec.UserInfo = &authv1.UserInfo{ + Username: "aaa", + UID: "111", + } + err := k8sClient.Update(ctx, createdBinding) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("updating service bindings is not supported")) + }) + }) }) Context("Delete", func() { diff --git a/controllers/serviceinstance_controller_test.go b/controllers/serviceinstance_controller_test.go index 4e50afc2..01fb7399 100644 --- a/controllers/serviceinstance_controller_test.go +++ b/controllers/serviceinstance_controller_test.go @@ -6,6 +6,8 @@ import ( "net/http" "strings" + authv1 "k8s.io/api/authentication/v1" + "github.com/SAP/sap-btp-service-operator/api/common" v1 "github.com/SAP/sap-btp-service-operator/api/v1" "github.com/SAP/sap-btp-service-operator/client/sm" @@ -616,6 +618,18 @@ var _ = Describe("ServiceInstance controller", func() { Expect(err.Error()).To(ContainSubstring("changing the btpAccessCredentialsSecret for an existing instance is not allowed")) }) }) + + When("UserInfo changed", func() { + It("should fail", func() { + serviceInstance.Spec.UserInfo = &authv1.UserInfo{ + Username: "a", + UID: "123", + } + err := k8sClient.Update(ctx, serviceInstance) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("modifying spec.userInfo is not allowed")) + }) + }) }) Describe("Delete", func() {