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

Break out proxying code from all providers #9

Closed
alexellis opened this issue Sep 19, 2018 · 12 comments
Closed

Break out proxying code from all providers #9

alexellis opened this issue Sep 19, 2018 · 12 comments

Comments

@alexellis
Copy link
Member

alexellis commented Sep 19, 2018

The proxying code is repeated in faas-swarm, faas-netes, openfaas-operator and possibly faas-fargate/faas-nomad.

At least the code for the first three should be moved here.

The proxying code is rarely used apart from when direct_functions is set to false. For some reason Capital One need this feature, so we should move the code into one place for maintenance. Each provider will then be able to utilize the package.

A similar move was made for the basic auth middleware that I wrote.

In addition the path parsing/trimming code that passes a HTTP Path to functions needs to be ported over so that functions are not invoked with a PATH like /function/<name>/param1/param2 but with /param1/param2 directly. This code resides in the gateway currently and so it may make sense to also refactor it into a package here.

Summary/tasks

To test turn off direct_functions on your API gateway

  • Refactor/move out the proxying code to a common type package here. Unit tests must be copied/added
  • Add path trimming support to match the gateway's direct_functions approach

Once the above is done and we have a release of faas-provider:

  • Remove code from each provider and have them vendor the package here
@ivanayov
Copy link

I can work on this

@alexellis
Copy link
Member Author

This is a very big task. Do you think there is a part up you could without committing to it all?

@alexellis
Copy link
Member Author

Derek add label: size/xxl

@derek derek bot added the size/xxl label Sep 21, 2018
@ivanayov
Copy link

As a start point I can move faas-swarm proxy code here and test on Swarm with faas-provider.

After having this, the work can be separated on those steps/issues:

Step 2: Make refactoring to merge faas-netes and openfaas-operator proxy in a separate package and test both using faas-provider. This is before step 3, as they are similar and safer to merge together.

Step 3 (advanced): Make refactoring to merge swarm and kubernetes proxy

@LucasRoesler
Copy link
Member

Derek assign: me

LucasRoesler added a commit to LucasRoesler/faas-provider that referenced this issue Oct 7, 2018
**What**
- Add standardized proxy handler based on the implementations for swarm
and kubernetes

**Why**
- Reduce duplicated code amoung the various providers
- Ensure the proxy behavior is consistent
- Addresses openfaas#9

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
LucasRoesler added a commit to LucasRoesler/faas-provider that referenced this issue Oct 7, 2018
**What**
- Add standardized proxy handler based on the implementations for swarm
and kubernetes

**Why**
- Reduce duplicated code amoung the various providers
- Ensure the proxy behavior is consistent
- Addresses openfaas#9

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
@LucasRoesler
Copy link
Member

I noticed an implementation detail in faas-swarm, you can specifiy the function name via the X-Function header

https://github.com/openfaas/faas-swarm/blob/6c10aa50f711139ec79bffbe938b2e2d366e3dc1/handlers/proxy.go#L55

But this doesn't exist in either k8s implementation

https://github.com/openfaas/faas-netes/blob/5dadd402921554fd846be8fdd97cf79c9a7ad9fc/handlers/proxy.go#L21

I noticed in the faas repo that there is a test that notes the implementation is being deprecated

https://github.com/openfaas/faas/blob/28c9ccd0aa4f1de7b00e919b4d140fdd1eaf1aa7/gateway/tests/integration/routes_test.go#L106

Can I remove it from this implementation?

alexellis pushed a commit that referenced this issue Nov 4, 2018
**What**
- Add standardized proxy handler based on the implementations for swarm
and kubernetes

**Why**
- Reduce duplicated code amoung the various providers
- Ensure the proxy behavior is consistent
- Addresses #9

Signed-off-by: Lucas Roesler <roesler.lucas@gmail.com>
@alexellis
Copy link
Member Author

Hi @LucasRoesler I don't believe anyone is using X-Function, feel free to deprecate it and if we find it's important for users we can reinstate.

@LucasRoesler
Copy link
Member

The X-Function behavior was removed #11

@alexellis
Copy link
Member Author

Can this issue be closed? @LucasRoesler

@LucasRoesler
Copy link
Member

Unfortunately, no. This was applied two faas-swarm in openfaas/faas-swarm#39

But faas-netes is not using the faas-provider proxy yet

@alexellis
Copy link
Member Author

How are we doing with this now? @LucasRoesler

@LucasRoesler
Copy link
Member

faas-netes now uses the provider, as of this commit openfaas/faas-netes@c1ba865 4 months ago

the operator is using the provider as of this commit https://github.com/openfaas/openfaas-operator/commit/156e27eac06d1db38c977ddcb41de07f45453a17 2 months ago

I thinik this means we can close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants