[proxy] Pre-flight requests to server /api/* endpoints #14897

andrew-farries · 2022-11-23T16:19:15Z

Description

Before handling server /api requests, make a 'pre-flight' request to /api/feature-flags/slow-database (on server) and use the X-Gitpod-Slow-Database header in the response to decide where to send the actual request; either to the regular server service or the new slow-server service.

This allows us to use a feature flag to simulate the performance of these endpoints when they use a higher latency database connection.

Related Issue(s)

Part of #9198

Closes #14960.

How to test

Log in to the preview environment.
Hit https://af-proxy-a5b2f4cfb56.preview.gitpod-dev.com/api/version and inspect the response timings.
Change the targeting rule for the slow_database feature flag (non-production environment) to this (replacing the user id with your user id in the preview):

Wait up to 3 minutes for the configcat client to refresh the feature flag values.
Hit https://af-proxy-a5b2f4cfb56.preview.gitpod-dev.com/api/version and inspect the response timings.

Release Notes

NONE

Documentation

Werft options:

/werft with-local-preview
If enabled this will build install/preview
/werft with-preview
/werft with-slow-database
/werft with-large-vm
/werft with-integration-tests=all
Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

werft-gitpod-dev-com · 2022-11-23T16:19:24Z

started the job as gitpod-build-af-proxy-api-to-slow-server.22 because the annotations in the pull request description changed
(with .werft/ from main)

andrew-farries · 2022-11-23T16:30:07Z

components/proxy/conf/Caddyfile

+					import upstream_headers
+					import upstream_connection
+
+					# required for smooth streaming of terminal logs
+					flush_interval -1


I'd like to be able to replace the body of these reverse_proxy directives with a snippet to reduce duplication but ran into this Caddy bug with nested imports: caddyserver/caddy#4914

Really sad that we have to duplicate 3 times...

💡

Would it work do do something like:

handle_response @slow { ... } # fall-through fast/default case uri strip_prefix /api reverse_proxy server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { import upstream_headers import upstream_connection # required for smooth streaming of terminal logs flush_interval -1 }

That way we only need to duplicate once...?

Looking at this: if we used forward_auth it seems we can even do away with one more level of nesting.

handle @backend { ... forward_auth server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { uri /api/feature-flags/slow-database copy_headers { X-Gitpod-Slow-Database } } handle_response @slow ... reverse_proxy ...

@andrew-farries WDYT?

Will take a look at forward_auth. When I looked before it didn't seem to match our use case, but will look again 👀

Would it work do do something like:
...

This looks like it should work according the Caddy docs on intercepting responses:

I tried a simplified version in this Gist:

https://gist.github.com/andrew-farries/a2ac60d51e6c812ded573720c1ffabf6

but the fallthrough case doesn't work. Caddy docs say:

When a response handler is invoked, the response from the backend is not written to the client, and the configured handle_response route will be executed instead, and it is up to that route to write a response. If the route does not write a response, then request handling will continue with any handlers that are ordered after this reverse_proxy.

but despite the @fast and @none matchers not writing a response, the respond directive after the reverse_proxy directive never gets run.

(You can try it for yourself with caddy run --watch and then http :2015/api/foo)

Another alternative to reduce duplication here would be to have the feature-flags/slow-database endpoint always set the X-Gitpod-Slow-Database response header. It currently only does so on 200 OK responses:

gitpod/components/server/src/feature-flag/featureflag-controller.ts

Lines 24 to 44 in c154c32

protected addSlowDatabaseFeatureFlagHandler(router: express.Router) {

router.get("/slow-database", async (req, res) => {

if (!User.is(req.user)) {

res.sendStatus(401);

return;

}

try {

const flagValue = await getExperimentsClientForBackend().getValueAsync("slow_database", false, {

user: req.user,

});

res.setHeader("X-Gitpod-Slow-Database", flagValue.toString());

res.status(200);

res.end();

} catch (error) {

log.error(`failed to retrieve value of 'slow_database' feature flag: ${error.message}`);

res.status(500);

res.end();

}

});

}

That way we could remove the @headerNotPresent response matcher.

Changed to use forward_auth, copy_headers and request matchers as we discussed in sync today.

This needs #14930 to land first to ensure the api/feature-flags/slow-database endpoint returns 200 OK consistently.

components/proxy/conf/Caddyfile

geropl · 2022-11-25T08:11:24Z

/werft run

👍 started the job as gitpod-build-af-proxy-api-to-slow-server.25
(with .werft/ from main)

geropl · 2022-11-25T10:00:36Z

Currently I get an "Unauthorized", and figured that this needs to be rebased on #14930 (as you mentioned above 😆 )

andrew-farries · 2022-11-25T12:37:22Z

This has been rebased now that #14930 has been merged. Do you want to try again @geropl?

It is working as expected for me.

geropl · 2022-11-25T12:50:07Z

{"@type":"type.googleapis.com/google.devtools.clouderrorreporting.v1beta1.ReportedErrorEvent","serviceContext":{"service":"server","version":"af-proxy-api-to-slow-server.29"},"component":"server","severity":"WARNING","time":"2022-11-25T12:43:39.445Z","message":"websocket path not matching","payload":{}}

I see a lot of these lines being logged, bc we forward websocket Upgrade-requests to the server, but on a rewritten URi.
I think a simple header_up -Upgrade inside of forward_auth would fix this. 👍

geropl

And it works - sloooooowly 😆

Nice work, @andrew-farries ! 🚀

/hold bc I think we should adress #14897 (comment) before we merge

andrew-farries · 2022-11-25T14:36:33Z

see a lot of these lines being logged, bc we forward websocket Upgrade-requests to the server, but on a rewritten URi.
I think a simple header_up -Upgrade inside of forward_auth would fix this. 👍

I'm not sure why we see these errors at all. In the Caddyfile we define two handle blocks, one with a path matcher for api/gitpod:

gitpod/components/proxy/conf/Caddyfile

Lines 207 to 230 in 54d7c00

    
           	@backend_wss { 
        
           		path /api/gitpod 
        
           	} 
        
           	handle @backend_wss { 
        
           		gitpod.sec_websocket_key 
        
           		@slow { 
        
           			header "Sec-WebSocket-Protocol" "slow-database" 
        
           		} 
        
           		@fast { 
        
           			not header "Sec-WebSocket-Protocol" "slow-database" 
        
           		} 
        
           		uri strip_prefix /api 
        
           		reverse_proxy @fast server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { 
        
           			import upstream_headers 
        
           		} 
        
           		reverse_proxy @slow slow-server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { 
        
           			import upstream_headers 
        
           		} 
        
           	}

and the other with a path matcher for api/* headless-logs/*:

gitpod/components/proxy/conf/Caddyfile

Lines 232 to 270 in 54d7c00

    
           	@backend path /api/* /headless-logs/* 
        
           	handle @backend { 
        
           		gitpod.cors_origin { 
        
           			base_domain {$GITPOD_DOMAIN} 
        
           		} 
        
           		forward_auth server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { 
        
           			uri /feature-flags/slow-database 
        
           			header_up -Upgrade 
        
           			copy_headers X-Gitpod-Slow-Database 
        
           		} 
        
           		@slow { 
        
           			header X-Gitpod-Slow-Database "true" 
        
           		} 
        
           		@fast { 
        
           			not header X-Gitpod-Slow-Database "true" 
        
           		} 
        
           		uri strip_prefix /api 
        
           		reverse_proxy @fast server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { 
        
           			import upstream_headers 
        
           			import upstream_connection 
        
           			# required for smooth streaming of terminal logs 
        
           			flush_interval -1 
        
           		} 
        
           		reverse_proxy @slow slow-server.{$KUBE_NAMESPACE}.{$KUBE_DOMAIN}:3000 { 
        
           			import upstream_headers 
        
           			import upstream_connection 
        
           			# required for smooth streaming of terminal logs 
        
           			flush_interval -1 
        
           		} 
        
           	}

AIUI The api/gitpod path is the destination for all websocket requests, and as it is a more specific path than api/*, the /api/gitpod handle block will be the one used for incoming Upgrade requests (see Caddy docs for 'handle'). So why should anything we add to the handle block for api/* have an effect on websocket connections?

andrew-farries · 2022-11-25T16:41:15Z

After a sync today we determined that the cause of the websocket errors in the server log was websocket requests arriving at api/v1 not being handled by the handle @backend_wss block but falling through to the modified /api/* handler block.

#14974 should resolve this.

andrew-farries · 2022-11-28T13:26:54Z

Using the version of proxy built by #14974 shows that the websocket errors are no longer present in the server logs 👍 .

Before serving /api requests, make a 'pre-flight' request to `/api/feature-flags/slow-database` and use the `X-Gitpod-Slow-Database` header in the response to decide where to send the actual request; either to the regular `server` service or the new `slow-server` service.

andrew-farries · 2022-11-28T14:47:47Z

/unhold

andrew-farries added the do-not-merge/hold label Nov 23, 2022

andrew-farries requested a review from a team November 23, 2022 16:19

roboquat added the release-note-none label Nov 23, 2022

roboquat added the size/M label Nov 23, 2022

github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 23, 2022

andrew-farries commented Nov 23, 2022

View reviewed changes

roboquat added size/S and removed size/M labels Nov 24, 2022

andrew-farries mentioned this pull request Nov 24, 2022

[server] Change response codes returned from api/feature-flags/slow-database endpoint #14930

Merged

4 tasks

geropl reviewed Nov 25, 2022

View reviewed changes

components/proxy/conf/Caddyfile Show resolved Hide resolved

andrew-farries force-pushed the af/proxy-api-to-slow-server branch 2 times, most recently from edb4963 to 9e4168a Compare November 25, 2022 11:31

geropl approved these changes Nov 25, 2022

View reviewed changes

andrew-farries mentioned this pull request Nov 25, 2022

[proxy] Match /api/v1 in the @backend_wss matcher #14974

Merged

4 tasks

andrew-farries force-pushed the af/proxy-api-to-slow-server branch from 54d7c00 to 9e4168a Compare November 28, 2022 11:37

andrew-farries force-pushed the af/proxy-api-to-slow-server branch from 9e4168a to 00ec6a2 Compare November 28, 2022 14:22

roboquat removed the do-not-merge/hold label Nov 28, 2022

roboquat merged commit a7d5bb4 into main Nov 28, 2022

roboquat deleted the af/proxy-api-to-slow-server branch November 28, 2022 14:47

roboquat added the deployed: webapp Meta team change is running in production label Nov 29, 2022

roboquat added the deployed Change is completely running in production label Nov 29, 2022

andrew-farries mentioned this pull request Nov 30, 2022

[proxy] Pre-flight server authentication related endpoints to add additional latency #15078

Merged

5 tasks

	protected addSlowDatabaseFeatureFlagHandler(router: express.Router) {
	router.get("/slow-database", async (req, res) => {
	if (!User.is(req.user)) {
	res.sendStatus(401);
	return;
	}

	try {
	const flagValue = await getExperimentsClientForBackend().getValueAsync("slow_database", false, {
	user: req.user,
	});
	res.setHeader("X-Gitpod-Slow-Database", flagValue.toString());
	res.status(200);
	res.end();
	} catch (error) {
	log.error(`failed to retrieve value of 'slow_database' feature flag: ${error.message}`);
	res.status(500);
	res.end();
	}
	});
	}

[proxy] Pre-flight requests to server /api/* endpoints #14897

[proxy] Pre-flight requests to server /api/* endpoints #14897

Uh oh!

Conversation

andrew-farries commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

How to test

Release Notes

Documentation

Werft options:

Uh oh!

werft-gitpod-dev-com bot commented Nov 23, 2022

Uh oh!

andrew-farries Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

geropl Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

geropl Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-farries Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-farries Nov 24, 2022 • 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!

andrew-farries Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

andrew-farries Nov 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geropl commented Nov 25, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geropl commented Nov 25, 2022

Uh oh!

andrew-farries commented Nov 25, 2022

Uh oh!

geropl commented Nov 25, 2022

Uh oh!

geropl left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-farries commented Nov 25, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-farries commented Nov 25, 2022

Uh oh!

andrew-farries commented Nov 28, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrew-farries commented Nov 28, 2022

Uh oh!

Uh oh!

andrew-farries commented Nov 23, 2022 •

edited

Loading

andrew-farries Nov 24, 2022 •

edited

Loading

geropl commented Nov 25, 2022 •

edited by werft-gitpod-dev-com bot

Loading

andrew-farries commented Nov 25, 2022 •

edited by werft-gitpod-dev-com bot

Loading

andrew-farries commented Nov 28, 2022 •

edited by werft-gitpod-dev-com bot

Loading