Add support for Encryption Providers #1241

moelsayed · 2021-02-05T11:18:34Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1225

Special notes for your reviewer:
While this adds support for custom configuration, they wouldn't be effective yet in enabling KMS deployment as that would require adding external volumes to the KubeAPI container. That will be implemented separately.
Does this PR introduce a user-facing change?:

Add support for Encryption Providers

xmudrii · 2021-02-16T12:16:26Z

/assign

xmudrii

I did the first run and overall it looks good to me. I've left a bunch of comments, but I hope that most of them are minor. I still haven't reviewed everything and I might have some additional comments and questions, but I'll try to take a deeper look tomorrow.

pkg/apis/kubeone/types.go

pkg/apis/kubeone/v1alpha1/types.go

pkg/cmd/apply.go

pkg/cmd/config.go

pkg/templates/encryption-providers/encryption_providers.go

pkg/cmd/apply.go

moelsayed · 2021-02-21T20:55:45Z

/retest

moelsayed · 2021-02-26T13:00:17Z

/retest

pkg/scripts/configs.go

pkg/tasks/probes.go

xmudrii · 2021-02-26T10:57:11Z

pkg/tasks/encryption_providers.go

+	return err
+}
+
+func UploadIdentityFirstEncryptionConficguration(s *state.State) error {


Nit: typo.

Suggested change

func UploadIdentityFirstEncryptionConficguration(s *state.State) error {

func UploadIdentityFirstEncryptionConfiguration(s *state.State) error {

xmudrii · 2021-02-26T12:50:39Z

pkg/cmd/apply.go

+	if opts.RotateEncryptionKey {
+		if !s.EncryptionEnabled() {
+			return errors.New("Encryption Providers support is not enabled for this cluster")
+		}
+
+		if s.Cluster.Features.EncryptionProviders != nil &&
+			s.Cluster.Features.EncryptionProviders.CustomEncryptionConfiguration != "" {
+			return errors.New("key rotation of custom providers file is not supported")
+		}
+		return runApplyRotateKey(s, opts)
+	}


There two edge cases that might not be covered by this:

What if we need to repair the cluster, but the --rotate-encryption-key flag is provided? Should we show a warning/error or do something?

What if the cluster is supposed to be upgraded, but the user also provided the --rotate-encryption-key flag? In that case, I'd expect that we do both if possible. Also, in that case, we don't need to require the --force-upgrade flag.

What if we need to repair the cluster, but the --rotate-encryption-key flag is provided? Should we show a warning/error or do something?

I have added a check to error out if the cluster is not healthy.

What if the cluster is supposed to be upgraded, but the user also provided the --rotate-encryption-key flag? In that case, I'd expect that we do both if possible. Also, in that case, we don't need to require the --force-upgrade flag.

I did this to split the two operations. Both rotation and upgrade are disruptive operations. I think it makes sense to try to minimize these sort of operations in each run. I also used the --force-upgrade here as a sort of make-sure-you-know-what-you're-doing barrier the same why it's used to update the Features section configuration.

xmudrii · 2021-02-26T12:58:06Z

pkg/cmd/apply.go

+
+	fmt.Println("The following actions will be taken: ")
+	fmt.Println("Run with --verbose flag for more information.")
+	tasksToRun := tasks.WithRotateKey(nil)


How is this going to behave if a custom EncryptionConfiguration is used? I think that we should fail in that case and ask the user to do it manually.

it's checked earlier here

xmudrii · 2021-02-26T13:00:03Z

pkg/features/encryption_providers.go

+	if feature == nil || !feature.Enable {
+		return
+	}
+	args.APIServer.ExtraArgs[apiServerEncryptionProviderFlag] = apiServerEncryptionProviderConfigPath


This is always going to set the flag to encryption-providers.yaml. How this works if we're using a custom EncryptionConfiguration? Isn't the file called custom-encryption-providers.yaml in that case?

xmudrii · 2021-02-26T13:13:18Z

pkg/tasks/encryption_providers.go

+	return err
+}
+
+func UploadIdentityFirstEncryptionConficguration(s *state.State) error {


Maybe we should safeguard this function that it can't be used if a custom EncryptionConfiguration is used.

pkg/tasks/tasks.go

moelsayed · 2021-03-04T11:38:41Z

/test pull-kubeone-test

moelsayed · 2021-03-19T05:51:16Z

/test pull-kubeone-test

pkg/scripts/configs.go

pkg/cmd/apply.go

pkg/tasks/tasks.go

kron4eg · 2021-04-07T15:06:31Z

pkg/cmd/apply.go

@@ -315,9 +339,20 @@ func runApplyUpgradeIfNeeded(s *state.State, opts *applyOpts) error {

 	operations := []string{}

-	tasksToRun := tasks.WithResources(nil)
+	var tasksToRun tasks.Tasks


But what tasks to run when upgradeNeeded || opts.ForceUpgrade is not true? We need to run tasks.WithResources in that case, but it will not happen this way.

@kron4eg nice catch, updated 👍

moelsayed · 2021-04-08T07:16:39Z

/retest

moelsayed · 2021-04-08T08:02:06Z

/test pull-kubeone-lint

moelsayed · 2021-04-08T08:16:55Z

/test pull-kubeone-e2e-aws-upgrade-1.18-1.19

moelsayed · 2021-04-08T08:26:29Z

/retest

xmudrii · 2021-04-08T10:21:30Z

/hold
there is one more thing to fix before we merge this

pkg/apis/kubeone/types.go

kron4eg

/approve

kubermatic-bot · 2021-04-18T21:14:09Z

LGTM label has been added.

Git tree hash: 1f36d0afd9f63a61476493af827360074faacbc1

kubermatic-bot · 2021-04-18T21:14:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg, moelsayed

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

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

kron4eg · 2021-04-19T08:49:26Z

/unhold

moelsayed requested a review from kron4eg February 5, 2021 11:18

moelsayed force-pushed the encryption_providers_1 branch 3 times, most recently from e35197f to 1eb5c72 Compare February 8, 2021 17:14

kubermatic-bot assigned xmudrii Feb 16, 2021

xmudrii self-requested a review February 16, 2021 12:16

xmudrii reviewed Feb 16, 2021

View reviewed changes

moelsayed force-pushed the encryption_providers_1 branch from 1eb5c72 to 3b63fdd Compare February 21, 2021 17:59

kubermatic-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 21, 2021

moelsayed force-pushed the encryption_providers_1 branch 2 times, most recently from 5785e8d to 2a84376 Compare February 21, 2021 18:06

kron4eg reviewed Feb 27, 2021

View reviewed changes

pkg/scripts/configs.go Show resolved Hide resolved

kron4eg reviewed Feb 27, 2021

View reviewed changes

pkg/tasks/probes.go Outdated Show resolved Hide resolved

xmudrii reviewed Mar 1, 2021

View reviewed changes

moelsayed force-pushed the encryption_providers_1 branch from 1f5658b to 502b6c4 Compare March 4, 2021 11:22

kubermatic-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 4, 2021

kubermatic-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021

moelsayed force-pushed the encryption_providers_1 branch from a150919 to 66048aa Compare March 19, 2021 04:40

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2021

kron4eg reviewed Mar 22, 2021

View reviewed changes

pkg/scripts/configs.go Outdated Show resolved Hide resolved

moelsayed force-pushed the encryption_providers_1 branch from 9933290 to 7ab297c Compare April 7, 2021 11:43

kubermatic-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021

kron4eg reviewed Apr 7, 2021

View reviewed changes

pkg/scripts/configs.go Outdated Show resolved Hide resolved

kron4eg reviewed Apr 7, 2021

View reviewed changes

pkg/cmd/apply.go Show resolved Hide resolved

pkg/tasks/tasks.go Outdated Show resolved Hide resolved

kron4eg reviewed Apr 7, 2021

View reviewed changes

moelsayed force-pushed the encryption_providers_1 branch from 1b65a75 to 4e9a7a1 Compare April 7, 2021 15:39

kron4eg mentioned this pull request Apr 8, 2021

Use spot instances for E2E on AWS #1310

Merged

kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 8, 2021

xmudrii reviewed Apr 8, 2021

View reviewed changes

pkg/apis/kubeone/types.go Outdated Show resolved Hide resolved

moelsayed added 3 commits April 13, 2021 14:18

add support for Encryption Providers

ed242bc

review comment

cf3029b

remove dead code

895ab94

moelsayed force-pushed the encryption_providers_1 branch from 4e9a7a1 to 8a99d99 Compare April 13, 2021 12:31

fix conversion

ac73b9c

moelsayed force-pushed the encryption_providers_1 branch from 8a99d99 to ac73b9c Compare April 14, 2021 08:21

kron4eg mentioned this pull request Apr 15, 2021

KubeOne Encryption Providers Guide kubermatic/docs#638

Merged

kron4eg approved these changes Apr 18, 2021

View reviewed changes

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2021

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 18, 2021

kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2021

kubermatic-bot merged commit fef8d68 into kubermatic:master Apr 19, 2021

kubermatic-bot added this to the KubeOne 1.3 milestone Apr 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Encryption Providers #1241

Add support for Encryption Providers #1241

moelsayed commented Feb 5, 2021

xmudrii commented Feb 16, 2021

xmudrii left a comment

moelsayed commented Feb 21, 2021

moelsayed commented Feb 26, 2021

xmudrii Feb 26, 2021

xmudrii Feb 26, 2021

moelsayed Mar 3, 2021

xmudrii Feb 26, 2021

moelsayed Mar 3, 2021

xmudrii Feb 26, 2021

xmudrii Feb 26, 2021

moelsayed commented Mar 4, 2021

moelsayed commented Mar 19, 2021

kron4eg Apr 7, 2021

moelsayed Apr 7, 2021

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

xmudrii commented Apr 8, 2021

kron4eg left a comment

kubermatic-bot commented Apr 18, 2021

kubermatic-bot commented Apr 18, 2021

kron4eg commented Apr 19, 2021

	func UploadIdentityFirstEncryptionConficguration(s *state.State) error {
	func UploadIdentityFirstEncryptionConfiguration(s *state.State) error {

Add support for Encryption Providers #1241

Add support for Encryption Providers #1241

Conversation

moelsayed commented Feb 5, 2021

xmudrii commented Feb 16, 2021

xmudrii left a comment

Choose a reason for hiding this comment

moelsayed commented Feb 21, 2021

moelsayed commented Feb 26, 2021

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

moelsayed commented Mar 4, 2021

moelsayed commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

moelsayed commented Apr 8, 2021

xmudrii commented Apr 8, 2021

kron4eg left a comment

Choose a reason for hiding this comment

kubermatic-bot commented Apr 18, 2021

kubermatic-bot commented Apr 18, 2021

kron4eg commented Apr 19, 2021