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.

Sign up for GitHub

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

Jump to bottom

feat: Graduate EKS out of experimental #2648

Merged
merged 1 commit into from
Aug 13, 2021
Merged

feat: Graduate EKS out of experimental #2648

merged 1 commit into from
Aug 13, 2021

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented Aug 7, 2021 •
edited
Loading

Signed-off-by: Richard Case [email protected]

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

The EKS functionality is graduated out of experimental with this change. This means it will be enabled by default.

As part of the graduation the separate controlplane and bootstrap providers have been removed and
merged into the main AWS infrastructure provider.

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 #2570
Fixes #2539

Special notes for your reviewer:

Checklist:

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

Release note:

action required
With the graduation of EKS out of experimental and it being enabled by default **all** users of Cluster API Provider AWS need to be aware of the following:

1. EKS support is now enabled by default. If you don't want to enable the EKS functionality then you will need to disable the creation of the IAM permissions via a `clusterawsadm` configuration file:

apiVersion: bootstrap.aws.infrastructure.cluster.x-k8s.io/v1alpha1
kind: AWSIAMConfiguration
spec:
  eks:
    disable: true

clusterawsadm bootstrap iam create-cloudformation-stack --config bootstrap-config.yaml

and then you need to set the `CAPA_EKS` environment variable to false before doing `clusterctl init`. For example:

export CAPA_EKS=false
clusterctl init --infrastructure=aws

2.  The EKS bootstrap and controlplane providers have been merged into the main infrastructure provider/manager. You will need to remove any reference to aws-eks in the control-plane/bootstrap flags for clusterctl init:

For example, change this:

clusterctl init --infrastructure=aws --control-plane aws-eks --bootstrap aws-eks

To this:

clusterctl init --infrastructure=aws

3. AWSManagedCluster has been removed. If you are using AWSManagedCluster then you will need to replace it with a reference to the AWSmanagedControlPlane. For example, change this:
apiVersion: cluster.x-k8s.io/v1alpha4
kind: Cluster
metadata:
  name: "cl1"
spec:
  clusterNetwork:
    pods:
      cidrBlocks: ["192.168.0.0/16"]
  infrastructureRef:
    apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
    kind: AWSManagedCluster
    name: "cl1"
  controlPlaneRef:
    kind: AWSManagedControlPlane
    apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
    name: "cl1-control-plane"

to this:

apiVersion: cluster.x-k8s.io/v1alpha4
kind: Cluster
metadata:
  name: "cl1"
spec:
  clusterNetwork:
    pods:
      cidrBlocks: ["192.168.0.0/16"]
  infrastructureRef:
    kind: AWSManagedControlPlane
    apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
    name: "cl1-control-plane"
  controlPlaneRef:
    kind: AWSManagedControlPlane
    apiVersion: controlplane.cluster.x-k8s.io/v1alpha4
    name: "cl1-control-plane"

4. Fargate profile support remains experimental and as suck must be enabled using the **EKSFargate** feature flag. This can be done before running `clusterctl init` using the **EXP_EKS_FARGATE** environment variable:

export EXP_EKS_FARGATE=true
clusterctl init --infrastructure=aws

You will also need to ensure you have the fargate default profile created. See the docs for further information.

@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. 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 labels Aug 7, 2021
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 7, 2021
@richardcase
Copy link
Member Author

/test ?

@k8s-ci-robot
Copy link
Contributor

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

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

The following commands are available to trigger optional jobs:

  • /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-blocking
  • /test pull-cluster-api-provider-aws-e2e
  • /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-test
  • pull-cluster-api-provider-aws-build
  • 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.

@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority labels Aug 7, 2021
@richardcase
Copy link
Member Author

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

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 9, 2021
@richardcase
Copy link
Member Author

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

1 similar comment
@richardcase
Copy link
Member Author

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

sedefsavas
sedefsavas reviewed Aug 10, 2021
View reviewed changes
Copy link
Contributor

@sedefsavas sedefsavas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need to update the quick start (in core cluster-api repo) and disable EKS permissions by passing a config to clusterawsadm there.

docs/book/src/topics/using-clusterawsadm-to-fulfill-prerequisites.md Show resolved Hide resolved
@richardcase
Copy link
Member Author

I think we also need to update the quick start (in core cluster-api repo) and disable EKS permissions by passing a config to clusterawsadm there.

I have added a section on this in the disabling eks documentation topic

@sedefsavas
Copy link
Contributor

I think we also need to update the quick start (in core cluster-api repo) and disable EKS permissions by passing a config to clusterawsadm there.

I have added a section on this in the disabling eks documentation topic

Yes, but for users that will not use EKS support, they will not read through the EKS section. In the quick start, we are not bringing up EKS clusters so the permissions for it is not needed.

@richardcase
Copy link
Member Author

In the quick start, we are not bringing up EKS clusters so the permissions for it is not needed.

I need to make a change to clusterctl to remove the bootstrap/controlplane providers so i will add a note to the quick start starts about disabling EKS support. Would that be ok?

@richardcase richardcase changed the title WIP: feat: graduate eks feat: graduate eks Aug 10, 2021
@k8s-ci-robot 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 Aug 10, 2021
@richardcase
Copy link
Member Author

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

@richardcase richardcase changed the title feat: graduate eks feat: Graduate EKS out of experimental Aug 10, 2021
@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

Until the EKS e2e have passed multiple times:

/hold

@sedefsavas
Copy link
Contributor

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

@richardcase
Copy link
Member Author

EKS e2e passed for the 2nd time.

@richardcase
Copy link
Member Author

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

@richardcase richardcase mentioned this pull request Aug 11, 2021
Closed
@randomvariable
Copy link
Member

All looks reasonable to me.
Going to give this a
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021
@randomvariable
Copy link
Member

randomvariable commented Aug 11, 2021 •
edited
Loading

For other reviewers (i.e. @sedefsavas ), we had discussed on Slack about IAM permissions, and we've concluded to enable EKS IAM by default as long as we include the release note warnings around those changes.

@richardcase
Copy link
Member Author

The e2e tests passed for a third time.

@richardcase
feat: graduate eks
ecec210 
The EKS functionality is graduated out of experimental with this change.
This means it will be enabled by default. As part of the graduation the
separate controlplane and bootstrap providers have been removed and
merged into the main AWS infrastructure provider.

clusterawsadm has been changed so that EKS is enabled by default. This
ensures that the correct pre-requisites are enabled by default. The
option in the bootstrap config to enable EKS have been changed to a
disable option. The docs have also been updated to reflect the fact that
EKS is enabled by default.

Fargate profiles is now enabled via the **EKSFargate** feature flag and
remains experimental.

Signed-off-by: Richard Case <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 11, 2021
@richardcase
Copy link
Member Author

richardcase commented Aug 11, 2021 •
edited
Loading

@randomvariable - sorry i had to resolve a conflict so the lgtm has been removed.

@randomvariable
Copy link
Member

/lgtm

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

/lgtm as well

@sedefsavas sedefsavas mentioned this pull request Aug 12, 2021
Merged
2 tasks
@randomvariable
Copy link
Member

/approve

Remove /hold wehn you're comfortable @richardcase

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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 Aug 12, 2021
@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

richardcase commented Aug 12, 2021 •
edited
Loading

Remove /hold wehn you're comfortable @richardcase

Will do, thanks @randomvariable

@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

After a flake in the teardown another pass of the e2e

@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

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

1 similar comment
@richardcase
Copy link
Member Author

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

@richardcase
Copy link
Member Author

Unmanaged e2e are passing as well.

So its time to:

/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 Aug 13, 2021
@k8s-ci-robot k8s-ci-robot merged commit e7d5636 into kubernetes-sigs:main Aug 13, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.7.0 milestone Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedefsavas sedefsavas sedefsavas left review comments

@dthorsen dthorsen Awaiting requested review from dthorsen

Assignees

@randomvariable randomvariable

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 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.

Graduate EKS out of experimental AWSClusterStaticIdentity requires duplicate secret for EKS
4 participants
@richardcase @k8s-ci-robot @sedefsavas @randomvariable