fix datasource, $exported_namespace variable in grafana nginx dashboard

[fschlich]

What this PR does / why we need it:

PR #8586 added two panels to the general "NGINX / Ingress controller" dashboard, which unfortunately introduced two issues:

1. The panels reference a datasource with a particular UID, instead of the general variable "${DS_PROMETHEUS}" used by the existing panels. This can lead to ugly error messages when opening the dashboard.
2. The new panels hard-code the label exported_namespace="uat", whereas this should be a dashboard variable for the user to choose a value (like with the other labels).

This PR addresses both issues, and fixes the sort order of the possible variable values to be alphabetical.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

described above, I didn't think opening separate issues would add value

How Has This Been Tested?

imported the fixed dashboard in Grafana 8.5.0 and verified that the issues described above no longer occur

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

• CLI change
• API change
• UI change
• configuration schema change
• behavioral change
• change in non-functional attributes such as efficiency or availability, availability of a new platform
• a warning about a deprecation
• fix of a previous Known Issue
• fix of a vulnerability (CVE)

No release notes are required for changes to the following:

• Tests
• Build infrastructure
• Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

NONE

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: fschlich / name: Florian Schlichting (b0c853e617f8a4d443e08cfb15acf3689b06a755, d4821a23b9252241ff9bb455d0801d09e92b0dc8)

[k8s-ci-robot]

@fschlich: 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.

[k8s-ci-robot]

Welcome @fschlich!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @fschlich. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[longwuyuan]

• @fschlich thanks for the contribution
• Please sign the CLA
• The description of the PR only has verbal explaination. There is no data trail leading to the validity and verification of the change. That implies that someone has to clone your branch to verify and validate. Or others have to take your word for it. This, in my opinion, is not adequate
• Its not easy to populate all the info in description but at least, my opinion, the screenshots of the old and new dashboard and some kubectl command outputs to show what was deployed (or anything similar to trail the software bits installed), would be a helpful record of what this change resulted into on the dashboard

[fschlich]

@longwuyuan thanks for looking into this!

• CLA is now done, I believe
• I was hoping someone with knowledge of Grafana dashboards would look at the diff, see that the version in the repo is obviously wrong and that this PR improves things. However, as you suggested I'm trying to illustrate what it does with the following two screenshots:

dashboard as-is:

dashboard fixed by this PR

• as discussed in add grafana dashboards to the chart #6381, the dashboard is not currently installed by the chart. The steps to reproduce are thus:
  • install Grafana, or ensure you have admin access to the instance installed by kube-prometheus-stack
  • configure a datasource that has access to the metrics scraped with the ServiceMonitor installed with this chart (metrics.serviceMonitor.enabled: true). This is automatic with kube-prometheus-stack
  • log into Grafana, click Dashboards -> Browse -> Import -> Upload JSON file, select nginx.json from this repository (the file changed by this PR), select the datasource from above, click Import
  • alternatively, one can copy deploy/grafana/dashboards/nginx.json into a custom helm chart that installs the file content as a configmap alongside kube-prometheus-stack.

[longwuyuan]

@fschlich thanks for the update. The project is in a feature-freeze phase. It was announced some months ago. All resources are directed towards stabilization efforts. So info like this helps to know if this change is part of stabilization without having to spend too much time.

[tghartland]

I was going to open an issue for these broken panels so I'm glad there's already an attempt to fix them.

@fschlich I have some suggestions for other improvements.

As you have it, the values for exported_namespace are filtered by $ingress.
https://github.com/kubernetes/ingress-nginx/pull/9092/files#diff-1fa14fb8fbdd40f4120ce282140d1c4523ea52ef112653b27390dbecc5a41fcfR1613
Which means that the dropdown list for namespaces is filtered by which ingress you selected, but the dropdown list for ingress names is not filtered by which namespace you select.

It would be better to switch things around, so that you can first filter by a namespace, and then by ingresses within that namespace.

I would also suggest renaming the label for the new variable to "Ingress namespace". The meaning seems a bit clearer to me.

Workflow based on the current state of the PR:
https://user-images.githubusercontent.com/11710676/197003875-c4c56b83-e08f-470a-942c-6ee000f965e2.mp4

New workflow:
https://user-images.githubusercontent.com/11710676/197003333-3a49cf2c-3f8e-42a4-b663-741ae10f45bb.mp4

The ingress namespace filter should probably also apply to the "Ingress Request Volume" and "Ingress Success Rate" panels.
I manage a multi-tenancy cluster so it's very useful for users to be able to filter for just what they want to see (their own namespaces).

[tghartland]

This is the diff that I have on top of your PR.
It looks a bit strange because the order of the variables is switched.
https://gist.github.com/tghartland/9147d88f991a95d4bab0fa7278c237eb

[fschlich]

Hi @tghartland, yes, yes and yes - for renaming to Ingress Namespace, switching around with Ingress, and finally apply that to "Ingress Request Volume" and "Ingress Success Rate" panels. Makes perfect sense.

Can you just add those changes to this PR, or do I have to do that?

[tghartland]

I can't push to your branch, but here's the file you can copy in yourself.
https://gist.github.com/tghartland/6cd724377aee3f7f8c3f6c25e3066b20

[fschlich]

Seems my first commit was essentially merged as #9284. Maybe we can move forward with the rest now, too?

[rikatz]

/lgtm
/approve
Thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fschlich, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment