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.

Sign up for GitHub

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

Jump to bottom

Incorrect service name emitted in some metrics #10210

Open
adrianmoisey opened this issue Jul 18, 2023 · 7 comments · May be fixed by #11294
Open

Incorrect service name emitted in some metrics #10210

adrianmoisey opened this issue Jul 18, 2023 · 7 comments · May be fixed by #11294
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. 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

@adrianmoisey
Copy link

adrianmoisey commented Jul 18, 2023 •
edited

What happened:

When defining two backends, as such:

     - backend:
          service:
            name: service-a
            port:
              number: 80
        path: /
        pathType: Exact
      - backend:
          service:
            name: service-b
            port:
              number: 80
        path: /
        pathType: Prefix

We only get metrics for the service-a backend.

What you expected to happen:

To be able to get metrics for each backend.

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

NGINX Ingress controller
  Release:       v1.3.0
  Build:         2b7b74854d90ad9b4b96a5011b9e8b67d20bfb8f
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.19.10

Kubernetes version (use kubectl version):

Server Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.8", GitCommit:"0ce7342c984110dfc93657d64df5dc3b2c0d1fe9", GitTreeState:"clean", BuildDate:"2023-03-15T13:33:02Z", GoVersion:"go1.19.7", Compiler:"gc", Platform:"linux/amd64"}

Environment:

  • Cloud provider or hardware configuration: AWS (via kOps)

  • OS (e.g. from /etc/os-release): Ubuntu 20.04.5 LTS

  • Kernel (e.g. uname -a): 5.15.0-1031-aws

  • Install tools: kOps

    • Please mention how/where was the cluster created like kubeadm/kops/minikube/kind etc.

  • Basic cluster related info:

    • kubectl version Server Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.8", GitCommit:"0ce7342c984110dfc93657d64df5dc3b2c0d1fe9", GitTreeState:"clean", BuildDate:"2023-03-15T13:33:02Z", GoVersion:"go1.19.7", Compiler:"gc", Platform:"linux/amd64"}

    • kubectl get nodes -o wide Ubuntu 20.04.5 LTS 5.15.0-1031-aws containerd://1.6.18

  • How was the ingress-nginx-controller installed:
    Installed via Helm using ArgoCD

  • Current State of the controller:
    Controller is running and working as expected, except for this metric

  • Current state of ingress object, if applicable:
    All Ingress objects are fine and the controller handles requests fine.

How to reproduce this issue:
Create an ingress object with multiple / paths, of different types.
Send traffic to both matches / and /test.
Look at resulting metrics.

Anything else we need to know:

I looked at the resulting configuration and compared them to see if I can find the issue. This seems to highlight the bug:

$ diff -u a b
--- a	2023-07-13 10:56:34
+++ b	2023-07-13 10:56:42
@@ -1,4 +1,4 @@
-		location / {
+		location = / {

 			set $namespace      "mynamespace";
 			set $ingress_name   "myingress";
@@ -46,7 +46,7 @@
 			port_in_redirect off;

 			set $balancer_ewma_score -1;
-			set $proxy_upstream_name "mynamespace-service-a-80";
+			set $proxy_upstream_name "mynamespace-service-b-80";
 			set $proxy_host          $proxy_upstream_name;
 			set $pass_access_scheme  $scheme;

This config is fine, except that set $service_name is the same in both.

It seems that the problem is happening around here:

Which I think comes from https://github.com/kubernetes/ingress-nginx/blob/controller-v1.3.0/internal/ingress/controller/template/template.go#L1061-L1149

I think the expected diff is as such:

$ diff -u a b
--- a	2023-07-18 13:13:39
+++ b	2023-07-13 10:56:42
@@ -1,8 +1,8 @@
-		location / {
+		location = / {

 			set $namespace      "mynamespace";
 			set $ingress_name   "myingress";
-			set $service_name   "service-a";
+			set $service_name   "service-b";
 			set $service_port   "80";
 			set $location_path  "/";
 			set $global_rate_limit_exceeding n;
@@ -46,7 +46,7 @@
 			port_in_redirect off;

 			set $balancer_ewma_score -1;
-			set $proxy_upstream_name "mynamespace-service-a-80";
+			set $proxy_upstream_name "mynamespace-service-b-80";
 			set $proxy_host          $proxy_upstream_name;
 			set $pass_access_scheme  $scheme;
@adrianmoisey adrianmoisey added the kind/bug Categorizes issue or PR as related to a bug. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 18, 2023
@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/test-infra repository.

@adrianmoisey adrianmoisey changed the title Incorrect service emitted in some metrics Incorrect service name emitted in some metrics Jul 18, 2023
@longwuyuan
Copy link
Contributor

/remove-kind bug

Does this seem like your use case https://kubernetes.github.io/ingress-nginx/user-guide/monitoring/#wildcard-ingresses

@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 Jul 18, 2023
@adrianmoisey
Copy link
Author

I don't think so. We do use the --metrics-per-host=false option
The particular tag we're worried about is service, which is the backend service that is bring routed to, not the host

@strongjz
Copy link
Member

Can you test with /test as a specific path and see if the issue is the same? There may be an issue with pathType and the same path, /, the controller may not be differianating them.

Also can you try this in the latest relase, 1.8.1?

@github-actions
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 20, 2023
@adrianmoisey
Copy link
Author

adrianmoisey commented Apr 21, 2024 •
edited

Can you test with /test as a specific path and see if the issue is the same? There may be an issue with pathType and the same path, /, the controller may not be differianating them.

Adding a /test path that is an exact match works as expected.
I think the problem is exactly as you say, the controller isn't differentiating between them.

Also can you try this in the latest relase, 1.8.1?

I've tested with 1.10.0 and still get this issue

@adrianmoisey
Copy link
Author

I think I understand what is happening here. I plan to submit a PR if that is fine with you

adrianmoisey added a commit to adrianmoisey/ingress-nginx that referenced this issue Apr 22, 2024
@adrianmoisey
Fix incorrect service name being set
ca7e1e6 
Fix: kubernetes#10210

This handles the case where multiple rules have identical paths, but
differing types.
@adrianmoisey adrianmoisey linked a pull request Apr 22, 2024 that will close this issue
Open
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. 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
[sig-network] Ingress-Nginx
Status: No status
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix incorrect service name being set in the nginx template adrianmoisey/ingress-nginx
4 participants
@adrianmoisey @strongjz @longwuyuan @k8s-ci-robot