⚠️ Adds TLS options to managers #7483

srm09 · 2022-11-02T19:15:13Z

What this PR does / why we need it:
This patch introduces two new flags to configure TLS options on the managers:

--tls-min-version which is used to set the minimum TLS version in use by the webhook server.
--tls-cipher-suites which is used to set the TLS cipher suites in use by the webhook server.

The minimum TLS version defaults to 1.2

Which issue(s) this PR fixes:
Fixes #6511

srm09 · 2022-11-02T19:16:07Z

12:14:48 ➜ ./capi -h
Usage of ./capi:
...
      --sync-period duration                   The minimum interval at which watched resources are reconciled (e.g. 15m) (default 10m0s)
      --tls-cipher-suites string               Comma-separated list of cipher suites for the server. If omitted, the default Go cipher suites will be used.
                                               Possible values are TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_ECDSA_WITH_RC4_128_SHA, TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, TLS_ECDHE_RSA_WITH_RC4_128_SHA, TLS_RSA_WITH_3DES_EDE_CBC_SHA, TLS_RSA_WITH_AES_128_CBC_SHA, TLS_RSA_WITH_AES_128_CBC_SHA256, TLS_RSA_WITH_AES_128_GCM_SHA256, TLS_RSA_WITH_AES_256_CBC_SHA, TLS_RSA_WITH_AES_256_GCM_SHA384, TLS_RSA_WITH_RC4_128_SHA.
      --tls-min-version string                 The minimum TLS version in use by the webhook server.
                                               Possible values are VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13. (default "VersionTLS12")
  -v, --v Level                                number for the log level verbosity
      --vmodule pattern=N,...                  comma-separated list of pattern=N settings for file-filtered logging (only works for text log format)
...
pflag: help requested

This is the help text for the new flags introduced.

srm09 · 2022-11-02T19:18:12Z

/hold

main.go

sbueringer · 2022-11-03T13:29:16Z

main.go

@@ -19,12 +19,14 @@ package main

 import (


Just to make it visible here: Let's also document this in the v1.2-v1.3 migration guide and link the current PR as a reference PR

Does this PR require any action from providers?

If so we should follow the new process since beta is already out:

All changes that impact providers' adoption of the new release should be announced in the provider updates section of the office hours meeting notes and approved in the PR or issue by both approvers and key affected providers

https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/developer/release-cycle.md#release-cycle

In my opinion it doesn't require any action. We would add a recommendation to the migration guide as we usually do. But the migration guide itself is not binding and part of the contract. Some providers follow it, some don't (at least that's my impression from previous CAPI bumps).

But you have a point, it's a change that providers have to actively implement if they follow our migration guide which we see as a best practice.

That being said, I tend to agree that we should bring this up next Wednesday in the office hours.

If we don't get it approved, I would like to merge this change to core CAPI itself regardless (but without changing the migration guide) and add the documentation for providers in the upcoming v1.3 => v1.4 migration guide. I think the change to CAPI itself doesn't affect providers, only the recommendation for providers to do the same.

P.S. @fabriziopandini I think we have to put the instructions for SSA that we added to the previous migration guide to one of the contract docs as well: https://cluster-api.sigs.k8s.io/developer/providers/v1.1-to-v1.2.html#required-api-changes-for-providers. Otherwise it's hard for new providers to implement it correctly for new providers. And given we see this part of the contract for providers supporting ClusterClass we have to specify it also outside of the migration guide.

From the wording of the guideline pointed out by Cecile above, this is not a change that needs to be adopted by the providers to adopt this CAPI release. This is more of a good to make change(klog logging change) rather than a must make change(like SSA).
My vote would be include these in the migration guide with an explicit callout saying that this is not required to be implemented/used by providers, but it is a good to be included for the sake of consistency amongst CAPI and providers.

We can chat more about this during the office hours and get more inputs.

Documenting it like that sounds like a good trade-off to me

@CecileRobertMichon Move the provider-relevant part to #7511

fabriziopandini

Q: what about creating two util func, one for adding flags and one for setting them?
We can eventually also add those func to utils and make them usable from providers

sbueringer · 2022-11-03T15:50:25Z

Q: what about creating two util func, one for adding flags and one for setting them? We can eventually also add those func to utils and make them usable from providers

Fine for me. If we go through the effort of implementing a util package including stuff like naming etc. I think we should consider exporting it directly, so providers don't even have to start copy&pasting our code

srm09 · 2022-11-04T17:39:41Z

On the topic of creating util functions, I was not super clear what @fabriziopandini exactly meant, so I tried to create a util function for generating the TLSOptions func slice from the provided inputs. Currently, there are 2 ways of generating the TLSOptions func in the PR,

a local method contained in the main.go
a util method contained in a new util/flags package.

The idea is to have a single option.

sbueringer · 2022-11-04T18:01:02Z

I think after my comment the idea was the separate package + another func in this package to define the flags so we don't have to copy&paste the flag definitions

util/flags/tls.go

srm09 · 2022-11-04T19:45:31Z

Added the changes to the migration docs as a separate commit, should I move it out to a separate PR and we can figure out whether to include it in 1.3.0 release or not after the next office hours on Wednesday.
That way this PR can be unblocked and made ready to merge.

util/flags/tls.go

sbueringer

Just two nits.

Yes agree. Let's move the doc change to a separate PR, everything else doesn't affect providers at all.

This way we can get it merged today/tomorrow and include it in beta.1 (which also makes it easier to pick up by providers earlier if they want to independent of the note in the migration doc)

util/flags/tls.go

fabriziopandini

overall lgtm, last round of nits

bootstrap/kubeadm/main.go

docs/book/src/developer/providers/v1.2-to-v1.3.md

util/flags/tls.go

This patch introduces two new flags to configure TLS options on the managers: 1. --tls-min-version which is used to set the minimum TLS version in use by the webhook server. 2. --tls-cipher-suites which is used to set the TLS cipher suites in use by the webhook server. The minimum TLS version defaults to 1.2 Signed-off-by: Sagar Muchhal <muchhals@vmware.com>

srm09 · 2022-11-07T18:09:22Z

/unhold
@fabriziopandini @sbueringer this should be ready to go now.

sbueringer · 2022-11-07T18:09:57Z

Thx!!

/lgtm

fabriziopandini

/lgtm
/approve

k8s-ci-robot · 2022-11-07T18:14:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

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

sbueringer · 2022-11-07T18:35:02Z

/retest

flake

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 2, 2022

k8s-ci-robot requested review from JoelSpeed and sbueringer November 2, 2022 19:15

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2022

sbueringer reviewed Nov 3, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

sbueringer reviewed Nov 3, 2022

View reviewed changes

fabriziopandini reviewed Nov 3, 2022

View reviewed changes

srm09 force-pushed the manager/add-tls-configs branch from d152b92 to dcb1cb1 Compare November 4, 2022 17:37

srm09 force-pushed the manager/add-tls-configs branch 2 times, most recently from 0d7f4a8 to 17d992c Compare November 4, 2022 18:53

fabriziopandini reviewed Nov 4, 2022

View reviewed changes

util/flags/tls.go Outdated Show resolved Hide resolved

util/flags/tls.go Outdated Show resolved Hide resolved

srm09 force-pushed the manager/add-tls-configs branch 2 times, most recently from 1bf11fd to f1c99e1 Compare November 4, 2022 19:42

sbueringer reviewed Nov 7, 2022

View reviewed changes

util/flags/tls.go Outdated Show resolved Hide resolved

sbueringer reviewed Nov 7, 2022

View reviewed changes

util/flags/tls.go Outdated Show resolved Hide resolved

util/flags/tls.go Outdated Show resolved Hide resolved

fabriziopandini reviewed Nov 7, 2022

View reviewed changes

bootstrap/kubeadm/main.go Show resolved Hide resolved

docs/book/src/developer/providers/v1.2-to-v1.3.md Outdated Show resolved Hide resolved

util/flags/tls.go Show resolved Hide resolved

srm09 force-pushed the manager/add-tls-configs branch from f1c99e1 to b0d9021 Compare November 7, 2022 17:58

srm09 mentioned this pull request Nov 7, 2022

📖 Adds webhook TLS config info to the migration doc #7511

Merged

srm09 force-pushed the manager/add-tls-configs branch from b0d9021 to 3ad56fa Compare November 7, 2022 18:08

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2022

k8s-ci-robot assigned sbueringer Nov 7, 2022

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

fabriziopandini reviewed Nov 7, 2022

View reviewed changes

k8s-ci-robot assigned fabriziopandini Nov 7, 2022

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

k8s-ci-robot merged commit 97a685a into kubernetes-sigs:main Nov 7, 2022

k8s-ci-robot added this to the v1.3 milestone Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Adds TLS options to managers #7483

⚠️ Adds TLS options to managers #7483

srm09 commented Nov 2, 2022 •

edited

srm09 commented Nov 2, 2022 •

edited

srm09 commented Nov 2, 2022

sbueringer Nov 3, 2022

CecileRobertMichon Nov 3, 2022

CecileRobertMichon Nov 3, 2022

sbueringer Nov 4, 2022 •

edited

srm09 Nov 4, 2022 •

edited

sbueringer Nov 4, 2022 •

edited

sbueringer Nov 7, 2022

fabriziopandini left a comment

sbueringer commented Nov 3, 2022

srm09 commented Nov 4, 2022

sbueringer commented Nov 4, 2022

srm09 commented Nov 4, 2022

sbueringer left a comment •

edited

fabriziopandini left a comment

srm09 commented Nov 7, 2022

sbueringer commented Nov 7, 2022

fabriziopandini left a comment

k8s-ci-robot commented Nov 7, 2022

sbueringer commented Nov 7, 2022

⚠️ Adds TLS options to managers #7483

⚠️ Adds TLS options to managers #7483

Conversation

srm09 commented Nov 2, 2022 • edited

srm09 commented Nov 2, 2022 • edited

srm09 commented Nov 2, 2022

sbueringer Nov 3, 2022

Choose a reason for hiding this comment

CecileRobertMichon Nov 3, 2022

Choose a reason for hiding this comment

CecileRobertMichon Nov 3, 2022

Choose a reason for hiding this comment

sbueringer Nov 4, 2022 • edited

Choose a reason for hiding this comment

srm09 Nov 4, 2022 • edited

Choose a reason for hiding this comment

sbueringer Nov 4, 2022 • edited

Choose a reason for hiding this comment

sbueringer Nov 7, 2022

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

sbueringer commented Nov 3, 2022

srm09 commented Nov 4, 2022

sbueringer commented Nov 4, 2022

srm09 commented Nov 4, 2022

sbueringer left a comment • edited

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

srm09 commented Nov 7, 2022

sbueringer commented Nov 7, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 7, 2022

sbueringer commented Nov 7, 2022

srm09 commented Nov 2, 2022 •

edited

srm09 commented Nov 2, 2022 •

edited

sbueringer Nov 4, 2022 •

edited

srm09 Nov 4, 2022 •

edited

sbueringer Nov 4, 2022 •

edited

sbueringer left a comment •

edited