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.

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

feat(header forwarding): add HeaderForwarder and plumbing #507

Merged
merged 9 commits into from
Oct 18, 2024

Conversation

potterbm-cb
Copy link
Contributor

Motivation

In order to support some common use cases, Mesh servers need the ability to pass request headers through to the native nodes, and to return headers from native node requests back to the caller.

Use cases:

  • passing a client ID header to the nodes — this client ID shouldn't originate with the Mesh server since it may have many different callers, so it should be passed through to the native node requests
  • sticky session cookies — to support sticky sessions on native nodes, AWS uses cookies. Cookies are managed by the Cookie and Set-Cookie headers, which will need to be passed from the caller to the native node, and returned from the native node ALB to the caller

Solution

The HeaderForwarder type extracts headers from potentially many native node requests and adds them to a Mesh response. HeaderForwarder can be injected into native node network requests as an http.RoundTripper, and can be applied to Mesh servers as middleware

A few key features:

  • implements http.RoundTripper so that it can be used to create an http.Client or other client
  • implements a common middleware pattern: func(http.Handler) http.Handler
  • assigns every request a unique ID (which is used to associate headers from native node responses back to Mesh request/responses)
  • allows forwarding only specific headers

Open questions

  1. Currently there is no proof-of-concept for non-geth chains, so it remains to be seen if this architecture will work well for cosmos chains or other.

@cb-heimdall
Copy link

cb-heimdall commented Oct 17, 2024

✅ Heimdall Review Status

Requirement Status More Info
Reviews 3/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

}

// shouldRememberHeaders is called to determine if response headers should be remembered for a given request.
// Response headers will only be remembered if the request does not contain all of the interesting

Choose a reason for hiding this comment

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

What if a sticky session cookie is provided in the request but it expires and a new sticky session cookie comes in the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

technically since the Cookie and Set-Cookie header are different, headers for requests with cookies will always be forwarded.

If that's eventually not what we want we'd have to write custom logic to associate Cookie and Set-Cookie together for request/response (and storing/accessing) respectively

@jingweicb
Copy link
Contributor

Hi @potterbm-cb , some CI checks failed, can you try to fix them, same for the other pr.

Copy link

@marklandgrebe-cb marklandgrebe-cb left a comment

Choose a reason for hiding this comment

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

lgtm, we should work on covering this repo with unit tests

@potterbm-cb potterbm-cb reopened this Oct 18, 2024
@potterbm-cb
Copy link
Contributor Author

closed and reopened PR to try to get the CI unstuck ("Minimum required reviews" wasn't registering the reviews)

@potterbm-cb potterbm-cb merged commit bad12fa into master Oct 18, 2024
19 checks passed
@potterbm-cb potterbm-cb deleted the bryan/add-header-forwarding branch October 18, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants