Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom-Headers annotation not working with 1.10.1 (changes are visible in git tag for 1.10.1) #11339

Closed
BernhardGruen opened this issue May 2, 2024 · 13 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@BernhardGruen
Copy link

What happened:

According to the tag controller-1.10.1 the file nginx.tmpl should support the Custom-Headers functionality implemented in #9742. But it does not.

Link to nginx.tmpl in tag controller-1.10.1: https://github.com/kubernetes/ingress-nginx/blob/controller-v1.10.1/rootfs/etc/nginx/template/nginx.tmpl

When comparing with the release-1.10 branch I see that the support is not included there: https://github.com/kubernetes/ingress-nginx/blob/release-1.10/rootfs/etc/nginx/template/nginx.tmpl

Therefore it seems either the tagging process was not working correctly (i.e. tagged based on main) or branch release-1.10 was not updated and was then used as base for the image build process.

What you expected to happen:

I expect that I am able to set custom headers. But as the function is not available in the file nginx.tmpl it basically can't work at all.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

1.10.1

Kubernetes version (use kubectl version):

1.28.6

Environment:

does not matter in that case, I believe.

How to reproduce this issue:

Add global-allowed-response-headers: Content-Type,X-no-meaning to the config map of the ingress controller.

Add an annotation nginx.ingress.kubernetes.io/custom-headers: custom-headers-configmap to an ingress.

Create said configmap in the same namespace (using Content-Type: text/json as data).

Test if the header has changed from text/html to text/json.

Anything else we need to know:

I believe the error happened while tagging controller-1.10.1 as it seems to be based of main instead of branch release-1.10.
The release notes of the controller (https://github.com/kubernetes/ingress-nginx/releases/tag/controller-v1.10.1) also do not reference #9742.

Maybe all this only happened because the documentation https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-headers already contains an annotation that will only be present with the next minor release 1.11.0. But I really don't know.

@BernhardGruen BernhardGruen added the kind/bug Categorizes issue or PR as related to a bug. label May 2, 2024
@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels May 2, 2024
@longwuyuan
Copy link
Contributor

/triage accepted
/priority important-soon

Yes true.

Line-391 in main

# Custom headers for response
seems different from same line-391 in branch release-1-10
{{ if or $cfg.DisableAccessLog $cfg.DisableHTTPAccessLog }}

@Gacko maybe the change was not cherry-picked. cc @strongjz @rikatz @tao12345666333

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels May 2, 2024
@longwuyuan
Copy link
Contributor

@cgroschupp can you confirm test this please

@adrianmoisey
Copy link

According to the tag controller-1.10.1 the file nginx.tmpl should support the Custom-Headers functionality implemented in #9742. But it does not.

I'm curious about this.

I've looked at the release for 1.10.1, and it doesn't seem to imply that #9742 is bundled with it.

Specifically if I visit https://github.com/kubernetes/ingress-nginx/releases/tag/controller-v1.10.1, I don't see the characters "9742" on the page, nor do I see "annotation"

What are you seeing that makes you say that the feature is in 1.10.1?

@BernhardGruen
Copy link
Author

According to that controller tag the controller image should contain that feature.
https://github.com/kubernetes/ingress-nginx/blob/controller-v1.10.1/rootfs/etc/nginx/template/nginx.tmpl

It seems the controller was not tagged based on release-1.10 branch but instead on main.

And also the documentation mentions custom headers. Therefore I thought that should work.

@longwuyuan
Copy link
Contributor

/assign

Will check and follow up. Maybe the cherrypicking to the branch did not happen

@adrianmoisey
Copy link

According to that controller tag the controller image should contain that feature. https://github.com/kubernetes/ingress-nginx/blob/controller-v1.10.1/rootfs/etc/nginx/template/nginx.tmpl

Oh right, I thought you meant that the tag's description (well, release) contains the feature.
Yeah, something is up with the 1.10.1 tag and branch, it looks like they diverged for some reason.

@adrianmoisey
Copy link

@codypperson
Copy link

I also experienced a problem related to skew between the nginx.tmpl on GitHub vs the container image. I typically grab the nginx.tmpl from the tag on GitHub when updating my manifest to deploy, but after deploying to my staging environment the controller was unable to start due to templating errors: can't evaluate field GeoIP2AutoReloadMinutes in type config.Configuration. I then figured out there was a large drift in the nginx.tmpl from GitHub, vs the nginx.tmpl from the actual container image, which is what I ended up using.

Here is the difference between the two:

cperson@THINKPAD:~/Downloads/ingress-nginx-v1.10.1$ curl -sSL https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.10.1/rootfs/etc/nginx/template/nginx.tmpl > nginx_from_github_tag_v1.10.1.tmpl
cperson@THINKPAD:~/Downloads/ingress-nginx-v1.10.1$ docker run --rm registry.k8s.io/ingress-nginx/controller:v1.10.1 cat /etc/nginx/template/nginx.tmpl > nginx_from_image_tag_v1.10.1.tmpl
cperson@THINKPAD:~/Downloads/ingress-nginx-v1.10.1$ diff nginx_from_github_tag_v1.10.1.tmpl nginx_from_image_tag_v1.10.1.tmpl 
175,177d174
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
189,191d185
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
203,205d196
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
229,231d219
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
255,257d242
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
265,267d249
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
275,277d256
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
292,294d270
<         {{ if (gt $cfg.GeoIP2AutoReloadMinutes 0) }}
<         auto_reload {{ $cfg.GeoIP2AutoReloadMinutes }}m;
<         {{ end }}
334,337d309
<     {{ if gt $cfg.GRPCBufferSizeKb 0 }}
<     grpc_buffer_size {{ $cfg.GRPCBufferSizeKb }}k;
<     {{ end }}
< 
1490,1496d1461
<             {{ end }}
< 
<             {{ if $location.CustomHeaders }}
<             # Custom Response Headers
<             {{ range $k, $v := $location.CustomHeaders.Headers }}
<             more_set_headers {{ printf "%s: %s" $k $v | escapeLiteralDollar | quote }};
<             {{ end }}
cperson@THINKPAD:~/Downloads/ingress-nginx-v1.10.1$ 

@gorge511
Copy link

We have the same issue with not matching template from 1.10.1 git tag template with 1.10.1 build.

Due to some specific configuration we need custom template in our deployment, so I updated the docker image to version 1.10.1 and updated the template accordingly from the version here on GitHub tag 1.10.1 and now the ingress controller is not able to start because of missing variables to the template.

I0513 12:11:35.449450       8 controller.go:190] "Configuration changes detected, backend reload required"
E0513 12:11:35.452153       8 controller.go:205] Unexpected failure reloading the backend:
template: nginx.tmpl:334:17: executing "nginx.tmpl" at <$cfg.GRPCBufferSizeKb>: can't evaluate field GRPCBufferSizeKb in type config.Configuration
E0513 12:11:35.452208       8 queue.go:131] "requeuing" err="template: nginx.tmpl:334:17: executing \"nginx.tmpl\" at <$cfg.GRPCBufferSizeKb>: can't evaluate field GRPCBufferSizeKb in type config.Configuration" key="luft-system/kube-state-metrics-cgpjj"
I0513 12:11:35.452339       8 event.go:364] Event(v1.ObjectReference{Kind:"Pod", Namespace:"luft-system", Name:"ingress-nginx-controller-7c4986467-v8w6c", UID:"b38b633e-482b-4609-a327-eb451cc0c1c7", APIVersion:"v1", ResourceVersion:"531509399", FieldPath:""}): type: 'Warning' reason: 'RELOAD' Error reloading NGINX: template: nginx.tmpl:334:17: executing "nginx.tmpl" at <$cfg.GRPCBufferSizeKb>: can't evaluate field GRPCBufferSizeKb in type config.Configuration
I0513 12:11:38.686242       8 sigterm.go:36] "Received SIGTERM, shutting down"

I'm going to update our custom template accordingly to the version baked in the image and we should be fine for now. However, it would be nice to fix this discrepancy so others don't have the same problem.

@Gacko
Copy link
Member

Gacko commented May 23, 2024

As already pointed out here, this issue is caused by the Git tag being set on the wrong branch. The release-1.10 branch the image was built and the release was forged from does not contain this feature, as we are not releasing new features but only patches on an existing minor release.

/close

@k8s-ci-robot
Copy link
Contributor

@Gacko: Closing this issue.

In response to this:

As already pointed out here, this issue is caused by the Git tag being set on the wrong branch. The release-1.10 branch the image was built and the release was forged from does not contain this feature, as we are not releasing new features but only patches on an existing minor release.

/close

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-sigs/prow repository.

@BernhardGruen
Copy link
Author

@Gacko I fully understand that this feature was not planned to be integrated into 1.10 as I also wrote at the end of my original message. But still there is a bug in the tagging process that can and will eventually lead to confusion.
Also the official documentation https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-headers already mentions custom-headers. Maybe the documentation should be based on the current stable branch instead of an unreleased branch.

@Gacko
Copy link
Member

Gacko commented May 23, 2024

I think the tagging issue only affects v1.10.1, we had some issues with forging that release.

Regarding the docs: It has been like this ever since. To fix this, we would need to have docs per tag, atm docs are being published directly from main branch. @longwuyuan, can you come up with a solution for that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

No branches or pull requests

7 participants