ingress-nginx
ingress-nginx copied to clipboard
kubernetes
Metrics: Fix namespace in `nginx_ingress_controller_ssl_expire_time_seconds`
What this PR does / why we need it:
now the metric in the namespace label gives the value of where the controller is located but not the ingress itself, which is misleading for an engineer who studies the NGINXCertificateExpiry alert created on the basis of the nginx_ingress_controller_ssl_expire_time_seconds metric.
Types of changes
- [x] 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)
- [x] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation only
Which issue/s this PR fixes
The hotfix will improve the NGINXCertificateExpiry alert. After the fix, it will contain information about the space where the ingress was created and the name of the secret, instead of where the ingress controller is, which is more convenient for understanding where to look for a faulty secret with a certificate.
How Has This Been Tested?
build and manually check the values of labels that are passed to Prometheus.
Checklist:
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I've read the CONTRIBUTION guide
- [x] I have added unit and/or e2e tests to cover my changes.
- [x] All new and existing tests passed.
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: alexey-gavrilov-flant (7dc34fa7000a80878fb45c27fa67284ddfa967c3)
![linux-foundation-easycla[bot] avatar](https://avatars.githubusercontent.com/in/17893?v=4 "Published by a pub.dev verified publisher"){kind=small}
Deploy Preview for kubernetes-ingress-nginx canceled.
| Name | Link |
|---|---|
| Latest commit | 7dc34fa7000a80878fb45c27fa67284ddfa967c3 |
| Latest deploy log | https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66e9949ff000040009d91cf2 |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
Welcome @alexey-gavrilov-flant!
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. :smiley:
Hi @alexey-gavrilov-flant. 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.
@strongjz Could you please see my changes and advance ci to the next step?
@rikatz Could you please see my changes and advance ci to the next step?
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: alexey-gavrilov-flant Once this PR has been reviewed and has the lgtm label, please assign rikatz for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Hi,
Very few readers can completely understand the current state and the change proposed in this PR.
If you think its worth it, then you can post screenshots of current view and the screenshots of view after changed code. It will help make the PR clear to readers. thanks
Sorry for the delay, got requested for review just now. Your change looks good so far, I'd like to try it locally and come back later.
/assign
@alexey-gavrilov-flant: Could you please rebase your PR? Also I'm not sure if just namespace is a proper name for the label as this could also refer to the namespace the controller is running in. I think therefore we're calling the secret name secret_name. WDYT about naming it secret_namespace then?
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: alexey-gavrilov-flant, Gacko
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [Gacko]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
With this understanding (the namespace was already defined for the metric but never set) we can safely cherry-pick this change to release-1.11 and release-1.10 once it got merged as it's rather a fix than a feature.
@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you.
In response to this:
/cherry-pick release-1.10
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.
@Gacko: once the present PR merges, I will cherry-pick it on top of release-1.11 in a new PR and assign it to you.
In response to this:
/cherry-pick release-1.11
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.
@Gacko: new pull request created: #11985
In response to this:
/cherry-pick release-1.10
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.
@Gacko: new pull request created: #11986
In response to this:
/cherry-pick release-1.11
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.