Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Feb 15, 2023
Merged

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

merged 2 commits into from
Feb 15, 2023
+195 −35

Conversation

geropl
Copy link
Member

@geropl geropl commented Feb 15, 2023 •
edited
Loading

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

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

@geropl
Copy link
Member Author

geropl commented Feb 15, 2023

/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
Copy link
Member Author

geropl commented Feb 15, 2023 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

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

🐌 DB...

@roboquat roboquat added size/L and removed size/M labels Feb 15, 2023
@easyCZ
Copy link
Member

easyCZ commented Feb 15, 2023 •
edited
Loading

@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

geropl
geropl commented Feb 15, 2023
View reviewed changes
components/public-api-server/pkg/origin/context.go
)

func ToContext(ctx context.Context, origin string) context.Context {
return context.WithValue(ctx, originContextKey, origin)
Copy link
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!

geropl
geropl commented Feb 15, 2023
View reviewed changes
components/public-api-server/pkg/origin/middleware.go
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
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)? 🤔

Copy link
Member

@easyCZ easyCZ Feb 15, 2023 •
edited
Loading

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.

geropl
geropl commented Feb 15, 2023
View reviewed changes
components/public-api-server/pkg/proxy/conn.go

// cache stores token to connection mapping
cache *lru.Cache
}

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

Choose a reason for hiding this comment

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

💯

@geropl
Copy link
Member Author

geropl commented Feb 15, 2023 •
edited
Loading

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

Would you go ahead and approve your own code? 🙃

@easyCZ
Copy link
Member

easyCZ commented Feb 15, 2023 •
edited by werft-gitpod-dev-com bot
Loading

Would you go ahead and approve your own code? 🙃

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

@easyCZ
Copy link
Member

easyCZ commented Feb 15, 2023

/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
@roboquat roboquat added the deployed Change is completely running in production label Feb 16, 2023
@GoVulnBot GoVulnBot mentioned this pull request Mar 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easyCZ easyCZ easyCZ approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geropl @easyCZ @roboquat