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

Update to v1beta1 Omnibus PR #2820

Merged

Conversation

randomvariable
Copy link
Member

@randomvariable randomvariable commented Oct 5, 2021

  • Graduate CRDs to v1beta1
  • IAM types moved to /iam/api/v1beta1
  • Graduate clusterawsadm configuration types to v1beta1
  • Add conversions for v1alpha3 & v1alpha4 --> v1beta1 CRDs
  • Add conversion for clusterawsadm configuration from v1alpha1 to v1beta1
  • Improve error handling for clusterawsadm when incorrect configuration is passed
  • Add defaulter-gen to all types
  • Split up /api/**/conversion.go into ones for separate types
  • Controllers will now recover from panics, allowing webhooks to continue running
  • Remove kustomize generated e2e test flavors and add to Makefile targets
  • Ensure CRD docs generation actually occurs
  • Rudimentary build caching for API go files
  • Add e2e upgrade test for v1alpha3 to v1beta1 (but not yet enabled)

Co-authored-by: Shivani Singhal
Co-authored-by: Naadir Jeewa
Co-authored-by: Sedef Savas

What type of PR is this?

/kind api-change

What this PR does / why we need it:

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 #2792
Fixes #2780
Fixes #2781
Part of #2788
Fixes #2783

Special notes for your reviewer:

Checklist:

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

Release note:

Updated to Cluster API v1beta1

All API types within Cluster API Provider AWS have been graduated to v1beta1, this includes:
* AWSCluster*, AWSMachine*, AWSManaged* and all experimental APIs, covering both EC2 and EKS.
* clusterawsadm configuration has also graduated to v1beta1. Clusterawsadm is backwards compatible with v1alpha1 configuration files and they are semantically the same. Running `clusterawsadm bootstrap iam print-config --config <old-file>`  will do an automated conversion to v1beta1.
* Cluster API Provider AWS will support upgrades directly from v1alpha3 to v1beta1 as well as v1alpha4 to v1beta1.

Additional changes:
* IAM types have been moved (back) out of the main `/api` package into a new `/iam/api` package where they are consumed by both EKS and clusterawsadm.

@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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 5, 2021
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 5, 2021
@randomvariable
Copy link
Member Author

/triage accepted
/priority important-soon
/milestone v1.0.0

@k8s-ci-robot k8s-ci-robot added this to the v1.0.0 milestone Oct 5, 2021
@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Oct 5, 2021
Makefile Outdated Show resolved Hide resolved
@sedefsavas
Copy link
Contributor

This PR as is working on my laptop. make verify succeeds. I don't know what is different, as we use the same conversion-gen, but @shivi28 has a fix on top of this that solves the issue seen in the tests here.
I did not observe any compileErrorOnMissingConversion errors with this version though.

@randomvariable randomvariable force-pushed the v1-update-attempt-2 branch 2 times, most recently from 8508757 to 64379ca Compare October 6, 2021 06:24
@randomvariable
Copy link
Member Author

randomvariable commented Oct 6, 2021

@shivi28 I didn't include the Makefile in #2824 because it didn't solve the problem:

Changing --peer-dirs to --input-dirs just means to regenerate the conversion for /api, so we actually end up generating the conversions twice for /api. The reason it no longer errors is because this Makefile removes the conversions for /bootstrap and /controlplane which are causing the problems. Anyway, my fix in

hack/tools/bin/conversion-gen \
	--input-dirs=./api/v1alpha3 \
	--input-dirs=./api/v1alpha4 \
	--input-dirs=./cmd/clusterawsadm/api/bootstrap/v1alpha1 \
	--extra-peer-dirs=sigs.k8s.io/cluster-api/api/v1alpha3 \
	--extra-peer-dirs=sigs.k8s.io/cluster-api/api/v1alpha4 \
	--build-tag=ignore_autogenerated_core \
	--output-file-base=zz_generated.conversion  \
	--go-header-file=./hack/boilerplate/boilerplate.generatego.txt
hack/tools/bin/conversion-gen \
	--input-dirs=./exp/api/v1alpha3 \
	--input-dirs=./exp/api/v1alpha4 \
	--input-dirs=./api/v1alpha3 \
	--input-dirs=./api/v1alpha4 \
	--extra-peer-dirs=sigs.k8s.io/cluster-api/api/v1alpha3 \
	--extra-peer-dirs=sigs.k8s.io/cluster-api/api/v1alpha4 \
	--build-tag=ignore_autogenerated_core \
	--output-file-base=zz_generated.conversion  \
	--go-header-file=./hack/boilerplate/boilerplate.generatego.txt
touch .build/generate-go-apis
make[4]: Entering directory '/home/prow/go/src/sigs.k8s.io/cluster-api-provider-aws'
hack/tools/bin/defaulter-gen \
	--input-dirs=./api/v1alpha3 \
	--input-dirs=./api/v1alpha4 \
	--input-dirs=./api/v1beta1 \
	--input-dirs=./exp/api/v1beta1 \
	--input-dirs=./cmd/clusterawsadm/api/bootstrap/v1beta1 \
	--input-dirs=./cmd/clusterawsadm/api/bootstrap/v1alpha1 \
	--extra-peer-dirs=sigs.k8s.io/cluster-api/api/v1beta1 \
	--v=0  \

I've gone for a different approach: which is to fork conversion-gen and remove the check for duplicate static functions. An issue needs to be opened in the kubernetes repo to fix it.

@sedefsavas
Copy link
Contributor

I didn't see you pushed changes, I pushed mine (please repush yours @randomvariable ). It somehow squashed them all.

Without any change needed in the conversion, mine started working for bootstrap and controlplane packages.
The changes I did are seen better here: 366db55

@randomvariable
Copy link
Member Author

probably had incorporated the forked conversion-gen. the make verify failure in this latest run was because of a missing dot in the CRD after i corrected a comment. No more pushes to this branch please.

Will run e2e in a mo...

@randomvariable
Copy link
Member Author

/test ?

@k8s-ci-robot
Copy link
Contributor

@randomvariable: The following commands are available to trigger required jobs:

  • /test pull-cluster-api-provider-aws-build
  • /test pull-cluster-api-provider-aws-test
  • /test pull-cluster-api-provider-aws-verify

The following commands are available to trigger optional jobs:

  • /test pull-cluster-api-provider-aws-e2e
  • /test pull-cluster-api-provider-aws-e2e-blocking
  • /test pull-cluster-api-provider-aws-e2e-conformance
  • /test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts
  • /test pull-cluster-api-provider-aws-e2e-eks

Use /test all to run the following jobs that were automatically triggered:

  • pull-cluster-api-provider-aws-build
  • pull-cluster-api-provider-aws-test
  • pull-cluster-api-provider-aws-verify

In response to this:

/test ?

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/test-infra repository.

@randomvariable
Copy link
Member Author

/test pull-cluster-api-provider-aws-e2e
/test pull-cluster-api-provider-aws-e2e-eks
/hold
/assign @richardcase

Ready for review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2021
@randomvariable
Copy link
Member Author

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

@randomvariable randomvariable changed the title Update to v1beta1 Omnibus PR 2 Update to v1beta1 Omnibus PR Oct 6, 2021
@randomvariable
Copy link
Member Author

Argh, and forgot to account for the fact that S3 behaves differently in us-east-1 and is more eventually consistent

@randomvariable
Copy link
Member Author

I'll start with this
/test pull-cluster-api-provider-aws-e2e

@randomvariable
Copy link
Member Author

This time the bucket didn't get created because IAM had not synced the bootstrap access key globally yet for S3's gateway.

@randomvariable
Copy link
Member Author

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

@randomvariable
Copy link
Member Author

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

@randomvariable
Copy link
Member Author

The upgrade test looks like it will need more work to run in CI, so I'm temporarily disabling, and we can sort out in follow up PR.

@randomvariable
Copy link
Member Author

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

@randomvariable
Copy link
Member Author

Ready for review.

I'm going to leave #2788 and do a follow up PR.

/assign @sedefsavas

Richard is OOO today.

@randomvariable
Copy link
Member Author

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2021
@randomvariable
Copy link
Member Author

/unhold

* Graduate CRDs to v1beta1
* IAM types moved to /iam/api/v1beta1
* Graduate clusterawsadm configuration types to v1beta1
* Add conversions for v1alpha3 & v1alpha4 --> v1beta1 CRDs
* Add conversion for clusterawsadm configuration from v1alpha1 to v1beta1
* Improve error handling for clusterawsadm when incorrect configuration is passed
* Add defaulter-gen to all types
* Split up /api/**/conversion.go into ones for separate types
* Controllers will now recover from panics, allowing webhooks to continue running
* Remove kustomize generated e2e test flavors and add to Makefile targets
* Ensure CRD docs generation actually occurs
* Rudimentary build caching for API go files
* conversion-gen was forked into hack/tools/third_party to remove incorrect duplicate static function warnings
* API packages given consistent naming and set throughout the codebase. golangci-lint configuration updated to match

Co-authored-by: Shivani Singhal <[email protected]>
Co-authored-by: Naadir Jeewa <[email protected]>
Co-authored-by: Sedef Savas <[email protected]>
@sedefsavas
Copy link
Contributor

/lgtm
Great work @randomvariable and @shivi28 🎉🎉

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2021
@sedefsavas
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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 6, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1a61dfc into kubernetes-sigs:main Oct 6, 2021
@k8s-ci-robot k8s-ci-robot modified the milestones: v1.0.0, v1.x Oct 6, 2021
@randomvariable randomvariable deleted the v1-update-attempt-2 branch October 6, 2021 16:20
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/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
5 participants