feat(conf): support multiple hosts and tls certificates when kong is…

[bergemalm]

… exposed via ingress

What this PR does / why we need it:

An Ingress support multiple hosts and it's common to use CNAME using "simpler" dns records pointing to the same ingress. We need to support this.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

• PR is based off the current tip of the main branch.
• Changes are documented under the "Unreleased" header in CHANGELOG.md
• New or modified sections of values.yaml are documented in the README.md
• Commits follow the Kong commit message guidelines

[czeslavo]

@bergemalm Thank you for the contribution. Have you considered making this change non-breaking by keeping the old keys and merging paths and TLS config from both?

As I can see the only clash we'd have would be the tls key name, but we could dispatch how we handle it by its type using helm's kindIs (e.g. kindIs "slice" .tls / kindIs "string" .tls).

[bergemalm]

Ok, if that is preferred I can have a look at it @czeslavo .

[bergemalm]

Hi again @czeslavo, how about solving it like what's in this PR now? (edited the description above removing the breaking change related text)
It's possible to mix and combine. I also added a requirement for actually setting a hostname. The host can't be empty in an ingress configuration.

[czeslavo]

Hey @bergemalm, thank you for adjusting the PR 🙂 I think we're on a good track, but I still find a couple of issues we may have here:

1. I think that we should keep the behavior that allowed using just proxy.ingress.enabled without proxy.ingress.hostname specified:

proxy:
  ingress:
    enabled: true

Before the changes, it was generating a valid Ingress (it has an empty host which is valid for K8s API):

# Source: kong/templates/service-kong-proxy.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: kong-ingress-controller-kong-proxy
  namespace: kong
  labels:
    app.kubernetes.io/name: kong
    helm.sh/chart: kong-2.22.0
    app.kubernetes.io/instance: "kong-ingress-controller"
    app.kubernetes.io/managed-by: "Helm"
    app.kubernetes.io/version: "3.2"
spec:
  rules:
  - host:
    http:
      paths:
        - backend:
            service:
              name: kong-ingress-controller-kong-proxy
              port:
                number: 443
          path: /
          pathType: ImplementationSpecific

After the changes, it generates no rules:

# Source: kong/templates/service-kong-proxy.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: kong-ingress-controller-kong-proxy
  namespace: kong
  labels:
    app.kubernetes.io/name: kong
    helm.sh/chart: kong-2.22.0
    app.kubernetes.io/instance: "kong-ingress-controller"
    app.kubernetes.io/managed-by: "Helm"
    app.kubernetes.io/version: "3.2"
spec:
  rules:

I'd suggest not requiring the proxy.ingress.hostname to be specified and in case there are no proxy.ingress.hostname or proxy.ingress.hosts, generate the default rule with no host (as in the output above).

2. We don't have any charts/kong/ci/*-values.yaml that would include both proxy.ingress.hostname and proxy.ingress.hosts to ensure they do not conflict and render correctly.

3. We do not have charts/kong/ci/*-values.yamls that would include proxy.ingress.tls as a slice and another one as a string to verify they both render properly.

For 2 and 3 I suggest adding the following *-values.yaml files to the charts/kong/ci directory so they're linted in the CI. It's also worth running helm install --dry-run kong ./charts/kong -n kong --values ./charts/kong/ci/<name>-values.yaml against each of them to manually verify that everything renders as expected. Unfortunately, we have no automatic testing machinery in place that would allow specifying an expected output somehow for a given test case so a manual check is required. 😞

[bergemalm]

Oh, I've never used an empty host and assumed it was an error. Sorry, I'll have another look then...

[bergemalm]

I have made changes now @czeslavo, sorry for the delay. I used your patch for tests but with some modifications, for example the "combined" hostname and hosts test since there would otherwise be a duplication of the / path.
What do you think?

[bergemalm]

I guess we would need a tls secret... Or multiple actually. Keep those in tests or not or how to solve for it @czeslavo ?

[bergemalm]

I see now that there are some certificate generation/cert-manager support on other places. The question is if we want similar support for the ingress?

[bergemalm]

Perhaps skip adding ingress tls in tests for now and get this merged? More comments? Ping @czeslavo

[pmalek]

I guess we would need a tls secret... Or multiple actually. Keep those in tests or not or how to solve for it @czeslavo ?

It's perfectly fine to define the secrets in test ( to rely on concrete, the same values across test runs). Since that's a non trivial change I'd suggest we try to pursue the route of adding some basic tests for this change.

[bergemalm]

I guess we would need a tls secret... Or multiple actually. Keep those in tests or not or how to solve for it @czeslavo ?

It's perfectly fine to define the secrets in test ( to rely on concrete, the same values across test runs). Since that's a non trivial change I'd suggest we try to pursue the route of adding some basic tests for this change.

Ok, so could you point me in the direction of where to create and apply the tls secret(s) before the helm installs?

[czeslavo]

@bergemalm You can leverage the extraObjects and just append the below to every *-values.yaml you've added:

extraObjects:
  - apiVersion: v1
    kind: Secret
    metadata:
      name: kong.proxy.example.secret

[bergemalm]

Changes has been pushed, checks are passing. Ready for another review.

[czeslavo]

Beside one tiny nit I commented, LGTM now. 👍

[bergemalm]

Updated now @czeslavo.

[czeslavo]

✅

[bergemalm]

I cannot merge, please do when appropriate.