Vhds readme by brian-avery · Pull Request #7415 · envoyproxy/envoy

brian-avery · 2019-06-27T18:11:40Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Adds documentation for on-demand VHDS/RDS
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

mattklein123 · 2019-06-27T18:22:30Z

Drive by: Can this instead be in the public docs vs. a markdown file? We should be talking about this probably in these locations:
https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/operations/dynamic_configuration
https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol (maybe if anything is different here)
https://www.envoyproxy.io/docs/envoy/latest/configuration/http_conn_man/rds

Thank you!

lambdai · 2019-06-27T18:23:17Z

BTW: IIRC we are moving docs to docs/root/ with .rst instead of .md. I could be wrong.

htuch

Thanks. Agree with the other comments that this probably belongs as a peer or nested documented with https://www.envoyproxy.io/docs/envoy/latest/api-docs/xds_protocol.
/wait

mattklein123 · 2019-07-08T16:49:56Z

High level comment. Can we please add references to VHDS from the other places that I mentioned? There should be docs in the HTTP arch overview, etc. Basically I think anywhere we mention scoped RDS we should also mention this. Thank you.

/wait

htuch

Thanks, a bunch of other feedback.
/wait

htuch · 2019-07-09T20:21:56Z

@AndresGuedez to review this section.

stale · 2019-07-16T21:03:33Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

AndresGuedez

Reviewed SRDS section and left comments primarily related to adding context and clarifying implications.

AndresGuedez · 2019-07-22T19:02:52Z

Both approaches

It's unclear which approaches this is referencing.

I agree VHDS doesn't conflict with SRDS; some :ref: pointers to SRDS documentation would be helpful to provide context.

I think it's worth pointing out in this section that using SRDS + RDS + VHDS means that two on-demand subscriptions per routing scope will be required; e.g., when a new ScopedRouteConfiguration is distributed via SRDS, an RDS subscription for the associated RouteConfiguration will be initiated, which may itself trigger a VHDS subscription.

stale · 2019-07-29T19:21:53Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2019-08-05T19:50:33Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

brian-avery · 2019-08-05T19:51:25Z

/reopen

mattklein123 · 2019-10-19T23:08:15Z

Closing, we can merge this into #8617

mattklein123 · 2019-10-24T18:53:06Z

Reopening as it seems like we are going to finish this out. This should include arch docs, detailed config docs, etc. please look at all the places RDS, SRDS, routing, etc. are mentioned and make sure include VHDS also. Thanks!

dmitri-d · 2019-10-24T18:58:40Z

What's the use-case for this? There currently isn't a way for Envoy to trigger an unsubscribe for a vhost.

Perhaps @htuch can answer this...

Signed-off-by: Brian Avery <bavery@redhat.com>

markdroth · 2019-12-04T16:35:17Z

+the active stream is paused while a :ref:`DeltaDiscoveryRequest <envoy_api_msg_DeltaDiscoveryRequest>` is sent. 
+When a :ref:`DeltaDiscoveryResponse <envoy_api_msg_DeltaDiscoveryResponse>` is
+recieved where the host/authority header values exactly matches the 
+:ref:`aliases <envoy_api_field_resource.aliases>`,


The name and aliases fields will have values in the form <route configuration name>/<host entry>, right? Those are the names that are used in the VHDS request, so that's what the resources have to be named in order to match.

I think the check here should be that the name or aliases fields match the route configuration name and host/authority header value in the format specified above.

markdroth · 2019-12-04T16:37:57Z

+:ref:`DeltaDiscoveryResponse <envoy_api_msg_DeltaDiscoveryResponse>` in which
+the :ref:`resources <envoy_api_field_DeltaDiscoveryResponse.resources>` entry
+for that virtual host has the :ref:`name <envoy_api_field_resource.name>` and
+:ref:`aliases <envoy_api_field_resource.aliases>` set to the requested host


In this case, it seems like it should set name but would not need to set aliases.

If the virtual host cannot be resolved, you know an alias that that virtual host could go by, but you don't the name of that virtual host since there is no virtual host.

I think there should be no difference in content or semantics between the name and aliases fields -- if we didn't care about backward-compatibility, we could just change name to be a repeated field instead of adding the aliases field. But even though they're split, the set of all values in both fields should be treated as a single list: all names in the list should be unique for the resource type, and a query for any one of the names should return the same object.

In this case, there is exactly one name that the client will be looking for. So what's the value of putting that in the aliases field instead of in the name field? And if we do put it in the aliases field, what will we put into the name field instead?

Every virtual host has a definitive name that populates the name field and aliases are other names that that resource can go by. I'm not sure that we want to mix the meanings of the names and aliases.

We also care about backwards compatibility of xDS, so I'm not sure we can change the API to combine names and aliases.

A resource can have any name that the management server wants to assign it and Envoy doesn't care. However, it does care about the contents of the alias field. In the case of a VHDS resource not being found, we know that the requested resource can go by a name matching /, but we don't know what name the management server assigned that resource since that resource could not be found.

Every virtual host has a definitive name that populates the name field and aliases are other names that that resource can go by. I'm not sure that we want to mix the meanings of the names and aliases.

Why would the meanings be different? Are there any use-cases in which the client would care about the difference between them?

We also care about backwards compatibility of xDS, so I'm not sure we can change the API to combine names and aliases.

This PR is exposing the aliases fields in the Resource message, which was not previously exposed, so we can define its semantics to be anything we want. Why would this break backward compatibility any more than introducing the aliases field in the first place?

A resource can have any name that the management server wants to assign it and Envoy doesn't care. However, it does care about the contents of the alias field. In the case of a VHDS resource not being found, we know that the requested resource can go by a name matching /, but we don't know what name the management server assigned that resource since that resource could not be found.

Envoy may not care about the name, but it's not true that no client cares. The gRPC xDS client does in fact care about resource names; for example, in LDS and CDS requests, gRPC clients make non-wildcard requests, but for backward compatibility, it cannot assume that the server will honor the resource_names field in the request, so it looks for the specific resource name(s) that it asked for.

I think the right way to think about this case is that, because there is no match for the resource name that the client requested, the server is going to return the name that the client requested with no attached resource. This tells the client that the requested resource does not exist.

Regardless, this is how it's implemented. Based on this conversation, it seems that @htuch @dmitri-d and myself are in agreement that this is good for now. Can we address this after this is merged in a separate from this PR?

Sure, let's take #9317 and see where this leads us. I feel that without another on-demand protocol, we don't have much in the way of examples to illustrate the utility of a canonical name, we can see how this plays out in v2 xDS to feed into UDPA.

I don't think that we can both accept #9317 and say that VHDS detects unknown virtual hosts based on the aliases field instead of the name field. This is basically a contradiction. It would mean that in practice, every management server would need to do something in violation of the spec.

I think we have two choices here:

Fix VHDS to check the name field instead of aliases field, and update this PR accordingly.

Change Document incremental xDS semantics about resources that do not exist. #9317 to say that clients need to be prepared to match unknown resources on both the name and aliases fields.

I would prefer option (1), since option (2) would impose additional requirements on all client implementations without actually adding any value.

The code is already implemented as described in this doc. 9317 is proposing to change that. Whether that's right or wrong, I'm not sure. However, what's documented here is current functionality and agreed upon by @htuch @dmitri-d and myself. I'm proposing pushing protocol and docs changes to the other PR and we can update if we add these changes if we need to.

It seems #9317 is a very recent PR and an attempt to change the requirements for work which has already been done and agreed to, this should ideally be handled through other PRs after this work is committed. IMHO this has been an enormous task and we need to draw a line under it so we can move forward with it being the basis for future changes.

markdroth · 2019-12-04T16:38:52Z

+
+.. _on_demand_vhds_rds_protocol:
+
+Virtual Host Discovery Service


This looks really good to me, but I'll let @htuch make the final call on this.

markdroth · 2019-12-04T16:45:33Z

+entry and the resource unpopulated. This will allow Envoy to match the
+requested resource to the response.
+
+Updates to the route configuration entry to which a virtual host belongs will


This paragraph should be updated based on what we agreed in the other thread:

VHDS subscriptions (and the corresponding locally cached resources) go away only when an RDS update changes the vhds field. Other RDS updates do not affect the VHDS data.

Any given virtual host must be sent either via RDS or via VHDS; the same host cannot be handled in both places. This means that if an RDS update adds a virtual host that was previously obtained via VHDS, the client should unsubscribe from that virtual host via VHDS.

Signed-off-by: Brian Avery <bavery@redhat.com>

htuch

LGTM modulo the aliases and virtual host orthogonality threads (and nits)

htuch · 2019-12-10T22:43:48Z

-  fi
-  # Check the version_history.rst contains current release version.
-  grep --fixed-strings "$VERSION_NUMBER" docs/root/intro/version_history.rst \
+    # Check the git tag matches the version number in the VERSION file.


Why the gratuitous reformat?

Signed-off-by: Brian Avery <bavery@redhat.com>

…#9317) Document incremental xDS semantics about resources that do not exist. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A This attempts to clarify the incremental xDS protocol semantics, as per discussion in #7415 Signed-off-by: Mark D. Roth <roth@google.com>

htuch · 2019-12-12T20:10:14Z

#9317 was merged earlier; this is a pretty reasonable way to handle missing resources. Can we update the implementation to match? This shouldn't be a huge change.

Signed-off-by: Brian Avery <bavery@redhat.com>

markdroth · 2019-12-13T17:06:34Z

This looks great to me! Thanks for making all of those changes.

htuch

Thanks! Really happy to see this land, this is our first on-demand spec.

mattklein123 · 2019-12-13T18:13:23Z

Epic! Thank you every one for seeing this through!

…envoyproxy#9317) Document incremental xDS semantics about resources that do not exist. Risk Level: Low Testing: N/A Docs Changes: Included in PR Release Notes: N/A This attempts to clarify the incremental xDS protocol semantics, as per discussion in envoyproxy#7415 Signed-off-by: Mark D. Roth <roth@google.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

Adds documentation for on-demand VHDS/RDS. Signed-off-by: Brian Avery <bavery@redhat.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

mattklein123 assigned mattklein123 and htuch Jun 27, 2019

htuch reviewed Jun 27, 2019

View reviewed changes

Comment thread source/docs/ondemand_rds_vhds.md Outdated

Comment thread source/docs/ondemand_rds_vhds.md Outdated

repokitteh-read-only Bot added the waiting label Jun 27, 2019

htuch reviewed Jun 27, 2019

View reviewed changes

Comment thread source/docs/ondemand_rds_vhds.md Outdated

repokitteh-read-only Bot removed the waiting label Jul 1, 2019

repokitteh-read-only Bot added the waiting label Jul 8, 2019

htuch suggested changes Jul 9, 2019

View reviewed changes

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 16, 2019

AndresGuedez suggested changes Jul 22, 2019

View reviewed changes

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 22, 2019

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 29, 2019

stale Bot closed this Aug 5, 2019

mattklein123 reopened this Aug 5, 2019

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 5, 2019

mattklein123 added the no stalebot Disables stalebot from closing an issue label Aug 5, 2019

mattklein123 mentioned this pull request Oct 15, 2019

On-demand VHDS implementation #8617

Merged

mattklein123 closed this Oct 19, 2019

repokitteh-read-only Bot added the api label Oct 24, 2019

mattklein123 reopened this Oct 24, 2019

dmitri-d reviewed Oct 24, 2019

View reviewed changes

repokitteh-read-only Bot removed the waiting label Dec 3, 2019

brian-avery added 2 commits December 3, 2019 17:39

Fixed reference

08a1102

Signed-off-by: Brian Avery <bavery@redhat.com>

-

d441b42

Signed-off-by: Brian Avery <bavery@redhat.com>

markdroth reviewed Dec 4, 2019

View reviewed changes

htuch added the waiting label Dec 4, 2019

Fixed formatting, fixed code review comments

50a6079

Signed-off-by: Brian Avery <bavery@redhat.com>

repokitteh-read-only Bot removed the waiting label Dec 6, 2019

Merge branch 'master' into vhds_readme

b9f4f68

htuch reviewed Dec 10, 2019

View reviewed changes

htuch added the waiting label Dec 10, 2019

markdroth mentioned this pull request Dec 11, 2019

Document incremental xDS semantics about resources that do not exist. #9317

Merged

repokitteh-read-only Bot removed the waiting label Dec 11, 2019

brian-avery added 4 commits December 11, 2019 15:11

Cleanup

4ac96e7

Signed-off-by: Brian Avery <bavery@redhat.com>

Revert build.sh

22443e9

Signed-off-by: Brian Avery <bavery@redhat.com>

Revert api.rst

52b8c91

Signed-off-by: Brian Avery <bavery@redhat.com>

Cleanup

cbdf6d3

Signed-off-by: Brian Avery <bavery@redhat.com>

Cleaned up as per xds protocol changes

09355b4

Signed-off-by: Brian Avery <bavery@redhat.com>

htuch approved these changes Dec 13, 2019

View reviewed changes

repokitteh-read-only Bot removed the api label Dec 13, 2019

htuch merged commit 8ea6ccb into envoyproxy:master Dec 13, 2019

prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020

Vhds readme (envoyproxy#7415)

12c2925

Adds documentation for on-demand VHDS/RDS. Signed-off-by: Brian Avery <bavery@redhat.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

markdroth mentioned this pull request Oct 14, 2020

xds: add support for TTLs #13201

Merged


		.. _on_demand_vhds_rds_protocol:

		Virtual Host Discovery Service

Conversation

brian-avery commented Jun 27, 2019

Uh oh!

mattklein123 commented Jun 27, 2019

Uh oh!

lambdai commented Jun 27, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Jul 8, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stale Bot commented Jul 16, 2019

Uh oh!

AndresGuedez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stale Bot commented Jul 29, 2019

Uh oh!

stale Bot commented Aug 5, 2019

Uh oh!

brian-avery commented Aug 5, 2019

Uh oh!

mattklein123 commented Oct 19, 2019

Uh oh!

mattklein123 commented Oct 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-avery Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

brian-avery Dec 11, 2019 •

edited

Loading