Ensure*PKI function indicates if one or more certificate files were c…

…hanged (#308)

Co-authored-by: Angelos Kolaitis <[email protected]>

src/k8s/pkg/k8sd/app/hooks_bootstrap.go

@@ -137,7 +137,7 @@ func (a *App) onBootstrapWorkerNode(s *state.State, encodedToken string, joinCon
 	if err := certificates.CompleteCertificates(); err != nil {
 		return fmt.Errorf("failed to initialize worker node certificates: %w", err)
 	}
-	if err := setup.EnsureWorkerPKI(snap, certificates); err != nil {
+	if _, err := setup.EnsureWorkerPKI(snap, certificates); err != nil {
 		return fmt.Errorf("failed to write worker node certificates: %w", err)
 	}
 
@@ -215,7 +215,7 @@ func (a *App) onBootstrapControlPlane(s *state.State, bootstrapConfig apiv1.Boot
 		if err := certificates.CompleteCertificates(); err != nil {
 			return fmt.Errorf("failed to initialize k8s-dqlite certificates: %w", err)
 		}
-		if err := setup.EnsureK8sDqlitePKI(snap, certificates); err != nil {
+		if _, err := setup.EnsureK8sDqlitePKI(snap, certificates); err != nil {
 			return fmt.Errorf("failed to write k8s-dqlite certificates: %w", err)
 		}
 
@@ -230,7 +230,7 @@ func (a *App) onBootstrapControlPlane(s *state.State, bootstrapConfig apiv1.Boot
 		if err := certificates.CheckCertificates(); err != nil {
 			return fmt.Errorf("failed to initialize external datastore certificates: %w", err)
 		}
-		if err := setup.EnsureExtDatastorePKI(snap, certificates); err != nil {
+		if _, err := setup.EnsureExtDatastorePKI(snap, certificates); err != nil {
 			return fmt.Errorf("failed to write external datastore certificates: %w", err)
 		}
 	default:
@@ -263,7 +263,7 @@ func (a *App) onBootstrapControlPlane(s *state.State, bootstrapConfig apiv1.Boot
 	if err := certificates.CompleteCertificates(); err != nil {
 		return fmt.Errorf("failed to initialize control plane certificates: %w", err)
 	}
-	if err := setup.EnsureControlPlanePKI(snap, certificates); err != nil {
+	if _, err := setup.EnsureControlPlanePKI(snap, certificates); err != nil {
 		return fmt.Errorf("failed to write control plane certificates: %w", err)
 	}
 

src/k8s/pkg/k8sd/app/hooks_join.go

@@ -55,7 +55,7 @@ func (a *App) onPostJoin(s *state.State, initConfig map[string]string) error {
 		if err := certificates.CompleteCertificates(); err != nil {
 			return fmt.Errorf("failed to initialize k8s-dqlite certificates: %w", err)
 		}
-		if err := setup.EnsureK8sDqlitePKI(snap, certificates); err != nil {
+		if _, err := setup.EnsureK8sDqlitePKI(snap, certificates); err != nil {
 			return fmt.Errorf("failed to write k8s-dqlite certificates: %w", err)
 		}
 	case "external":
@@ -67,7 +67,7 @@ func (a *App) onPostJoin(s *state.State, initConfig map[string]string) error {
 		if err := certificates.CheckCertificates(); err != nil {
 			return fmt.Errorf("failed to initialize external datastore certificates: %w", err)
 		}
-		if err := setup.EnsureExtDatastorePKI(snap, certificates); err != nil {
+		if _, err := setup.EnsureExtDatastorePKI(snap, certificates); err != nil {
 			return fmt.Errorf("failed to write external datastore certificates: %w", err)
 		}
 	default:
@@ -103,7 +103,7 @@ func (a *App) onPostJoin(s *state.State, initConfig map[string]string) error {
 	if err := certificates.CompleteCertificates(); err != nil {
 		return fmt.Errorf("failed to initialize control plane certificates: %w", err)
 	}
-	if err := setup.EnsureControlPlanePKI(snap, certificates); err != nil {
+	if _, err := setup.EnsureControlPlanePKI(snap, certificates); err != nil {
 		return fmt.Errorf("failed to write control plane certificates: %w", err)
 	}
 

src/k8s/pkg/k8sd/controllers/control_plane_configuration.go

@@ -70,19 +70,23 @@ func (c *ControlPlaneConfigurationController) reconcile(ctx context.Context, con
 	switch config.Datastore.GetType() {
 	case "external":
 		// certificates
-		if err := setup.EnsureExtDatastorePKI(c.snap, &pki.ExternalDatastorePKI{
+		certificatesChanged, err := setup.EnsureExtDatastorePKI(c.snap, &pki.ExternalDatastorePKI{
 			DatastoreCACert:     config.Datastore.GetExternalCACert(),
 			DatastoreClientCert: config.Datastore.GetExternalClientCert(),
 			DatastoreClientKey:  config.Datastore.GetExternalClientKey(),
-		}); err != nil {
+		})
+		if err != nil {
 			return fmt.Errorf("failed to reconcile external datastore certificates: %w", err)
 		}
 
 		// kube-apiserver arguments
 		updateArgs, deleteArgs := config.Datastore.ToKubeAPIServerArguments(c.snap)
-		if mustRestart, err := snaputil.UpdateServiceArguments(c.snap, "kube-apiserver", updateArgs, deleteArgs); err != nil {
+		argsChanged, err := snaputil.UpdateServiceArguments(c.snap, "kube-apiserver", updateArgs, deleteArgs)
+		if err != nil {
 			return fmt.Errorf("failed to update kube-apiserver datastore arguments: %w", err)
-		} else if mustRestart {
+		}
+
+		if certificatesChanged || argsChanged {
 			if err := c.snap.RestartService(ctx, "kube-apiserver"); err != nil {
 				return fmt.Errorf("failed to restart kube-apiserver to apply configuration: %w", err)
 			}

src/k8s/pkg/k8sd/setup/certificates.go

@@ -11,54 +11,89 @@ import (
 )
 
 // ensureFile creates fname with the specified contents, mode and owner bits.
-// ensureFile will delete the file if contents is an empty string.
-func ensureFile(fname string, contents string, uid, gid int, mode fs.FileMode) error {
+// It will delete the file if contents parameter is an empty string. Trying to ensure a inexistent file
+// with an empty contents parameter does not result in an error.
+// It returns true if any of these is true: the file's content changed, it was created or it was deleted.
+// It also returns any error that occured.
+func ensureFile(fname string, contents string, uid, gid int, mode fs.FileMode) (bool, error) {
 	if contents == "" {
-		if err := os.Remove(fname); err != nil && !os.IsNotExist(err) {
-			return fmt.Errorf("failed to delete: %w", err)
+		if err := os.Remove(fname); err != nil {
+			if !os.IsNotExist(err) {
+				// File exists but failed to delete.
+				return false, fmt.Errorf("failed to delete: %w", err)
+			}
+			// File does not exist, nothing to do.
+			return false, nil
 		}
-		return nil
+
+		// File was deleted.
+		return true, nil
+	}
+
+	origContent, err := os.ReadFile(fname)
+	if err != nil && !os.IsNotExist(err) {
+		// File exists but failed to read.
+		return false, fmt.Errorf("failed to read: %w", err)
 	}
 
-	if err := os.WriteFile(fname, []byte(contents), mode); err != nil {
-		return fmt.Errorf("failed to write: %w", err)
+	var contentChanged bool
+
+	if contents != string(origContent) {
+		if err := os.WriteFile(fname, []byte(contents), mode); err != nil {
+			return false, fmt.Errorf("failed to write: %w", err)
+		}
+		contentChanged = true
 	}
+
 	if err := os.Chown(fname, uid, gid); err != nil {
-		return fmt.Errorf("failed to chown: %w", err)
+		return false, fmt.Errorf("failed to chown: %w", err)
 	}
 	if err := os.Chmod(fname, mode); err != nil {
-		return fmt.Errorf("failed to chmod: %w", err)
+		return false, fmt.Errorf("failed to chmod: %w", err)
 	}
 
-	return nil
+	return contentChanged, nil
 }
 
-// ensureFiles calls ensureFile for many files
-func ensureFiles(uid, gid int, mode fs.FileMode, files map[string]string) error {
-	for fname, cert := range files {
-		if err := ensureFile(fname, cert, uid, gid, mode); err != nil {
-			return fmt.Errorf("failed to configure %s: %w", path.Base(fname), err)
+// ensureFiles calls ensureFile for many files.
+// It returns true if one or more files were updated and any error that occured.
+func ensureFiles(uid, gid int, mode fs.FileMode, files map[string]string) (bool, error) {
+	var changed bool
+	for fname, content := range files {
+		if v, err := ensureFile(fname, content, uid, gid, mode); err != nil {
+			return false, fmt.Errorf("failed to configure %s: %w", path.Base(fname), err)
+		} else if v {
+			changed = true
 		}
 	}
-	return nil
+	return changed, nil
 }
 
-func EnsureExtDatastorePKI(snap snap.Snap, certificates *pki.ExternalDatastorePKI) error {
+// EnsureExtDatastorePKI ensures the external datastore PKI files are present
+// and have the correct content, permissions and ownership.
+// It returns true if one or more files were updated and any error that occured.
+func EnsureExtDatastorePKI(snap snap.Snap, certificates *pki.ExternalDatastorePKI) (bool, error) {
 	return ensureFiles(snap.UID(), snap.GID(), 0600, map[string]string{
 		path.Join(snap.EtcdPKIDir(), "ca.crt"):     certificates.DatastoreCACert,
 		path.Join(snap.EtcdPKIDir(), "client.key"): certificates.DatastoreClientKey,
 		path.Join(snap.EtcdPKIDir(), "client.crt"): certificates.DatastoreClientCert,
 	})
 }
 
-func EnsureK8sDqlitePKI(snap snap.Snap, certificates *pki.K8sDqlitePKI) error {
+// EnsureK8sDqlitePKI ensures the k8s dqlite PKI files are present
+// and have the correct content, permissions and ownership.
+// It returns true if one or more files were updated and any error that occured.
+func EnsureK8sDqlitePKI(snap snap.Snap, certificates *pki.K8sDqlitePKI) (bool, error) {
 	return ensureFiles(snap.UID(), snap.GID(), 0600, map[string]string{
 		path.Join(snap.K8sDqliteStateDir(), "cluster.crt"): certificates.K8sDqliteCert,
 		path.Join(snap.K8sDqliteStateDir(), "cluster.key"): certificates.K8sDqliteKey,
 	})
 }
 
-func EnsureControlPlanePKI(snap snap.Snap, certificates *pki.ControlPlanePKI) error {
+// EnsureControlPlanePKI ensures the control plane PKI files are present
+// and have the correct content, permissions and ownership.
+// It returns true if one or more files were updated and any error that occured.
+func EnsureControlPlanePKI(snap snap.Snap, certificates *pki.ControlPlanePKI) (bool, error) {
 	return ensureFiles(snap.UID(), snap.GID(), 0600, map[string]string{
 		path.Join(snap.KubernetesPKIDir(), "apiserver-kubelet-client.crt"): certificates.APIServerKubeletClientCert,
 		path.Join(snap.KubernetesPKIDir(), "apiserver-kubelet-client.key"): certificates.APIServerKubeletClientKey,
@@ -76,7 +111,10 @@ func EnsureControlPlanePKI(snap snap.Snap, certificates *pki.ControlPlanePKI) er
 	})
 }
 
-func EnsureWorkerPKI(snap snap.Snap, certificates *pki.WorkerNodePKI) error {
+// EnsureWorkerPKI ensures the worker PKI files are present
+// and have the correct content, permissions and ownership.
+// It returns true if one or more files were updated and any error that occured.
+func EnsureWorkerPKI(snap snap.Snap, certificates *pki.WorkerNodePKI) (bool, error) {
 	return ensureFiles(snap.UID(), snap.GID(), 0600, map[string]string{
 		path.Join(snap.KubernetesPKIDir(), "ca.crt"):      certificates.CACert,
 		path.Join(snap.KubernetesPKIDir(), "kubelet.crt"): certificates.KubeletCert,

src/k8s/pkg/k8sd/setup/certificates_internal_test.go

@@ -0,0 +1,83 @@
+package setup
+
+import (
+	"os"
+	"path"
+	"testing"
+
+	. "github.com/onsi/gomega"
+)
+
+func TestEnsureFile(t *testing.T) {
+	t.Run("CreateFile", func(t *testing.T) {
+		g := NewWithT(t)
+
+		tempDir := t.TempDir()
+		fname := path.Join(tempDir, "test")
+		updated, err := ensureFile(fname, "test", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeTrue())
+
+		createdFile, _ := os.ReadFile(fname)
+		g.Expect(string(createdFile) == "test").To(BeTrue())
+	})
+
+	t.Run("DeleteFile", func(t *testing.T) {
+		g := NewWithT(t)
+		tempDir := t.TempDir()
+		fname := path.Join(tempDir, "test")
+
+		// Create a file with some content.
+		updated, err := ensureFile(fname, "test", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeTrue())
+
+		// Delete the file.
+		updated, err = ensureFile(fname, "", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeTrue())
+
+		_, err = os.Stat(fname)
+		g.Expect(os.IsNotExist(err)).To(BeTrue())
+	})
+
+	t.Run("ChangeContent", func(t *testing.T) {
+		g := NewWithT(t)
+		tempDir := t.TempDir()
+		fname := path.Join(tempDir, "test")
+
+		// Create a file with some content.
+		updated, err := ensureFile(fname, "test", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeTrue())
+
+		// ensureFile with same content should return that the file was not updated.
+		updated, err = ensureFile(fname, "test", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeFalse())
+
+		// Change the content and ensureFile should return that the file was updated.
+		updated, err = ensureFile(fname, "new content", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeTrue())
+
+		createdFile, _ := os.ReadFile(fname)
+		g.Expect(string(createdFile) == "new content").To(BeTrue())
+
+		// Change permissions and ensureFile should return that the file was not updated.
+		updated, err = ensureFile(fname, "new content", os.Getuid(), os.Getgid(), 0666)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeFalse())
+	})
+
+	t.Run("NotExist", func(t *testing.T) {
+		g := NewWithT(t)
+		tempDir := t.TempDir()
+		fname := path.Join(tempDir, "test")
+
+		// ensureFile on inexistent file with empty content should return that the file was not updated.
+		updated, err := ensureFile(fname, "", os.Getuid(), os.Getgid(), 0777)
+		g.Expect(err).To(BeNil())
+		g.Expect(updated).To(BeFalse())
+	})
+}