🐛issue-1737: Add unit tests for openstackmachine_webhook #2068

MykolaRodin · 2024-05-08T07:51:44Z

What this PR does / why we need it:

It extends tests for openstackmachine_webhook

k8s-ci-robot · 2024-05-08T07:51:53Z

Hi @MykolaRodin. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-05-08T07:52:01Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name	Link
🔨 Latest commit	`89d7fe6`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/663df27741791c0008b2b115
😎 Deploy Preview	https://deploy-preview-2068--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mdbooth · 2024-05-08T09:09:39Z

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -50,7 +50,7 @@ var _ = Describe("OpenStackMachine API validations", func() {
 		Expect(k8sClient.Create(ctx, defaultMachine())).To(Succeed(), "OpenStackMachine creation should succeed")
 	})

-	It("should only allow the providerID to be set once", func() {
+	It("should only allow the providerID to be set once and identityRef to be set several times", func() {


Please can you add a separate test for identityRef instead of adding it to this one? Unlike the e2e tests these tests are pretty fast to initialise, so it's ok to run lots of them. It's simpler to understand and maintain if they each do one thing only.

mdbooth · 2024-05-08T09:23:21Z

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -63,6 +63,14 @@ var _ = Describe("OpenStackMachine API validations", func() {
 		By("Modifying the providerID")
 		machine.Spec.ProviderID = ptr.To("bar")
 		Expect(k8sClient.Update(ctx, machine)).NotTo(Succeed(), "Updating providerID should fail")
+
+		By("Setting the identityRef")
+		machine.Spec.IdentityRef = ptr.To(infrav1.OpenStackIdentityReference{Name: "foo", CloudName: "GCP"})


Incidentally, it's not important to this test, except for readability, but CloudName here refers to an entry for an OpenStack cloud in clouds.yaml. So it's going to be something like production, staging, etc. In practise people don't tend to push a clouds.yaml with more than 1 cloud in it so this is academic, but we still support it.

mdbooth · 2024-05-08T09:26:49Z

test/e2e/suites/apivalidations/openstackmachine_test.go

+
+		// By("Modifying the identityRef")
+		// machine.Spec.IdentityRef = ptr.To(infrav1.OpenStackIdentityReference{Name: "bar", CloudName: "AWS"})
+		// Expect(k8sClient.Update(ctx, machine)).To(Succeed(), "Updating identityRef should succeed")


Is it this one that fails?

I have not uncommented it yet because it fails before.

If I change providerID and identityRef in different tests, it works fine. If I change both values withing the same test, if fails. I updated cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go - ValidateUpdate() a bit to print old and new objects on failure. Here the results of the failure are:

Message: "admission webhook \"validation.openstackmachine.infrastructure.cluster.x-k8s.io\" denied the request: OpenStackMachine.infrastructure.cluster.x-k8s.io \"machine-vjrtn\" is invalid: spec: Forbidden: cannot be modified old:map[flavor: image:map[filter:map[name:test-image]] providerID:foo] new:map[flavor: image:map[filter:map[name:test-image]] providerID:bar]"

Please clarify if it is expected.

OK, it looks like I got the reason for the failure when updating both providerID and identityRef. The test failed because updating providerID for the second time failed. Once I removed it, the test with updating providerID once and updating identityRef twice succeeded. Please clarify whether this test with updating both providerID and identityRef should be kept or it should be deleted

Ah, of course! The local object still contains the dirty ProviderID from the previous test.

No, it's not worth keeping this test.

mdbooth · 2024-05-08T09:27:14Z

/ok-to-test

MykolaRodin · 2024-05-08T12:49:57Z

/retest

MykolaRodin · 2024-05-08T13:07:22Z

HI @mdbooth,

I have removed the test with updating providerID once and updating identityRef twice. I have removed my temporary changes in cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go - ValidateUpdate() and left just a comment there that should help to understand why instanceID is mentioned there. The unit tests that I added beside the e2e test work fine for me. Please clarify what else I am expected to implement in this MR?

mdbooth · 2024-05-08T13:13:22Z

test/e2e/suites/apivalidations/openstackmachine_test.go

@@ -65,6 +65,21 @@ var _ = Describe("OpenStackMachine API validations", func() {
 		Expect(k8sClient.Update(ctx, machine)).NotTo(Succeed(), "Updating providerID should fail")
 	})

+	It("should only allow the identityRef to be set several times", func() {


Nit:

Suggested change

It("should only allow the identityRef to be set several times", func() {

It("should allow the identityRef to be set several times", func() {

mdbooth · 2024-05-08T13:18:57Z

pkg/webhooks/openstackmachine_webhook.go

@@ -96,6 +96,7 @@ func (*openStackMachineWebhook) ValidateUpdate(_ context.Context, oldObjRaw, new
 		delete(newOpenStackMachineSpec, "providerID")
 	}

+	// instanceID is present in v1alpha6 but is deprecated in v1beta1


This is the problem of writing these webhooks with Unstructured instead of proper types. The code below is a no-op because instanceID can never be set. If we were using proper types it would have been a compile error and we'd have noticed.

This can be deleted. Probably best in a separate PR, though, or at least a separate commit.

// instanceID is present in v1alpha6 but is deprecated in v1beta1 was removed

mdbooth · 2024-05-08T13:25:00Z

pkg/webhooks/openstackmachine_webhook_test.go

+			wantErr: false,
+		},
+		{
+			name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with non-root user on create",


Suggested change

name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with non-root user on create",

name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with non-root device name on create",

mdbooth · 2024-05-08T13:27:06Z

pkg/webhooks/openstackmachine_webhook_test.go

+			wantErr: false,
+		},
+		{
+			name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with root user on create",


Suggested change

name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with root user on create",

name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with root device name on create",

mdbooth · 2024-05-08T13:31:35Z

pkg/webhooks/openstackmachine_webhook_test.go

These tests are good. Ideally they'd be implemented in the apivalidations suite instead, though, because in the medium term I actually want to delete this webhook entirely and implement these validations in the apiserver directly. When we do that, these tests will no longer pass.

However, if we implement these tests in apivalidations instead we can completely change the implementation of the validations while ensuring that the exact same test suite continues to pass.

If you are able, my preference would be to move them to apivalidations. If not, though, I'll take these anyway. Apart from anything else they document the test cases that need to be implemented in apivalidations when we change the validations.

Thank you for the review. I have updated my PR in accordance with your code review remarks. On the other hand, I am sorry I cannot get what you mean under moving the tests to apivalidations. The e2e test I added has already been placed in cluster-api-provider-openstack/test/e2e/suites/apivalidations folder. The unit tests I added are placed in the file in the same folder as cluster-api-provider-openstack/pkg/webhooks/openstackmachine_webhook.go file. I cannot find other apivalidations folder and cannot see what I cad additionally change in the paths.

If it is possible, I would like to merge this PR as is. If you provide me with the additional clarification related to moving the tests to apivalidations, I am ready to do it in a separate PR.

Could you please clarify if I should have added several e2e tests instead of the unit tests?

My preference was that they be e2e tests instead. However these tests improve the current state of our testing and I'll gladly merge unit tests.

Hi @mdbooth,

It looks like the PR should still be modified to be taken into account and be reviewed. I removed the unit tests that I added and substituted them with e2e tests. I hope it is a bit closer to your preferences. Please review it and let me know if I am still missing something.

mdbooth · 2024-05-08T16:15:35Z

/approve

These are great, thanks! If you wanted to convert the unit tests to be e2e tests instead that would also be great, but I'm happy for these to merge as-is.

mdbooth · 2024-05-08T19:22:23Z

FYI, I added:

Fixes: #1737

to the PR description so the issue will be automatically closed when this PR merges.

mdbooth

This is great, thanks. I have already approved. Lets wait for an lgtm from another reviewer.

lentzi90

/lgtm

MykolaRodin · 2024-05-10T11:59:43Z

Hi @mdbooth ,

The tests failed with this error:

   [FAILED] Timed out after 1800.001s.
  Timed out waiting for 3 control plane machines to exist
  Expected
      <int>: 2
  to equal
      <int>: 3
  In [BeforeEach] at: /root/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/controlplane_helpers.go:116 @ 05/10/24 11:31:12.929
  Full Stack Trace
    sigs.k8s.io/cluster-api/test/framework.WaitForKubeadmControlPlaneMachinesToExist({0x2ac29b8, 0x4033060}, {{0x7f2068780ca0?, 0xc000b122d0?}, 0xc00122c1a0?, 0xc000613208?}, {0xc000f695a0, 0x2, 0x2})
    	/root/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/controlplane_helpers.go:116 +0x33b
    sigs.k8s.io/cluster-api/test/framework.WaitForControlPlaneAndMachinesReady({0x2ac29b8, 0x4033060}, {{0x7f2068758ad0?, 0xc000b122d0?}, 0xc00122c1a0?, 0xc000613208?}, {0xc000f695a0, 0x2, 0x2})
    	/root/go/pkg/mod/sigs.k8s.io/cluster-api/[email protected]/framework/controlplane_helpers.go:287 +0x692
    sigs.k8s.io/cluster-api/test/framework/clusterctl.ApplyCustomClusterTemplateAndWait.setDefaults.func3({_, _}, {{0x2ad4d18, 0xc000801800}, {0xc00100a001, 0x40529, 0x4052a}, {0xc00093c7e0, 0x12}, {0xc000c85590, ...}, ...}, ...)

I am sorry, I am not quite sure what it actually means. make test runs successfully on my local machine.

lentzi90 · 2024-05-10T12:14:33Z

It is an e2e test that failed. They are a bit flaky sometimes unfortunately.
As you only touched unit test code there cannot be any relation to these changes. I'll retrigger
/test pull-cluster-api-provider-openstack-e2e-test

EmilienM

/lgtm

k8s-ci-robot · 2024-05-10T12:51:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, mdbooth

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~~ [mdbooth]

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

EmilienM · 2024-05-10T12:52:20Z

/hold waiting for the CI job

mdbooth · 2024-05-10T14:16:16Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2024

k8s-ci-robot requested review from dulek and mdbooth May 8, 2024 07:51

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 8, 2024

MykolaRodin mentioned this pull request May 8, 2024

Add unit tests for openstackmachine_webhook #1737

Closed

MykolaRodin marked this pull request as draft May 8, 2024 08:15

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024

mdbooth reviewed May 8, 2024

View reviewed changes

MykolaRodin marked this pull request as ready for review May 8, 2024 13:01

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024

k8s-ci-robot requested a review from EmilienM May 8, 2024 13:02

mdbooth reviewed May 8, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2024

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 10, 2024

MykolaRodin requested a review from mdbooth May 10, 2024 07:17

mdbooth reviewed May 10, 2024

View reviewed changes

issue-1737: Add unit tests for openstackmachine_webhook

89d7fe6

lentzi90 reviewed May 10, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 May 10, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2024

EmilienM approved these changes May 10, 2024

View reviewed changes

k8s-ci-robot assigned EmilienM May 10, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2024

k8s-ci-robot merged commit 1e0b60f into kubernetes-sigs:main May 10, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛issue-1737: Add unit tests for openstackmachine_webhook #2068

🐛issue-1737: Add unit tests for openstackmachine_webhook #2068

MykolaRodin commented May 8, 2024 •

edited by mdbooth

Loading

k8s-ci-robot commented May 8, 2024

netlify bot commented May 8, 2024 •

edited

Loading

mdbooth May 8, 2024

mdbooth May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 8, 2024 •

edited

Loading

MykolaRodin May 8, 2024

mdbooth May 8, 2024

mdbooth commented May 8, 2024

MykolaRodin commented May 8, 2024

MykolaRodin commented May 8, 2024 •

edited

Loading

mdbooth May 8, 2024

MykolaRodin May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 8, 2024

MykolaRodin May 8, 2024

mdbooth May 8, 2024

MykolaRodin May 10, 2024 •

edited

Loading

mdbooth commented May 8, 2024

mdbooth commented May 8, 2024 •

edited

Loading

mdbooth left a comment

lentzi90 left a comment

MykolaRodin commented May 10, 2024

lentzi90 commented May 10, 2024

EmilienM left a comment

k8s-ci-robot commented May 10, 2024

EmilienM commented May 10, 2024

mdbooth commented May 10, 2024

	It("should only allow the identityRef to be set several times", func() {
	It("should allow the identityRef to be set several times", func() {

	name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with non-root user on create",
	name: "OpenStackMachine.Spec.RootVolume and OpenStackMachine.Spec.AdditionalBlockDevices with non-root device name on create",

🐛issue-1737: Add unit tests for openstackmachine_webhook #2068

🐛issue-1737: Add unit tests for openstackmachine_webhook #2068

Conversation

MykolaRodin commented May 8, 2024 • edited by mdbooth Loading

k8s-ci-robot commented May 8, 2024

netlify bot commented May 8, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MykolaRodin May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented May 8, 2024

MykolaRodin commented May 8, 2024

MykolaRodin commented May 8, 2024 • edited Loading

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

MykolaRodin May 10, 2024 • edited Loading

Choose a reason for hiding this comment

mdbooth commented May 8, 2024

mdbooth commented May 8, 2024 • edited Loading

mdbooth left a comment

Choose a reason for hiding this comment

lentzi90 left a comment

Choose a reason for hiding this comment

MykolaRodin commented May 10, 2024

lentzi90 commented May 10, 2024

EmilienM left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 10, 2024

EmilienM commented May 10, 2024

mdbooth commented May 10, 2024

MykolaRodin commented May 8, 2024 •

edited by mdbooth

Loading

netlify bot commented May 8, 2024 •

edited

Loading

MykolaRodin May 8, 2024 •

edited

Loading

MykolaRodin commented May 8, 2024 •

edited

Loading

MykolaRodin May 10, 2024 •

edited

Loading

mdbooth commented May 8, 2024 •

edited

Loading