WiP: review of the whole codebase

cmd/deploy.go

@@ -225,16 +225,15 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("failed to set feature flags: %w", err)
 	}
 
-	// Resolve "auto" resources based on cluster type
-	resolvedResources := resources
+	// TODO(#91): validate the user-supplied value earlier than here
 	if resources == "auto" {
-		resolvedResources = resolveAutoResources(env.GetCurrentClusterType(), log)
+		resources = resolveAutoResources(env.GetCurrentClusterType(), log)
 	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
 	defer cancel()
 
-	if err := d.Deploy(ctx, components, resolvedResources, exposure); err != nil {
+	if err := d.Deploy(ctx, components, resources, exposure); err != nil {
 		return fmt.Errorf("deployment failed: %w", err)
 	}
 
@@ -256,6 +255,7 @@ func runDeploy(cmd *cobra.Command, args []string) error {
 
 // resolveAutoResources determines the appropriate resource tier based on cluster type
 func resolveAutoResources(clusterType env.ClusterType, log *logger.Logger) string {
+	// TODO(#91): should probably be a first-class type, not a free-form string...
 	var resolvedResources string
 
 	switch clusterType {

cmd/subshell.go

@@ -116,7 +116,7 @@ defaults
     timeout server  30s
 
 frontend http_front
-    bind *:8080
+    bind *:8080  # TODO(#91): this should probably be configurable?
     default_backend https_back
 
 backend https_back
@@ -131,6 +131,8 @@ backend https_back
 	configFile.Close()
 
 	cmd := exec.Command("haproxy", "-f", configPath)
+	// TODO(#91): What about stdin? We probably should prevent haproxy from reading from
+	// the terminal just in case...
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 

internal/clusterdefaults/clusterdefaults.go

@@ -34,6 +34,11 @@ func (ct ClusterType) String() string {
 	}
 }
 
+// TODO(#91): Maybe I'm missing something, but this manager/detector/applicator abstraction
+// seems massively over-engineered since the is only one concrete implementation. AFAICT this
+// could all be just a single function that the deployer calls with log, kubeconfig, resources,
+// exposure and portForward.
+
 // DeploymentDefaults holds the recommended defaults for a cluster type
 type DeploymentDefaults struct {
 	Resources          string // Resource preset (e.g., "small", "default")

internal/deployer/deploy_via_helm.go

@@ -447,6 +447,10 @@ func extractMainImageReferences(renderedYAML string) []string {
 	lines := strings.Split(renderedYAML, "\n")
 	for _, line := range lines {
 		// Look for lines like: image: "quay.io/stackrox-io/main:4.8.2"
+		// TODO(#91): this assumes image names will always be quoted with `"` and there will be
+		// a single space between `:` and `"`, we should make this more robust. It might be
+		// better to parse the yaml and walk the resulting map, returning string values of
+		// `image` keys...
 		if strings.Contains(line, `image: "`) {
 			parts := strings.SplitN(line, `image: "`, 2)
 			if len(parts) == 2 {

internal/deployer/deploy_via_operator.go

@@ -178,7 +178,7 @@ func (d *Deployer) getDeployedOperatorImage(ctx context.Context) (string, error)
 	return image, nil
 }
 
-// prepareNamespace creates pull secrets in the namespace
+// prepareNamespace creates pull secrets in the namespace if needed
 func (d *Deployer) prepareNamespace(ctx context.Context, namespace string) error {
 	d.logger.Infof("Preparing namespace %s", namespace)
 
@@ -443,6 +443,8 @@ func (d *Deployer) waitForCentralReady(ctx context.Context, timeout int) error {
 			}
 		}
 
+		// TODO(#91): using `kubectl wait` (which in turn - I hope - uses a watch) instead of
+		// polling would allow us to not waste time here
 		time.Sleep(checkInterval)
 	}
 
@@ -525,7 +527,9 @@ func (d *Deployer) fetchCentralCACert(ctx context.Context) error {
 		return fmt.Errorf("failed to decode CA cert: %w", err)
 	}
 
+	// TODO(#91): possible symlink race vulnerability
 	d.roxCACertFile = "/tmp/roxie-ca-cert.pem"
+	// TODO(#91): this is a REALLY weird way to write a temporary file in go
 	writeCmd := exec.CommandContext(ctx, "sh", "-c", fmt.Sprintf("cat > %s", d.roxCACertFile))
 	writeCmd.Stdin = bytes.NewReader(caCert)
 	if err := writeCmd.Run(); err != nil {
@@ -562,6 +566,7 @@ func (d *Deployer) configureCentralEndpoint(ctx context.Context, exposure string
 			return fmt.Errorf("failed to get LoadBalancer endpoint: %w", err)
 		}
 		// Remove https:// prefix if present (waitForLoadBalancer returns https://ip:443)
+		// TODO(#91): This is silly, why add these affixes there, and then strip here?!
 		d.centralEndpoint = strings.TrimPrefix(endpoint, "https://")
 		d.centralEndpoint = strings.TrimSuffix(d.centralEndpoint, ":443")
 		d.centralEndpoint = d.centralEndpoint + ":443"
@@ -784,6 +789,7 @@ func (d *Deployer) waitForSecuredClusterReady(ctx context.Context, timeout int)
 			}
 
 			// collector seems to be crashing on some local cluster types/versions.
+			// TODO(#91): skip the check only on local clusters, then?
 		}
 
 		if allReady {

internal/deployer/deployer.go

@@ -32,6 +32,9 @@ var (
 
 	pauseReconcileAnnotationKey = "stackrox.io/pause-reconcile"
 
+	// TODO(#91): at some point this will get out of date. If we filter by the app.../part-of
+	// label anyway, then maybe we should just delete all resource kinds present on cluster?
+	// also we should use the fully-qualified types
 	allInstallableCentralResourceKinds = []string{
 		"applications",
 		"clusterroles",
@@ -343,6 +346,7 @@ func (d *Deployer) SetCombinedOverrideFile(overrideFile string) error {
 func setOverrideSetExpressions(overrides map[string]interface{}, prefix string, overrideSetExpressions []string) ([]string, error) {
 	remainingSetExpressions := make([]string, 0)
 	for _, expr := range overrideSetExpressions {
+		// TODO(#91): would https://pkg.go.dev/strings#Cut work instead?
 		parts := splitAtFirstEquals(expr)
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("invalid override expression '%s': expected format 'key.path=value'", expr)
@@ -586,8 +590,8 @@ func (d *Deployer) Deploy(ctx context.Context, components component.Component, r
 	return nil
 }
 
-// prepareCredentials prepares and verifies Docker credentials early to fail fast.
-// The verified credentials are stored for later use.
+// prepareCredentials prepares and verifies Docker credentials early to allow failing fast.
+// The verified credentials are stored in the Deployer object for later use.
 func (d *Deployer) prepareCredentials() error {
 	d.logger.Dimf("Preparing and verifying Docker credentials...")
 
@@ -1251,6 +1255,8 @@ func (d *Deployer) checkPodProgressInNamespace(ctx context.Context, namespace st
 	}
 }
 
+// TODO(#91): plenty of code in common with the central variant that should probably be
+// extracted
 func (d *Deployer) PrintSecuredClusterDeploymentSummary() {
 	component := "Secured Cluster"
 	mainImageTag := d.mainImageTag

internal/deployer/kubectl.go

@@ -26,6 +26,8 @@ type KubectlResult struct {
 }
 
 // runKubectl executes a kubectl command with automatic retries on transient errors
+// TODO(#91): perhaps this should not return a pointer so that the lack of nil checks elsewhere
+// is robust?
 func (d *Deployer) runKubectl(ctx context.Context, opts KubectlOptions) (*KubectlResult, error) {
 	if opts.MaxAttempts <= 0 {
 		opts.MaxAttempts = 3

internal/deployer/operator.go

@@ -107,6 +107,8 @@ func (d *Deployer) identifyCRDFileNames(bundleDir string) ([]string, error) {
 			return nil
 		}
 
+		// TODO(#91): The following detection logic does not seem particularly robust. We should
+		// probably parse the YAML and check api group and kind fields.
 		name := strings.ToLower(info.Name())
 		if strings.Contains(name, "customresourcedefinition") ||
 			strings.Contains(name, "platform.stackrox.io") ||

internal/deployer/operator_olm.go

@@ -82,6 +82,9 @@ func (d *Deployer) checkOLMInstalled(ctx context.Context) error {
 	}
 
 	for _, crd := range requiredCRDs {
+		// TODO(#91): actually this is not the right way to check whether it's safe to create a resource of a given kind.
+		// A CRD can be present, but still being loaded or end up not accepted by the API server.
+		// Instead we should use the `kubectl api-resources` subcommand which exposes the status we're looking for.
 		_, err := d.runKubectl(ctx, KubectlOptions{
 			Args: []string{"get", "crd", crd},
 		})
@@ -124,7 +127,7 @@ func (d *Deployer) createCatalogSource(ctx context.Context, indexImage string) e
 		},
 	}
 
-	// Add security context config if supported (OCP 4.14+).
+	// TODO(#91): Add security context config if supported.
 	if hasSecurityContextConfig {
 		spec := catalogSource["spec"].(map[string]interface{})
 		spec["grpcPodConfig"] = map[string]interface{}{
@@ -158,6 +161,8 @@ func (d *Deployer) catalogSourceSupportsSecurityContextConfig(ctx context.Contex
 		return false, err
 	}
 
+	// TODO(#91): this is overly optimistic and would incorrectly succeed if an api version
+	// that contains this had "serving: false"
 	return strings.Contains(result.Stdout, "securityContextConfig"), nil
 }
 
@@ -251,6 +256,8 @@ func (d *Deployer) waitForAndApproveInstallPlan(ctx context.Context) error {
 	}
 
 	if time.Since(start) >= timeout {
+		// TODO(#91): some more info on what was wrong would be useful: a dump of the
+		// subscription or at least its name so that the user can investigate
 		return errors.New("timeout waiting for InstallPlan to be created")
 	}
 
@@ -321,6 +328,7 @@ func (d *Deployer) waitForCSVSuccess(ctx context.Context) error {
 		time.Sleep(5 * time.Second)
 	}
 
+	// TODO(#91): same as above
 	return fmt.Errorf("timeout waiting for CSV to succeed")
 }
 
@@ -344,6 +352,8 @@ func (d *Deployer) detectOperatorDeploymentMode(ctx context.Context) (bool, Oper
 		result, err := d.runKubectl(ctx, KubectlOptions{
 			Args: []string{"get", "deployment", operatorDeploymentName, "-n", operatorNamespace, "-o", "jsonpath={.metadata.labels}"},
 		})
+		// TODO(#91): This is not very robust. Better retrieve a specific label in the `get`
+		// command?
 		if err == nil && strings.Contains(result.Stdout, "olm.owner") {
 			return true, OperatorModeOLM
 		}

internal/deployer/override.go

@@ -35,6 +35,8 @@ func setNestedValue(m map[string]interface{}, path string, value interface{}) er
 			if existingMap, isMap := existing.(map[string]interface{}); isMap {
 				current = existingMap
 			} else {
+				// TODO(#91): shouldn't this be an error instead? I think it's more likely someone
+				// made a typo...
 				// Existing value is not a map, we need to replace it
 				newMap := make(map[string]interface{})
 				current[key] = newMap

internal/env/env.go

@@ -328,6 +328,10 @@ func fetchAPIResources() ([]string, error) {
 }
 
 func IsInStackroxRepository() bool {
+	// TODO(#91): This assumes that:
+	// - origin is the name of the upstream remote (not true if someone cloned their fork)
+	// - the git transport was used
+	// How about instead looking for "# StackRox Kubernetes Security Platform" in README?
 	cmd := exec.Command("git", "remote", "get-url", "origin")
 	outputBytes, err := cmd.Output()
 	if err != nil {

internal/helpers/tag.go

@@ -9,6 +9,7 @@ import (
 )
 
 const (
+	// TODO(#91): Is the plan to keep bumping this on new ACS releases?
 	defaultMainImageTag = "4.9.2"
 )
 

internal/imagecache/imagecache.go

@@ -33,6 +33,7 @@ func New(log *logger.Logger, cacheFile string, maxEntries int) *ImageCache {
 		if err != nil {
 			home = "."
 		}
+		// TODO(#91): how about using something XDG-compliant like ~/.cache/roxie/images?
 		cacheFile = filepath.Join(home, ".roxie.image_cache")
 	}
 

internal/portforward/portforward.go

@@ -106,6 +106,7 @@ func (m *Manager) Start(namespace, serviceName string, remotePort, preferredLoca
 			if err == nil {
 				syscall.Kill(-pgid, syscall.SIGTERM)
 			}
+			// TODO(#91): AFAICT this can get stuck forever if the process blocks SIGTERM...
 			cmd.Wait()
 		}
 		return "", fmt.Errorf("port-forward did not become ready")

tests/e2e/e2e_test.go

@@ -17,6 +17,7 @@ import (
 )
 
 const (
+	// TODO(#91): this is a bit old...
 	defaultMainImageTag = "4.8.2"
 	deployTimeout       = 30 * time.Minute
 	teardownTimeout     = 10 * time.Minute
@@ -67,6 +68,7 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
+// TODO(#91): maybe put the helper functions in a separate file?
 func teardownAllDeployments() error {
 	fmt.Println("=== Tearing down all deployments before running tests ===")