Skip to content

cli: aws mixed nodes install fix #36336

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
Merged
merged 1 commit into from
Dec 18, 2024
Merged

cli: aws mixed nodes install fix #36336

merged 1 commit into from
Dec 18, 2024

Conversation

viktor-kurchenko
Copy link
Contributor

@viktor-kurchenko viktor-kurchenko commented Dec 3, 2024 •
edited
Loading

The PR fixes AWS installation for mixed node clusters by logging warning instead of breaking it with error.

Fixes: #36320

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 3, 2024
@github-actions github-actions bot added cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary labels Dec 3, 2024
@viktor-kurchenko viktor-kurchenko added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. release-note/misc This PR makes changes that have no direct user impact. labels Dec 3, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 3, 2024
@viktor-kurchenko
Copy link
Contributor Author

/test

@viktor-kurchenko viktor-kurchenko marked this pull request as ready for review December 3, 2024 19:04
@viktor-kurchenko viktor-kurchenko requested review from a team as code owners December 3, 2024 19:04
@viktor-kurchenko viktor-kurchenko mentioned this pull request Dec 3, 2024
Closed
3 tasks
squeed
squeed requested changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

I think we can get rid of this detection entirely. We use it do determine the prefix of the masquerade interfaces, but multiple prefixes are allowed. If we just set egressMasqueradeInterfaces=eth+,ens+ we can cull this entire code path.

@viktor-kurchenko
Copy link
Contributor Author

I think we can get rid of this detection entirely. We use it do determine the prefix of the masquerade interfaces, but multiple prefixes are allowed. If we just set egressMasqueradeInterfaces=eth+,ens+ we can cull this entire code path.

To be honest I really like that idea!
@nbusseneau WDYT?

@nbusseneau
Copy link
Member

I think we can get rid of this detection entirely. We use it do determine the prefix of the masquerade interfaces, but multiple prefixes are allowed. If we just set egressMasqueradeInterfaces=eth+,ens+ we can cull this entire code path.

Are we sure this is working? I had tried it but it didn't seem to work 🤔

@viktor-kurchenko
Copy link
Contributor Author

Are we sure this is working? I had tried it but it didn't seem to work 🤔

It's working after this fix: #36103 )))

@viktor-kurchenko viktor-kurchenko force-pushed the vk/pr/cli/aws/fix branch 2 times, most recently from bf841b5 to 35319a8 Compare December 6, 2024 19:42
@viktor-kurchenko
Copy link
Contributor Author

/test

@viktor-kurchenko
Copy link
Contributor Author

/test

@viktor-kurchenko
Copy link
Contributor Author

/test

squeed
squeed approved these changes Dec 7, 2024
View reviewed changes
Copy link
Contributor

@squeed squeed left a comment

Choose a reason for hiding this comment

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

Nice and clean :)

@viktor-kurchenko
Copy link
Contributor Author

Nice and clean :)

Thank you!

@viktor-kurchenko
Copy link
Contributor Author

@nbusseneau kind ping)

@viktor-kurchenko viktor-kurchenko mentioned this pull request Dec 12, 2024
Merged
@viktor-kurchenko
cli: aws install fix
232337d 
The PR fixes AWS installation by dropping unneeded AMI detetction logic
because Cilium supports the list of egress masquerade interfaces.

Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>
@viktor-kurchenko
Copy link
Contributor Author

/test

@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 Dec 18, 2024
@gandro gandro mentioned this pull request Dec 18, 2024
Closed
8 tasks
@squeed squeed added this pull request to the merge queue Dec 18, 2024
Merged via the queue into main with commit 08f72c3 Dec 18, 2024
210 checks passed
@squeed squeed deleted the vk/pr/cli/aws/fix branch December 18, 2024 14:39
@github-actions github-actions bot mentioned this pull request Jan 13, 2025
Open
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

@nbusseneau nbusseneau nbusseneau approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
cilium-cli This PR contains changes related with cilium-cli cilium-cli-exclusive This PR only impacts cilium-cli binary 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. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Encounter cluster has nodes with different OS images issue when using 1.17.0-pre.3
4 participants
@viktor-kurchenko @nbusseneau @squeed @tommyp1ckles