Helm dir update by jaydeokar · Pull Request #2144 · aws/amazon-vpc-cni-k8s

jaydeokar · 2022-11-17T19:51:35Z

What type of PR is this?
cleanup

Which issue does this PR fix:
#2075

What does this PR do / Why do we need it:
Defining chart folder structure as per helm v3 standard. Putting CRDs into crds folder also ensures they get installed before the chart is rendered and avoid any failures.

If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:

Testing done on this change: Yes tested install and upgrade on 1.23 EKS cluster. Helm v2 users will have to migrate to v3 using the documented steps provided by Helm.

Automation added to e2e:
N/A

Will this PR introduce any new dependencies?:
N/A

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No break for upgrade/downgrade. Yes

Does this change require updates to the CNI daemonset config files to work?:
No

Does this PR introduce any user-facing change?:
No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jdn5126

Can you also run make generate-cni-yaml to update the manifests in config/master/aws-vpc-cni*?

charts/aws-vpc-cni/crds/customresourcedefinition.yaml

jdn5126 · 2022-11-22T23:04:57Z

charts/aws-vpc-cni/crds/customresourcedefinition.yaml

 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  name: eniconfigs.crd.k8s.amazonaws.com
  labels:
-{{ include "aws-vpc-cni.labels" . | indent 4 }}


Does this YAML file need all of the labels defined by aws-vpc-cni.labels? I think we are fine just keeping k8s-app: aws-node here

We can get rid of those extra helm labels, they aren't required

Unresolving as we decided it is safer to keep these labels

jdn5126

Changes LGTM. I did not realize that every resource was including "aws-vpc-cni.labels" by default, but I think removing the unnecessary labels for the CRD is still fine.

jayanthvn · 2022-11-23T18:05:53Z

Lets be careful before removing the labels because someone might be using it :)

kishorj · 2022-11-29T22:19:47Z

Also verify you can update the crds via kubectl apply -k option similar to other charts.

charts/aws-vpc-cni/crds/customresourcedefinition.yaml

charts/aws-vpc-cni/README.md

kishorj · 2022-12-06T01:33:41Z

LGTM

jdn5126

approving as all comments have been resolved

* create publisher with logger (#2119) * Add missing rules when NodePort support is disabled (#2026) * Add missing rules when NodePort support is disabled * the rules that need to be installed for NodePort support and SNAT support are very similar. The same traffic mark is needed for both. As a result, rules that are currently installed only when NodePort support is enabled should also be installed when external SNAT is disabled, which is the case by default. * remove "-m state --state NEW" from a rule in the nat table. This is always true for packets that traverse the nat table. * fix typo in one rule's name (extra whitespace). Fixes #2025 Co-authored-by: Quan Tian <[email protected]> Signed-off-by: Antonin Bas <[email protected]> * Fix typos and unit tests Signed-off-by: Antonin Bas <[email protected]> * Minor improvement to code comment Signed-off-by: Antonin Bas <[email protected]> * Address review comments * Delete legacy nat rule * Fix an unrelated log message Signed-off-by: Antonin Bas <[email protected]> Signed-off-by: Antonin Bas <[email protected]> Co-authored-by: Jayanth Varavani <[email protected]> Co-authored-by: Sushmitha Ravikumar <[email protected]> * downgrade test go.mod to align with root go.mod (#2128) * skip addon installation when addon info is not available (#2131) * Merging test/Makefile and test/go.mod to the root Makefil and go.mod, adjust the .github/workflows and integration test instructions (#2129) * update troubleshooting docs for CNI image (#2132) fix location where make command is run * fix env name in test script (#2136) * optionally allow CLUSTER_ENDPOINT to be used rather than the cluster-ip (#2138) * optionally allow CLUSTER_ENDPOINT to be used rather than the kubernetes cluster ip * remove check for kube-proxy * add version to readme * Add resources config option to cni metrics helper (#2141) * Add resources config option to cni metrics helper * Remove default-empty resources block; replace with conditional * Add metrics for ec2 api calls made by CNI and expose via prometheus (#2142) Co-authored-by: Jay Deokar <[email protected]> * increase workflow role duration to 4 hours (#2148) * Update golang 1.19.2 EKS-D (#2147) * Update golang * Move to EKS distro builds * [HELM]: Move CRD resources to a separate folder as per helm standard (#2144) Co-authored-by: Jay Deokar <[email protected]> * VPC-CNI minimal image builds (#2146) * VPC-CNI minimal image builds * update dependencies for ginkgo when running integration tests * address review comments and break up init main function * review comments for sysctl * Simplify binary installation, fix review comments Since init container is required to always run, let binary installation for external plugins happen in init container. This simplifies the main container entrypoint and the dockerfile for each image. * when IPAMD connection fails, try to teardown pod network using prevResult (#2145) * add env var to enable nftables (#2155) * fix failing weekly cron tests (#2154) * Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER and remove no-op setter (#2153) * Deprecate AWS_VPC_K8S_CNI_CONFIGURE_RPFILTER * update release version comments Signed-off-by: Antonin Bas <[email protected]> Co-authored-by: Jeffrey Nelson <[email protected]> Co-authored-by: Antonin Bas <[email protected]> Co-authored-by: Jayanth Varavani <[email protected]> Co-authored-by: Sushmitha Ravikumar <[email protected]> Co-authored-by: Jerry He <[email protected]> Co-authored-by: Brandon Wagner <[email protected]> Co-authored-by: Jonathan Ogilvie <[email protected]> Co-authored-by: Jay Deokar <[email protected]>

…ws#2144) Co-authored-by: Jay Deokar <[email protected]>

mindw · 2023-01-20T07:56:54Z

From Helm documentation:

Some caveats (and explanations)
There is no support at this time for upgrading or deleting CRDs using Helm.

The above translates to special upgrade instructions whenever a CRD is changed. It also doesn't solve the chicken and egg problem (see rest of Some caveats above).
.
Please consider reverting the PR and suggesting option 2 as for #2075.

jdn5126 · 2023-01-20T16:25:44Z

@mindw we did consider this problem and decided that a crds folder within the aws-vpc-cni helm chart is the best solution. At this time, we only have one CRD, ENIConfig, and the definition is not something that will change frequently, if ever. If we ever add another CRD or change the definition for ENIConfig, we will include special instructions in the release notes for how to install the new CRDs.

The crds folder is still the recommended method and operator standard. If we get more CRDs and this is not as simple to manage, then we will use two charts (method two).

jaydeokar requested a review from a team as a code owner November 17, 2022 19:51

jaydeokar force-pushed the helm_dir_update branch from 76926a8 to 9326399 Compare November 18, 2022 19:31

This was referenced Nov 22, 2022

[aws-vpc-cni] Helm install failed, due to missing required annotation for CRD metadata aws/eks-charts#421

Closed

Handle helm upgrade of AWS VPC CNI more gracefully aws/eks-charts#57

Closed

jdn5126 reviewed Nov 22, 2022

View reviewed changes

jaydeokar force-pushed the helm_dir_update branch 2 times, most recently from 96ce530 to fecdf02 Compare November 23, 2022 15:56

jdn5126 previously approved these changes Nov 23, 2022

View reviewed changes

jayanthvn added this to the v1.12.1 milestone Nov 23, 2022

kishorj reviewed Nov 29, 2022

View reviewed changes

charts/aws-vpc-cni/crds/customresourcedefinition.yaml Outdated Show resolved Hide resolved

charts/aws-vpc-cni/README.md Outdated Show resolved Hide resolved

charts/aws-vpc-cni/README.md Show resolved Hide resolved

jaydeokar dismissed jdn5126’s stale review via 447c34f December 1, 2022 01:03

jaydeokar force-pushed the helm_dir_update branch 2 times, most recently from 447c34f to 288a567 Compare December 1, 2022 01:40

jaydeokar requested a review from kishorj December 1, 2022 19:57

[HELM]: Move CRD resources to a separate folder as per helm standard

e7e05b5

jdn5126 force-pushed the helm_dir_update branch from 288a567 to e7e05b5 Compare December 6, 2022 16:31

jdn5126 approved these changes Dec 6, 2022

View reviewed changes

jdn5126 merged commit a632a4f into aws:master Dec 6, 2022

haouc pushed a commit to haouc/amazon-vpc-cni-k8s that referenced this pull request Dec 13, 2022

[HELM]: Move CRD resources to a separate folder as per helm standard (a…

353cbfa

…ws#2144) Co-authored-by: Jay Deokar <[email protected]>

jaydeokar deleted the helm_dir_update branch February 25, 2025 20:39

Conversation

jaydeokar commented Nov 17, 2022

Uh oh!

jdn5126 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdn5126 Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

jaydeokar Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

jdn5126 Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

jdn5126 left a comment

Choose a reason for hiding this comment

Uh oh!

jayanthvn commented Nov 23, 2022

Uh oh!

kishorj commented Nov 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kishorj commented Dec 6, 2022

Uh oh!

jdn5126 left a comment

Choose a reason for hiding this comment

Uh oh!

mindw commented Jan 20, 2023

Uh oh!

jdn5126 commented Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jdn5126 commented Jan 20, 2023 •

edited

Loading