Disable user snippets per default

[rikatz]

What this PR does / why we need it:

Disables user snippets per default

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

Part of #10186

How Has This Been Tested?

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	ff5beef
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/64fe4bdac5082e00080f0f78

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tao12345666333]

lint error

[rikatz]

lint error

yup, there's something weird with the env preparation actually. I can run it locally, probably something on the action.Will check later

[tao12345666333]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, tao12345666333

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rikatz,tao12345666333]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rouke-broersma]

@rikatz since this is marked as a breaking change, and your docs mention that ingress-nginx follows semver, shouldn't this have been released as a new major version? Or am I misunderstanding the release process of ingress-nginx?

[perosb]

Something broke in our ingress as well related to snippets. I assume this is the reason.

Will try adding allowSnippetAnnotations: true value.

[iamdmitrij]

Ran into same issue when using charts which create NGINX ingress for you with server snippet inside (for example, Argo CD chart).

@perosb, adding controller.allowSnippetAnnotations with true value worked for me, Thanks!

Would have been nice if this workaround was documented in the release notes.

[rikatz]

I've missed the docs, sorry

@iamdmitrij do you mind opening a PR for it and tagging me and @longwuyuan ? Maybe also add on the description of snippet annotations that they need to be enabled by ingress-admin.

Thanks

[strongjz]

We've talked about this change for several releases and in the community meetings multiple times. It is documented what it does in the docs https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#allow-snippet-annotations and it's highlighted in the release notes and we also did a beta release for folks to test it.

[rouke-broersma]

We've talked about this change for several releases and in the community meetings multiple times. It is documented what it does in the docs https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/configmap/#allow-snippet-annotations and it's highlighted in the release notes and we also did a beta release for folks to test it.

The docs note the default would change in the next major release. It was not a major release. I'm just asking for clarification on that, as it seems to go against your protocol which states you use semver.

[zoomoid]

@rouke-broersma I'm assuming they use the term "semver" loosely the same way as Kubernetes does, where minor versions are used as major versions and the project remains in v1 indefinitely. Only the helm chart version seems to adhere to strict semver.

[rouke-broersma]

@rouke-broersma I'm assuming they use the term "semver" loosely the same way as Kubernetes does, where minor versions are used as major versions and the project remains in v1 indefinitely. Only the helm chart version seems to adhere to strict semver.

The Helm chart also did not receive a major version bump.

[zoomoid]

@rouke-broersma I'm assuming they use the term "semver" loosely the same way as Kubernetes does, where minor versions are used as major versions and the project remains in v1 indefinitely. Only the helm chart version seems to adhere to strict semver.

The Helm chart also did not receive a major version bump.

Probably because releasing v1.9.0 of the controller did not require a breaking change in the chart

[strongjz]

We apologize to folks who missed the release notes and other notifications about this change, we strive to make these change known as soon as possible. No need for philosophical debate about whether a config change is major minor or even a patch change. We gave what we believed to be ample warning about this change. We invite you to the community meetings to discuss future changes with us. We are also discussing v2.0.0 which will have architecture changes in it as well.

[strongjz]

We can discuss if default config changes should reflect a minor change like we did in 1.9.0 in our next community meeting. I'll make sure to put it on the agenda.

[rouke-broersma]

@rouke-broersma I'm assuming they use the term "semver" loosely the same way as Kubernetes does, where minor versions are used as major versions and the project remains in v1 indefinitely. Only the helm chart version seems to adhere to strict semver.

The Helm chart also did not receive a major version bump.

Probably because releasing v1.9.0 of the controller did not require a breaking change in the chart

They changed the default value in the chart, that is a breaking change any way you look at it. They may have forgotten sure but there's no rational reason for it not to be a major version bump. In any case I noticed it myself before upgrading anyway so that's not the issue. I just want clarification from the release team on if it was a mistake or if we need to be extra mindful of these kinds of changes in any new release instead of depending on semver to inform our decisions, that's all.

[rhummelmose]

I can confirm that:

---
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: nginx-ingress
spec:
  values:
    controller:
      allowSnippetAnnotations: true # <-

resolves the issue.

[indrekj]

This broke our environment as well. I even went through the changelog before upgrading the controller but I didn't pay enough attention to Disable user snippets per default bullet point. Would have been nice if this had been highlighted in some way.

EDIT: I see this was actually highlighted on the releases page. I looked at the changelogs in the changelog directory.

controller.allowSnippetAnnotations: true did fix it for us as well.

[cablunar]

@indrekj we had the same problem - I also missed the "testing section", because these things are usually tagged with a BREAKING note in the release notes 😞

[aofei]

EDIT: I see this was actually highlighted on the releases page. I looked at the changelogs in the changelog directory.

Placing it in a duplicate "Changelog" section with a regular font weight isn't considered "highlighting". Should've placed it in a "Breaking Changes" section at the top. Changes like this are undoubtedly considered breaking.

[StefanLobbenmeierObjego]

I would also suggest a much stronger wording in the documentation:

https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#configuration-snippet

I would expect something like

Has to be explicitly enabled by , otherwise it will not be applied

Instead of a vague "might be disabled in multi tenant clusters"

[strongjz]

Thank you for the feedback. Feel free to open a PR and/or join the community meeting tomorrow to discuss more.