Skip to content

Refactor: consolidate reference tracking in pkg/k8s/deployer.go #3678

@lkingland

Description

@lkingland

Background

Several functions in pkg/k8s/deployer.go track references to cluster
resources (Secrets, ConfigMaps, PVCs) so that CheckResourcesArePresent
can fail fast if anything referenced by a function isn't on the
cluster. The current shape passes three independent sets as
out-parameters via pointer:

func ProcessEnvs(envs []fn.Env, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func ProcessVolumes(volumes []fn.Volume, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (...)
func createEnvFromSource(value string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func createEnvVarSource(slices []string, referencedSecrets, referencedConfigMaps *sets.Set[string]) (...)
func (d *Deployer) generateDeployment(f fn.Function, namespace string, daprInstalled bool, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string]) (...)
func CheckResourcesArePresent(ctx context.Context, namespace string, referencedSecrets, referencedConfigMaps, referencedPVCs *sets.Set[string], referencedServiceAccount, imagePullSecret string) error

Most recently propagated to generateDeployment in #3671 to fix a
silent-validation gap on the first-deploy path. That PR is the right
fix for the bug; this issue captures the design cleanup that the PR
deliberately did not absorb.

Why this is worth cleaning up

Three layers of smell sit on top of each other:

1. Pointer-to-map is doubly indirect.
k8s.io/apimachinery/pkg/util/sets.Set[T] is map[T]Empty. Maps in
Go are reference types — passing a Set by value already lets the
callee mutate the same backing storage. The * adds a level of
indirection that buys nothing the callers actually use: nobody
reassigns the set, nobody distinguishes a nil pointer from an empty
set, and the only operations the callees invoke are Insert and
Has. Idiomatic Go here is sets.Set[T] by value.

2. The triple is structurally coupled but passed as three loose
parameters.
Every signature that touches reference tracking repeats
referencedSecrets, referencedConfigMaps, referencedPVCs. That's a
cohesive triple — it deserves a name:

type References struct {
    Secrets    sets.Set[string]
    ConfigMaps sets.Set[string]
    PVCs       sets.Set[string]
}

Once the triple has a type, every call site collapses from three
arguments to one.

3. Accumulator-by-out-param is C-style in a language with
multi-return.
Two cleaner shapes are available:

// (a) multi-return — pure-function flavor:
func ProcessEnvs(envs []fn.Env) ([]corev1.EnvVar, []corev1.EnvFromSource, References, error)

// (b) receiver-as-tracker — OO flavor:
type Tracker struct { References }
func (t *Tracker) ProcessEnvs(envs []fn.Env) ([]corev1.EnvVar, []corev1.EnvFromSource, error)

Either reads better than threading three out-pointers through the call
chain. (b) also makes it harder to forget to wire one of the sets
through, which is exactly the bug class that #3671 fixed (the local
sets in generateDeployment were silently invisible to the caller).

Suggested approach

Pick one shape — (a) or (b) above — and apply it consistently.
Either change is mechanical:

  • Define References (and Tracker, if going with shape (b)) in a
    shared spot, likely near the top of pkg/k8s/deployer.go.
  • Update the affected functions to use it:
    • ProcessEnvs
    • ProcessVolumes
    • createEnvFromSource
    • createEnvVarSource
    • generateDeployment
    • CheckResourcesArePresent
  • Update call sites in Deploy() — both the create branch and the
    update branch in pkg/k8s/deployer.go. The keda deployer inherits
    via delegation, no separate work needed there.
  • Update the unit tests in pkg/k8s/deployer_test.go for the new
    signatures. Integration tests should not need behavior changes.
  • Confirm no behavior changes — this is purely a shape refactor.

Out of scope

Why this wasn't done in #3671

#3671 is a focused bug fix from a contributor. Asking them to absorb
a refactor of a pre-existing pattern would have widened scope unrelated
to the bug and gated a real user-facing fix on cleanup work. This issue
captures the cleanup as separable, picker-uppable work.

Good first issue?

Reasonable candidate — the change is mechanical, the affected
functions are localized to one file (plus the test file), and the
desired end state is clearly specified. The judgment call between
shapes (a) and (b) is the only design decision; either is acceptable.

Metadata

Metadata

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions