Bug fix - block user updates on spec.userInfo (#399)

SAP · Feb 22, 2024 · bb925b0 · bb925b0
1 parent b434879
commit bb925b0
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 30 deletions.
diff --git 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
@@ -30,42 +30,40 @@ 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)
 	}
 
 	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
@@ -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
@@ -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() {