search

kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
289 stars 253 forks source link

Add unit tests for `openstackmachine_webhook` #1737

Closed EmilienM closed 4 months ago

EmilienM commented 11 months ago

This file has no unit tests.

_Originally posted by @EmilienM in https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1696#discussion_r1374772941_

EmilienM commented 11 months ago

From Matt:

Ideally these would be envtests, btw. We could then re-use them to validate CEL.
k8s-triage-robot commented 8 months ago

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

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

You can:

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

/lifecycle stale

mdbooth commented 8 months ago

/remove-lifecycle stale

k8s-triage-robot commented 5 months ago

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

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

You can:

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

/lifecycle stale

huxcrux commented 5 months ago

/remove-lifecycle stale

mdbooth commented 5 months ago

We kinda have these with https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1919 and to a lesser extent in https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/2043. We could definitely extend the coverage of the api validation tests now they exist.

MykolaRodin commented 5 months ago

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 commented 5 months ago

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: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/078a7925dfaf65a8166cf425383deb8175c48633/pkg/webhooks/openstackmachine_webhook.go#L109-L111

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: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/078a7925dfaf65a8166cf425383deb8175c48633/test/e2e/suites/apivalidations/openstackmachine_test.go#L53-L66

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: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/078a7925dfaf65a8166cf425383deb8175c48633/api/v1beta1/types.go#L31-L45

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 commented 4 months ago

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 (https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/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 commented 4 months ago

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 🤔

  1. 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 commented 4 months ago

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.