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

docs: support for peer label in DNS lookups #15374

Closed
wants to merge 6 commits into from

Conversation

jkirschner-hashicorp
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp commented Nov 15, 2022

Ready for education team review at this preview link

@jkirschner-hashicorp jkirschner-hashicorp added type/docs Documentation needs to be created/updated/clarified pr/no-changelog PR does not need a corresponding .changelog entry backport-inactive/1.14 This release series is no longer active labels Nov 15, 2022
@jkirschner-hashicorp jkirschner-hashicorp requested a review from a team as a code owner November 15, 2022 13:24
- Lookup a service, optionally in a cluster peer:

```text
[<tag>.]<service>.service[.<namespace>.ns][.<peer>.peer][.<partition>.ap].<domain>
Copy link
Member

Choose a reason for hiding this comment

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

We don't currently support this format with .service.p1.peer in 1.14.0, so we'll need to remove anything referring to it. I believe we'll only be able to use the new .peer syntax with virtual entries for the time-being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - I'll move that stuff out into another PR ...

... once the accuracy of some of the other content is reviewed (so I can batch all my changes).

I'm particularly not sure about the changes I made to the ### Service Virtual IP Lookups section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: .peer support varies by service lookup type and version.

.virtual has supported as of 1.14.0.
.service and .node have support as of 1.14.2.

.ingress and .connect lack support.

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Left some comments on what to keep as part of this and what's for the future!

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
Consul assigns a virtual IP to each service in the service mesh.
A downstream service in the mesh must address an upstream service
using its virtual IP in the following scenarios:
- The upstream service is imported from a cluster peer
Copy link
Contributor

Choose a reason for hiding this comment

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

You can still address an upstream service imported from a cluster peer using explicit upstreams. In that case you wouldn't need to use the virtual IP. So the only time we must address an upstream using its virtual ip is when the downstream has tproxy enabled.

"The peer name is an optional part of the FQDN, and it is used to query for the virtual IP
of a service imported from that peer." <- i think this sentence is still accurate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ndhanushkodi : So if using explicit upstreams, you can use either a .virtual or a .service lookup? I guess I'm still confused about when you'd need to use a .virtual lookup versus just using a .service lookup all the time (as of 1.14.2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the only time we need .virtual "when the downstream has tproxy enabled"? And in all other cases, use .service?

Copy link
Member

Choose a reason for hiding this comment

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

@jkirschner-hashicorp If a downstream application is using an explicit upstream, it would be communicating to localhost:<port>, and would not be using the IP address returned from a Consul DNS lookup.

Accessing a virtual address requires the use of tproxy. The VIPs are allocated from an IP range that is not likely to be present on a user's internal network, and thus are not routable by the upstream network. The application's sidecar needs to have tproxy enabled so that Envoy is correctly configured to map the destination [V]IP to the correct upstream service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a downstream application is using an explicit upstream, it would be communicating to localhost:, and would not be using the IP address returned from a Consul DNS lookup.

@blake : I don't think I had ever considered how namespace, partition, and peer are specified in an explicit upstream. Somehow I had thought that format would use a DNS-style lookup, but it doesn't. It uses proxy.upstreams[].destination_* fields.

I think I understand now:

  • For mesh:
    • Implicit upstreams with transparent proxy must use the .virtual format (if using Consul DNS format rather than Kube DNS format)
    • DNS formats doesn't come into the picture at all for explicit upstreams
  • For service discovery, use .service lookups.

Side note-to-self: We don't document destination_peer (if there is such a field) in the upstream config reference: https://developer.hashicorp.com/consul/docs/connect/registration/service-registration#upstream-configuration-reference

It also seems like there's duplicate content between https://developer.hashicorp.com/consul/docs/connect/registration/sidecar-service and the link above that can lead to inconsistencies, like the omission of destination namespace and partition in one place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though, use of this .virtual format requires that dns.enableRedirection is set to true, right?

Copy link
Member

Choose a reason for hiding this comment

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

We don't document destination_peer (if there is such a field) in the upstream config reference: https://developer.hashicorp.com/consul/docs/connect/registration/service-registration#upstream-configuration-reference

This field is documented on that page. At the time of writing this comment, it is documented in the third row in the table of supported parameters under Upstream configuration reference.

It also seems like there's duplicate content between https://developer.hashicorp.com/consul/docs/connect/registration/sidecar-service…

This page shows the default config options used by Consul when a service registration has Connect enabled using the minimal "connect": { "sidecar_service": {} } config stanza. It also shows an example of how these parameters can be overridden with minimal additional configuration.

The /docs/connect/registration/service-registration contains an exhaustive list of all parameters supported for a proxy service registration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This field is documented on that page. At the time of writing this comment, it is documented in the third row in the table of supported parameters under Upstream configuration reference.

Ugh, didn't see that. Thanks!

@@ -444,33 +473,46 @@ If you need more complex behavior, please use the

### Service Virtual IP Lookups

To find the unique virtual IP allocated for a service:
To find the unique virtual IP allocated for a [Connect-capable](/docs/connect) service:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self before resubmitting PR: add cross-references to when to use virtual IP lookups back in

@jkirschner-hashicorp
Copy link
Contributor Author

@im2nguyen, @trujillo-adam: This is finally ready for education team review!

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Left some comments and feedback. Let me know when you want me to have another look.

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
Comment on lines 213 to 215
RFC 2782 style lookups have the same behavior as
[standard service lookups](#standard-lookup),
but differ in format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RFC 2782 style lookups have the same behavior as
[standard service lookups](#standard-lookup),
but differ in format.
You can use RFC 2782-style lookups to query for services.

We don't really describe the behavior of standard lookups--just gave some examples of how to format the lookups--so there isn't really anything to compare it to. Even if there were details about the behavior, it would be better to just state the behavior of this lookup format here.


The lookup uses the datacenter of the Consul agent acting as the DNS server.

- Lookup a service in a cluster peer:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Lookup a service in a cluster peer:
- Add the `peer` segment to lookup a service in a cluster peer:

Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly. They need to specify two labels. If the peer name is "foo", they'd have to add foo.peer.

At a higher level, I'm wondering about the benefit of changing this text from "context in which the format is useful" to "actions you need to take to use the format below". It feels strange to me to have one of the "valid formats for standard service lookups" be titled "lookup a local service" and the next be in a different form: "add the peer segment to lookup a service in a cluster peer".

Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer that we describe what users are supposed to do in each of these rather than leaving it for them to figure it out based on the snippet we're providing. That is my reason for these suggestions -- we're giving the what without the how.

[<tag>.]<service>.service.<peer>.peer.<domain>
```

- Lookup a service in a WAN federated datacenter:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Lookup a service in a WAN federated datacenter:
- Specify the name of a datacenter to lookup a service in a WAN federated datacenter:

WAN federated datacenter, or cluster peer in service lookups.
In each format, all fields in square brackets are optional.

- Lookup a service, optionally in a cluster peer:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Lookup a service, optionally in a cluster peer:
- Lookup a service in a cluster peer:

I was a little confused by the word "optionally" in this sentence. The syntax shows the peer segment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything in square brackets is optional, as stated in Line 412:

In each format, all fields in square brackets are optional.

If you don't include .peer-name.peer, then it does a local lookup. Partition and namespace are also optional.

Therefore, I'm inclined to decline this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think this line makes sense. The explanation actually confirms that we should change it because we already state in the introduction that the peer is optional.

Comment on lines 420 to 422
This format supports `.service` lookup types in Consul 1.14.2 or later
and supports `.virtual` lookup types in Consul 1.14.0 or later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This format supports `.service` lookup types in Consul 1.14.2 or later
and supports `.virtual` lookup types in Consul 1.14.0 or later.
~> **Compatibility warning**: Consul 1.14.2+ is required for `.service` lookup types.

We should only call out the minor version where the change was introduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to mention somewhere which lookup types are supported by each format. This suggested change removes mention that .virtual lookup types are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll mention that .service and .virtual types are supported above the compatibility warning.

Comment on lines 423 to 434
The `peer` is a cluster peer of the `partition`.
If no `peer` is specified, the service lookup applies to the specified `partition`
rather than one of its cluster peers.

For example, assume that an agent in the `default` partition is acting as the
DNS server, the `k8s-app-1` partition is in the same datacenter, and
the `k8s-app-1` partition has a cluster peer named `billing`.
To perform a lookup of non-mesh service `api` in namespace `foo`
of that cluster peer, use the query
`api.service.foo.ns.billing.peer.k8s-app-1.partition.consul`.

- Lookup a service, optionally in a WAN federated datacenter:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `peer` is a cluster peer of the `partition`.
If no `peer` is specified, the service lookup applies to the specified `partition`
rather than one of its cluster peers.
For example, assume that an agent in the `default` partition is acting as the
DNS server, the `k8s-app-1` partition is in the same datacenter, and
the `k8s-app-1` partition has a cluster peer named `billing`.
To perform a lookup of non-mesh service `api` in namespace `foo`
of that cluster peer, use the query
`api.service.foo.ns.billing.peer.k8s-app-1.partition.consul`.
- Lookup a service, optionally in a WAN federated datacenter:
Specify the name of a peered cluster in the `peer` segment.
Specify the name of the admin partition in the cluster in the `partition` segment.
If you do not include a `peer`, the service lookup queries the `partition` instead of one of the peered clusters.
In the following example, a Consul agent in the `default` partition acts as the DNS server, the `k8s-app-1` partition is in the same datacenter, and
the `k8s-app-1` partition has a cluster peer named `billing`.
To perform a lookup of non-mesh service `api` in namespace `foo`
of that cluster peer, use the query
`api.service.foo.ns.billing.peer.k8s-app-1.partition.consul`.
- Lookup a service in a WAN federated datacenter:

Did I restate that correctly? The original text is a little unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took your 2nd paragraph, but changed the first paragraph to the following:

If `.<peer>.peer` is included, the `<peer>` label identifies a cluster peer
of the query's partition in which to lookup the specified `<service>`.
If the query does not specify a partition,
the query uses the partition of the Consul agent that received the DNS query.

I agree that the original text wasn't clear. Let me know how much this change helps (or not)!

website/content/docs/discovery/dns.mdx Outdated Show resolved Hide resolved
Comment on lines 443 to 445
Cluster peering must be used to connect datacenters containing admin partitions,
not WAN federation. Therefore, this format is not applicable to lookup a partition
in a different datacenter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Cluster peering must be used to connect datacenters containing admin partitions,
not WAN federation. Therefore, this format is not applicable to lookup a partition
in a different datacenter.
You must establish a peer connection between your Consul clusters to connect datacenters containing admin partitions. You cannot query an admin partition in another datacenter over a WAN federated network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The limitation is not on connection / querying. WAN federation and admin partitions are mutually exclusive. If two datacenters are WAN federated, they can't have (non-default) admin partitions.

I'm not sure if that nuance is important here. I'll have to think about whether there's a clearer way than the original text...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New suggestion:

Suggested change
Cluster peering must be used to connect datacenters containing admin partitions,
not WAN federation. Therefore, this format is not applicable to lookup a partition
in a different datacenter.
This format does not support specifying both a partition and another datacenter.
Datacenters containing admin partitions can only be connected using cluster peering,
not WAN federation. To lookup a service in a partition of another datacenter,
you must establish a cluster peering relationship with that partition and
use the cluster peer lookup format.

@jkirschner-hashicorp
Copy link
Contributor Author

@trujillo-adam : I considered all of the feedback. In a few cases, I decided not to make the suggested changes (and left a comment with my thinking). Let me know your follow-up thoughts - it's ready for review again. Thanks!

Copy link
Contributor

@trujillo-adam trujillo-adam left a comment

Choose a reason for hiding this comment

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

Added a few comments and suggestions


The lookup uses the datacenter of the Consul agent acting as the DNS server.

- Lookup a service in a cluster peer:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still prefer that we describe what users are supposed to do in each of these rather than leaving it for them to figure it out based on the snippet we're providing. That is my reason for these suggestions -- we're giving the what without the how.

Comment on lines +223 to +226
The RFC 2782 lookup format supports the same suffixes as standard lookups
for optionally specifying a cluster peer or WAN federated datacenter,
but differs in how service name and optional tag are specified.
The valid RFC 2782 lookup formats below assume a local service lookup:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The RFC 2782 lookup format supports the same suffixes as standard lookups
for optionally specifying a cluster peer or WAN federated datacenter,
but differs in how service name and optional tag are specified.
The valid RFC 2782 lookup formats below assume a local service lookup:
The RFC 2782 lookup format supports the same suffixes as standard lookups for specifying cluster peers or WAN federated datacenters, but it follows a different format for specifying service names and tags.
The following RFC 2782 lookups show how to format queries for a local service:

Switch to active voice
I have the same comment as above w/r/t relying on users to recognize the differences by looking at the snippet rather than helping them with a description.

Comment on lines +243 to +245
Per [RFC 2782](https://tools.ietf.org/html/rfc2782), SRV queries must
prepend an underscore (`_`) to the RFC 2782 `service` and `protocol` values
in a query to prevent DNS collisions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Per [RFC 2782](https://tools.ietf.org/html/rfc2782), SRV queries must
prepend an underscore (`_`) to the RFC 2782 `service` and `protocol` values
in a query to prevent DNS collisions.
Per [RFC 2782](https://tools.ietf.org/html/rfc2782), SRV queries must
prepend an underscore (`_`) to the RFC 2782 `service` and `protocol` values
in a query to prevent DNS collisions.

Ah, this is the explanation. Consider moving this into the opening paragraph just before we introduce the example formats.

WAN federated datacenter, or cluster peer in service lookups.
In each format, all fields in square brackets are optional.

- Lookup a service, optionally in a cluster peer:
Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't think this line makes sense. The explanation actually confirms that we should change it because we already state in the introduction that the peer is optional.

Comment on lines +438 to +439
If `.<peer>.peer` is included, the `<peer>` label identifies a cluster peer
of the query's partition in which to lookup the specified `<service>`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If `.<peer>.peer` is included, the `<peer>` label identifies a cluster peer
of the query's partition in which to lookup the specified `<service>`.
A lookups that contains the `.<peer>.peer` label searches for `<service>` in a cluster connected to the query's partition with a [peer connection](/consul/docs/connect/cluster-peering) .

Feel free to ignore this one. I was trying to use active voice and more precise language, but this might be more convoluted.

Comment on lines +460 to +461
Datacenters containing admin partitions can only be connected using cluster peering,
not WAN federation. To lookup a service in a partition of another datacenter,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Datacenters containing admin partitions can only be connected using cluster peering,
not WAN federation. To lookup a service in a partition of another datacenter,
You can only use peer connections to join datacenters that contain admin partitions, not WAN federation. To lookup a service in a partition of another datacenter,

Active voice

Comment on lines +500 to +501
To lookup a namespaced service in a cluster peer,
the [canonical format](#canonical-format) must be used instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
To lookup a namespaced service in a cluster peer,
the [canonical format](#canonical-format) must be used instead.
Use the [canonical format](#canonical-format) to lookup services bound to a namespace in a cluster peer.

It's a bit of a gray area, but I think "namespace" is a noun. I recall seeing it in the K8s docs as verb, but it read as poor grammar to me. "Bound" might not be the right verb, either. I won't die on this hill if you disagree.

Comment on lines +581 to +582
Use the following query formats to perform a service virtual IP lookup
on namespaced or partitioned services:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use the following query formats to perform a service virtual IP lookup
on namespaced or partitioned services:
Use the following query formats to perform a service virtual IP lookup
on services within a namespace or partition:

Same comment as above about parts of speech w/r/t namespace (and partition).

Comment on lines +584 to +591
- Use the [canonical format](#canonical-format) to optionally specify
namespace, peer, and/or partition:

```text
<service>.virtual[.<namespace>.ns][.<peer>.peer][.<partition>.ap].<domain>
```

- Specify cluster peer and optional namespace in the lookup:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Use the [canonical format](#canonical-format) to optionally specify
namespace, peer, and/or partition:
```text
<service>.virtual[.<namespace>.ns][.<peer>.peer][.<partition>.ap].<domain>
```
- Specify cluster peer and optional namespace in the lookup:
- Use the [canonical format](#canonical-format) to specify
namespace, peer, and/or partition:
```text
<service>.virtual[.<namespace>.ns][.<peer>.peer][.<partition>.ap].<domain>
  • Specify cluster peer and namespace in the lookup:
Do we need to keep prefacing as these parts as optional?

@github-actions
Copy link

This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.

@github-actions github-actions bot added the meta/stale Automatically flagged for inactivity by stalebot label Jun 23, 2023
Copy link

github-actions bot commented Jun 5, 2024

Closing due to inactivity. If you feel this was a mistake or you wish to re-open at any time in the future, please leave a comment and it will be re-surfaced for the maintainers to review.

@github-actions github-actions bot closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta/stale Automatically flagged for inactivity by stalebot pr/no-changelog PR does not need a corresponding .changelog entry type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants