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

✨ Add EKS control plane #1724

Merged
merged 19 commits into from
Aug 28, 2020
Merged

✨ Add EKS control plane #1724

merged 19 commits into from
Aug 28, 2020

Conversation

richardcase
Copy link
Member

@richardcase richardcase commented May 14, 2020

What this PR does / why we need it:
Add support for EKS. This is WIP.

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 #1787

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 14, 2020
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 14, 2020
@richardcase richardcase changed the title ⚠️ [WIP] Adding eks control plane WIP: Adding eks control plane May 14, 2020
@k8s-ci-robot 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 14, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2020
@dthorsen
Copy link
Contributor

dthorsen commented Jun 4, 2020

@richardcase We will be interested in using this functionality soon. Is there anything we can do to help get this merged?

@richardcase
Copy link
Member Author

@dthorsen - i have another PR that i want to get merged first and then i will focis on this and push the additional changes i have made.

@rudoi rudoi mentioned this pull request Jul 1, 2020
11 tasks
@rudoi
Copy link
Contributor

rudoi commented Jul 1, 2020

hi @richardcase 👋, any updates for this PR? are there any TODO pieces of this that could perhaps be split into separate issues?

we (at New Relic) are looking to start work on the worker node aspect next week and would love to help get this controlplane support merged in any way we can.

@richardcase
Copy link
Member Author

@rudoi - been distracted by something else. Will pick it up again today and try and get it into a place where it can be used.

@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 Jul 3, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2020
@detiber detiber added this to the v0.5.x milestone Jul 13, 2020
@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 Jul 15, 2020
@randomvariable
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 15, 2020
@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 Jul 17, 2020
@randomvariable randomvariable dismissed vincepri’s stale review August 27, 2020 10:27

Has been fixed up

@randomvariable
Copy link
Member

ok, i think we're good to go, can we clean up the commit history (either squashed into one, or 'tell the story'), and then can lgtm.

richardcase and others added 19 commits August 28, 2020 11:07
This is work in progress to add support for an EKS control plane. Its
enabled via a feature flag.
Initialized when cluster creation was started
Ready when cluster is active
A small number of changes needed after the rebase and
added controller name when creating managed scope so that
metrics are reported correctly. Also fixed some linting issues.
This change enables AWSManagedCluster. The api types for
AWSManagedCluster and AWSmanagedControlPlane have been updated and their
corresponding controllers. Conditions have also been introduced.

The managed template has also been updated in light of field changes to
AWSmanagedCluster and AWSManagedControlPlane .
A few changes as a result of tesing. Also changed the control plane
logging so that the log types are listed.
The AWSManagedCluster has had most of its fields moved to the
AWSManagedControlPlane so that the reconciliiation logic is
cleaner. This follows the pattern used by CAPZ for the AKS
implementation.

The controller for AWSManagedCluster just reports back the
control plane endpoint, ready and failure domains to CAPI
from the managed control plane.

In the kubeconfig generation we support both aws-iam-authenticator and
also aws cli for generating the auth token. This is available as an
option on the AWSManagedControlPlane with the default being to use
aws-iam-authenticator.

Tags reconcilation has been added so that if the only things that changes
is the tags then these changes will be realised in the EKS related
resources.

The API group name has been changed to remove the `exp` prefix.
The AWSManagedControlPlane has been changed so that the EKS cluster name
can be specified as part of the Spec. If its not specified then a
defaulting admission webhook will create it. The default name will be
created using the follwoing rules:

- EKS cluster will be named [namespace]_[name] where namespace is the
name namespace of the AWSManagedControlPlane CRD instance and name is
the name of the AWSManagedControlPlane CRD instance.
- If there are any . in the cluster name then these will be replaced
with a _
-If the namespace/name combination is over 100 chars then a name will be
created from a hash.

Changes to security group service so that it can be reused between
managed and unmanaged clusters. Changed the managed control plane
so that it reconciles the LB and bastion SG. The automatically created
security group for the EKS control plane is looked up and added to the
status of the managed control plane.

The kubeconfig reconcilation has been changed in 2 ways. The
kubeconfig expected by CAPI has been changed to use a token that is
based on a presign URL. This token is only valid for a maximum of
15 minutes so on every sync it updates the kubeconfig with a new token
and this means we check that the --sync-period isn't above 10 mins if
EKS is enabled.

We also generate an additional kubeconfig that a use can use that will
utilize aws-iam-authenticator or the AWS cli to get the token and this
is what the user will need to get to connect to a cluster.
For now, this at least lets `--flavor managed` work.
Also remove `kustomization.yaml`s.
A new field called `KubernetesClusterName` has been added to the scopes.
For a non-managed (i.e. not EKS) cluster this will return the sample as
the Name(). But for a managed cluster (i.e. EKS) this will return the
name of the EKS cluster.
Remove patch, add "v" prefix

Drop patch from version as a workaround to support
`clusterctl config cluster --kubernetes-version X.Y.Z`,
which requires a patch version.

This includes checking that the version is not decreased

Tolerate patch versions in the controller
The EKSClusterName field has been removed from the managed scope and
dependent code has been updated to use the new KubernetesClusterName
field.

Additionally, the load balancer security group has been removed from the
control plane reconcilation.
Fixed a couple of typos in the name of an error and the message of
an error in the eks package after review.
@michaelbeaumont
Copy link
Contributor

@randomvariable Cleaned up now to where it "tells the story" pretty well

@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 28, 2020
@randomvariable
Copy link
Member

/approve
based on conversation https://kubernetes.slack.com/archives/CD6U2V71N/p1598515539065200

@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 28, 2020
@k8s-ci-robot k8s-ci-robot merged commit 936a024 into kubernetes-sigs:master Aug 28, 2020
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. area/provider/eks Issues or PRs related to Amazon EKS provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EKS Control Plane Implementation
10 participants