Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Health probe configuration should be applied to specific port. #949

Closed
MartinForReal opened this issue Dec 22, 2021 · 12 comments · Fixed by #963 or #1139
Closed

Health probe configuration should be applied to specific port. #949

MartinForReal opened this issue Dec 22, 2021 · 12 comments · Fixed by #963 or #1139
Assignees
@MartinForReal
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@MartinForReal
Copy link
Contributor

What happened:

Health probe configurations are applied to all ports of the service.

What you expected to happen:

Health probe configuration should be applied to specific port.

How to reproduce it (as minimally and precisely as possible):

apiVersion: v1
kind: Service
metadata:
  name: sctpservice
  annotations:
    service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol: "tcp"
spec:
  type: LoadBalancer
  selector:
    app: sctpserver
  ports:
    - name: sctpserver
      protocol: SCTP
      port: 30102
      targetPort: 30102
    - name: webserver
      protocol: TCP
      port: 80
      targetPort: 30102

Anything else we need to know?:

Nothing else

Environment:

Not needed.
@MartinForReal MartinForReal added the kind/bug Categorizes issue or PR as related to a bug. label Dec 22, 2021
@MartinForReal
Copy link
Contributor Author

we may need to alter the annotation pattern to

azure.service.beta.kubernetes.io/port/{name}/health-probe-protocol: "tcp"

And to keep the provider backward compatible, We could apply health probe configuration to the first port.

@feiskyer @nilo19 Could you please give some advice?

@feiskyer
Copy link
Member

Agreed, because no port is defined in the annotation, the protocol is applied to all ports. +1 to add a new annotation for overwriting the protocol for a specific port.

@MartinForReal
Copy link
Contributor Author

/assign

@MartinForReal MartinForReal mentioned this issue Dec 27, 2021
Closed
@MartinForReal
Copy link
Contributor Author

Following parameter should be per-port specific and should be depreciated later.

cloud-provider-azure/pkg/consts/consts.go

Lines 258 to 273 in bb44e05

// ServiceAnnotationLoadBalancerHealthProbeProtocol determines the network protocol that the load balancer health probe use.
// If not set, the local service would use the HTTP and the cluster service would use the TCP by default.
ServiceAnnotationLoadBalancerHealthProbeProtocol = "service.beta.kubernetes.io/azure-load-balancer-health-probe-protocol"
// ServiceAnnotationLoadBalancerHealthProbeInterval determines the probe interval of the load balancer health probe.
// The minimum probe interval is 5 seconds and the default value is 15. The total duration of all intervals cannot exceed 120 seconds.
ServiceAnnotationLoadBalancerHealthProbeInterval = "service.beta.kubernetes.io/azure-load-balancer-health-probe-interval"
// ServiceAnnotationLoadBalancerHealthProbeNumOfProbe determines the minimum number of unhealthy responses which load balancer cannot tolerate.
// The minimum number of probe is 1. The total duration of all intervals cannot exceed 120 seconds.
ServiceAnnotationLoadBalancerHealthProbeNumOfProbe = "service.beta.kubernetes.io/azure-load-balancer-health-probe-num-of-probe"
// ServiceAnnotationLoadBalancerHealthProbeRequestPath determines the request path of the load balancer health probe.
// This is only useful for the HTTP and HTTPS, and would be ignored when using TCP. If not set,
// `/healthz` would be configured by default.
ServiceAnnotationLoadBalancerHealthProbeRequestPath = "service.beta.kubernetes.io/azure-load-balancer-health-probe-request-path"

In order to keep backward compatible, probe configuration is applied only when there is only one port defined in service. Otherwise, the provider should reject the input params and throw an error.

We also need to introduce a few constraints.

  • Probe rules are only applied to the port with port name defined.
  • Only one probe rule is applied to single port.

@feiskyer @nilo19 Any advice is appreciated. Thanks!

@nilo19
Copy link
Contributor

nilo19 commented Dec 27, 2021

sgtm

@feiskyer
Copy link
Member

Probe rules are only applied to the port with port name defined.

Why introducing this limitation? Could we fallback to port number if the name is not defined?

@nilo19
Copy link
Contributor

nilo19 commented Dec 27, 2021

I think that's because of the new annotation pattern

azure.service.beta.kubernetes.io/port/{name}/health-probe-protocol: "tcp"

@feiskyer
Copy link
Member

OK, that makes sense

@MartinForReal
Copy link
Contributor Author

I think that's because of the new annotation pattern

azure.service.beta.kubernetes.io/port/{name}/health-probe-protocol: "tcp"

Exactly!

I will raise a pr later. Thanks for your time!

@MartinForReal
Copy link
Contributor Author

MartinForReal commented Dec 28, 2021 •
edited
Loading

One more scenario we should consider is HA mode of routing rule in internal SLB.

When HA mode in routing rule is enabled, traffic on all ports of frontend IP is load balanced. Should we support this mode? Because lb accepts traffic from unmanaged port range which is different from what is expected from kubernetes service object and It also makes sensitive ports of VM hosts exposed to vnet and we need to add extra inbound rules to eliminate security concerns we mentioned above.

@feiskyer @nilo19 Could you please share your option on this ? Thanks!

@feiskyer
Copy link
Member

Refer HA ports annotationservice.beta.kubernetes.io/azure-load-balancer-enable-high-availability-ports. But for health probe, you should only setup one port in one probing rule. Hence, I think it should be supported similar as normal LB rules.

@MartinForReal
Copy link
Contributor Author

MartinForReal commented Dec 29, 2021 •
edited
Loading

we may need to alter the annotation pattern to

azure.service.beta.kubernetes.io/port/{name}/health-probe-protocol: "tcp"

And to keep the provider backward compatible, We could apply health probe configuration to the first port.

@feiskyer @nilo19 Could you please give some advice?

Since service port is unique and we can use following pattern instead

azure.service.beta.kubernetes.io/port/{port}/health-probe-protocol: "tcp"

@MartinForReal MartinForReal mentioned this issue Dec 31, 2021
Merged
This was referenced Feb 10, 2022
Merged
Closed
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@MartinForReal MartinForReal

Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
No milestone
3 participants
@feiskyer @MartinForReal @nilo19