consul icon indicating copy to clipboard operation
consul copied to clipboard

Published 20 hours ago • verified publisher icon hashicorp

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

Open apollo13 opened this issue 2 years ago • 7 comments

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

  • [x] updated test coverage
  • [x] external facing docs updated
  • [x] not a security concern
  • [x] checklist folder consulted

apollo13 avatar May 11 '22 15:05 apollo13

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

apollo13 avatar May 11 '22 15:05 apollo13

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! 😄

Amier3 avatar Jun 14 '22 15:06 Amier3

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

apollo13 avatar Aug 10 '22 07:08 apollo13

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.

apollo13 avatar Oct 07 '22 07:10 apollo13

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.

kisunji avatar Oct 11 '22 14:10 kisunji

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: Screenshot 2022-10-12 at 12-40-35 crm - Consul

with my patch it properly shows the connection as allowed via intentions: Screenshot 2022-10-12 at 12-41-15 crm - Consul

apollo13 avatar Oct 12 '22 10:10 apollo13

@Amier3 Any chance to get someone to review this?

apollo13 avatar Jan 22 '23 10:01 apollo13

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

ventaubain avatar Jun 28 '23 07:06 ventaubain

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

apollo13 avatar Jul 20 '23 08:07 apollo13

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 avatar Jul 28 '23 20:07 apollo13

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

apollo13 avatar Jul 29 '23 15:07 apollo13

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

hc-github-team-consul-core avatar Jul 31 '23 12:07 hc-github-team-consul-core

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

hc-github-team-consul-core avatar Jul 31 '23 12:07 hc-github-team-consul-core

Thank you for reviewing and merging, much appreciated 👍

On Mon, Jul 31, 2023, at 14:11, Chris S. Kim wrote:

Merged #13023 https://github.com/hashicorp/consul/pull/13023 into main.

— Reply to this email directly, view it on GitHub https://github.com/hashicorp/consul/pull/13023#event-9963098227, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAT5C42ATCE52NUBCRSYRTXS6OFTANCNFSM5VVLSYIQ. You are receiving this because you were mentioned.Message ID: @.***>

apollo13 avatar Jul 31 '23 16:07 apollo13