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

feat(proto-detect): Convert opaque ports to app protocol #13659

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

sfleen
Copy link
Contributor

@sfleen sfleen commented Feb 14, 2025

Currently, we store whether or not a port in a parent is opaque with a simple boolean. This works, but is somewhat restrictive if we want to specifiy different port protocols in the resource definition.

This converts it to a proper AppProtocol enum, which will allow us to use different protocol specifications for ports in the future.

@sfleen sfleen requested a review from a team as a code owner February 14, 2025 19:35
@sfleen sfleen force-pushed the protocol-detect-2 branch 2 times, most recently from e23fd8b to b412a89 Compare February 14, 2025 21:06
Copy link
Member

@olix0r olix0r left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! A few small things:

jaeger/charts/linkerd-jaeger/values.yaml Outdated Show resolved Hide resolved
policy-controller/core/src/outbound.rs Outdated Show resolved Hide resolved
policy-controller/grpc/src/outbound.rs Outdated Show resolved Hide resolved
@sfleen sfleen force-pushed the protocol-detect-2 branch 2 times, most recently from 32afa9d to 3ebe410 Compare February 17, 2025 14:53
@olix0r olix0r requested a review from adleong February 17, 2025 16:42
@olix0r
Copy link
Member

olix0r commented Feb 17, 2025

This looks good to me. I'd like @adleong to take a look before merging so we're in sync as the followup PRs come into view.

@olix0r
Copy link
Member

olix0r commented Feb 17, 2025

When merging, the subject should probably be feat(policy): ... for consistency.

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.

Food for thought, feel free to ignore:

Is it redundant that we store and Option and AppProtocol has an Unknown variant? Is there a semantic difference between None and Some(Unknown)?

@sfleen
Copy link
Contributor Author

sfleen commented Feb 18, 2025

There is a difference, None we'd want to do protocol detection and Some(Unknown) we'd want to handle as opaque. Plus logging/diagnostics for unknown protocols.

Currently, we store whether or not a port in a parent is opaque with a simple boolean. This works, but is somewhat restrictive if we want to specifiy different port protocols in the resource definition.

This converts it to a proper `AppProtocol` enum, which will allow us to use different protocol specifications for ports in the future.

Signed-off-by: Scott Fleener <[email protected]>
@olix0r olix0r merged commit a04b83a into linkerd:main Feb 18, 2025
42 checks passed
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.

3 participants