OCPBUGS-49683: CA cert and catalog cache bug fixes

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 (
@@ -97,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')")
@@ -115,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 <namespace>/<name> of the global pull secret that is going to be used to pull bundle images.")
 
 	klog.InitFlags(flag.CommandLine)
@@ -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)
 	}
 
@@ -271,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")

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"

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

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"
 )
 
@@ -101,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,
@@ -283,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)
@@ -297,12 +300,17 @@ 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) {
 			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")
@@ -361,7 +369,7 @@ func main() {
 		crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
 	}
 
-	applier := &applier.Helm{
+	helmApplier := &applier.Helm{
 		ActionClientGetter: acg,
 		Preflights:         preflights,
 	}
@@ -381,7 +389,7 @@ func main() {
 		Client:                cl,
 		Resolver:              resolver,
 		Unpacker:              unpacker,
-		Applier:               applier,
+		Applier:               helmApplier,
 		InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
 		Finalizers:            clusterExtensionFinalizers,
 		Manager:               cm,

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"

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{

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)