Merge pull request kubernetes#76862 from fabriziopandini/fix-upgrade-certs-renew

kubeadm: fix certs renewal during upgrade

Author: k8s-ci-robot

cmd/kubeadm/app/cmd/upgrade/apply.go

@@ -52,6 +52,7 @@ type applyFlags struct {
 	force              bool
 	dryRun             bool
 	etcdUpgrade        bool
+	renewCerts         bool
 	criSocket          string
 	imagePullTimeout   time.Duration
 }
@@ -67,6 +68,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 		applyPlanFlags:   apf,
 		imagePullTimeout: defaultImagePullTimeout,
 		etcdUpgrade:      true,
+		renewCerts:       true,
 		// Don't set criSocket to a default value here, as this will override the setting in the stored config in RunApply below.
 	}
 
@@ -90,6 +92,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
 	cmd.Flags().BoolVarP(&flags.force, "force", "f", flags.force, "Force upgrading although some requirements might not be met. This also implies non-interactive mode.")
 	cmd.Flags().BoolVar(&flags.dryRun, options.DryRun, flags.dryRun, "Do not change any state, just output what actions would be performed.")
 	cmd.Flags().BoolVar(&flags.etcdUpgrade, "etcd-upgrade", flags.etcdUpgrade, "Perform the upgrade of etcd.")
+	cmd.Flags().BoolVar(&flags.renewCerts, "certificate-renewal", flags.renewCerts, "Perform the renewal of certificates used by component changed during upgrades.")
 	cmd.Flags().DurationVar(&flags.imagePullTimeout, "image-pull-timeout", flags.imagePullTimeout, "The maximum amount of time to wait for the control plane pods to be downloaded.")
 
 	// The CRI socket flag is deprecated here, since it should be taken from the NodeRegistrationOptions for the current
@@ -231,7 +234,7 @@ func PerformControlPlaneUpgrade(flags *applyFlags, client clientset.Interface, w
 	}
 
 	// Don't save etcd backup directory if etcd is HA, as this could cause corruption
-	return PerformStaticPodUpgrade(client, waiter, internalcfg, flags.etcdUpgrade)
+	return PerformStaticPodUpgrade(client, waiter, internalcfg, flags.etcdUpgrade, flags.renewCerts)
 }
 
 // GetPathManagerForUpgrade returns a path manager properly configured for the given InitConfiguration.
@@ -241,14 +244,14 @@ func GetPathManagerForUpgrade(internalcfg *kubeadmapi.InitConfiguration, etcdUpg
 }
 
 // PerformStaticPodUpgrade performs the upgrade of the control plane components for a static pod hosted cluster
-func PerformStaticPodUpgrade(client clientset.Interface, waiter apiclient.Waiter, internalcfg *kubeadmapi.InitConfiguration, etcdUpgrade bool) error {
+func PerformStaticPodUpgrade(client clientset.Interface, waiter apiclient.Waiter, internalcfg *kubeadmapi.InitConfiguration, etcdUpgrade, renewCerts bool) error {
 	pathManager, err := GetPathManagerForUpgrade(internalcfg, etcdUpgrade)
 	if err != nil {
 		return err
 	}
 
 	// The arguments oldEtcdClient and newEtdClient, are uninitialized because passing in the clients allow for mocking the client during testing
-	return upgrade.StaticPodControlPlane(client, waiter, pathManager, internalcfg, etcdUpgrade, nil, nil)
+	return upgrade.StaticPodControlPlane(client, waiter, pathManager, internalcfg, etcdUpgrade, renewCerts, nil, nil)
 }
 
 // DryRunStaticPodUpgrade fakes an upgrade of the control plane

cmd/kubeadm/app/cmd/upgrade/node.go

@@ -66,6 +66,7 @@ type controlplaneUpgradeFlags struct {
 	advertiseAddress string
 	nodeName         string
 	etcdUpgrade      bool
+	renewCerts       bool
 	dryRun           bool
 }
 
@@ -114,6 +115,7 @@ func NewCmdUpgradeControlPlane() *cobra.Command {
 		kubeConfigPath:   constants.GetKubeletKubeConfigPath(),
 		advertiseAddress: "",
 		etcdUpgrade:      true,
+		renewCerts:       true,
 		dryRun:           false,
 	}
 
@@ -151,6 +153,7 @@ func NewCmdUpgradeControlPlane() *cobra.Command {
 	options.AddKubeConfigFlag(cmd.Flags(), &flags.kubeConfigPath)
 	cmd.Flags().BoolVar(&flags.dryRun, options.DryRun, flags.dryRun, "Do not change any state, just output the actions that would be performed.")
 	cmd.Flags().BoolVar(&flags.etcdUpgrade, "etcd-upgrade", flags.etcdUpgrade, "Perform the upgrade of etcd.")
+	cmd.Flags().BoolVar(&flags.renewCerts, "certificate-renewal", flags.renewCerts, "Perform the renewal of certificates used by component changed during upgrades.")
 	return cmd
 }
 
@@ -221,18 +224,13 @@ func RunUpgradeControlPlane(flags *controlplaneUpgradeFlags) error {
 		return errors.Wrap(err, "unable to fetch the kubeadm-config ConfigMap")
 	}
 
-	// Rotate API server certificate if needed
-	if err := upgrade.BackupAPIServerCertIfNeeded(cfg, flags.dryRun); err != nil {
-		return errors.Wrap(err, "unable to rotate API server certificate")
-	}
-
 	// Upgrade the control plane and etcd if installed on this node
 	fmt.Printf("[upgrade] Upgrading your Static Pod-hosted control plane instance to version %q...\n", cfg.KubernetesVersion)
 	if flags.dryRun {
 		return DryRunStaticPodUpgrade(cfg)
 	}
 
-	if err := PerformStaticPodUpgrade(client, waiter, cfg, flags.etcdUpgrade); err != nil {
+	if err := PerformStaticPodUpgrade(client, waiter, cfg, flags.etcdUpgrade, flags.renewCerts); err != nil {
 		return errors.Wrap(err, "couldn't complete the static pod upgrade")
 	}
 

cmd/kubeadm/app/phases/upgrade/BUILD

@@ -15,7 +15,6 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//cmd/kubeadm/app/apis/kubeadm:go_default_library",
-        "//cmd/kubeadm/app/apis/kubeadm/v1beta2:go_default_library",
         "//cmd/kubeadm/app/constants:go_default_library",
         "//cmd/kubeadm/app/images:go_default_library",
         "//cmd/kubeadm/app/phases/addons/dns:go_default_library",
@@ -45,7 +44,6 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/version:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes:go_default_library",
-        "//staging/src/k8s.io/client-go/util/cert:go_default_library",
         "//vendor/github.com/pkg/errors:go_default_library",
     ],
 )

cmd/kubeadm/app/phases/upgrade/postupgrade.go

@@ -17,7 +17,6 @@ limitations under the License.
 package upgrade
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"time"
@@ -29,15 +28,12 @@ import (
 	errorsutil "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/version"
 	clientset "k8s.io/client-go/kubernetes"
-	certutil "k8s.io/client-go/util/cert"
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
-	kubeadmapiv1beta2 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta2"
 	kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/addons/dns"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/addons/proxy"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/bootstraptoken/clusterinfo"
 	nodebootstraptoken "k8s.io/kubernetes/cmd/kubeadm/app/phases/bootstraptoken/node"
-	certsphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/certs"
 	kubeletphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/kubelet"
 	patchnodephase "k8s.io/kubernetes/cmd/kubeadm/app/phases/patchnode"
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/uploadconfig"
@@ -101,11 +97,6 @@ func PerformPostUpgradeTasks(client clientset.Interface, cfg *kubeadmapi.InitCon
 		errs = append(errs, err)
 	}
 
-	// Rotate the kube-apiserver cert and key if needed
-	if err := BackupAPIServerCertIfNeeded(cfg, dryRun); err != nil {
-		errs = append(errs, err)
-	}
-
 	// Upgrade kube-dns/CoreDNS and kube-proxy
 	if err := dns.EnsureDNSAddon(&cfg.ClusterConfiguration, client); err != nil {
 		errs = append(errs, err)
@@ -152,37 +143,6 @@ func removeOldDNSDeploymentIfAnotherDNSIsUsed(cfg *kubeadmapi.ClusterConfigurati
 	}, 10)
 }
 
-// BackupAPIServerCertIfNeeded rotates the kube-apiserver certificate if older than 180 days
-func BackupAPIServerCertIfNeeded(cfg *kubeadmapi.InitConfiguration, dryRun bool) error {
-	certAndKeyDir := kubeadmapiv1beta2.DefaultCertificatesDir
-	shouldBackup, err := shouldBackupAPIServerCertAndKey(certAndKeyDir)
-	if err != nil {
-		// Don't fail the upgrade phase if failing to determine to backup kube-apiserver cert and key.
-		return errors.Wrap(err, "[postupgrade] WARNING: failed to determine to backup kube-apiserver cert and key")
-	}
-
-	if !shouldBackup {
-		return nil
-	}
-
-	// If dry-running, just say that this would happen to the user and exit
-	if dryRun {
-		fmt.Println("[postupgrade] Would rotate the API server certificate and key.")
-		return nil
-	}
-
-	// Don't fail the upgrade phase if failing to backup kube-apiserver cert and key, just continue rotating the cert
-	// TODO: We might want to reconsider this choice.
-	if err := backupAPIServerCertAndKey(certAndKeyDir); err != nil {
-		fmt.Printf("[postupgrade] WARNING: failed to backup kube-apiserver cert and key: %v\n", err)
-	}
-	return certsphase.CreateCertAndKeyFilesWithCA(
-		&certsphase.KubeadmCertAPIServer,
-		&certsphase.KubeadmCertRootCA,
-		cfg,
-	)
-}
-
 func writeKubeletConfigFiles(client clientset.Interface, cfg *kubeadmapi.InitConfiguration, newK8sVer *version.Version, dryRun bool) error {
 	kubeletDir, err := GetKubeletDir(dryRun)
 	if err != nil {
@@ -228,20 +188,6 @@ func GetKubeletDir(dryRun bool) (string, error) {
 	return kubeadmconstants.KubeletRunDirectory, nil
 }
 
-// backupAPIServerCertAndKey backups the old cert and key of kube-apiserver to a specified directory.
-func backupAPIServerCertAndKey(certAndKeyDir string) error {
-	subDir := filepath.Join(certAndKeyDir, "expired")
-	if err := os.Mkdir(subDir, 0700); err != nil {
-		return errors.Wrapf(err, "failed to created backup directory %s", subDir)
-	}
-
-	filesToMove := map[string]string{
-		filepath.Join(certAndKeyDir, kubeadmconstants.APIServerCertName): filepath.Join(subDir, kubeadmconstants.APIServerCertName),
-		filepath.Join(certAndKeyDir, kubeadmconstants.APIServerKeyName):  filepath.Join(subDir, kubeadmconstants.APIServerKeyName),
-	}
-	return moveFiles(filesToMove)
-}
-
 // moveFiles moves files from one directory to another.
 func moveFiles(files map[string]string) error {
 	filesToRecover := map[string]string{}
@@ -264,21 +210,3 @@ func rollbackFiles(files map[string]string, originalErr error) error {
 	}
 	return errors.Errorf("couldn't move these files: %v. Got errors: %v", files, errorsutil.NewAggregate(errs))
 }
-
-// shouldBackupAPIServerCertAndKey checks if the cert of kube-apiserver will be expired in 180 days.
-func shouldBackupAPIServerCertAndKey(certAndKeyDir string) (bool, error) {
-	apiServerCert := filepath.Join(certAndKeyDir, kubeadmconstants.APIServerCertName)
-	certs, err := certutil.CertsFromFile(apiServerCert)
-	if err != nil {
-		return false, errors.Wrapf(err, "couldn't load the certificate file %s", apiServerCert)
-	}
-	if len(certs) == 0 {
-		return false, errors.New("no certificate data found")
-	}
-
-	if time.Since(certs[0].NotBefore) > expiry {
-		return true, nil
-	}
-
-	return false, nil
-}

cmd/kubeadm/app/phases/upgrade/postupgrade_test.go

@@ -21,40 +21,13 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/pkg/errors"
 
-	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
-	certsphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/certs"
 	testutil "k8s.io/kubernetes/cmd/kubeadm/test"
 )
 
-func TestBackupAPIServerCertAndKey(t *testing.T) {
-	tmpdir := testutil.SetupTempDir(t)
-	defer os.RemoveAll(tmpdir)
-	os.Chmod(tmpdir, 0766)
-
-	certPath := filepath.Join(tmpdir, constants.APIServerCertName)
-	certFile, err := os.OpenFile(certPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666)
-	if err != nil {
-		t.Fatalf("Failed to create cert file %s: %v", certPath, err)
-	}
-	defer certFile.Close()
-
-	keyPath := filepath.Join(tmpdir, constants.APIServerKeyName)
-	keyFile, err := os.OpenFile(keyPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0666)
-	if err != nil {
-		t.Fatalf("Failed to create key file %s: %v", keyPath, err)
-	}
-	defer keyFile.Close()
-
-	if err := backupAPIServerCertAndKey(tmpdir); err != nil {
-		t.Fatalf("Failed to backup cert and key in dir %s: %v", tmpdir, err)
-	}
-}
-
 func TestMoveFiles(t *testing.T) {
 	tmpdir := testutil.SetupTempDir(t)
 	defer os.RemoveAll(tmpdir)
@@ -128,59 +101,3 @@ func TestRollbackFiles(t *testing.T) {
 		t.Fatalf("Expected error contains %q, got %v", errString, err)
 	}
 }
-
-func TestShouldBackupAPIServerCertAndKey(t *testing.T) {
-	cfg := &kubeadmapi.InitConfiguration{
-		LocalAPIEndpoint: kubeadmapi.APIEndpoint{AdvertiseAddress: "1.2.3.4"},
-		ClusterConfiguration: kubeadmapi.ClusterConfiguration{
-			Networking: kubeadmapi.Networking{ServiceSubnet: "10.96.0.0/12", DNSDomain: "cluster.local"},
-		},
-		NodeRegistration: kubeadmapi.NodeRegistrationOptions{Name: "test-node"},
-	}
-
-	for desc, test := range map[string]struct {
-		adjustedExpiry time.Duration
-		expected       bool
-	}{
-		"default: cert not older than 180 days doesn't needs to backup": {
-			expected: false,
-		},
-		"cert older than 180 days need to backup": {
-			adjustedExpiry: expiry + 100*time.Hour,
-			expected:       true,
-		},
-	} {
-		t.Run(desc, func(t *testing.T) {
-			tmpdir := testutil.SetupTempDir(t)
-			defer os.RemoveAll(tmpdir)
-			cfg.CertificatesDir = tmpdir
-
-			caCert, caKey, err := certsphase.KubeadmCertRootCA.CreateAsCA(cfg)
-			if err != nil {
-				t.Fatalf("failed creation of ca cert and key: %v", err)
-			}
-			caCert.NotBefore = caCert.NotBefore.Add(-test.adjustedExpiry).UTC()
-
-			err = certsphase.KubeadmCertAPIServer.CreateFromCA(cfg, caCert, caKey)
-			if err != nil {
-				t.Fatalf("Test %s: failed creation of cert and key: %v", desc, err)
-			}
-
-			certAndKey := []string{filepath.Join(tmpdir, constants.APIServerCertName), filepath.Join(tmpdir, constants.APIServerKeyName)}
-			for _, path := range certAndKey {
-				if _, err := os.Stat(path); os.IsNotExist(err) {
-					t.Fatalf("Test %s: %s not exist: %v", desc, path, err)
-				}
-			}
-
-			shouldBackup, err := shouldBackupAPIServerCertAndKey(tmpdir)
-			if err != nil {
-				t.Fatalf("Test %s: failed to check shouldBackupAPIServerCertAndKey: %v", desc, err)
-			}
-
-			if shouldBackup != test.expected {
-				t.Fatalf("Test %s: shouldBackupAPIServerCertAndKey expected %v, got %v", desc, test.expected, shouldBackup)
-			}
-		})
-	}
-}