-
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
Fetch Region and CLUSTER_ID information from cni-metrics-helper env #1715
Conversation
2732843
to
2d7f32d
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.
Can you also add a section here - https://github.com/aws/amazon-vpc-cni-k8s/tree/master/cmd/cni-metrics-helper. We can give steps on how to enable IRSA for cni-metrics helper and include this -
Case 1: Cx not using IRSA, we need to get region and clusterID using IMDS (existing approach)
// Case 2: Cx using IRSA but not specified clusterID, we can still get this info if IMDS is not blocked
// Case 3: Cx blocked IMDS access and not using IRSA (which means region == "") OR
// not specified clusterID then its a Cx error
Also as part of this, lets update https://docs.aws.amazon.com/eks/latest/userguide/cni-metrics-helper.html for IRSA support. |
0f986b5
to
f5eed93
Compare
Updated Readme |
dd0ae62
to
f7f15e5
Compare
4343997
to
f2f7709
Compare
b1ff807
to
32a756a
Compare
Fix compilation errors (aws#1751) add support for running canary script in different regions (aws#1752) Regenerate pod eni values for new instance types (aws#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Closed issue message (aws#1761) * closed issue message * update message fix typo in upload script (aws#1763) Update calico file path Use an unique s3 bucket name (aws#1760) Update region Workflow to build arm and x86 images (aws#1764) DataStore.GetStats() refactoring to simplify adding new fields (aws#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (aws#1770) Automation script for running IT (aws#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (aws#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (aws#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (aws#1777) Chart and Manifest updates (aws#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (aws#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file Add HostNetworking Test for PPSG in test agent (aws#1720) * Add HostNetworking Test for PPSG in test agent * Updated PPSG test to validate vlan.eth link
60dafc0
to
e394298
Compare
Fix for cni-metrics-helper failing integration test
Nice! Any ETA on |
Will soon have one, but will be around mid February. If you want to use this change then we do have private image release. Change the image tag to v1.10.2-rc1 in this manifest and then apply this manifest |
I've tried upgrading
|
Add these env vars to your aws-node ds
Or instead apply the manifest for v1.10.1 first and then change the image tag. |
Thanks! It worked. |
@cgchinmay - Can you update your branch? We can finish the review this week. |
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.
Just one minor nit, rest lgtm.
What type of PR is this?
feature
Which issue does this PR fix:
Allows Customers to use cni-metrics-helper by completely blocking access to IMDS
What does this PR do / Why do we need it:
Removes hard dependency of cni-metrics-helper on IMDS
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
#1287
Testing done on this change:
Automation added to e2e:
Added Unit test
Manually tested on sample cluster
Tested by disabling pod-imds
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?:
Yes, if IMDS access needs to be blocked
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.