Do not expose ingress path metric when service is nil #1841

evir35 · 2022-09-22T13:07:51Z

What this PR does / why we need it:

Currently, the kube-state-metrics can down suddenly when an ingress without backend service is created.

https://github.com/kubernetes/api/blob/79091dac6a32a1365edbc6de9576ffa74768a75c/networking/v1/types.go#L484-L496

The Service of IngressBackend is a nullable variable. But, the current code doesn't check the Service is nil or not.
So, I created PR to solve this using an additional if statement that checks Service is nil or not.
And I checked both path.Backend.Service.Name and path.Backend.Service.Port.Number variables can't be nil.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)

This PR doesn't affect the cardinality of KSM.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1660

linux-foundation-easycla · 2022-09-22T13:07:55Z

The committers listed above are authorized under a signed CLA.

✅ login: evir35 (10647b6)

k8s-ci-robot · 2022-09-22T13:08:00Z

Welcome @evir35!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. 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/kube-state-metrics 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. 😃

evir35 · 2022-09-22T13:18:37Z

/easycla

fpetkovski · 2022-09-23T07:57:04Z

internal/store/ingress.go

-								LabelValues: []string{rule.Host, path.Path, path.Backend.Service.Name, strconv.Itoa(int(path.Backend.Service.Port.Number))},
-								Value:       1,
-							})
+							if path.Backend.Service != nil {


Thanks for this PR. I wonder if we can just set service_name and service_port to blank values instead of not adding exporting a metric at all.

Thanks for your comment! 🙇‍♂️

I updated it to return blank values("<none>") refer to other code.
Please tell me if I don't understand correctly.

And I added test code to the ingress_test.go file.

fpetkovski

Just one small suggestion

fpetkovski · 2022-09-23T11:06:32Z

internal/store/ingress.go

+							} else {
+								ms = append(ms, &metric.Metric{
+									LabelKeys:   []string{"host", "path", "service_name", "service_port"},
+									LabelValues: []string{rule.Host, path.Path, "<none>", "<none>"},


Thanks, this should be sufficient:

Suggested change

LabelValues: []string{rule.Host, path.Path, "<none>", "<none>"},

LabelValues: []string{rule.Host, path.Path, "", ""},

Oh, I updated.

Thank you!

fpetkovski

Thanks 👍

/lgtm

k8s-ci-robot · 2022-09-23T13:12:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evir35, fpetkovski

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~~ [fpetkovski]

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

…ress-path-metric-when-service-is-nil Do not expose ingress path metric when service is nil

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 22, 2022

k8s-ci-robot requested review from dgrisonnet and fpetkovski September 22, 2022 13:08

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 22, 2022

Do not expose ingress path metric when service is nil

d7a079e

evir35 force-pushed the 1660-do-not-expose-ingress-path-metric-when-service-is-nil branch from 10647b6 to d7a079e Compare September 22, 2022 16:46

fpetkovski reviewed Sep 23, 2022

View reviewed changes

Fix return blank string when Service is nil

242cea3

evir35 force-pushed the 1660-do-not-expose-ingress-path-metric-when-service-is-nil branch from 7e663e9 to 242cea3 Compare September 23, 2022 10:36

evir35 requested review from fpetkovski and removed request for dgrisonnet September 23, 2022 10:50

fpetkovski reviewed Sep 23, 2022

View reviewed changes

Fix none to blank string

7f453f0

evir35 requested a review from fpetkovski September 23, 2022 12:40

fpetkovski reviewed Sep 23, 2022

View reviewed changes

k8s-ci-robot assigned fpetkovski Sep 23, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2022

k8s-ci-robot merged commit 12402a5 into kubernetes:master Sep 23, 2022

rexagod pushed a commit to rexagod/kube-state-metrics that referenced this pull request Nov 3, 2022

Merge pull request kubernetes#1841 from evir35/1660-do-not-expose-ing…

2bfc75d

…ress-path-metric-when-service-is-nil Do not expose ingress path metric when service is nil

rexagod pushed a commit to rexagod/kube-state-metrics that referenced this pull request Nov 3, 2022

Merge pull request kubernetes#1841 from evir35/1660-do-not-expose-ing…

64e85f9

…ress-path-metric-when-service-is-nil Do not expose ingress path metric when service is nil

JoaoBraveCoding mentioned this pull request Nov 24, 2022

OCPBUGS-4089: cherry-pick, do not expose ingress path metric when service is nil openshift/kube-state-metrics#81

Merged

clamoriniere mentioned this pull request Feb 16, 2023

fix: public Builder compatibility with the BuilderInterface #1994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not expose ingress path metric when service is nil #1841

Do not expose ingress path metric when service is nil #1841

evir35 commented Sep 22, 2022

linux-foundation-easycla bot commented Sep 22, 2022 •

edited

Loading

k8s-ci-robot commented Sep 22, 2022

evir35 commented Sep 22, 2022

fpetkovski Sep 23, 2022

evir35 Sep 23, 2022

fpetkovski left a comment

fpetkovski Sep 23, 2022

evir35 Sep 23, 2022

fpetkovski left a comment

k8s-ci-robot commented Sep 23, 2022

	LabelValues: []string{rule.Host, path.Path, "<none>", "<none>"},
	LabelValues: []string{rule.Host, path.Path, "", ""},

Do not expose ingress path metric when service is nil #1841

Do not expose ingress path metric when service is nil #1841

Conversation

evir35 commented Sep 22, 2022

linux-foundation-easycla bot commented Sep 22, 2022 • edited Loading

k8s-ci-robot commented Sep 22, 2022

evir35 commented Sep 22, 2022

fpetkovski Sep 23, 2022

Choose a reason for hiding this comment

evir35 Sep 23, 2022

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

fpetkovski Sep 23, 2022

Choose a reason for hiding this comment

evir35 Sep 23, 2022

Choose a reason for hiding this comment

fpetkovski left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 23, 2022

linux-foundation-easycla bot commented Sep 22, 2022 •

edited

Loading