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

Add a Metadata protobuf module #123

Merged
merged 8 commits into from
Jul 6, 2022
Merged

Add a Metadata protobuf module #123

merged 8 commits into from
Jul 6, 2022

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Jul 6, 2022

As we introduce HTTP routes (for inbound policy), we need the control
plane to describe resources references for metrics, and other
diagonstics in the proxy. Currently, the control plane uses a free-form
map of strings, but the proxy only references a specific set of keys,
which is not clearly documented via the gRPC API.

This change replaces the Authz type's labels map with a Metadata
type. Controllers should set both fields for at least one stable
version.

Signed-off-by: Oliver Gould ver@buoyant.io

As we introduce HTTP routes (for inbound policy), we need the control
plane to describe resources references for metrics, and other
diagonstics in the proxy. Currently, the control plane uses a free-form
map of strings, but the proxy only references a specific set of keys,
which is not clearly documented via the gRPC API.

This change replaces the `Authz` type's `labels` map with a `Metadata`
type. Controllers should set both fields for at least one stable
version.

Signed-off-by: Oliver Gould <ver@buoyant.io>
@olix0r olix0r requested a review from a team July 6, 2022 20:58
@hawkw
Copy link
Contributor

hawkw commented Jul 6, 2022

looks like the CI build is broken due to missing imports...but only for docs? seems weird: https://github.com/linkerd/linkerd2-proxy-api/runs/7222796039?check_suite_focus=true

Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

some docs nits, and i wasn't super clear on the way the default variant for Metadata is supposed to work. it would be nice to have a bit more of a description of that in the comments. other than that, lgtm!

Comment on lines +7 to +8
// General metadata about a configuration object. Typically references either an
// implicit default configuration or a Kubernetes resource.
Copy link
Contributor

Choose a reason for hiding this comment

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

the semantics of the default variant is not super clear from this...is it expected to be populated with a string? how is that string handled?

Copy link
Member

Choose a reason for hiding this comment

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

based on the current behavior of the policy-controller, I think this is supposed to describe what the default policy is (e.g. "all-authenticated"). But I agree, it's not clear.

proto/inbound.proto Outdated Show resolved Hide resolved
proto/inbound.proto Outdated Show resolved Hide resolved
olix0r added 4 commits July 6, 2022 22:19
Signed-off-by: Oliver Gould <ver@buoyant.io>
Signed-off-by: Oliver Gould <ver@buoyant.io>
Signed-off-by: Oliver Gould <ver@buoyant.io>
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

one last typo, but this seems good now

proto/meta.proto Show resolved Hide resolved
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Oliver Gould <ver@buoyant.io>
@olix0r olix0r merged commit 7ec7798 into main Jul 6, 2022
@olix0r olix0r deleted the ver/meta branch July 6, 2022 22:48
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