✨ Allow TLS to be entirely configured on webhook server #1897

akalenyu · 2022-05-11T14:38:02Z

Some operators might want to respect cluster-wide TLS ciphers for example,
which means that these will eventually have to be passed down to the webhook server.

Fixes #1754

linux-foundation-easycla · 2022-05-11T14:38:05Z

The committers listed above are authorized under a signed CLA.

✅ login: akalenyu (e19256a)

k8s-ci-robot · 2022-05-11T14:38:10Z

Welcome @akalenyu!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. 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/controller-runtime 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 · 2022-05-11T14:38:11Z

Hi @akalenyu. 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.

alvaroaleman · 2022-05-13T18:00:44Z

pkg/webhook/server.go

@@ -76,6 +79,10 @@ type Server struct {
 	// "", "1.0", "1.1", "1.2" and "1.3" only ("" is equivalent to "1.0" for backwards compatibility)
 	TLSMinVersion string

+	// TLSCiphers is used to specify the cipher algorithms that are negotiated
+	// during the TLS handshake, refer to https://pkg.go.dev/crypto/tls#CipherSuites
+	TLSCiphers []string


Why not just make it TLSOpts []func(*tls.Config) to avoid needing a new setting everytime someone finds something new they would like to configure about TLS?

Thanks, that makes sense!
I have kept the previously-existing TLS fields to not break existing usages, let me know if that's OK

jdumars · 2022-05-14T01:01:29Z

/ok-to-test

alvaroaleman · 2022-05-16T02:55:02Z

pkg/webhook/server.go

@@ -41,6 +41,9 @@ import (
 // DefaultPort is the default port that the webhook server serves.
 var DefaultPort = 9443

+// ExportedTLSConfig is used in unit tests to ensure propagation of tls related configurables to server.
+var ExportedTLSConfig *tls.Config


Lets find a way to test this without globals

Done, hopefully better

Some operators might want to respect cluster-wide TLS ciphers for example, which means that these will eventually have to be passed down to the webhook server. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu · 2022-05-16T09:24:03Z

Can't find an open issue about this, should I open one?
/test pull-controller-runtime-test-master

@akalenyu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-controller-runtime-test-master 187187e link true /test pull-controller-runtime-test-master
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

akalenyu · 2022-05-17T08:11:26Z

/test pull-controller-runtime-test-master

alvaroaleman

thank you!

k8s-ci-robot · 2022-05-17T13:29:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu, alvaroaleman

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~~ [alvaroaleman]

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

akalenyu · 2022-05-25T13:17:29Z

/cherrypick release-0.12

k8s-infra-cherrypick-robot · 2022-05-25T13:17:36Z

@akalenyu: only kubernetes-sigs org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherrypick release-0.12

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.

akalenyu · 2022-05-25T13:20:01Z

@alvaroaleman @camilamacedo86
Would you consider a backport of this to 0.12 so we can have it in a release?
We wanted to capitalize on this (not huge) feature but really shouldn't use unreleased/unpinned versions in our projects.

alvaroaleman · 2022-05-25T13:31:42Z

sure
/cherrypick release-0.12

k8s-infra-cherrypick-robot · 2022-05-25T13:32:19Z

@alvaroaleman: new pull request created: #1914

In response to this:

sure
/cherrypick release-0.12

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.

mythi · 2022-06-07T18:04:29Z

Should this be possible via manager's Options too?

akalenyu · 2022-06-07T18:37:30Z

Should this be possible via manager's Options too?

Should be able to pass it in

controller-runtime/pkg/manager/manager.go

Line 247 in e4e13b9

WebhookServer *webhook.Server

mythi · 2022-06-07T19:38:37Z

@akalenyu true but I was wondering if it was useful to control the default server created by Manager just like you can do with Host/Port/CertDir.

akalenyu · 2022-06-09T08:35:31Z

@akalenyu true but I was wondering if it was useful to control the default server created by Manager just like you can do with Host/Port/CertDir.

I might be missing something, but, the snippet above is from the Manager.Options{}, so you indeed should be able to do that?

mythi · 2022-06-09T11:29:31Z

I might be missing something, but, the snippet above is from the Manager.Options{}, so you indeed should be able to do that?

Right, I'm all good passing *webhook.Server.

But when that is not passed, some elements of webhook.Server can still be set using Manager.Options{} with Host/Port/CertDir but that's just a subset. To set the new TLS* parameters (that have been added in the past 12 months) users need to pass in *webhook.Server but they cannot be set using Manager.Options{} directly.

Upgrading controller_runtime to have it support full configuring TLS on webhooks (kubernetes-sigs/controller-runtime#1897)

Upgrading controller_runtime to have it support full configuring TLS on webhooks (kubernetes-sigs/controller-runtime#1897) Signed-off-by: borod108 <boris.od@gmail.com>

srm09 · 2022-10-24T15:30:51Z

Are there any plans to backport this fix to the 1.11.x release line?

This field has been added in kubernetes-sigs#1548 It then turned out that people want to configure more parts of the TLSConfig and the generic TLSOpts was added in kubernetes-sigs#1897 Deprecate TLSMinVersion in favor of the more generic TLSOpts.

vincepri · 2023-05-26T14:16:59Z

/cherrypick release-0.11

k8s-infra-cherrypick-robot · 2023-05-26T14:17:35Z

@vincepri: new pull request created: #2356

In response to this:

/cherrypick release-0.11

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-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 11, 2022

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 May 11, 2022

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 11, 2022

k8s-ci-robot requested review from alenkacz and gerred May 11, 2022 14:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 11, 2022

akalenyu force-pushed the tls-cipher-suites-configuration branch 2 times, most recently from 5dd273b to dcba7ee Compare May 12, 2022 16:34

alvaroaleman reviewed May 13, 2022

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 May 14, 2022

akalenyu force-pushed the tls-cipher-suites-configuration branch from dcba7ee to 5cf5bf9 Compare May 15, 2022 14:34

akalenyu changed the title ~~✨ Allow TLS ciphers to be configured on webhook server~~ ✨ Allow TLS to be entirely configured on webhook server May 15, 2022

alvaroaleman reviewed May 16, 2022

View reviewed changes

Allow TLS config to be entirely configured on webhook server

187187e

Some operators might want to respect cluster-wide TLS ciphers for example, which means that these will eventually have to be passed down to the webhook server. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>

akalenyu force-pushed the tls-cipher-suites-configuration branch from 5cf5bf9 to 187187e Compare May 16, 2022 09:06

alvaroaleman approved these changes May 17, 2022

View reviewed changes

k8s-ci-robot assigned alvaroaleman May 17, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2022

k8s-ci-robot merged commit fde61ba into kubernetes-sigs:master May 17, 2022

k8s-ci-robot added this to the v0.10.x milestone May 17, 2022

k8s-infra-cherrypick-robot mentioned this pull request May 25, 2022

✨ Allow TLS to be entirely configured on webhook server #1914

Merged

mythi mentioned this pull request Jun 21, 2022

redundant manager.Options fields #1936

Closed

borod108 added a commit to borod108/ssp-operator that referenced this pull request Jul 6, 2022

Upgrade controller_runtime

68b789d

Upgrading controller_runtime to have it support full configuring TLS on webhooks (kubernetes-sigs/controller-runtime#1897)

borod108 added a commit to borod108/ssp-operator that referenced this pull request Jul 6, 2022

Upgrade controller_runtime

a54f2c1

Upgrading controller_runtime to have it support full configuring TLS on webhooks (kubernetes-sigs/controller-runtime#1897)

borod108 added a commit to borod108/ssp-operator that referenced this pull request Jul 6, 2022

Upgrade controller_runtime

ae506d7

Upgrading controller_runtime to have it support full configuring TLS on webhooks (kubernetes-sigs/controller-runtime#1897)

borod108 mentioned this pull request Jul 6, 2022

Upgrade controller_runtime kubevirt/ssp-operator#397

Merged

lubronzhan mentioned this pull request Oct 12, 2022

Allow configuration of TLS Version for Webhook servers kubernetes-sigs/cluster-api#6511

Closed

mythi mentioned this pull request Nov 8, 2022

Expose TLS configuration options for default webhook server creation #2020

Closed

alvaroaleman mentioned this pull request Nov 9, 2022

🌱 Deprecate Webhook Server TLSMinVersion #2041

Merged

itomsawyer mentioned this pull request Apr 13, 2023

Suspect code in certwatcher.go #2182

Closed

k8s-infra-cherrypick-robot mentioned this pull request May 26, 2023

✨ Allow TLS to be entirely configured on webhook server #2356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow TLS to be entirely configured on webhook server #1897

✨ Allow TLS to be entirely configured on webhook server #1897

akalenyu commented May 11, 2022 •

edited

linux-foundation-easycla bot commented May 11, 2022 •

edited

k8s-ci-robot commented May 11, 2022

k8s-ci-robot commented May 11, 2022

alvaroaleman May 13, 2022

akalenyu May 15, 2022 •

edited

jdumars commented May 14, 2022

alvaroaleman May 16, 2022

akalenyu May 16, 2022

akalenyu commented May 16, 2022

akalenyu commented May 17, 2022

alvaroaleman left a comment

k8s-ci-robot commented May 17, 2022

akalenyu commented May 25, 2022

k8s-infra-cherrypick-robot commented May 25, 2022

akalenyu commented May 25, 2022 •

edited

alvaroaleman commented May 25, 2022

k8s-infra-cherrypick-robot commented May 25, 2022

mythi commented Jun 7, 2022

akalenyu commented Jun 7, 2022

mythi commented Jun 7, 2022

akalenyu commented Jun 9, 2022

mythi commented Jun 9, 2022

srm09 commented Oct 24, 2022

vincepri commented May 26, 2023

k8s-infra-cherrypick-robot commented May 26, 2023

✨ Allow TLS to be entirely configured on webhook server #1897

✨ Allow TLS to be entirely configured on webhook server #1897

Conversation

akalenyu commented May 11, 2022 • edited

linux-foundation-easycla bot commented May 11, 2022 • edited

k8s-ci-robot commented May 11, 2022

k8s-ci-robot commented May 11, 2022

alvaroaleman May 13, 2022

Choose a reason for hiding this comment

akalenyu May 15, 2022 • edited

Choose a reason for hiding this comment

jdumars commented May 14, 2022

alvaroaleman May 16, 2022

Choose a reason for hiding this comment

akalenyu May 16, 2022

Choose a reason for hiding this comment

akalenyu commented May 16, 2022

akalenyu commented May 17, 2022

alvaroaleman left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 17, 2022

akalenyu commented May 25, 2022

k8s-infra-cherrypick-robot commented May 25, 2022

akalenyu commented May 25, 2022 • edited

alvaroaleman commented May 25, 2022

k8s-infra-cherrypick-robot commented May 25, 2022

mythi commented Jun 7, 2022

akalenyu commented Jun 7, 2022

mythi commented Jun 7, 2022

akalenyu commented Jun 9, 2022

mythi commented Jun 9, 2022

srm09 commented Oct 24, 2022

vincepri commented May 26, 2023

k8s-infra-cherrypick-robot commented May 26, 2023

akalenyu commented May 11, 2022 •

edited

linux-foundation-easycla bot commented May 11, 2022 •

edited

akalenyu May 15, 2022 •

edited

akalenyu commented May 25, 2022 •

edited