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 does not apply the configuration automatically #11680

Open
PavelGloba opened this issue Jul 25, 2024 · 7 comments · May be fixed by #11709
Open

custom-headers annotation does not apply the configuration automatically #11680

PavelGloba opened this issue Jul 25, 2024 · 7 comments · May be fixed by #11709
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@PavelGloba
Copy link

PavelGloba commented Jul 25, 2024

What happened:

I tried to use a new feature to set custom headers per ingress. After setting the annotation the configuration does not apply automatically. While there are sync events in the logs, the config reload is not triggered and nginx.conf is not changed. The feature itself is working if I restart the nginx controller. Also it's working if I set incorrect path to the configmap and then change it back to the correct path.
The update of the applied headers also does not work. If I change the configmap and then delete and add the annotation again, the headers are not changing. I also tried to create a new configmap and change the annotation to point to it, but it does not trigger config reload. Basically the only way to apply the configuration of the custom headers is to either reload the controller or to trigger an errror and then apply working configuration again.

What you expected to happen:

custom-headers annotation changes (add, edit, delete) should trigger the config reload.
Ideally custom headers configuartion should be also updated when the configmap with headers changes, but it's probably not possible

NGINX Ingress controller version:

NGINX Ingress controller
Release: v1.11.1
Build: 7c44f99
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.25.5

Kubernetes version:

v1.27.15-eks-db838b0

Environment:

AWS EKS

Configuration:

   allow-snippet-annotations: "false"
   global-allowed-response-headers: Referrer-Policy,Strict-Transport-Security,X-Content-Type-Options,X-Frame-Options,Content-Security-Policy
   http2-max-field-size: 512k
   http2-max-header-size: 512k
   large-client-header-buffers: 8 512k
   use-proxy-protocol: "true"
@PavelGloba PavelGloba added the kind/bug Categorizes issue or PR as related to a bug. label Jul 25, 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 Jul 25, 2024
@PavelGloba PavelGloba changed the title custom-headers annotation applies configuration only once until restart of the nginx-controller custom-headers annotation does not always apply the configuration Jul 25, 2024
@PavelGloba PavelGloba changed the title custom-headers annotation does not always apply the configuration custom-headers annotation does not apply the configuration automatically Jul 25, 2024
@longwuyuan
Copy link
Contributor

I did not test this yet, but this behavior is consistent with a lot of other features. That means config reload is not triggered on the events that you listed.

I don't think a behavior change can be implemented in the near future so you are better off doing a manual reload or deploying the changes during initial controller installation.

/remove-kind bug
/kind feature
/triage accepted

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 26, 2024
@PavelGloba
Copy link
Author

@longwuyuan are you sure this is a consistent behavior, because for example configuration snippets work out of the box. Basically every feature I used which is set per ingress works when it's applied to ingress. I understand that features itself should be enabled and configured before you can use them and the controller reload is needed. With the current implementation updates of ingresses are not possible, only the recreation or some workarounds like setting wrong configmap. If setting wrong configmap and then fixing it triggers the reload, the reload should also be triggered on any changes

@longwuyuan
Copy link
Contributor

@PavelGloba your expectation is correct and fair. Your expectation is the desired way such config changes should work.

The "proxy_set_header" directive is the configuration that is the result of adding a custom header. While there seems to be a watch/loop type of design for server_blocks & location blocks, the proxy_set_header is in the global config. And the global config seems to get its nginx.conf directives only at the time of creating a new pod or some other specific event like recovering from a broken controller config.

More importantly, there is a acute shortage of resources and there is a required direction that the development activities of the ingress-nginx controller must and absolutely must take. Because of these reasons it will not be possible to work on changing the controller's behavior to reload and live update nginx.conf for adding the proxy_set_header directive in the global context of nginx.conf .

I understand this is not your desired result or the desired result of any other user who has to make changes to environments that are not offering downtime. But I thought that you should be well informed to make required decisions. If its possible for someone to work on this change, they will comment here, but I think it is not likely.

Please wait for comments from others.

@jgoelen
Copy link

jgoelen commented Jul 30, 2024

@longwuyuan the fix for this problem doesn't seem to be a complex one.
The function func (l1 *Location) Equal(l2 *Location) bool in the file pkg/apis/ingress/types_equals.go is missing an equals check for the customheaders.Config type.
Maybe with some support I can start a pull request for this issue?

@longwuyuan
Copy link
Contributor

@jgoelen I am not a developer so I can not comment effectively.

What I do not know is if that check you mention is unique to just one annotation or if that code-path is shared by other annotations. I also don't know if the e2e tests will cause a false positive. What I do know is that in the past this may seem trivial for one annotation but there is a need for developers to comment on this. So I will wait for a developer to comment.

On the other hand its a great help to get contributions as its the priceless value to K8S projects. Hence waiting for comments from others.

@longwuyuan
Copy link
Contributor

You can message in the Kubernetes slack in the channel ingress-nginx-dev

Copy link

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

@github-actions github-actions bot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants