-
Notifications
You must be signed in to change notification settings - Fork 737
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
Auto gen of AWS CNI, metrics helper and calico artifacts through helm #1271
Conversation
4c221b6
to
e22efe3
Compare
4a0e0d0
to
6599eca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks for taking this on! 🚀 I left a few minor comments on the charts.
Install the Calico CRDs: | ||
|
||
```shell | ||
kubectl apply -k github.com/aws/eks-charts/tree/master/stable/aws-calico/crds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these not be installed w/ helm? If not, can these be uploaded to the release assets in the aws-calico repo? I don't think it's a good practice to download crds directly from the default branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure Brandon, I have exported these from eks-charts. Will fix this as part of following PR.
affinity: | ||
nodeAffinity: | ||
requiredDuringSchedulingIgnoredDuringExecution: | ||
nodeSelectorTerms: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does calico support ARM and windows? If not, you may want to set an OS and arch node selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does support ARM. I am working on this - #1218. Once I validate will update the image.
Thanks for the comments Brandon. Appreciate your help on this :) |
6599eca
to
f665046
Compare
To install the chart with the release name `aws-calico` and default configuration: | ||
|
||
```shell | ||
$ helm install --name aws-calico --namespace kube-system eks/aws-calico |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the helm2 install command. We updated this not too long ago in the aws-node-termination-handler readme. It's probably worth it to change to the helm3 version. Keeping the helm2 instructions in addition to helm3 is up to you.
#Helm2
$ helm install --name aws-calico --namespace kube-system eks/aws-calico
#Helm3
$ helm install aws-calico --namespace kube-system eks/aws-calico
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure will update it in the following PR. There are 2 AIs for the following PR - calico multi arch and helm3 in README.
To install the chart with the release name `aws-vpc-cni` and default configuration: | ||
|
||
```shell | ||
$ helm install --name aws-vpc-cni --namespace kube-system eks/aws-vpc-cni |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for helm2 and helm3 installation instructions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a minor comment on each of the chart readmes, but otherwise, looks good! 👍
Any chance merging this anytime soon? |
Hi @diego-leapyear, I will try to merge this next week. |
Thank you @jayanthvn ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ecrRegion": "cn-northwest-1", | ||
"ecrAccount": "961992271922", | ||
"ecrDomain": "amazonaws.com.cn" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be so nice to get rid of these and use ECR Public repo (soon) :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!! ship it 🚀
Thanks to Brandon Wagner (@bwagner5) for the scripts. Modified it to create CNI/CNI metrics and calico manifests for all the supported regions/account id and copy as assets.
What type of PR is this?
feature
Which issue does this PR fix:
#758
What does this PR do / Why do we need it:
Jay Pipes comment -
"We will be replacing the config/ directory and its contents with a single charts/ directory containing a Helm chart that installs the equivalent of :latest Docker image tag with a set of reasonable production default configuration values. We will write a script that runs helm template to generate static manifests and attach those static manifests as artifacts to Github releases of the source repository's release. Finally, we'll be adding a script that automates the process of copying the Helm chart from this repository's charts/ directory into the github.com/aws/eks-charts repository's stable/amazon-vpc-cni-k8s Chart when releases are cut."
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Yes
Yaml creates -
Auto upload of yamls to GitHub -
https://github.com/jayanthvn/amazon-vpc-cni-k8s/releases/tag/v1.8
Automation added to e2e:
No
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No
Does this change require updates to the CNI daemonset config files to work?:
No
Does this PR introduce any user-facing change?:
Yes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.