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.

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

fix: add mapping of current POD IP to external IP #10

Merged

Conversation

denis-tingaikin
Copy link
Member

Signed-off-by: denis-tingajkin denis.tingajkin@xored.com

Motivation

Related to networkservicemesh/sdk#1015

Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
@denis-tingaikin denis-tingaikin changed the title fixx: add mapping of current POD cluster IP to external IP fix: add mapping of current POD cluster IP to external IP Jul 12, 2021
Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
@denis-tingaikin denis-tingaikin changed the title fix: add mapping of current POD cluster IP to external IP fix: add mapping of current POD IP to external IP Jul 13, 2021
@edwarnicke
Copy link
Member

Why are we mapping Pod IP and not Node IP to external IP? The forwarder should be running on the hostnetwork:true Node networking namespace...

@denis-tingaikin
Copy link
Member Author

denis-tingaikin commented Jul 14, 2021

The forwarder should be running on the hostnetwork:true Node networking namespace...

@edwarnicke yes, you're correct for mechanisms and we already swapping Node IP to external IP for remote mechanisms.
But we also need to swap nsmgr-proxy IP to External IP on NSE registration to make NSE reachable over domains in case of public floating inter-domain scenario.

@edwarnicke
Copy link
Member

Ah... so how are we getting an External IP for the nsmgr-proxy Pod?

@denis-tingaikin
Copy link
Member Author

denis-tingaikin commented Jul 14, 2021

Ah... so how are we getting an External IP for the nsmgr-proxy Pod?

In case of interdomain we are getting it via DNS.
In case of floating interdomain we can store it in floating registry entries in NSE registration: url:${external-ip}:${nsmgr-proxy-port}.

@edwarnicke
Copy link
Member

I mean how is the nsmgr-proxy Pod figuring out what its externalIP is for purposes of registering it upstream to the registry.

@denis-tingaikin
Copy link
Member Author

nsmgr-proxy is not need to figuring out what its externalIP, it just can swap one address to another from mapip file.
See at networkservicemesh/sdk#1016 there we added a possible to map IPs on NSE registration.
So this PR just added an entry for swapping the IP in case of registration in floating registry in public clusters.

@edwarnicke
Copy link
Member

@denis-tingaikin Is the nsmgr-proxy also running in hostNetwork:true ?

@denis-tingaikin
Copy link
Member Author

denis-tingaikin commented Jul 14, 2021

@edwarnicke Currently is not, and I think we may try to run in hostNetwork as an alternative to this patch, but I feel this patch direction is more natural and flexible. WDYT?

@edwarnicke
Copy link
Member

@denis-tingaikin I'm very much in favor of not running nsmgr-proxy in hostNetwork:true.

I am a bit confused though because this PR seems to go looking for the ExternalIP for the Node, and is using that... which I would expect to be mapped to the Nodes InternalIP... and thus be delivered to hostNetwork:true

What am I misssing?

@denis-tingaikin
Copy link
Member Author

denis-tingaikin commented Jul 14, 2021

I am a bit confused though because this PR seems to go looking for the ExternalIP for the Node, and is using that... which I would expect to be mapped to the Nodes InternalIP... and thus be delivered to hostNetwork:true

What am I misssing?

With this PR we'll add map current POD IP to External nodeName of the POD where cmd-map-ip-k8s is running.

In this variant nsmgr-proxy will not have a hostNetwork:true and all registrations to upstream floating-registry will have a ExternalIP of the node where nsmgr-proxy is running.

Note: To access to ${externaIP}:${port} we have this line in deployment: https://github.com/networkservicemesh/deployments-k8s/blob/main/apps/nsmgr-proxy/nsmgr-proxy.yaml#L24

So with this patch we'll not need to modify somehow cmd-nsmgr-proxy, but need to add nodeName ref in cmd-ip-map sidecar.

Signed-off-by: denis-tingajkin <denis.tingajkin@xored.com>
@edwarnicke
Copy link
Member

Ah, got it, hostPort :)

@edwarnicke
Copy link
Member

Happy to merge this... I wish we didn't need hostPort... but I don't know what other options we have to solve this problem. So if something creative occurs to you, that's good... but otherwise, we can continue with this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants