Skip to content

BuildJobPlan reads from process environment without documenting or parameterizing #954

Description

@xdu31

Problem

The BuildJobPlan() function in pkg/api/validator/v1/job_plan_internal.go reads from the calling process environment via os.Getenv() without documenting this behavior or providing parameters to control it:

// pkg/api/validator/v1/job_plan_internal.go:86-92
if override := os.Getenv("AICR_VALIDATOR_IMAGE_REGISTRY"); override != "" {
    env = append(env, corev1.EnvVar{
        Name:  "AICR_VALIDATOR_IMAGE_REGISTRY",
        Value: override,
    })
}
if tag := os.Getenv("AICR_VALIDATOR_IMAGE_TAG"); tag != "" {
    env = append(env, corev1.EnvVar{
        Name:  "AICR_VALIDATOR_IMAGE_TAG",
        Value: tag,
    })
}

Impact:

  • External controllers calling BuildJobPlan() have hidden dependencies on their process environment
  • Job output varies based on caller's environment variables without explicit control
  • Public API contract is unclear about this behavior
  • Hard to test reliably

Origin:
This behavior existed in main:deployer.go:256 before the job planning API refactoring. The refactoring surfaced it in a public API, making the issue more visible.

Proposed Solutions

Option 1: Document (Minimum Fix)

Add godoc to BuildJobPlan() explaining the environment variable dependency:

// BuildJobPlan creates a JobPlan from a validator entry.
//
// Environment Variables:
// This function reads from the calling process environment:
//   - AICR_VALIDATOR_IMAGE_REGISTRY: Overrides image registry for all validators
//   - AICR_VALIDATOR_IMAGE_TAG: Overrides image tag for all validators
// These values are forwarded to validator containers. External controllers should
// be aware that their process environment will affect the generated JobPlan.
func BuildJobPlan(...) JobPlan {

Pros:

  • ✅ Non-breaking change
  • ✅ Makes behavior explicit
  • ✅ Quick to implement

Cons:

  • ⚠️ Doesn't give callers control
  • ⚠️ Still couples API to process environment

Option 2: Parameterize (Better Fix)

Add explicit parameters to control these values:

func BuildJobPlan(
    entry ValidatorEntry,
    runID string,
    namespace string,
    version string,
    commit string,
    serviceAccount string,
    imagePullSecrets []string,
    tolerations []corev1.Toleration,
    nodeSelector map[string]string,
    imageRegistryOverride string,  // New parameter
    imageTagOverride string,        // New parameter
) JobPlan {
    // Use parameters instead of os.Getenv()
    if imageRegistryOverride != "" {
        env = append(env, corev1.EnvVar{
            Name:  "AICR_VALIDATOR_IMAGE_REGISTRY",
            Value: imageRegistryOverride,
        })
    }
    // ...
}

Pros:

  • ✅ Explicit in function signature
  • ✅ Caller has full control
  • ✅ Easy to test
  • ✅ No hidden dependencies

Cons:

  • ⚠️ Breaking API change
  • ⚠️ Requires updating all callers

References

Acceptance Criteria

  • Document the environment variable dependencies in godoc, OR
  • Refactor to use explicit parameters instead of os.Getenv()
  • Update tests to verify behavior
  • Update caller code if parameterizing

Metadata

Metadata

Assignees

No one assigned

    Labels

    Fields

    No fields configured for Enhancement.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions