kustomize-controller icon indicating copy to clipboard operation
kustomize-controller copied to clipboard

Postbuild substitution should substitute when kustomization have no substitution defined

Open NFRBEZ opened this issue 2 years ago • 5 comments

Postbuild substitution is not working when there is no substitution in kustomization :

For example, the following kustomization

---
apiVersion: kustomize.toolkit.fluxcd.io/v1beta2
kind: Kustomization
metadata:
  name: my-app
  namespace: flux-system
spec:
  path: ./apps/staging/my-app
  targetNamespace: my-app
  interval: 1m
  prune: true
  sourceRef:
    kind: GitRepository
    name: flux-system
  healthChecks:
    - apiVersion: helm.toolkit.fluxcd.io/v2beta1
      kind: HelmRelease
      name: my-app
      namespace: my-app

Generate me the following ressource

---
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  labels:
    kustomize.toolkit.fluxcd.io/name: my-app
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  name: my-app
  namespace: my-app
spec:
  chart:
    ...
  values:
    metricsServer:
      dnsPolicy: ${dnsPolicy:=ClusterFirst}
      useHostNetwork: ${hostnetwork:="false"}
...

Which result on problem with my release installation I need to add at least one postBuild argument to make the substitution occurs, even If it is not referred in my ressources So doing like this :

---
apiVersion: kustomize.toolkit.fluxcd.io/v1beta2
kind: Kustomization
metadata:
  name: my-app
  namespace: flux-system
spec:
  path: ./apps/staging/my-app
  targetNamespace: my-app
  interval: 1m
  prune: true
  sourceRef:
    kind: GitRepository
    name: flux-system
  healthChecks:
    - apiVersion: helm.toolkit.fluxcd.io/v2beta1
      kind: HelmRelease
      name: my-app
      namespace: my-app
  postBuild:
    substitute:
      foo: bar

will work well :

---
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  labels:
    kustomize.toolkit.fluxcd.io/name: my-app
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  name: my-app
  namespace: my-app
spec:
  chart:
    ...
  values:
    metricsServer:
      dnsPolicy: ClusterFirst
      useHostNetwork: "false"
...

NFRBEZ avatar Nov 15 '23 14:11 NFRBEZ

This is by design, substitutions are opt-in as this operation is expensive for users which have thousands of manifests managed by Flux. You need to specify at least one value, even if you rely on defaults.

stefanprodan avatar Nov 15 '23 14:11 stefanprodan

Ok, got it Wouldn't it be clearer to also add a postbuild.enabled field in the schema with a default to false, so we don't have to add a foo value on substitution array to enable substitution ?

This is a small change for a clearer view of it, as we wouldn't have to check on pointed folder if the variable is set somewhere. It would by default apply all default value

NFRBEZ avatar Nov 16 '23 15:11 NFRBEZ

If we add an enabled filed which defaults to false, then we'll break everyone's clusters, as this will skip the replacements even if there are in place, people will have to edit all their Flux Kustomizations to set enabled true. This type of change can't happen since the Kustomization API is v1 GA.

stefanprodan avatar Nov 16 '23 16:11 stefanprodan

It depend of the implementation, postbuild would be implicit false without anything, enforced to true if enabled flag is set and defined to true if people defined any postbuild substitution in there kustomization. This way, people would not break theirs clusters.

Something there : https://github.com/fluxcd/kustomize-controller/blob/abdfab3dde0036f1712c3d1b6da90b276bace5a1/internal/controller/kustomization_controller.go#L621C33-L621C33

Which would change to something like the following :

if obj.Spec.PostBuild != nil || obj.Spec.PostBuildEnforce {

NFRBEZ avatar Nov 16 '23 16:11 NFRBEZ

That will be very confusing, why would something still apply if it's set to enabled: false, none of Kubernetes API work like this. Just add a dummy substitution like this:

  postBuild:
    substitute:
      enabled: "yes"

stefanprodan avatar Nov 16 '23 17:11 stefanprodan