-
Notifications
You must be signed in to change notification settings - Fork 991
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
AWS VPC CNI Chart updates for 1.7.3 release #253
Conversation
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.
Thanks a lot @shaunofneuron, this looks great!
I do think you need to bump stable/aws-vpc-cni/Chart.yaml
as well though, both version
and appVersion
.
Also, note that we did release v1.7.3 version yesterday with just one more commit, affecting the init-container.
I'll bump the version and also run through some additional testing, will let you know. |
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.
Thanks a lot @shaunofneuron, everything looks good to me. 🚀
You're welcome @mogren ! Thank you for all the good feedback. |
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.
I've some minor comments, changes look good otherwise.
- apiGroups: ["extensions"] | ||
resources: | ||
- daemonsets | ||
- '*' |
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.
are there any specific resources?
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.
I believe this change was made to match the config in the CNI repo, v1.7/aws-k8s-cni.yaml. The change was added in aws/amazon-vpc-cni-k8s#1082 ping @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.
If we know for sure which resources are needed, we can update the helm chart independently of the cni repo. By specifying * we might be allowing more than necessary access.
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 has been a while since I've had to write policies and not entirely sure where to get the entire list outside of kubectl cache but here are the resources I came up with for the extensions api group:
DaemonSet
Deployment
DeploymentRollback
Ingress
NetworkPolicy
PodSecurityPolicy
ReplicaSet
Scale
I might go try to figure out what ipamd change sparked the new resource requirement from that pr.
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.
@Kishorb @mogren I wasn't able to figure out what specific resources inside extension would be required outside of DaemonSet though I do know there are deprecation changes post 1.16 to the extensions group. I see my changes proposed here as orthogonal to the granularity of clusterrole policies as they represent upstream definitions more closely related to the vpc cni project. I do however agree with the least privileged approach and think this should be figured out rather soon. I'm not sure what other help I can provide, please let me know!
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.
@kishorj I agree that we should scope this down, but I guess for now it would be ok that the Helm chart is matching the current CNI config sample. That said, I opened aws/amazon-vpc-cni-k8s#1232 to address this in the CNI repo, and we should follow up with a PR here to match.
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.
sounds good.
Issue #, if available:
Description of changes: Changes reflect the new configurations provided by the latest release of aws-vpc-cni 1.7.2. In addition the readiness/liveness probe configurations are exposed to values.yaml.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@mogren There are some big changes here but this should be a good start to getting the helm chart up to date. I've diffed as much as possible against what is defined here and what I have running, you may have a better way of testing changes (please share).