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

Use pre-defined proxy if RequestMeta contains proxyId #187

Closed
wants to merge 1 commit into from

Conversation

reki2000
Copy link

Proof-of-concept implementation for #180.

Client API for newRequest is changed - need additional meta parameter for 'proxyId' (optional). Clients should set this value by using 'X-OCTOPARTS-PROXY-ID' from HTTP header. This value is also passed to service provider so that the service provider pass it to other subsequencial request via Octoparts.

@lloydmeta
Copy link
Contributor

Nice job putting together a PoC 👍

A few thoughts:

  • Can you move this to a fork (see Contributor guidelines)
  • Proxy should probably become it's own class and persisted into the DB (likely in its own separate table) rather than be something that needs to be set via the config file. That way no new code or resets need to happen for adding new proxies (goes along with the design philosophy of Octoparts)
    • An HttpPartConfig can then have a Option[ProxyConfig] or something similar
  • I think asking for an invalid proxy should not fail silently. For example, asking for an invalid part id (one that does not exist) prevents the part from even working at all; asking for a proxy by an invalid id, it follows, should also fail, otherwise there could be lots of confusion

@xevix would like to hear your thoughts as well.

@@ -98,7 +98,7 @@ trait HttpPartRequestHandler extends Handler {
AggregateRequestIdHeader -> partRequestInfo.requestMeta.id,
PartRequestIdHeader -> partRequestInfo.partRequestId,
PartIdHeader -> partRequestInfo.partRequest.partId
)
) ++ partRequestInfo.requestMeta.proxyId.map { case s => ProxyIdHeader -> s }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a case here.

We should also make sure to test that the header is being properly forwarded (and left out if no proxy) in subsequent requests to backends from Octoparts.

Also, since this method is called buildTracingHeaders, we should probably do proxy id header adding in another method or rename this method.

@reki2000
Copy link
Author

I made another PR from my fork as commedted by @lloydmeta. Closing this PR.

@lloydmeta
Copy link
Contributor

Closing for realsies.

@lloydmeta lloydmeta closed this Dec 16, 2015
@lloydmeta lloydmeta deleted the feature/180-proxy-override branch December 16, 2015 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants