vidispine / hull

The incredible HULL - Helm Uniform Layer Library - is a Helm library chart to improve Helm chart based workflows
https://github.com/vidispine/hull
Apache License 2.0
231 stars 13 forks source link

Bug: Ingress with TLS #237

Closed snowping closed 1 year ago

snowping commented 1 year ago

Dear maintainers,

First of all, thanks for this library, it's very helpful to reduce the helm boilerplate. It seems there is a small issue with ingress templating.

The hosts section is not correctly rendered. The following snippet is the input

objects:
    ingress:
      frontend:
        enabled: true
        rules:
          frontend:
            host: _HT*hull.config.specific.ingress.host
            http:
              paths:
                standard:
                  path: /
                  pathType: Prefix
                  backend:
                    service: 
                      name: frontend
                      port:
                        name: http
        tls:
          frontend:
            hosts:
              - _HT*hull.config.specific.ingress.host

the output is then

...
 spec:
  rules:
  - host: workspace.qualicoat.test
    http:
      paths:
      - backend:
          service:
            name: release-name-workspace-frontend
            port:
              name: http
        path: /
        pathType: Prefix
  tls:
  - hosts: foo.com
    secretName: release-name-workspace-foo.com

however, the tls section of the output should be

  tls:
  - hosts: 
     - foo.com
    secretName: release-name-workspace-foo.com

Or am I overlooking something?

snowping commented 1 year ago

I am sorry, it seems it's only an issue when I use a reference like _HT*hull.config.specific.ingress.host as a host.

After some digging I could solve the issue using:

        tls:
          frontend:
            hosts:
              - _HT![{{ (index . "$").Values.hull.config.specific.ingress.host }}]
gre9ory commented 1 year ago

Hi @snowping,

First of all, thanks for this library, it's very helpful to reduce the helm boilerplate.

Thank you for using it!

I am sorry, it seems it's only an issue when I use a reference like _HT*hull.config.specific.ingress.host as a host.

No worries, please feel free to raise an issue even if it resolves itself, helps me understand problems users face.

After some digging I could solve the issue using: ...

Great that you found a working approach to solve your problem! Tells me there is at least some flexibility in case something doesn't work out directly or intuitively.

Concerning the initial problem I think there are a couple of valid ways to solve your case of filling up the hosts list with one (or multiple) referenced entries from hull.config.specific - your solution being one of them.

Here is some explanations:

First of, your solution which is to put a _HT-prefixed transformation as first array element to hosts:

              hosts:
                - _HT ... 

is by now eqivalent to directly using _HT ... as value:

              hosts: _HT ...

Any value to a key (below hull.config or any hull.object instance key) is scanned for being a HULL transformation (starts with _HT) and processed accordingly if this is detected.

If you don't have a string starting with _HT as a value but an array type instead (as in your solution), you hit the case where we check whether exactly the first item of an array matches a transformation (again starts with _HT). If yes the transformation is also done and it is expected to deliver the full array as a result same as above.

The checking of the first array item for transformation signifier is by now more a compatibility mode that only applies if the value type in your spec is an array. Reason is that initially the HULL schema did not allow to have _HT-prefixed strings used everywhere and hence where the schema demanded an array type value you would have used the "array-way" of triggering the transformation as described above. There exists a similar compatibility mode for dictionaries as described here.

These days this limitation is gone, you can put _HT-prefixed transformations almost anywhere directly as a string value and it should work given your used transformation provides a valid result of the expected type.

So regarding the transformation result type, since here you need to create an array as a value of hosts: you need to make sure your transformation returns an array too, With _HT![{{ (index . "$").Values.hull.config.specific.ingress.host }}] you did that nicely by wrapping the single referenced entry to an array by putting the [ and ] around it. Works.

If you had multiple individual entries you wanted to combine you need to seperate them with , like this:

            hosts: _HT!
              [
                {{ (index . "$").Values.hull.config.specific.ingress.host_one }},
                {{ (index . "$").Values.hull.config.specific.ingress.host_two }},
              ]

A potential modeling alternative could be to reference an array of hosts like this:

hull:
  config:
    specific:
      ingress:
        hosts:
          - foo.com
          - bar.com

in which case the _HT* way of referencing the array directly maintaining its type would work:

            hosts: _HT*hull.config.specific.ingress.hosts

or again in compatibility mode:

            hosts:
              - _HT*hull.config.specific.ingress.hosts

_HT* can be very helpful because it detects the source fields type (string, int, bool, array, dictionary) and takes the required steps to copy the source fields value maintaining the correct source type. This can be more complicated using _HT! where more flexiblity exists but the handling is more complex, e.g. to correctly reference and copy the value of an array or dictionary field you need to employ a range command and copy the values from the source one by one like this:

            hosts: _HT!
              [
                {{ range $item := (index . "$").Values.hull.config.specific.ingress.hosts }}
                {{ $item }},
                {{ end }}
              ]

Whether that modeling suggestion to have an array as source field is an better or worse alternative to have an array as source depends on your preference and chart design.

Last remark:

With arrays in HULL it is only possible to transform an array with something returning the arrays full value, not the values of individual items. Deep nesting of transforming arrays that may be children of other arrays or similar is unfortunately out of scope for what can be technically done with HULL. In my experience it was however always sufficient to be able to transform an array completely and create the desired result within your transformation.

gre9ory commented 1 year ago

Here is a test values.yaml covering the various approaches mentioned. It makes no sense functionally and just demonstrates the discussed cases.

You can test it out by downloading latest hull-demo and running helm template -f <saved-snippet> hull-demo-1.28.1.tgz against it:


hull:
  config:
    specific:
      ingress:
        host_one: foobar.com
        host_two: barfoo.com
        hosts:
         - foo.com
         - bar.com
  objects:
    ingress:
      frontend:
        enabled: true
        rules:
          frontend:
            host: _HT*hull.config.specific.ingress.host_one
            http:
              paths:
                standard:
                  path: /
                  pathType: Prefix
                  backend:
                    service: 
                      name: frontend
                      port:
                        name: http
        tls:
          test1: # create array with single entry in compatibility way
            hosts:
              - _HT![{{ (index . "$").Values.hull.config.specific.ingress.host_one }}]
          test2: # create array with single entry in standard way
            hosts: _HT![{{ (index . "$").Values.hull.config.specific.ingress.host_one }}]
          test3: # reference array with two entries in compatibility way
            hosts:
              - _HT*hull.config.specific.ingress.hosts
          test4: # reference array with two entries in standard way
            hosts: _HT*hull.config.specific.ingress.hosts
          test5: # reference array with two entries by copying entries one by one
            hosts: _HT!
              [
              {{ range $item := (index . "$").Values.hull.config.specific.ingress.hosts }}
              {{ $item }},
              {{ end }}
              ]
          test6: # combine all fields from array and individual items 
            hosts: _HT!
              [
              {{ range $item := (index . "$").Values.hull.config.specific.ingress.hosts }}
              {{ $item }},
              {{ end }}
              {{ (index . "$").Values.hull.config.specific.ingress.host_one }},
              {{ (index . "$").Values.hull.config.specific.ingress.host_two }}
              ]

snowping commented 1 year ago

Hi @gre9ory

Awesome! thank you for the explanation and further examples. That helps me a lot.