proxy: use LocalNodeStore to retrieve local node #39268
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3cc2f35 to
c5e19a9
Compare
…status Currently, resolving the proxy IP for the proxy status uses `node.GetInternalIPv4Router` which uses a global variable that stores the local node. It's preferred that the local node should be retrieved via `LocalNodeStore` instead. Therefore, this commit refactors the proxy status accordingly. Note: fetching the proxy ip is best-effort, as the status collection will periodically try to resolve the status. (unfortunately it also doesn't pass a proper `context.Context` - therefore using a new background context with a timeout for the time being. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
…ules This commit uses the localnodestore to retrieve the localnode in the function `ReinstallRoutingRules` instead of using the global functions that use the stored local node from the global field. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
c5e19a9 to
782a90d
Compare
|
/test |
| if err := installFromProxyRoutesIPv4(logger, node.GetInternalIPv4Router(), defaults.HostDevice, fromIngressProxy, fromEgressProxy, mtu); err != nil { | ||
| if err := installFromProxyRoutesIPv4(logger, localNode.GetCiliumInternalIP(false), defaults.HostDevice, fromIngressProxy, fromEgressProxy, mtu); err != nil { |
There was a problem hiding this comment.
@mhofstetter 👋 did you look at the corresponding change for the IPv6 path, so that we can also get rid of the getCiliumNetIPv6() uglyness?
There was a problem hiding this comment.
No I didn't. But it looks like we're using the v4 IP from cilium_host (CiliumInternalIPv4 on the local node object) whereas we're using the first IPv6 from cilium_net - that isn't accessible from the local node object).
Is there a specific reason why we handle this differently for IPv6? Or can we just change this (and use the CiliumInternalIPv6) and get rid of getCiliumNetIPv6 completely? (happy to open that PR if we can agree on the change)
cc @rgo3 - maybe you can provide some additional context as well
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces usages of
node.GetInternalIPv4Router()inpkg/proxywithlocalNode.GetCiliumInternalIP(false)(with the local node retrieved from the injectedLocalNodeStore)Please review the individual commits.