Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support targeted Patches in the PostRenderer specification #432

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

trekawek
Copy link
Contributor

This PR adds support for the target selector for strategic merge Kustomize patches, similarly as it's supported for the JSON6902 patches.

If the strategic merge patch have following form:

- target:
    group: apps
    version: v1
    kind: Deployment
    name: nginx
  patch:
    apiVersion: apps/v1
    kind: Deployment
    metadata:
      name: nginx
    spec:
      template:
        spec:
          containers:
            - name: nginx
              image: nignx:latest

it'll be translated into appropriate targeted Patch in the effective Kustomize configuration.

Of course, the non-targeted form of the strategic merge patch is still supported:

- apiVersion: apps/v1
  kind: Deployment
  metadata:
    name: nginx
  spec:
    template:
      spec:
        containers:
          - name: nginx
            image: nignx:latest

The rationale behind it is a case in which certain Deployments or other K8s resources can be enabled or disabled with a Helm property. Now if we want to patch such a Deployment with the strategic merge patch (e.g. in order to add an env variable), the reconciliation will fail if the Deployment doesn't exist - this means we can't have a universal postRenderer configuration which will work regardless of the Helm values configuration.

The JSON6902 patches, with the explicit "target" section will work, as they'll simply ignore the missing targets. However, manipulating env variables with the JSON6902 patches is much harder than with the strategic merge patches and sometimes may not be possible. This is also true for other properties nested inside arrays, indexed by their "name" property (containers, envs, volumes, etc.)

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! 🙇 🥇

Given the kustomize-controller has a likewise feature, I think it would be better to align the APIs to work the same. This means that we would need the following:

  1. The function from here: https://github.com/fluxcd/kustomize-controller/blob/main/controllers/kustomization_generator.go#L220
  2. A new Patches API field, as present in: https://github.com/fluxcd/kustomize-controller/blob/main/api/v1beta2/kustomization_types.go#L89. We import the same package (github.com/fluxcd/pkg/apis/kustomize), so this should be available.
  3. Tiny bit of code to take the field into account (but modified to work with the HelmRelease):
    for _, m := range kg.kustomization.Spec.Patches {
    	kus.Patches = append(kus.Patches, kustypes.Patch{
    		Patch:  m.Patch,
    		Target: adaptSelector(&m.Target),
    	})
    }

@hiddeco hiddeco added enhancement New feature or request area/kustomize Kustomize (post-rendering) related issues and pull requests labels Mar 11, 2022
@trekawek trekawek force-pushed the kustomize-strategic-merge-target branch from c93e10d to 48735f3 Compare March 11, 2022 13:26
@trekawek
Copy link
Contributor Author

Thanks for the review @hiddeco! I applied your suggestions and adjusted the tests. Should I rebase/squash the commits to have a clean commit history for this feature?

@hiddeco
Copy link
Member

hiddeco commented Mar 11, 2022

Yes, please rebase and squash 😄

@trekawek trekawek force-pushed the kustomize-strategic-merge-target branch from 8ea8bb9 to e34c193 Compare March 11, 2022 13:34
Signed-off-by: Tomek Rękawek <rekawek@adobe.com>
@trekawek trekawek force-pushed the kustomize-strategic-merge-target branch from e34c193 to 5b1b1ce Compare March 11, 2022 13:34
@trekawek
Copy link
Contributor Author

@hiddeco - done.

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks for following through on the suggested changes @trekawek

@hiddeco hiddeco changed the title Support target selector for the strategic merge Kustomize renderers. @trekawek Support targeted Patches in the PostRenderer specification Mar 11, 2022
@hiddeco hiddeco changed the title @trekawek Support targeted Patches in the PostRenderer specification Support targeted Patches in the PostRenderer specification Mar 11, 2022
@hiddeco hiddeco merged commit 57626fb into fluxcd:main Mar 11, 2022
@trekawek
Copy link
Contributor Author

That was quick. Thanks for the feedback and merging it so fast, much appreciated!

@hiddeco
Copy link
Member

hiddeco commented Mar 11, 2022

The release will take a bit longer I am afraid, due to some extensive changes to the source-controller we are currently working on to finalize (which need to be taken into account on this side). If your patch is business critical, I can provide you with a RC build which needs to be combined with the updated CRD.

@trekawek
Copy link
Contributor Author

There's no need to create a RC build, but thanks for the offer @hiddeco!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kustomize Kustomize (post-rendering) related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants