Skip to content

[public-api-server] Forward Origin header where provided - #16405

Merged
roboquat merged 2 commits into
mainfrom
gpl/papi-forward
Feb 15, 2023
Merged

roboquat merged 2 commits into
mainfrom
gpl/papi-forward

Conversation

@geropl

@geropl geropl commented Feb 15, 2023 •

Copy link
Copy Markdown
Member

Description

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-slow-database
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@geropl
geropl requested a review from a team February 15, 2023 06:35
@werft-gitpod-dev-com

Copy link
Copy Markdown

started the job as gitpod-build-gpl-papi-forward.2 because the annotations in the pull request description changed
(with .werft/ from main)

@geropl

geropl commented Feb 15, 2023

Copy link
Copy Markdown
Member Author

/hold for testing in preview env

@github-actions github-actions Bot added the team: webapp Issue belongs to the WebApp team label Feb 15, 2023
@geropl

geropl commented Feb 15, 2023 •

Copy link
Copy Markdown
Member Author

/werft run

👍 started the job as gitpod-build-gpl-papi-forward.5
(with .werft/ from main)

🐌 DB...

Comment thread components/public-api-server/pkg/auth/context.go Outdated
Comment thread components/public-api-server/pkg/proxy/conn.go Outdated
@easyCZ

easyCZ commented Feb 15, 2023 •

Copy link
Copy Markdown
Member

@geropl Added the context-style approach.

  1. Middleware always extracts the Origin
  2. The cached client constructor always populates it with the extracted value
  3. Also udpated the cache key to include the origin, to prevent one origin being used to populate the cache, and another to (ab)use it

)

func ToContext(ctx context.Context, origin string) context.Context {
return context.WithValue(ctx, originContextKey, origin)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I like this!

func (i *Interceptor) WrapUnary(next connect.UnaryFunc) connect.UnaryFunc {
return connect.UnaryFunc(func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) {
if req.Spec().IsClient {
req.Header().Add("Origin", FromContext(ctx))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand what this branch is for: Is this interceptor both usable for servers and clients (e.g., when forwarding upstream)? 🤔

@easyCZ easyCZ Feb 15, 2023 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. The way the interceptor is implemented (as per the interceptor inteface) is that you could write a single interceptor for both clients, and servers.

Here, we don't focus on the client version too much, but we do "make it work" by honouring the value that was on the context.

}

func (p *ConnectionPool) Get(ctx context.Context, token auth.Token) (gitpod.APIInterface, error) {
cached, found := p.cache.Get(token)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@geropl

geropl commented Feb 15, 2023 •

Copy link
Copy Markdown
Member Author

@easyCZ Thank you, I like it (and tested and works) ✔️

Would you go ahead and approve your own code? 🙃

@easyCZ

easyCZ commented Feb 15, 2023 •

Copy link
Copy Markdown
Member

Would you go ahead and approve your own code? 🙃

Care to create a few more "blank check" PRs for me?

@easyCZ

easyCZ commented Feb 15, 2023

Copy link
Copy Markdown
Member

/unhold

@roboquat
roboquat merged commit 0a7ca4c into main Feb 15, 2023
@roboquat
roboquat deleted the gpl/papi-forward branch February 15, 2023 14:39
@roboquat roboquat added the deployed: webapp Meta team change is running in production label Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/L team: webapp Issue belongs to the WebApp team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants