fix for OCPBUGS-55135: userDataSecret.name should be a Required value on AWS #1399

germanparente · 2025-08-04T14:58:04Z

aligning the check on webhook side to prevent the userDataSecret name to be empty, as it's already happening in other providers.

… on AWS

openshift-ci-robot · 2025-08-04T14:58:12Z

@germanparente: This pull request references Jira Issue OCPBUGS-55135, which is invalid:

expected the bug to target the "4.20.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

aligning the check on webhook side to prevent the userDataSecret name to be empty, as it's already happening in other providers.

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 openshift-eng/jira-lifecycle-plugin repository.

germanparente · 2025-08-04T15:00:09Z

/jira refresh

germanparente · 2025-08-04T16:29:06Z

/jira refresh

openshift-ci-robot · 2025-08-04T16:29:17Z

@germanparente: This pull request references Jira Issue OCPBUGS-55135, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.20.0) matches configured target version for branch (4.20.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @sunzhaohua2

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

germanparente · 2025-08-04T16:33:13Z

I see that ci/prow/unit
is failing testing the removal of the userDataSecret.

I had checked that in other providers, when there's no userDataSecret, it's set to a default one and them I wanted to have the same behaviour in AWS. Should we allow removing it ?

JoelSpeed · 2025-08-04T16:46:48Z

Requiring a field that previously was not required is a breaking change, and would prevent existing MachineSets from spawning new Machines. We need to be very careful here.

What exactly is the behaviour today if no name is provided?

germanparente · 2025-08-04T19:09:55Z

@JoelSpeed in fact, when we install from scratch, we have a default userDataSecret what a name.

And there's a test failing by removing the full userDataSecret, not only the name:

--- FAIL: TestMachineUpdate/with_an_AWS_ProviderSpec,_removing_the_user_data_secret (1.13s)

So, it seems that in aws we should allow removing the full userDataSecret but not allow to leave the userDataSecret with name nullified.

I will check this and come back with another patch.

…uired value on AWS

JoelSpeed · 2025-08-05T09:04:27Z

Requiring a name where we did not previously is still a breaking change. Even though the default machinesets include a name, this does not mean that all machinesets in all clusters do.

If you want to make this change, can we verify from insights data that all recorded AWS machinesets have a name field set? This would give us at least some confidence that the majority would not be broken by this change

JoelSpeed · 2025-08-05T09:13:05Z

Hmm, looking at the linked bug, it appears that if you do not have this value, then machines cannot be successfully created, if that's the case, we could argue that no one should have a machineset where this value is missing, and therefore, the breaking change here is safe 🤔

germanparente · 2025-08-05T11:20:03Z

Thanks @JoelSpeed then, I think the current patch is the right one requiring a value for name but not necessarily requiring the full userDaraSecret information that seems can be removed. Since there's a unit test doing this

I will check the test failures on Thursday.

Thanks for your review

JoelSpeed · 2025-08-06T15:13:34Z

pkg/webhooks/machine_webhook.go

-	if providerSpec.UserDataSecret == nil {
-		errs = append(
-			errs,
-			field.Required(
-				field.NewPath("providerSpec", "userDataSecret"),
-				"expected providerSpec.userDataSecret to be populated",
-			),
-		)


I suspect we still want this, and the new validation. Though looking at the new defaulting, can we ever get here?

Thanks @JoelSpeed
My understanding was that there's a test that is checking that we can remove completely:

userDataSecret:
name: xxxxx

That's why I have switched to validate only that the name cannot be empty if the sctructure exists that is what the ocpbugs states as improvement.

If I re-add those lines and there's a test removing the full userDataSecret information, we will have the failure.

…uired value on AWS

germanparente · 2025-08-07T15:45:03Z

Just to clarify, the first time the test has failed because it was expecting this message:

"expected providerSpec.userDataSecret to be populated"

and my pr was returning instead "userDataSecret must be provided"

germanparente · 2025-08-08T00:38:23Z

/test e2e-metal-ipi-ovn-ipv6

JoelSpeed · 2025-08-08T10:42:48Z

pkg/webhooks/machine_webhook.go

-		)
+		errs = append(errs, field.Required(field.NewPath("providerSpec", "userDataSecret"), "expected providerSpec.userDataSecret to be populated"))
+	} else if providerSpec.UserDataSecret.Name == "" {
+		errs = append(errs, field.Required(field.NewPath("providerSpec", "userDataSecret", "name"), "name must be provided"))


Lets add a test case that checks that this error is produced please

germanparente · 2025-08-08T13:03:09Z

unit test added.

JoelSpeed · 2025-08-08T13:06:12Z

/approve

openshift-ci · 2025-08-08T13:06:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

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

damdo

Thanks a lot @germanparente 🎉

The change looks reasonable to me, as it aligns with GCP and Azure.

Regarding breaking changes behaviour, if the Machine wouldn't come up, I think it is ok to break behaviour as it improves the UX preventing the Provisioning + Failure as it explains why it is invalid.

Left a small fixup suggestion for the unit failure.

pkg/webhooks/machine_webhook.go

pkg/webhooks/machine_webhook_test.go

Co-authored-by: Damiano Donati <[email protected]>

openshift-ci · 2025-08-08T17:25:10Z

@germanparente: 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/okd-scos-e2e-aws-ovn	`1682aa6`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-vsphere-host-groups-ovn	`1682aa6`	link	false	`/test e2e-vsphere-host-groups-ovn`
ci/prow/e2e-vsphere-ovn-serial	`1682aa6`	link	false	`/test e2e-vsphere-ovn-serial`
ci/prow/e2e-gcp-ovn	`1682aa6`	link	false	`/test e2e-gcp-ovn`
ci/prow/e2e-vsphere-ovn	`1682aa6`	link	false	`/test e2e-vsphere-ovn`
ci/prow/e2e-vsphere-ovn-techpreview-serial	`1682aa6`	link	false	`/test e2e-vsphere-ovn-techpreview-serial`
ci/prow/e2e-vsphere-ovn-multi-vcenter	`1682aa6`	link	false	`/test e2e-vsphere-ovn-multi-vcenter`
ci/prow/e2e-vsphere-static-ovn	`1682aa6`	link	false	`/test e2e-vsphere-static-ovn`
ci/prow/e2e-openstack	`1682aa6`	link	false	`/test e2e-openstack`

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.

germanparente · 2025-08-08T17:44:55Z

/cherry-pick release-4.19

openshift-cherrypick-robot · 2025-08-08T17:44:58Z

@germanparente: once the present PR merges, I will cherry-pick it on top of release-4.19 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.19

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.

JoelSpeed · 2025-08-12T14:54:53Z

@damdo looks like your changes are addressed, can you LGTM or have you any further feedback?

damdo

Thanks a lot @germanparente 🎉

/lgtm

theobarberbany · 2025-08-12T15:26:11Z

Heya @germanparente, can we squash down these commits to an atomic one? :)

openshift-ci-robot · 2025-08-12T15:41:08Z

@germanparente: Jira Issue OCPBUGS-55135: All pull requests linked via external trackers have merged:

openshift/machine-api-operator#1399

Jira Issue OCPBUGS-55135 has been moved to the MODIFIED state.

In response to this:

aligning the check on webhook side to prevent the userDataSecret name to be empty, as it's already happening in other providers.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-cherrypick-robot · 2025-08-12T15:42:01Z

@germanparente: new pull request created: #1403

In response to this:

/cherry-pick release-4.19

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.

openshift-bot · 2025-08-13T00:02:39Z

[ART PR BUILD NOTIFIER]

Distgit: ose-machine-api-operator
This PR has been included in build ose-machine-api-operator-container-v4.20.0-202508122224.p0.gd977735.assembly.stream.el9.
All builds following this will include this PR.

fix for OCPBUGS-55135: userDataSecret.name should be a Required value…

b80bdd5

… on AWS

openshift-ci bot requested review from JoelSpeed and theobarberbany August 4, 2025 15:00

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Aug 4, 2025

openshift-ci bot requested a review from sunzhaohua2 August 4, 2025 16:29

fix (reviewed) for OCPBUGS-55135: userDataSecret.name should be a Req…

403c9cb

…uired value on AWS

JoelSpeed reviewed Aug 6, 2025

View reviewed changes

germanparente added 2 commits August 7, 2025 17:26

fix (reviewed) for OCPBUGS-55135: userDataSecret.name should be a Req…

cc136d1

…uired value on AWS

fix (reviewed) for OCPBUGS-55135: userDataSecret.name should be a Req…

dae7665

…uired value on AWS

JoelSpeed reviewed Aug 8, 2025

View reviewed changes

Adding unit test for providerSpec.userDataSecret.name

ef5d3f2

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

damdo reviewed Aug 8, 2025

View reviewed changes

pkg/webhooks/machine_webhook.go Outdated Show resolved Hide resolved

pkg/webhooks/machine_webhook_test.go Outdated Show resolved Hide resolved

germanparente and others added 2 commits August 8, 2025 15:39

Update pkg/webhooks/machine_webhook.go

c25f257

Co-authored-by: Damiano Donati <[email protected]>

Update pkg/webhooks/machine_webhook_test.go

1682aa6

Co-authored-by: Damiano Donati <[email protected]>

damdo reviewed Aug 12, 2025

View reviewed changes

openshift-ci bot assigned damdo Aug 12, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2025

openshift-merge-bot bot merged commit d977735 into openshift:main Aug 12, 2025
26 of 35 checks passed

openshift-cherrypick-robot mentioned this pull request Aug 12, 2025

[release-4.19] fix for OCPBUGS-60427: userDataSecret.name should be a Required value on AWS #1403

Merged

fix for OCPBUGS-55135: userDataSecret.name should be a Required value on AWS #1399

fix for OCPBUGS-55135: userDataSecret.name should be a Required value on AWS #1399

Uh oh!

Conversation

germanparente commented Aug 4, 2025

Uh oh!

openshift-ci-robot commented Aug 4, 2025

Uh oh!

germanparente commented Aug 4, 2025

Uh oh!

germanparente commented Aug 4, 2025

Uh oh!

openshift-ci-robot commented Aug 4, 2025

Uh oh!

germanparente commented Aug 4, 2025

Uh oh!

JoelSpeed commented Aug 4, 2025

Uh oh!

germanparente commented Aug 4, 2025

Uh oh!

JoelSpeed commented Aug 5, 2025

Uh oh!

JoelSpeed commented Aug 5, 2025

Uh oh!

germanparente commented Aug 5, 2025

Uh oh!

JoelSpeed Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

germanparente Aug 7, 2025

Choose a reason for hiding this comment

Uh oh!

germanparente commented Aug 7, 2025

Uh oh!

germanparente commented Aug 8, 2025

Uh oh!

JoelSpeed Aug 8, 2025

Choose a reason for hiding this comment

Uh oh!

germanparente commented Aug 8, 2025

Uh oh!

JoelSpeed commented Aug 8, 2025

Uh oh!

openshift-ci bot commented Aug 8, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

openshift-ci bot commented Aug 8, 2025

Uh oh!

germanparente commented Aug 8, 2025

Uh oh!

openshift-cherrypick-robot commented Aug 8, 2025

Uh oh!

JoelSpeed commented Aug 12, 2025

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

theobarberbany commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 12, 2025

Uh oh!

openshift-cherrypick-robot commented Aug 12, 2025

Uh oh!

openshift-bot commented Aug 13, 2025

Uh oh!

Uh oh!

theobarberbany commented Aug 12, 2025 •

edited

Loading