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.

Add unit tests for openstackmachine_webhook #1737

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Add unit tests for `openstackmachine_webhook` #1737