Add CORS filter by codesuki · Pull Request #1528 · envoyproxy/envoy

codesuki · 2017-08-24T10:37:56Z

Fixes #300

Implemented according to the issue.

There are some things to discuss. For example should max_age be an integer and should the headers, origins be a list or a string.

Also I am not sure the C++ code is nice, didn't use it for a couple of years.

mattklein123 · 2017-08-24T15:17:44Z

@alyssawilk do you mind driving review of the CORS filter?

alyssawilk

Looks good overall! Please correct any points of mine where I'm misunderstanding CORS - I'm not familar with it and only wanted to delay your review reading docs for so long :-P

alyssawilk · 2017-08-24T15:31:12Z

+CORS filter
+====================
+
+This is a filter which handles CORS requests based on route or virtual host settings.


Mind spelling Cross-origin resource sharing out once before the first mention for those who haven't reached the link yet?

alyssawilk · 2017-08-24T15:37:27Z

+  {
+    "cors": {
+        "enabled": false,
+        "allow_origin": "",


I see other filters filling in pseudo values so folks can see an example of what it would look like (e.g. grpc_json_transcoder_filter.rst). Mind doing the same?

Good point! Will amend.

alyssawilk · 2017-08-24T15:45:35Z

+  *(optional, boolean)* Defaults to false.
+
+allow_origin
+  *(required, string)* The content of the access-control-allow-origin header.


super nitty nit, consider replacing "of the" with "for the" here and below since it's the content envoy will be adding not the content which is present int he header on arrival.

alyssawilk · 2017-08-24T15:47:06Z

+   */
+  virtual const std::string& maxAge() const PURE;
+
+  /**


"is" or "should be"?

alyssawilk · 2017-08-24T16:01:39Z

+CorsFilter::~CorsFilter() {}
+
+FilterHeadersStatus CorsFilter::decodeHeaders(HeaderMap& headers, bool) {
+  initialize();


would it maks sense to make this initializeCorsPolicy and only call if the origin header is present on an OPTIONS request?

That is definitely better!
I had it inside setDecoderFilterCallbacks first but got an exception because the route was not yet available. Then I moved it there and forgot about it >_<

alyssawilk · 2017-08-24T16:06:27Z

+enabled
+  *(optional, boolean)* Defaults to false.
+
+allow_origin


Curious why is this required when the code allows it to be empty?

I wanted to get some feedback of what you think should be the required parameters. I think making enabled required would make sense too.

True, I didn't think about that much. I should have checked if it's the empty string.

alyssawilk · 2017-08-24T16:55:10Z

+void CorsFilter::initialize() {
+  auto routeEntry = decoder_callbacks_->route()->routeEntry();
+  const Envoy::Router::CorsPolicy& routeCorsPolicy = routeEntry->corsPolicy();
+  const Envoy::Router::CorsPolicy& virtualHostCorsPolicy = routeEntry->virtualHost().corsPolicy();


What about making these shared pointers to avoid all of the string copies? You could instead have helpers allowOrigin() allowCredentials() etc. which would pull the appropriate value from the config with precedence.

I was worrying about the copying. Good point. I'll make some changes. Having the helpers I could make the code cleaner and get rid of the initializer. The reason I didn't do it was that I thought calling it several times would maybe add too much overhead, but thinking about it, origin() would be called twice and others would be called only once so maybe not that much of a problem after all!

alyssawilk · 2017-08-24T16:55:25Z

+  }
+}
+
+void CorsFilter::onDestroy() {}


Might as well leave this inline in the .h file

alyssawilk · 2017-08-24T16:57:04Z

+  const Envoy::Router::CorsPolicy& virtualHostCorsPolicy = routeEntry->virtualHost().corsPolicy();
+
+  if (routeCorsPolicy.enabled() && virtualHostCorsPolicy.enabled()) {
+    cors_enabled_ = true;


If we opt to stick with the code below, any reason to not early return if cors is not enabled?

alyssawilk · 2017-08-24T17:02:24Z

+Settings
+--------
+
+Settings on a route take precedence over settings on the virtual host.


That's surprising to me. I would have assumed global config was overriden by per-cluster was overriden by per-host in most cases. Unfortunately I'm having trouble telling the order from existing docs. @mattklein123 can you confirm this is consistent?

Maybe I mistook it. Here is how I understood it:

{ "virtual_host": { ... "cors": {}, "routes": [{"prefix": "...", "cors": {...}}] }

The CORS inside would take precedence.

No problem to change it though. What I didn't like much about the description in the issue is that you have to enable CORS explicitly on the virtual_host and on the route because it says if either of them is off => it's off.
Maybe there is a better way?

Route should override virtual host. IMO if either of them is set it's on.

Makes sense! Thanks.

codesuki · 2017-08-24T23:50:34Z

+  bool allowCredentials() const override { return allow_credentials_; };
+  bool enabled() const override { return enabled_; };
+  /*
+  MOCK_CONST_METHOD0(allowOrigin, const std::string&());


TODO: remove this commented out code

codesuki · 2017-08-24T23:51:15Z

+  Http::TestHeaderMapImpl request_headers{{"method", "options"}};
+
+  EXPECT_EQ(FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
+  // EXPECT_EQ(false, filter_.is_cors_request_);


@alyssawilk is there any way to do this check? Maybe only by having a public accessor?

Thinking about it i'll go with friend class CorsFilterTest; is that ok?

Good question - I'm new enough I'm not entirely sure of the answer :-P I'm fan of test peers and I'd be inclined to claim DnsResolverImplPeer is precedent (and I prefer having Test in the name) enough to say go for it, but @mattklein123 may have a different opinion :-)

A friend class is fine if you really need it, however, in this case my preference would be to just test the outward behaviors. Meaning, you don't really care how the filter works, you just care that it's not handling in this case and returning continue. So that would be what I would do. I don't have a very strong preference though.

I was doing that first but then I had the problem that in almost all cases it returns continue and I cannot test with confidence if it really went into the specific if case. I also think black box testing is ok in general. But since there is a spec for when to return I wanted to test that somehow. If it's just an implementation detail I wouldn't want to test it.
I have some problems with the tests now. While testing in my dev environment works some tests fail.

I never heard the term test peers and Google didn't bring anything up. But I guess it means friending the class? Judging from the name. :)

Yes basically.

codesuki · 2017-08-24T23:52:24Z

  const ConfigImpl& globalRouteConfig() const { return global_route_config_; }

  // Router::VirtualHost
+  const CorsPolicy& corsPolicy() const override { return cors_policy_; }


I was never sure if the ordering is important. BUILD files where alphabetical but the code wasn't.

I don't think we bother for function names. includes and buildfiles yes though :-)

AdamCmiel · 2017-08-28T20:31:51Z

+
+  HeaderMapPtr response_headers{
+      new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}}};
+


@codesuki check here if the Origin header value matches the one in the config, no?

something like:

if (allow_origin_.find(*origin_) == std::string::npos) { return FilterHeadersStatus::Continue; } response_headers->insertAccessControlAllowOrigin().value(*origin_); if (allow_credentials_) { response_headers->insertAccessControlAllowCredentials().value( Http::Headers::get().CORSValues.True); }

Thanks! Your comment made me re-read the RFC. I first thought we can use regexes but doesn't seem so after all. I'll read the spec once more and make some changes.

Per w3c the header must contain the domain that was sent as the Origin header for the request to be accepted. How the server decides how to accept that string is implementation dependent.

codesuki · 2017-08-29T20:00:32Z

I am on vacation this week. Will push the changes next week!

alyssawilk

Looking much cleaner!

Have you considered adding an integration test? It would be good to have some end to end coverage of things working as expected. I don't think it's necessary to have end-to-end coverage of the interplay between global and route config but iff you want to I'd suggest extending the proto_integration_test once your API changes are merged, since it's easier to add multiple tests with the same config in that one. If you just want to test one config end to end (which is totally fine given the thorough unit testing) you can just extend the appropriate test/config/integration/*.json and the corresponding integration test.

alyssawilk · 2017-09-05T18:53:47Z

+  CorsFilterConfigConstSharedPtr config_;
+  StreamDecoderFilterCallbacks* decoder_callbacks_{};
+  StreamEncoderFilterCallbacks* encoder_callbacks_{};
+  const Envoy::Router::CorsPolicy* routeCorsPolicy_{};


member variables get snake case so route_cors_policy_{} and virtual_host_cors_policy_{}

Meanwhile Envoy isn't big on raw pointers where avoidable* and I'm unsure this would be safe if there were a config reload. Should they be shared pointers instead?

*there's totally precedent for the Encoder/Decoder callbacks being raw pointers so don't worry about those.

I have a shared pointer version stashed. Let me apply and push it. I hesitated adding it because no other policy / config used it. I added it as shared pointers all the way from config_impl router::routeentry and router::virtualhost. Not sure that's all correct.

alyssawilk · 2017-09-05T18:55:08Z

+  const Envoy::Router::CorsPolicy* virtualHostCorsPolicy_{};
+  bool is_cors_request_{};
+  Http::HeaderEntry* origin_{};
+  std::string origin__{};


I think this is now unused. If you end up using, I'd suggest origin_header_ and origin_value_ or some such - if you have conflicting names it's better to make them more descriptive :-)

Sorry, accidentally stashed those changes.

alyssawilk · 2017-09-05T18:56:26Z

+  void initialize();
+
+  // Http::StreamFilterBase
+  void onDestroy() override{};


I think clang_format will require a space after override.

nit: remove ';' after definition (will fix Alyssa's comment)

alyssawilk · 2017-09-05T18:59:59Z

  const ConfigImpl& globalRouteConfig() const { return global_route_config_; }

  // Router::VirtualHost
+  const CorsPolicy& corsPolicy() const override { return cors_policy_; }


I don't think we bother for function names. includes and buildfiles yes though :-)

codesuki · 2017-09-05T23:59:36Z

Very good, I'll add an integration test :)

Now it is possible to disable CORS for specific routes even if there is a CORS policy on the virtual host.

codesuki · 2017-09-06T08:41:59Z

I tried adding integration tests but got some compile errors. Any ideas? I am inheriting from the BaseIntegrationTest that has the members. Maybe I am too tired.

test/integration/cors_filter_integration_test.cc:32:23: error: use of undeclared identifier 'codec_client_'
                      codec_client_ =
                      ^
test/integration/cors_filter_integration_test.cc:36:23: error: use of undeclared identifier 'request_encoder_'
                      request_encoder_ = &codec_client_->startRequest(
                      ^
test/integration/cors_filter_integration_test.cc:36:43: error: use of undeclared identifier 'codec_client_'
                      request_encoder_ = &codec_client_->startRequest(
                                          ^
test/integration/cors_filter_integration_test.cc:41:28: error: use of undeclared identifier 'response_'
                          *response_);
                           ^
test/integration/cors_filter_integration_test.cc:44:37: error: use of undeclared identifier 'codec_client_'
                    [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); },
                                    ^
test/integration/cors_filter_integration_test.cc:44:62: error: use of undeclared identifier 'request_encoder_'
                    [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); },
                                                             ^
test/integration/cors_filter_integration_test.cc:45:37: error: use of undeclared identifier 'response_'
                    [&]() -> void { response_->waitForEndStream(); },
                                    ^
test/integration/cors_filter_integration_test.cc:46:37: error: use of undeclared identifier 'codec_client_'
                    [&]() -> void { codec_client_->close(); }});
                                    ^
test/integration/cors_filter_integration_test.cc:48:17: error: use of undeclared identifier 'response_'
    EXPECT_TRUE(response_->complete());
                ^
test/integration/cors_filter_integration_test.cc:49:25: error: use of undeclared identifier 'response_'
    EXPECT_STREQ("200", response_->headers().Status()->value().c_str());

#include "test/integration/integration.h"
#include "test/mocks/http/mocks.h"

#include "gtest/gtest.h"

namespace Envoy {

class CorsFilterIntegrationTest : public BaseIntegrationTest,
                                  public testing::TestWithParam<Network::Address::IpVersion> {
public:
  CorsFilterIntegrationTest() : BaseIntegrationTest(GetParam()) {}
  /**
   * Global initializer for all integration tests.
   */
  void SetUp() override {
    fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_));
    registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port());
    createTestServer("test/config/integration/server_cors_filter.json", {"http"});
  }

  /**
   * Global destructor for all integration tests.
   */
  void TearDown() override {
    test_server_.reset();
    fake_upstreams_.clear();
  }

protected:
  void testTest() {
    executeActions({[&]() -> void {
                      codec_client_ =
                          makeHttpConnection(lookupPort("http"), Http::CodecClient::Type::HTTP2);
                    },
                    [&]() -> void {
                      request_encoder_ = &codec_client_->startRequest(
                          Http::TestHeaderMapImpl{{":method", "GET"},
                                                  {":path", "/healthcheck"},
                                                  {":scheme", "http"},
                                                  {":authority", "host"}},
                          *response_);
                    },
                    [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); },
                    [&]() -> void { response_->waitForEndStream(); },
                    [&]() -> void { codec_client_->close(); }});

    EXPECT_TRUE(response_->complete());
    EXPECT_STREQ("200", response_->headers().Status()->value().c_str());
  }
}; // namespace Envoy

INSTANTIATE_TEST_CASE_P(IpVersions, CorsFilterIntegrationTest,
                        testing::ValuesIn(TestEnvironment::getIpVersionsForTest()));


TEST_P(CorsFilterIntegrationTest, TestCors) { testTest(); }
} // namespace Envoy

BUILD

envoy_cc_test(
    name = "cors_filter_integration_test",
    srcs = [
        "cors_filter_integration_test.cc"
    ],
    data = [
        "//test/config/integration:server_cors_filter.json",
    ],
    deps = [
        ":integration_lib",
        "//source/common/buffer:buffer_lib",
        "//source/common/http:header_map_lib",
        "//test/mocks/http:http_mocks",
        "//test/mocks/upstream:upstream_mocks",
        "//test/test_common:utility_lib",
    ],
)

alyssawilk · 2017-09-06T17:23:02Z

Have you pulled from and merged with upstream/master recently? You can check to see if your local copy of integration.h has the additions from PR 1532 <#1532>

…

On Wed, Sep 6, 2017 at 4:42 AM, Neri Marschik ***@***.***> wrote: I tried adding integration tests but got some compile errors. Any ideas? I am inheriting from the BaseIntegrationTest that has the members. Maybe I am too tired. test/integration/cors_filter_integration_test.cc:32:23: error: use of undeclared identifier 'codec_client_' codec_client_ = ^ test/integration/cors_filter_integration_test.cc:36:23: error: use of undeclared identifier 'request_encoder_' request_encoder_ = &codec_client_->startRequest( ^ test/integration/cors_filter_integration_test.cc:36:43: error: use of undeclared identifier 'codec_client_' request_encoder_ = &codec_client_->startRequest( ^ test/integration/cors_filter_integration_test.cc:41:28: error: use of undeclared identifier 'response_' *response_); ^ test/integration/cors_filter_integration_test.cc:44:37: error: use of undeclared identifier 'codec_client_' [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); }, ^ test/integration/cors_filter_integration_test.cc:44:62: error: use of undeclared identifier 'request_encoder_' [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); }, ^ test/integration/cors_filter_integration_test.cc:45:37: error: use of undeclared identifier 'response_' [&]() -> void { response_->waitForEndStream(); }, ^ test/integration/cors_filter_integration_test.cc:46:37: error: use of undeclared identifier 'codec_client_' [&]() -> void { codec_client_->close(); }}); ^ test/integration/cors_filter_integration_test.cc:48:17: error: use of undeclared identifier 'response_' EXPECT_TRUE(response_->complete()); ^ test/integration/cors_filter_integration_test.cc:49:25: error: use of undeclared identifier 'response_' EXPECT_STREQ("200", response_->headers().Status()->value().c_str()); #include "test/integration/integration.h" #include "test/mocks/http/mocks.h" #include "gtest/gtest.h" namespace Envoy { class CorsFilterIntegrationTest : public BaseIntegrationTest, public testing::TestWithParam<Network::Address::IpVersion> { public: CorsFilterIntegrationTest() : BaseIntegrationTest(GetParam()) {} /** * Global initializer for all integration tests. */ void SetUp() override { fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP2, version_)); registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port()); createTestServer("test/config/integration/server_cors_filter.json", {"http"}); } /** * Global destructor for all integration tests. */ void TearDown() override { test_server_.reset(); fake_upstreams_.clear(); } protected: void testTest() { executeActions({[&]() -> void { codec_client_ = makeHttpConnection(lookupPort("http"), Http::CodecClient::Type::HTTP2); }, [&]() -> void { request_encoder_ = &codec_client_->startRequest( Http::TestHeaderMapImpl{{":method", "GET"}, {":path", "/healthcheck"}, {":scheme", "http"}, {":authority", "host"}}, *response_); }, [&]() -> void { codec_client_->sendData(*request_encoder_, 0, true); }, [&]() -> void { response_->waitForEndStream(); }, [&]() -> void { codec_client_->close(); }}); EXPECT_TRUE(response_->complete()); EXPECT_STREQ("200", response_->headers().Status()->value().c_str()); } }; // namespace Envoy INSTANTIATE_TEST_CASE_P(IpVersions, CorsFilterIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); TEST_P(CorsFilterIntegrationTest, TestCors) { testTest(); } } // namespace Envoy BUILD envoy_cc_test( name = "cors_filter_integration_test", srcs = [ "cors_filter_integration_test.cc" ], data = [ "//test/config/integration:server_cors_filter.json", ], deps = [ ":integration_lib", "//source/common/buffer:buffer_lib", "//source/common/http:header_map_lib", "//test/mocks/http:http_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:utility_lib", ], ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1528 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvfV-ZuVAuxDLw632xVNTFXwSI46Lks5sflrsgaJpZM4PBM4J> .

codesuki · 2017-09-06T20:13:57Z

Thanks! I noticed there were some issues with the integration tests. I'll try later!

codesuki · 2017-09-11T06:10:12Z

Tests passing now.

alyssawilk

LGTM. @mattklein123 mind taking a look now?

mattklein123

Overall excellent work, thank you. Some random comments to get started.

mattklein123 · 2017-09-11T16:08:00Z

  The first route that matches will be used.

+:ref:`cors <config_http_filters_cors>`
+  *(optional, object)* Specifies the route's CORS policy.


"Specifies the virtual host's ..."

mattklein123 · 2017-09-11T16:09:05Z

+
+This is a filter which handles Cross-Origin Resource Sharing requests based on route or virtual host settings.
+For the meaning of the headers please refer to the pages below.
+https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS


nit: I would potentially use bulleted list here. (Either way take a look at generated docs locally to make sure they are to your liking).

mattklein123 · 2017-09-11T16:09:31Z

+.. code-block:: json
+
+  {
+    "type": "both",


"type" is deprecated now so just go ahead and kill from docs.

mattklein123 · 2017-09-11T16:11:18Z

+  }
+
+enabled
+  *(optional, boolean)* Defaults to false. Setting *enabled* to false on a route disables CORS


Q: Is this the most common case for this field? Or do you think we should potentially default to true? (I'm fine with whatever just want to make sure we default to the most common thing).

Sorry! This should say defaults to true.

Good catch!

mattklein123 · 2017-09-11T16:12:05Z

+  for this route only. The setting has no effect on a virtual host.
+
+allow_origin
+  *(optional, string)* The origins that will be allowed to do CORS request.


Is this a comma delimited list? Can you specify? Same for other fields below where this applies.

mattklein123 · 2017-09-11T16:21:25Z

+};
+
+void CorsFilter::initialize() {
+  route_cors_policy_ = decoder_callbacks_->route()->routeEntry()->corsPolicy();


Both of these calls aren't always safe. The route() might be empty. It might also be a redirect. You need to handle both of these cases.

I will skip the filter in case it's a redirect, since the final host should probably handle that. Is that ok?

Yes, that's what I would do. (Skip of no route or redirect)

mattklein123 · 2017-09-11T16:21:43Z

+    return FilterHeadersStatus::Continue;
+  }
+
+  auto requestMethod = headers.AccessControlRequestMethod();


nit: const auto request_method

mattklein123 · 2017-09-11T16:21:57Z

+
+  is_cors_request_ = true;
+
+  auto method = headers.Method();


mattklein123 · 2017-09-11T16:22:38Z

+
+bool CorsFilter::isOriginAllowed(Http::HeaderString& origin) {
+  for (const auto& o : allowOrigin()) {
+    if (o == "*" || origin == o.c_str()) {


Looking at this review now. If you want to use a map here, please use a map. I'm not sure where anyone is getting the impression that I don't like maps! :)

mattklein123 · 2017-09-11T16:23:28Z

  // Default timeout is 15s if nothing is specified in the route config.
  static const uint64_t DEFAULT_ROUTE_TIMEOUT_MS = 15000;

+  CorsPolicySharedPtr cors_policy_;


per other comments, I think if we remove shared_ptr, this can become unique_ptr

With unique pointer do you mean the CorsPolicy* you wrote above or unique_ptr?

I just meant that internally you can store as unique_ptr<CorsPolicy) then just return cors_policy_.get()

Understood. If using new then use unique_ptr. But since I will just initialize it in the constructor I don't need new so I will just make it a member like RetryPolicy etc.
( https://github.com/lyft/envoy/blob/master/source/common/router/config_impl.h#L327-L333 )

codesuki · 2017-09-11T22:00:23Z

Thanks for the review! The points I didn't reply to I will fix directly.

This reverts commit e9e876c.

codesuki · 2017-09-12T06:21:19Z

All points should be addressed.

mattklein123

Looks good. Few small comments and then good to go. Note that there are 2 PRs that will merge today (integration test and access log one which will cause you to have to do a few small merges. Sorry).

mattklein123 · 2017-09-12T15:13:35Z

+  expose_headers_ = config.expose_headers();
+  max_age_ = config.max_age();
+  allow_credentials_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, allow_credentials, false);
+  enabled_ = PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enabled, true);


Unless I'm missing something, isn't this going to make a fully empty CORS config be "enabled" which will kick on the filter? It seems like if the user does't specify any CORS config at all the default instantiation should be for it to be disabled?

True! ... is there any way to detect absence of config? Maybe by making it an unique_ptr after all?
Setting it to false would mean that a user has to enable it on every route even if all the settings are contained on the vhost.

You could make it a unique_ptr and return a const pointer. The issue though is when you translate v1 JSON config to v2. You will probably have to pass a pointer to a v2 object vs. reference, and then pivot on whether it is nullptr or not.

mattklein123 · 2017-09-12T15:13:55Z

@@ -0,0 +1,31 @@
+#include "server/config/http/cors.h"
+
+#include <chrono>


nit: not needed

mattklein123 · 2017-09-12T15:14:17Z

+#include "envoy/registry/registry.h"
+
+#include "common/http/filter/cors_filter.h"
+#include "common/json/config_schemas.h"


nit: not needed (maybe others also, please clean up BUILD file also)

Oh, sorry. I'll clean that up. Totally used to go cleaning up imports by itself.

mattklein123 · 2017-09-12T15:15:05Z

+  HttpFilterFactoryCb createFilterFactory(const Json::Object& json_config,
+                                          const std::string& stats_prefix,
+                                          FactoryContext& context) override;
+  std::string name() override { return "cors"; }


Note: This is going to need a merge/fix of: #1609

mattklein123 · 2017-09-12T15:15:55Z

+  decoder_callbacks_ = &callbacks;
+};
+
+void CorsFilter::initialize() {


nit: Since this is only called from decodeHeaders(), can we inline this code there?

mattklein123 · 2017-09-12T15:16:44Z

+
+void CorsFilter::initialize() {
+  if (decoder_callbacks_->route() == nullptr ||
+      decoder_callbacks_->route()->redirectEntry() != nullptr ||


nit: redirectEntry() vs. routeEntry() is guaranteed to be XOR, so you can just check that routeEntry() != nullptr.

mattklein123 · 2017-09-12T15:17:34Z

+  return FilterHeadersStatus::StopIteration;
+}
+
+FilterHeadersStatus CorsFilter::encodeHeaders(HeaderMap& headers, bool) {


Can you add a small comment here on why we need to do this in encodeHeaders()? (Either link to spec or just tiny explanation for future readers).

mattklein123 · 2017-09-12T15:18:26Z

+CorsFilter::CorsFilter() : is_cors_request_(false) {}
+
+FilterHeadersStatus CorsFilter::decodeHeaders(HeaderMap& headers, bool) {
+  origin_ = headers.Origin();


Per my other comment about default init of the policy, IMO, the first thing we should do here is the current contents of initialize(), then, if not enabled, quickly continue.

codesuki · 2017-09-14T04:27:48Z

Addressed all of the above I think.

mattklein123

Looks solid. Great contribution. Few small comments then ready to go.

mattklein123 · 2017-09-14T14:50:30Z

+  }
+
+  origin_ = headers.Origin();
+  if (origin_ == nullptr || origin_->value() == "") {


nit: origin_->value().empty()

Sorry, have to focus more.

mattklein123 · 2017-09-14T14:51:56Z

+  }
+
+  const auto requestMethod = headers.AccessControlRequestMethod();
+  if (requestMethod == nullptr || requestMethod->value() == "") {


nit: Same .empty()

mattklein123 · 2017-09-14T14:53:35Z

+
+  StreamDecoderFilterCallbacks* decoder_callbacks_{};
+  StreamEncoderFilterCallbacks* encoder_callbacks_{};
+  std::vector<const Envoy::Router::CorsPolicy*> policies_{};


Perf nit: I would make this an std::array of fixed size 2, then just initialize in decodeHeaders().

I had it as an array first because of that reason. Then I switched to vector to use push_back. I'll switch back.

mattklein123 · 2017-09-14T14:55:52Z

+  expose_headers_ = config.expose_headers();
+  max_age_ = config.max_age();
+  if (config.has_allow_credentials()) {
+    allow_credentials_.value(PROTOBUF_GET_WRAPPED_REQUIRED(config, allow_credentials));


I'm a little confused as to why we need to make this optional. Can't you just default this to false?

Premise: we check route first then fall back to vhost if not set.

If I default to false I cannot check if it's set or not. I could accept that it's false so the user would have to set true every time he adds a route config to change the headers or such. So I just made this behave the same.
We could change it back to default false and document it. Might be unexpected though behavior though :)

No that's fine, was just curious. Sounds good.

I might even want to make expose headers the same. Because it could make sense to set it to an empty string. Have to confirm. The others make no sense to be empty I think. (Headers+methods+age)

mattklein123

awesome work. Thanks for working on this.

**Description** https://aigateway.envoyproxy.io/docs/0.3/getting-started/prerequisites has error in the doc mentioning v0.3.0 version for envoy gateway helm chart, it needs to be 1.5.0 instead **Related Issues/PRs (if applicable)** N/A **Special notes for reviewers (if applicable)** N/A --------- Signed-off-by: Deepak Deore <deepakdeore2004@gmail.com>

codesuki added 3 commits August 24, 2017 19:32

Add CORS documentation

05ac0e2

Add CORS filter implementation

a67be87

Add CORS filter tests

e42f868

mattklein123 requested a review from alyssawilk August 24, 2017 15:17

alyssawilk reviewed Aug 24, 2017

View reviewed changes

codesuki commented Aug 24, 2017

View reviewed changes

AdamCmiel reviewed Aug 28, 2017

View reviewed changes

codesuki added 5 commits September 5, 2017 11:29

Refactor CORS filter

9fe28e9

Make allow_origin parameter optional

6bb7423

Revert ci/do_ci.sh

75ce91c

Fix allow_origin documentation

8518b3a

Refactor to use auto variable

ab5de0d

alyssawilk reviewed Sep 5, 2017

View reviewed changes

codesuki added 6 commits September 6, 2017 11:26

Remove unused member

0b8a653

Change to shared_ptr

e9e876c

Change allow_origin config to be a list of strings

4264daa

clang-format

4bac210

Fix behavior of enabled config

c26412e

Now it is possible to disable CORS for specific routes even if there is a CORS policy on the virtual host.

Fix config test

864c89e

codesuki added 3 commits September 7, 2017 16:30

Fix documentation to reflect changes to enabled

c464722

Fix code to work with master branch

b47d385

Add integration tests

8c329c3

Fix format

711ac73

alyssawilk previously approved these changes Sep 11, 2017

View reviewed changes

mattklein123 reviewed Sep 11, 2017

View reviewed changes

codesuki added 5 commits September 12, 2017 14:29

Revert "Change to shared_ptr"

abdd136

This reverts commit e9e876c.

Fix return type in comment

eb10734

Update documentation

46d6e55

Fix nits

116450a

Handle empty routes and redirect routes

243f588

codesuki dismissed alyssawilk’s stale review via 243f588 September 12, 2017 05:30

codesuki added 2 commits September 12, 2017 14:47

Update documentation

65bc834

Rename accessor to reflect return type

cfc777a

mattklein123 reviewed Sep 12, 2017

View reviewed changes

codesuki added 4 commits September 14, 2017 10:49

Make CORS config optional

731194d

Merge branch 'master' of github.com:lyft/envoy into cors

431fcf4

Fix integration tests

2c92bf0

Add cors filter to well known names

d5f8133

Fix format

613d4bd

mattklein123 reviewed Sep 14, 2017

View reviewed changes

codesuki added 2 commits September 15, 2017 09:14

Use std::array instead of std::vector

d4cb4cf

Remove logging header

2ad686b

mattklein123 closed this Sep 15, 2017

mattklein123 reopened this Sep 15, 2017

mattklein123 approved these changes Sep 15, 2017

View reviewed changes

mattklein123 merged commit d3303a1 into envoyproxy:master Sep 15, 2017


		HeaderMapPtr response_headers{
		new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(Http::Code::OK))}}};

		@@ -0,0 +1,31 @@
		#include "server/config/http/cors.h"

		#include <chrono>

Conversation

codesuki commented Aug 24, 2017 • edited by mattklein123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Aug 24, 2017

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!

codesuki Aug 24, 2017 • 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!

codesuki Aug 25, 2017 • 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!

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 Aug 25, 2017 • 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!

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!

AdamCmiel Aug 28, 2017 • 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

codesuki commented Aug 24, 2017 •

edited by mattklein123

Loading

codesuki Aug 24, 2017 •

edited

Loading

codesuki Aug 25, 2017 •

edited

Loading

mattklein123 Aug 25, 2017 •

edited

Loading

AdamCmiel Aug 28, 2017 •

edited

Loading