Fix traefik annotations as they are not working properly #31

aleks-fofanov · 2020-06-18T11:55:08Z

Signed-off-by: Aleksandr Fofanov avl.fofanov@gmail.com

Description

Please see the detailed description of the issue and context in #30

I've tested the changes to the annotation with the following function ingresses:

Same as in the issue mentioned above

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: podinfo
  namespace: dev-openfaas
spec:
  function: podinfo
  domain: podinfo.local.dev
  ingressType: "traefik"
  bypassGateway: false
  tls:
    enabled: false

Urls tested:

Function ingress with custom path

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: podinfo
  namespace: dev-openfaas
spec:
  function: podinfo
  domain: podinfo.local.dev
  ingressType: "traefik"
  path: "/v1/(*.)"
  bypassGateway: false
  tls:
    enabled: false

Urls tested:

Motivation and Context

I have raised an issue to propose this change (required)

Fixes #30

How Has This Been Tested?

I've added the test that verifies that operator produce correct annotations for Traefik ingresses.
I also tested the changes to the annotations by following the steps described in #30

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Impact to existing users

This change shouldn't break anything in existing deployments.

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have signed-off my commits with git commit -s
I have added tests to cover my changes.
All new and existing tests passed.

aleks-fofanov · 2020-06-18T11:55:59Z

@alexellis Please advise whether documentation should be updated and where exactly, I'm not sure about the right place. Thank you.

alexellis · 2020-06-18T12:05:42Z

Hi thanks for the PR and issue.

What kind of URLs have you tested for this fix? Have you tried subpaths and files? I.e. when serving a folder structure or varying HTTP paths with a suffix

alexellis

Adding a comment. You will find any docs we have in the readme.

alexellis · 2020-06-18T12:06:22Z

pkg/controller/controller.go

@@ -513,7 +513,7 @@ func makeAnnotations(fni *faasv1.FunctionIngress) map[string]string {
 			annotations["zalando.org/skipper-filter"] = `setPath("/function/` + fni.Spec.Function + `")`
 			break
 		case "traefik":
-			annotations["traefik.ingress.kubernetes.io/rewrite-target"] = "/function/" + fni.Spec.Function + "/$1"


Why would you say that $1 is no longer required?

Because traefik internally converts the annotations to the following routing rule: PathPrefix:/;ReplacePathRegex: ^/(.*) /function/podinfo/$1$1
Note the two replacements ($1). So all request will be forwarded to the function with incorrect path (older versions of Traefik, except for the root path) or won't be routed at all because of indefinite redirects (latest versions of Traefik).

derek · 2020-06-18T12:35:39Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

aleks-fofanov · 2020-06-18T12:37:42Z

What kind of URLs have you tested for this fix? Have you tried subpaths and files? I.e. when serving a folder structure or varying HTTP paths with a suffix

@alexellis I tested root path, subpaths and files too.

I also updated the docs.

alexellis · 2020-06-18T12:40:03Z

Please can you give an example of the FNI and the paths tested for it in the description of the PR?

aleks-fofanov · 2020-06-18T12:54:50Z

@alexellis Added.

alexellis · 2020-06-18T12:58:21Z

This is looking great. Almost ready to merge, I'd like to see a FunctionIngress that uses a path prefix now. See here: https://github.com/openfaas-incubator/ingress-operator/blob/master/README.md#schema

The idea is that you can say this function is mounted on the domain at /api/blog/v1

alexellis · 2020-06-18T13:29:40Z

Example for Nginx (I suspect you may want to remove the (*.) for Traefik, but you may know better than I do with regards to that one.

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: webpage
  namespace: openfaas
spec:
  domain: "webpage.myfaas.club"
  function: "webpage"
  ingressType: "nginx"
  path: "/v1/web/(*.)" # Optionally set a path for the domain i.e. nodeinfo.myfaas.club/v1/profiles/

This FNI would mount the webpage function at webpage.myfaas.club/v1/web/ so that anything sent there would appear as / in the webpage function.

derek · 2020-06-18T13:51:49Z

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

aleks-fofanov · 2020-06-18T13:59:48Z

@alexellis Yes, thanks for the pointer to FNIs with path. Totally forgot about this feature. I fixed the routing expression for this case, tested the changes and updated the description.
Related: It looks to me that Ingress Operator CRD in faas-netes chart is not up-to-date with the actual spec? There is no path attribute the in the spec.

alexellis · 2020-06-18T14:04:46Z

The chart for the inlets-operator moved into the faas-netes repo here ->

https://github.com/openfaas/faas-netes/tree/master/chart/openfaas/templates

We tend to install it like: arkade install --ingress-operator which generates a command like:

Command: /Users/alex/.arkade/bin/helm3/helm [upgrade --install openfaas openfaas/openfaas --namespace openfaas --values /var/folders/p7/ptxtv_pd3n12fd6mc5wjhk1h0000gn/T/charts/openfaas/values.yaml --set clusterRole=false --set openfaasImagePullPolicy=IfNotPresent --set faasnetes.imagePullPolicy=Always --set gateway.replicas=1 --set ingressOperator.create=true --set queueWorker.replicas=1 --set basic_auth=true --set gateway.directFunctions=true --set operator.create=false --set basicAuthPlugin.replicas=1 --set queueWorker.maxInflight=1 --set serviceType=NodePort]

Can you add path as a separate PR into the faas-netes repo?

I also noticed that you have some issues with the last few commits, we can't accept a merge commit in a PR, so can you do a rebase? git rebase -i HEAD~N where N is the amount of commits in total.

Pick s for squash and then do a force push.

alexellis · 2020-06-18T14:06:24Z

Despite the CRD validation not specifying path as a field, it is accepted and used:

Expand to view the logs:

space-mini:vagrant alex$ kubectl apply -f -
apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
name: webpage
namespace: openfaas
spec:
domain: "webpage.myfaas.club"
function: "webpage"
ingressType: "nginx"
path: "/v1/web/(.)" # Optionally set a path for the domain i.e. nodeinfo.myfaas.club/v1/profiles/
functioningress.openfaas.com/webpage created
space-mini:vagrant alex$
space-mini:vagrant alex$ kubectl get functioningress.openfaas.com/webpage -o yaml
Error from server (NotFound): functioningresses.openfaas.com "webpage" not found
space-mini:vagrant alex$ kubectl get functioningress.openfaas.com/webpage -o yaml -n openfaas
apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"openfaas.com/v1alpha2","kind":"FunctionIngress","metadata":{"annotations":{},"name":"webpage","namespace":"openfaas"},"spec":{"domain":"webpage.myfaas.club","function":"webpage","ingressType":"nginx","path":"/v1/web/(.)"}}
creationTimestamp: "2020-06-18T14:05:01Z"
generation: 1
managedFields:

apiVersion: openfaas.com/v1alpha2
fieldsType: FieldsV1
fieldsV1:
f:metadata:
f:annotations:
.: {}
f:kubectl.kubernetes.io/last-applied-configuration: {}
f:spec:
.: {}
f:domain: {}
f:function: {}
f:ingressType: {}
f:path: {}
manager: kubectl
operation: Update
time: "2020-06-18T14:05:01Z"
name: webpage
namespace: openfaas
resourceVersion: "90086"
selfLink: /apis/openfaas.com/v1alpha2/namespaces/openfaas/functioningresses/webpage
uid: 2c8020d3-e8a6-4520-90f9-c8731730750a
spec:
domain: webpage.myfaas.club
function: webpage
ingressType: nginx
path: /v1/web/(*.)
space-mini:vagrant alex$

…s except root Signed-off-by: Aleksandr Fofanov <avl.fofanov@gmail.com> Update README.md Signed-off-by: Aleksandr Fofanov <avl.fofanov@gmail.com> Fix an issue with incorrect path prefix for traefik in case when custom path defined Signed-off-by: Aleksandr Fofanov <avl.fofanov@gmail.com> Fix an issue with incorrect path prefix for traefik in case when custom path defined

aleks-fofanov · 2020-06-18T14:16:28Z

@alexellis Ok, squashed all commits in one.

Can you add path as a separate PR into the faas-netes repo?

Yes, will try to add this on the next week.

alexellis · 2020-06-18T14:24:06Z

For the separate PR, can you just copy the code from my new PR? #33

It should be very quick to copy/paste from the new openfaas.com_functioningress.yaml file into the chart template in faas-netes.

Alex

aleks-fofanov · 2020-06-18T14:49:06Z

@alexellis openfaas/faas-netes#656

alexellis · 2020-06-18T14:53:39Z

That's merged now. How are we looking for the path usecase when specific in the FunctionIngress?

alexellis · 2020-06-18T14:54:55Z

Very much a pleasure having this quality of contribution. Thank you again.

alexellis · 2020-06-18T15:32:19Z

Oh my goodness, I clearly didn't catch this, sorry.

aleks-fofanov · 2020-06-18T15:35:08Z

Sorry @alexellis , all my fault. Will pay more attention to this in the future.

derek bot added the new-contributor label Jun 18, 2020

alexellis reviewed Jun 18, 2020

View reviewed changes

derek bot added the no-dco label Jun 18, 2020

aleks-fofanov force-pushed the master branch from d1242ca to adbb476 Compare June 18, 2020 12:36

derek bot removed the no-dco label Jun 18, 2020

derek bot added the no-dco label Jun 18, 2020

aleks-fofanov force-pushed the master branch from 4b3fcac to 556de02 Compare June 18, 2020 14:12

derek bot removed the no-dco label Jun 18, 2020

alexellis mentioned this pull request Jun 18, 2020

The Path field from FunctionIngress' Spec is missing from the YAML CRD #32

Closed

alexellis merged commit a88feb0 into openfaas:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix traefik annotations as they are not working properly #31

Fix traefik annotations as they are not working properly #31

aleks-fofanov commented Jun 18, 2020 •

edited

Loading

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis left a comment

alexellis Jun 18, 2020

aleks-fofanov Jun 18, 2020

derek bot commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020 •

edited

Loading

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

derek bot commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

Fix traefik annotations as they are not working properly #31

Fix traefik annotations as they are not working properly #31

Conversation

aleks-fofanov commented Jun 18, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Impact to existing users

Checklist:

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis left a comment

Choose a reason for hiding this comment

alexellis Jun 18, 2020

Choose a reason for hiding this comment

aleks-fofanov Jun 18, 2020

Choose a reason for hiding this comment

derek bot commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020 • edited Loading

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

derek bot commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

alexellis commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020

aleks-fofanov commented Jun 18, 2020 •

edited

Loading

aleks-fofanov commented Jun 18, 2020 •

edited

Loading