KEP-4939: Support TLS in gRPC probe #5029
Conversation
kkoch986
commented
Jan 8, 2025
kkoch986
commented
- One-line PR description: Initial KEP for TLS support in gRPC probes
- Issue link: Support TLS Credentials in gRPC Probe #4939
- Other comments: see also GRPC healthprobe cannot handle TLS kubernetes#128365
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kkoch986 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
sig/node
|
Welcome @kkoch986! |
|
Hi @kkoch986. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-ci-robot
added
the
size/XL
| either through providing a CA certificate from a secret | ||
| or disabling certificate verification. |
There was a problem hiding this comment.
we don't validate certificates with HTTPS probes, I don't feel we should add this complexity in this iteration and just make grpc probes the same as http probes
There was a problem hiding this comment.
👍 that makes sense to me, just pushed up some fixups to update the KEP 09308e9
| [Probe GRPCAction](https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/#Probe). | ||
|
|
||
| They can be used to indicate whether or not TLS should be used | ||
| as well as provide a reference to the certificate to use or the ability to disable verification. |
There was a problem hiding this comment.
+100 on supporting TLS without validation certiticates, skeptical about the ROI on adding certificate validation, I suggest moving only with one option
There was a problem hiding this comment.
updated that section to remove the part about providing certs 09308e9
| If configured to to no-verify TLS: | ||
|
|
||
| ```go | ||
| tlsConfig := &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| } | ||
| tlsCredentials := credentials.NewTLS(tlsConfig) | ||
|
|
||
| opts := []grpc.DialOption{ | ||
| // ... | ||
| grpc.WithTransportCredentials(tlsCredentials), | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
this seems simple to implement, the other need to deal with the input of passing certificates that will make probes more complex
| latest-milestone: "v1.19" | ||
|
|
||
| # The milestone at which this feature was, or is targeted to be, at each stage. | ||
| milestone: | ||
| alpha: "v1.19" | ||
| beta: "v1.20" | ||
| stable: "v1.22" |
There was a problem hiding this comment.
yea i wasnt 100% sure what to put in there, for now just updated this to 1.33 although since this is my first contribution may take me some more time to actually implement 😄
| - "@kkoch986" | ||
| owning-sig: sig-node | ||
| participating-sigs: | ||
| - sig-node |
There was a problem hiding this comment.
you can add sig-network as participating and enlist me as reviewer
There was a problem hiding this comment.
done, thanks! i pushed up some fixups so its easier to see the changes, i can rebase once everything is looking good
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
