Add missing features to ingress metrics #1233

spencerhance · 2020-08-26T11:33:40Z

These features have been implemented but are currently not included in the metrics

SSLPolicy
Regional Static IP
Custom Health Checks

This PR adds metrics for them
All of them can be cherry-picked to v1.9 + v1.10

HTTPS Redirects will be added in a follow up since it is currently 1.10 only

k8s-ci-robot · 2020-08-26T11:33:48Z

Hi @spencerhance. 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.

spencerhance · 2020-08-26T18:32:15Z

/assign @freehan

spencerhance · 2020-08-26T21:15:26Z

/assign @prameshj

Pavithra, any chance you have bandwidth to review this?

prameshj · 2020-08-26T22:30:02Z

/ok-to-test

prameshj · 2020-08-26T22:30:31Z

Would it be better to split into 2 PRs - one with metrics that can be cherrypicked to 1.9 and one with those that cannot?

prameshj · 2020-08-26T22:35:45Z

pkg/metrics/metrics_test.go

-						StaticIPNameKey: "user-spec-static-ip",
-						staticIPKey:     "user-spec-static-ip",
+						StaticGlobalIPNameKey: "user-spec-static-ip",
+						staticIPKey:           "user-spec-static-ip",


nit: should this value be an ip address?

Both of the annotations represent the name of the address, rather than the address itself

spencerhance · 2020-08-26T22:48:55Z

@prameshj Good call! I removed redirects from this. Once it is merged I'll submit a follow up PR.

prameshj · 2020-08-26T22:53:06Z

pkg/metrics/features.go

@@ -191,15 +201,24 @@ func featuresForIngress(ing *v1beta1.Ingress) []feature {
 	if val, ok := ingAnnotations[staticIPKey]; ok && val != "" {


Does this mean user has to specify both staticIPKey and StaticGlobalIPNameKey in order to use a given ip address name? Why do we need 2?

oh, no staticIPKey is used as a status annotation

prameshj · 2020-08-26T23:54:57Z

pkg/metrics/features.go

+
+	// Check for regional static IP
+	// We do this separately from the global static IP because regional static IP
+	// does not support the StaticIPKey annotation or a managed regional IP


Can you rephrase this to say - "Controller does not populate StaticIPKey annotation when processing Regional static IP"?

Also, these annotations are also defined in annotations/ingress.go. Do they need to be redefined in features.go?

@skmatti what was the reason for this? Was there a circular dependency?

Updated the comment

This was to enable backporting for the metrics PR to the release branches that do not have the annotations defined. Maybe it does not apply for these metrics.

Okay I removed the annotation

prameshj · 2020-08-28T21:41:25Z

pkg/metrics/features.go

+
+	// Check for regional static IP
+	// We do this separately from the global static IP because Controller does not
+	// populate StaticIPKey annotation when processing Regional static IP regional static IP


These features have been implemented but are currently not included in the metrics - SSLPolicy - Regional Static IP - Custom Health Checks

freehan

/lgtm
/approve

k8s-ci-robot · 2020-08-28T23:20:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, spencerhance

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

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

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-9

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-10

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 26, 2020

k8s-ci-robot requested review from bowei and MrHohn August 26, 2020 11:33

k8s-ci-robot assigned freehan Aug 26, 2020

k8s-ci-robot assigned prameshj Aug 26, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 26, 2020

prameshj reviewed Aug 26, 2020

View reviewed changes

spencerhance force-pushed the 1-9-beta-metrics branch from 57a10f6 to ddf0e15 Compare August 26, 2020 22:48

prameshj reviewed Aug 26, 2020

View reviewed changes

spencerhance force-pushed the 1-9-beta-metrics branch from ddf0e15 to 53a667b Compare August 26, 2020 23:23

prameshj reviewed Aug 26, 2020

View reviewed changes

spencerhance force-pushed the 1-9-beta-metrics branch 2 times, most recently from fd32fc2 to 4989b38 Compare August 28, 2020 02:26

prameshj approved these changes Aug 28, 2020

View reviewed changes

Add missing features to ingress metrics

355178a

These features have been implemented but are currently not included in the metrics - SSLPolicy - Regional Static IP - Custom Health Checks

spencerhance force-pushed the 1-9-beta-metrics branch from 4989b38 to 355178a Compare August 28, 2020 21:45

freehan reviewed Aug 28, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2020

k8s-ci-robot merged commit 906ebe7 into kubernetes:master Aug 28, 2020

spencerhance deleted the 1-9-beta-metrics branch August 28, 2020 23:22

This was referenced Aug 28, 2020

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-9 #1237

Merged

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-10 #1238

Merged

k8s-ci-robot added a commit that referenced this pull request Aug 31, 2020

Merge pull request #1237 from spencerhance/cp-1233-release-1-9

0bc0085

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-9

k8s-ci-robot added a commit that referenced this pull request Aug 31, 2020

Merge pull request #1238 from spencerhance/cp-1233-release-1-10

0a651f4

Cherry-pick #1233 [Add missing features to ingress metrics] into release-1-10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing features to ingress metrics #1233

Add missing features to ingress metrics #1233

spencerhance commented Aug 26, 2020 •

edited

Loading

k8s-ci-robot commented Aug 26, 2020

spencerhance commented Aug 26, 2020

spencerhance commented Aug 26, 2020

prameshj commented Aug 26, 2020

prameshj commented Aug 26, 2020

prameshj Aug 26, 2020

spencerhance Aug 26, 2020

spencerhance commented Aug 26, 2020

prameshj Aug 26, 2020

spencerhance Aug 26, 2020

prameshj Aug 26, 2020

spencerhance Aug 27, 2020

spencerhance Aug 27, 2020

skmatti Aug 28, 2020

spencerhance Aug 28, 2020

prameshj Aug 28, 2020

spencerhance Aug 28, 2020

freehan left a comment

k8s-ci-robot commented Aug 28, 2020

		@@ -191,15 +201,24 @@ func featuresForIngress(ing *v1beta1.Ingress) []feature {
		if val, ok := ingAnnotations[staticIPKey]; ok && val != "" {

Add missing features to ingress metrics #1233

Add missing features to ingress metrics #1233

Conversation

spencerhance commented Aug 26, 2020 • edited Loading

k8s-ci-robot commented Aug 26, 2020

spencerhance commented Aug 26, 2020

spencerhance commented Aug 26, 2020

prameshj commented Aug 26, 2020

prameshj commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerhance commented Aug 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 28, 2020

spencerhance commented Aug 26, 2020 •

edited

Loading