[Kubeapps Chart] Simplify ingress configuration for cert-manager #740

juan131 · 2018-10-17T13:44:02Z

What this PR does / why we need it:

This PR allows the user to configure the proper annotations when using cert-manager to handle TLS certificates in an easy way. The user just needs to setup a boolean to true to do so.

E.g.:

helm install --name kubeapps --namespace kubeapps bitnami/kubeapps \
  --set ingress.enabled=true \
  --set ingress.hosts[0].name=kubeapps.custom.domain \
  --set ingress.hosts[0].tls=true \
  --set ingress.hosts[0].tlsSecret=kubeapps-tls \
  --set ingress.hosts[0].certManager=true

This will generate the following ingress:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: kubeapps
  namespace: kubeapps
  labels:
    app: kubeapps
    chart: kubeapps-0.8.0
    release: kubeapps
    heritage: Tiller
  annotations:
    kubernetes.io/tls-acme: "true"
spec:
  rules:
  - host: kubeapps.custom.domain
    http:
      paths:
      - path: /
        backend:
          serviceName: kubeapps
          servicePort: http
  tls:
  - hosts:
    - kubeapps.custom.domain 
    secretName: kubeapps-tls

See also bitnami/charts#882

andresmgot

Thanks for the PR! Apart than the tag changes the rest looks good to me. Wait for @migmartri input on this since he is already configuring Kubeapps with TLS in an internal cluster.

andresmgot · 2018-10-17T15:41:00Z

chart/kubeapps/Chart.yaml

-version: 0.7.0
-appVersion: DEVEL
+version: 0.8.0
+appVersion: v1.0.0-beta.2


revert this change, here it should be DEVEL

andresmgot · 2018-10-17T15:44:10Z

chart/kubeapps/values.yaml

@@ -60,7 +90,7 @@ apprepository:
  image:
    registry: docker.io
    repository: kubeapps/apprepository-controller
-    tag: latest
+    tag: v1.0.0-beta.2


the same here, it's needed for this to be latest

migmartri

Thanks @juan131, I am requesting changes until you rollback the appVersion DEVEL and image.tag changes.

A couple of comments:

helm install --name kubeapps --namespace kubeapps bitnami/kubeapps
--set ingress.enabled=true
--set ingress.hosts[0].name=kubeapps.custom.domain
--set ingress.hosts[0].tls=true
--set ingress.hosts[0].tlsSecret=kubeapps-tls
--set ingress.hosts[0].certManager=true

I am a little bit confused about the scope of the different options offered, for example:

In the example above, you referenced --set ingress.hosts[0].certManager=true which seems to reference to host[0] while the annotation is set at the ingress top level. Shouldn't then not be scoped inside the host?

Also noticed that in the description you had:

tls:
  - hosts:
    - kubeapps.juan.bkpr.run
    secretName: kubeapps-tls

I am assuming that kubeapps.juan.bkpr.run is a typo and was actually meant to be kubeapps.custom.domain?

migmartri · 2018-10-17T16:00:16Z

chart/kubeapps/templates/ingress.yaml

@@ -1,38 +1,36 @@
 {{- if .Values.ingress.enabled -}}
-{{- $fullName := include "kubeapps.fullname" . -}}
-{{- $ingressPath := .Values.ingress.path -}}
+{{- range .Values.ingress.hosts }}


so this is creating an ingress per host? Why don't we want to use the multi-host capabilities of a single ingress?

Having different ingresses could mean that cert-manager instead of issuing a cert with multiple CN will potentially do different certs no?

Yes, I think it's better to use multi-host since it requires issuing many certificates

migmartri · 2018-10-17T16:04:10Z

chart/kubeapps/values.yaml

+    annotations:
+    #  kubernetes.io/ingress.class: nginx
+
+  secrets:


I can not see the reference where this is being used.

Just added!

migmartri · 2018-10-17T16:05:50Z

chart/kubeapps/README.md

-TLS can be configured using the `ingress.tls` object in the same format that the Kubernetes Ingress requests. Please see [this example](https://github.com/kubernetes/contrib/tree/master/ingress/controllers/nginx/examples/tls) for more information.
+TLS can be configured using setting the `ingress.hosts[].tls` boolean of the corresponding hostname to true, then you can choose the TLS secret name setting `ingress.hosts[].tlsSecret`. Please see [this example](https://github.com/kubernetes/contrib/tree/master/ingress/controllers/nginx/examples/tls) for more information.
+
+If your cluster has a [cert-manager](https://github.com/jetstack/cert-manager) add-on to automate the management and issuance of TLS certificates, set `ingress.hosts[].certManager` boolean to true to enable the corresponding annotations for cert-manager. Otherwise, you can provide your own certificates using the `ingress.secrets` object.


Maybe you can add here the example you put in the description

helm install --name kubeapps --namespace kubeapps bitnami/kubeapps \ --set ingress.enabled=true \ --set ingress.hosts[0].name=kubeapps.custom.domain \ --set ingress.hosts[0].tls=true \ --set ingress.hosts[0].tlsSecret=kubeapps-tls \ --set ingress.hosts[0].certManager=true

migmartri · 2018-10-17T16:11:52Z

Out of curiosity, could you also post what would have been the equivalent to the following command before the change?

helm install --name kubeapps --namespace kubeapps bitnami/kubeapps \
  --set ingress.enabled=true \
  --set ingress.hosts[0].name=kubeapps.custom.domain \
  --set ingress.hosts[0].tls=true \
  --set ingress.hosts[0].tlsSecret=kubeapps-tls \
  --set ingress.hosts[0].certManager=true

I am wondering if it would make sense to just add the certManager annotation helper but not the multi-host <-> multi-ingress support.

Thanks!

juan131 · 2018-10-18T09:12:34Z

In the example above, you referenced --set ingress.hosts[0].certManager=true which seems to reference to host[0] while the annotation is set at the ingress top level. Shouldn't then not be scoped inside the host?

Totally agree!! We should set it at the ingress top level.

I am assuming that kubeapps.juan.bkpr.run is a typo and was actually meant to be kubeapps.custom.domain?

Yes, it's a typo.

I am wondering if it would make sense to just add the certManager annotation helper but not the multi-host <-> multi-ingress support.

I'd rather user multi-host too, let me adapt it!

migmartri

Thanks @juan131 for the changes!

Just added a couple of comments.

Also, would you mind attaching the diff of the output of helm template before and after the change when a chart is configured with 1 or 2 tls domains please?

Thanks!

migmartri · 2018-10-18T11:27:57Z

chart/kubeapps/templates/ingress.yaml

-    release: {{ .Release.Name }}
-    heritage: {{ .Release.Service }}
-{{- with .Values.ingress.annotations }}
+    app: {{ template "kubeapps.name" $ }}


Thanks for reverting back to template since it seems that here was the only place in which we used include.

Why are you using $ instead of current scope .? It seems that in the other templates we use .

We can use .. I'll update it. The $ was necessary when we were using {{- range ... at the top.

migmartri · 2018-10-18T11:37:04Z

chart/kubeapps/templates/ingress.yaml

+    app: {{ template "kubeapps.name" $ }}
+    chart: {{ template "kubeapps.chart" $ }}
+    release: {{ $.Release.Name }}
+    heritage: {{ $.Release.Service }}
  annotations:


note that this key annotations in this new case will be always added regardless of we passing .Values.ingress.annotations or .Values.ingress.certManager is that ok?

It's fine. The result is: no annotations

migmartri · 2018-10-18T11:39:31Z

chart/kubeapps/templates/ingress.yaml

+    kubernetes.io/tls-acme: "true"
+    {{- end }}
+    {{- range $key, $value := .Values.ingress.annotations }}
+    {{ $key }}: {{ $value | quote }}


Why do you prefer this approach vs dumping everything like before? {{ toYaml . | indent 4 }}

I just used because it's how we use it on others charts. Anyway, it's only adding the value of quoting the value...

I think it's irrelevant to use one or the another.

I just used because it's how we use it on others charts

ok, it makes sense. thanks for the explanation

We might need to move the rest of the chart to this approach in another PR since I believe that we use {{ toYaml . | indent 4 }} in other places.

migmartri · 2018-10-18T11:40:55Z

chart/kubeapps/templates/ingress.yaml

  {{- end }}
+---


is this multi-yaml new line neccessary?

migmartri · 2018-10-18T11:43:53Z

chart/kubeapps/templates/tls-secrets.yaml

+apiVersion: v1
+kind: Secret
+metadata:
+  name: {{ template "kubeapps.fullname" $ }}


isn't the name going to collide if there are two secrets?

Nice catch!! I guess we can do sth like:

metadata: name: {{ .name }}

juan131 · 2018-10-18T13:24:20Z

Before:

$ helm template --name kubeapps --namespace kubeapps chart/kubeapps  \
--set ingress.enabled=true \
--set ingress.hosts[0]=kubeapps.custom.domain  \
--set ingress.hosts[1]=kubeapps.other.domain
...
# Source: kubeapps/templates/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: kubeapps
  labels:
    app: kubeapps
    chart: kubeapps-0.7.0
    release: kubeapps
    heritage: Tiller
spec:
  rules:
    - host: "kubeapps.custom.domain"
      http:
        paths:
          - path: /
            backend:
              serviceName: kubeapps
              servicePort: http
    - host: "kubeapps.other.domain"
      http:
        paths:
          - path: /
            backend:
              serviceName: kubeapps
              servicePort: http

After:

$ helm template --name kubeapps --namespace kubeapps chart/kubeapps  \
--set ingress.enabled=true  \
--set ingress.certManager=true  \
--set ingress.hosts[0].name=kubeapps.custom.domain  \
--set ingress.hosts[0].tls=true \
--set ingress.hosts[0].tlsSecret=kubeapps-tls  \
--set ingress.hosts[1].name=kubeapps.other.domain
--set ingress.hosts[1].path=/other
...
---
# Source: kubeapps/templates/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: kubeapps
  labels:
    app: kubeapps
    chart: kubeapps-0.8.0
    release: kubeapps
    heritage: Tiller
  annotations:
    kubernetes.io/tls-acme: "true"
spec:
  rules:
  - host: kubeapps.custom.domain
    http:
      paths:
      - path: /
        backend:
          serviceName: kubeapps
          servicePort: http
  - host: kubeapps.other.domain
    http:
      paths:
      - path: /other
        backend:
          serviceName: kubeapps
          servicePort: http
  tls:
  - hosts:
    - kubeapps.custom.domain
    secretName: kubeapps-tls

juan131 · 2018-10-18T13:25:37Z

@migmartri as you can check it's not easy to setup the annotations and tls before the changes. An also, you can't choose a different path per host

migmartri

Thanks for the changes @juan131! It looks good! 🎉

migmartri · 2018-10-18T13:28:30Z

chart/kubeapps/templates/ingress.yaml

+    kubernetes.io/tls-acme: "true"
+    {{- end }}
+    {{- range $key, $value := .Values.ingress.annotations }}
+    {{ $key }}: {{ $value | quote }}


I just used because it's how we use it on others charts

ok, it makes sense. thanks for the explanation

We might need to move the rest of the chart to this approach in another PR since I believe that we use {{ toYaml . | indent 4 }} in other places.

migmartri · 2018-10-18T14:44:09Z

BTW, now that I think about this. I am a little bit worried about the change on the way we define the hosts since it will mean a breaking change right? Meaning that if somebody today has an automated upgrade process that relies on having ingress enable it will fail right?

In that case, if we still decide to go ahead with the change we might need to bump the mayor version not the minor one.

what do you think @prydonius @juan131 @andresmgot ?

juan131 · 2018-10-18T14:45:53Z

I agree. It breaks backwards compatibility and, therefore, it should be considered a major change.

juan131 · 2018-10-18T14:46:38Z

PR updated bumping the major version.

migmartri

@migmartri as you can check it's not easy to setup the annotations and tls before the changes. An also, you can't choose a different path per host

Wait, but in your example you are not setting TLS in the before case no?

And changing the example to do what your new code does (expect the path and annotation) would be something like:

helm template --name kubeapps --namespace kubeapps chart/kubeapps  \
--set ingress.enabled=true \
--set ingress.hosts[0]=kubeapps.custom.domain  \
--set ingress.hosts[1]=kubeapps.other.domain \
--set ingress.tls[0].secretName=kubeapps-tls \
--set ingress.tls.hosts[0]=kubeapps.custom.domain

which does not seems much more difficult that the new syntax. I agree that the new one is better though but I am not sure if it's worth breaking chart compatibility.

It might make sense to only add support to the custom certManager annotation without breaking compatibility, I am not even sure if adding support to path is even supported by out Nginx frontend?

I am removing my approval until more people chime in, probably breaking compatibility is fine since we are in beta and this change makes it consistent (I think) with our other upstreamed charts. But I'd like to hear from others.

Thanks!

migmartri

See my previous comment.

If we go ahead with this change we will probably need to upgrade the mayor version of the chart instead of the minor one.

juan131 · 2018-10-18T15:08:11Z

IMHO doing a major release of a chart should not be a drama.

We can add the steps you need to perform to do an upgrade if you consider it necessary but I think it's something normal since charts are improved constantly and some of these changes can break backward compatibility.

migmartri · 2018-10-18T16:17:54Z

Actually @andresmgot raised a good point that in semver, for 0.x versions, minor version changes are considered breaking. So feel free to revert back to 0.9 (in your case since 0.8 has already been taken).

Thanks @juan131!

[Kubeapps Chart] Simplify ingress configuration for cert-manager

2f8560b

andresmgot reviewed Oct 17, 2018

View reviewed changes

migmartri suggested changes Oct 17, 2018

View reviewed changes

Use 'multi-host' ingress instead of 'multi-ingress'

ab0d96e

migmartri reviewed Oct 18, 2018

View reviewed changes

Use the right scope

6eef7d9

migmartri approved these changes Oct 18, 2018

View reviewed changes

Bump major version

989d771

migmartri reviewed Oct 18, 2018

View reviewed changes

migmartri suggested changes Oct 18, 2018

View reviewed changes

migmartri approved these changes Oct 18, 2018

View reviewed changes

Andres and others added 2 commits October 18, 2018 18:20

Bump minor version

3be9b92

Merge branch 'master' into kubeapps_tls

0d1c344

migmartri merged commit 8cc2ba2 into vmware-tanzu:master Oct 19, 2018

juan131 deleted the kubeapps_tls branch October 23, 2018 14:32

juan131 mentioned this pull request Oct 23, 2018

Adapt Notes.txt to new Ingress configuration #760

Merged

[Kubeapps Chart] Simplify ingress configuration for cert-manager #740

[Kubeapps Chart] Simplify ingress configuration for cert-manager #740

Conversation

juan131 commented Oct 17, 2018 • edited Loading

What this PR does / why we need it:

andresmgot left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migmartri commented Oct 17, 2018

juan131 commented Oct 18, 2018

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan131 commented Oct 18, 2018

juan131 commented Oct 18, 2018

migmartri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

migmartri commented Oct 18, 2018

juan131 commented Oct 18, 2018

juan131 commented Oct 18, 2018

migmartri left a comment

Choose a reason for hiding this comment

migmartri left a comment

Choose a reason for hiding this comment

juan131 commented Oct 18, 2018

migmartri commented Oct 18, 2018

juan131 commented Oct 17, 2018 •

edited

Loading

andresmgot left a comment •

edited

Loading