Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Support ignition v3 and presigned URLs #4553

Merged
merged 1 commit into from Oct 11, 2023

Conversation

vincepri
Copy link
Member

@vincepri vincepri commented Oct 6, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR adds support for more versions of ignition, based on the work the @JoelSpeed started in https://github.com/openshift/cluster-api-provider-aws/pull/475/files.

It includes the ability for CAPA to generate presigned URLs and not rely on IAM roles.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4491

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

Ignition v3 is now supported as a valid version. S3 Bucket to store bootstrap data can now optionally use presigned URLs instead of relying on IAM permissions for nodes and control-plane nodes. The feature is opt-in and backward compatible, if you'd like to switch using presigned URLs, set `AWSCluster.S3Bucket.PresignedURLDuration`.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 6, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 6, 2023
@vincepri
Copy link
Member Author

vincepri commented Oct 6, 2023

/assign @nrb @JoelSpeed @richardcase

@vincepri vincepri force-pushed the ignition-presigned branch 3 times, most recently from b4ce100 to 46a5d21 Compare October 6, 2023 20:33
@nrb
Copy link
Contributor

nrb commented Oct 6, 2023

This looks pretty reasonable to me.

The only thing that gives me pause is the release note being one line; it could be taken to imply that the ignition and presigned URL support are coupled, but I don't believe they are.

@vincepri
Copy link
Member Author

vincepri commented Oct 8, 2023

Manual testing was performed, and presigned URLs worked great 🎉

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023
@JoelSpeed
Copy link

Changes look reasonable to me, I've already tested and manually verified the ignition v3 changes

@Skarlso
Copy link
Contributor

Skarlso commented Oct 9, 2023

Manual testing was performed, and presigned URLs worked great 🎉

Can you please include some basic proof for statements like these?

@Skarlso
Copy link
Contributor

Skarlso commented Oct 9, 2023

Also, some issues would be great so we could discuss them and their usefulness.

@JoelSpeed
Copy link

Here's an example of a user data created by this patch when using the IAM roles, showing backward compatibility, and version 3.4 of ignition is used
Screenshot 2023-10-09 at 14 54 30

Signed-off-by: Vince Prignano <vincepri@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023
@Skarlso
Copy link
Contributor

Skarlso commented Oct 10, 2023

/lgtm

Should be okay after rebase.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2023
@Ankitasw
Copy link
Member

/test pull-cluster-api-provider-aws-e2e

@vincepri
Copy link
Member Author

/retest

failure is not related

@vincepri
Copy link
Member Author

/retest

@Ankitasw
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ankitasw

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit e0f6c4e into kubernetes-sigs:main Oct 11, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Ignition v3.0+
7 participants