From 15f4aba0e0750b61e30a54a3d37f31a54b7a98e2 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 14 Oct 2022 09:17:13 +0200 Subject: [PATCH] spod: Make the webhook's ObjectSelector configurable Implements a suggestion from code review of the PR that made webhooks opt-in. I wanted to document a way to make the objectSelector configurable and to my surprise found out that this is not possible because we don't implement configurable objectSelectors. --- api/spod/v1alpha1/spod_types.go | 3 + api/spod/v1alpha1/zz_generated.deepcopy.go | 5 + ...8s.io_securityprofilesoperatordaemons.yaml | 45 +++++++++ .../crds/securityprofilesoperatordaemon.yaml | 45 +++++++++ deploy/helm/crds/crds.yaml | 45 +++++++++ deploy/namespace-operator.yaml | 45 +++++++++ deploy/openshift-dev.yaml | 45 +++++++++ deploy/operator.yaml | 45 +++++++++ deploy/webhook-operator.yaml | 45 +++++++++ installation-usage.md | 22 +++-- internal/pkg/manager/spod/bindata/webhook.go | 16 ++++ test/tc_webhook_config_test.go | 92 +++++++++++-------- 12 files changed, 404 insertions(+), 49 deletions(-) diff --git a/api/spod/v1alpha1/spod_types.go b/api/spod/v1alpha1/spod_types.go index 1cc66ab020..c818fbd18f 100644 --- a/api/spod/v1alpha1/spod_types.go +++ b/api/spod/v1alpha1/spod_types.go @@ -44,6 +44,9 @@ type WebhookOptions struct { // NamespaceSelector sets webhook's namespace selector // +optional NamespaceSelector *metav1.LabelSelector `json:"namespaceSelector,omitempty"` + // ObjectSelector sets webhook's object selector + // +optional + ObjectSelector *metav1.LabelSelector `json:"objectSelector,omitempty"` } // SPODStatus defines the desired state of SPOD. diff --git a/api/spod/v1alpha1/zz_generated.deepcopy.go b/api/spod/v1alpha1/zz_generated.deepcopy.go index a5ff1b40b9..8bb885a3e3 100644 --- a/api/spod/v1alpha1/zz_generated.deepcopy.go +++ b/api/spod/v1alpha1/zz_generated.deepcopy.go @@ -192,6 +192,11 @@ func (in *WebhookOptions) DeepCopyInto(out *WebhookOptions) { *out = new(metav1.LabelSelector) (*in).DeepCopyInto(*out) } + if in.ObjectSelector != nil { + in, out := &in.ObjectSelector, &out.ObjectSelector + *out = new(metav1.LabelSelector) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new WebhookOptions. diff --git a/bundle/manifests/security-profiles-operator.x-k8s.io_securityprofilesoperatordaemons.yaml b/bundle/manifests/security-profiles-operator.x-k8s.io_securityprofilesoperatordaemons.yaml index 0bc40a1546..c7c718af45 100644 --- a/bundle/manifests/security-profiles-operator.x-k8s.io_securityprofilesoperatordaemons.yaml +++ b/bundle/manifests/security-profiles-operator.x-k8s.io_securityprofilesoperatordaemons.yaml @@ -1050,6 +1050,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/base-crds/crds/securityprofilesoperatordaemon.yaml b/deploy/base-crds/crds/securityprofilesoperatordaemon.yaml index 5b5dda3ad0..a43564f502 100644 --- a/deploy/base-crds/crds/securityprofilesoperatordaemon.yaml +++ b/deploy/base-crds/crds/securityprofilesoperatordaemon.yaml @@ -1048,6 +1048,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/helm/crds/crds.yaml b/deploy/helm/crds/crds.yaml index b453073028..3425398585 100644 --- a/deploy/helm/crds/crds.yaml +++ b/deploy/helm/crds/crds.yaml @@ -1575,6 +1575,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/namespace-operator.yaml b/deploy/namespace-operator.yaml index 3d48baef58..4e184753d0 100644 --- a/deploy/namespace-operator.yaml +++ b/deploy/namespace-operator.yaml @@ -1575,6 +1575,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/openshift-dev.yaml b/deploy/openshift-dev.yaml index a8d0eb6a4f..2ef97f6318 100644 --- a/deploy/openshift-dev.yaml +++ b/deploy/openshift-dev.yaml @@ -1575,6 +1575,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/operator.yaml b/deploy/operator.yaml index b09323212b..1fded4ba50 100644 --- a/deploy/operator.yaml +++ b/deploy/operator.yaml @@ -1575,6 +1575,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/deploy/webhook-operator.yaml b/deploy/webhook-operator.yaml index c9a7cbe4bd..5b3dcfe45d 100644 --- a/deploy/webhook-operator.yaml +++ b/deploy/webhook-operator.yaml @@ -1751,6 +1751,51 @@ spec: type: object type: object x-kubernetes-map-type: atomic + objectSelector: + description: ObjectSelector sets webhook's object selector + properties: + matchExpressions: + description: matchExpressions is a list of label selector + requirements. The requirements are ANDed. + items: + description: A label selector requirement is a selector + that contains values, a key, and an operator that relates + the key and values. + properties: + key: + description: key is the label key that the selector + applies to. + type: string + operator: + description: operator represents a key's relationship + to a set of values. Valid operators are In, NotIn, + Exists and DoesNotExist. + type: string + values: + description: values is an array of string values. + If the operator is In or NotIn, the values array + must be non-empty. If the operator is Exists or + DoesNotExist, the values array must be empty. This + array is replaced during a strategic merge patch. + items: + type: string + type: array + required: + - key + - operator + type: object + type: array + matchLabels: + additionalProperties: + type: string + description: matchLabels is a map of {key,value} pairs. + A single {key,value} in the matchLabels map is equivalent + to an element of matchExpressions, whose key field is + "key", the operator is "In", and the values array contains + only "value". The requirements are ANDed. + type: object + type: object + x-kubernetes-map-type: atomic type: object type: array type: object diff --git a/installation-usage.md b/installation-usage.md index 498c481ef2..8ee41aa35a 100644 --- a/installation-usage.md +++ b/installation-usage.md @@ -1167,16 +1167,20 @@ security_profiles_operator_seccomp_profile_audit_total{container="log-container" Both profile binding and profile recording make use of webhooks. Their configuration (an instance of `MutatingWebhookConfiguration` CR) is managed by SPO itself and not part of the deployed YAML manifests. While the defaults should be acceptable for the majority of users and the webhooks do nothing unless an -instance of either `ProfileBinding` or `ProfileRecording` exists in a namespace, it might still be useful -to configure the webhooks. - -In order to change webhook's configuration, the `spod` CR exposes an object `webhookOptions` that allows -the `failurePolicy` and the `namespaceSelector` to be set. This way you can set the webhooks to "soft-fail" -or restrict them to a subset of a namespaces so that even if the webhooks had a bug that would prevent them -from running at all, other namespaces or resources wouldn't be affected. +instance of either `ProfileBinding` or `ProfileRecording` exists in a namespace and in addition the +namespace must be labeled with either `spo.x-k8s.io/enable-binding` or `spo.x-k8s.io/enable-recording` +respectively by default, it might still be useful to configure the webhooks. + +In order to change webhook's configuration, the `spod` CR exposes an object +`webhookOptions` that allows the `failurePolicy`, `namespaceSelector` +and `objectSelector` to be set. This way you can set the webhooks to +"soft-fail" or restrict them to a subset of a namespaces and inside those namespaces +select only a subset of object matching the `objectSelector` so that even +if the webhooks had a bug that would prevent them from running at all, +other namespaces or resources wouldn't be affected. For example, to set the `binding.spo.io` webhook's configuration to ignore errors as well as restrict it -to a subset of namespaces labeled with `spo.x-k8s.io/enable-binding=true`, create a following patch file: +to a subset of namespaces labeled with `spo.x-k8s.io/bind-here=true`, create a following patch file: ```yaml spec: @@ -1185,7 +1189,7 @@ spec: failurePolicy: Ignore namespaceSelector: matchExpressions: - - key: spo.x-k8s.io/enable-binding + - key: spo.x-k8s.io/bind-here operator: In values: - "true" diff --git a/internal/pkg/manager/spod/bindata/webhook.go b/internal/pkg/manager/spod/bindata/webhook.go index 86cb39803c..b704aef0bc 100644 --- a/internal/pkg/manager/spod/bindata/webhook.go +++ b/internal/pkg/manager/spod/bindata/webhook.go @@ -185,6 +185,10 @@ func applyWebhookOptions(cfg *admissionregv1.MutatingWebhookConfiguration, opts if userOpt.NamespaceSelector != nil { hook.NamespaceSelector = userOpt.NamespaceSelector } + + if userOpt.ObjectSelector != nil { + hook.ObjectSelector = userOpt.ObjectSelector + } } } } @@ -247,6 +251,18 @@ func webhookNeedsUpdate(existing, configured *admissionregv1.MutatingWebhook) bo return true } + if existing.ObjectSelector == nil && configured.ObjectSelector != nil || + existing.ObjectSelector != nil && configured.ObjectSelector == nil { + // comparing pointers, not values + return true + } + + if existing.ObjectSelector != nil && + configured.ObjectSelector != nil && + !reflect.DeepEqual(*existing.ObjectSelector, *configured.ObjectSelector) { + return true + } + return false } diff --git a/test/tc_webhook_config_test.go b/test/tc_webhook_config_test.go index 88054c7dcc..b3a5a27397 100644 --- a/test/tc_webhook_config_test.go +++ b/test/tc_webhook_config_test.go @@ -21,67 +21,79 @@ import ( "time" ) -const whNamespaceSelector = `{"matchExpressions":[{"key":"prod","operator":"In","values":["true"]}]}` +const ( + whNamespaceSelector = `{"matchExpressions":[{"key":"prod","operator":"In","values":["true"]}]}` + whObjectSelector = `{"matchExpressions":[{"key":"record-pod","operator":"In","values":["true"]}]}` +) + +type whConfigOutput struct { + name string + namespaceSelector string + objectSelector string + failurePolicy string +} + +const ( + bindingIdx = iota + recordingIdx + numWebhooks +) func (e *e2e) testCaseWebhookOptionsChange([]string) { if !e.testWebhookConfig { e.T().Skip("Skipping webhook config related tests") } e.logf("Change webhook options") - origOutput0 := e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"binding.spo.io\")].namespaceSelector}") - origOutput1 := e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"recording.spo.io\")].namespaceSelector}") - whPatch := fmt.Sprintf(`{"spec":{"webhookOptions":[{"name":"binding.spo.io","failurePolicy":"Ignore","namespaceSelector":%s}]}}`, whNamespaceSelector) //nolint:lll // very long patch line + whDefault := e.getAllWebhookAttributes() + + whPatch := fmt.Sprintf(`{"spec":{"webhookOptions":[{"name":"binding.spo.io","failurePolicy":"Ignore","namespaceSelector":%s, "objectSelector":%s}]}}`, whNamespaceSelector, whObjectSelector) //nolint:lll // very long patch line e.logf(whPatch) e.kubectlOperatorNS("patch", "spod", "spod", "-p", whPatch, "--type=merge") time.Sleep(defaultWaitTime) // check the configured hook - output := e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"binding.spo.io\")].failurePolicy}") - e.Equal("Ignore", output) - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"binding.spo.io\")].namespaceSelector}") - e.Equal(whNamespaceSelector, output) - + whPatchedConfig := e.getAllWebhookAttributes() + e.Equal("Ignore", whPatchedConfig[bindingIdx].failurePolicy) + e.Equal(whNamespaceSelector, whPatchedConfig[bindingIdx].namespaceSelector) + e.Equal(whObjectSelector, whPatchedConfig[bindingIdx].objectSelector) // check the other hook did not change - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"recording.spo.io\")].failurePolicy}") - e.Equal("Fail", output) - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"recording.spo.io\")].namespaceSelector}") - e.Equal(origOutput1, output) + e.Equal("Fail", whPatchedConfig[recordingIdx].failurePolicy) + e.Equal(whDefault[recordingIdx].namespaceSelector, whPatchedConfig[recordingIdx].namespaceSelector) // go back to defaults e.kubectlOperatorNS("patch", "spod", "spod", "-p", `{"spec":{"webhookOptions":[]}}`, "--type=merge") time.Sleep(defaultWaitTime) // check we are back to defaults - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"binding.spo.io\")].failurePolicy}") - e.Equal("Fail", output) + whRevertedConfig := e.getAllWebhookAttributes() + e.Equal("Fail", whRevertedConfig[bindingIdx].failurePolicy) + e.Equal(whDefault[bindingIdx].namespaceSelector, whRevertedConfig[bindingIdx].namespaceSelector) + // check the other hook did not change + e.Equal("Fail", whRevertedConfig[recordingIdx].failurePolicy) + e.Equal(whDefault[recordingIdx].namespaceSelector, whRevertedConfig[recordingIdx].namespaceSelector) +} - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"binding.spo.io\")].namespaceSelector}") - e.Equal(origOutput0, output) +func getWhConfigs() []*whConfigOutput { + whConfigs := make([]*whConfigOutput, numWebhooks) + whConfigs[0] = &whConfigOutput{name: "binding.spo.io"} + whConfigs[1] = &whConfigOutput{name: "recording.spo.io"} + return whConfigs +} - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", - "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"recording.spo.io\")].failurePolicy}") - e.Equal("Fail", output) +func (e *e2e) getAllWebhookAttributes() []*whConfigOutput { + out := getWhConfigs() + for i := range out { + out[i].objectSelector = e.getWebhookAttribute(out[i].name, "objectSelector") + out[i].namespaceSelector = e.getWebhookAttribute(out[i].name, "namespaceSelector") + out[i].failurePolicy = e.getWebhookAttribute(out[i].name, "failurePolicy") + } + return out +} - output = e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", +func (e *e2e) getWebhookAttribute(hook, attr string) string { + jsonPath := fmt.Sprintf("{.webhooks[?(@.name==\"%s\")].%s}", hook, attr) + return e.kubectlOperatorNS("get", "MutatingWebhookConfiguration", "spo-mutating-webhook-configuration", - "--output", "jsonpath={.webhooks[?(@.name==\"recording.spo.io\")].namespaceSelector}") - e.Equal(origOutput1, output) + "--output", "jsonpath="+jsonPath) }