router: add support for direct (non-proxied) http responses. by brian-pane · Pull Request #2335 · envoyproxy/envoy

brian-pane · 2018-01-10T04:26:12Z

Description:
This PR implements support for returning an HTTP response with a configured
status code without proxying to an upstream.

Risk Level: Medium

Limitations:
There are two features in the corresponding data-plane-api PR that are not implemented in this PR:

Sending an entity-body with the response
Sending the configured response_headers_to_add with the response

I plan to put those feature in a separate PR, in order to keep this PR manageably small.
There are TODO comments for them in this PR.

Testing: Updated unit and integration tests included

Docs Changes: N/A

Release Notes:
N/A (I'll add this feature to the release notes once I've added support for sending
a configured response body and headers.)

Fixes: #2315

API Changes:
envoyproxy/data-plane-api#393

Signed-off-by: Brian Pane bpane@pinterest.com

Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk

Cool, this will be a super useful feature to have!

a couple of drive by comments below

alyssawilk · 2018-01-10T14:57:50Z

include/envoy/router/router.h

 };

+/**
+ * A routing primitive that specifies a direct (non-proxied) HTTP response .


alyssawilk · 2018-01-10T15:01:02Z

include/envoy/router/router.h

 typedef std::unique_ptr<const Decorator> DecoratorConstPtr;

 /**
- * An interface that holds a RedirectEntry or a RouteEntry for a request.


I'd prefer this replace RedirectEntry rather than be a slightly duplicate thing.
newPath could be optional, and expected for redirect responses. I'm neutral if it should be done here or in a follow-up but maybe add a TODO?

Thanks for the suggestion; combining RedirectEntry and DirectResponseEntry sounds good to me. The combined code will have to deal with some incompatibilities between the two -- e.g., under the current data plane API, redirects aren't allowed to send the response_headers_to_add, whereas direct responses are required to -- but it will probably still be cleaner than the current diff.

I'll merge the implementations post an update to this PR later today.

cool. hopefully if we do some sanity checks with an IsRedirectReturnCode() in the right places we'll guard against bad configs/code

I don't see any reason redirects cannot also populate response_headers_to_add. That seems pretty useful to me. Thoughts?

Adding response_headers_to_add to redirects would change the behavior of existing configs. If we were designing the config schema from scratch, I’d advocate for sending the headers with redirects. But at this point it would impact backward compatibility. Unless we added a “send_headers_with_redirects” flag that defaults to false, I suppose :(

Yeah, fair enough. I kind of think we could "just do it" and document it in this case as a change. But that's me. Fine to block/TODO for now if you think that would be best.

alyssawilk · 2018-01-10T15:04:48Z

include/envoy/router/router.h

  virtual const RedirectEntry* redirectEntry() const PURE;

+  /**
+   * @return the direct response entry or nullptr if there is no direct response for the request.


@return DirectResponseEntry*

alyssawilk · 2018-01-10T15:09:06Z

source/common/router/config_impl.cc

+const DirectResponseEntry* RouteEntryImplBase::directResponseEntry() const {
+  // A route for a request can exclusively be a route entry, a direct response entry,
+  // or a redirect entry.
+  if (isDirectResponse()) {


Yeah, I think this is confusing, since a redirect entry is a direct response too

alyssawilk · 2018-01-10T15:11:23Z

test/config/utility.h


+  // Add an additional route with a direct (non-proxied) response.
+  void addDirectResponse(const std::string& domains, const std::string& prefix,
+                         Http::Code responseCode, const std::string& body);


Do you think this will be used widely? If not I'd just suggest doing this as with void addConfigModifier(HttpModifierFunction function); in testRouterDirectResponse

mattklein123

2 drive by comments. Thanks for working on this. I'm +1 on combining redirect and direct response somehow. I think it will make the code simpler.

mattklein123 · 2018-01-10T17:14:31Z

source/common/config/rds_json.cc

      throw EnvoyException("Redirect route entries must not have WebSockets set");
    }
  }
+  bool has_direct_response = false;


All the v1 code can be dropped, right?

mattklein123 · 2018-01-10T17:15:46Z

source/common/ssl/context_config_impl.cc

              validateAndAppendKey(ret, datasource.inline_bytes());
              break;
            }
+            case envoy::api::v2::DataSource::kInlineString: {


Can we remove from this PR?

I’ll move this to a separate PR

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane · 2018-01-11T05:03:29Z

I'll add another commit later to address a couple of open review comments:

removing v1 API support
removing the new addDirectResponse utility function that's only used once in the test suite

alyssawilk

This is looking great - thanks for taking the time to extend/rename the redirect entry

alyssawilk · 2018-01-11T21:14:00Z

source/common/router/router.cc

+  if (route_->directResponseEntry()) {
+    auto response_code = route_->directResponseEntry()->responseCode();
+    if (response_code >= Http::Code::MultipleChoices && response_code < Http::Code::BadRequest) {
+      config_.stats_.rq_redirect_.inc();


I think we probably want a direct response stat which supplements (and hopefully eventually replaces) the redirect stat.

I added a counter of direct responses. Is there a precedent in Envoy for also having counters per response status or per response code family (2xx, 3xx, etc)?

We already have split out codes for all downstream responses. IMO that's probably sufficient.

alyssawilk · 2018-01-11T21:17:11Z

source/common/router/router.cc

+                                  response_code);
+      return Http::FilterHeadersStatus::StopIteration;
+    }
+    Http::Utility::sendLocalReply(


Take a look at callers of sendLocalReply - I think you can simplify the arguments a bit (and if I'm incorrect please educate me! :-P)

Oh, I see, this method is in the same class that provides a friendly wrapper around sendLocalReply. I'll switch to that.

Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123

looks solid, thanks. Few small comments.

mattklein123 · 2018-01-12T02:33:49Z

source/common/json/config_schemas.cc

      "weighted_clusters": {"$ref" : "#/definitions/weighted_clusters"},
      "host_redirect" : {"type" : "string"},
      "path_redirect" : {"type" : "string"},
+      "status": {"type": "integer"},


mattklein123 · 2018-01-12T02:36:05Z

source/common/router/config_impl.h


  const DecoratorConstPtr decorator_;
-  const Http::Code redirect_response_code_;
+  const Http::Code direct_response_code_;


nit: I might consider making this Optional<Http::Code>. I think it would be slightly clearer in all the places you set and consume it.

mattklein123 · 2018-01-12T02:38:07Z

source/common/router/router.cc

+                                  response_code);
+      return Http::FilterHeadersStatus::StopIteration;
+    }
+    config_.stats_.rq_direct_response_.inc();


please add an expectation in one of the tests that checks this stat gets incremented.

Oh, and I almost forgot - we should update docs for this. Please add an envoy-API pull updating router_filter.rst and note it in your PR description

@brian-pane you plan on finishing up docs in a follow up, right?

Yes, I was planning to make a followup PR that adds the remainder of this feature (adding optional response headers and body), with the docs linked to that new PR.

OK that works. Will merge this.

Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123

LGTM, thank you. Will defer to @alyssawilk for final review/merge. (She is out Fridays so probably Tuesday).

PiotrSikora · 2018-01-16T02:53:36Z

Could you please add YAML tests?

brian-pane · 2018-01-16T06:28:44Z

I'll add SAML tests and post an updated diff tomorrow

alyssawilk

LGTM, Looks great to me :-)
Merge pending Piotr's sign off on tests.

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane · 2018-01-16T22:06:21Z

@PiotrSikora I added some YAML tests. Let me know if they look okay.

mattklein123

Nice, thanks!

And include in GitHub release artifacts Updates rules_swift to pull in bazelbuild/rules_swift#838 When users open the archive, they'll have rich docs they can explore using Xcode's documentation navigator. When we update to Xcode 13.4, we'll be able to generate HTML docs with this approach and publish to the documentation website. Here's a demo of what that static site looks like: https://envoy-mobile-docc-demo.netlify.app/documentation/envoy/ https://user-images.githubusercontent.com/474794/171704142-993072a3-fbc2-4db6-9c70-65e9e1a47aac.mp4 Risk Level: Low Testing: Local, CI Docs Changes: Added Release Notes: Added Signed-off-by: JP Simard <jp@jpsim.com>

Brian Pane added 2 commits January 10, 2018 04:11

Add support for direct (non-proxied) HTTP responses.

942d251

Signed-off-by: Brian Pane <bpane@pinterest.com>

Fix formatting

eec6775

Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk reviewed Jan 10, 2018

View reviewed changes

mattklein123 reviewed Jan 10, 2018

View reviewed changes

Brian Pane added 2 commits January 11, 2018 04:23

Merge the redirect and direct-response implementations

3f04d58

Signed-off-by: Brian Pane <bpane@pinterest.com>

Fix edit error that caused a merge conflict

dfc0b8d

Signed-off-by: Brian Pane <bpane@pinterest.com>

alyssawilk reviewed Jan 11, 2018

View reviewed changes

Brian Pane added 2 commits January 11, 2018 21:33

Cleanups: remove v1 API support and addDirectResponse function

ea91a05

Signed-off-by: Brian Pane <bpane@pinterest.com>

Simplify the test code and add instrumentation.

77a170f

Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123 reviewed Jan 12, 2018

View reviewed changes

Clean up JSON schema, use Optional<Http::Code>, and add counter test.

49b2732

Signed-off-by: Brian Pane <bpane@pinterest.com>

mattklein123 previously approved these changes Jan 12, 2018

View reviewed changes

mattklein123 assigned alyssawilk Jan 12, 2018

alyssawilk previously approved these changes Jan 16, 2018

View reviewed changes

Add YAML test

e309720

Signed-off-by: Brian Pane <bpane@pinterest.com>

brian-pane dismissed stale reviews from alyssawilk and mattklein123 via e309720 January 16, 2018 21:05

PiotrSikora approved these changes Jan 16, 2018

View reviewed changes

mattklein123 approved these changes Jan 16, 2018

View reviewed changes

mattklein123 merged commit a651288 into envoyproxy:master Jan 16, 2018

brian-pane deleted the direct-response/2315 branch January 22, 2018 22:47

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Add a CODEOWNERS, will delete OWNERS separately. (envoyproxy#2335)

2f84cce

mbana mentioned this pull request Oct 5, 2022

Allow to automatically add client_secret to ConfigMap in OAuth2 kubeshop/kusk-gateway#703

Open

Conversation

brian-pane commented Jan 10, 2018

Uh oh!

alyssawilk left a comment

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!

mattklein123 Jan 10, 2018 • 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!

mattklein123 left a comment

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-pane commented Jan 11, 2018

Uh oh!

alyssawilk left a comment

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!

mattklein123 left a comment

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Jan 16, 2018

Uh oh!

brian-pane commented Jan 16, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

brian-pane commented Jan 16, 2018

mattklein123 Jan 10, 2018 •

edited

Loading