image-automation-controller icon indicating copy to clipboard operation
image-automation-controller copied to clipboard

Image markers are not usable with configuration transpilers

Open toddkazakov opened this issue 3 years ago • 16 comments

Image markers cannot be emitted by configuration transpilers like jsonnet, because they are comments in yaml files. In our case this makes the migration from flux v1 nearly impossible, because we'll have to completely rewrite our entire infrastructure configuration.

It would be great to have support for additional way of defining image markers as data in the yaml file, ideally as flux v1 annotations.

toddkazakov avatar Mar 23 '21 13:03 toddkazakov

@toddkazakov do you generate with jsonnet the Flux custom resources like GitRepository/Kustomization/ImagePolicy/etc or these YAMLs are generated with flux CLI?

stefanprodan avatar Mar 23 '21 16:03 stefanprodan

Currently, we're only exploring the migration path, but ideally we'd like to be able to use jsonnet for generating all yaml files, including the flux custom resources.

toddkazakov avatar Mar 23 '21 17:03 toddkazakov

Can you place a YAML file next to jsonnet and use the values from that file when generating the manifests? If so then you could create a config file like:

apiVersion: my.config.k8s.io/v1
kind: MyImages
images:
 app1: init-tag # {"$imagepolicy": "apps-ns:app1:tag"}
 app2: init-tag # {"$imagepolicy": "apps-ns:app2:tag"}

Flux will update the config file, then you'll generate the manifests using the tags from the config in CI.

stefanprodan avatar Mar 23 '21 17:03 stefanprodan

Currently it's not possible, because flux works with the generated files. It's not that it can't be set up, but it will slow down the development workflow, because generation is fairly slow.

toddkazakov avatar Mar 23 '21 18:03 toddkazakov

If you make changes to the generated files, wouldn't those changes be overwritten next time the generation step ran?

squaremo avatar Mar 24 '21 08:03 squaremo

If you make changes to the generated files, wouldn't those changes be overwritten next time the generation step ran?

The previously applied manifest is available at generation time. In our case, if the image attribute has been previously set by flux it will be used, otherwise we apply a default value. E.g.:

image: lib.getElse(prev, 'spec.values.blob.deployment.image', 'tidepool/platform-blob:master-latest'),

toddkazakov avatar Mar 24 '21 11:03 toddkazakov

The previously applied manifest is available at generation time

I see, the way it works is that you generate the YAMLs, and sync those, and update them with flux's automation; then occasionally, you regenerate the YAMLs from jsonnet, referring to the previous YAMLs for fields that were updated by automation.

Setting aside changes to the automation for a second: one way to mostly get what you want would be to use a kustomization.yaml with the image update markers, which doesn't get generated but refers to the generated files.

Whether or not that works for you, I recognise that it's not a general solution. I have opened a discussion to solicit suggestions: https://github.com/fluxcd/flux2/discussions/1207

squaremo avatar Apr 05 '21 10:04 squaremo

@toddkazakov

I think Stefan's suggestion can work if you make the generator (tpctl) into a Kubernetes controller and change the "values.yaml" file into a "Values" Custom Resource that that controller manages.

Check out this example of a jsonnet controller.

With this approach you will not need the old versions of the manifests since the necessary updates will be reflected in the Values CR.

derrickburns avatar Jul 17 '21 19:07 derrickburns

I would like to extend the Jsonnet example to show how the image automation can be effectively used together with ImageUpdateAutomation: https://fluxcd.io/docs/use-cases/gh-actions-manifest-generation/

The key to this actually is that I am not using Jsonnet directly, or not by itself; in this example there are a few places where 95% of the solution is already shown, see: https://fluxcd.io/docs/use-cases/gh-actions-manifest-generation/#jsonnet-configmap-with-envsubst - the solution being, to store your source in a configmap and load it in by reference at runtime, with whatever facility you have available for that (it obviously does not have to be Jsonnet, it can also be Envsubst by itself)

The missing part is the example does not actually invoke ImageUpdateAutomation. So we are loading version information from a configmap, the configmap could be automated with ImageUpdateAutomation, it lives on disk with a YAML format, so it can get kyaml setter comments and be parsed by IUA at reconcile time. The example just doesn't show this final leap, of actually using this configmap together with an ImageUpdateAutomation resource enabled on it.

kingdonb avatar Aug 20 '21 08:08 kingdonb

@kingdonb your proposed solution still relies on a manually managed configmap, that can't be generated from jsonnet for example.

@stefanprodan is there an interest to introduce a new image policy mechanism that would work similar to flux v1? I'll be more than happy to contribute if that's the case.

toddkazakov avatar Sep 07 '22 15:09 toddkazakov

manually managed configmap

There's nobody manually managing the configmap, maybe you mean that you need a separate Flux Kustomization to apply them to the cluster, or a separate directory of manifests than the one that your Jsonnet is using, but that configmap is just a receiver for the information that Flux is going to need to have been injected into the cluster, later on when you're reconciling.

You put the image marker on the configmap, maybe there are a dozen of them (or separate entries in one configmap) if you have that many app environments that you want to manage with ImageUpdateAutomation, and those configmaps are where the version information is maintained.

There are better solutions in Flux now, since 0.32 we have OCIRepository support from all appliers. It doesn't quite obviate ImageUpdateAutomation completely, as there are still use cases where you might want IUA, but it does provide better ways to ship manifests to your cluster, especially if you are using manifest generation tools or configuration transpilers. You do not need a Git branch for the output of the manifest generator anymore, you can just flux push artifact from CI and ship those manifests into the cluster as an OCI Image, tagged with revision and source for easy tracing back to the original git commit.

The big advantage of the OCI approach is security (there is no longer a need to maintain read-write git credentials in any automation, so the source of truth is better protected from risk of compromise.) But another big advantage is that you can avoid spamming your repositories with commits from automation. It's much better, we think, for automation's effects to be "seen not heard" – the repo should contain expressions of intent, you should not be rebasing your work on top of noisy commits that are made by automation.

I'm personally open to anything that makes our ImageUpdateAutomation more user friendly and competitive with the ease of use from Flux v1, but it needs a proposal, discussion, and an RFC, as it represents a major change and this is required by the governance of Flux, which is on its way to GA and is considered to already have a generally stable set of APIs that cloud providers are already building atop of them. Where we've made changes from Flux v1, they've generally been undertaken with a great deal of consideration and are unlikely to be reversed.

But if you're volunteering to work on it yourself, that's a very different type of proposal than the other kind... 👍

I don't think we're opposed in principle to the idea that we can have an annotation somewhere, on a deployment or something, that results in some manifest generation which speaks the Flux Image API, to create the necessary Flux objects that keep the deployment source updated through IUA. I would personally be very happy to see any advancement providing users with an opportunity to escape from boilerplate hell.

But FWIW, I am quite sure this solution is already implemented by many users with Kyverno and likely others. Building such functionality back into Flux itself would need a compelling argument for why that functionality doesn't belong in policy tools like Kyverno that already implement it.

kingdonb avatar Sep 07 '22 22:09 kingdonb

maybe you mean that you need a separate Flux Kustomization to apply them to the cluster, or a separate directory of manifests than the one that your Jsonnet is using, but that configmap is just a receiver for the information that Flux is going to need to have been injected into the cluster, later on when you're reconciling.

That's correct. However, the problem is that there are two sources of truth now - the values for the templated manifests and a separate set of yaml configmaps for flux image automation. This is not ideal as it's hard to justify which config belongs to the template values file vs manually managed configmaps.

You do not need a Git branch for the output of the manifest generator anymore

I consider having the output of the generator stored in git a good thing - we can always inspect the actual config that will be applied to kubernetes. This is one of the shortfalls of helm (and jsonnet) as well - it's hard to tell what the rendered manifest will look like only by inspecting a template and the template variables.

OCI looks like a good solution, but it doesn't solve the issue of automated image updates if we're not already storing the application config in the application repo. Our application and docker hub repositories are public and we don't feel comfortable storing and exposing the secrets publicly, even if they encrypted. Currently we have a gitops repository per cluster, which is the single source of truth what the desired state ought to be.

I don't think we're opposed in principle to the idea that we can have an annotation somewhere, on a deployment or something, that results in some manifest generation which speaks the Flux Image API, to create the necessary Flux objects that keep the deployment source updated through IUA. I would personally be very happy to see any advancement providing users with an opportunity to escape from boilerplate hell.

It doesn't even have to be that complicated. The only issue with markers is that they are not available to 3rd party tools because they are non-standard yaml (i.e. comments).

My proposal is to define those in annotations instead of comments. E.g.:

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: podinfo
  namespace: default
  annotations:
    fluxcd.io/marker.spec.chart.values.shoreline.image.repository: {"$imagepolicy": "flux-system:podinfo:name"}
    fluxcd.io/marker.spec.chart.values.shoreline.image.tag: {"$imagepolicy": "flux-system:podinfo:tag"}
spec:
  values:
    image:
      repository: ghcr.io/stefanprodan/podinfo
      tag: 5.0.0

instead of

apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: podinfo
  namespace: default
spec:
  values:
    image:
      repository: ghcr.io/stefanprodan/podinfo # {"$imagepolicy": "flux-system:podinfo:name"}
      tag: 5.0.0  # {"$imagepolicy": "flux-system:podinfo:tag"}

Where we've made changes from Flux v1, they've generally been undertaken with a great deal of consideration and are unlikely to be reversed.

I'm not asking for a decision to be reversed, but to provide an additional way to configure the image automation controller. The schema even allows for different update strategies to be defined:

// ImageUpdateAutomationSpec defines the desired state of ImageUpdateAutomation
type ImageUpdateAutomationSpec struct {
         
	// Update gives the specification for how to update the files in
	// the repository. This can be left empty, to use the default
	// value.
	// +kubebuilder:default={"strategy":"Setters"}
	Update *UpdateStrategy `json:"update,omitempty"`
}

My proposal is to add a new strategy e.g. AnnotationSetters that would allows us to configure the values to be replaced by annotation instead of a comment in the yaml file.

But FWIW, I am quite sure this solution is already implemented by many users with Kyverno and likely others. Building such functionality back into Flux itself would need a compelling argument for why that functionality doesn't belong in policy tools like Kyverno that already implement it.

I don't understand how Kyverno can help here. I'd be genuinely surprised if a Kyverno mutation hook can convert annotation to a comment.

toddkazakov avatar Sep 09 '22 08:09 toddkazakov

That's very helpful to add clarity. A lot of people say "can you make Image Update Automation more like Flux v1" and what they generally mean can be very hard to interpret, for a lot of values of that question the answer is "no, the change was 100% deliberate, we can't put it back for performance reasons" etc...

AnnotationSetters actually sounds like a really solid idea now that you've explained what you mean in a bit more detail, although I think third-party tools can use the standard Kyaml library that Flux uses to handle comment setters, IDK how well adopted that is or how easy it is to implement, compared to the basic YAML standard data structures that are persistent and should survive any parse/reserialize operation from any conformant YAML parser implementation.

I don't understand how Kyverno can help here. I'd be genuinely surprised if a Kyverno mutation hook can convert annotation to a comment.

You're right, the comment is not part of the formal yaml data structure, it's markup that's meant to be consumed only in the git repository and never from a resource that has already been applied to the cluster and lives in etcd. But that's not the only time that YAML gets parsed, which means your point stands well.

You do not need a Git branch for the output of the manifest generator anymore I consider having the output of the generator stored in git a good thing

Fair enough, Image Update Automation is not going away 👍

OCI looks like a good solution, but it doesn't solve ...

We haven't documented many of the ways that OCI solution can be used yet. One thing about this solution which has been hard to grapple with for me at least is the fact that we've historically told everyone ":latest is bad, don't use mutable tags" and now, for this specific case, we can reverse that guidance and say:

OCI Repository mutable tag is no different than git branch, if you use GitRepository sources with the main branch, then you should have no issues using an environment tag on an OCIRepository

I'm not trying to talk you out of using ImageUpdateAutomation, 😆 let's stay on topic then... would you be willing to open a discussion on fluxcd/flux2 Proposals with the details that you posted here? That's a more appropriate place that is likely to get more attention from maintainers and feedback about the idea. You can link back to this issue, but this discussion goes back a ways, and your proposal while it addresses this issue, could stand alone and be better received with its own context.

kingdonb avatar Sep 09 '22 13:09 kingdonb

AnnotationSetters actually sounds like a really solid idea now that you've explained what you mean in a bit more detail, although I think third-party tools can use the standard Kyaml library that Flux uses to handle comment setters, IDK how well adopted that is or how easy it is to implement, compared to the basic YAML standard data structures that are persistent and should survive any parse/reserialize operation from any conformant YAML parser implementation

@kingdonb thank you for your comment, it actually pointed me to the right direction. I took a look at how setters are implemented in the automation controller and implemented a command line tool kyaml-a2c for post-processing the yaml manifests which are produced from the jsonnet templates. It converts annotations to image markers. I didn't know kyaml had a comment preserving parser! My issue is actually solved now as it was easy adding a post-processing step to our config generation process. Let me know if you still think adopting this officially in flux is worth pursuing.

toddkazakov avatar Sep 12 '22 10:09 toddkazakov

@toddkazakov I think it's worth implementing AnnotationSetters, for this we'll need an RFC. Would you be able to drive this? I can sponsor the RFC if you're up to it. Thanks!

stefanprodan avatar Sep 12 '22 10:09 stefanprodan

I can drive this. I'll get this going towards the end of the week.

toddkazakov avatar Sep 13 '22 09:09 toddkazakov