Skip to content

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Nov 29, 2023

Since the "NodePort" and "Primary" flag computation is mixed together we should do it for non-selected devices as well, so mark address for NodePort use only if the device is selected.

Additionally, only consider addresses on selected devices when filtering with --nodeport-addresses.

Simplify the special-handling for the addresses of cilium_host by just adjusting the maximum scope.

Finally add an assertion to tests to check that primary is correctly set.

@joamaki joamaki requested a review from a team as a code owner November 29, 2023 14:34
@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 Nov 29, 2023
@joamaki joamaki added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Nov 29, 2023
@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 Nov 29, 2023
@joamaki joamaki force-pushed the pr/joamaki/fix-node-address-primary branch from 848dfc1 to 60926d7 Compare November 29, 2023 14:36
@joamaki joamaki force-pushed the pr/joamaki/fix-node-address-primary branch from 60926d7 to 86a0182 Compare November 30, 2023 10:14
@joamaki
Copy link
Contributor Author

joamaki commented Nov 30, 2023

/test

@joamaki joamaki requested a review from bimmlerd November 30, 2023 13:54
@joamaki joamaki enabled auto-merge November 30, 2023 13:54
Since the "NodePort" and "Primary" flag computation is mixed together
we should do it for non-selected devices as well, so mark address
for NodePort use only if the device is selected.

Additionally, only consider addresses on selected devices when filtering
with --nodeport-addresses.

Simplify the special-handling for the addresses of cilium_host by just adjusting
the maximum scope.

Finally add an assertion to tests to check that primary is correctly set.

Fixes: 5342d01 ("datapath/tables: Add Table[NodeAddress]")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/fix-node-address-primary branch from 86a0182 to 0be3fc0 Compare December 1, 2023 08:25
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

I believe the logic is alright, one small style nit, in case you need to touch the code anyway

@joamaki
Copy link
Contributor Author

joamaki commented Dec 1, 2023

/test

@joamaki joamaki added this pull request to the merge queue Dec 1, 2023
@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 1, 2023
Merged via the queue into cilium:main with commit 6613f95 Dec 1, 2023
@joamaki joamaki deleted the pr/joamaki/fix-node-address-primary branch December 1, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants