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

Send the Namespace/Partition as gRPC metadata headers in the request to the Consul DNS service. #172

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jun 22, 2023

Description

By passing along the tenant of the service being proxied we can more intelligently select the correct partition/namespace to use as defaults for DNS.

This also passes along the Consul ACL token as the x-consul-token grpc metadata. Eventually Consul should be able to use this token instead of the servers default token for authorizing access.

TODO

  • Tests
  • Support for handling this metadata in Consul (other PRs are in the works for that)
  • A review of whether this was the best way to get the tenancy information to the DNS server

mkeeler added 2 commits March 26, 2024 16:29
…ervice

This can be used to enable authenticated DNS and prevent needing to set the default token on the servers.
@DanStough DanStough marked this pull request as ready for review April 1, 2024 15:03
@DanStough DanStough requested a review from a team as a code owner April 1, 2024 15:03
@DanStough DanStough requested a review from jmurret April 1, 2024 15:04
Comment on lines +57 to +63
// store the final resolved service for others to use.
cdp.resolvedProxyConfig = ProxyConfig{
NodeName: rsp.NodeName,
ProxyID: cdp.cfg.Proxy.ProxyID,
Namespace: rsp.Namespace,
Partition: rsp.Partition,
}
Copy link
Member

@nathancoleman nathancoleman Apr 1, 2024

Choose a reason for hiding this comment

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

Would you expect that the node, namespace + partition would sometimes differ between rsp and cdp.cfg.Proxy? I ask because I was curious why we can't use cfg.cfg.Proxy directly vs. hanging an additional ProxyConfig on the struct

Copy link
Contributor

Choose a reason for hiding this comment

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

Matt might know differently, but I assumed it was that the serviceaccount/Token used to make the bootstrapping could imply a namespace/partition that wasn't specified using the config flags.

Copy link
Member

@jmurret jmurret 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. 👏

@DanStough DanStough merged commit 5e77798 into main Apr 19, 2024
38 checks passed
@DanStough DanStough deleted the dns-tenancy branch April 19, 2024 15:11
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