Fix topology view when displaying mixed connect-native/normal services. by apollo13 · Pull Request #13023 · hashicorp/consul

apollo13 · 2022-05-11T15:34:56Z

Description

If a service is registered twice, once with connect-native and once
without, the topology views would prune the existing intentions. This
change brings the code more in line with the transparent proxy behavior.

PR Checklist

updated test coverage
external facing docs updated
not a security concern
checklist folder consulted

agent/ui_endpoint_test.go

apollo13 · 2022-05-11T15:40:02Z

@kisunji This is a follow up to #12098 -- would be great if you could give that a review

agent/consul/state/catalog.go

Amier3 · 2022-06-14T15:07:41Z

Hey @apollo13

Hope you're doing well! Apologies for the delayed review on this, looks like it slipped through the cracks. We'll get this triaged shortly! 😄

apollo13 · 2022-08-10T07:53:56Z

@Amier3 Friendly ping :) any chance of a review?

apollo13 · 2022-10-07T07:18:44Z

Hi @Amier3, @kisunji. Any chance of a review? I realize that this is a small fix in the grand scheme of things, but nevertheless would improve traefik integration quite a bit.

kisunji · 2022-10-11T14:32:36Z

Hi @apollo13, I don't have the bandwidth to review at this time. If you could describe what the bug is that you are trying to fix (maybe replication steps if it is simple enough) it might help future reviewers understand what behavioral change you are making here.

apollo13 · 2022-10-12T10:44:27Z

Ok, here is a reproducer:
cfg.zip
Start with: consul agent -dev -config-dir=cfg

With the current main branch one will get the following result:

with my patch it properly shows the connection as allowed via intentions:

apollo13 · 2023-01-22T10:07:16Z

@Amier3 Any chance to get someone to review this?

dod-ia · 2023-06-28T07:08:04Z

I'm interested by seeing this commit validated to fix #14795

apollo13 · 2023-07-20T08:51:26Z

@kisunji I have rebased the code. Any chance you or someone else could spare the time to review this?

agent/consul/state/catalog.go

.changelog/13023.txt

kisunji

Looks good 👍 Please consider my feedback on the changelog and perhaps test locally to ensure things still look as expected.

If a service is registered twice, once with connect-native and once without, the topology views would prune the existing intentions. This change brings the code more in line with the transparent proxy behavior.

apollo13 · 2023-07-28T20:07:24Z

Thank you for your help! I have squashed the commits to reduce the noise (the later commits where just small fixes anyways) and will test if it works correctly (aside from the tests) over the weekend.

apollo13 · 2023-07-29T13:37:24Z

agent/ui_endpoint_test.go

+						ServiceSummary: ServiceSummary{
+							Name:           "cproxy",
+							Datacenter:     "dc1",
+							Tags:           []string{"http", "https"},


@kisunji I found a possible bug here. http and it's service-id cproxy-http is not connect enabled. Should this tag show up here then or not? Only cproxy-https with the tag https is connect enabled.

And I guess Nodes below could get deduped as well like it is done for the tags:

consul/agent/ui_endpoint.go

Line 597 in 449e050

for _, tag := range svc.Tags {

?

I pushed a commit to dedupe the nodes in this branch as well: bf3865f

Regarding my first comment in this thread: For transparent proxies it is checked if all instances are in the same "state" (

consul/agent/ui_endpoint.go

Lines 586 to 588 in 449e050

// Only consider the target service to be transparent when all its proxy instances are in that mode.

// This is done because the flag is used to display warnings about proxies needing to enable

// transparent proxy mode. If ANY instance isn't in the right mode then the warming applies.

). I guess it would make sense to apply a similar code connect-native services -- I will add code towards that end in another commit. If we think this should not be the case we revert / remove it easily again.

Actually I am leaning towards a service to be considered connect native if any of it's instances are. Connect native is not exactly the same as transparent proxies and allows for even more flexibility (like taking to only a subset of instances of a service which are connect enabled etc…). Will push a commit towards that goal.

Yep, I agree with your decision. Thanks for being thorough with the changes!

apollo13 · 2023-07-29T15:13:24Z

I have addressed the issues I ran over and successfully tested the changes in our cluster, jay :)

hc-github-team-consul-core · 2023-07-31T12:11:31Z

Backport failed @kisunji. Run: https://github.com/hashicorp/consul/actions/runs/5714489659

hc-github-team-consul-core · 2023-07-31T12:20:15Z

Backport failed @kisunji. Run: https://github.com/hashicorp/consul/actions/runs/5714489659

…s. (#13023) * Fix topoloy intention with mixed connect-native/normal services. If a service is registered twice, once with connect-native and once without, the topology views would prune the existing intentions. This change brings the code more in line with the transparent proxy behavior. * Dedupe nodes in the ServiceTopology ui endpoint (like done with tags). * Consider a service connect-native as soon as one instance is.

…l services. (#18330) Fix topology view when displaying mixed connect-native/normal services. (#13023) * Fix topoloy intention with mixed connect-native/normal services. If a service is registered twice, once with connect-native and once without, the topology views would prune the existing intentions. This change brings the code more in line with the transparent proxy behavior. * Dedupe nodes in the ServiceTopology ui endpoint (like done with tags). * Consider a service connect-native as soon as one instance is. Co-authored-by: Florian Apolloner <[email protected]>

apollo13 · 2023-07-31T16:13:59Z

Thank you for reviewing and merging, much appreciated 👍

…

On Mon, Jul 31, 2023, at 14:11, Chris S. Kim wrote: Merged #13023 <#13023> into main. — Reply to this email directly, view it on GitHub <#13023 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAT5C42ATCE52NUBCRSYRTXS6OFTANCNFSM5VVLSYIQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

apollo13 force-pushed the mixed-service-topology branch from 377860f to 94f1d38 Compare May 11, 2022 15:36

apollo13 commented May 11, 2022

View reviewed changes

agent/ui_endpoint_test.go Outdated Show resolved Hide resolved

apollo13 marked this pull request as ready for review May 11, 2022 15:39

apollo13 commented May 11, 2022

View reviewed changes

agent/consul/state/catalog.go Outdated Show resolved Hide resolved

apollo13 force-pushed the mixed-service-topology branch from 94f1d38 to 9a55042 Compare October 12, 2022 10:44

apollo13 mentioned this pull request Oct 12, 2022

Failed to correctly represent intentions in Consul UI #14795

Open

apollo13 force-pushed the mixed-service-topology branch from 9a55042 to 319f68b Compare November 12, 2022 20:00

apollo13 force-pushed the mixed-service-topology branch from 319f68b to c53330a Compare January 22, 2023 09:54

apollo13 force-pushed the mixed-service-topology branch from c53330a to 4e3320e Compare February 27, 2023 21:02

apollo13 force-pushed the mixed-service-topology branch from 4e3320e to bf3f5ff Compare July 20, 2023 08:50

apollo13 force-pushed the mixed-service-topology branch from bf3f5ff to c564e8c Compare July 28, 2023 15:26

apollo13 commented Jul 28, 2023

View reviewed changes

agent/consul/state/catalog.go Outdated Show resolved Hide resolved

apollo13 force-pushed the mixed-service-topology branch from 4b83eb6 to c772b45 Compare July 28, 2023 17:16

apollo13 commented Jul 28, 2023

View reviewed changes

agent/consul/state/catalog.go Outdated Show resolved Hide resolved

kisunji added the backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. label Jul 28, 2023

kisunji force-pushed the mixed-service-topology branch from c772b45 to abbea8f Compare July 28, 2023 20:00

kisunji reviewed Jul 28, 2023

View reviewed changes

.changelog/13023.txt Outdated Show resolved Hide resolved

kisunji changed the title ~~Fix topoloy intention with mixed connect-native/normal services.~~ Fix topology view when displaying mixed connect-native/normal services. Jul 28, 2023

kisunji approved these changes Jul 28, 2023

View reviewed changes

Fix topoloy intention with mixed connect-native/normal services.

efa5a6b

If a service is registered twice, once with connect-native and once without, the topology views would prune the existing intentions. This change brings the code more in line with the transparent proxy behavior.

apollo13 force-pushed the mixed-service-topology branch from 2bbb1a3 to efa5a6b Compare July 28, 2023 20:06

apollo13 commented Jul 29, 2023

View reviewed changes

apollo13 added 2 commits July 29, 2023 16:01

Dedupe nodes in the ServiceTopology ui endpoint (like done with tags).

bf3865f

Consider a service connect-native as soon as one instance is.

292869d

kisunji merged commit 6ada2e0 into hashicorp:main Jul 31, 2023

kisunji mentioned this pull request Jul 31, 2023

[1.16.x] Fix topology view when displaying mixed connect-native/normal services. #18330

Merged

chenrui333 mentioned this pull request Aug 9, 2023

consul 1.16.1 Homebrew/homebrew-core#139040

Merged

apollo13 deleted the mixed-service-topology branch September 1, 2023 16:41

asheshvidyut added a commit that referenced this pull request Sep 4, 2023

	// Only consider the target service to be transparent when all its proxy instances are in that mode.
	// This is done because the flag is used to display warnings about proxies needing to enable
	// transparent proxy mode. If ANY instance isn't in the right mode then the warming applies.

Conversation

apollo13 commented May 11, 2022

Description

PR Checklist

Uh oh!

Uh oh!

apollo13 commented May 11, 2022

Uh oh!

Uh oh!

Amier3 commented Jun 14, 2022

Uh oh!

apollo13 commented Aug 10, 2022

Uh oh!

apollo13 commented Oct 7, 2022

Uh oh!

kisunji commented Oct 11, 2022

Uh oh!

apollo13 commented Oct 12, 2022

Uh oh!

apollo13 commented Jan 22, 2023

Uh oh!

dod-ia commented Jun 28, 2023

Uh oh!

apollo13 commented Jul 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kisunji left a comment

Choose a reason for hiding this comment

Uh oh!

apollo13 commented Jul 28, 2023

Uh oh!

apollo13 Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apollo13 Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

apollo13 Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

apollo13 Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

apollo13 Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

kisunji Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

apollo13 commented Jul 29, 2023

Uh oh!

hc-github-team-consul-core commented Jul 31, 2023

Uh oh!

hc-github-team-consul-core commented Jul 31, 2023

Uh oh!

apollo13 commented Jul 31, 2023 via email

Uh oh!

apollo13 Jul 29, 2023 •

edited

Loading