diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller.go b/pkg/operator/staticpod/certsyncpod/certsync_controller.go index 111776d994..6911454be0 100644 --- a/pkg/operator/staticpod/certsyncpod/certsync_controller.go +++ b/pkg/operator/staticpod/certsyncpod/certsync_controller.go @@ -2,10 +2,12 @@ package certsyncpod import ( "context" + "fmt" "os" "path/filepath" "reflect" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" utilerrors "k8s.io/apimachinery/pkg/util/errors" @@ -17,17 +19,19 @@ import ( "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" - "github.com/openshift/library-go/pkg/operator/staticpod" "github.com/openshift/library-go/pkg/operator/staticpod/controller/installer" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" ) +const stagingDirUID = "cert-sync" + type CertSyncController struct { destinationDir string namespace string configMaps []installer.UnrevisionedResource secrets []installer.UnrevisionedResource - configmapGetter corev1interface.ConfigMapInterface + configMapGetter corev1interface.ConfigMapInterface configMapLister v1.ConfigMapLister secretGetter corev1interface.SecretInterface secretLister v1.SecretLister @@ -42,10 +46,10 @@ func NewCertSyncController(targetDir, targetNamespace string, configmaps, secret secrets: secrets, eventRecorder: eventRecorder.WithComponentSuffix("cert-sync-controller"), - configmapGetter: kubeClient.CoreV1().ConfigMaps(targetNamespace), + configMapGetter: kubeClient.CoreV1().ConfigMaps(targetNamespace), configMapLister: informers.Core().V1().ConfigMaps().Lister(), - secretLister: informers.Core().V1().Secrets().Lister(), secretGetter: kubeClient.CoreV1().Secrets(targetNamespace), + secretLister: informers.Core().V1().Secrets().Lister(), } return factory.New(). @@ -60,15 +64,12 @@ func NewCertSyncController(targetDir, targetNamespace string, configmaps, secret ) } -func getConfigMapDir(targetDir, configMapName string) string { - return filepath.Join(targetDir, "configmaps", configMapName) -} - -func getSecretDir(targetDir, secretName string) string { - return filepath.Join(targetDir, "secrets", secretName) -} - func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncContext) error { + if err := os.RemoveAll(getStagingDir(c.destinationDir)); err != nil { + c.eventRecorder.Warningf("CertificateUpdateFailed", fmt.Sprintf("Failed to prune staging directory: %v", err)) + return err + } + errors := []error{} klog.Infof("Syncing configmaps: %v", c.configMaps) @@ -80,7 +81,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue case apierrors.IsNotFound(err) && cm.Optional: - configMapFile := getConfigMapDir(c.destinationDir, cm.Name) + configMapFile := getConfigMapTargetDir(c.destinationDir, cm.Name) if _, err := os.Stat(configMapFile); os.IsNotExist(err) { // if the configmap file does not exist, there is no work to do, so skip making any live check and just return. // if the configmap actually exists in the API, we'll eventually see it on the watch. @@ -88,7 +89,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte } // Check with the live call it is really missing - configMap, err = c.configmapGetter.Get(ctx, cm.Name, metav1.GetOptions{}) + configMap, err = c.configMapGetter.Get(ctx, cm.Name, metav1.GetOptions{}) if err == nil { klog.Infof("Caches are stale. They don't see configmap '%s/%s', yet it is present", configMap.Namespace, configMap.Name) // We will get re-queued when we observe the change @@ -113,9 +114,10 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - contentDir := getConfigMapDir(c.destinationDir, cm.Name) + contentDir := getConfigMapTargetDir(c.destinationDir, cm.Name) + stagingDir := getConfigMapStagingDir(c.destinationDir, cm.Name) - data := map[string]string{} + data := make(map[string]string, len(configMap.Data)) for filename := range configMap.Data { fullFilename := filepath.Join(contentDir, filename) @@ -138,7 +140,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte klog.V(2).Infof("Syncing updated configmap '%s/%s'.", configMap.Namespace, configMap.Name) // We need to do a live get here so we don't overwrite a newer file with one from a stale cache - configMap, err = c.configmapGetter.Get(ctx, configMap.Name, metav1.GetOptions{}) + configMap, err = c.configMapGetter.Get(ctx, configMap.Name, metav1.GetOptions{}) if err != nil { // Even if the error is not exists we will act on it when caches catch up c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed getting configmap: %s/%s: %v", c.namespace, cm.Name, err) @@ -152,27 +154,11 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) { - c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed creating directory for configmap: %s/%s: %v", configMap.Namespace, configMap.Name, err) - errors = append(errors, err) - continue - } - for filename, content := range configMap.Data { - fullFilename := filepath.Join(contentDir, filename) - // if the existing is the same, do nothing - if reflect.DeepEqual(data[fullFilename], content) { - continue - } - - klog.Infof("Writing configmap manifest %q ...", fullFilename) - if err := staticpod.WriteFileAtomic([]byte(content), 0644, fullFilename); err != nil { - c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed writing file for configmap: %s/%s: %v", configMap.Namespace, configMap.Name, err) - errors = append(errors, err) - continue - } + files := make(map[string][]byte, len(configMap.Data)) + for k, v := range configMap.Data { + files[k] = []byte(v) } - c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated configmap: %s/%s", configMap.Namespace, configMap.Name) + errors = append(errors, syncDirectory(c.eventRecorder, "configmap", configMap.ObjectMeta, contentDir, 0755, stagingDir, files, 0644)) } klog.Infof("Syncing secrets: %v", c.secrets) @@ -184,7 +170,7 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue case apierrors.IsNotFound(err) && s.Optional: - secretFile := getSecretDir(c.destinationDir, s.Name) + secretFile := getSecretTargetDir(c.destinationDir, s.Name) if _, err := os.Stat(secretFile); os.IsNotExist(err) { // if the secret file does not exist, there is no work to do, so skip making any live check and just return. // if the secret actually exists in the API, we'll eventually see it on the watch. @@ -218,9 +204,10 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - contentDir := getSecretDir(c.destinationDir, s.Name) + contentDir := getSecretTargetDir(c.destinationDir, s.Name) + stagingDir := getSecretStagingDir(c.destinationDir, s.Name) - data := map[string][]byte{} + data := make(map[string][]byte, len(secret.Data)) for filename := range secret.Data { fullFilename := filepath.Join(contentDir, filename) @@ -257,29 +244,50 @@ func (c *CertSyncController) sync(ctx context.Context, syncCtx factory.SyncConte continue } - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil && !os.IsExist(err) { - c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed creating directory for secret: %s/%s: %v", secret.Namespace, secret.Name, err) - errors = append(errors, err) - continue - } - for filename, content := range secret.Data { - // TODO fix permissions - fullFilename := filepath.Join(contentDir, filename) - // if the existing is the same, do nothing - if reflect.DeepEqual(data[fullFilename], content) { - continue - } + errors = append(errors, syncDirectory(c.eventRecorder, "secret", secret.ObjectMeta, contentDir, 0755, stagingDir, secret.Data, 0600)) + } + return utilerrors.NewAggregate(errors) +} - klog.Infof("Writing secret manifest %q ...", fullFilename) - if err := staticpod.WriteFileAtomic(content, 0600, fullFilename); err != nil { - c.eventRecorder.Warningf("CertificateUpdateFailed", "Failed writing file for secret: %s/%s: %v", secret.Namespace, secret.Name, err) - errors = append(errors, err) - continue - } +func syncDirectory( + eventRecorder events.Recorder, + typeName string, o metav1.ObjectMeta, + targetDir string, targetDirPerm os.FileMode, stagingDir string, + fileContents map[string][]byte, filePerm os.FileMode, +) error { + files := make(map[string]types.File, len(fileContents)) + for filename, content := range fileContents { + files[filename] = types.File{ + Content: content, + Perm: filePerm, } - c.eventRecorder.Eventf("CertificateUpdated", "Wrote updated secret: %s/%s", secret.Namespace, secret.Name) } - return utilerrors.NewAggregate(errors) + if err := atomicdir.Sync(targetDir, targetDirPerm, stagingDir, files); err != nil { + err = fmt.Errorf("failed to sync %s %s/%s (directory %q): %w", typeName, o.Name, o.Namespace, targetDir, err) + eventRecorder.Warning("CertificateUpdateFailed", err.Error()) + return err + } + eventRecorder.Eventf("CertificateUpdated", "Wrote updated %s: %s/%s", typeName, o.Namespace, o.Name) + return nil +} + +func getStagingDir(targetDir string) string { + return filepath.Join(targetDir, "staging", stagingDirUID) +} + +func getConfigMapTargetDir(targetDir, configMapName string) string { + return filepath.Join(targetDir, "configmaps", configMapName) +} + +func getConfigMapStagingDir(targetDir, configMapName string) string { + return filepath.Join(getStagingDir(targetDir), "configmaps", configMapName) +} + +func getSecretTargetDir(targetDir, secretName string) string { + return filepath.Join(targetDir, "secrets", secretName) +} + +func getSecretStagingDir(targetDir, secretName string) string { + return filepath.Join(getStagingDir(targetDir), "secrets", secretName) } diff --git a/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go b/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go new file mode 100644 index 0000000000..f4611d7fde --- /dev/null +++ b/pkg/operator/staticpod/certsyncpod/certsync_controller_linux_test.go @@ -0,0 +1,780 @@ +//go:build linux + +package certsyncpod + +import ( + "bytes" + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "fmt" + "math/big" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/apiserver/pkg/server/dynamiccertificates" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + ktesting "k8s.io/client-go/testing" + clocktesting "k8s.io/utils/clock/testing" + + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/openshift/library-go/pkg/operator/staticpod/controller/installer" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) + +const testingNamespace = "test-namespace" + +// TestCertSyncController_Sync tests the sync method in various scenarios. +func TestCertSyncController_Sync(t *testing.T) { + testCases := []struct { + name string + configMapsToSync []installer.UnrevisionedResource + configMapObjects []*corev1.ConfigMap + configMapGetErrors map[string]error + secretsToSync []installer.UnrevisionedResource + secretObjects []*corev1.Secret + secretGetErrors map[string]error + // Keys are paths relative to the controller destination directory. + existingDirectories map[string]adtesting.DirectoryState + expectedError bool + expectedEventTypes []string + // Keys are paths relative to the controller destination directory. + expectedDirectories map[string]adtesting.DirectoryState + }{ + { + name: "create first configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "test-config-content", + }), + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + }, + }, + { + name: "add another configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config-1"}, + {Name: "test-config-2"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config-1", map[string]string{ + "config.yaml": "test-config-content-1", + }), + createConfigMap("test-config-2", map[string]string{ + "config.yaml": "test-config-content-2", + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "config.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "config.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + "configmaps/test-config-2": { + "config.yaml": { + Content: []byte("test-config-content-2"), + Perm: 0644, + }, + }, + }, + }, + { + name: "update existing configmap", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config", Optional: false}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "updated-config-content", + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("updated-config-content"), + Perm: 0644, + }, + }, + }, + }, + { + name: "succeed when optional configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "optional-config", Optional: true}, + }, + }, + { + name: "fail when required configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "required-config", Optional: false}, + }, + expectedError: true, + }, + { + name: "remove directory when optional configmap missing", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "optional-config", Optional: true}, + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/optional-config": { + "config.yaml": { + Content: []byte("optional-config-content"), + Perm: 0644, + }, + }, + }, + expectedEventTypes: []string{"CertificateRemoved"}, + }, + { + name: "configmap unchanged on get error", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "error-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("error-config", map[string]string{ + "config.yaml": "updated-config-content", + }), + }, + configMapGetErrors: map[string]error{ + "error-config": apierrors.NewInternalError(fmt.Errorf("API server error")), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/error-config": { + "config.yaml": { + Content: []byte("error-config-content"), + Perm: 0644, + }, + }, + }, + expectedError: true, + expectedEventTypes: []string{"CertificateUpdateFailed"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/error-config": { + "config.yaml": { + Content: []byte("error-config-content"), + Perm: 0644, + }, + }, + }, + }, + + { + name: "create first secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("test-cert-content"), + "tls.key": []byte("test-key-content"), + }), + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + }, + { + name: "add another secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret-1"}, + {Name: "test-secret-2"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret-1", map[string][]byte{ + "tls-1.crt": []byte("test-cert-content-1"), + "tls-1.key": []byte("test-key-content-1"), + }), + createSecret("test-secret-2", map[string][]byte{ + "tls-2.crt": []byte("test-cert-content-2"), + "tls-2.key": []byte("test-key-content-2"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret-1": { + "tls-1.crt": { + Content: []byte("test-cert-content-1"), + Perm: 0600, + }, + "tls-1.key": { + Content: []byte("test-key-content-1"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret-1": { + "tls-1.crt": { + Content: []byte("test-cert-content-1"), + Perm: 0600, + }, + "tls-1.key": { + Content: []byte("test-key-content-1"), + Perm: 0600, + }, + }, + "secrets/test-secret-2": { + "tls-2.crt": { + Content: []byte("test-cert-content-2"), + Perm: 0600, + }, + "tls-2.key": { + Content: []byte("test-key-content-2"), + Perm: 0600, + }, + }, + }, + }, + { + name: "update existing secret", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret", Optional: true}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("updated-cert-content"), + "tls.key": []byte("updated-key-content"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/test-secret": { + "tls.crt": { + Content: []byte("updated-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("updated-key-content"), + Perm: 0600, + }, + }, + }, + }, + { + name: "succeed when optional secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "optional-secret", Optional: true}, + }, + }, + { + name: "fail when required secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "required-secret", Optional: false}, + }, + expectedError: true, + }, + { + name: "remove directory when optional secret missing", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "optional-secret", Optional: true}, + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/optional-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: []string{"CertificateRemoved"}, + }, + { + name: "secret unchanged on get error", + secretsToSync: []installer.UnrevisionedResource{ + {Name: "error-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("error-secret", map[string][]byte{ + "token": []byte("updated-secret-content"), + }), + }, + secretGetErrors: map[string]error{ + "error-secret": apierrors.NewInternalError(fmt.Errorf("API server error")), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "secrets/error-secret": { + "token": { + Content: []byte("error-config-content"), + Perm: 0600, + }, + }, + }, + expectedError: true, + expectedEventTypes: []string{"CertificateUpdateFailed"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "secrets/error-secret": { + "token": { + Content: []byte("error-config-content"), + Perm: 0600, + }, + }, + }, + }, + + { + name: "create multiple configmaps and secrets", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config-1", Optional: false}, + {Name: "test-config-2", Optional: true}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config-1", map[string]string{ + "app.yaml": "test-config-content-1", + }), + createConfigMap("test-config-2", map[string]string{ + "opt.yaml": "test-config-content-2", + }), + }, + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret-1", Optional: false}, + {Name: "test-secret-2", Optional: true}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret-1", map[string][]byte{ + "token": []byte("test-secret-content-1"), + }), + createSecret("test-secret-2", map[string][]byte{ + "key": []byte("test-secret-content-2"), + }), + }, + expectedEventTypes: []string{"CertificateUpdated", "CertificateUpdated", "CertificateUpdated", "CertificateUpdated"}, + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config-1": { + "app.yaml": { + Content: []byte("test-config-content-1"), + Perm: 0644, + }, + }, + "configmaps/test-config-2": { + "opt.yaml": { + Content: []byte("test-config-content-2"), + Perm: 0644, + }, + }, + "secrets/test-secret-1": { + "token": { + Content: []byte("test-secret-content-1"), + Perm: 0600, + }, + }, + "secrets/test-secret-2": { + "key": { + Content: []byte("test-secret-content-2"), + Perm: 0600, + }, + }, + }, + }, + { + name: "already synchronized", + configMapsToSync: []installer.UnrevisionedResource{ + {Name: "test-config"}, + }, + configMapObjects: []*corev1.ConfigMap{ + createConfigMap("test-config", map[string]string{ + "config.yaml": "test-config-content", + }), + }, + secretsToSync: []installer.UnrevisionedResource{ + {Name: "test-secret"}, + }, + secretObjects: []*corev1.Secret{ + createSecret("test-secret", map[string][]byte{ + "tls.crt": []byte("test-cert-content"), + "tls.key": []byte("test-key-content"), + }), + }, + existingDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + expectedEventTypes: nil, // No events when no changes. + expectedDirectories: map[string]adtesting.DirectoryState{ + "configmaps/test-config": { + "config.yaml": { + Content: []byte("test-config-content"), + Perm: 0644, + }, + }, + "secrets/test-secret": { + "tls.crt": { + Content: []byte("test-cert-content"), + Perm: 0600, + }, + "tls.key": { + Content: []byte("test-key-content"), + Perm: 0600, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + controller, eventRecorder, stopCh := createController(t.TempDir(), + tc.configMapsToSync, tc.configMapObjects, tc.configMapGetErrors, + tc.secretsToSync, tc.secretObjects, tc.secretGetErrors, + ) + defer close(stopCh) + + for path, state := range tc.existingDirectories { + targetPath := filepath.Join(controller.destinationDir, path) + state.Write(t, targetPath, 0755) + } + + syncCtx := factory.NewSyncContext("CertSyncController", eventRecorder) + err := controller.sync(context.Background(), syncCtx) + if err != nil { + t.Log("sync returned an error:", err) + } + + if tc.expectedError && err == nil { + t.Errorf("Expected error but got none") + } else if !tc.expectedError && err != nil { + t.Errorf("Unexpected error: %v", err) + } + + verifyEvents(t, eventRecorder, tc.expectedEventTypes) + + // Check filesystem state. We need to gather all paths to know which are extra. + extraPaths := sets.NewString() + filepath.Walk(controller.destinationDir, func(path string, info os.FileInfo, err error) error { + if !info.IsDir() || + path == controller.destinationDir || + strings.HasSuffix(path, "/staging") || + strings.HasSuffix(path, "/staging/cert-sync") || + strings.HasSuffix(path, "/staging/cert-sync/secrets") || + strings.HasSuffix(path, "/staging/cert-sync/configmaps") || + path == filepath.Join(controller.destinationDir, "configmaps") || + path == filepath.Join(controller.destinationDir, "secrets") { + + return nil + } + + extraPaths.Insert(path) + return nil + }) + for path, state := range tc.expectedDirectories { + targetPath := filepath.Join(controller.destinationDir, path) + state.CheckDirectoryMatches(t, targetPath, 0755) + extraPaths.Delete(targetPath) + } + if extraPaths.Len() > 0 { + t.Errorf("Directories that should not be there detected: %v", extraPaths.List()) + } + }) + } +} + +func createController( + destinationDir string, + configMapsToSync []installer.UnrevisionedResource, + configMapObjects []*corev1.ConfigMap, + configMapGetErrors map[string]error, + secretsToSync []installer.UnrevisionedResource, + secretObjects []*corev1.Secret, + secretGetErrors map[string]error, +) (*CertSyncController, events.Recorder, chan struct{}) { + kubeObjects := make([]runtime.Object, 0) + for _, cm := range configMapObjects { + kubeObjects = append(kubeObjects, cm) + } + for _, secret := range secretObjects { + kubeObjects = append(kubeObjects, secret) + } + kubeClient := fake.NewClientset(kubeObjects...) + + if configMapGetErrors != nil { + kubeClient.PrependReactor("get", "configmaps", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(ktesting.GetAction) + if err, exists := configMapGetErrors[getAction.GetName()]; exists { + return true, nil, err + } + return false, nil, nil + }) + } + + if secretGetErrors != nil { + kubeClient.PrependReactor("get", "secrets", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + getAction := action.(ktesting.GetAction) + if err, exists := secretGetErrors[getAction.GetName()]; exists { + return true, nil, err + } + return false, nil, nil + }) + } + + informers := informers.NewSharedInformerFactoryWithOptions(kubeClient, 1*time.Hour) + eventRecorder := events.NewInMemoryRecorder("CertSyncController", clocktesting.NewFakeClock(time.Now())) + + controller := &CertSyncController{ + destinationDir: destinationDir, + namespace: testingNamespace, + configMaps: configMapsToSync, + secrets: secretsToSync, + configMapGetter: kubeClient.CoreV1().ConfigMaps(testingNamespace), + configMapLister: informers.Core().V1().ConfigMaps().Lister(), + secretGetter: kubeClient.CoreV1().Secrets(testingNamespace), + secretLister: informers.Core().V1().Secrets().Lister(), + eventRecorder: eventRecorder, + } + + stopCh := make(chan struct{}) + informers.Start(stopCh) + informers.WaitForCacheSync(stopCh) + + return controller, eventRecorder, stopCh +} + +func verifyEvents(t *testing.T, eventRecorder events.Recorder, expectedEventTypes []string) { + var gotEventTypes []string + for _, event := range eventRecorder.(events.InMemoryRecorder).Events() { + gotEventTypes = append(gotEventTypes, event.Reason) + } + + if !cmp.Equal(gotEventTypes, expectedEventTypes) { + t.Errorf("Unexpected event types (-want, +got): \n%v", cmp.Diff(expectedEventTypes, gotEventTypes)) + } +} + +func createConfigMap(name string, data map[string]string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testingNamespace, + }, + Data: data, + } +} + +func createSecret(name string, data map[string][]byte) *corev1.Secret { + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testingNamespace, + }, + Data: data, + } +} + +// TestDynamicCertificates makes sure the receiving side of certificate synchronization works as expected. +// It reads and watches the certificates being synchronized in the same way as e.g. kube-apiserver, +// the very same libraries are being used. +func TestDynamicCertificates(t *testing.T) { + const typeName = "secret" + om := metav1.ObjectMeta{ + Namespace: "openshift-kube-apiserver", + Name: "s1", + } + + // Generate all necessary keypairs. + tlsCert, tlsKey := generateKeypair(t) + tlsCertUpdated, tlsKeyUpdated := generateKeypair(t) + + // Write the keypair into a secret directory. + secretDir := filepath.Join(t.TempDir(), "secrets", om.Name) + stagingDir := filepath.Join(t.TempDir(), "staging", stagingDirUID, "secrets", om.Name) + certFile := filepath.Join(secretDir, "tls.crt") + keyFile := filepath.Join(secretDir, "tls.key") + + if err := os.MkdirAll(secretDir, 0700); err != nil { + t.Fatalf("Failed to create secret directory %q: %v", secretDir, err) + } + if err := os.WriteFile(certFile, tlsCert, 0600); err != nil { + t.Fatalf("Failed to write TLS certificate into %q: %v", certFile, err) + } + if err := os.WriteFile(keyFile, tlsKey, 0600); err != nil { + t.Fatalf("Failed to write TLS key into %q: %v", keyFile, err) + } + + // Start the watcher. + // This reads the keypair synchronously so the initial state is loaded here. + dc, err := dynamiccertificates.NewDynamicServingContentFromFiles("localhost TLS", certFile, keyFile) + if err != nil { + t.Fatalf("Failed to init dynamic certificate: %v", err) + } + + // Check the initial keypair is loaded. + cert, key := dc.CurrentCertKeyContent() + if !bytes.Equal(cert, tlsCert) || !bytes.Equal(key, tlsKey) { + t.Fatal("Unexpected initial keypair loaded") + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + dc.Run(ctx, 1) + }() + defer wg.Wait() + defer cancel() + + // Poll until update detected. + files := map[string]types.File{ + "tls.crt": {Content: tlsCertUpdated, Perm: 0600}, + "tls.key": {Content: tlsKeyUpdated, Perm: 0600}, + } + err = wait.PollUntilContextCancel(ctx, 250*time.Millisecond, true, func(ctx context.Context) (bool, error) { + // Replace the secret directory. + if err := atomicdir.Sync(secretDir, 0700, stagingDir, files); err != nil { + t.Errorf("Failed to write files: %v", err) + return false, err + } + + // Check the loaded content matches. + // This is most probably updated based on write in a previous Poll invocation. + cert, key := dc.CurrentCertKeyContent() + return bytes.Equal(cert, tlsCertUpdated) && bytes.Equal(key, tlsKeyUpdated), nil + }) + if err != nil { + t.Fatalf("Failed to wait for dynamic certificate: %v", err) + } +} + +// generateKeypair returns (cert, key). +func generateKeypair(t *testing.T) ([]byte, []byte) { + t.Helper() + + privateKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader) + if err != nil { + t.Fatalf("Failed to generate TLS key: %v", err) + } + + notBefore := time.Now() + notAfter := notBefore.Add(1 * time.Hour) + + serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) + serialNumber, err := rand.Int(rand.Reader, serialNumberLimit) + if err != nil { + t.Fatalf("Failed to generate serial number for TLS keypair: %v", err) + } + + template := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + Organization: []string{"Example Org"}, + }, + NotBefore: notBefore, + NotAfter: notAfter, + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + DNSNames: []string{"example.com"}, + } + + publicKeyBytes, err := x509.CreateCertificate(rand.Reader, &template, &template, &privateKey.PublicKey, privateKey) + if err != nil { + t.Fatalf("Failed to create TLS certificate: %v", err) + } + + var certOut bytes.Buffer + if err := pem.Encode(&certOut, &pem.Block{Type: "CERTIFICATE", Bytes: publicKeyBytes}); err != nil { + t.Fatalf("Failed to write certificate PEM: %v", err) + } + + privateKeyBytes, err := x509.MarshalPKCS8PrivateKey(privateKey) + if err != nil { + t.Fatalf("Unable to marshal private key: %v", err) + } + + var keyOut bytes.Buffer + if err := pem.Encode(&keyOut, &pem.Block{Type: "PRIVATE KEY", Bytes: privateKeyBytes}); err != nil { + t.Fatalf("Failed to write certificate PEM: %v", err) + } + + return certOut.Bytes(), keyOut.Bytes() +} diff --git a/pkg/operator/staticpod/installerpod/cmd.go b/pkg/operator/staticpod/installerpod/cmd.go index 5d065b40fc..68666ff196 100644 --- a/pkg/operator/staticpod/installerpod/cmd.go +++ b/pkg/operator/staticpod/installerpod/cmd.go @@ -3,39 +3,43 @@ package installerpod import ( "context" "fmt" - "k8s.io/utils/clock" "os" "path" + "path/filepath" "sort" "strconv" "strings" "time" - "k8s.io/apimachinery/pkg/util/wait" - "github.com/blang/semver/v4" "github.com/davecgh/go-spew/spew" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" "github.com/spf13/cobra" "github.com/spf13/pflag" - "k8s.io/klog/v2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/server" "k8s.io/client-go/kubernetes" "k8s.io/client-go/rest" + "k8s.io/klog/v2" + "k8s.io/utils/clock" "github.com/openshift/library-go/pkg/config/client" "github.com/openshift/library-go/pkg/operator/events" "github.com/openshift/library-go/pkg/operator/resource/resourceread" "github.com/openshift/library-go/pkg/operator/resource/retry" - "github.com/openshift/library-go/pkg/operator/staticpod" "github.com/openshift/library-go/pkg/operator/staticpod/internal" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir" "github.com/openshift/library-go/pkg/operator/staticpod/internal/flock" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/fsutil" ) +const stagingDirUID = "installer" + type InstallOptions struct { // TODO replace with genericclioptions KubeConfig string @@ -218,8 +222,10 @@ func (o *InstallOptions) kubeletVersion(ctx context.Context) (string, error) { func (o *InstallOptions) copySecretsAndConfigMaps(ctx context.Context, resourceDir string, secretNames, optionalSecretNames, configNames, optionalConfigNames sets.Set[string], prefixed bool) error { - klog.Infof("Creating target resource directory %q ...", resourceDir) - if err := os.MkdirAll(resourceDir, 0755); err != nil && !os.IsExist(err) { + + stagingDirBase := getStagingDir(resourceDir) + klog.Infof("Pruning staging directory %q ...", stagingDirBase) + if err := os.RemoveAll(stagingDirBase); err != nil { return err } @@ -257,34 +263,43 @@ func (o *InstallOptions) copySecretsAndConfigMaps(ctx context.Context, resourceD if prefixed { secretBaseName = o.prefixFor(secret.Name) } - contentDir := path.Join(resourceDir, "secrets", secretBaseName) - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil { - return err - } - for filename, content := range secret.Data { - if err := writeSecret(content, path.Join(contentDir, filename)); err != nil { - return err + + contentDir := getSecretTargetDir(resourceDir, secretBaseName) + stagingDir := getSecretStagingDir(resourceDir, secretBaseName) + + files := make(map[string]types.File, len(secret.Data)) + for name, content := range secret.Data { + files[name] = types.File{ + Content: content, + Perm: getFilePermissionsSecret(name), } } + + if err := atomicdir.Sync(contentDir, 0755, stagingDir, files); err != nil { + return fmt.Errorf("failed to sync secret %s/%s (directory %q): %w", secret.Namespace, secret.Name, contentDir, err) + } } for _, configmap := range configs { configMapBaseName := configmap.Name if prefixed { configMapBaseName = o.prefixFor(configmap.Name) } - contentDir := path.Join(resourceDir, "configmaps", configMapBaseName) - klog.Infof("Creating directory %q ...", contentDir) - if err := os.MkdirAll(contentDir, 0755); err != nil { - return err - } - for filename, content := range configmap.Data { - if err := writeConfig([]byte(content), path.Join(contentDir, filename)); err != nil { - return err + + contentDir := getConfigMapTargetDir(resourceDir, configMapBaseName) + stagingDir := getConfigMapStagingDir(resourceDir, configMapBaseName) + + files := make(map[string]types.File, len(configmap.Data)) + for name, content := range configmap.Data { + files[name] = types.File{ + Content: []byte(content), + Perm: getFilePermissionsConfigMap(name), } } - } + if err := atomicdir.Sync(contentDir, 0755, stagingDir, files); err != nil { + return fmt.Errorf("failed to sync configmap %s/%s (directory %q): %w", configmap.Namespace, configmap.Name, contentDir, err) + } + } return nil } @@ -608,8 +623,8 @@ func (o *InstallOptions) writePod(rawPodBytes []byte, manifestFileName, resource // Write secrets, config maps and pod to disk // This does not need timeout, instead we should fail hard when we are not able to write. klog.Infof("Writing pod manifest %q ...", path.Join(resourceDir, manifestFileName)) - if err := os.WriteFile(path.Join(resourceDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { - return err + if err := fsutil.WriteFileFsync(path.Join(resourceDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { + return fmt.Errorf("failed writing %q: %w", path.Join(resourceDir, manifestFileName), err) } // remove the existing file to ensure kubelet gets "create" event from inotify watchers @@ -619,28 +634,42 @@ func (o *InstallOptions) writePod(rawPodBytes []byte, manifestFileName, resource return err } klog.Infof("Writing static pod manifest %q ...\n%s", path.Join(o.PodManifestDir, manifestFileName), finalPodBytes) - if err := os.WriteFile(path.Join(o.PodManifestDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { - return err + if err := fsutil.WriteFileFsync(path.Join(o.PodManifestDir, manifestFileName), []byte(finalPodBytes), 0600); err != nil { + return fmt.Errorf("failed writing %q: %w", path.Join(o.PodManifestDir, manifestFileName), err) } return nil } -func writeConfig(content []byte, fullFilename string) error { - klog.Infof("Writing config file %q ...", fullFilename) +func getStagingDir(targetDir string) string { + return filepath.Join(targetDir, "staging", stagingDirUID) +} + +func getConfigMapTargetDir(targetDir, configMapName string) string { + return filepath.Join(targetDir, "configmaps", configMapName) +} - filePerms := os.FileMode(0600) - if strings.HasSuffix(fullFilename, ".sh") { - filePerms = 0755 - } - return staticpod.WriteFileAtomic(content, filePerms, fullFilename) +func getConfigMapStagingDir(targetDir, configMapName string) string { + return filepath.Join(getStagingDir(targetDir), "configmaps", configMapName) +} + +func getSecretTargetDir(targetDir, secretName string) string { + return filepath.Join(targetDir, "secrets", secretName) } -func writeSecret(content []byte, fullFilename string) error { - klog.Infof("Writing secret manifest %q ...", fullFilename) +func getSecretStagingDir(targetDir, secretName string) string { + return filepath.Join(getStagingDir(targetDir), "secrets", secretName) +} + +func getFilePermissionsConfigMap(filename string) os.FileMode { + if strings.HasSuffix(filename, ".sh") { + return 0755 + } + return 0600 +} - filePerms := os.FileMode(0600) - if strings.HasSuffix(fullFilename, ".sh") { - filePerms = 0700 +func getFilePermissionsSecret(filename string) os.FileMode { + if strings.HasSuffix(filename, ".sh") { + return 0700 } - return staticpod.WriteFileAtomic(content, filePerms, fullFilename) + return 0600 } diff --git a/pkg/operator/staticpod/installerpod/cmd_test.go b/pkg/operator/staticpod/installerpod/cmd_test.go index 5e6201b47b..cd046920ec 100644 --- a/pkg/operator/staticpod/installerpod/cmd_test.go +++ b/pkg/operator/staticpod/installerpod/cmd_test.go @@ -4,11 +4,11 @@ import ( "context" "os" "path" - "reflect" "strings" "testing" "time" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -51,8 +51,8 @@ func TestCopyContent(t *testing.T) { Revision: "006", Namespace: "some-ns", PodConfigMapNamePrefix: "kube-apiserver-pod", - SecretNamePrefixes: []string{"first", "second"}, - ConfigMapNamePrefixes: []string{"alpha", "bravo"}, + SecretNamePrefixes: []string{"first", "second", "third"}, + ConfigMapNamePrefixes: []string{"alpha", "bravo", "delta"}, }, client: func() *fake.Clientset { return fake.NewSimpleClientset( @@ -70,6 +70,12 @@ func TestCopyContent(t *testing.T) { "dos-B.crt": []byte("dos"), }, }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "third-006"}, + Data: map[string][]byte{ + "run-third.sh": []byte("echo third"), + }, + }, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "alpha-006"}, Data: map[string]string{ @@ -90,23 +96,31 @@ func TestCopyContent(t *testing.T) { "pod.yaml": podYaml, }, }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Namespace: "some-ns", Name: "delta-006"}, + Data: map[string]string{ + "run-delta.sh": "echo delta", + }, + }, ) }, expected: func(t *testing.T, resourceDir, podDir string) { - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano") + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "run-third.sh"), "echo third", 0700) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "delta", "run-delta.sh"), "echo delta", 0755) checkFileContentMatchesPod(t, path.Join(resourceDir, "kube-apiserver-pod-006", "kube-apiserver-pod.yaml"), podYaml) checkFileContentMatchesPod(t, path.Join(podDir, "kube-apiserver-pod.yaml"), podYaml) }, }, { - name: "optional-secrets-confmaps", + name: "optional-secrets-configmaps", o: InstallOptions{ Revision: "006", Namespace: "some-ns", @@ -169,18 +183,18 @@ func TestCopyContent(t *testing.T) { ) }, expected: func(t *testing.T, resourceDir, podDir string) { - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "tres-C.crt"), "tres") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "cuatro-C.crt"), "cuatro") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "apple-C.crt"), "apple") - checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "banana-C.crt"), "banana") + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "one-A.crt"), "one", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "first", "two-A.crt"), "two", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "uno-B.crt"), "uno", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "second", "dos-B.crt"), "dos", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "tres-C.crt"), "tres", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "secrets", "third", "cuatro-C.crt"), "cuatro", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "apple-A.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "alpha", "banana-A.crt"), "banana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "manzana-B.crt"), "manzana", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "bravo", "platano-B.crt"), "platano", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "apple-C.crt"), "apple", 0600) + checkFileContent(t, path.Join(resourceDir, "kube-apiserver-pod-006", "configmaps", "charlie", "banana-C.crt"), "banana", 0600) checkFileContentMatchesPod(t, path.Join(resourceDir, "kube-apiserver-pod-006", "kube-apiserver-pod.yaml"), podYaml) checkFileContentMatchesPod(t, path.Join(podDir, "kube-apiserver-pod.yaml"), podYaml) }, @@ -215,13 +229,7 @@ func TestCopyContent(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - testDir, err := os.MkdirTemp("", "copy-content-test") - if err != nil { - t.Fatal(err) - } - defer func() { - os.Remove(testDir) - }() + testDir := t.TempDir() o := test.o o.KubeClient = test.client() @@ -230,7 +238,7 @@ func TestCopyContent(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - err = o.copyContent(ctx) + err := o.copyContent(ctx) switch { case err == nil && len(test.expectedErr) == 0: case err != nil && len(test.expectedErr) == 0: @@ -258,15 +266,25 @@ func TestKubeletVersion(t *testing.T) { } } -func checkFileContent(t *testing.T, file, expected string) { - actual, err := os.ReadFile(file) +func checkFileContent(t *testing.T, file, expected string, expectedPerm os.FileMode) { + actualBytes, err := os.ReadFile(file) if err != nil { t.Error(err) return } + actual := string(actualBytes) + + stat, err := os.Stat(file) + if err != nil { + t.Errorf("Failed to stat %q: %v", file, err) + return + } + if gotPerm := stat.Mode().Perm(); gotPerm != expectedPerm { + t.Errorf("File permissions mismatch for %q: expected %v, got %v", file, gotPerm, expectedPerm) + } - if !reflect.DeepEqual(expected, string(actual)) { - t.Errorf("%q: expected %q, got %q", file, expected, string(actual)) + if !cmp.Equal(expected, actual) { + t.Errorf("File content mismatch for %q:\n%s", file, cmp.Diff(expected, actual)) } } diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux.go new file mode 100644 index 0000000000..9ce912af76 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux.go @@ -0,0 +1,22 @@ +//go:build linux + +package atomicdir + +import ( + "golang.org/x/sys/unix" +) + +// swap can be used to exchange two directories atomically. +func swap(firstDir, secondDir string) error { + // Renameat2 can be used to exchange two directories atomically when RENAME_EXCHANGE flag is specified. + // The paths to be exchanged can be specified in multiple ways: + // + // * You can specify a file descriptor and a relative path to that descriptor. + // * You can specify an absolute path, in which case the file descriptor is ignored. + // + // We use AT_FDCWD special file descriptor so that when any of the paths is relative, + // it's relative to the current working directory. + // + // For more details, see `man renameat2` as that is the associated C library function. + return unix.Renameat2(unix.AT_FDCWD, firstDir, unix.AT_FDCWD, secondDir, unix.RENAME_EXCHANGE) +} diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go new file mode 100644 index 0000000000..a477b2fa45 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_linux_test.go @@ -0,0 +1,185 @@ +//go:build linux + +package atomicdir + +import ( + "os" + "path/filepath" + "testing" + + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" +) + +func TestSwap(t *testing.T) { + stateFirst := adtesting.DirectoryState{ + "1.txt": { + Content: []byte("hello 1 world"), + Perm: 0600, + }, + "2.txt": { + Content: []byte("hello 2 world"), + Perm: 0400, + }, + } + stateSecond := adtesting.DirectoryState{ + "a.txt": { + Content: []byte("hello a world"), + Perm: 0600, + }, + } + stateEmpty := adtesting.DirectoryState{} + + expectNoError := func(t *testing.T, err error) { + t.Helper() + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + } + + checkSuccess := func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { + t.Helper() + expectNoError(t, err) + + // Make sure the contents are swapped. + stateFirst.CheckDirectoryMatches(t, pathSecond, 0755) + stateSecond.CheckDirectoryMatches(t, pathFirst, 0755) + } + + testCases := []struct { + name string + setup func(t *testing.T, tmpDir string) (pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState) + checkResult func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) + }{ + { + name: "success with absolute paths", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) + + return pathFirst, stateFirst, pathSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with the first path relative", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) + + cwd, err := os.Getwd() + expectNoError(t, err) + + relFirst, err := filepath.Rel(cwd, pathFirst) + expectNoError(t, err) + + return relFirst, stateFirst, pathSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with the second path relative", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst, 0755) + stateSecond.Write(t, pathSecond, 0755) + + cwd, err := os.Getwd() + expectNoError(t, err) + + relSecond, err := filepath.Rel(cwd, pathSecond) + expectNoError(t, err) + + return pathFirst, stateFirst, relSecond, stateSecond + }, + checkResult: checkSuccess, + }, + { + name: "success with an empty directory", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateFirst.Write(t, pathFirst, 0755) + stateEmpty.Write(t, pathSecond, 0755) + + return pathFirst, stateFirst, pathSecond, stateEmpty + }, + checkResult: checkSuccess, + }, + { + name: "success with both directories empty", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + stateEmpty.Write(t, pathFirst, 0755) + stateEmpty.Write(t, pathSecond, 0755) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: checkSuccess, + }, + { + name: "error with the first directory not existing", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + expectNoError(t, os.Mkdir(pathSecond, 0755)) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { + if !os.IsNotExist(err) { + t.Errorf("Expected a directory not exists error, got %v", err) + } + }, + }, + { + name: "error with the second directory not existing", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + expectNoError(t, os.Mkdir(pathFirst, 0755)) + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { + if !os.IsNotExist(err) { + t.Errorf("Expected a directory not exists error, got %v", err) + } + }, + }, + { + name: "error with no directory existing", + setup: func(t *testing.T, tmpDir string) (string, adtesting.DirectoryState, string, adtesting.DirectoryState) { + pathFirst := filepath.Join(tmpDir, "first") + pathSecond := filepath.Join(tmpDir, "second") + + return pathFirst, stateEmpty, pathSecond, stateEmpty + }, + checkResult: func(t *testing.T, pathFirst string, stateFirst adtesting.DirectoryState, pathSecond string, stateSecond adtesting.DirectoryState, err error) { + if !os.IsNotExist(err) { + t.Errorf("Expected a directory not exists error, got %v", err) + } + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pathFirst, stateFirst, pathSecond, stateSecond := tc.setup(t, t.TempDir()) + tc.checkResult(t, pathFirst, stateFirst, pathSecond, stateSecond, swap(pathFirst, pathSecond)) + }) + } +} diff --git a/pkg/operator/staticpod/internal/atomicdir/swap_other.go b/pkg/operator/staticpod/internal/atomicdir/swap_other.go new file mode 100644 index 0000000000..6f5c0fed9e --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/swap_other.go @@ -0,0 +1,12 @@ +//go:build !linux + +package atomicdir + +import "errors" + +// swap can be used to exchange two directories atomically. +// +// This function is only implemented for Linux and returns an error on other platforms. +func swap(firstDir, secondDir string) error { + return errors.New("swap is not supported on this platform") +} diff --git a/pkg/operator/staticpod/internal/atomicdir/sync.go b/pkg/operator/staticpod/internal/atomicdir/sync.go new file mode 100644 index 0000000000..779eb21288 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/sync.go @@ -0,0 +1,100 @@ +package atomicdir + +import ( + "fmt" + "os" + "path/filepath" + + "k8s.io/klog/v2" + + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/fsutil" +) + +// Sync atomically and durably replaces the contents of targetDir with the given files. +// It writes files to a staging directory with fsync, then atomically swaps it with +// the target directory via renameat2(RENAME_EXCHANGE), fsyncing parent directories +// to ensure the swap is persisted. Extra files in targetDir are pruned. +// The old target directory (now at stagingDir) is removed after the swap. +func Sync(targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) error { + return sync(&realFS, targetDir, targetDirPerm, stagingDir, files) +} + +type fileSystem struct { + MkdirAll func(path string, perm os.FileMode) error + RemoveAll func(path string) error + WriteFileFsync func(name string, data []byte, perm os.FileMode) error + SwapDirectories func(dirA, dirB string) error + Fsync func(name string) error +} + +var realFS = fileSystem{ + MkdirAll: os.MkdirAll, + RemoveAll: os.RemoveAll, + WriteFileFsync: fsutil.WriteFileFsync, + SwapDirectories: swap, + Fsync: fsutil.Fsync, +} + +// sync writes files into the staging directory, then durably swaps it with the target. +// Each file write is individually fsynced (including its parent directory entry) via +// fs.WriteFileFsync. After the swap, parent directories are fsynced to persist the exchange. +// +// Note: the upstream Kubernetes AtomicWriter uses symlinks for atomic updates but does +// not fsync, leaving file data in the page cache with no crash durability guarantee: +// https://github.com/kubernetes/kubernetes/blob/v1.34.0/pkg/volume/util/atomic_writer.go#L58 +// We use renameat2(RENAME_EXCHANGE) instead of symlinks because we need to swap an +// existing directory that is already being watched. Migrating to symlinks would still +// require an atomic swap at the OS level, adding complexity for no benefit. +func sync(fs *fileSystem, targetDir string, targetDirPerm os.FileMode, stagingDir string, files map[string]types.File) (retErr error) { + klog.Infof("Ensuring target directory %q exists ...", targetDir) + if err := fs.MkdirAll(targetDir, targetDirPerm); err != nil { + return fmt.Errorf("failed creating target directory: %w", err) + } + + klog.Infof("Creating staging directory %q ...", stagingDir) + if err := fs.MkdirAll(stagingDir, targetDirPerm); err != nil { + return fmt.Errorf("failed creating staging directory: %w", err) + } + defer func() { + if err := fs.RemoveAll(stagingDir); err != nil { + if retErr != nil { + retErr = fmt.Errorf("failed removing staging directory %q: %w; previous error: %w", stagingDir, err, retErr) + return + } + retErr = fmt.Errorf("failed removing staging directory %q: %w", stagingDir, err) + } + }() + + for filename, file := range files { + // Make sure filename is a plain filename, not a path. + // This also ensures the staging directory cannot be escaped. + if filename != filepath.Base(filename) { + return fmt.Errorf("filename cannot be a path: %q", filename) + } + + fullFilename := filepath.Join(stagingDir, filename) + klog.Infof("Writing file %q ...", fullFilename) + + if err := fs.WriteFileFsync(fullFilename, file.Content, file.Perm); err != nil { + return fmt.Errorf("failed writing %q: %w", fullFilename, err) + } + } + + klog.Infof("Atomically swapping staging directory %q with target directory %q ...", stagingDir, targetDir) + if err := fs.SwapDirectories(targetDir, stagingDir); err != nil { + return fmt.Errorf("failed swapping target directory %q with staging directory %q: %w", targetDir, stagingDir, err) + } + + // fsync parent directories to ensure the swap is durable on disk. + // renameat2(RENAME_EXCHANGE) modifies parent directory entries, so the + // parents must be fsynced to persist which inode each name points to. + if err := fs.Fsync(filepath.Dir(targetDir)); err != nil { + return fmt.Errorf("failed syncing parent directory of %q: %w", targetDir, err) + } + if err := fs.Fsync(filepath.Dir(stagingDir)); err != nil { + return fmt.Errorf("failed syncing parent directory of %q: %w", stagingDir, err) + } + + return +} diff --git a/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go new file mode 100644 index 0000000000..24f1a9542a --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/sync_linux_test.go @@ -0,0 +1,450 @@ +//go:build linux + +package atomicdir + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + adtesting "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/testing" + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) + +func TestSync(t *testing.T) { + newRealFS := func() *fileSystem { + fs := realFS + return &fs + } + + type testCase struct { + name string + // newFS is the main mocking factory for the test run. + newFS func() *fileSystem + // existingFiles is used to populate the target directory state before testing. + // An empty map will cause the directory to be created, a nil map will cause no directory to be created. + existingFiles map[string]types.File + // filesToSync will be synchronized into the target directory. + filesToSync map[string]types.File + // expectedFiles contains the files that are expected to be in the target directory after sync is called. + expectedFiles map[string]types.File + // expectSyncError check the return value from Sync. + expectSyncError bool + // expectLingeringStagingDirectory can be set to true to expect the temporary directory not to be removed. + expectLingeringStagingDirectory bool + } + + errorTestCase := func(name string, newFS func() *fileSystem) testCase { + return testCase{ + name: name, + newFS: newFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectSyncError: true, + } + } + + testCases := []testCase{ + { + name: "target directory does not exist", + newFS: newRealFS, + existingFiles: nil, + filesToSync: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + }, + { + name: "target directory is empty", + newFS: newRealFS, + existingFiles: map[string]types.File{}, + filesToSync: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + }, + { + name: "target directory already synchronized", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + }, + { + name: "change file contents preserving the filenames", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + }, + { + name: "change filenames preserving the file contents", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + }, + { + name: "change filenames and file contents", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + }, + { + name: "replace a single file content", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("3"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("3"), Perm: 0600}, + }, + }, + { + name: "replace a single file", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("3"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("3"), Perm: 0600}, + }, + }, + { + name: "rename a single file", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("2"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "3.txt": {Content: []byte("2"), Perm: 0600}, + }, + }, + { + name: "add new files", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, + "another_tls.key": {Content: []byte("another TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + "another_tls.crt": {Content: []byte("another TLS cert"), Perm: 0600}, + "another_tls.key": {Content: []byte("another TLS key"), Perm: 0600}, + }, + }, + { + name: "delete a single file", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + }, + }, + { + name: "delete all files", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "1.txt": {Content: []byte("1"), Perm: 0600}, + "2.txt": {Content: []byte("2"), Perm: 0600}, + }, + filesToSync: map[string]types.File{}, + expectedFiles: map[string]types.File{}, + }, + errorTestCase("directory unchanged on failed to create object directory", func() *fileSystem { + fs := newRealFS() + mkdirAll := fs.MkdirAll + fs.MkdirAll = func(path string, perm os.FileMode) error { + // Fail on the content dir. + if !strings.Contains(path, "/staging/") { + return errors.New("nuked") + } + return mkdirAll(path, perm) + } + return fs + }), + errorTestCase("directory unchanged on failed to create staging directory", func() *fileSystem { + fs := newRealFS() + mkdirAll := fs.MkdirAll + fs.MkdirAll = func(path string, perm os.FileMode) error { + // Fail on the staging dir. + if strings.Contains(path, "/staging/") { + return errors.New("nuked") + } + return mkdirAll(path, perm) + } + return fs + }), + errorTestCase("directory unchanged on failed to write the first file", func() *fileSystem { + fs := newRealFS() + fs.WriteFileFsync = failToWriteNth(fs.WriteFileFsync, 0) + return fs + }), + errorTestCase("directory unchanged on failed to write the second file", func() *fileSystem { + fs := newRealFS() + fs.WriteFileFsync = failToWriteNth(fs.WriteFileFsync, 1) + return fs + }), + errorTestCase("directory unchanged on failed to swap directories", func() *fileSystem { + fs := newRealFS() + fs.SwapDirectories = func(dirA, dirB string) error { + return errors.New("nuked") + } + return fs + }), + { + name: "directory synchronized on failed to sync parent of target directory", + newFS: func() *fileSystem { + fs := newRealFS() + origFsync := realFS.Fsync + var fsyncCount int + fs.Fsync = func(name string) error { + fsyncCount++ + if fsyncCount == 1 { + return errors.New("nuked") + } + return origFsync(name) + } + return fs + }, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, + { + name: "directory synchronized on failed to sync parent of staging directory", + newFS: func() *fileSystem { + fs := newRealFS() + origFsync := realFS.Fsync + var fsyncCount int + fs.Fsync = func(name string) error { + fsyncCount++ + if fsyncCount == 2 { + return errors.New("nuked") + } + return origFsync(name) + } + return fs + }, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, + { + name: "directory synchronized then failing to remove temporary directory", + newFS: func() *fileSystem { + fs := newRealFS() + fs.RemoveAll = func(path string) error { + return errors.New("nuked") + } + return fs + }, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectSyncError: true, + expectLingeringStagingDirectory: true, + }, + { + name: "invalid filename specified (relative path)", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + // This fails even though the actual resolved path is just "api.crt". + // We simply do not handle paths in any way, we expect filenames. + "home/../api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, + { + name: "invalid filename specified (absolute path)", + newFS: newRealFS, + existingFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + filesToSync: map[string]types.File{ + "/api.crt": {Content: []byte("rotated TLS cert"), Perm: 0600}, + "api.key": {Content: []byte("rotated TLS key"), Perm: 0600}, + }, + expectedFiles: map[string]types.File{ + "tls.crt": {Content: []byte("TLS cert"), Perm: 0600}, + "tls.key": {Content: []byte("TLS key"), Perm: 0600}, + }, + expectSyncError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + // Write the current directory contents. + contentDir := filepath.Join(t.TempDir(), "secrets", "tls-cert") + if tc.existingFiles != nil { + adtesting.DirectoryState(tc.existingFiles).Write(t, contentDir, 0755) + } + + // Replace with the object data. + stagingDir := filepath.Join(t.TempDir(), "staging", "secrets", "tls-cert") + err := sync(tc.newFS(), contentDir, 0755, stagingDir, tc.filesToSync) + + // Check the resulting state. + adtesting.DirectoryState(tc.expectedFiles).CheckDirectoryMatches(t, contentDir, 0755) + + if (err != nil) != tc.expectSyncError { + t.Errorf("Expected error from sync = %v, got %v", tc.expectSyncError, err) + } + + if !tc.expectLingeringStagingDirectory { + // Note that staging/secrets is still there, though. Which is fine. + ensureDirectoryNotFound(t, stagingDir) + } + }) + } +} + +type writeFileFunc func(path string, data []byte, perm os.FileMode) error + +func failToWriteNth(writeFile writeFileFunc, n int) writeFileFunc { + var c int + return func(path string, data []byte, perm os.FileMode) error { + i := c + c++ + if i == n { + return errors.New("nuked") + } + return writeFile(path, data, perm) + } +} + +func ensureDirectoryNotFound(t *testing.T, path string) { + if _, stat := os.Stat(path); !os.IsNotExist(stat) { + t.Errorf("Directory %q should not exist", path) + } +} diff --git a/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go b/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go new file mode 100644 index 0000000000..9312157383 --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/testing/directory_state.go @@ -0,0 +1,74 @@ +package testing + +import ( + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + + "github.com/openshift/library-go/pkg/operator/staticpod/internal/atomicdir/types" +) + +// DirectoryState can be used both bootstrap and check a directory state. +type DirectoryState map[string]types.File + +// Write writes the given state into the given directory. +func (dir DirectoryState) Write(tb testing.TB, dirPath string, dirPerm os.FileMode) { + if err := os.MkdirAll(dirPath, dirPerm); err != nil { + tb.Fatalf("Failed to create directory %q: %v", dirPath, err) + } + + for filename, state := range dir { + fullFilename := filepath.Join(dirPath, filename) + if err := os.WriteFile(fullFilename, state.Content, state.Perm); err != nil { + tb.Fatalf("Failed to write file %q: %v", fullFilename, err) + } + } +} + +// CheckDirectoryMatches checks the given directory against the given state. +func (dir DirectoryState) CheckDirectoryMatches(tb testing.TB, dirPath string, dirPerm os.FileMode) { + // Ensure the directory permissions match. + stat, err := os.Stat(dirPath) + if err != nil { + tb.Fatalf("Failed to stat %q: %v", dirPath, err) + } + if perm := stat.Mode().Perm(); perm != dirPerm { + tb.Errorf("Permissions mismatch detected for %q: expected %v, got %v", dirPath, dirPerm, perm) + } + + // Ensure all files are in sync. + entries, err := os.ReadDir(dirPath) + if err != nil { + tb.Fatalf("Failed to read directory %q: %v", dirPath, err) + } + + actualState := make(DirectoryState, len(entries)) + for _, entry := range entries { + filePath := filepath.Join(dirPath, entry.Name()) + + info, err := entry.Info() + if err != nil { + tb.Fatalf("Failed to stat %q: %v", filePath, err) + } + + if info.IsDir() { + tb.Errorf("Unexpected directory detected: %q", filePath) + continue + } + + content, err := os.ReadFile(filePath) + if err != nil { + tb.Fatalf("Failed to read %q: %v", filePath, err) + } + + actualState[entry.Name()] = types.File{ + Content: content, + Perm: info.Mode(), + } + } + if !cmp.Equal(dir, actualState) { + tb.Errorf("Unexpected directory content for %q:\n%s\n", dirPath, cmp.Diff(dir, actualState)) + } +} diff --git a/pkg/operator/staticpod/internal/atomicdir/types/file.go b/pkg/operator/staticpod/internal/atomicdir/types/file.go new file mode 100644 index 0000000000..f99f41c57c --- /dev/null +++ b/pkg/operator/staticpod/internal/atomicdir/types/file.go @@ -0,0 +1,10 @@ +// Package types exists to avoid import cycles as it's imported by both atomicdir and atomicdir/testing. +package types + +import "os" + +// File represents file content together with the associated permissions. +type File struct { + Content []byte + Perm os.FileMode +} diff --git a/pkg/operator/staticpod/internal/fsutil/fsutil.go b/pkg/operator/staticpod/internal/fsutil/fsutil.go new file mode 100644 index 0000000000..5b205f52db --- /dev/null +++ b/pkg/operator/staticpod/internal/fsutil/fsutil.go @@ -0,0 +1,33 @@ +package fsutil + +import ( + "os" + "path/filepath" +) + +// Fsync fsyncs a file or directory to ensure it is durable on disk. +func Fsync(name string) error { + f, err := os.Open(name) + if err != nil { + return err + } + syncErr := f.Sync() + // close(2) can surface errors not caught by fsync(2), e.g. on NFS. + closeErr := f.Close() + if syncErr != nil { + return syncErr + } + return closeErr +} + +// WriteFileFsync writes data to a file and fsyncs both the file and its +// parent directory to ensure the write is durable on disk. +func WriteFileFsync(name string, data []byte, perm os.FileMode) error { + if err := os.WriteFile(name, data, perm); err != nil { + return err + } + if err := Fsync(name); err != nil { + return err + } + return Fsync(filepath.Dir(name)) +} diff --git a/pkg/operator/staticpod/internal/fsutil/fsutil_test.go b/pkg/operator/staticpod/internal/fsutil/fsutil_test.go new file mode 100644 index 0000000000..f21d3b91d1 --- /dev/null +++ b/pkg/operator/staticpod/internal/fsutil/fsutil_test.go @@ -0,0 +1,113 @@ +package fsutil + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteFileFsync(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T) string + data []byte + perm os.FileMode + expectError bool + }{ + { + name: "writes file with correct content and permissions", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "test.txt") + }, + data: []byte("hello world"), + perm: 0600, + }, + { + name: "creates file in nonexistent directory fails", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "nodir", "test.txt") + }, + data: []byte("hello"), + perm: 0600, + expectError: true, + }, + { + name: "overwrites existing file", + setup: func(t *testing.T) string { + p := filepath.Join(t.TempDir(), "test.txt") + if err := os.WriteFile(p, []byte("old content"), 0600); err != nil { + t.Fatal(err) + } + return p + }, + data: []byte("new content"), + perm: 0600, + }, + { + name: "writes empty file", + setup: func(t *testing.T) string { + return filepath.Join(t.TempDir(), "empty.txt") + }, + data: []byte{}, + perm: 0644, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + path := tc.setup(t) + err := WriteFileFsync(path, tc.data, tc.perm) + + if tc.expectError { + if err == nil { + t.Fatal("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read back file: %v", err) + } + if string(got) != string(tc.data) { + t.Errorf("content mismatch: got %q, want %q", got, tc.data) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatalf("failed to stat file: %v", err) + } + if info.Mode().Perm() != tc.perm { + t.Errorf("permission mismatch: got %o, want %o", info.Mode().Perm(), tc.perm) + } + }) + } +} + +func TestFsync(t *testing.T) { + t.Run("syncs existing file", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "test.txt") + if err := os.WriteFile(path, []byte("data"), 0600); err != nil { + t.Fatal(err) + } + if err := Fsync(path); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("syncs existing directory", func(t *testing.T) { + dir := t.TempDir() + if err := Fsync(dir); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("fails on nonexistent path", func(t *testing.T) { + if err := Fsync(filepath.Join(t.TempDir(), "nonexistent")); err == nil { + t.Fatal("expected error, got nil") + } + }) +}