internal: add HTTProxy CORS support by aberasarte · Pull Request #2890 · projectcontour/contour

aberasarte · 2020-09-08T14:01:16Z

Closes #437

codecov · 2020-09-08T14:05:25Z

Codecov Report

Merging #2890 into main will increase coverage by 0.26%.
The diff coverage is 93.82%.

@@            Coverage Diff             @@
##             main    #2890      +/-   ##
==========================================
+ Coverage   74.91%   75.18%   +0.26%     
==========================================
  Files          87       87              
  Lines        5604     5677      +73     
==========================================
+ Hits         4198     4268      +70     
- Misses       1315     1316       +1     
- Partials       91       93       +2

Impacted Files	Coverage Δ
internal/dag/dag.go	`96.84% <ø> (ø)`
internal/contour/v2/route.go	`91.37% <83.33%> (-1.89%)`	⬇️
internal/dag/httpproxy_processor.go	`94.08% <91.66%> (-0.16%)`	⬇️
internal/envoy/v2/listener.go	`97.02% <100.00%> (+0.03%)`	⬆️
internal/envoy/v2/route.go	`100.00% <100.00%> (ø)`
internal/timeout/timeout.go	`100.00% <100.00%> (ø)`
internal/dag/cache.go	`96.13% <0.00%> (+0.77%)`	⬆️

skriss

Thanks for the PR @aberasarte! I added a few initial comments.

apis/projectcontour/v1/httpproxy.go

skriss · 2020-09-10T20:45:11Z

internal/dag/httpproxy_processor.go

+	maxAge := timeout.DefaultSetting()
+	if perTryMaxAge, err := time.ParseDuration(policy.MaxAge); err == nil {
+		maxAge = timeout.DurationSetting(perTryMaxAge)
+		if maxAge.Duration().Seconds() < 0 {
+			return nil, fmt.Errorf("invalid max age value: %q", policy.MaxAge)
+		}
+	}


Do you want to support disabling this setting? Per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age, the value -1 disables the timeout.

If so, the standard for other Contour timeouts is that the input string infinity disables the timeout. The timeout.Parse function already handles parsing this.

In most other contexts "disables the timeout" means that the operation can (in principle) take forever, but -1 here means that the max age never applies, so max-age of "infinity" seems misleading. For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

Both 0 and -1 mean disable caching for the preflight request but I decided to support just the 0 value. I found tricky to handle the -1 taking into account that the durations are expressed in the Go duration format. If we wanted to support -1 how would we do that? -1s disables caching but -1h is an invalid value? I'd rather allow negative values and assume that all of them disable the cache.

skriss · 2020-09-10T20:46:15Z

internal/envoy/route.go

+	if !cp.MaxAge.UseDefault() {
+		rcp.MaxAge = fmt.Sprintf("%.0f", cp.MaxAge.Duration().Seconds())
+	}


If you want to support disabling MaxAge (see other comment), then you'd also want to check for cp.MaxAge.IsDisabled() here, and if true, pass a value of -1 to Envoy.

yeh, -1 here means an immediate timeout, not no timeout.

Now I'm doing that but for the 0 value.

jpeach

This looks like it's in pretty good shape to me. I have a few comments. I'll be out of office for a couple of weeks, so happy to defer to @skriss on reviewing subsequent iterations.

Thanks @aberasarte :)

jpeach · 2020-09-11T02:19:57Z

apis/projectcontour/v1/httpproxy.go

+	ExposeHeaders []string `json:"exposeHeaders"`
+	//  +optional
+	// MaxAge specifies the content for the *access-control-max-age* header.
+	MaxAge string `json:"maxAge"`


Since MaxAge is parsed by timeout.Setting, please include the boilerplate format description (this will be consolidated into validation regex in the future):

// MaAge durations are expressed in the Go [Duration format](https://godoc.org/time#ParseDuration). // Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". // The string "infinity" is also a valid input and specifies no timeout. // A value of "0s" will be treated as if the field were not set, i.e. by using Envoy's default behavior.

internal/dag/dag.go

jpeach · 2020-09-11T02:46:22Z

apis/projectcontour/v1/httpproxy.go

+	AllowMethods []string `json:"allowMethods"`
+	// AllowHeaders specifies the content for the *access-control-allow-headers* header.
+	//  +optional
+	AllowHeaders []string `json:"allowHeaders"`


These strings end up being joined with , and passed directly to one envoy filter, so we should add validation. Here's what I suggest (the pattern approximates the tchar spec from RFC 7230 sec 3.2.6):

// +kubebuilder:validation:Pattern=[a-zA-Z0-9!#$%&'*+.^_`|~-]+ type CorsHeaderValue string type CorsPolicy struct { AllowOrigin []CORSHeaderValue AllowMethods []CORSHeaderValue AllowHeaders []CORSHeaderValue ExposeHeaders []CORSHeaderValue } ```

We can't apply this validation pattern for the AllowOrigin header as URLs like http://example.com are valid values. @skriss should I apply it for the rest of the headers and leave AllowOrigin as a []string?

jpeach · 2020-09-11T03:02:49Z

internal/dag/httpproxy_processor.go

+	maxAge := timeout.DefaultSetting()
+	if perTryMaxAge, err := time.ParseDuration(policy.MaxAge); err == nil {
+		maxAge = timeout.DurationSetting(perTryMaxAge)
+		if maxAge.Duration().Seconds() < 0 {
+			return nil, fmt.Errorf("invalid max age value: %q", policy.MaxAge)
+		}
+	}


In most other contexts "disables the timeout" means that the operation can (in principle) take forever, but -1 here means that the max age never applies, so max-age of "infinity" seems misleading. For practical purposes, a max-age of -1 and 0 seem like they would be treated similarly?

apis/projectcontour/v1/httpproxy.go

jpeach · 2020-09-11T03:50:10Z

internal/envoy/listener.go

 		},
+		&http.HttpFilter{
+			Name: wellknown.CORS,
+		},


Reading the envoy docs, it's not clear to me whether simply adding the filter activates CORS. The filter_enabled field says:

If neither enabled, filter_enabled, nor shadow_enabled are specified, the CORS filter will be enabled for 100% of the requests.

So does that mean that when CORS policy is omitted, we need to generate a CorsPolicy_Enabled message with Enabled=false?

After testing it, I think that if we don't set the EnabledSpecifier property of &envoy_api_v2_route.CorsPolicy, the CORS filter will be enabled by default. When CORS configuration is omitted, we are not setting any CORS policy to the VirtualHost therefore, the filter is disabled.

jpeach · 2020-09-11T03:52:22Z

internal/envoy/route.go

+					Numerator:   100,
+					Denominator: envoy_type.FractionalPercent_HUNDRED,
+				},
+			},


Rather than enable this a fractional 100%, would it be simpler to do:

EnabledSpecified: &CorsPolicy_Enabled { Enabled: protobuf.Bool(true), }

I think that we should keep using filter_enabled as enabled only applies to routes and according to the docs, it is deprecated:

enabled
(BoolValue) Specifies if the CORS filter is enabled. Defaults to true. Only effective on route.
Attention
This field is deprecated. Set the filter_enabled field instead.

I've ended up leaving the EnabledSpecified property unset as the CORS filter is enabled by default (see my comment above).

internal/envoy/route.go

jpeach · 2020-09-11T03:57:25Z

internal/envoy/route.go

+	if !cp.MaxAge.UseDefault() {
+		rcp.MaxAge = fmt.Sprintf("%.0f", cp.MaxAge.Duration().Seconds())
+	}


yeh, -1 here means an immediate timeout, not no timeout.

internal/featuretests/corspolicy_test.go

apis/projectcontour/v1/httpproxy.go

aberasarte · 2020-09-18T14:07:37Z

I think that all the comments have been addressed so it should be ready for a second review, PTAL @skriss . If everything looks good, I'll rebase the main branch bringing the latest changes.

skriss · 2020-09-18T17:21:41Z

@aberasarte I'll take another look through this afternoon; thanks for the updates!

skriss

@aberasarte this is looking pretty good, I just had a few more comments. Feel free to rebase when you're ready.

I am taking one more look at the MaxAge field just to make sure we have reasonable/consistent API semantics, I'll add another comment on that shortly.

apis/projectcontour/v1/httpproxy.go

internal/featuretests/corspolicy_test.go

skriss · 2020-09-18T19:00:03Z

internal/envoy/route.go

+	}
+
+	rcp.AllowOriginStringMatch = []*matcher.StringMatcher{}
+	for _, ao := range cp.AllowOrigin {


It's worth a comment here about that since it's non-obvious.

apis/projectcontour/v1/httpproxy.go

youngnick · 2020-09-23T00:44:31Z

@aberasarte, this seems pretty close! If you can get this merged in the next 7 days, we should be able to get it into Contour 1.9. Seems like it just needs a rebase and the MaxAge fix? @skriss is that the only todo left?

skriss · 2020-09-23T14:48:26Z

I had a few other comments that still need to be addressed, but it was nothing major. Agree that we should be able to get this into 1.9!

aberasarte · 2020-09-23T20:02:57Z

All comments addressed and main rebased. PTAL.

skriss

Just one minor thing, plus it looks like one more rebase is needed (sorry, we've been merging a lot of big changes lately).

internal/timeout/timeout.go

skriss

No further comments, LGTM! @stevesloka or @youngnick it'd be great if you could take a look as well.

I have filed #2943 as a followup issue to add some more documentation on this new feature, it'd be great if we could get this in by mid-next week in order to have it in the upcoming 1.9 release.

stevesloka

Just a small nit, but otherwise lgtm! Thanks for the work here @aberasarte!! 🎉

stevesloka · 2020-09-24T21:36:53Z

apis/projectcontour/v1/httpproxy.go

+	// MaxAge indicates for how long the results of a preflight request can be cached.
+	// MaxAge durations are expressed in the Go [Duration format](https://godoc.org/time#ParseDuration).
+	// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
+	// Only possitive values are allowed while 0 disables the cache requiring a preflight OPTIONS


nit: /possitive/positive/

youngnick · 2020-09-25T01:36:14Z

Sorry @aberasarte, one more rebase and that nit, and I'll merge this for you. Yay!

Closes projectcontour#437 Signed-off-by: Aritz Berasarte <raskasso@hotmail.com>

aberasarte · 2020-09-25T04:54:36Z

It should be ready for merge now 🤞 .

skriss · 2020-09-25T14:52:29Z

integration tests flaking on SNI. Merging.

glerchundi · 2020-09-25T22:18:15Z

I cannot believe this have been finally merged. Titanic work @aberasarte, thanks for pushing this forward and for being very responsive throughout all the design & impl process. Also thanks to all the reviewers for the time you dedicated here. 👏🏻👏🏻👏🏻

aberasarte force-pushed the feature/cors-policy branch 4 times, most recently from 31f3da8 to 97f6d70 Compare September 8, 2020 15:15

jpeach self-requested a review September 8, 2020 21:35

jpeach self-assigned this Sep 8, 2020

skriss requested changes Sep 10, 2020

View reviewed changes

jpeach suggested changes Sep 11, 2020

View reviewed changes

aberasarte force-pushed the feature/cors-policy branch 5 times, most recently from b7398cc to 3f23600 Compare September 18, 2020 13:49

aberasarte commented Sep 18, 2020

View reviewed changes

apis/projectcontour/v1/httpproxy.go Outdated Show resolved Hide resolved

skriss requested changes Sep 18, 2020

View reviewed changes

skriss reviewed Sep 18, 2020

View reviewed changes

apis/projectcontour/v1/httpproxy.go Show resolved Hide resolved

aberasarte force-pushed the feature/cors-policy branch 3 times, most recently from 793a591 to 64824db Compare September 23, 2020 20:00

skriss requested changes Sep 24, 2020

View reviewed changes

internal/timeout/timeout.go Outdated Show resolved Hide resolved

aberasarte force-pushed the feature/cors-policy branch 2 times, most recently from c602dd0 to 9249b7b Compare September 24, 2020 21:12

skriss mentioned this pull request Sep 24, 2020

add CORS documentation to HTTPProxy reference #2943

Closed

skriss approved these changes Sep 24, 2020

View reviewed changes

skriss added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 24, 2020

stevesloka approved these changes Sep 24, 2020

View reviewed changes

internal: add HTTProxy CORS support

9d8c3d5

Closes projectcontour#437 Signed-off-by: Aritz Berasarte <raskasso@hotmail.com>

aberasarte force-pushed the feature/cors-policy branch from 9249b7b to 9d8c3d5 Compare September 25, 2020 04:50

skriss merged commit 04c2044 into projectcontour:main Sep 25, 2020

github-actions bot mentioned this pull request Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

Conversation

aberasarte commented Sep 8, 2020

Uh oh!

codecov bot commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skriss 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!

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!

jpeach 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!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aberasarte commented Sep 18, 2020

Uh oh!

skriss commented Sep 18, 2020

Uh oh!

skriss 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!

youngnick commented Sep 23, 2020

Uh oh!

skriss commented Sep 23, 2020

Uh oh!

aberasarte commented Sep 23, 2020

Uh oh!

skriss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Sep 8, 2020 •

edited

Loading