KEP-4939: Support TLS in gRPC probe #5029

kkoch986 · 2025-01-08T22:38:45Z

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 · 2025-01-08T22:38:54Z

Welcome @kkoch986!

It looks like this is your first PR to kubernetes/enhancements 🎉. 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/enhancements 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 · 2025-01-08T22:38:54Z

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 /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-sigs/prow repository.

aojea · 2025-01-09T10:46:00Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+either through providing a CA certificate from a secret
+or disabling certificate verification.


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

👍 that makes sense to me, just pushed up some fixups to update the KEP 09308e9

aojea · 2025-01-09T10:47:10Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+[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.


+100 on supporting TLS without validation certiticates, skeptical about the ROI on adding certificate validation, I suggest moving only with one option

updated that section to remove the part about providing certs 09308e9

aojea · 2025-01-09T10:48:17Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+If configured to to no-verify TLS:
+
+```go
+tlsConfig := &tls.Config{
+    InsecureSkipVerify: true,
+}
+tlsCredentials := credentials.NewTLS(tlsConfig)
+
+opts := []grpc.DialOption{
+    // ...
+    grpc.WithTransportCredentials(tlsCredentials),
+    // ...
+}
+```


this seems simple to implement, the other need to deal with the input of passing certificates that will make probes more complex

aojea · 2025-01-09T10:48:51Z

keps/sig-node/4939-tls-in-grpc-probe/kep.yaml

+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"


I assume this is copy and paste

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 😄

09308e9

aojea · 2025-01-09T10:49:11Z

keps/sig-node/4939-tls-in-grpc-probe/kep.yaml

+  - "@kkoch986"
+owning-sig: sig-node
+participating-sigs:
+  - sig-node


you can add sig-network as participating and enlist me as reviewer

done, thanks! i pushed up some fixups so its easier to see the changes, i can rebase once everything is looking good

aojea · 2025-01-09T10:50:54Z

/ok-to-test

kkoch986 · 2025-01-30T01:42:12Z

pushed up one more fixup to define the config option better. @aojea any other changes i should make to get this KEP merged?

aojea · 2025-01-30T11:27:05Z

/lgtm

I think that not having a TLS option reduce significantly the usability of this probes and is worth having parity with the http probes, also the changes requested seem to be very minimal, please @bowei take a look as you were involved with the original implementation so you may have some more historic context

/assign @thockin @dchen1107

for approval

k8s-ci-robot · 2025-01-30T13:52:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kkoch986
Once this PR has been reviewed and has the lgtm label, please ask for approval from dchen1107. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

keps/sig-node/OWNERS

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

kkoch986 · 2025-01-30T13:53:23Z

oops @aojea looks like when i rebased it removed the lgtm tag sorry about that, didnt change anything just squashed the fixups down

aojea · 2025-01-30T17:58:46Z

/lgtm

thockin

FTR: I am reading this for API review in particular, but will hold my API comments until @bowei

thockin · 2025-01-30T20:47:49Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+They can be used to indicate whether or not TLS should be used.
+
+### User Stories (Optional)


Either fill these optional section in or remove them :)

thockin · 2025-01-30T21:11:46Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+[testing-guidelines]: https://git.k8s.io/community/contributors/devel/sig-testing/testing.md
+-->
+
+[ ] I/we understand the owners of the involved components may require updates to


you gotta check the box :) It's like your bank's EULA - what are you going to do, not sign it?

thockin · 2025-01-30T21:38:10Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+implementing this enhancement to ensure the enhancements have also solid foundations.
+-->
+
+##### Unit tests


please describe the tests you think will be needed. Take a look at nearby tests and see how you would extend them - unit seems obvious, maybe node_e2e is sufficient?

thockin · 2025-01-30T21:39:22Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+- <test>: <link to test coverage>
+
+### Graduation Criteria


Please fill in the rest of the doc.

thockin · 2025-01-30T21:40:01Z

keps/sig-node/4939-tls-in-grpc-probe/kep.yaml

+reviewers:
+  - aojea
+approvers:
+  - TBD


@SergeyKanzhelev @dchen1107 @mrunalp can you suggest an approver?

bowei · 2025-01-30T22:10:32Z

Antonio probably did the heavy lifting here. I didn't find much other than some prods filling out the testing and production related templates. Thanks!

bowei · 2025-01-30T21:49:02Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+### Test Plan
+
+<!--


It would be good to spec out what tests you plan on adding -- probably mirror those already done for gRPC.

bowei · 2025-01-30T22:06:55Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+We should add a boolean field to the
+[GRPCAction](https://github.com/kubernetes/kubernetes/blob/f422a58398d457a88ac7b05afca85a0e6b805605/pkg/apis/core/types.go#L2665-L2676)
+named `useTLS` which is optional and would default to `false`.


Proposal looks generally OK.

There are couple of things. Note: I don't think these will change the general design and KEP:

In HTTP, we chose based on scheme, but it looks like gRPC does not have such a notion exposed, so there is no need to push for consistency in naming there.

gRPC over QUIC only works with TLS. This means if/when we support gRPC over HTTP/3, this will require adding some validation that UseTLS is true.

bowei · 2025-01-30T22:07:54Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+### Graduation Criteria
+
+<!--


Please fill this out -- you can use the original gRPC probe KEP as a reference.

bowei · 2025-01-30T22:09:02Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+
+## Production Readiness Review Questionnaire
+
+<!--


Make sure to get a PRR review.

thockin · 2025-01-30T21:49:57Z

keps/sig-node/4939-tls-in-grpc-probe/README.md

+-->
+
+We should add a boolean field to the
+[GRPCAction](https://github.com/kubernetes/kubernetes/blob/f422a58398d457a88ac7b05afca85a0e6b805605/pkg/apis/core/types.go#L2665-L2676)


Boolean flags are generally a bad idea. They almost always end up insufficient. For example, I can imagine a future where someone wants to do secure verification against a known CA or set of CAs. Perhaps they load a volume with their CA's pubkey or something.

In this particular case it doesn't seem terrible, it's just hard to know what we don't know.

For example, this could evolve to:

grpcAction: port: 12345 useTLS: true tlsConfig: mode: Verify certVolume: my-certs

But let's consider options. What if this was instead framed as:

grpcAction: port: 12345 tls: mode: NoVerify

The existence of the tls field implies a demand for TLS. The mode only has one value, so it's optional. People could write: tls: {}

It's not markedly better, so I'm not demanding it (yet?).

Are there ANY OTHER configuration knobs we want in the short term?

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jan 8, 2025

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 8, 2025

k8s-ci-robot requested review from dchen1107 and mrunalp January 8, 2025 22:38

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 8, 2025

aojea reviewed Jan 9, 2025

View reviewed changes

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 Jan 9, 2025

kkoch986 requested a review from aojea January 9, 2025 14:25

k8s-ci-robot assigned dchen1107, thockin and aojea Jan 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2025

KEP-4939: Support TLS in gRPC probe

4855713

kkoch986 force-pushed the grpc-probe-auth branch from 093742b to 4855713 Compare January 30, 2025 13:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2025

thockin assigned bowei Jan 30, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2025

thockin reviewed Jan 30, 2025

View reviewed changes

bowei reviewed Jan 30, 2025

View reviewed changes

thockin reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-4939: Support TLS in gRPC probe #5029

KEP-4939: Support TLS in gRPC probe #5029

kkoch986 commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

aojea Jan 9, 2025

kkoch986 Jan 9, 2025

aojea Jan 9, 2025

kkoch986 Jan 9, 2025

aojea Jan 9, 2025

aojea Jan 9, 2025

kkoch986 Jan 9, 2025

aojea Jan 9, 2025

kkoch986 Jan 9, 2025 •

edited

Loading

aojea commented Jan 9, 2025

kkoch986 commented Jan 30, 2025 •

edited

Loading

aojea commented Jan 30, 2025

k8s-ci-robot commented Jan 30, 2025

kkoch986 commented Jan 30, 2025

aojea commented Jan 30, 2025

thockin left a comment

thockin Jan 30, 2025

thockin Jan 30, 2025

thockin Jan 30, 2025

thockin Jan 30, 2025

thockin Jan 30, 2025

bowei commented Jan 30, 2025

bowei Jan 30, 2025

bowei Jan 30, 2025

bowei Jan 30, 2025

bowei Jan 30, 2025

thockin Jan 30, 2025

		either through providing a CA certificate from a secret
		or disabling certificate verification.


		They can be used to indicate whether or not TLS should be used.

		### User Stories (Optional)

KEP-4939: Support TLS in gRPC probe #5029

Are you sure you want to change the base?

KEP-4939: Support TLS in gRPC probe #5029

Conversation

kkoch986 commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

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

kkoch986 Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

aojea commented Jan 9, 2025

kkoch986 commented Jan 30, 2025 • edited Loading

aojea commented Jan 30, 2025

k8s-ci-robot commented Jan 30, 2025

kkoch986 commented Jan 30, 2025

aojea commented Jan 30, 2025

thockin left a 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

bowei commented Jan 30, 2025

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

kkoch986 Jan 9, 2025 •

edited

Loading

kkoch986 commented Jan 30, 2025 •

edited

Loading