Adding chart for the AWS CNI #27
Conversation
|
The CI is failing due to the fact that Helm v3 wants the CRDs in a |
There was a problem hiding this comment.
Thanks @max-rocket-internet! I'll create an issue in this repo to update when the next stable CNI release is out.
Helm 3 does not even have a stable release yet. Sounds good. |
stable/aws-vpc-cni/Chart.yaml
Outdated
| - name: Nicholas Turner | ||
| url: https://github.com/nckturner | ||
| email: nckturner@users.noreply.github.com | ||
| - name: Stefan Prodan |
There was a problem hiding this comment.
Can you swap Stefan with Claes Mogren?
stable/aws-vpc-cni/README.md
Outdated
| helm repo add eks https://aws.github.io/eks-charts | ||
| ``` | ||
|
|
||
| To install the chart with the release name `aws-node` and default configuration: |
There was a problem hiding this comment.
Should we use aws-vpc-cni as the chart name/install name as well? As long as the daemonset is called aws-node everything should be ok... @mogren thoughts?
There was a problem hiding this comment.
I agree that aws-vpc-cni is a better name, but I think people are too used to aws-node by now. Would it be confusing if the daemonset has a different name?
There was a problem hiding this comment.
This is just the release name. I chose aws-node as an example in the README but it can be anything and with the default configuration the resources will be named aws-node* regardless due to fullnameOverride and nameOverride in values.yaml.
It's unconventional to set fullnameOverride and nameOverride by default but I think it makes sense given the current situation.
Other option is we rename the whole chart to aws-node and leave the name override options blank as is convention. But aws-node is a not a great name...
Up to you how you want it 🙂
There was a problem hiding this comment.
Let's call the chart aws-vpc-cni please. As @nckturner mentioned, as long as the Daemonset remains aws-node, we're good.
There was a problem hiding this comment.
There was a problem hiding this comment.
Sorry, I meant the release name, not the chart name.
There was a problem hiding this comment.
OK cool. Done! To be clear, this is cosmetic though. Users can choose their own name for the release.
stable/aws-vpc-cni/README.md
Outdated
| helm repo add eks https://aws.github.io/eks-charts | ||
| ``` | ||
|
|
||
| To install the chart with the release name `aws-node` and default configuration: |
There was a problem hiding this comment.
Let's call the chart aws-vpc-cni please. As @nckturner mentioned, as long as the Daemonset remains aws-node, we're good.
|
Can we merge this now? |
|
@max-rocket-internet the helm3 issue (#32) merged. I'm good merging this now. |
This is essentially a copy of v1.5.3 release YAML but with the standard Helm labels and options added.
I tested it and it works for me. I did a YAML diff of a pod and the daemonset between what this chart produces and what's in an EKS cluster by default and there's no major differences apart from labels and
securityContext.procMount: Defaultbut this isn't in the release YAML so I guess it's not required.Some notes....
Naming:
amazon-vpc-cni-k8sis not a good name for a chart and neither isaws-nodeso I've chosenaws-vpc-cniand set the default name override options toaws-nodeso as to be compatible with current EKS design.Installation: To install it over the top of the existing
aws-noderesources is super easy. Helm just overwrites them. See the README 😃Resource labels: These are the default Helm labels for any change but I also added
k8s-app: aws-nodefor backwards compatibility.Related issues:
aws/amazon-vpc-cni-k8s#336
aws/amazon-vpc-cni-k8s#679
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.