Add unit tests for openstackmachine_webhook

[EmilienM]

This file has no unit tests.

Originally posted by @EmilienM in #1696 (comment)

[EmilienM]

From Matt:

Ideally these would be envtests, btw. We could then re-use them to validate CEL.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[mdbooth]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[huxcrux]

/remove-lifecycle stale

[mdbooth]

We kinda have these with #1919 and to a lesser extent in #2043. We could definitely extend the coverage of the api validation tests now they exist.

[MykolaRodin]

Hi @mdbooth,

I would like to give this task a try yet I have no clue about the actual functionality of openstackmachine_webhook. Please clarify if my lack of knowledge is not a show stopper for this task. If it is still not, please clarify the coverage of which existing unit tests I could try to increase (I could use existing unit tests as the example).

[mdbooth]

Hi, Mykola! This would be great. I'll try to give you some context, but as I don't know your current level please forgive me if I give too much, or let me know if it's too little.

Our webhooks are defined here: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/tree/main/pkg/webhooks. They run as part of the CAPO manager process, so the requests are sent to CAPO itself via the webhook service.

We only use admission webhooks. tl;dr When a user makes a CRUD call for one of our objects to kube-apiserver we get a callback from kube-apiserver which allows us to accept or reject the operation.

Mostly we use the webhooks to ensure we only allow certain parts of the spec to mutate. The OpenStackMachine webhook ValidateUpdate ends by comparing oldOpenStackMachineSpec and newOpenStackMachineSpec and returning an error if they are different:

cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go

Lines 109 to 111 in 078a792

	if !reflect.DeepEqual(oldOpenStackMachineSpec, newOpenStackMachineSpec) {
	allErrs = append(allErrs, field.Forbidden(field.NewPath("spec"), "cannot be modified"))
	}

However, the preceeding function variously zeroes out parts of both objects if we allow them to be updated.

We now have an apivalidations test suite for testing API operations: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/test/e2e/suites/apivalidations/openstackmachine_test.go

You can easily run just this test suite like this:

$ make test TEST_PATHS=./test/e2e/suites/apivalidations/...

This test suite uses envtest, so it downloads and runs an actual kube-apiserver and etcd just for the test, loads our CRDs, and starts CAPO (for the webhooks) before running any tests. The tests consist of performing client operations with an actual k8s client. e.g. This one ensures that CAPO can set the providerID but it can't subsequently be updated:

cluster-api-provider-openstack/test/e2e/suites/apivalidations/openstackmachine_test.go

Lines 53 to 66 in 078a792

	It("should only allow the providerID to be set once", func() {
	machine := defaultMachine()
	By("Creating a bare machine")
	Expect(k8sClient.Create(ctx, machine)).To(Succeed(), "OpenStackMachine creation should succeed")
	By("Setting the providerID")
	machine.Spec.ProviderID = ptr.To("foo")
	Expect(k8sClient.Update(ctx, machine)).To(Succeed(), "Setting providerID should succeed")
	By("Modifying the providerID")
	machine.Spec.ProviderID = ptr.To("bar")
	Expect(k8sClient.Update(ctx, machine)).NotTo(Succeed(), "Updating providerID should fail")
	})

The testing in this test suite is inadequate. For example, we don't test that mutations to identityRef are allowed.

I think the above fully describes the immediate task. For some additional context (stop reading if it's not helpful), we likely want to move most validations that can be performed by CEL or other CRD-based validations out of the webhooks and into the CRDs. Here's an example of this in ImageParam:

cluster-api-provider-openstack/api/v1beta1/types.go

Lines 31 to 45 in 078a792

	// ImageParam describes a glance image. It can be specified by ID or filter.
	// +kubebuilder:validation:MaxProperties:=1
	// +kubebuilder:validation:MinProperties:=1
	type ImageParam struct {
	// ID is the uuid of the image. ID will not be validated before use.
	// +kubebuilder:validation:Format:=uuid
	// +optional
	ID optional.String `json:"id,omitempty"`
	// Filter describes a query for an image. If specified, the combination
	// of name and tags must return a single matching image or an error will
	// be raised.
	// +optional
	Filter *ImageFilter `json:"filter,omitempty"`
	}

Here we assert that exactly one of the properties must be set, and that ID, if set, must be a valid uuid. These validations are performed by kube-apiserver directly without making a webhook call.

The apivalidations suite, because it is an envtest and therefore makes real api calls, can validate this logic whether it is implemented as a webhook or not. My strong preference is to implement these tests in apivalidations so as we gradually reduce what we validate in the webhook, we can continue to test that the user-visible behaviour remains the same.

[MykolaRodin]

Hi @mdbooth,

Thanks a lot for the detailed explanation. Please provide as more details as it is required. If something is not clear to me, I will ask for the clarification. I still doubt if I can successfully start. I have prepared some basic PR (#2068) a have a few questions:

1. I tried to modify identityRef in the test you mentioned yet the test fails with this message: Message: "admission webhook \"validation.openstackmachine.infrastructure.cluster.x-k8s.io\" denied the request: OpenStackMachine.infrastructure.cluster.x-k8s.io \"machine-wqxpj\" is invalid: spec: Forbidden: cannot be modified". Which way should I do it correctly?
2. In cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go - ValidateUpdate() I can see that instanceID can be present in oldOpenStackMachineSpec / newOpenStackMachineSpec. Could you please clarify if I should test it in this test as well?
   Edited: If I should do p.2, is it expected that InstanceID is present in cluster-api-provider-openstack/api/v1alpha6/openstackmachine_types.go - OpenStackMachineSpec but is absent in cluster-api-provider-openstack/api/v1beta1/openstackmachine_types.go - OpenStackMachineSpec?

[mdbooth]

Hi @mdbooth,

Thanks a lot for the detailed explanation. Please provide as more details as it is required. If something is not clear to me, I will ask for the clarification. I still doubt if I can successfully start. I have prepared some basic PR (#2068) a have a few questions:

1. I tried to modify identityRef in the test you mentioned yet the test fails with this message: Message: "admission webhook \"validation.openstackmachine.infrastructure.cluster.x-k8s.io\" denied the request: OpenStackMachine.infrastructure.cluster.x-k8s.io \"machine-wqxpj\" is invalid: spec: Forbidden: cannot be modified". Which way should I do it correctly?

Looking at your test, I can't immediately explain that. Is that a bug in the webhook? It looks ok 🤔

2. In cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go - ValidateUpdate() I can see that instanceID can be present in oldOpenStackMachineSpec / newOpenStackMachineSpec. Could you please clarify if I should test it in this test as well?
   Edited: If I should do p.2, is it expected that InstanceID is present in cluster-api-provider-openstack/api/v1alpha6/openstackmachine_types.go - OpenStackMachineSpec but is absent in cluster-api-provider-openstack/api/v1beta1/openstackmachine_types.go - OpenStackMachineSpec?

We only validate v1beta1, which doesn't contain instanceID. v1alpha6 will be converted to v1beta1 via the separate conversion webhook first, so we don't need to worry about it here.

[MykolaRodin]

Thank you for the information. I will add the unit tests to cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook_test.go like it has been done for cluster-api-provider-openstack/pkg/webhooks/openstackcluster_webhook_test.go and try to figure out the reason for failing the e2e test.