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

Metrics are enabled by default for ingress-nginx #12023

Closed
mitulshah-suse opened this issue Sep 27, 2024 · 4 comments · Fixed by #12095
Closed

Metrics are enabled by default for ingress-nginx #12023

mitulshah-suse opened this issue Sep 27, 2024 · 4 comments · Fixed by #12095
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@mitulshah-suse
Copy link

What happened:

Metrics are enabled by default in the controller code.
https://github.com/kubernetes/ingress-nginx/blob/main/pkg/flags/flags.go#L177
But the chart by default disables it.
https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/values.yaml#L861

What you expected to happen:

As a result of the above there is a flag --enable-metrics=false which gets added.
Intuitively removing that flag should keep the metrics disabled, but it gets enabled instead, which seems like unwanted behaviour.
Ideally, both default values should be false, and end user can enable it if needed. Or at least both should be true if that is the expected default value for ingress-nginx.

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

1.11.2

@mitulshah-suse mitulshah-suse added the kind/bug Categorizes issue or PR as related to a bug. label Sep 27, 2024
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Sep 27, 2024
@k8s-ci-robot
Copy link
Contributor

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

@longwuyuan
Copy link
Contributor

/remove-kind bug

This is not a bug. All user requirements are not identical and hence there is always going to be options. Most users have to tune the install to their requirement.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Sep 27, 2024
@mitulshah-suse
Copy link
Author

mitulshah-suse commented Sep 27, 2024

@longwuyuan while i agree that the users would install as per their requirement, if we install the controller using a manifest, we get metrics enabled, but with the chart we get it disabled. I believe its better to have both of them in sync.
In fact until this PR was merged in 1.10 #10959, both the controller and chart were in sync and were enabling it by default. (though i suppose it was unintentional)

@Gacko
Copy link
Member

Gacko commented Sep 27, 2024

Actually I didn't want to change the default. Metrics have been disabled in the chart by default for even longer, but we never passed the according argument to the controller, so inside the controller they were always enabled even if they were disabled in the chart and therefore not accessible.

So my PR only ensures they are really disabled if requested by the chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants