Add support for specifying probe protocol / probe port via annotation per service port

[rainest]

What type of PR is this?

/kind feature

What this PR does / why we need it:

This adds service.beta.kubernetes.io/port_<num>_health-probe_protocol and service.beta.kubernetes.io/port_<num>_health-probe_port annotations for LoadBalancer Services. These allow overriding the health check port or protocol for a given port. This is useful for ports that cannot respond affirmatively to the health checks and expose health information elsewhere.

Which issue(s) this PR fixes:

Fixes #1505

Special notes for your reviewer:

This continues #1508 and attempts to address the open issue with it. I rebased onto upstream master and added commits to de-duplicate checks and correct an apparent error in the original docs (/ is the default check path, not /healthz).

I'm not quite sure I understand the duplication concern raised in #1508 (review), and it looks like the line indicated is maybe outdated. As I read it, we pass the rule name (for internal and external LBs) to the check generator, which then just uses that string as-is for the probe name.

That name's generator will return something like SERVICE_UID-??-tcp-443 (the service, subnet, appProtocol, and port--not sure about the second's appearance, but it's optional anyway). That doesn't seem like it can conflict because there will only be one probe per LB rule name, and the the probes are named for the rule they check, not the port they check.

For example, if we have a Service with HTTP appProtocol ports on 8443 and 8009, and want to override the former to use an HTTP check on 8009 instead, the resulting probes would be:

• ABC-123-http-8443 (an HTTP check on port 8009)
• ABC-123-http-8009 (an HTTP check on port 8009)

Based on the discussion after, my additional change looks at the generated probes and only keeps those with unique properties, so that the above would only generate one probe. Naming is still a bit iffy, since you'll still get a probe named after only one of the rules, even though it's pulling double-duty. What name scheme would be desirable (changing it requires updating a bunch of tests, so I wanted to confirm before writing it)? I was thinking either ABC-123-http-8009-8443 (it indicates all ports it checks) or ABC-123-http-8009-10-5-/, indicating all of the probe properties. The latter doesn't work great, however, since it needs to account for HTTP paths, which could get quite unwieldy. It could indicate the port and protocol only, but that would result in name conflicts.

Does this PR introduce a user-facing change?

Added: support for new annotations **service.beta.kubernetes.io/port_<num>_health-probe_protocol** and **service.beta.kubernetes.io/port_<num>_health-probe_port** to allow explicitly setting the health probe protocol individually for each service port. Useful for services like Istio which have health check seperate from the main service port.

[netlify]

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

Name	Link
🔨 Latest commit	ad38bed
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/636a1db8c5ba4f00089a6182

[k8s-ci-robot]

Welcome @rainest!

It looks like this is your first PR to kubernetes-sigs/cloud-provider-azure 🎉. 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-sigs/cloud-provider-azure 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 @rainest. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[MartinForReal]

@rainest Thanks for your PR! Could you please merge the commit into one commit?

[coveralls]

Coverage increased (+0.03%) to 79.668% when pulling ad38bed on rainest:feature/azure-load-balancer-per-port-protocol into e8e8bec on kubernetes-sigs:master.

[MartinForReal]

/retest

[rainest]

@MartinForReal I can once it's complete, yes. Did you have comments on the de-duplication/naming question and any further work needed there? Or is the change introduced in 7f80566 all of what you were looking for?

[MartinForReal]

Yes! That's what I'm looking for.

[rainest]

Alright, sounds good, squashed and rebased onto the current tip of master here.

[MartinForReal]

/retest

[MartinForReal]

LGTM.
/assign @feiskyer

[MartinForReal]

please rebase the branch in order to fix error reported by ci pipeline.

[rainest]

FYI I'll be out away from internet access through the 25th, so if this needs further changes I'll address them then.

[rainest]

@MartinForReal checking in on this, are there any further changes it would need?

[MartinForReal]

I can't think of any, @feiskyer @nilo19 Could you please take a look?

[MartinForReal]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MartinForReal, rainest

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 [MartinForReal]

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

[MartinForReal]

/ok-to-test

[rainest]

/retest

[rainest]

Unsure what would have broken the one test for service.beta.kubernetes.io/azure-load-balancer-resource-group. The Service never receives an IP assignment. Other tests that rely on the same check do get a Service LB IP and succeed, and AFAIK the PR shouldn't do anything special when a resource group is set.

Is there information elsewhere in the test logs that would show this particular Service's state and/or cloud provider logs related to assigning it an IP? I'm unfortunately not too familiar with test suite output, but it looks like it might only be logs from the test client.

[MartinForReal]

/retest

[MartinForReal]

/test pull-cloud-provider-azure-e2e-ccm-capz

[MartinForReal]

/lgtm

[MartinForReal]

/test pull-cloud-provider-azure-e2e-ccm-capz

[rainest]

@MartinForReal thanks for the help with the tests. Do you know roughly when changes here propagate out to being available in Azure for users? I didn't find any scheduled update lifecycle in the SIG FAQ, but it looks like new minor provider releases go out roughly every 6mo and are incorporated into Azure shortly after from reviewing the previous releases.

[MartinForReal]

/cherrypick release-1.25

[MartinForReal]

/cherrypick release-1.24

[MartinForReal]

/cherrypick release-1.23

[MartinForReal]

/cherrypick release-1.1

[k8s-infra-cherrypick-robot]

@MartinForReal: #2452 failed to apply on top of branch "release-1.25":

Applying: Add port and protocol probe annotations
Using index info to reconstruct a base tree...
M	pkg/consts/consts.go
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
Auto-merging pkg/consts/consts.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add port and protocol probe annotations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.25

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-infra-cherrypick-robot]

@MartinForReal: #2452 failed to apply on top of branch "release-1.24":

Applying: Add port and protocol probe annotations
Using index info to reconstruct a base tree...
M	pkg/consts/consts.go
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
M	site/content/en/topics/loadbalancer.md
Falling back to patching base and 3-way merge...
Auto-merging site/content/en/topics/loadbalancer.md
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
Auto-merging pkg/consts/consts.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add port and protocol probe annotations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.24

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-infra-cherrypick-robot]

@MartinForReal: #2452 failed to apply on top of branch "release-1.23":

Applying: Add port and protocol probe annotations
Using index info to reconstruct a base tree...
M	pkg/consts/consts.go
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
M	site/content/en/topics/loadbalancer.md
Falling back to patching base and 3-way merge...
Auto-merging site/content/en/topics/loadbalancer.md
CONFLICT (content): Merge conflict in site/content/en/topics/loadbalancer.md
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer.go
Auto-merging pkg/consts/consts.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add port and protocol probe annotations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.23

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-infra-cherrypick-robot]

@MartinForReal: #2452 failed to apply on top of branch "release-1.1":

Applying: Add port and protocol probe annotations
Using index info to reconstruct a base tree...
M	pkg/consts/consts.go
M	pkg/provider/azure_loadbalancer.go
M	pkg/provider/azure_loadbalancer_test.go
M	site/content/en/topics/loadbalancer.md
Falling back to patching base and 3-way merge...
Auto-merging site/content/en/topics/loadbalancer.md
CONFLICT (content): Merge conflict in site/content/en/topics/loadbalancer.md
Auto-merging pkg/provider/azure_loadbalancer_test.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer_test.go
Auto-merging pkg/provider/azure_loadbalancer.go
CONFLICT (content): Merge conflict in pkg/provider/azure_loadbalancer.go
Auto-merging pkg/consts/consts.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add port and protocol probe annotations
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.1

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.

[MartinForReal]

@MartinForReal thanks for the help with the tests. Do you know roughly when changes here propagate out to being available in Azure for users? I didn't find any scheduled update lifecycle in the SIG FAQ, but it looks like new minor provider releases go out roughly every 6mo and are incorporated into Azure shortly after from reviewing the previous releases.

This pr should be cherrypicked to all of release branches and then will be shipped into next cloud provider release(1 week)

and it will take another 2 weeks before it is deployed to all of the regions.