Create a CSV webhook #516

iamniting · 2025-01-15T07:51:40Z

Signed-off-by: Nitin Goyal nigoyal@redhat.com

openshift-ci · 2025-01-15T07:51:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [iamniting]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

leelavg · 2025-01-17T10:17:06Z

config/default/kustomization.yaml

@@ -25,6 +25,7 @@ namePrefix: odf-operator-
 # endpoint w/o any authn/z, please comment the following line.
 patchesStrategicMerge:
 - manager_auth_proxy_patch.yaml
+- manager_webhook_patch.yaml


why is this required?

It has webhook related changes port and volume details.

leelavg · 2025-01-17T10:24:48Z

controllers/webhook.go

+	csvWebhook := admrv1.MutatingWebhook{
+		Name: WebhookName,
+		ClientConfig: admrv1.WebhookClientConfig{
+			Service: &admrv1.ServiceReference{
+				Name: WebhookServiceName,
+				Path: ptr.To(odfwebhook.WebhookPath),
+				Port: ptr.To(int32(443)),
+			},
+		},
+		Rules: []admrv1.RuleWithOperations{
+			{
+				Rule: admrv1.Rule{
+					APIGroups:   []string{"operators.coreos.com"},
+					APIVersions: []string{"v1alpha1"},
+					Resources:   []string{"clusterserviceversions"},
+					Scope:       ptr.To(admrv1.NamespacedScope),
+				},
+				Operations: []admrv1.OperationType{admrv1.Create},
+			},
+		},
+		SideEffects:             ptr.To(admrv1.SideEffectClassNone),
+		TimeoutSeconds:          ptr.To(int32(30)),
+		AdmissionReviewVersions: []string{"v1"},
+		// fail the validation if webhook can't be reached
+		FailurePolicy: ptr.To(admrv1.Fail),
+	}


this is recreated in every func invocation, why can't move this to a template and deep copy whatever is required?

added as a variable.

leelavg · 2025-01-17T10:24:57Z

controllers/webhook.go

+	webhookService := corev1.Service{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      WebhookServiceName,
+			Namespace: operatorNamespace,
+		},
+		Spec: corev1.ServiceSpec{
+			Ports: []corev1.ServicePort{
+				{
+					Name:       "https",
+					Port:       443,
+					Protocol:   corev1.ProtocolTCP,
+					TargetPort: intstr.FromInt32(9443),
+				},
+			},
+			Selector: map[string]string{
+				"app.kubernetes.io/name": "odf-operator",
+			},
+			Type: corev1.ServiceTypeClusterIP,
+		},
+	}
+
+	svc := &corev1.Service{
+		ObjectMeta: webhookService.ObjectMeta,
+	}


this is recreated in every func invocation, why can't move this to a template and deep copy whatever is required?

added as a variable.

leelavg · 2025-02-11T07:58:59Z

does this PR needs a push? where are you registering the webhook?

iamniting · 2025-02-11T13:12:09Z

does this PR needs a push? where are you registering the webhook?

No it is getting created in controllers/storagesystem_controller.go and controllers/subscription_controller.go

leelavg · 2025-02-24T02:52:10Z

does this PR needs a push? where are you registering the webhook?

No it is getting created in controllers/storagesystem_controller.go and controllers/subscription_controller.go

trying one more time, you have a type created in webhook/csv.go which has SetupWebhookWithManager method defined and I'm unable to find where this method was called.

iamniting · 2025-03-10T07:52:59Z

does this PR needs a push? where are you registering the webhook?

No it is getting created in controllers/storagesystem_controller.go and controllers/subscription_controller.go

trying one more time, you have a type created in webhook/csv.go which has SetupWebhookWithManager method defined and I'm unable to find where this method was called.

I fixed it, It was missed, It is called in the main.

leelavg · 2025-03-12T07:21:41Z

bundle/odf-operator/manifests/odf-operator-webhook-service_v1_service.yaml

+kind: Service
+metadata:
+  annotations:
+    service.beta.openshift.io/serving-cert-secret-name: webhook-server-cert


better to have a prefix for this secret as well as other operators could be using generic names.

renamed it to odf-operator-webhook-server-cert

leelavg · 2025-03-13T05:36:50Z

bundle/odf-operator/manifests/odf-operator.clusterserviceversion.yaml

+          - get
+          - list
+          - update
+          - watch


we also need delete perms to delete the webhook upon DF uninstallation.

Who will delete this? We can not even have an owner ref added to the webhook.

leelavg · 2025-03-13T05:42:42Z

controllers/storagesystem_controller.go

+		func(ctx context.Context, obj client.Object) []reconcile.Request {
+			logger := log.FromContext(ctx)
+			ssList := &odfv1alpha1.StorageSystemList{}
+			err := r.Client.List(ctx, ssList, &client.ListOptions{Namespace: r.OperatorNamespace})


Suggested change

err := r.Client.List(ctx, ssList, &client.ListOptions{Namespace: r.OperatorNamespace})

err := r.Client.List(ctx, ssList, client.InNamespace(r.OperatorNamespace)

i think using cmd construction options is more ergonomically

leelavg · 2025-03-13T05:44:32Z

controllers/subscription_controller.go

 	if err != nil {
 		return ctrl.Result{}, err
 	}

-	err = r.ensureSubscriptions(logger, req.NamespacedName.Namespace)
+	err = ensureWebhook(r.ctx, r.Client, logger, r.OperatorNamespace)


why is webhook deployment in two controllers?

Subscriptions are handled from these 2 controllers. any of these can be triggered first, and we need to make sure we have the webhook before subscriptions.

leelavg · 2025-03-13T05:46:35Z

controllers/subscription_controller.go

+	err := mgr.Add(manager.RunnableFunc(func(ctx context.Context) error {
+
+		logger := log.FromContext(ctx)
+		err := ensureWebhook(ctx, r.Client, logger, r.OperatorNamespace)


third instance of same reconcile function, Runnable runs in parallel to all reconcilers and doesn't have any priority afair.

It should not cause any issues i believe, This was created so that even if we hit other issues, we still create the webhook and odf-deps subs.

leelavg · 2025-03-13T05:56:45Z

controllers/webhook.go

+	WebhookPath        = "/mutate-operators-coreos-com-v1alpha1-csv"
+)
+
+var (


vars' declared here should form a template w/ a facility to change the names and fill the info dynamically during reconcile.

better have only servicespec here and in webhook no name, namespace selector which'll be replaced in a single place.

What will be the benefits of that?

leelavg · 2025-03-13T06:03:02Z

webhook/csv.go

+	return admission.PatchResponseFromRaw(req.Object.Raw, marshaledInstance)
+}
+
+func (r *ClusterServiceVersionDefaulter) getOldCsvReplicas(instance *opv1a1.ClusterServiceVersion) (int32, error) {


rather than instance, csv is more discoverable as param var.

i would prefer instance, because that is being used and created by scaffolding mostly at controller layers.

leelavg · 2025-03-13T06:04:11Z

webhook/csv.go

+	}
+
+	for _, pkgName := range controllers.PkgNames {
+		if strings.HasPrefix(instance.ObjectMeta.Name, pkgName) {


ObjectMeta and TypeMeta is embedded into the struct and could directly use .Name

leelavg · 2025-03-13T06:05:21Z

webhook/csv.go

+	return 0, nil
+}
+
+func (r *ClusterServiceVersionDefaulter) mutateCsv(instance *opv1a1.ClusterServiceVersion, replicas *int32) {


why do we need a func here, this is neither a single unit nor being called multiple times.

This func is the place where all the changes are made to the csv. This func is called Default in scafolding. It is also easy to track that what and where changes are made to the csv.

leelavg · 2025-03-13T06:07:33Z

webhook/csv.go

+
+func (r *ClusterServiceVersionDefaulter) getOldCsvReplicas(instance *opv1a1.ClusterServiceVersion) (int32, error) {
+
+	if instance.Spec.Replaces == "" {


so this is a new operator from this release and should we also check if we have the name/package in our records or else this will always stay scaled down?

If it is a new package, we should scale down by default, We should scale it up by the controller if needed.

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

openshift-ci · 2025-03-13T16:18:31Z

@iamniting: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-bundle-odf-dependencies-bundle	`94351a1`	link	true	`/test ci-bundle-odf-dependencies-bundle`
ci/prow/odf-operator-e2e-aws	`79473fd`	link	true	`/test odf-operator-e2e-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 15, 2025

iamniting force-pushed the webhook branch 3 times, most recently from a57ca02 to 263ec5e Compare January 15, 2025 10:05

leelavg reviewed Jan 17, 2025

View reviewed changes

iamniting force-pushed the webhook branch from 1d1b666 to b45c735 Compare January 21, 2025 07:07

openshift-merge-robot added the needs-rebase label Feb 19, 2025

iamniting force-pushed the webhook branch from b45c735 to 0fb974b Compare March 10, 2025 06:09

openshift-merge-robot removed the needs-rebase label Mar 10, 2025

iamniting force-pushed the webhook branch 4 times, most recently from 295aff4 to 0b0498b Compare March 10, 2025 07:19

iamniting force-pushed the webhook branch 6 times, most recently from eaf99a2 to 94351a1 Compare March 11, 2025 16:43

iamniting requested a review from leelavg March 11, 2025 16:53

leelavg reviewed Mar 13, 2025

View reviewed changes

iamniting added 2 commits March 13, 2025 14:00

controllers: create a csv webhook

dcddaf2

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

test: fix tests

0794bad

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

iamniting force-pushed the webhook branch from 94351a1 to 618dead Compare March 13, 2025 08:30

makefile: add webhook config and dockerfile requirements

5971e23

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

bundle: update generated changes

79473fd

Signed-off-by: Nitin Goyal <nigoyal@redhat.com>

iamniting force-pushed the webhook branch from 618dead to 79473fd Compare March 13, 2025 08:38

iamniting requested a review from leelavg March 13, 2025 09:22

iamniting force-pushed the webhook branch from 8b76c7a to 79473fd Compare March 13, 2025 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a CSV webhook #516

Create a CSV webhook #516

iamniting commented Jan 15, 2025 •

edited

Loading

openshift-ci bot commented Jan 15, 2025

leelavg Jan 17, 2025

iamniting Jan 21, 2025

leelavg Jan 17, 2025

iamniting Jan 21, 2025

leelavg Jan 17, 2025

iamniting Jan 21, 2025

leelavg commented Feb 11, 2025

iamniting commented Feb 11, 2025

leelavg commented Feb 24, 2025

iamniting commented Mar 10, 2025

leelavg Mar 12, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

leelavg Mar 13, 2025

iamniting Mar 13, 2025

openshift-ci bot commented Mar 13, 2025

	err := r.Client.List(ctx, ssList, &client.ListOptions{Namespace: r.OperatorNamespace})
	err := r.Client.List(ctx, ssList, client.InNamespace(r.OperatorNamespace)


		func (r ClusterServiceVersionDefaulter) getOldCsvReplicas(instance opv1a1.ClusterServiceVersion) (int32, error) {

		if instance.Spec.Replaces == "" {

Create a CSV webhook #516

Are you sure you want to change the base?

Create a CSV webhook #516

Conversation

iamniting commented Jan 15, 2025 • edited Loading

openshift-ci bot commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg commented Feb 11, 2025

iamniting commented Feb 11, 2025

leelavg commented Feb 24, 2025

iamniting commented Mar 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 13, 2025

iamniting commented Jan 15, 2025 •

edited

Loading