Overhaul ingress configuration #679

2024-07-05T13:03:51Z

pat-s commented

2024-07-05 13:03:51 +00:00

(Migrated from gitea.com)

Description of the change

Redesigned ingress configuration to align better with implicit best practices.

Benefits

Smarter defaults
More tests
Remove deprecated API Versions (e.g. extensions/v1beta1) and always use networking.k8s.io/v1

Possible drawbacks

Hopefully 🙃 none

Applicable issues

fix #674

Additional information

Define ingress.annotations via helpers
Move tests from tests/deployment to tests/ingress
Use own tests file for ingress tpl tests
Ensure defaults of path and pathType are always rendered
Set top-level default value for ingress. pathType
Change default of ingress.hosts[0].paths to [] to ensure proper rendering via template

⚠ BREAKING

I think all of these changes should be backward comp with existing ingress definitions, but surely worth highlighting in the changelog of the release.

Checklist

Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm
Breaking changes are documented in the README.md
Templating unittests are added

### Description of the change Redesigned ingress configuration to align better with implicit best practices. ### Benefits - Smarter defaults - More tests - Remove [deprecated API Versions](extensions/v1beta1) (e.g. `extensions/v1beta1`) and always use `networking.k8s.io/v1` ### Possible drawbacks Hopefully 🙃 none ### Applicable issues fix #674 ### Additional information - Define `ingress.annotations` via helpers - Move tests from `tests/deployment` to `tests/ingress` - Use own tests file for ingress tpl tests - Ensure defaults of `path` and `pathType` are always rendered - Set top-level default value for `ingress. pathType` - Change default of `ingress.hosts[0].paths` to `[]` to ensure proper rendering via template ### ⚠ BREAKING I think all of these changes should be backward comp with existing ingress definitions, but surely worth highlighting in the changelog of the release. ### Checklist  - [x] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm) - [x] Breaking changes are documented in the `README.md` - [x] Templating unittests are added

justusbunsi commented

2024-07-12 17:42:16 +00:00

(Migrated from gitea.com)

-{{- if .Values.ingress.apiVersion -}}
-{{- $apiVersion = .Values.ingress.apiVersion -}}

Looks like the ability to customize is now gone. We either have to restore this, or drop the unused values key.

``` -{{- if .Values.ingress.apiVersion -}} -{{- $apiVersion = .Values.ingress.apiVersion -}} ``` Looks like the ability to customize is now gone. We either have to restore this, or drop the unused values key.

pat-s commented

2024-07-13 09:23:36 +00:00

(Migrated from gitea.com)

Well spotted!

The doc says:

Mostly would only be used for argocd

Maybe this was needed in the past but I don't see how/why argocd specifically would need this option. Having ingress.apiVersion as an option is also quite uncommon, looking at other charts.
I'll remove it from the docs. Let me know if this is ok for you.

EDIT: Ah, found the initial reason: https://gitea.com/gitea/helm-chart/issues/251
3 years ago, meanwhile the networking resource is out of beta anyway, and I think argocd is also fine with it. At least I haven't found any issues in my argocd install.

Well spotted! The doc says: > Mostly would only be used for argocd Maybe this was needed in the past but I don't see how/why argocd specifically would need this option. Having `ingress.apiVersion` as an option is also quite uncommon, looking at other charts. I'll remove it from the docs. Let me know if this is ok for you. EDIT: Ah, found the initial reason: https://gitea.com/gitea/helm-chart/issues/251 3 years ago, meanwhile the networking resource is out of beta anyway, and I think argocd is also fine with it. At least I haven't found any issues in my argocd install.

pat-s commented

2024-07-13 12:00:35 +00:00

(Migrated from gitea.com)

@justusbunsi I also added a deprecation for ingress.className in favor of ingress.ingressClassName.

I had some issues with the deprecation.yaml template and getting the unittests to work.
Specifically, the fail call wasn't honored as the unittest only renders the ingress.yaml test file and not deprecations.yaml. When including the latter, other testing issues arise.

We might want to think about moving everything inside deprecation.yaml into helpers.tpl and include the specific tasks with a define into the respective templates. This might ease compatibility with helm-unittests. Let me know what you think about the current structure here and the general topic. This is of course it's own topic then.

@justusbunsi I also added a deprecation for `ingress.className` in favor of `ingress.ingressClassName`. I had some issues with the `deprecation.yaml` template and getting the unittests to work. Specifically, the `fail` call wasn't honored as the unittest only renders the `ingress.yaml` test file and not `deprecations.yaml`. When including the latter, other testing issues arise. We might want to think about moving everything inside `deprecation.yaml` into `helpers.tpl` and include the specific tasks with a `define` into the respective templates. This might ease compatibility with `helm-unittests`. Let me know what you think about the current structure here and the general topic. This is of course it's own topic then.

justusbunsi commented

2024-07-15 15:24:58 +00:00

(Migrated from gitea.com)

Maybe this was needed in the past but I don't see how/why argocd specifically would need this option. Having ingress.apiVersion as an option is also quite uncommon, looking at other charts. I'll remove it from the docs. Let me know if this is ok for you.

The old apiVersion is outdated since Kubernetes 1.22. Quite some time. Works for me to remove this option. 👍

I also added a deprecation for ingress.className in favor of ingress.ingressClassName.

What is the intention for doing so? IMO, ingress.ingressClassName is duplicated information. You are already in ingress object.

I had some issues with the deprecation.yaml template and getting the unittests to work.
Specifically, the fail call wasn't honored as the unittest only renders the ingress.yaml test file and not deprecations.yaml. When including the latter, other testing issues arise.

We might want to think about moving everything inside deprecation.yaml into helpers.tpl and include the specific tasks with a define into the respective templates. This might ease compatibility with helm-unittests. Let me know what you think about the current structure here and the general topic. This is of course it's own topic then.

Good idea to move the deprecations into helpers.tpl. Should be done in a separate PR.

> Maybe this was needed in the past but I don't see how/why argocd specifically would need this option. Having `ingress.apiVersion` as an option is also quite uncommon, looking at other charts. I'll remove it from the docs. Let me know if this is ok for you. The old apiVersion is outdated since Kubernetes 1.22. Quite some time. Works for me to remove this option. 👍 > I also added a deprecation for `ingress.className` in favor of `ingress.ingressClassName`. What is the intention for doing so? IMO, `ingress.ingressClassName` is duplicated information. You are already in `ingress` object. > I had some issues with the `deprecation.yaml` template and getting the unittests to work. Specifically, the `fail` call wasn't honored as the unittest only renders the `ingress.yaml` test file and not `deprecations.yaml`. When including the latter, other testing issues arise. > > We might want to think about moving everything inside `deprecation.yaml` into `helpers.tpl` and include the specific tasks with a `define` into the respective templates. This might ease compatibility with `helm-unittests`. Let me know what you think about the current structure here and the general topic. This is of course it's own topic then. Good idea to move the deprecations into `helpers.tpl`. Should be done in a separate PR.

pat-s commented

2024-07-16 08:05:39 +00:00

(Migrated from gitea.com)

What is the intention for doing so? IMO, ingress.ingressClassName is duplicated information. You are already in ingress object.

ingressClassName is the canonical naming across most charts and from k8s itself. While on it to revamp the ingress definition, I wanted to align with naming conventions.

> What is the intention for doing so? IMO, ingress.ingressClassName is duplicated information. You are already in ingress object. `ingressClassName` is the canonical naming across most charts and from [k8s itself](https://kubernetes.io/docs/concepts/services-networking/ingress/#the-ingress-resource). While on it to revamp the ingress definition, I wanted to align with naming conventions.

pat-s commented

2024-07-16 08:06:54 +00:00

(Migrated from gitea.com)

If you're ok with this PR in general, we could include it in the release together with the 1.22.1 upstream release (which we should get out soonish, as it contains many bug fixes).

Feel free to merge and create a release if everything looks ok to you.

If you're ok with this PR in general, we could include it in the release together with the 1.22.1 upstream release (which we should get out soonish, as it contains many bug fixes). Feel free to merge and create a release if everything looks ok to you.

justusbunsi (Migrated from gitea.com) reviewed 2024-07-21 11:40:21 +00:00

justusbunsi (Migrated from gitea.com) left a comment

I obviously made changes 😉. But somehow I am still not completely happy with the current changes. I can see similarities to the Hashicorp Vault Helm Chart¹ which I consider not the easiest approach from a user perspective. Right now it is very hidden that the ingress path can be changed. The empty ingress.hosts[0].paths is misleading with the newly extracted values. What would take precedence: Extracted values or explicitly defining an entry of that paths array? You'd have to mix'n'mesh them all to get the same rendered resource as right now. The pathType change is even breaking right now.

I am not against a breaking change, not at all. Especially not for our ingress definition. I suggest we completely eliminate the ingress.hosts array and define the main ingress resource settings at root level of the ingress object. Like the ones you introduced. That way, the main ingress settings become easier to define (and to handle during templating). If we want to keep the ability to add additional hosts/paths/..., I suggest adding an extraHosts array that is capable of the current ingress.hosts. I took inspiration from the Bitnami MinIO Helm Chart².

I suggest we improve this PR and release it as a dedicated major change version (we don't have to wait for the next Gitea release to do so). I didn't liked the idea to completely rewrite the PR without discussing this.

I obviously made changes 😉. But somehow I am still not completely happy with the current changes. I can see similarities to the Hashicorp Vault Helm Chart[^1] which I consider not the easiest approach from a user perspective. Right now it is very hidden that the ingress `path` can be changed. The empty `ingress.hosts[0].paths` is misleading with the newly extracted values. What would take precedence: Extracted values or explicitly defining an entry of that `paths` array? You'd have to mix'n'mesh them all to get the same rendered resource as right now. The `pathType` change is even breaking right now. I am not against a breaking change, not at all. Especially not for our ingress definition. I suggest we completely eliminate the `ingress.hosts` array and define the main ingress resource settings at root level of the `ingress` object. Like the ones you introduced. That way, the main ingress settings become easier to define (and to handle during templating). If we want to keep the ability to add additional hosts/paths/..., I suggest adding an `extraHosts` array that is capable of the current `ingress.hosts`. I took inspiration from the Bitnami MinIO Helm Chart[^2]. I suggest we improve this PR and release it as a dedicated major change version (we don't have to wait for the next Gitea release to do so). I didn't liked the idea to completely rewrite the PR without discussing this. [^1]: https://github.com/hashicorp/vault-helm/blob/636f8da6a614e596fb0af9cb119e3a5816f8ef5a/templates/server-ingress.yaml#L56 and https://github.com/hashicorp/vault-helm/blob/636f8da6a614e596fb0af9cb119e3a5816f8ef5a/values.yaml#L423-L434 [^2]: https://github.com/bitnami/charts/blob/8cf3a792d4ec1db94d581b12e342a892404a0461/bitnami/minio/values.yaml#L796-L854

pat-s commented

2024-09-02 08:52:57 +00:00

(Migrated from gitea.com)

Just FYI I am still on it and hope to get to it again in the coming days/weeks.

👍 1

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin fix-674:fix-674

git checkout fix-674

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#679