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.

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

Healthcheck fails when tls is enabled in the helm chart on v0.15.0 #1533

Closed
weikinhuang opened this issue Jan 3, 2024 · 4 comments · Fixed by #1535
Closed

Healthcheck fails when tls is enabled in the helm chart on v0.15.0 #1533

weikinhuang opened this issue Jan 3, 2024 · 4 comments · Fixed by #1535
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@weikinhuang
Copy link

What happened:

When using tls.enable=true, the health check will never pass.

Logs:

I0103 15:21:52.156943       1 component.go:36] [core][Server #1] grpc: Server.Serve failed to create ServerTransport: connection error: desc = "ServerHandshake(\"10.200.1.25:37784\") failed: tls: first record does not look like a TLS handshake"

10.200.1.25 is the IP of the host this pod is running on.

What you expected to happen:

Health checks should pass with a tls option.

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

With helm values set to:

tls:
  enable: true

Anything else we need to know?:

In https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/deployment/helm/node-feature-discovery/templates/master.yaml#L44-L54, when this was changed to use native health checks, the consideration for tls was not taken into account.

Environment:

  • Kubernetes version (use kubectl version): 1.28.x
  • Cloud provider or hardware configuration: baremetal
  • OS (e.g: cat /etc/os-release): Ubuntu 22.04
  • Kernel (e.g. uname -a):
  • Install tools:
  • Network plugin and version (if this is a network-related bug): Cilium
  • Others:
@weikinhuang weikinhuang added the kind/bug Categorizes issue or PR as related to a bug. label Jan 3, 2024
@marquiz
Copy link
Contributor

marquiz commented Jan 3, 2024

@weikinhuang thanks for reporting this issue. The deprecated gRPC interface and TLS does not have too much coverage. Are you using the gRPC interface (with enableNodeFeatureApi: false in values.yaml)? If not, the TLS isn't really doing anything and you could just drop the tls.enable=true setting.

Nevertheless, this is an issue we need to fix as the K8s built-in gRPC liveness probe doesn't support TLS (afaiu) 🫤 Two options that immediately came to my mind:

  1. Revert back to using grpc-health-probe (effectively reverting Replace gRPC health probe utility with k8s built-in health probe #1046) until gRPC support is dropped
  2. Serve non-TLS gRPC endpoint on a different port, just for liveness probe purposes

/cc @ArangoGutierrez @fmuyassarov

@weikinhuang
Copy link
Author

weikinhuang commented Jan 3, 2024

@marquiz I am not, I'm just using NFD as a operator with CRDs, most of everything else is the default values. In that case I'll leave the tls flag disabled then.

Thanks for the quick update.

@fmuyassarov
Copy link
Member

@weikinhuang thanks for reporting this issue. The deprecated gRPC interface and TLS does not have too much coverage. Are you using the gRPC interface (with enableNodeFeatureApi: false in values.yaml)? If not, the TLS isn't really doing anything and you could just drop the tls.enable=true setting.

Nevertheless, this is an issue we need to fix as the K8s built-in gRPC liveness probe doesn't support TLS (afaiu) 🫤 Two options that immediately came to my mind:

  1. Revert back to using grpc-health-probe (effectively reverting Replace gRPC health probe utility with k8s built-in health probe #1046) until gRPC support is dropped
  2. Serve non-TLS gRPC endpoint on a different port, just for liveness probe purposes

/cc @ArangoGutierrez @fmuyassarov

Ah sorry for that. I'm +1 for reverting the #1046 for the time being.

@marquiz
Copy link
Contributor

marquiz commented Jan 4, 2024

@fmuyassarov I decided to try the other option, see #1535

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants