Allow users to inject a postrenderer #219

ludydoo · 2023-06-28T07:31:23Z

It would be useful for us to be able to inject a per-object PostRenderer to implement a feature such as istio overlays. see https://istio.io/latest/docs/setup/additional-setup/customize-installation/#patching-the-output-manifest

We cannot use the actionClientConfigGetter or actionClientGetter with a WithInstallOption(...) because this method doesn't pass the CustomResource to the renderer at the moment of installing/upgrading/dry-running. Though, our usecase would be that each CustomResource would have configurable "patches" that would be evaluated against the manifest at the moment of reconciliation.

Example of usecase:

https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/apis/platform/v1alpha1/central_types.go#L62C3-L62C3

https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/apis/platform/v1alpha1/overlay_types.go#L17

https://github.com/stackrox/stackrox/blob/ROX-18085-operator-overlays/operator/pkg/overlays/postrenderer.go

https://github.com/stackrox/stackrox/blob/1fd296314419b25df5831e472e74512bd5e01e5a/operator/pkg/reconciler/reconciler_factory.go#L82

ludydoo · 2023-07-04T13:21:30Z

@joelanford pinging you for your review on this one 🙏

porridge · 2023-07-11T07:57:38Z

pkg/reconciler/reconciler_test.go

+	objs := manifestToObjects(renderedManifests.String())
+	var manifests = make([]string, len(objs))
+	for i, obj := range objs {
+		var annotations = obj.GetAnnotations()
+		if annotations == nil {
+			annotations = map[string]string{}
+		}
+		annotations["foo"] = "bar"
+		obj.SetAnnotations(annotations)
+		manifest, err := yaml.Marshal(obj)


Disclaimer: I am aware that this is test code, and in this particular context speed does not matter much, but I'm making this point thinking about the use case that this change is designed to serve, i.e. ~~adding annotations~~ arbitrary modifications to all objects.

I'm concerned that this added round of deserialization/serialization will slow things down even more (compared to how slow helm-based operator already is).

Can you think of any other way that we could inject annotations to already-deserialized objects? Perhaps there is a different helm library hook that we could leverage? Or maybe we can ask for one?

One of the ways we could do this is using the controller-runtime interceptor, intercept the create/update calls, and handle structured and unstructured objects and lists to append the annotations before creating or updating the objects. Users could also implement a custom MutatingWebHook to add annotations. Or maybe even update the charts themselves ?

Though in stackrox's desired usecase (apply user-provided overlay patches), this could only be done, AFAIK, using de & reserialization.

Oh, right, it's for arbitrary modifications, not just annotations. I updated my comment above.

However given that this library already does a deserialize+reserialize in the ownerPostRenderer I think it might be better to refactor the code there to allow adding custom visitors that could walk already parsed objects, rather than custom postrenderers that would repeat the same work?

I'm not sure, because the user might want to remove parts of the manifest altogether, or add new resources to it, which would not be possible with a walk.
I ran some benchmark tests, and the impact seems negligible. Deserializing then re-serializing an object from-to a yaml manifest takes roughly 0.0003 sec. I ran a test with 1000 deployments in 1 manifest (extremely large manifest), and it takes 0.27 sec.

Good point about visitor not being powerful enough for some use cases!

However I still think this change would be exposing functionality at the wrong level of abstraction.
It looks like the PostRenderer hook exposed in the helm library works on the level of bytes.Buffer objects because this is a natural fit for exposing it from the helm CLI - since that works on the level of command pipelines.

While this works, and the overhead seems manageable (thanks for checking this!), I think it forces the API user to do unnecessary work, so is not elegant.

What do you think about exposing a kube.ResourceList instead? (That one has Append and Filter, for example).

pkg/reconciler/reconciler_test.go

pkg/client/actionclient.go

porridge · 2023-07-24T10:42:36Z

pkg/reconciler/reconciler_test.go

+	objs := manifestToObjects(renderedManifests.String())
+	var manifests = make([]string, len(objs))
+	for i, obj := range objs {
+		var annotations = obj.GetAnnotations()
+		if annotations == nil {
+			annotations = map[string]string{}
+		}
+		annotations["foo"] = "bar"
+		obj.SetAnnotations(annotations)
+		manifest, err := yaml.Marshal(obj)


Good point about visitor not being powerful enough for some use cases!

However I still think this change would be exposing functionality at the wrong level of abstraction.
It looks like the PostRenderer hook exposed in the helm library works on the level of bytes.Buffer objects because this is a natural fit for exposing it from the helm CLI - since that works on the level of command pipelines.

While this works, and the overhead seems manageable (thanks for checking this!), I think it forces the API user to do unnecessary work, so is not elegant.

What do you think about exposing a kube.ResourceList instead? (That one has Append and Filter, for example).

pkg/client/actionclient.go

varshaprasad96 · 2023-07-28T17:35:48Z

Reopening this to run the CI again

everettraven

Code changes look good and make sense to me. Approving but pending merge based on resolving CI failures

varshaprasad96 · 2023-08-02T14:08:48Z

@ludydoo could you please fix the linter errors as mentioned in the CI so that we can merge this

pkg/client/actionclient_test.go

coveralls · 2023-08-03T09:32:33Z

Pull Request Test Coverage Report for Build 5748961402

13 of 13 (100.0%) changed or added relevant lines in 1 file are covered.
50 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.09%) to 88.918%

Files with Coverage Reduction	New Missed Lines	%
pkg/reconciler/internal/updater/updater.go	7	85.71%
pkg/reconciler/reconciler.go	43	90.83%

Totals
Change from base Build 5694720579:	-0.09%
Covered Lines:	1717
Relevant Lines:	1931

💛 - Coveralls

ludydoo · 2023-08-03T09:38:15Z

@ludydoo could you please fix the linter errors as mentioned in the CI so that we can merge this

Done :)

openshift-ci bot requested review from joelanford and porridge June 28, 2023 07:31

github-actions bot added area/sdk area/testing labels Jun 28, 2023

ludydoo force-pushed the allow-user-defined-postrenderer branch from 820887e to f067470 Compare June 28, 2023 08:14

ludydoo mentioned this pull request Jun 30, 2023

ROX-18085: Allow custom post-renderer stackrox/helm-operator#41

Merged

porridge reviewed Jul 11, 2023

View reviewed changes

ludydoo force-pushed the allow-user-defined-postrenderer branch from fc4a4aa to f11cb2f Compare July 11, 2023 11:57

github-actions bot removed the area/testing label Jul 11, 2023

ludydoo requested a review from porridge July 13, 2023 10:56

porridge reviewed Jul 24, 2023

View reviewed changes

varshaprasad96 reviewed Jul 27, 2023

View reviewed changes

pkg/client/actionclient.go Outdated Show resolved Hide resolved

pkg/client/actionclient.go Show resolved Hide resolved

github-actions bot added the area/testing label Jul 28, 2023

ludydoo requested a review from varshaprasad96 July 28, 2023 13:22

ludydoo added 2 commits July 28, 2023 15:50

Allow user-provided post-renderers

2a0b495

PR Comments

8ac34c0

ludydoo force-pushed the allow-user-defined-postrenderer branch from 4fafe80 to 8ac34c0 Compare July 28, 2023 13:51

varshaprasad96 closed this Jul 28, 2023

varshaprasad96 reopened this Jul 28, 2023

everettraven approved these changes Jul 28, 2023

View reviewed changes

varshaprasad96 reviewed Aug 2, 2023

View reviewed changes

pkg/client/actionclient_test.go Outdated Show resolved Hide resolved

ludydoo added 4 commits August 3, 2023 10:33

Fix mock postRenderer

201750e

Fix leftover

1984ea2

Fix leftover

27b2ba4

Fix failing test

3666f74

ludydoo requested a review from varshaprasad96 August 3, 2023 09:34

ludydoo requested a review from everettraven August 3, 2023 09:49

varshaprasad96 merged commit caa0f13 into operator-framework:main Aug 3, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to inject a postrenderer #219

Allow users to inject a postrenderer #219

ludydoo commented Jun 28, 2023 •

edited

Loading

ludydoo commented Jul 4, 2023

porridge Jul 11, 2023 •

edited

Loading

ludydoo Jul 11, 2023 •

edited

Loading

porridge Jul 13, 2023

ludydoo Jul 14, 2023

porridge Jul 24, 2023

porridge Jul 24, 2023

varshaprasad96 commented Jul 28, 2023

everettraven left a comment

varshaprasad96 commented Aug 2, 2023

coveralls commented Aug 3, 2023

ludydoo commented Aug 3, 2023

Allow users to inject a postrenderer #219

Allow users to inject a postrenderer #219

Conversation

ludydoo commented Jun 28, 2023 • edited Loading

ludydoo commented Jul 4, 2023

porridge Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

ludydoo Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

porridge Jul 13, 2023

Choose a reason for hiding this comment

ludydoo Jul 14, 2023

Choose a reason for hiding this comment

porridge Jul 24, 2023

Choose a reason for hiding this comment

porridge Jul 24, 2023

Choose a reason for hiding this comment

varshaprasad96 commented Jul 28, 2023

everettraven left a comment

Choose a reason for hiding this comment

varshaprasad96 commented Aug 2, 2023

coveralls commented Aug 3, 2023

Pull Request Test Coverage Report for Build 5748961402

💛 - Coveralls

ludydoo commented Aug 3, 2023

ludydoo commented Jun 28, 2023 •

edited

Loading

porridge Jul 11, 2023 •

edited

Loading

ludydoo Jul 11, 2023 •

edited

Loading