Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
update protocol detection doc #1848
base: main
Are you sure you want to change the base?
update protocol detection doc #1848
Changes from 6 commits
cc98d98
c7bbafc
6f55e6d
77c713b
5c493b2
769d7e1
e068f76
65fb2e0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, this is a big semantic change! Is this really true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when clients discover policies for a service clusterIP and port, we need to be able to know opaqueness at the service level. If the service is a headless, the service-level annotation will be ignored (because discovery cannot be for the clusterIP), but the general advice to annotate services is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, the old version of this doc says that you can set
config.linkerd.io/opaque-ports
on either the Service or the destination pods/namespace, but this version says that you must set it in both places. That's the semantic change I'm asking about.It makes a lot of sense to me that there are cases that you'd want to annotate the Service; it's very surprising to me if we're changing to saying that you must annotate both the Service and the destination pods/namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also required to configure the workloads (via Server or annotation) to control inbound behavior when clients are unmeshed or do not target a clusterIP service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically there are situations where one or the other suffice, and thus the "must" is not strictly necessary. But rather than describing the nuances of those situations I opted to just say, you must set it in both places. If you think that's overkill I'm not committed to the choice, so feel free to suggest other language and we can iterate.