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

Unmentioned breaking change in 1.11 for GRPC #11866

Closed
flphvlck opened this issue Aug 26, 2024 · 7 comments
Closed

Unmentioned breaking change in 1.11 for GRPC #11866

flphvlck opened this issue Aug 26, 2024 · 7 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@flphvlck
Copy link

flphvlck commented Aug 26, 2024

There is unmentioned breaking change in 1.11 related to GRPC in combination with nginx.ingress.kubernetes.io/configuration-snippet annotation.

This feature came from issue #11250 and pull request #11258.

It would be nice to mention it in release notes.

What happened:
Nginx exits with error:

Error: exit status 1
2024/08/19 12:10:49 [emerg] 60#60: "grpc_read_timeout" directive is duplicate in /tmp/nginx/nginx-cfg658058193:2367
nginx: [emerg] "grpc_read_timeout" directive is duplicate in /tmp/nginx/nginx-cfg658058193:2367
nginx: configuration file /tmp/nginx/nginx-cfg658058193 test failed

This happens because we are using

      nginx.ingress.kubernetes.io/configuration-snippet: |
        grpc_next_upstream_tries X;
        grpc_read_timeout Xs;
        grpc_send_timeout Xs;

In the /tmp/nginx/nginx-cfg658058193 there really are duplicated directives for grpc (default values from annotations and our custom values from configuration-snippet

			# Grpc settings
			grpc_connect_timeout                    5s;
			grpc_send_timeout                       60s;
			grpc_read_timeout                       60s;

			grpc_next_upstream error timeout http_502 http_503 http_504 non_idempotent;
			grpc_next_upstream_tries X;
			grpc_read_timeout Xs;
			grpc_send_timeout Xs;
@flphvlck flphvlck added the kind/bug Categorizes issue or PR as related to a bug. label Aug 26, 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 Aug 26, 2024
@flphvlck
Copy link
Author

flphvlck commented Aug 26, 2024

If it's just about editing files in https://github.com/kubernetes/ingress-nginx/tree/main/changelog and the rest is magic behind, I can do it.
something like:

Breaking change
Stop using grpc_connect_timeout, grpc_send_timeout, grpc_read_timeout in nginx.ingress.kubernetes.io/configuration-snippet annotation (otherwise controller crash). These GRPC options (and eventually their default values) are inherited from respective proxy timeouts. Find out more in docs: https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#custom-timeouts

@longwuyuan
Copy link
Contributor

/assign @Gacko
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 26, 2024
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 lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Sep 26, 2024
@soupdiver
Copy link

soupdiver commented Sep 30, 2024

It's month now and just after breaking our deployments I ended up here.
Is there any plan to do something about this?
An updated changelog or so would also help.

@cflmarques
Copy link

We also had this exact issue. Any updates?

@strongjz
Copy link
Member

strongjz commented Dec 4, 2024

We apologize this caused issues, we are working on updating the templating engine in hopes that it would catch issues like this. There hundreds of possible config combinations and we request end to end test for all new features. The major issue with config and server snippets is that we can never tell exactly what a user will have in them, making it near impossible to test that scenario.

We generally do not update release notes, as they are automatically generated during releases.

I suggest moved to using the new feature and not using the config snippets.

/close

@k8s-ci-robot
Copy link
Contributor

@strongjz: Closing this issue.

In response to this:

We apologize this caused issues, we are working on updating the templating engine in hopes that it would catch issues like this. There hundreds of possible config combinations and we request end to end test for all new features. The major issue with config and server snippets is that we can never tell exactly what a user will have in them, making it near impossible to test that scenario.

We generally do not update release notes, as they are automatically generated during releases.

I suggest moved to using the new feature and not using the config snippets.

/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.

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. needs-priority triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

No branches or pull requests

7 participants