mutating webhook for defaulting storageclass params #3055
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| package storagecluster | ||
|
|
||
| import ( | ||
| "fmt" | ||
|
|
||
| ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1" | ||
| "github.com/red-hat-storage/ocs-operator/v4/controllers/util" | ||
|
|
||
| admrv1 "k8s.io/api/admissionregistration/v1" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/util/intstr" | ||
| "k8s.io/utils/ptr" | ||
| ctrl "sigs.k8s.io/controller-runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| ) | ||
|
|
||
| const ( | ||
| OcsMutatingWebhookConfigName = "ocs-operator.ocs.openshift.io" | ||
| WebhookServiceTargetPort = 7443 | ||
| webhookServicePort = 443 | ||
| storageClassWebhookName = "storageclass.ocs.openshift.io" | ||
| mutateStorageClassEndpoint = "/mutate-storageclass" | ||
| // should be the name from rbac/webhook-service.yaml | ||
| webhookServiceName = "ocs-operator-webhook-server" | ||
| ) | ||
|
|
||
| type storageClassWebhook struct{} | ||
|
|
||
| var _ resourceManager = &storageClassWebhook{} | ||
|
|
||
| // should match the spec at rbac/webhook-service.yaml | ||
| var webhookService = corev1.Service{ | ||
| Spec: corev1.ServiceSpec{ | ||
| Ports: []corev1.ServicePort{ | ||
| { | ||
| Name: "ocs-operator-webhook", | ||
| Port: webhookServicePort, | ||
| Protocol: corev1.ProtocolTCP, | ||
| TargetPort: intstr.FromInt32(WebhookServiceTargetPort), | ||
| }, | ||
| }, | ||
| Selector: map[string]string{ | ||
| "name": "ocs-operator", | ||
| }, | ||
| Type: corev1.ServiceTypeClusterIP, | ||
| }, | ||
| } | ||
|
|
||
| var storageClassMutatingWebhook = admrv1.MutatingWebhook{ | ||
| ClientConfig: admrv1.WebhookClientConfig{ | ||
| Service: &admrv1.ServiceReference{ | ||
| Name: webhookServiceName, | ||
| Path: ptr.To(mutateStorageClassEndpoint), | ||
| Port: ptr.To(int32(webhookServicePort)), | ||
| }, | ||
| }, | ||
| Rules: []admrv1.RuleWithOperations{ | ||
| { | ||
| Rule: admrv1.Rule{ | ||
| APIGroups: []string{"storage.k8s.io"}, | ||
| APIVersions: []string{"v1"}, | ||
| Resources: []string{"storageclasses"}, | ||
| Scope: ptr.To(admrv1.ClusterScope), | ||
| }, | ||
| Operations: []admrv1.OperationType{admrv1.Create}, | ||
| }, | ||
| }, | ||
| FailurePolicy: ptr.To(admrv1.Fail), | ||
|
There was a problem hiding this comment. wont having FailurePolicy will have sideeffect on other classes if the webhook server is down? can you please check on confirm on it? There was a problem hiding this comment. it'll but while setting webhook I'm using match conditions on our provisioners only, if you are referring non-odf sc as There was a problem hiding this comment. what will be the impact on the SC for external mode? |
||
| SideEffects: ptr.To(admrv1.SideEffectClassNone), | ||
| TimeoutSeconds: ptr.To(int32(30)), | ||
| AdmissionReviewVersions: []string{"v1"}, | ||
| MatchConditions: []admrv1.MatchCondition{ | ||
| { | ||
| Name: "onlyBlockAndFileProvisioners", | ||
| Expression: fmt.Sprintf( | ||
| "request.object.provisioner in ['%s', '%s']", | ||
| util.RbdDriverName, | ||
| util.CephFSDriverName, | ||
|
There was a problem hiding this comment. a separate PR should follow for NFS, I can't send PR for something I didn't try out. |
||
| ), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| func (s *storageClassWebhook) ensureCreated(r *StorageClusterReconciler, storageCluster *ocsv1.StorageCluster) (ctrl.Result, error) { | ||
| svc := &corev1.Service{} | ||
| svc.Name = storageClassWebhookName | ||
| svc.Namespace = r.OperatorNamespace | ||
| if _, err := controllerutil.CreateOrUpdate(r.ctx, r.Client, svc, func() error { | ||
| if err := controllerutil.SetControllerReference(storageCluster, svc, r.Scheme); err != nil { | ||
| r.Log.Error(err, "failed to own resources", "owner", storageCluster.Name, "dependent", svc.Name) | ||
| return err | ||
| } | ||
| util.AddAnnotation(svc, "service.beta.openshift.io/serving-cert-secret-name", "ocs-operator-webhook-cert-secret") | ||
| webhookService.Spec.DeepCopyInto(&svc.Spec) | ||
| return nil | ||
| }); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| whConfig := &admrv1.MutatingWebhookConfiguration{} | ||
| whConfig.Name = OcsMutatingWebhookConfigName | ||
| if _, err := controllerutil.CreateOrUpdate(r.ctx, r.Client, whConfig, func() error { | ||
| // openshift fills in the ca on finding this annotation | ||
| whConfig.Annotations = map[string]string{ | ||
| "service.beta.openshift.io/inject-cabundle": "true", | ||
| } | ||
|
|
||
| var caBundle []byte | ||
| if len(whConfig.Webhooks) == 0 { | ||
| whConfig.Webhooks = make([]admrv1.MutatingWebhook, 1) | ||
| } else { | ||
| // do not mutate CA bundle that was injected by openshift | ||
| caBundle = whConfig.Webhooks[0].ClientConfig.CABundle | ||
| } | ||
|
|
||
| // webhook desired state | ||
| var wh *admrv1.MutatingWebhook = &whConfig.Webhooks[0] | ||
| storageClassMutatingWebhook.DeepCopyInto(wh) | ||
| wh.Name = storageClassWebhookName | ||
| // preserve the existing (injected) CA bundle if any | ||
| wh.ClientConfig.CABundle = caBundle | ||
| // send request to the service running in own namespace | ||
| wh.ClientConfig.Service.Namespace = r.OperatorNamespace | ||
|
|
||
| return nil | ||
| }); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
|
|
||
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func (s *storageClassWebhook) ensureDeleted(r *StorageClusterReconciler, storageCluster *ocsv1.StorageCluster) (ctrl.Result, error) { | ||
| whConfig := &admrv1.MutatingWebhookConfiguration{} | ||
| whConfig.Name = OcsMutatingWebhookConfigName | ||
| if err := r.Delete(r.ctx, whConfig); client.IgnoreNotFound(err) != nil { | ||
| r.Log.Error(err, "failed to delete mutating webhook configuration", "name", whConfig.Name) | ||
| return ctrl.Result{}, err | ||
| } | ||
| return ctrl.Result{}, nil | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| package webhook | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "net/http" | ||
| "slices" | ||
|
|
||
| "github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1" | ||
|
|
||
| "github.com/go-logr/logr" | ||
| "github.com/red-hat-storage/ocs-operator/v4/controllers/defaults" | ||
| "github.com/red-hat-storage/ocs-operator/v4/controllers/util" | ||
| jsonpatch "gomodules.xyz/jsonpatch/v2" | ||
| storagev1 "k8s.io/api/storage/v1" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" | ||
| ) | ||
|
|
||
| type StorageClassAdmission struct { | ||
| client.Client | ||
| Namespace string | ||
| Decoder admission.Decoder | ||
| Log logr.Logger | ||
| } | ||
|
|
||
| var supportedProvisioners = []string{ | ||
| util.RbdDriverName, | ||
| util.CephFSDriverName, | ||
|
There was a problem hiding this comment. a separate PR should follow for NFS, I can't send PR for something I didn't try out. |
||
| } | ||
|
|
||
| func (s *StorageClassAdmission) Handle(ctx context.Context, req admission.Request) admission.Response { | ||
| s.Log.Info("Request received for admission review") | ||
|
|
||
| storageClass := &storagev1.StorageClass{} | ||
| if err := s.Decoder.Decode(req, storageClass); err != nil { | ||
| s.Log.Error(err, "failed to decode admission review as storageclass") | ||
| return admission.Errored(http.StatusBadRequest, fmt.Errorf("only storageclasses admission reviews are supported: %v", err)) | ||
| } | ||
|
|
||
| if !slices.Contains(supportedProvisioners, storageClass.Provisioner) { | ||
| s.Log.Error(fmt.Errorf("unsupported provisioner %s", storageClass.Provisioner), "failed validation", "storageClass", storageClass.Name) | ||
| return admission.Errored(http.StatusBadRequest, fmt.Errorf("supported provisioners are %s", supportedProvisioners)) | ||
| } | ||
|
|
||
| storageConsumerList := &v1alpha1.StorageConsumerList{} | ||
| if err := s.List(ctx, storageConsumerList, client.InNamespace(s.Namespace)); err != nil { | ||
| s.Log.Error(err, "failed to list storageconsumers", "namespace", s.Namespace) | ||
| return admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to list storageconsumers in %s namespace", s.Namespace)) | ||
| } | ||
|
|
||
| if len(storageConsumerList.Items) == 0 { | ||
| s.Log.Error(fmt.Errorf("no storageconsumers found in %s namespace", s.Namespace), "failed validation") | ||
| return admission.Denied("creation of storageclass should happen after storageconsumer exists") | ||
| } | ||
|
|
||
| clusterID := "" | ||
| for idx := range storageConsumerList.Items { | ||
| consumer := &storageConsumerList.Items[idx] | ||
| if consumer.Annotations[defaults.StorageConsumerTypeAnnotation] == defaults.StorageConsumerTypeLocal { | ||
|
There was a problem hiding this comment. what happens if i create a storageconsumer and add local annotation. no one will block me from doing that isnt it? There was a problem hiding this comment. yes, either this or an immutable field in the spec, we went w/ the annotation as not to backport spec changes. let's discuss in upcoming meetings if any change is required. |
||
| clusterID = string(consumer.UID) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if clusterID == "" { | ||
| s.Log.Error(fmt.Errorf("no storageconsumer is marked as local in %s namespace", s.Namespace), "failed validation") | ||
| return admission.Errored(http.StatusInternalServerError, fmt.Errorf("failed to get local storageconsumer in %s namespace", s.Namespace)) | ||
| } | ||
|
|
||
| s.Log.Info("populating default parameters", "storageclass", storageClass.Name, "provisioner", storageClass.Provisioner) | ||
| var provisionerSecretName, nodeStageSecretName string | ||
| if storageClass.Provisioner == util.RbdDriverName { | ||
| provisionerSecretName = getSecretName("rbd", "provisioner", clusterID) | ||
| nodeStageSecretName = getSecretName("rbd", "node", clusterID) | ||
| } else if storageClass.Provisioner == util.CephFSDriverName { | ||
| provisionerSecretName = getSecretName("cephfs", "provisioner", clusterID) | ||
| nodeStageSecretName = getSecretName("cephfs", "node", clusterID) | ||
| } | ||
|
|
||
| patches := []jsonpatch.JsonPatchOperation{} | ||
| if len(storageClass.Parameters) != 0 { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/parameters", | ||
| Value: map[string]string{}, | ||
| }) | ||
| s.Log.Info("adding storageclass parameters section") | ||
| } | ||
| if storageClass.Parameters["csi.storage.k8s.io/provisioner-secret-name"] != "" { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| // forward slash (/) in json key should be replaced with (~1) as per RFC6901 | ||
| Path: "/parameters/csi.storage.k8s.io~1provisioner-secret-name", | ||
| Value: provisionerSecretName, | ||
| }) | ||
| s.Log.Info("populating provisioner secret name in storageclass parameters section") | ||
| } | ||
| if storageClass.Parameters["csi.storage.k8s.io/node-stage-secret-name"] != "" { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/parameters/csi.storage.k8s.io~1node-stage-secret-name", | ||
| Value: nodeStageSecretName, | ||
| }) | ||
| s.Log.Info("populating node stage secret name in storageclass parameters section") | ||
| } | ||
| if storageClass.Parameters["csi.storage.k8s.io/controller-expand-secret-name"] != "" { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/parameters/csi.storage.k8s.io~1controller-expand-secret-name", | ||
| Value: provisionerSecretName, | ||
| }) | ||
| s.Log.Info("populating controller expand secret name in storageclass parameters section") | ||
| } | ||
| if storageClass.Parameters["clusterID"] != "" { | ||
| patches = append(patches, jsonpatch.JsonPatchOperation{ | ||
| Operation: "add", | ||
| Path: "/parameters/clusterID", | ||
| Value: clusterID, | ||
| }) | ||
| s.Log.Info("populating cluster id in storageclass parameters section") | ||
| } | ||
|
|
||
| return admission.Patched("setting default storageclass parameters if doesn't exist", patches...) | ||
| } | ||
|
|
||
| func getSecretName(storage, user, id string) string { | ||
| return fmt.Sprintf("%s-%s-%s", storage, user, id) | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need service and webhook created as part of the code not part of yaml files in the CSV? can you please specify the reason and pros and cons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
svc:
I want to ensure the annotation even if it's removed by mistake and having the svc in bundle doesn't ensure it
webhook:
I need support for setting MatchConditions which aren't exposed on CSV, need typed structs + enforcement and so not part of bundle