TT-968 enabled h2c by default #3380

sredxny · 2020-11-04T20:25:57Z

Description

now for all the http proxies (including the main router of gw) is activated h2c. However, this doesn't means we will consume the upstream via h2c as this should be activated via target_url. This should be merged after #3372

Related Issue

https://tyktech.atlassian.net/browse/TT-968

Motivation and Context

give solution to https://tyktech.atlassian.net/browse/TT-968

How This Has Been Tested

Run gw in http mode and dashboard
Created api A without custom port
consume api A via postman, and via curl (Eg: curl -v --http2-prior-knowledge http://tyk-gateway:8080/keyless/uuid)
Created api B with custom port
consume api B via postman and curl
Created and configured grpc service
Consume grpc service via grpcurl (Eg: grpcurl -plaintext -proto helloworld.proto -d '{"name": "sredny"}' tyk-gateway:12345 helloworld.Greeter/SayHello)

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

…rl which should contains h2c://

matiasinsaurralde

LGTM, we need a rebase

gernest · 2021-01-05T07:28:47Z

gateway/api_loader.go

+	if strings.HasPrefix(spec.Proxy.TargetURL, "h2c://") {
+		spec.Proxy.TargetURL = strings.Replace(spec.Proxy.TargetURL, "h2c://", "http://", 1)
+	}


why are we replacing target scheme here?

Because we treat h2c as alias to http. Probably should be followed by conned in source code.

I'm confused, from reading h2c is not http traffic please see this doc comment https://pkg.go.dev/golang.org/x/net/http2/h2c#NewHandler

This link actually has good description, saying that h2c is http1.1 compatible

gernest · 2021-01-05T07:34:42Z

gateway/api_loader.go

@@ -763,6 +766,9 @@ func loadApps(specs []*APISpec) {
 	shouldTrace := trace.IsEnabled()
 	for _, spec := range specs {
 		func() {
+			if strings.Contains(spec.Proxy.TargetURL, "h2c://") {
+				spec.Protocol = "h2c"


should we check if spec.Protocol is already set? If for instance spec.Protocol=http and target is h2c is that allowed? If not then spec is not valid we should emit an error or something.

@gernest if http is set, and the target contains "h2c://" then it has major precedence, hence we set h2c as protocol. Also, I recall that from the very beggining we did this in this way due that FE doesn't have a protocol option h2c. At this point we infer the protocol from the target url

gernest · 2021-01-05T07:46:33Z

gateway/proxy_muxer.go

-					w: h.(*handleWrapper),
-					h: h2c.NewHandler(h, &http2.Server{}),
-				}
+			// by default enabling h2c by wrapping handler in h2c. This ensures all features including tracing work


This is a very big assumptions. By default we provide http proxy, so h2c,tcp etc are special case. You explicitly set protocol to h2c inside loadApps why not check it here?. I see this will be a major source of headache.

@gernest hi, can you elaborate regarding why it could be a major source of headache? currently by enabling h2c by default we're looking to simplify the process the most that we can

@sredxny
By design http2 is supposed to be served over TLS, h2c is an edge case/optional that is not supported by major browsers.

The only use case for h2c is when using gRPC without tls, which is also an edge case. Why do we make edge cases default behaviour? Like I said before http/http2 is the only thing we should take for granted, we should let our customers to choose anything else

Please see this stackoverflow answer on why http2 should always be on TLS https://stackoverflow.com/a/46789195

currently by enabling h2c by default we're looking to simplify the process the most that we can

What process are you simplifying? A process of choosing protocol to use? Why this protocol: "h2c" is not simple?

protocol field require you to have separate gateway port, while in general there is no reasons for this.

So the issue here is to remove the need for separate port?

In general yes. The whole idea of h2c that it works though standard http protocol, and if special header found, it enable special mode which kinda emulate http2 by using http as transport.

gateway/proxy_muxer.go

sredxny

changes done

gateway/proxy_muxer.go

tbuchaillot

Looks good!

matiasinsaurralde

LGTM

matiasinsaurralde · 2021-02-17T15:48:52Z

/release to release-3-lts

tykbot · 2021-02-17T15:49:00Z

Working on it! Note that it can take a few minutes.

* fix grpc proxy connection * clean and format code * clean code * remove h2c config from gateway config * replace h2c protocol from url and set it as http * removed config enable_h2c at api level and infer it from the target url which should contains h2c:// * update grpc test * gofmt gw file * enabled h2c by default to http servers * remove duplicated registeering of healthcheck Co-authored-by: tbuchaillot <tombuchaillot89@gmail.com> (cherry picked from commit 5c8870a)

tykbot · 2021-02-17T15:49:22Z

@matiasinsaurralde Succesfully merged 5c8870a53bca583556e7c04aeb4d31eee999187a to release-3-lts branch.

matiasinsaurralde · 2021-02-17T15:49:50Z

/release to release-3

tykbot · 2021-02-17T15:50:38Z

Working on it! Note that it can take a few minutes.

* fix grpc proxy connection * clean and format code * clean code * remove h2c config from gateway config * replace h2c protocol from url and set it as http * removed config enable_h2c at api level and infer it from the target url which should contains h2c:// * update grpc test * gofmt gw file * enabled h2c by default to http servers * remove duplicated registeering of healthcheck Co-authored-by: tbuchaillot <tombuchaillot89@gmail.com> (cherry picked from commit 5c8870a)

tykbot · 2021-02-17T15:50:59Z

@matiasinsaurralde Succesfully merged 5c8870a53bca583556e7c04aeb4d31eee999187a to release-3 branch.

sredxny added 9 commits October 27, 2020 09:47

fix grpc proxy connection

83fce3d

clean and format code

0fbf321

clean code

78894a3

remove h2c config from gateway config

af9b479

replace h2c protocol from url and set it as http

fd11f11

removed config enable_h2c at api level and infer it from the target u…

d52ce80

…rl which should contains h2c://

update grpc test

b124558

gofmt gw file

96429aa

enabled h2c by default to http servers

b94c9aa

sredxny changed the title ~~Tt 968 enabled h2c by default~~ TT-968 enabled h2c by default Nov 4, 2020

sredxny marked this pull request as ready for review November 6, 2020 02:11

sredxny requested review from gernest, matiasinsaurralde, buger and tbuchaillot November 6, 2020 02:12

matiasinsaurralde approved these changes Jan 5, 2021

View reviewed changes

gernest suggested changes Jan 5, 2021

View reviewed changes

master merge

e2260fc

sredxny requested review from gernest and matiasinsaurralde February 17, 2021 13:08

gernest approved these changes Feb 17, 2021

View reviewed changes

gernest suggested changes Feb 17, 2021

View reviewed changes

gateway/proxy_muxer.go Outdated Show resolved Hide resolved

remove duplicated registeering of healthcheck

1aa0e4b

sredxny commented Feb 17, 2021

View reviewed changes

gateway/proxy_muxer.go Outdated Show resolved Hide resolved

tbuchaillot approved these changes Feb 17, 2021

View reviewed changes

matiasinsaurralde approved these changes Feb 17, 2021

View reviewed changes

gregdelhon merged commit 5c8870a into master Feb 17, 2021

gregdelhon deleted the TT-968-enabled-h2c-by-default branch February 17, 2021 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TT-968 enabled h2c by default #3380

TT-968 enabled h2c by default #3380

sredxny commented Nov 4, 2020 •

edited

matiasinsaurralde left a comment

gernest Jan 5, 2021

buger Feb 16, 2021

gernest Feb 16, 2021

buger Feb 16, 2021 •

edited

gernest Jan 5, 2021

sredxny Feb 16, 2021 •

edited

gernest Jan 5, 2021

sredxny Feb 15, 2021

gernest Feb 16, 2021

buger Feb 16, 2021

gernest Feb 16, 2021

buger Feb 16, 2021

sredxny left a comment

tbuchaillot left a comment

matiasinsaurralde left a comment

matiasinsaurralde commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

matiasinsaurralde commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

TT-968 enabled h2c by default #3380

TT-968 enabled h2c by default #3380

Conversation

sredxny commented Nov 4, 2020 • edited

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

matiasinsaurralde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buger Feb 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sredxny Feb 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sredxny left a comment

Choose a reason for hiding this comment

tbuchaillot left a comment

Choose a reason for hiding this comment

matiasinsaurralde left a comment

Choose a reason for hiding this comment

matiasinsaurralde commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

matiasinsaurralde commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

tykbot bot commented Feb 17, 2021

sredxny commented Nov 4, 2020 •

edited

buger Feb 16, 2021 •

edited

sredxny Feb 16, 2021 •

edited