review comments

docs/proposals/20210112-encryption-roviders.md

@@ -40,7 +40,7 @@ kind: KubeOneCluster
 features:
   encryptionProviders:
     enabled: true
-    customProvidersFile: |
+    customEncryptionConfiguration: |
       apiVersion: apiserver.config.k8s.io/v1
       kind: EncryptionConfiguration
       resources:

pkg/apis/kubeone/types.go

@@ -364,6 +364,7 @@ type Features struct {
 	// OpenIDConnect
 	OpenIDConnect *OpenIDConnect `json:"openidConnect,omitempty"`
 	// Encryption Providers
+	// +k8s:conversion-gen=false
 	EncryptionProviders *EncryptionProviders `json:"encryptionProviders,omitempty"`
 }
 
@@ -575,6 +576,6 @@ type Addons struct {
 type EncryptionProviders struct {
 	// Enable
 	Enable bool `json:"enable"`
-	// CustomProvidersFile
-	CustomProvidersFile string `json:"customProvidersFile"`
+	// CustomEncryptionConfiguration
+	CustomEncryptionConfiguration string `json:"customEncryptionConfiguration"`
 }

pkg/apis/kubeone/v1alpha1/types.go

@@ -217,13 +217,12 @@ type MachineControllerConfig struct {
 type Features struct {
 	PodNodeSelector *PodNodeSelector `json:"podNodeSelector"`
 	// Deprecated: will be removed once Kubernetes 1.19 reaches EOL
-	PodPresets          *PodPresets          `json:"podPresets"`
-	PodSecurityPolicy   *PodSecurityPolicy   `json:"podSecurityPolicy"`
-	StaticAuditLog      *StaticAuditLog      `json:"staticAuditLog"`
-	DynamicAuditLog     *DynamicAuditLog     `json:"dynamicAuditLog"`
-	MetricsServer       *MetricsServer       `json:"metricsServer"`
-	OpenIDConnect       *OpenIDConnect       `json:"openidConnect"`
-	EncryptionProviders *EncryptionProviders `json:"encryptionProviders,omitempty"`
+	PodPresets        *PodPresets        `json:"podPresets"`
+	PodSecurityPolicy *PodSecurityPolicy `json:"podSecurityPolicy"`
+	StaticAuditLog    *StaticAuditLog    `json:"staticAuditLog"`
+	DynamicAuditLog   *DynamicAuditLog   `json:"dynamicAuditLog"`
+	MetricsServer     *MetricsServer     `json:"metricsServer"`
+	OpenIDConnect     *OpenIDConnect     `json:"openidConnect"`
 }
 
 // SystemPackages controls configurations of APT/YUM
@@ -323,11 +322,3 @@ type Addons struct {
 	// Path on the local file system to the directory with addons manifests.
 	Path string `json:"path"`
 }
-
-// Encryption Providers feature flag
-type EncryptionProviders struct {
-	// Enable
-	Enable bool `json:"enable"`
-	// CustomProvidersFile
-	CustomProvidersFile string `json:"customProvidersFile"`
-}

pkg/apis/kubeone/v1beta1/types.go

@@ -575,6 +575,6 @@ type Addons struct {
 type EncryptionProviders struct {
 	// Enable
 	Enable bool `json:"enable"`
-	// CustomProvidersFile
-	CustomProvidersFile string `json:"customProvidersFile"`
+	// CustomEncryptionConfiguration
+	CustomEncryptionConfiguration string `json:"customEncryptionConfiguration"`
 }

pkg/cmd/apply.go

@@ -252,7 +252,12 @@ func runApply(opts *applyOpts) error {
 	}
 
 	if opts.RotateEncryptionKey {
-		if s.Cluster.Features.EncryptionProviders.CustomProvidersFile != "" {
+		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)
@@ -332,7 +337,7 @@ func runApplyUpgradeIfNeeded(s *state.State, opts *applyOpts) error {
 		// disable case, we do this as early as possible.
 		if s.ShouldDisableEncryption() {
 			// something should happen here
-			tasksToRun = tasks.WithDisableEncryptionProviders(nil, s.Cluster.Features.EncryptionProviders.CustomProvidersFile != "")
+			tasksToRun = tasks.WithDisableEncryptionProviders(nil, s.Cluster.Features.EncryptionProviders.CustomEncryptionConfiguration != "")
 		}
 
 		tasksToRun = tasks.WithUpgrade(tasksToRun)
@@ -400,11 +405,11 @@ func runApplyUpgradeIfNeeded(s *state.State, opts *applyOpts) error {
 func runApplyRotateKey(s *state.State, opts *applyOpts) error {
 	if !opts.ForceUpgrade {
 		s.Logger.Error("rotating encryption keys requires the --force-upgrade flag")
-		return nil
+		return errors.New("rotating encryption keys requires the --force-upgrade flag")
 	}
 	if !s.EncryptionEnabled() {
 		s.Logger.Error("rotating encryption keys failed: Encryption Providers support is not enabled")
-		return nil
+		return errors.New("rotating encryption keys failed: Encryption Providers support is not enabled")
 	}
 
 	fmt.Println("The following actions will be taken: ")

pkg/cmd/config.go

@@ -386,6 +386,9 @@ func createAndPrintManifest(printOptions *printOpts) error {
 		cfg.Set(yamled.Path{"features", "openidConnect", "config", "requiredClaim"}, "")
 		cfg.Set(yamled.Path{"features", "openidConnect", "config", "caFile"}, "")
 	}
+	if printOptions.EnableEncryptionProviders {
+		cfg.Set(yamled.Path{"features", "encryptionProviders", "enable"}, printOptions.EnableEncryptionProviders)
+	}
 
 	// machine-controller
 	if !printOptions.DeployMachineController {
@@ -685,8 +688,8 @@ features:
   # For more information: https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/
   encryptionProviders:
     # disabled by default
-	enable: {{ .EnableEncryptionProviders }}
-	customProvidersConfig: # inline string
+    enable: {{ .EnableEncryptionProviders }}
+    customProvidersConfig: # inline string
 systemPackages:
   # will add Docker and Kubernetes repositories to OS package manager
   configureRepositories: true # it's true by default

pkg/state/cluster.go

@@ -21,6 +21,7 @@ import (
 	"sync"
 
 	"github.com/Masterminds/semver/v3"
+
 	apiserverconfigv1 "k8s.io/apiserver/pkg/apis/config/v1"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
@@ -41,6 +42,7 @@ type EncryptionConfiguration struct {
 	Config *apiserverconfigv1.EncryptionConfiguration
 	Custom bool
 }
+
 type Host struct {
 	Config *kubeone.HostConfig
 

pkg/state/context.go

@@ -107,7 +107,7 @@ func (s *State) EncryptionEnabled() bool {
 }
 
 func (s *State) GetEncryptionProviderConfigName() string {
-	if (s.EncryptionEnabled() && s.Cluster.Features.EncryptionProviders.CustomProvidersFile != "") ||
+	if (s.EncryptionEnabled() && s.Cluster.Features.EncryptionProviders.CustomEncryptionConfiguration != "") ||
 		s.LiveCluster.EncryptionConfiguration.Custom {
 		return customEncryptionProvidersFile
 	}

pkg/tasks/encryption_providers.go

@@ -18,22 +18,22 @@ package tasks
 
 import (
 	"context"
-	"errors"
 	"fmt"
-	"time"
+
+	"github.com/pkg/errors"
 
 	kubeoneapi "k8c.io/kubeone/pkg/apis/kubeone"
 	"k8c.io/kubeone/pkg/scripts"
 	"k8c.io/kubeone/pkg/ssh"
 	"k8c.io/kubeone/pkg/state"
 	"k8c.io/kubeone/pkg/templates"
+	encryptionproviders "k8c.io/kubeone/pkg/templates/encryptionproviders"
 
-	encryptionproviders "k8c.io/kubeone/pkg/templates/encryption-providers"
 	corev1 "k8s.io/api/core/v1"
-	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	apiserverconfigv1 "k8s.io/apiserver/pkg/apis/config/v1"
+	"k8s.io/client-go/util/retry"
 	dynclient "sigs.k8s.io/controller-runtime/pkg/client"
 	kyaml "sigs.k8s.io/yaml"
 )
@@ -55,9 +55,10 @@ func FetchEncryptionProvidersFile(s *state.State) error {
 	if err != nil {
 		return err
 	}
-
+	s.LiveCluster.Lock.Lock()
 	s.LiveCluster.EncryptionConfiguration.Config = &apiserverconfigv1.EncryptionConfiguration{}
 	err = kyaml.UnmarshalStrict([]byte(config), s.LiveCluster.EncryptionConfiguration.Config)
+	s.LiveCluster.Lock.Unlock()
 	return err
 }
 
@@ -71,7 +72,9 @@ func UploadIdentityFirstEncryptionConficguration(s *state.State) error {
 
 	oldConfig := s.LiveCluster.EncryptionConfiguration.Config.DeepCopy()
 
-	encryptionproviders.UpdateEncryptionConfigDecryptOnly(oldConfig)
+	if err := encryptionproviders.UpdateEncryptionConfigDecryptOnly(oldConfig); err != nil {
+		return err
+	}
 
 	config, err := templates.KubernetesToYAML([]runtime.Object{oldConfig})
 	if err != nil {
@@ -89,7 +92,6 @@ func UploadEncryptionConfigurationWithNewKey(s *state.State) error {
 		return errors.New("failed to read live cluster encryption providers configuration")
 	}
 
-	// oldConfig := s.LiveCluster.EncryptionConfiguration.Config.DeepCopy()
 	if err := encryptionproviders.UpdateEncryptionConfigWithNewKey(s.LiveCluster.EncryptionConfiguration.Config); err != nil {
 		return err
 	}
@@ -111,8 +113,6 @@ func UploadEncryptionConfigurationWithoutOldKey(s *state.State) error {
 		return errors.New("failed to read live cluster encryption providers configuration")
 	}
 
-	// oldConfig := s.LiveCluster.EncryptionConfiguration.Config.DeepCopy()
-
 	encryptionproviders.UpdateEncryptionConfigRemoveOldKey(s.LiveCluster.EncryptionConfiguration.Config)
 
 	config, err := templates.KubernetesToYAML([]runtime.Object{s.LiveCluster.EncryptionConfiguration.Config})
@@ -144,40 +144,24 @@ func RewriteClusterSecrets(s *state.State) error {
 	if err != nil {
 		return err
 	}
-	for i := range secrets.Items {
-		secret := secrets.Items[i]
-		if err = s.DynamicClient.Update(context.Background(), &secret, &dynclient.UpdateOptions{}); err != nil {
-			if kerrors.IsConflict(err) {
-				err = s.DynamicClient.Get(context.Background(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, &secret)
-				if err != nil {
-					return err
-				}
-				if err = s.DynamicClient.Update(context.Background(), &secret, &dynclient.UpdateOptions{}); err != nil {
-					return err
-				}
+	for _, secret := range secrets.Items {
+		updateErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+			toRewrite := corev1.Secret{}
+			name := secret.Name
+			namespace := secret.Namespace
+			if err := s.DynamicClient.Get(s.Context, types.NamespacedName{Name: name, Namespace: namespace}, &toRewrite); err != nil {
+				return err
 			}
-			return err
+
+			return s.DynamicClient.Update(s.Context, &toRewrite)
+		})
+		if updateErr != nil {
+			return errors.WithStack(updateErr)
 		}
 	}
 	return nil
 }
 
-// FIXME: Static pods are not managed by the API, so we can't simply delete them to restart.
-// We should use a cleaner method to do this.
-func RestartKubeAPI(s *state.State) error {
-	s.Logger.Infof("Restarting KubeAPI...")
-	return s.RunTaskOnControlPlane(func(s *state.State, _ *kubeoneapi.HostConfig, _ ssh.Connection) error {
-		_, _, err := s.Runner.RunRaw(`docker restart $(docker ps --filter="label=io.kubernetes.container.name=kube-apiserver" -q)`)
-		return err
-	}, state.RunParallel)
-}
-
-func WaitForAPI(s *state.State) error {
-	s.Logger.Infof("Waiting %v to ensure all components are up...", 2*timeoutNodeUpgrade)
-	time.Sleep(2 * timeoutNodeUpgrade)
-	return nil
-}
-
 func RemoveEncryptionProviderFile(s *state.State) error {
 	s.Logger.Infof("Removing EncryptionProviders configuration file...")
 	return s.RunTaskOnControlPlane(func(s *state.State, _ *kubeoneapi.HostConfig, _ ssh.Connection) error {