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.

Sign up for GitHub

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

Jump to bottom

Enable update-ec2-adapter-limit-via-api by default #24564

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Enable update-ec2-adapter-limit-via-api by default #24564

merged 1 commit into from
Apr 12, 2023

Conversation

christarazi
Copy link
Member

This prevents reports of crashes of the Operator because the instance
type limits are not up-to-date.

Related: #18197

Signed-off-by: Chris Tarazi chris@isovalent.com

@christarazi christarazi added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/eni Impacts ENI based IPAM. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. sig/ipam IP address management, including cloud IPAM labels Mar 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Mar 24, 2023
@christarazi christarazi force-pushed the pr/christarazi/update-aws-ec2-default branch 2 times, most recently from ebbc4e5 to bb890f9 Compare March 24, 2023 21:56
@christarazi christarazi marked this pull request as ready for review March 24, 2023 22:23
@christarazi christarazi requested review from a team as code owners March 24, 2023 22:23
@christarazi christarazi requested a review from squeed March 24, 2023 22:23
@christarazi
Copy link
Member Author

/test

@chancez
Copy link
Contributor

chancez commented Mar 24, 2023

You'll want to update eni.updateEC2AdapterLimitViaAPI in helm values too.

@chancez
Copy link
Contributor

chancez commented Mar 24, 2023

This could also require updated IAM permissions for Cilium or the node IAM role to allow ec2:DescribeInstances. In EKS, nodes usually have AmazonEKSWorkerNodePolicy which includes this permission, so it should work in most cases unless the user configured Cilium to use it's own AWS credentials separate from the node.

@christarazi
Copy link
Member Author

christarazi commented Mar 25, 2023 •
edited

You'll want to update eni.updateEC2AdapterLimitViaAPI in helm values too.

That is the value that I modified, no?

This could also require updated IAM permissions for Cilium or the node IAM role to allow ec2:DescribeInstances. In EKS, nodes usually have AmazonEKSWorkerNodePolicy which includes this permission, so it should work in most cases unless the user configured Cilium to use it's own AWS credentials separate from the node.

That's a good point. I think it would be good to clarify that in the upgrade notes. I'll push a change with that.

@chancez
Copy link
Contributor

chancez commented Mar 27, 2023

That is the value that I modified, no?

Github hid the diff, my bad. I confused it with the markdown files being changed.

@christarazi christarazi force-pushed the pr/christarazi/update-aws-ec2-default branch 2 times, most recently from 4cb8964 to 8623707 Compare March 29, 2023 00:43
@christarazi christarazi requested a review from a team as a code owner March 29, 2023 00:43
@christarazi
Copy link
Member Author

@chancez Pushed a note to the upgrade guide.

@christarazi
Copy link
Member Author

/test

qmonnet
qmonnet requested changes Mar 31, 2023
View reviewed changes
install/kubernetes/cilium/values.yaml Show resolved Hide resolved
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet added the upgrade-impact This PR has potential upgrade or downgrade impact. label Mar 31, 2023
@christarazi christarazi requested review from a team as code owners April 4, 2023 23:36
@christarazi
Copy link
Member Author

Apologies for the noise, mis-clicked on the GH UI.

@christarazi christarazi force-pushed the pr/christarazi/update-aws-ec2-default branch from 35e5d04 to 05afee6 Compare April 4, 2023 23:45
@christarazi
Copy link
Member Author

/test

@qmonnet qmonnet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 5, 2023
@christarazi
Enable update-ec2-adapter-limit-via-api by default
8699ac6 
This prevents reports of crashes of the Operator because the instance
type limits are not up-to-date.

Related: cilium#18197

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/update-aws-ec2-default branch from 05afee6 to 8699ac6 Compare April 11, 2023 18:42
@christarazi christarazi removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Apr 11, 2023
@christarazi
Copy link
Member Author

christarazi commented Apr 11, 2023 •
edited

/test

Edit: ConformanceKind hit variant of #22217, re-running

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 12, 2023
@christarazi christarazi merged commit 9a5e7dc into cilium:master Apr 12, 2023
41 of 42 checks passed
@christarazi christarazi deleted the pr/christarazi/update-aws-ec2-default branch April 12, 2023 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@chancez chancez chancez approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/eni Impacts ENI based IPAM. integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/ipam IP address management, including cloud IPAM upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@christarazi @chancez @squeed @nbusseneau @qmonnet