feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip #2116

sudeeptoroy · 2021-06-07T09:41:16Z

advertise address can be used to let the control plane know the public routable address to reach the dataplane in cases where address is not reachable. ex: overlay n/w

Signed-off-by: Sudeepto Roy sudeepto.roy@gmail.com
Fix #1961

…know the public routable address to reach the dataplane in cases where address is not reachable. Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

…oy/kuma into feat/advertise-address

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

jpeach · 2021-06-10T06:52:33Z

api/mesh/v1alpha1/dataplane.proto

@@ -46,6 +46,10 @@ message Dataplane {
    // Public IP on which the dataplane is accessible in the network.
    string address = 5;

+    // AdvertiseAddress is the public routable address for the DP in case
+    // address is not routable; envoy binds to address
+    string advertiseAddress = 7;


Can you explain this more? The "address" field is documented as being publicly accessible, so if this is the real publicly accessible IP, that implies that "address" is set wrong. If there's 2 different publicly accessible addresses, then can there be more than 2? Do certain clients need to pick specific addresses sometime?

Hi @jpeach , yes there could be cases where part of the micro-service is running in an overlay network. Address will be allocated from the overlay n/w. Other data-plane will not be able to connect to these data-plane.
For these cases, we may want to add another advertise address so that other data-plane can connect to this advertise address and there will be additional mapping done to patch advertise address with address.
ex: docker application running in a private network on a VM.

So some DPs will connect to address and others will connect to advertiseAddress? How does any DP know which one to use?

The explanation of what this field is for and how it changes the behavior of the system needs to go somewhere, preferably i a long block comment above the field.

I expect we can come up with a better name than "advertiseAddress", which sounds like a command rather than a description. Maybe "externalAddress" or "secondaryAddress"?

Hi @jpeach dp will always bind to address and advertise address will take precedence in case advertise address is provided. I will add a block comment for this section.

For renaming, should we choose external address because secondary address might confuse user. let me know your opinion I will change it accordingly.

@jpeach Let me know if i should rename this variable to externalAddress
@jakubdyszkiewicz

I like adverisitedAddress name because it's consistent with ZoneIngress.
If we were to change this, I think we should do it for both resources.

Hi @jpeach dp will always bind to address and advertise address will take precedence in case advertise address is provided. I will add a block comment for this section.

So you only ever bind to one of the two addresses?

Hi @jpeach , DP always bind to address, advertiseAddress is just to update the service entry for this DP in the control plane.

I like adverisitedAddress name because it's consistent with ZoneIngress.
If we were to change this, I think we should do it for both resources.

Hi @jakubdyszkiewicz , yes that is why this name was choose.

@jpeach @jakubdyszkiewicz Let me know your thoughts on this. I will make changes accordingly.

this should be advertisedAddress

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

jakubdyszkiewicz · 2021-06-22T12:33:59Z

api/mesh/v1alpha1/dataplane.proto

@@ -46,6 +46,10 @@ message Dataplane {
    // Public IP on which the dataplane is accessible in the network.
    string address = 5;

+    // AdvertiseAddress is the public routable address for the DP in case
+    // address is not routable; envoy binds to address
+    string advertiseAddress = 7;


this should be advertisedAddress

jakubdyszkiewicz · 2021-06-22T12:34:30Z

api/mesh/v1alpha1/dataplane.proto

@@ -46,6 +46,10 @@ message Dataplane {
    // Public IP on which the dataplane is accessible in the network.
    string address = 5;

+    // AdvertiseAddress is the public routable address for the DP in case
+    // address is not routable; envoy binds to address


but Envoy binds to the address not advertisedAddress

Hey, @sudeeptoroy I'm sorry this PR did not land in Kuma 1.2.0. This contribution should have been included.
I also needed more time to test the PR.
There is a "typo" in the field and misleading comment but other than that it works as expected.

We can include this release on release branch and release it in 1.2.1

Thanks for testing this @jakubdyszkiewicz
I will make the changes you mentioned.

There is another suggestion i need.
The code has a function GetIP(). I think this needs to be changed to so that prom can query advertise address if configured.
This will lead to 3 additional changes and hence the question.

1. pkg/envoy/admin/client.go ---> should this use address or advertise address?
2. pkg/mads/util.go. --> this should use advertise address if configured.
3. pkg/xds/envoy/listeners/v3/access_log_configurer.go -> this should use advertise address if configured.

All of those 3 can use AdvertisedAddress, so we should change GetIP() to return AdvertisedAddress if it is set

jakubdyszkiewicz · 2021-06-22T12:38:59Z

Hey, @sudeeptoroy I'm sorry this PR did not land in Kuma 1.2.0. This contribution should have been included.
I also needed more time to test the PR.
There is a "typo" in the field and misleading comment but other than that it works as expected.

We can include this release on release branch and release it in 1.2.1

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

codecov-commenter · 2021-06-26T12:23:16Z

Codecov Report

Merging #2116 (60ef4ab) into master (3bd7955) will decrease coverage by 36.33%.
The diff coverage is 14.28%.

@@             Coverage Diff             @@
##           master    #2116       +/-   ##
===========================================
- Coverage   51.68%   15.34%   -36.34%     
===========================================
  Files         899       45      -854     
  Lines       40752     6327    -34425     
===========================================
- Hits        21061      971    -20090     
+ Misses      17672     5274    -12398     
+ Partials     2019       82     -1937

Impacted Files	Coverage Δ
api/mesh/v1alpha1/dataplane.pb.go	`16.05% <0.00%> (-21.01%)`	⬇️
api/mesh/v1alpha1/dataplane_helpers.go	`49.43% <25.00%> (-28.15%)`	⬇️
api/mesh/v1alpha1/mesh_helpers.go	`0.00% <0.00%> (-75.00%)`	⬇️
api/mesh/v1alpha1/timeout_helpers.go	`0.00% <0.00%> (-66.67%)`	⬇️
api/system/v1alpha1/zone_insight_helpers.go	`0.00% <0.00%> (-48.65%)`	⬇️
api/mesh/v1alpha1/externalservice_helpers.go	`0.00% <0.00%> (-39.29%)`	⬇️
api/mesh/v1alpha1/circuit_breaker.pb.go	`7.43% <0.00%> (-38.67%)`	⬇️
api/mesh/v1alpha1/fault_injection.pb.go	`12.08% <0.00%> (-35.17%)`	⬇️
api/mesh/v1alpha1/rate_limit.pb.go	`12.08% <0.00%> (-34.62%)`	⬇️
api/system/v1alpha1/zone_helpers.go	`0.00% <0.00%> (-33.34%)`	⬇️
... and 885 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bd7955...60ef4ab. Read the comment docs.

… address is not public ip (#2116) Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com> (cherry picked from commit 118cf06) # Conflicts: # api/mesh/v1alpha1/dataplane.pb.go

… address is not public ip (#2116) Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

feat(kuma-cp) advertise address can be used to let the control plane …

8bf4683

…know the public routable address to reach the dataplane in cases where address is not reachable. Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy requested a review from a team as a code owner June 7, 2021 09:41

sudeeptoroy and others added 7 commits June 7, 2021 17:20

make check changes

7c61c31

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

Merge branch 'kumahq:master' into feat/advertise-address

063cec9

Merge branch 'kumahq:master' into feat/advertise-address

4df95d8

go test changes

5c711e3

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

Merge branch 'feat/advertise-address' of https://github.com/sudeeptor…

94f84a5

…oy/kuma into feat/advertise-address

go test changes

8d04f07

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

go test changes

08cf3b7

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

jpeach reviewed Jun 10, 2021

View reviewed changes

merged master

71ad270

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy requested review from jpeach and jakubdyszkiewicz June 16, 2021 16:53

jakubdyszkiewicz approved these changes Jun 17, 2021

View reviewed changes

jakubdyszkiewicz requested changes Jun 22, 2021

View reviewed changes

sudeeptoroy added 4 commits June 26, 2021 00:47

taken care of comments

c87e5b0

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

merged latest master

b27a0ec

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

Merge branch 'kumahq-master' into feat/advertise-address

fd447d3

corrected test cases

96b2d63

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy requested a review from jakubdyszkiewicz June 25, 2021 19:37

sudeeptoroy added 3 commits June 26, 2021 10:27

test cases, variable renamed

ca5f582

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

make check changes

bc2103e

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

integration check correct

60ef4ab

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

jakubdyszkiewicz approved these changes Jun 28, 2021

View reviewed changes

jakubdyszkiewicz added the backport-to-stable label Jun 28, 2021

jakubdyszkiewicz merged commit 118cf06 into kumahq:master Jun 28, 2021

mergify bot mentioned this pull request Jun 28, 2021

feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip (backport #2116) #2226

Merged

jakubdyszkiewicz pushed a commit that referenced this pull request Jun 28, 2021

feat(kuma-cp) dataplane use advertise address to add a routable ip if…

095f24b

… address is not public ip (#2116) Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy deleted the feat/advertise-address branch July 26, 2021 14:45

lobkovilya mentioned this pull request Nov 29, 2021

Missing docs for Dataplane.AdvertisedAddress kumahq/kuma-website#613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip #2116

feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip #2116

sudeeptoroy commented Jun 7, 2021

jpeach Jun 10, 2021

sudeeptoroy Jun 10, 2021

jpeach Jun 11, 2021

sudeeptoroy Jun 11, 2021

sudeeptoroy Jun 14, 2021

jakubdyszkiewicz Jun 14, 2021

sudeeptoroy Jun 15, 2021

sudeeptoroy Jun 15, 2021

sudeeptoroy Jun 16, 2021

jakubdyszkiewicz Jun 22, 2021

jakubdyszkiewicz Jun 22, 2021

jakubdyszkiewicz Jun 22, 2021

sudeeptoroy Jun 22, 2021

jakubdyszkiewicz Jun 24, 2021

jakubdyszkiewicz commented Jun 22, 2021

codecov-commenter commented Jun 26, 2021

feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip #2116

feat(kuma-cp) dataplane use advertise address to add a routable ip if address is not public ip #2116

Conversation

sudeeptoroy commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubdyszkiewicz commented Jun 22, 2021

codecov-commenter commented Jun 26, 2021

Codecov Report