CHAOSPLT-216: Allow for requiring specific user groups before creating a disruption (#831)

* Rename validateUserInfo

* CHAOSPLT-216: Allow for restricting disruption creation based on group

* Improve the logging and errors

---------

Co-authored-by: Matthieu Bono <luphaz@users.noreply.github.com>

Author: ptnapoleon

api/v1beta1/disruption_webhook.go

@@ -36,22 +36,24 @@ import (
 )
 
 var (
-	logger                        *zap.SugaredLogger
-	k8sClient                     client.Client
-	metricsSink                   metrics.Sink
-	tracerSink                    tracer.Sink
-	recorder                      record.EventRecorder
-	deleteOnly                    bool
-	enableSafemode                bool
-	defaultNamespaceThreshold     float64
-	defaultClusterThreshold       float64
-	handlerEnabled                bool
-	maxDuration                   time.Duration
-	defaultDuration               time.Duration
-	cloudServicesProvidersManager cloudservice.CloudServicesProvidersManager
-	chaosNamespace                string
-	ddmarkClient                  ddmark.Client
-	safemodeEnvironment           string
+	logger                          *zap.SugaredLogger
+	k8sClient                       client.Client
+	metricsSink                     metrics.Sink
+	tracerSink                      tracer.Sink
+	recorder                        record.EventRecorder
+	deleteOnly                      bool
+	enableSafemode                  bool
+	defaultNamespaceThreshold       float64
+	defaultClusterThreshold         float64
+	handlerEnabled                  bool
+	maxDuration                     time.Duration
+	defaultDuration                 time.Duration
+	cloudServicesProvidersManager   cloudservice.CloudServicesProvidersManager
+	chaosNamespace                  string
+	ddmarkClient                    ddmark.Client
+	safemodeEnvironment             string
+	permittedUserGroups             map[string]struct{}
+	permittedUserGroupWarningString string
 )
 
 const SafemodeEnvironmentAnnotation = GroupName + "/environment"
@@ -80,6 +82,13 @@ func (r *Disruption) SetupWebhookWithManager(setupWebhookConfig utils.SetupWebho
 	cloudServicesProvidersManager = setupWebhookConfig.CloudServicesProvidersManager
 	chaosNamespace = setupWebhookConfig.ChaosNamespace
 	safemodeEnvironment = setupWebhookConfig.Environment
+	permittedUserGroups = map[string]struct{}{}
+
+	for _, group := range setupWebhookConfig.PermittedUserGroups {
+		permittedUserGroups[group] = struct{}{}
+	}
+
+	permittedUserGroupWarningString = strings.Join(setupWebhookConfig.PermittedUserGroups, ",")
 
 	return ctrl.NewWebhookManagedBy(setupWebhookConfig.Manager).
 		For(r).
@@ -120,6 +129,10 @@ func (r *Disruption) ValidateCreate() error {
 		return errors.New("the controller is currently in delete-only mode, you can't create new disruptions for now")
 	}
 
+	if err = r.validateUserInfoGroup(); err != nil {
+		return err
+	}
+
 	// reject disruptions with a name which would not be a valid label value
 	// according to https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set
 	if _, err := labels.Parse(fmt.Sprintf("name=%s", r.Name)); err != nil {
@@ -233,7 +246,7 @@ func (r *Disruption) ValidateUpdate(old runtime.Object) error {
 
 	oldDisruption := old.(*Disruption)
 
-	if err := r.validateUserInfo(oldDisruption); err != nil {
+	if err := r.validateUserInfoImmutable(oldDisruption); err != nil {
 		return err
 	}
 
@@ -316,7 +329,35 @@ You first need to remove those chaos pods (and potentially their finalizers) to
 	return nil
 }
 
-func (r *Disruption) validateUserInfo(oldDisruption *Disruption) error {
+// validateUserInfoGroup checks that if permittedUserGroups is set, which is controlled in controller.safeMode.permittedUserGroups in the configmap,
+// then we will return an error if the user in r.UserInfo does not belong to any groups. If permittedUserGroups is unset, or if the user belongs to one of those
+// groups, then we will return nil
+func (r *Disruption) validateUserInfoGroup() error {
+	if len(permittedUserGroups) == 0 {
+		return nil
+	}
+
+	userInfo, err := r.UserInfo()
+	if err != nil {
+		return err
+	}
+
+	for _, group := range userInfo.Groups {
+		_, ok := permittedUserGroups[group]
+		if ok {
+			logger.Debugw("permitting user disruption creation, due to group membership", "group", group)
+
+			return nil
+		}
+	}
+
+	logger.Warnw("rejecting user from creating this disruption", "permittedUserGroups", permittedUserGroups, "userGroups", userInfo.Groups)
+
+	return fmt.Errorf("lacking sufficient authorization to create disruptions. your user groups are %s, but you must be in one of the following groups: %s", userInfo.Groups, permittedUserGroupWarningString)
+}
+
+// validateUserInfoImmutable checks that no changes have been made to the oldDisruption's UserInfo in the latest update
+func (r *Disruption) validateUserInfoImmutable(oldDisruption *Disruption) error {
 	oldUserInfo, err := oldDisruption.UserInfo()
 	if err != nil {
 		return nil

api/v1beta1/disruption_webhook_test.go

@@ -219,6 +219,7 @@ var _ = Describe("Disruption", func() {
 		Describe("general errors expectations", func() {
 			BeforeEach(func() {
 				k8sClient = makek8sClientWithDisruptionPod()
+				recorder = record.NewFakeRecorder(1)
 				tracerSink = tracernoop.New(logger)
 				deleteOnly = false
 			})
@@ -231,6 +232,7 @@ var _ = Describe("Disruption", func() {
 			AfterEach(func() {
 				k8sClient = nil
 				newDisruption = nil
+				permittedUserGroups = map[string]struct{}{}
 			})
 
 			When("disruption has delete-only mode enable", func() {
@@ -384,6 +386,32 @@ var _ = Describe("Disruption", func() {
 					Expect(newDisruption.ValidateCreate().Error()).Should(ContainSubstring("inject.notBefore must come after createPods.notBefore if both are specified"))
 				})
 			})
+
+			When("user group membership is invalid", func() {
+				It("should return an error if they lack membership", func() {
+					permittedUserGroups = map[string]struct{}{}
+					permittedUserGroups["system:nobody"] = struct{}{}
+					permittedUserGroupWarningString = "system:nobody"
+
+					err := newDisruption.ValidateCreate()
+
+					Expect(err).Should(HaveOccurred())
+					Expect(err.Error()).Should(ContainSubstring("lacking sufficient authorization to create disruptions. your user groups are [some], but you must be in one of the following groups: system:nobody"))
+				})
+
+				It("should not return an error if they are within a permitted group", func() {
+					ddmarkMock.EXPECT().ValidateStructMultierror(mock.Anything, mock.Anything).Return(&multierror.Error{})
+					permittedUserGroups = map[string]struct{}{}
+					permittedUserGroups["some"] = struct{}{}
+					permittedUserGroups["any"] = struct{}{}
+					permittedUserGroupWarningString = "some, any"
+
+					err := newDisruption.ValidateCreate()
+
+					Expect(err).ShouldNot(HaveOccurred())
+					Expect(ddmarkMock.AssertNumberOfCalls(GinkgoT(), "ValidateStructMultierror", 1)).To(BeTrue())
+				})
+			})
 		})
 
 		Describe("expectations with a disk failure disruption", func() {
@@ -502,7 +530,8 @@ func makeValidNetworkDisruption() *Disruption {
 				IntVal: 1,
 			},
 			Network: &NetworkDisruptionSpec{
-				Drop: 100,
+				Drop:  100,
+				Hosts: []NetworkDisruptionHostSpec{{Port: 80}},
 			},
 			Selector: labels.Set{
 				"name":      "random",

chart/templates/configmap.yaml

@@ -56,6 +56,10 @@ data:
         port: {{ .Values.controller.webhook.port }}
       safeMode:
         enable: {{ .Values.controller.safeMode.enable }}
+        permittedUserGroups:
+          {{- range $index, $group := .Values.controller.safeMode.permittedUserGroups }}
+          - {{ $group | quote }}
+          {{- end }}
         environment: {{ tpl .Values.controller.safeMode.environment . }}
         namespaceThreshold: {{ .Values.controller.safeMode.namespaceThreshold }}
         clusterThreshold: {{ .Values.controller.safeMode.clusterThreshold }}

chart/values.yaml

@@ -66,6 +66,8 @@ controller:
     port: 9443 # port to use to serve requests
   safeMode:
     environment: ""
+    permittedUserGroups: # (optional) specify a list of strings which represent user info groups. if set, a user must belong to at least one in order to create a Disruption
+      - system:authenticated
     enable: false
     namespaceThreshold: 80
     clusterThreshold: 66

config/config.go

@@ -53,10 +53,11 @@ type controllerWebhookConfig struct {
 }
 
 type safeModeConfig struct {
-	Environment        string `json:"environment"`
-	Enable             bool   `json:"enable"`
-	NamespaceThreshold int    `json:"namespaceThreshold"`
-	ClusterThreshold   int    `json:"clusterThreshold"`
+	Environment         string   `json:"environment"`
+	PermittedUserGroups []string `json:"permittedUserGroups"`
+	Enable              bool     `json:"enable"`
+	NamespaceThreshold  int      `json:"namespaceThreshold"`
+	ClusterThreshold    int      `json:"clusterThreshold"`
 }
 
 type injectorConfig struct {
@@ -320,6 +321,12 @@ func New(logger *zap.SugaredLogger, osArgs []string) (config, error) {
 		return cfg, err
 	}
 
+	mainFS.StringSliceVar(&cfg.Controller.SafeMode.PermittedUserGroups, "permitted-user-groups", []string{}, "Set of user groups which, if set, a user must belong to at least one in order to create a disruption")
+
+	if err := viper.BindPFlag("controller.safeMode.permittedUserGroups", mainFS.Lookup("permitted-user-groups")); err != nil {
+		return cfg, err
+	}
+
 	mainFS.BoolVar(&cfg.Controller.SafeMode.Enable, "safemode-enable", true,
 		"Enable or disable the safemode functionality of the chaos-controller")
 

main.go

@@ -343,6 +343,7 @@ func main() {
 		ChaosNamespace:                cfg.Injector.ChaosNamespace,
 		CloudServicesProvidersManager: cloudProviderManager,
 		Environment:                   cfg.Controller.SafeMode.Environment,
+		PermittedUserGroups:           cfg.Controller.SafeMode.PermittedUserGroups,
 	}
 	if err = (&chaosv1beta1.Disruption{}).SetupWebhookWithManager(setupWebhookConfig); err != nil {
 		logger.Fatalw("unable to create webhook", "webhook", chaosv1beta1.DisruptionKind, "error", err)

utils/utils.go

@@ -55,4 +55,5 @@ type SetupWebhookWithManagerConfig struct {
 	ChaosNamespace                string
 	CloudServicesProvidersManager cloudservice.CloudServicesProvidersManager
 	Environment                   string
+	PermittedUserGroups           []string
 }