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

Fix up default enconfig name #1918

Merged
merged 1 commit into from
Mar 8, 2022
Merged

Fix up default enconfig name #1918

merged 1 commit into from
Mar 8, 2022

Conversation

jayanthvn
Copy link
Contributor

What type of PR is this?
bug

Which issue does this PR fix:
Fixes #1917

What does this PR do / Why do we need it:
Default eniconfig name recommended in EKS docs - https://aws.amazon.com/premiumsupport/knowledge-center/eks-multiple-cidr-ranges/ is AZ name but we used to build the name as region + key(az name) but not annotating the node. Instead we just default it to AZ name and the below label setting will automatically pick the nodes AZ -

kubectl set env daemonset aws-node \
    -n kube-system ENI_CONFIG_LABEL_DEF=topology.kubernetes.io/zone

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

Testing done on this change:

Yes

---
# Source: aws-vpc-cni/templates/eniconfig.yaml
apiVersion: crd.k8s.amazonaws.com/v1alpha1
kind: ENIConfig
metadata:
  name: us-west-2a
spec:
  securityGroups:
    - sg-123
  subnet: subnet-123
---
# Source: aws-vpc-cni/templates/eniconfig.yaml
apiVersion: crd.k8s.amazonaws.com/v1alpha1
kind: ENIConfig
metadata:
  name: us-west-2b
spec:
  securityGroups:
    - sg-456
  subnet: subnet-456
---

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?:
N/A

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

N/A

Does this PR introduce any user-facing change?:

N/A


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

@jayanthvn jayanthvn requested a review from a team as a code owner March 8, 2022 20:48
Copy link
Contributor

@haouc haouc left a comment

Choose a reason for hiding this comment

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

lgtm

@jayanthvn jayanthvn merged commit f84db36 into aws:master Mar 8, 2022
sushrk pushed a commit to sushrk/amazon-vpc-cni-k8s that referenced this pull request Mar 9, 2022
@technotaff-nbs
Copy link

technotaff-nbs commented May 6, 2022

This change has broken our deployment, I think the intention was that you set your naming inside your values file to "a", "b", or "c". We have changed our values-file generator now but this was a breaking-change.

@joshuastern
Copy link

This change has broken our deployment, I think the intention was that you set your naming inside your values file to "a", "b", or "c". We have changed our values-file generator now but this was a breaking-change.

We experienced the same issue.

@jayanthvn
Copy link
Contributor Author

Sorry for the inconvenience. This was done to keep the helm chart same as the docs recommendation.

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.

Helm chart compatibility with EKS docs
4 participants