diff --git a/cmd/deploy.go b/cmd/deploy.go index 7f3aee80..efb7c501 100644 --- a/cmd/deploy.go +++ b/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 { diff --git a/cmd/subshell.go b/cmd/subshell.go index 4a9b5c1b..93199c5c 100644 --- a/cmd/subshell.go +++ b/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 diff --git a/internal/clusterdefaults/clusterdefaults.go b/internal/clusterdefaults/clusterdefaults.go index a2f5693c..7d555bff 100644 --- a/internal/clusterdefaults/clusterdefaults.go +++ b/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") diff --git a/internal/deployer/deploy_via_helm.go b/internal/deployer/deploy_via_helm.go index bc96c110..a410cdf2 100644 --- a/internal/deployer/deploy_via_helm.go +++ b/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 { diff --git a/internal/deployer/deploy_via_operator.go b/internal/deployer/deploy_via_operator.go index d6b6e5ac..2b73baa7 100644 --- a/internal/deployer/deploy_via_operator.go +++ b/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 { diff --git a/internal/deployer/deployer.go b/internal/deployer/deployer.go index c6947423..b957b74d 100644 --- a/internal/deployer/deployer.go +++ b/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 diff --git a/internal/deployer/kubectl.go b/internal/deployer/kubectl.go index 0fd8aa23..1205253d 100644 --- a/internal/deployer/kubectl.go +++ b/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 diff --git a/internal/deployer/operator.go b/internal/deployer/operator.go index 919209d8..bb2d7936 100644 --- a/internal/deployer/operator.go +++ b/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") || diff --git a/internal/deployer/operator_olm.go b/internal/deployer/operator_olm.go index 62eda9c6..94b33f04 100644 --- a/internal/deployer/operator_olm.go +++ b/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 } diff --git a/internal/deployer/override.go b/internal/deployer/override.go index f44890d3..324ac1b9 100644 --- a/internal/deployer/override.go +++ b/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 diff --git a/internal/env/env.go b/internal/env/env.go index 9d632cea..d3ae4fae 100644 --- a/internal/env/env.go +++ b/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 { diff --git a/internal/helpers/tag.go b/internal/helpers/tag.go index a0d1903b..39a3cbb3 100644 --- a/internal/helpers/tag.go +++ b/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" ) diff --git a/internal/imagecache/imagecache.go b/internal/imagecache/imagecache.go index e072389a..8d61187d 100644 --- a/internal/imagecache/imagecache.go +++ b/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") } diff --git a/internal/portforward/portforward.go b/internal/portforward/portforward.go index 3c864a85..d1ea1159 100644 --- a/internal/portforward/portforward.go +++ b/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") diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go index 82f17608..90219a11 100644 --- a/tests/e2e/e2e_test.go +++ b/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 ===")