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

service-mesh: blackhole traffic destined for the TPROXY port #1171

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

3u13r
Copy link
Member

@3u13r 3u13r commented Jan 21, 2025

  • blackhole traffic that would cause the envoy to infinitely connect to itself
  • add tests for the envoy config

@3u13r 3u13r force-pushed the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch from ac085b5 to 47b9850 Compare January 21, 2025 03:56
@3u13r 3u13r changed the title service-mesh: blackhole traffic destined for the TROXY port service-mesh: blackhole traffic destined for the TPROXY port Jan 21, 2025
@3u13r 3u13r force-pushed the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch 2 times, most recently from b62d878 to 85a7740 Compare January 21, 2025 12:51
@3u13r 3u13r added the bug fix Fixing a user facing bug label Jan 21, 2025
@3u13r 3u13r force-pushed the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch from 85a7740 to 5a6afdd Compare January 21, 2025 12:53
@katexochen katexochen added this to the v1.4.0 milestone Jan 21, 2025
@3u13r 3u13r marked this pull request as ready for review January 21, 2025 13:10
@3u13r 3u13r requested a review from katexochen as a code owner January 21, 2025 13:10
@burgerdev burgerdev self-requested a review January 21, 2025 13:55
service-mesh/config.go Outdated Show resolved Hide resolved
service-mesh/config.go Outdated Show resolved Hide resolved
service-mesh/config.go Outdated Show resolved Hide resolved
service-mesh/config.go Outdated Show resolved Hide resolved
func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you introduce this for a specific reason? This binary looks pretty serial...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I thought we would just add this per convention for new tests, since e.g.,

func TestMain(m *testing.M) {
also looks pretty serial. It looks like at least Malte was of the same opinion, since all the times where this was also "unnecessarily" included are tests he wrote. But I don't have any (strong) opinion about this, does anyone else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, was just curious.

@3u13r 3u13r force-pushed the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch from 5a6afdd to 68a224f Compare January 27, 2025 02:13
3u13r added 3 commits January 27, 2025 03:14
Traffic to the TPROXY port (15006/15007) led to a traffic storm as envoy used the original destination to forward the traffic to, therefore forwarding it again to the TPROXY port where envoy listens.

This commit introduces a Blackhole cluster where we send traffic to, that arrives on the TPROXY listeners and which original destination port is the TPROXY.
With increasing envoy config complexity it gets more difficult to parse the final envoy config.
Therefore we introduce tests which compare the envoy config for specific scenarios with golden JSON representations of the expected output.
@3u13r 3u13r force-pushed the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch from 68a224f to e938b27 Compare January 27, 2025 02:50
@3u13r 3u13r requested a review from burgerdev January 27, 2025 03:05
@3u13r 3u13r merged commit a7f7e30 into main Jan 27, 2025
14 checks passed
@3u13r 3u13r deleted the euler/fix/service-mesh/blackhole-direct-tproxy-traffic branch January 27, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Fixing a user facing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants