From b3c4f0a717b580d43e74914c9bf661fe10fce0a3 Mon Sep 17 00:00:00 2001 From: oceanc80 Date: Thu, 30 Jan 2025 12:34:52 -0500 Subject: [PATCH 1/4] UPSTREAM: : Clear cache on startup, use tempDir for unpacking (#1669) Signed-off-by: Per Goncalves da Silva --- catalogd/cmd/catalogd/main.go | 5 +- catalogd/internal/source/containers_image.go | 81 +++-------------- cmd/operator-controller/main.go | 10 ++- internal/rukpak/source/containers_image.go | 79 +++-------------- .../rukpak/source/containers_image_test.go | 11 ++- internal/rukpak/source/util.go | 86 +++++++++++++++++++ internal/util/fs.go | 23 +++++ 7 files changed, 155 insertions(+), 140 deletions(-) create mode 100644 internal/rukpak/source/util.go create mode 100644 internal/util/fs.go diff --git a/catalogd/cmd/catalogd/main.go b/catalogd/cmd/catalogd/main.go index 35854aeae..45baed165 100644 --- a/catalogd/cmd/catalogd/main.go +++ b/catalogd/cmd/catalogd/main.go @@ -63,6 +63,7 @@ import ( "github.com/operator-framework/operator-controller/catalogd/internal/storage" "github.com/operator-framework/operator-controller/catalogd/internal/version" "github.com/operator-framework/operator-controller/catalogd/internal/webhook" + "github.com/operator-framework/operator-controller/internal/util" ) var ( @@ -257,8 +258,8 @@ func main() { systemNamespace = podNamespace() } - if err := os.MkdirAll(cacheDir, 0700); err != nil { - setupLog.Error(err, "unable to create cache directory") + if err := util.EnsureEmptyDirectory(cacheDir, 0700); err != nil { + setupLog.Error(err, "unable to ensure empty cache directory") os.Exit(1) } diff --git a/catalogd/internal/source/containers_image.go b/catalogd/internal/source/containers_image.go index c00db5c0f..d67221efc 100644 --- a/catalogd/internal/source/containers_image.go +++ b/catalogd/internal/source/containers_image.go @@ -29,6 +29,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" catalogdv1 "github.com/operator-framework/operator-controller/catalogd/api/v1" + "github.com/operator-framework/operator-controller/internal/rukpak/source" + "github.com/operator-framework/operator-controller/internal/util" ) const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1" @@ -70,12 +72,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv // ////////////////////////////////////////////////////// unpackPath := i.unpackPath(catalog.Name, canonicalRef.Digest()) - if unpackStat, err := os.Stat(unpackPath); err == nil { - if !unpackStat.IsDir() { - panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath)) - } + if isUnpacked, unpackTime, err := source.IsImageUnpacked(unpackPath); isUnpacked && err == nil { l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String()) - return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil + return successResult(unpackPath, canonicalRef, unpackTime), nil + } else if err != nil { + return nil, fmt.Errorf("error checking image already unpacked: %w", err) } ////////////////////////////////////////////////////// @@ -148,7 +149,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv // ////////////////////////////////////////////////////// if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil { - if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil { + if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil { err = errors.Join(err, cleanupErr) } return nil, fmt.Errorf("error unpacking image: %w", err) @@ -189,7 +190,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa } func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error { - if err := deleteRecursive(i.catalogPath(catalog.Name)); err != nil { + if err := source.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil { return fmt.Errorf("error deleting catalog cache: %w", err) } return nil @@ -288,8 +289,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st return wrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical) } - if err := os.MkdirAll(unpackPath, 0700); err != nil { - return fmt.Errorf("error creating unpack directory: %w", err) + if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil { + return fmt.Errorf("error ensuring empty unpack directory: %w", err) } l := log.FromContext(ctx) l.Info("unpacking image", "path", unpackPath) @@ -307,10 +308,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st l.Info("applied layer", "layer", i) return nil }(); err != nil { - return errors.Join(err, deleteRecursive(unpackPath)) + return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath)) } } - if err := setReadOnlyRecursive(unpackPath); err != nil { + if err := source.SetReadOnlyRecursive(unpackPath); err != nil { return fmt.Errorf("error making unpack directory read-only: %w", err) } return nil @@ -354,69 +355,13 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo continue } imgDirPath := filepath.Join(catalogPath, imgDir.Name()) - if err := deleteRecursive(imgDirPath); err != nil { + if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil { return fmt.Errorf("error removing image directory: %w", err) } } return nil } -func setReadOnlyRecursive(root string) error { - if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - - fi, err := d.Info() - if err != nil { - return err - } - - if err := func() error { - switch typ := fi.Mode().Type(); typ { - case os.ModeSymlink: - // do not follow symlinks - // 1. if they resolve to other locations in the root, we'll find them anyway - // 2. if they resolve to other locations outside the root, we don't want to change their permissions - return nil - case os.ModeDir: - return os.Chmod(path, 0500) - case 0: // regular file - return os.Chmod(path, 0400) - default: - return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String()) - } - }(); err != nil { - return err - } - return nil - }); err != nil { - return fmt.Errorf("error making catalog cache read-only: %w", err) - } - return nil -} - -func deleteRecursive(root string) error { - if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if os.IsNotExist(err) { - return nil - } - if err != nil { - return err - } - if !d.IsDir() { - return nil - } - if err := os.Chmod(path, 0700); err != nil { - return err - } - return nil - }); err != nil { - return fmt.Errorf("error making catalog cache writable for deletion: %w", err) - } - return os.RemoveAll(root) -} - func wrapTerminal(err error, isTerminal bool) error { if !isTerminal { return err diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index b7b8551a4..3bda706aa 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -68,6 +68,7 @@ import ( "github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety" "github.com/operator-framework/operator-controller/internal/rukpak/source" "github.com/operator-framework/operator-controller/internal/scheme" + "github.com/operator-framework/operator-controller/internal/util" "github.com/operator-framework/operator-controller/internal/version" ) @@ -297,6 +298,11 @@ func main() { } } + if err := util.EnsureEmptyDirectory(cachePath, 0700); err != nil { + setupLog.Error(err, "unable to ensure empty cache directory") + os.Exit(1) + } + unpacker := &source.ContainersImageRegistry{ BaseCachePath: filepath.Join(cachePath, "unpack"), SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) { @@ -361,7 +367,7 @@ func main() { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } - applier := &applier.Helm{ + helmApplier := &applier.Helm{ ActionClientGetter: acg, Preflights: preflights, } @@ -381,7 +387,7 @@ func main() { Client: cl, Resolver: resolver, Unpacker: unpacker, - Applier: applier, + Applier: helmApplier, InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg}, Finalizers: clusterExtensionFinalizers, Manager: cm, diff --git a/internal/rukpak/source/containers_image.go b/internal/rukpak/source/containers_image.go index 22f072da2..12a822a2b 100644 --- a/internal/rukpak/source/containers_image.go +++ b/internal/rukpak/source/containers_image.go @@ -23,6 +23,8 @@ import ( "github.com/opencontainers/go-digest" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/operator-framework/operator-controller/internal/util" ) type ContainersImageRegistry struct { @@ -62,12 +64,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour // ////////////////////////////////////////////////////// unpackPath := i.unpackPath(bundle.Name, canonicalRef.Digest()) - if unpackStat, err := os.Stat(unpackPath); err == nil { - if !unpackStat.IsDir() { - panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath)) - } + if isUnpacked, _, err := IsImageUnpacked(unpackPath); isUnpacked && err == nil { l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String()) return successResult(bundle.Name, unpackPath, canonicalRef), nil + } else if err != nil { + return nil, fmt.Errorf("error checking bundle already unpacked: %w", err) } ////////////////////////////////////////////////////// @@ -140,7 +141,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour // ////////////////////////////////////////////////////// if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil { - if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil { + if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil { err = errors.Join(err, cleanupErr) } return nil, fmt.Errorf("error unpacking image: %w", err) @@ -168,7 +169,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic } func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error { - return deleteRecursive(i.bundlePath(bundle.Name)) + return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name)) } func (i *ContainersImageRegistry) bundlePath(bundleName string) string { @@ -251,8 +252,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st return fmt.Errorf("error creating image source: %w", err) } - if err := os.MkdirAll(unpackPath, 0700); err != nil { - return fmt.Errorf("error creating unpack directory: %w", err) + if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil { + return fmt.Errorf("error ensuring empty unpack directory: %w", err) } l := log.FromContext(ctx) l.Info("unpacking image", "path", unpackPath) @@ -270,10 +271,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st l.Info("applied layer", "layer", i) return nil }(); err != nil { - return errors.Join(err, deleteRecursive(unpackPath)) + return errors.Join(err, DeleteReadOnlyRecursive(unpackPath)) } } - if err := setReadOnlyRecursive(unpackPath); err != nil { + if err := SetReadOnlyRecursive(unpackPath); err != nil { return fmt.Errorf("error making unpack directory read-only: %w", err) } return nil @@ -310,65 +311,9 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK continue } imgDirPath := filepath.Join(bundlePath, imgDir.Name()) - if err := deleteRecursive(imgDirPath); err != nil { + if err := DeleteReadOnlyRecursive(imgDirPath); err != nil { return fmt.Errorf("error removing image directory: %w", err) } } return nil } - -func setReadOnlyRecursive(root string) error { - if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if err != nil { - return err - } - - fi, err := d.Info() - if err != nil { - return err - } - - if err := func() error { - switch typ := fi.Mode().Type(); typ { - case os.ModeSymlink: - // do not follow symlinks - // 1. if they resolve to other locations in the root, we'll find them anyway - // 2. if they resolve to other locations outside the root, we don't want to change their permissions - return nil - case os.ModeDir: - return os.Chmod(path, 0500) - case 0: // regular file - return os.Chmod(path, 0400) - default: - return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String()) - } - }(); err != nil { - return err - } - return nil - }); err != nil { - return fmt.Errorf("error making bundle cache read-only: %w", err) - } - return nil -} - -func deleteRecursive(root string) error { - if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { - if os.IsNotExist(err) { - return nil - } - if err != nil { - return err - } - if !d.IsDir() { - return nil - } - if err := os.Chmod(path, 0700); err != nil { - return err - } - return nil - }); err != nil { - return fmt.Errorf("error making bundle cache writable for deletion: %w", err) - } - return os.RemoveAll(root) -} diff --git a/internal/rukpak/source/containers_image_test.go b/internal/rukpak/source/containers_image_test.go index ea7a69832..29f2788c6 100644 --- a/internal/rukpak/source/containers_image_test.go +++ b/internal/rukpak/source/containers_image_test.go @@ -277,7 +277,16 @@ func TestUnpackUnexpectedFile(t *testing.T) { require.NoError(t, os.WriteFile(unpackPath, []byte{}, 0600)) // Attempt to pull and unpack the image - assert.Panics(t, func() { _, _ = unpacker.Unpack(context.Background(), bundleSource) }) + _, err := unpacker.Unpack(context.Background(), bundleSource) + require.NoError(t, err) + + // Ensure unpack path is now a directory + stat, err := os.Stat(unpackPath) + require.NoError(t, err) + require.True(t, stat.IsDir()) + + // Unset read-only to allow cleanup + require.NoError(t, source.UnsetReadOnlyRecursive(unpackPath)) } func TestUnpackCopySucceedsMountFails(t *testing.T) { diff --git a/internal/rukpak/source/util.go b/internal/rukpak/source/util.go new file mode 100644 index 000000000..ca9aa9c2b --- /dev/null +++ b/internal/rukpak/source/util.go @@ -0,0 +1,86 @@ +package source + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "time" +) + +// SetReadOnlyRecursive sets directory with path given by `root` as read-only +func SetReadOnlyRecursive(root string) error { + return filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + + fi, err := d.Info() + if err != nil { + return err + } + + if err := func() error { + switch typ := fi.Mode().Type(); typ { + case os.ModeSymlink: + // do not follow symlinks + // 1. if they resolve to other locations in the root, we'll find them anyway + // 2. if they resolve to other locations outside the root, we don't want to change their permissions + return nil + case os.ModeDir: + return os.Chmod(path, 0500) + case 0: // regular file + return os.Chmod(path, 0400) + default: + return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String()) + } + }(); err != nil { + return err + } + return nil + }) +} + +// UnsetReadOnlyRecursive unsets directory with path given by `root` as read-only +func UnsetReadOnlyRecursive(root string) error { + return filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if os.IsNotExist(err) { + return nil + } + if err != nil { + return err + } + if !d.IsDir() { + return nil + } + if err := os.Chmod(path, 0700); err != nil { + return err + } + return nil + }) +} + +// DeleteReadOnlyRecursive deletes read-only directory with path given by `root` +func DeleteReadOnlyRecursive(root string) error { + if err := UnsetReadOnlyRecursive(root); err != nil { + return fmt.Errorf("error making directory writable for deletion: %w", err) + } + return os.RemoveAll(root) +} + +// IsImageUnpacked checks whether an image has been unpacked in `unpackPath`. +// If true, time of unpack will also be returned. If false unpack time is gibberish (zero/epoch time). +// If `unpackPath` is a file, it will be deleted and false will be returned without an error. +func IsImageUnpacked(unpackPath string) (bool, time.Time, error) { + unpackStat, err := os.Stat(unpackPath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, time.Time{}, nil + } + return false, time.Time{}, err + } + if !unpackStat.IsDir() { + return false, time.Time{}, os.Remove(unpackPath) + } + return true, unpackStat.ModTime(), nil +} diff --git a/internal/util/fs.go b/internal/util/fs.go new file mode 100644 index 000000000..137b0735d --- /dev/null +++ b/internal/util/fs.go @@ -0,0 +1,23 @@ +package util + +import ( + "io/fs" + "os" + "path/filepath" +) + +// EnsureEmptyDirectory ensures the directory given by `path` is empty. +// If the directory does not exist, it will be created with permission bits +// given by `perm`. +func EnsureEmptyDirectory(path string, perm fs.FileMode) error { + entries, err := os.ReadDir(path) + if err != nil && !os.IsNotExist(err) { + return err + } + for _, entry := range entries { + if err := os.RemoveAll(filepath.Join(path, entry.Name())); err != nil { + return err + } + } + return os.MkdirAll(path, perm) +} From 5375d60688a48a910590fbdb938978dcede8f716 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Thu, 30 Jan 2025 14:51:51 -0500 Subject: [PATCH 2/4] UPSTREAM: : use projected volume for CAs to avoid subPath limitations Signed-off-by: Joe Lanford --- .../patches/manager_deployment_certs.yaml | 14 ++----- ...t-catalogd-catalogd-controller-manager.yml | 42 +++++++++---------- .../patches/manager_deployment_certs.yaml | 14 ++----- ...operator-controller-controller-manager.yml | 42 +++++++++---------- 4 files changed, 50 insertions(+), 62 deletions(-) diff --git a/openshift/catalogd/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml b/openshift/catalogd/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml index 2a8207da6..540e545a8 100644 --- a/openshift/catalogd/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml +++ b/openshift/catalogd/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml @@ -3,19 +3,13 @@ value: {"name":"catalogserver-certs", "secret":{"optional":false,"secretName":"catalogserver-cert"}} - op: add path: /spec/template/spec/volumes/- - value: {"name":"trusted-ca-bundle", "configMap":{"optional":false,"name":"trusted-ca-bundle", "items":[{"key":"ca-bundle.crt","path":"ca-bundle.crt"}]}} -- op: add - path: /spec/template/spec/volumes/- - value: {"name":"service-ca", "configMap":{"optional":false,"name":"openshift-service-ca.crt", "items":[{"key":"service-ca.crt","path":"service-ca.crt"}]}} + value: {"name":"ca-certs", "projected": {"sources":[{"configMap":{"optional":false,"name":"trusted-ca-bundle", "items":[{"key":"ca-bundle.crt","path":"ca-bundle.crt"}]}},{"configMap":{"optional":false,"name":"openshift-service-ca.crt", "items":[{"key":"service-ca.crt","path":"service-ca.crt"}]}}]}} - op: add path: /spec/template/spec/containers/0/volumeMounts/- value: {"name":"catalogserver-certs", "mountPath":"/var/certs"} - op: add path: /spec/template/spec/containers/0/volumeMounts/- - value: {"name":"trusted-ca-bundle", "mountPath":"/var/trusted-cas/ca-bundle.crt", "subPath":"ca-bundle.crt"} -- op: add - path: /spec/template/spec/containers/0/volumeMounts/- - value: {"name":"service-ca", "mountPath":"/var/trusted-cas/service-ca.crt", "subPath":"service-ca.crt"} + value: {"name":"ca-certs", "mountPath":"/var/ca-certs", "readOnly": true} - op: add path: /spec/template/spec/containers/0/args/- value: "--tls-cert=/var/certs/tls.crt" @@ -23,5 +17,5 @@ path: /spec/template/spec/containers/0/args/- value: "--tls-key=/var/certs/tls.key" - op: add - path: /spec/template/spec/containers/0/args/- - value: "--ca-certs-dir=/var/trusted-cas" + path: /spec/template/spec/containers/0/env + value: [{"name":"SSL_CERT_DIR", "value":"/var/ca-certs"}] diff --git a/openshift/catalogd/manifests/14-deployment-openshift-catalogd-catalogd-controller-manager.yml b/openshift/catalogd/manifests/14-deployment-openshift-catalogd-catalogd-controller-manager.yml index f2297bc3c..6d22a31a7 100644 --- a/openshift/catalogd/manifests/14-deployment-openshift-catalogd-catalogd-controller-manager.yml +++ b/openshift/catalogd/manifests/14-deployment-openshift-catalogd-catalogd-controller-manager.yml @@ -46,11 +46,13 @@ spec: - --external-address=catalogd-service.openshift-catalogd.svc - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key - - --ca-certs-dir=/var/trusted-cas - --v=${LOG_VERBOSITY} - --global-pull-secret=openshift-config/pull-secret command: - ./catalogd + env: + - name: SSL_CERT_DIR + value: /var/ca-certs image: ${CATALOGD_IMAGE} imagePullPolicy: IfNotPresent livenessProbe: @@ -81,12 +83,9 @@ spec: name: cache - mountPath: /var/certs name: catalogserver-certs - - mountPath: /var/trusted-cas/ca-bundle.crt - name: trusted-ca-bundle - subPath: ca-bundle.crt - - mountPath: /var/trusted-cas/service-ca.crt - name: service-ca - subPath: service-ca.crt + - mountPath: /var/ca-certs + name: ca-certs + readOnly: true - mountPath: /etc/containers name: etc-containers readOnly: true @@ -121,20 +120,21 @@ spec: secret: optional: false secretName: catalogserver-cert - - configMap: - items: - - key: ca-bundle.crt - path: ca-bundle.crt - name: catalogd-trusted-ca-bundle - optional: false - name: trusted-ca-bundle - - configMap: - items: - - key: service-ca.crt - path: service-ca.crt - name: openshift-service-ca.crt - optional: false - name: service-ca + - name: ca-certs + projected: + sources: + - configMap: + items: + - key: ca-bundle.crt + path: ca-bundle.crt + name: catalogd-trusted-ca-bundle + optional: false + - configMap: + items: + - key: service-ca.crt + path: service-ca.crt + name: openshift-service-ca.crt + optional: false - hostPath: path: /etc/containers type: Directory diff --git a/openshift/operator-controller/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml b/openshift/operator-controller/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml index 4100ff569..874a496a6 100644 --- a/openshift/operator-controller/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml +++ b/openshift/operator-controller/kustomize/overlays/openshift/olmv1-ns/patches/manager_deployment_certs.yaml @@ -3,19 +3,13 @@ value: {"name":"operator-controller-certs", "secret":{"optional":false,"secretName":"operator-controller-cert"}} - op: add path: /spec/template/spec/volumes/- - value: {"name":"trusted-ca-bundle", "configMap":{"optional":false,"name":"trusted-ca-bundle", "items":[{"key":"ca-bundle.crt","path":"ca-bundle.crt"}]}} -- op: add - path: /spec/template/spec/volumes/- - value: {"name":"service-ca", "configMap":{"optional":false,"name":"openshift-service-ca.crt", "items":[{"key":"service-ca.crt","path":"service-ca.crt"}]}} + value: {"name":"ca-certs", "projected": {"sources":[{"configMap":{"optional":false,"name":"trusted-ca-bundle", "items":[{"key":"ca-bundle.crt","path":"ca-bundle.crt"}]}},{"configMap":{"optional":false,"name":"openshift-service-ca.crt", "items":[{"key":"service-ca.crt","path":"service-ca.crt"}]}}]}} - op: add path: /spec/template/spec/containers/0/volumeMounts/- value: {"name":"operator-controller-certs", "mountPath":"/var/certs"} - op: add path: /spec/template/spec/containers/0/volumeMounts/- - value: {"name":"trusted-ca-bundle", "mountPath":"/var/trusted-cas/ca-bundle.crt", "subPath":"ca-bundle.crt" } -- op: add - path: /spec/template/spec/containers/0/volumeMounts/- - value: {"name":"service-ca", "mountPath":"/var/trusted-cas/service-ca.crt", "subPath":"service-ca.crt" } + value: {"name":"ca-certs", "mountPath":"/var/ca-certs", "readOnly": true} - op: add path: /spec/template/spec/containers/0/args/- value: "--tls-cert=/var/certs/tls.crt" @@ -23,5 +17,5 @@ path: /spec/template/spec/containers/0/args/- value: "--tls-key=/var/certs/tls.key" - op: add - path: /spec/template/spec/containers/0/args/- - value: "--ca-certs-dir=/var/trusted-cas" + path: /spec/template/spec/containers/0/env + value: [{"name":"SSL_CERT_DIR", "value":"/var/ca-certs"}] diff --git a/openshift/operator-controller/manifests/20-deployment-openshift-operator-controller-operator-controller-controller-manager.yml b/openshift/operator-controller/manifests/20-deployment-openshift-operator-controller-operator-controller-controller-manager.yml index 3cb5f9ad0..1f407b2f9 100644 --- a/openshift/operator-controller/manifests/20-deployment-openshift-operator-controller-operator-controller-controller-manager.yml +++ b/openshift/operator-controller/manifests/20-deployment-openshift-operator-controller-operator-controller-controller-manager.yml @@ -45,11 +45,13 @@ spec: - --leader-elect - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key - - --ca-certs-dir=/var/trusted-cas - --v=${LOG_VERBOSITY} - --global-pull-secret=openshift-config/pull-secret command: - /operator-controller + env: + - name: SSL_CERT_DIR + value: /var/ca-certs image: ${OPERATOR_CONTROLLER_IMAGE} imagePullPolicy: IfNotPresent livenessProbe: @@ -80,12 +82,9 @@ spec: name: cache - mountPath: /var/certs name: operator-controller-certs - - mountPath: /var/trusted-cas/ca-bundle.crt - name: trusted-ca-bundle - subPath: ca-bundle.crt - - mountPath: /var/trusted-cas/service-ca.crt - name: service-ca - subPath: service-ca.crt + - mountPath: /var/ca-certs + name: ca-certs + readOnly: true - mountPath: /etc/containers name: etc-containers readOnly: true @@ -120,20 +119,21 @@ spec: secret: optional: false secretName: operator-controller-cert - - configMap: - items: - - key: ca-bundle.crt - path: ca-bundle.crt - name: operator-controller-trusted-ca-bundle - optional: false - name: trusted-ca-bundle - - configMap: - items: - - key: service-ca.crt - path: service-ca.crt - name: openshift-service-ca.crt - optional: false - name: service-ca + - name: ca-certs + projected: + sources: + - configMap: + items: + - key: ca-bundle.crt + path: ca-bundle.crt + name: operator-controller-trusted-ca-bundle + optional: false + - configMap: + items: + - key: service-ca.crt + path: service-ca.crt + name: openshift-service-ca.crt + optional: false - hostPath: path: /etc/containers type: Directory From fa64ffebe0e1b7b8eccaa8d01c796572849d30e9 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 30 Jan 2025 16:23:59 -0500 Subject: [PATCH 3/4] UPSTREAM: : Add support for SSL env vars to cert pool watcher (#1672) The SystemRoot store looks at the SSL_CERT_DIR and SSL_CERT_FILE environment variables for certificate locations. Because these variables are under control of the user, we should assume that the user wants to control the contents of the SystemRoot, and subsequently that those contents could change (as compared to certs located in the default /etc/pki location). Thus, we should watch those locations if they exist. Signed-off-by: Todd Short --- internal/httputil/certpoolwatcher.go | 24 +++++++++++++++++++++-- internal/httputil/certpoolwatcher_test.go | 4 ++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/internal/httputil/certpoolwatcher.go b/internal/httputil/certpoolwatcher.go index 2a250d069..0cce70312 100644 --- a/internal/httputil/certpoolwatcher.go +++ b/internal/httputil/certpoolwatcher.go @@ -4,6 +4,8 @@ import ( "crypto/x509" "fmt" "os" + "slices" + "strings" "sync" "time" @@ -44,8 +46,26 @@ func NewCertPoolWatcher(caDir string, log logr.Logger) (*CertPoolWatcher, error) if err != nil { return nil, err } - if err = watcher.Add(caDir); err != nil { - return nil, err + + // If the SSL_CERT_DIR or SSL_CERT_FILE environment variables are + // specified, this means that we have some control over the system root + // location, thus they may change, thus we should watch those locations. + watchPaths := strings.Split(os.Getenv("SSL_CERT_DIR"), ":") + watchPaths = append(watchPaths, caDir, os.Getenv("SSL_CERT_FILE")) + watchPaths = slices.DeleteFunc(watchPaths, func(p string) bool { + if p == "" { + return true + } + if _, err := os.Stat(p); err != nil { + return true + } + return false + }) + + for _, p := range watchPaths { + if err := watcher.Add(p); err != nil { + return nil, err + } } cpw := &CertPoolWatcher{ diff --git a/internal/httputil/certpoolwatcher_test.go b/internal/httputil/certpoolwatcher_test.go index bfebebd28..2ea3f862a 100644 --- a/internal/httputil/certpoolwatcher_test.go +++ b/internal/httputil/certpoolwatcher_test.go @@ -72,6 +72,10 @@ func TestCertPoolWatcher(t *testing.T) { t.Logf("Create cert file at %q\n", certName) createCert(t, certName) + // Update environment variables for the watcher - some of these should not exist + os.Setenv("SSL_CERT_DIR", tmpDir+":/tmp/does-not-exist.dir") + os.Setenv("SSL_CERT_FILE", "/tmp/does-not-exist.file") + // Create the cert pool watcher cpw, err := httputil.NewCertPoolWatcher(tmpDir, log.FromContext(context.Background())) require.NoError(t, err) From 9a301c12de6d82297ddfdbcbfd4d84a05ee4afae Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 30 Jan 2025 16:55:40 -0500 Subject: [PATCH 4/4] UPSTREAM: : Separate CA configuration for pulls vs catalogd services (#1673) Rename the flags that provide CAs to image pulling to indicate the use. Keep the old flag around (for backward compatibility), but prefer the new flag(s). Signed-off-by: Todd Short --- catalogd/cmd/catalogd/main.go | 8 ++++---- .../ca/patches/manager_deployment_cacerts.yaml | 2 +- cmd/operator-controller/main.go | 12 +++++++----- .../tls/patches/manager_deployment_cert.yaml | 5 ++++- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/catalogd/cmd/catalogd/main.go b/catalogd/cmd/catalogd/main.go index 45baed165..91d82bedd 100644 --- a/catalogd/cmd/catalogd/main.go +++ b/catalogd/cmd/catalogd/main.go @@ -98,7 +98,7 @@ func main() { certFile string keyFile string webhookPort int - caCertDir string + pullCasDir string globalPullSecret string ) flag.StringVar(&metricsAddr, "metrics-bind-address", "", "The address for the metrics endpoint. Requires tls-cert and tls-key. (Default: ':7443')") @@ -116,7 +116,7 @@ func main() { flag.StringVar(&certFile, "tls-cert", "", "The certificate file used for serving catalog and metrics. Required to enable the metrics server. Requires tls-key.") flag.StringVar(&keyFile, "tls-key", "", "The key file used for serving catalog contents and metrics. Required to enable the metrics server. Requires tls-cert.") flag.IntVar(&webhookPort, "webhook-server-port", 9443, "The port that the mutating webhook server serves at.") - flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of CA certificate to use for verifying HTTPS connections to image registries.") + flag.StringVar(&pullCasDir, "pull-cas-dir", "", "The directory of TLS certificate authoritiess to use for verifying HTTPS connections to image registries.") flag.StringVar(&globalPullSecret, "global-pull-secret", "", "The / of the global pull secret that is going to be used to pull bundle images.") klog.InitFlags(flag.CommandLine) @@ -272,8 +272,8 @@ func main() { BaseCachePath: unpackCacheBasePath, SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) { srcContext := &types.SystemContext{ - DockerCertPath: caCertDir, - OCICertPath: caCertDir, + DockerCertPath: pullCasDir, + OCICertPath: pullCasDir, } if _, err := os.Stat(authFilePath); err == nil && globalPullSecretKey != nil { logger.Info("using available authentication information for pulling image") diff --git a/catalogd/config/components/ca/patches/manager_deployment_cacerts.yaml b/catalogd/config/components/ca/patches/manager_deployment_cacerts.yaml index b5b03633e..6b0816706 100644 --- a/catalogd/config/components/ca/patches/manager_deployment_cacerts.yaml +++ b/catalogd/config/components/ca/patches/manager_deployment_cacerts.yaml @@ -6,4 +6,4 @@ value: {"name":"olmv1-certificate", "readOnly": true, "mountPath":"/var/ca-certs/"} - op: add path: /spec/template/spec/containers/0/args/- - value: "--ca-certs-dir=/var/ca-certs" + value: "--pull-cas-dir=/var/ca-certs" diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 3bda706aa..a1f1bc68d 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -102,12 +102,14 @@ func main() { cachePath string operatorControllerVersion bool systemNamespace string - caCertDir string + catalogdCasDir string + pullCasDir string globalPullSecret string ) flag.StringVar(&metricsAddr, "metrics-bind-address", "", "The address for the metrics endpoint. Requires tls-cert and tls-key. (Default: ':8443')") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") - flag.StringVar(&caCertDir, "ca-certs-dir", "", "The directory of TLS certificate to use for verifying HTTPS connections to the Catalogd and docker-registry web servers.") + flag.StringVar(&catalogdCasDir, "catalogd-cas-dir", "", "The directory of TLS certificate authorities to use for verifying HTTPS connections to the Catalogd web service.") + flag.StringVar(&pullCasDir, "pull-cas-dir", "", "The directory of TLS certificate authorities to use for verifying HTTPS connections to image registries.") flag.StringVar(&certFile, "tls-cert", "", "The certificate file used for the metrics server. Required to enable the metrics server. Requires tls-key.") flag.StringVar(&keyFile, "tls-key", "", "The key file used for the metrics server. Required to enable the metrics server. Requires tls-cert") flag.BoolVar(&enableLeaderElection, "leader-elect", false, @@ -284,7 +286,7 @@ func main() { os.Exit(1) } - certPoolWatcher, err := httputil.NewCertPoolWatcher(caCertDir, ctrl.Log.WithName("cert-pool")) + certPoolWatcher, err := httputil.NewCertPoolWatcher(catalogdCasDir, ctrl.Log.WithName("cert-pool")) if err != nil { setupLog.Error(err, "unable to create CA certificate pool") os.Exit(1) @@ -307,8 +309,8 @@ func main() { BaseCachePath: filepath.Join(cachePath, "unpack"), SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) { srcContext := &types.SystemContext{ - DockerCertPath: caCertDir, - OCICertPath: caCertDir, + DockerCertPath: pullCasDir, + OCICertPath: pullCasDir, } if _, err := os.Stat(authFilePath); err == nil && globalPullSecretKey != nil { logger.Info("using available authentication information for pulling image") diff --git a/config/components/tls/patches/manager_deployment_cert.yaml b/config/components/tls/patches/manager_deployment_cert.yaml index 18afac59d..8fbdb5592 100644 --- a/config/components/tls/patches/manager_deployment_cert.yaml +++ b/config/components/tls/patches/manager_deployment_cert.yaml @@ -6,7 +6,10 @@ value: {"name":"olmv1-certificate", "readOnly": true, "mountPath":"/var/certs/"} - op: add path: /spec/template/spec/containers/0/args/- - value: "--ca-certs-dir=/var/certs" + value: "--catalogd-cas-dir=/var/certs" +- op: add + path: /spec/template/spec/containers/0/args/- + value: "--pull-cas-dir=/var/certs" - op: add path: /spec/template/spec/containers/0/args/- value: "--tls-cert=/var/certs/tls.cert"