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 support for custom subnets in EKS #164

Closed

Conversation

janwillies
Copy link
Contributor

Description of your changes

PoC which adds support for custom subnets/secondary cidr ranges in EKS according to https://docs.aws.amazon.com/eks/latest/userguide/cni-custom-network.html

In a nutshell, we need to create an ENIConfig CR per AvailabilityZone:

apiVersion: crd.k8s.amazonaws.com/v1alpha1
kind: ENIConfig
metadata: 
  name: us-west-2a
spec: 
  securityGroups: 
    - sg-0dff111a1d11c1c11
  subnet: subnet-011b111c1f11fdf11

For this we need:

  • workergroup securitygroup
  • custom subnet & availabilityzone tuple

func (r *Reconciler) _customnetwork is the entry point for this.

I'd like some comments on the approach and what's necessary to get this upstream

Fixes #163

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

This looks like a really cool use-case @janwillies! Apologies for the current state of the EKS controller as it is a bit unwieldy at this time. It does a lot of "magic" behind the scenes, and I imagine we will wan to move it to having separate API types for clusters and node groups. I was wondering if you would prefer to try to get your functionality implemented in the short-term (with the knowledge that it will likely need to be refactored in the not too distant future) or work towards getting a v1beta1 representation of EKS prior to moving forward with this custom subnet functionality?

A few observations for the time-being:

  • Looks like this got caught in the naming refactor (stack-aws -> provider-aws) so I believe a rebase will be necessary
  • It looks like you may be trying to create objects in the remote cluster using the EKS client. I think to do this we will need to get the kubeconfig info for provisioned cluster, then use it to get a kube client. As I am thinking about this, I wonder if we could accomplish some of this functionality just using a KubernetesApplication that we schedule to the EKS cluster we create. Have you considered that approach? Do you see major benefits to your approach here instead?

Signed-off-by: Jan Willies <jan.willies@accenture.com>
@janwillies
Copy link
Contributor Author

janwillies commented Mar 23, 2020

Thanks for the quick feedback! Challenges with KubernetesApplication might be

  1. wait until ENIConfig was deployed successfully (before nodes register I believe)
  2. add env variables to existing aws-node daemon-set

This has to be done before pods are scheduled. I basically followed the same approach as _awsauth()

@janwillies
Copy link
Contributor Author

this is a good writeup with lots of comments: eksctl-io/eksctl#1096

@muvaf
Copy link
Member

muvaf commented Mar 28, 2020

I think there could be two paths forward as we get closer to make EKS v1beta1, which will be accelerated when composition is implemented, which will be in latest v0.10.0:

  • Implement this in EKS controller, then remove it all when we move to v1beta1 because of the high fidelity goal and then write the necessary KubernetesApplication in a composition.
  • Create a KubernetesApplication in the namespace in the same namespace of the EKSCluster connection secret. Mark EKSCluster as ready only if the KubernetesApplication completed making the patches and creations. Then when we do move to v1beta1 EKS, the refactor is much less.

I am leaning towards the second scenario where we try to hardcode a composition into EKSCluster controller in case CNI is enabled.

I have a feeling that the challenges you listed should be doable. @hasheddan is the one who worked on that resource recently.

add env variables to existing aws-node daemon-set

I think KubernetesApplication patches the remote resource if it already exists. So, it should work in theory though we'd have to test.

wait until ENIConfig was deployed successfully (before nodes register I believe)

If EKSCluster controller marks the cluster ready only if KubernetesApplication is ready in case CNI is enabled, this should be OK.

FWIW, you might need to use KubernetesApplicationResource which represents an individual remote cluster resource instead of KubernetesApplication so that you get more granular view of the status of the remote resource in status of KubernetesApplicationResource; which is not the case in KubernetesApplication because it includes many resources and only concerned by whether KubernetesApplicationResources are submitted or not.

All this should be testable with current EKSCluster, i.e. after it's ready create the KubernetesApplication or set of KubernetesApplicationResources that does what you want and see if it worked.

@muvaf
Copy link
Member

muvaf commented Jul 2, 2020

@janwillies do you think this PR is still relevant?

@janwillies
Copy link
Contributor Author

No, let me close this in favor of a solution on top of v1beta1 eks cluster.

Looking at v1beta1 it seems there is no way to edit Kubernetes resources in the target cluster (like aws-auth ConfigMap or the aws-node DaemonSet). I wonder what will be a good way to support this.

@janwillies janwillies closed this Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for custom subnets in EKS (secondary cidr ranges)
3 participants