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

Support for resources opting-out of tap #2807

Merged
merged 3 commits into from
May 10, 2019
Merged

Support for resources opting-out of tap #2807

merged 3 commits into from
May 10, 2019

Conversation

alpeb
Copy link
Member

@alpeb alpeb commented May 9, 2019

Fixes #2778

This one is just for the control-plane side of things.
The env var injected into the sidecar proxy was labeled LINKERD2_PROXY_TAP_DISABLED.

Fixes #2778

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented May 10, 2019

Integration test results for ecd598f: success 🎉
Log output: https://gist.github.com/2c79a6be1e5cf28ab34a5890d958ddea

@siggy siggy added this to In progress in 2.4 - Debt via automation May 10, 2019
2.4 - Debt automation moved this from In progress to Reviewer approved May 10, 2019
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

looks good! left a couple tioli comments. 🚢 👍

flags.BoolVar(
&options.disableTap, "disable-tap", options.disableTap,
"Disables resources from from being tapped",
)
Copy link
Member

Choose a reason for hiding this comment

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

until the proxy supports it, should we do flags.MarkHidden("disable-tap") ? maybe reference a Github issue in a comment for the proxy change? or, since the control-plane is also filtering, maybe this is ok?

related: does this mean both the control-plane and the proxy will independently guard against tapping?

Copy link
Member Author

Choose a reason for hiding this comment

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

As is, tapping gets disabled through the control plane API, but that doesn't disallow someone hitting directly the proxy. I think it makes sense to mark the flag hidden in the meantime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #2811 to track proxy-side work

controller/tap/server.go Outdated Show resolved Hide resolved
test/tap/tap_test.go Show resolved Hide resolved
alpeb added 2 commits May 10, 2019 10:20
…tappable

Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented May 10, 2019

Integration test results for a6d3528: success 🎉
Log output: https://gist.github.com/68d1add85ceee6873863d70df707b0d8

Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

⭐ very nice 🚫 👞

@alpeb alpeb merged commit 065c221 into master May 10, 2019
2.4 - Debt automation moved this from Reviewer approved to Done May 10, 2019
@alpeb alpeb deleted the alpeb/tap-disabling branch May 10, 2019 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.4 - Debt
  
Done
Development

Successfully merging this pull request may close these issues.

Add inject flag for disabling tap
4 participants