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

update protocol detection doc #1848

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

update protocol detection doc #1848

wants to merge 8 commits into from

Conversation

wmorgan
Copy link
Member

@wmorgan wmorgan commented Sep 29, 2024

  • Clean up and clarify various bits of language
  • Add NATS to the list of protocols that need opaque ports
  • Instruct that the resource should always be set on the Service resource

In the interests of simplicity, I kept the guidance around setting the annotation at the Service resource relatively simple.

This content probably needs to be moved into a separate Reference doc. I did not do that in this doc.

- Add NATS to the list of protocols that need opaque ports (not sure how this
  was dropped)
- Instruct that the resource should always be set on the Service object.

Signed-off-by: William Morgan <[email protected]>
@wmorgan wmorgan requested a review from a team September 29, 2024 13:46
Signed-off-by: William Morgan <[email protected]>
@olix0r olix0r requested a review from cratelyn October 4, 2024 16:06
Copy link
Collaborator

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

🌮 lovely work @wmorgan, thank you! i left some minor notes and suggestions, but broadly, i'm very happy to see these docs gain some additional clarity.

linkerd.io/content/2.16/features/protocol-detection.md Outdated Show resolved Hide resolved
linkerd.io/content/2.16/features/protocol-detection.md Outdated Show resolved Hide resolved
linkerd.io/content/2.16/features/protocol-detection.md Outdated Show resolved Hide resolved
1. In an [authorization policy](../server-policy/) `Server` object's
`proxyProtocol` field, in which case it will apply to all pods targeted by that
`Server`.
This annotation *must* be set in two places:
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants