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

envoy: Better track updates when qualifying Envoy resource names #29020

Merged
merged 2 commits into from
Nov 21, 2023
Merged

envoy: Better track updates when qualifying Envoy resource names #29020

merged 2 commits into from
Nov 21, 2023

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Nov 7, 2023 •
edited

Pass a boolean "updated" pointer to api.ResourceQualifiedName() in order to better track if updates were done or not.

In the case of Http Connection Manager RouteConfig the need for the update was ignored, so this is a bug fix for that.

Factor out the TCP proxy qualification logic to
qualifyTcpProxyResourceNames() which is similar to qualifyRouteConfigurationResourceNames() to keep the main logic simpler.

Fixes: #26037

"envoy-admin" cluster is renamed as "/envoy-admin", requiring all references in CEC/CCEC to be updated.

@jrajahalme jrajahalme added kind/bug This is a bug in the Cilium logic. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Nov 7, 2023
@jrajahalme jrajahalme self-assigned this Nov 7, 2023
@jrajahalme jrajahalme requested review from a team as code owners November 7, 2023 09:47
@jrajahalme jrajahalme added affects/v1.13 This issue affects v1.13 branch affects/v1.14 This issue affects v1.14 branch labels Nov 7, 2023
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the envoy-track-qualified-name-updates branch from a890abc to 1847dbc Compare November 7, 2023 11:43
@jrajahalme
Copy link
Member Author

Fixed pkg/k8s/watchers/cilium_envoy_config_test.go test case for the route config name that is now properly updated.

mhofstetter
mhofstetter reviewed Nov 7, 2023
View reviewed changes
pkg/policy/api/utils.go Outdated Show resolved Hide resolved
@jrajahalme
Copy link
Member Author

/test

mhofstetter
mhofstetter approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the issue! I won't block this PR for a personal opinion on the "pointer passing" part! 😃

pippolo84
pippolo84 approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 👍
I've left a couple of comments inline, but I understand that, being also a bugfix, we might not want to block this further. Maybe we can improve the updated pattern in a follow up PR?

pkg/envoy/ciliumenvoyconfig.go Outdated Show resolved Hide resolved
pkg/policy/api/utils.go Outdated Show resolved Hide resolved
@jrajahalme jrajahalme force-pushed the envoy-track-qualified-name-updates branch from 1847dbc to f2b7388 Compare November 13, 2023 10:07
@jrajahalme
envoy: Add qualifyTcpProxyResourceNames helper
4930ac6 
Factor out TcpProxy resource name qualifications to a helper function and
check for nil pointers.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
envoy: Better track updates when qualifying Envoy resource names
bcaeb41 
Pass a boolean "updated" pointer to api.ResourceQualifiedName() in order
to better track if updates were done or not.

In the case of Http Connection Manager RouteConfig the need for the
update was ignored, so this is a bug fix for that. This omission hid a
bug where HCM RouteConfigs in our examples (and tests) refer to
"envoy-admin" cluster while unqualified cluster names should no longer be
accessible to CEC/CCEC. Fix this by renaming "envoy-admin" as
"/envoy-admin". This requres all references to "envoy-admin" cluster to
be changed to "/envoy-admin", including in any CEC/CCEC users may have.

Factor out the TCP proxy qualification logic to
qualifyTcpProxyResourceNames() which is similar to
qualifyRouteConfigurationResourceNames() to keep the main logic simpler.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme force-pushed the envoy-track-qualified-name-updates branch from f2b7388 to bcaeb41 Compare November 17, 2023 12:54
@jrajahalme jrajahalme requested review from a team as code owners November 17, 2023 12:54
@jrajahalme jrajahalme added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Nov 17, 2023
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

Just noticed (due to a failing test in pkg/k8s/watchers that the missing update on HCM config's RouteConfig a bug was hidden. It is no longer possible to refer to the admin-cluster from a CEC/CCEC, as it is a bare name without any qualifications. IMO it is good to keep the clusters we use for Cilium policy enforcement hidden like this, but it may be desirable to give access to the admin cluster.

I see two paths to fis this:

  1. Rename admin-cluster as /admin-cluster. Then /admin-cluster is seen as already qualified, it is left as is. Current use of admin-cluster would break, requiring CEC/CCEC using it to be updated (which is acceptable, IMO)
  2. Require users to create their own "admin-cluster" in each CEC/CCEC they need it.

Both are breaking changes for any user CEC/CCEC that may exist. As such, 1. seems to be a better way forward, so I'll do it in this PR, open to your feedback of cause :-)

(ping @mhofstetter)

@mhofstetter
Copy link
Member

  1. Rename admin-cluster as /admin-cluster. Then /admin-cluster is seen as already qualified, it is left as is. Current use of admin-cluster would break, requiring CEC/CCEC using it to be updated (which is acceptable, IMO)

Makes sense. Thanks a lot!

@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 Nov 21, 2023
@jrajahalme jrajahalme added needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch and removed affects/v1.14 This issue affects v1.14 branch labels Nov 21, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.5 Nov 21, 2023
@jrajahalme jrajahalme merged commit 2e00194 into cilium:main Nov 21, 2023
60 of 61 checks passed
@joamaki joamaki mentioned this pull request Nov 29, 2023
Merged
14 tasks
@joamaki joamaki added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Nov 29, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.5 Nov 29, 2023
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Dec 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.5 Dec 1, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.14 in 1.14.5 Dec 1, 2023
@nebril nebril mentioned this pull request Dec 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@pippolo84 pippolo84 pippolo84 approved these changes

@mhofstetter mhofstetter mhofstetter approved these changes

@sayboras sayboras sayboras approved these changes

Assignees

@jrajahalme jrajahalme

Labels
affects/v1.13 This issue affects v1.13 branch backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. kind/bug This is a bug in the Cilium logic. kind/enhancement This would improve or streamline existing functionality. 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
No open projects
1.14.5
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jrajahalme @mhofstetter @nathanjsweet @pippolo84 @sayboras @joamaki