rstudio / helm

Helm Resources for RStudio Products
MIT License
36 stars 28 forks source link

Create ingress examples #52

Open pat-s opened 3 years ago

pat-s commented 3 years ago

Hi, thanks for the helm chart!

I am having trouble getting the ingress right.

Ingress specification

When I try to set the ingress in the current specification as

  - host: rsw.example.org
    paths:
      - path: /

I see

Error: failed to create resource: Ingress.extensions "rstudio-rstudio-workbench" is invalid: [spec.rules[0].http.paths[0].path: Invalid value: "map[path:/]": must be an absolute path, spec.rules[0].http.paths[0].backend: Required value: port name or number is required]

If I don't specify paths I get an error. Maybe there could be a default for this option?

  - host: rsw.example.org
    paths: []

Edit: In the template

https://github.com/rstudio/helm/blob/8b2deeecf9ba5c7229786712c02745a3c1213d53/charts/rstudio-workbench/templates/ingress.yaml#L29-L31

I found that the rules category comes before host: - so wouldn't the specification need to look as follows?

ingress:
  enabled: true
  rules:
    - host: subdomain.example.org
      paths:
        - path: /

Though this one errors with

    │ Error: failed to create resource: Ingress.extensions "rstudio-rstudio-workbench" is invalid: spec: Invalid value: []networking.IngressRule(nil): either `backend` or `rules` must be specified

Guidance how to set the ingress properly would be highly appreciated!

pathType Option

Since k8s v1.18 it is common to also provide a pathType option in the ingress. Would it be possible to adapt the template to support this?

See the gitea helm chart as an example.

So in the end I would like to write the following

ingress:
  enabled: true
  annotations: {}
  hosts:
    - host: rsw.example.com
      paths:
        - path: /
          pathType: Prefix
pat-s commented 3 years ago

After many hours of trial and error I found the following specification which works

ingress:
  enabled: true
  hosts:
    - host: subdomain.example.org
      paths: [/]

Reference

This only works if service.port mapping is added in values.yml because it is referenced in

https://github.com/rstudio/helm/blob/7caafdf5159f6c4e662bc0d5d740f1e588500b4a/charts/rstudio-workbench/templates/ingress.yaml#L3

and currently not defined in the values.yml template.

service:
  port: 8787

When missing it causes the following error

    │ Error: cannot patch "rsw" with kind Ingress: Ingress.extensions "rsw" is invalid: spec.rules[0].http.paths[0].backend: Required value: port name or number is required
Aarhan commented 3 years ago

Hey There!

I actually ran into these same issues while deploying the rstudio-pm chart yesterday. I submitted a pull request that should resolve the port issues by adding the service ports value to the values.yaml and also removing the hardcoded port 80 in svc.yaml and replaces it with the $svcPort variable used in the ingress.

56

As for the ingress path, I used the following value which worked without issue for me:

ingress: 
  enabled: true 
  hosts: 
    - host: subdomain.example.org 
      paths: 
        - /
colearendt commented 3 years ago

Apologies for the trouble y'all! Thanks for the PR @Aarhan !

We definitely need to create some examples in the examples/ directory to make some of this easier. For what it's worth, the connect examples (for cross-chart features like this) are a bit more robust currently, and can be a good source of referenceable material:

https://github.com/rstudio/helm/blob/4d94ced4e594f56fd0f488dc013bd82d21b02d18/examples/connect/high-availability-traefik/values.yaml#L56-L72

pat-s commented 2 years ago

After the latest ingress changes in #117, my previous ingress definition is failing.

We have several ingresses defined in our cluster and all besides the rstudio charts use the same notation, which is as follows:

ingress:
  hosts:
  - host: <subdomain.domain.topleveldomain>
    paths:
  - path: /
    pathType: Prefix

If I use this one for the RSW chart, I get

│ Error: cannot patch "rsw" with kind Ingress: Ingress.extensions "rsw" is invalid: spec.rules[0].http.paths[0].path: Invalid value: "map[path:/]": must be an absolute path

Comparing the ingress templates of several charts which use the notation from above, I see the following differences

RStudio charts use

https://github.com/rstudio/helm/blob/4685fd5504e1361e94f4f1d4fef9d9f41f42accb/charts/rstudio-workbench/templates/ingress.yaml#L32-L37

whereas the other charts we have deployed use

http:
  paths:
  {{- range .paths }}
  - path: {{ .path }}
  {{- if and .pathType (eq $apiVersion "networking.k8s.io/v1") }}
  pathType: {{ .pathType }}

The differences in the RStudio charts that I see are

I've forked the chart and deployed RSW using the path: {{ .path }} notation. This deployment then worked by using (as above)

  - path: /
    pathType: Prefix

Would it make sense to make this change in the all rstudio templates? Or are there any reasons why path: {{ . }} would even be preferable?

I don't have much of an expertise on ingress definitions 😃️ but would be happy to use one (canonical) definitions across all charts.

colearendt commented 2 years ago

Ahh yes, sorry for the trouble! Thanks for raising! This is almost certainly a regression that we missed in PR review - I'll take a look and get a fix in! 😄

colearendt commented 2 years ago

I think the reason that this was an issue is because we were passing the value directly, so this would have worked too in values:

paths:
  - /

https://github.com/rstudio/helm/blob/4685fd5504e1361e94f4f1d4fef9d9f41f42accb/examples/connect/high-availability-traefik/values.yaml#L65

It may be that we are using an older convention in the interest of backwards compatibility. Do you have a link to a reference chart that uses the pattern you talk about? Unfortunately this may end up being a breaking change, so we may need to be a bit more cautious about merging / seeing if we can make it backwards compatible, but I will have a branch up shortly for discussion 😄

colearendt commented 2 years ago

A backwards compatible approach for discussion: https://github.com/rstudio/helm/pull/134

Adds some complexity but leaves things backwards compatible 😅 Tradeoffs

pat-s commented 2 years ago

Thanks!

We've been using

    - host: <domain>
      paths: [/]

before which stopped working. After that I've tried a bunch of different notations which all failed (differently). Then I went on inspecting the template and comparing it to other charts. Here's an example of the Gitea chart which showcases what I was referring to.

Yet I can confirm that

paths:
  - /

works! So using this for the moment (until there's been the change made to the ingress as in #134)

Unfortunately this may end up being a breaking change, so we may need to be a bit more cautious about merging / seeing if we can make it backwards compatible, but I will have a branch up shortly for discussion

Sure, completely understandable.