Support request for Helm Chart's ingress section

blaargh commented 3 months ago

Setting up the ingress values leads to an incomplete (yet not broken) ingress definition. It is completely missing the http path options, rendering the current implementation useless, as every update of the upstream chart requires manual edit of the resulting ingress resource.

Expected Behaviour

The ingress definition should be complete, given the values required.

Current Behaviour

The ingress definition is incomplete.

spec:
  {{- if .Values.ingress.ingressClassName }}
  ingressClassName: {{ .Values.ingress.ingressClassName }}
  {{- end }}
  rules:

{{ toYaml .Values.ingress.hosts | indent 2 }}
  {{- if .Values.ingress.tls }}
  tls:
{{ toYaml .Values.ingress.tls | indent 4 }}
  {{- end -}}

toYaml of the .hosts path in values.yaml is not enough to have a proper ingress definition:

ingress:
  enabled: false
  ## For k8s >= 1.18 you need to specify the pathType
  ## See https://kubernetes.io/blog/2020/04/02/improvements-to-the-ingress-api-in-kubernetes-1.18/#better-path-matching-with-path-types
  #pathType: ImplementationSpecific

  # Used to create Ingress record (should be used with exposeServices: false).
  hosts:
    - host: gateway.openfaas.local  # Replace with gateway.example.com if public-facing
      serviceName: gateway
      servicePort: 8080
      path: /

it is missing:

http:
  paths:
    - path: /
       pathType: Prefix
       backend:
         service:
            name: gateway
            port:
              number: 8080

Why is this needed?

So the ingress definition doesn't need to be updated manually after each install

Who is this for?

Everyone using the chart and wanting to receive the latest updates.

List All Possible Solutions and Workarounds

Proposed solution of the ingress.yaml template (specifying everything after the spec key):

spec:
  {{- if .Values.ingress.ingressClassName }}
  ingressClassName: {{ .Values.ingress.ingressClassName }}
  {{- end }}
  rules:
  {{- range .Values.ingress.hosts }}
    - host: {{ .host }}
      http:
        paths:
          - path: {{ .path | default "/" }}
            pathType: Prefix
            backend:
              service:
                name: {{ .serviceName }}
                port:
                  number: {{ .servicePort }}
  {{- end }}

{{- if .Values.ingress.tls }}
  tls:
{{ toYaml .Values.ingress.tls | indent 4 }}
{{- end }}

Steps to Reproduce (for bugs)

Install the chart with values set to enable and configure the ingress
Observe incomplete ingress resource

Context

Your Environment

FaaS-CLI version ( Full output from: faas-cli version ):
Docker version docker version (e.g. Docker 17.0.05 ):
What version and distriubtion of Kubernetes are you using? kubectl version
Operating System and version (e.g. Linux, Windows, MacOS):
Link to your project or a code example to reproduce issue:
What network driver are you using and what CIDR? i.e. Weave net / Flannel

alexellis commented 2 months ago

Hi @blaargh

I don't recognise your profile and I can't see your company in your profile, do you work for a company that is a customer?

If like you say, the Ingress definition is working, why do you need to edit it to include the additional http section?

Alex

alexellis commented 2 months ago

I took a look into your question. Have you already read the docs for OpenFaaS?

https://docs.openfaas.com/reference/tls-openfaas/#configure-tls-for-the-openfaas-gateway

The http section needs to be inputted via values.yaml, it's all explained above.

Am I missing something else?

blaargh commented 2 months ago

Hi @alexellis

I am using openfaas for private projects.

The ingress definition is working in the sense that it is syntactically correct, so Kubernetes doesn't decline it. But without the missing http section, it will never map to any service. I didn't see the documentation you mentioned, I just pulled the chart from artifacthub.

I would expect a mention of that within the chart directly, instead of putting that in the documentation. Because in the chart it is misleading/wrong what you would have to put for a correct ingress definition:

alexellis commented 2 months ago

The values.yaml you see is what's required for the previous versions of Kubernetes, you need to refer to the docs for the newer format as I explained in my previous comment.

There is no problem with the Helm chart, it has a minimal configuration, and those wishing to use Ingress and TLS need to follow the documentation: https://docs.openfaas.com/reference/tls-openfaas/#configure-tls-for-the-openfaas-gateway

It generates the "complete" YAML definition you are requesting.

values-custom-ingress.yaml:

ingress:
  enabled: true
  hosts:
  - host: gateway.openfaas.local  # Replace with gateway.example.com if public-facing
    http:
      paths:
      - path: /
        pathType: Prefix
        backend:
          service:
            name: gateway
            port:
              number: 8080

Then:

helm repo update  && helm upgrade openfaas   --install ./chart/openfaas   --namespace openfaas    -f ./chart/openfaas/values.yaml   -f ./chart/openfaas/values-pro.yaml -f ./chart/openfaas/values-custom-ingress.yaml

Output:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    meta.helm.sh/release-name: openfaas
    meta.helm.sh/release-namespace: openfaas
  creationTimestamp: "2024-08-19T11:34:56Z"
  generation: 1
  labels:
    app: openfaas
    app.kubernetes.io/managed-by: Helm
    chart: openfaas-14.2.65
    heritage: Helm
    release: openfaas
  name: openfaas-ingress
  namespace: openfaas
  resourceVersion: "1352"
  uid: 3ed2e7fb-c42d-4987-9277-5ee53faaeb97
spec:
  rules:
  - host: gateway.openfaas.local
    http:
      paths:
      - backend:
          service:
            name: gateway
            port:
              number: 8080
        path: /
        pathType: Prefix
status:
  loadBalancer: {}

Bear in mind that OpenFaaS CE is only licensed for non-commercial personal use. OpenFaaS Standard is available on a monthly basis and faasd can be used on a single host for commercial purposes if needed.

If it's less confusing for you, we can migrate the empty/default example in values.yaml to follow the newer pattern for K8s ingress, but customers will still need to follow the docs.

bobbiec commented 2 months ago

@alexellis , I think the change made in https://github.com/openfaas/faas-netes/commit/c6427d2529b2dac60f0774f0c23a897431093a09 broke arkade install openfaas, which is the installation method recommended by docs.

arkade install openfaas on a fresh cluster now results in the following error:

VALUES values.yaml
Command: /Users/bobbiec/.arkade/bin/helm [upgrade --install openfaas openfaas/openfaas --namespace openfaas --values /var/folders/89/qswnsxyn10z9kzwy6f2fj87h0000gn/T/charts/openfaas/values.yaml --set autoscaler.enabled=false --set dashboard.publicURL=http://127.0.0.1:8080 --set serviceType=LoadBalancer --set openfaasImagePullPolicy=IfNotPresent --set basicAuthPlugin.replicas=1 --set gateway.replicas=1 --set queueWorker.replicas=1 --set queueWorker.maxInflight=1 --set ingressOperator.create=true --set clusterRole=false --set operator.create=false --set faasnetes.imagePullPolicy=Always --set dashboard.enabled=false --set basic_auth=true --set gateway.directFunctions=false]
Error: UPGRADE FAILED: resource mapping not found for name: "openfaas-ingress" namespace: "openfaas" from "": no matches for kind "Ingress" in version "extensions/v1beta1"
ensure CRDs are installed first
Error: exit code 1, stderr: Error: UPGRADE FAILED: resource mapping not found for name: "openfaas-ingress" namespace: "openfaas" from "": no matches for kind "Ingress" in version "extensions/v1beta1"
ensure CRDs are installed first

And, reverting the values.yaml change to the old version (.ingress.enabled = false and related) fixed the issue.

alexellis commented 2 months ago

Hi thanks for your interest in OpenFaaS CE.

OpenFaaS CE is for personal use. I was curious to learn more about your use-case if you don't mind sharing?

The error no matches for kind "Ingress" in version "extensions/v1beta1" implies that you may be on an old K8s version? What does kubectl version give you?

ingress.enabled was not meant to be commited, but gave no error for me when I ran arkade install openfaas with the below version:

Client Version: v1.30.1
Server Version: v1.30.0

That said, I'll disable it and you should be unaffected going forward. You can also use --set ingress.enabled=false as an argument to arkade install openfaas

openfaas / faas-netes