Adds support for extension custom manifests #468

skonto · 2021-02-12T14:32:29Z

Proposed Changes

Appends manifests defined in the controller extension into the main manifested loaded by the controller.
Why?
Currently downstream we have a scenario where a number of custom monitoring resources are installed along with the
core Serving ones. Although we can ship them together with the core ones, we would like to be able to create
a number of manifests dynamically from a resource blueprint as there is a lot of repetition in the monitoring custom resources.
Another issue is that we need to filter core resources to apply specific transformations and this requires special transformation where we need to pick a resource based on some naming convention.
Custom resources go through the validation process along with the rest of the manifests as usual assuming the appropriate label is defined.
Although there is support for custom manifests at the CRD level this does not really fit into our requirements for platform specific custom resources. Afaik, that functionality was meant for loading manifests from a Knative user perspective eg. ops persona. This patch serves the vendor persona.

knative-prow-robot

@skonto: 0 warnings.

In response to this:

Proposed Changes

Appends manifests defined in the controller extension into the main manifested loaded by the controller.

Why?
Currently downstream we have a scenario where a number of custom monitoring resources are installed along with the
core Serving ones. Although we can ship them together with the core ones, we would like to be able to create
a number of manifests dynamically from a resource blueprint as there is a lot of repetition in the monitoring custom resources.
Another issue is that we need to filter core resources to apply specific transformations and this requires special transformation where we need to pick a resource based on some naming convention.

Custom resources go through the validation process along with the rest of the manifests as usual assuming the appropriate label is defined.

Although there is support for custom manifests at the CRD level this does not really fit into our requirements for platform specific custom resources. Afaik, that functionality was meant for loading manifests from a Knative user perspective eg. ops persona. This patch refers to the vendor persona.

/cc @markusthoemmes

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2021-02-12T14:32:40Z

Welcome @skonto! It looks like this is your first PR to knative/operator 🎉

knative-prow-robot · 2021-02-12T14:32:40Z

Hi @skonto. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

markusthoemmes · 2021-02-12T14:39:22Z

pkg/reconciler/common/extensions.go

@@ -39,6 +40,9 @@ func NoExtension(context.Context) Extension {

 type nilExtension struct{}

+func (nilExtension) Manifests() []mf.Manifest {


Should this take the KComponent to be able to dynamically decide based on Spec settings?

Yes good idea does not hurt anything to add it.

markusthoemmes · 2021-02-12T14:39:44Z

pkg/reconciler/common/extensions_test.go

+func (t TestExtension) Manifests() []mf.Manifest {
+	manifest, err := mf.NewManifest("testdata/kodata/additional-manifests/additional-sa.yaml")
+	if err != nil {
+		return nil


Feels like we should be able to return the error in case we want to abort.

houshengbo · 2021-02-12T16:16:34Z

/ok-to-test

houshengbo · 2021-02-12T16:49:07Z

#374
#379
@skonto
I have added the support to specify the additional manifests.
Isn't it this PR a repetition?

skonto · 2021-02-12T23:01:10Z

@houshengbo hi, from a ux perspective and given the number of platform specific yaml files to maintain it is not. My scenario is not about exposing extra manifests which are accessed by some url etc. I dont want these files exposed, to be part of the crd contract. I have already mentioned why from my point of view it is not a repititon in the description. Btw from a vendor perspective this PR adds to the existing stuff.

matzew · 2021-02-15T15:03:59Z

pkg/reconciler/common/extensions_test.go

@@ -26,6 +26,14 @@ import (

 type TestExtension string

+func (t TestExtension) Manifests(v1alpha1.KComponent) ([]mf.Manifest, error) {
+	manifest, err := mf.NewManifest("testdata/kodata/additional-manifests/additional-sa.yaml")


So in my custom extension, I can add new custom manifests by calling this. Only on code, right? not via Yaml?

Should we document that? Or at least capture this as a docs issue? Happy to help - I already have a use-case for this feature ;-)

@matzew there is already a way to add custom manifests via the CRD (yaml) check @houshengbo pointers above ;)
If you want to add the manifests via code without letting them being exposed to the user (so they cannot be subject to illegal change) you can do it with this PR. What else should we allow here? Do you have any pointers for the documentation? Feel free to add whatever you think would be useful ;)

matzew · 2021-02-15T17:07:49Z

If you want to add the manifests via code without letting them being exposed to the user (so they cannot be subject to illegal change) you can do it with this PR.

+1 that's exactly what I want!

/lgtm

leaving approval to others

houshengbo · 2021-02-15T18:16:58Z

@skonto Are you leveraging the capability of the platform-specific extension?
Do you have an example of how to use this capability?
Knative lacks of docs on that aspect.

skonto · 2021-02-15T18:52:19Z

@houshengbo yes check my link in the description, my plan is to create all the service monitor resources via a blueprint that is loaded from a local path and create the manifests dynamically by defining that manifests function like in the transform case.

houshengbo · 2021-02-16T14:26:43Z

pkg/reconciler/knativeserving/knativeserving.go

@@ -109,7 +109,11 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1alpha1.KnativeServ
 		common.CheckDeployments,
 		common.DeleteObsoleteResources(ctx, ks, r.installed),
 	}
-	manifest := r.manifest.Append()
+	platformManifests, err := r.extension.Manifests(ks)


@skonto plz update your local source code and do a rebase.
Your code is surely obsolete for now.

Another remark: as in the stages, we load the core manifests first, then the ingress manifests(you should have them after the rebase). And probably the extended manifests. It is order-critical. The one loaded first will be applied first, the one loaded in the end will overwrite the same ones before.
The question is if we want to make sure the extended manifest go over the core, we need to load them in the order: core first, ingress manifest second, and lastly the extended manifest.
But for the logic in this PR, it seems to be extended manifest first, core second... Considering changing the order.
You can create a function similar to common.AppendTarget for extension, and add it in the stage.

func (r *Reconciler) appendExtension(ctx context.Context, manifest *mf.Manifest, instance v1alpha1.KComponent) error { platformManifests, err := r.extension.Manifests(instance) if err != nil { return err } *manifest = manifest.Append(platformManifests) return nil }

stages := common.Stages{ common.AppendTarget, ingress.AppendTargetIngresses, r.filterDisabledIngresses, r.appendExtension, r.transform, common.Install, common.CheckDeployments, common.DeleteObsoleteResources(ctx, ks, r.installed), }

thanks @houshengbo will take a look.

houshengbo · 2021-02-16T14:27:35Z

pkg/reconciler/knativeeventing/knativeeventing.go

@@ -106,7 +106,11 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ke *v1alpha1.KnativeEven
 		common.CheckDeployments,
 		common.DeleteObsoleteResources(ctx, ke, r.installed),
 	}
-	manifest := r.manifest.Append()
+	platformManifests, err := r.extension.Manifests(ke)


plz check my comment below for the serving. it applies here as well.

skonto · 2021-02-17T12:42:36Z

/hold

skonto · 2021-02-17T12:44:58Z

/unhold

matzew · 2021-02-17T13:04:01Z

pkg/reconciler/knativeserving/knativeserving.go

@@ -108,6 +108,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1alpha1.KnativeServ
 		common.AppendTarget,
 		ingress.AppendTargetIngresses,
 		r.filterDisabledIngresses,
+		r.appendExtension,


should we name it appendExtensionManifests or the like?

We can do that in a different PR, but just wondering, since it's not appending the extension at whole, right ?

knative-metrics-robot · 2021-02-17T13:10:09Z

The following is the coverage report on the affected files.
Say /test pull-knative-operator-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/common/extensions.go	100.0%	80.0%	-20.0

matzew

/lgtm
/approve

knative-prow-robot · 2021-02-17T13:11:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow-robot requested a review from markusthoemmes February 12, 2021 14:32

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Feb 12, 2021

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 12, 2021

skonto changed the title ~~Adds support for custom manifests~~ Adds support for extension custom manifests Feb 12, 2021

knative-prow-robot reviewed Feb 12, 2021

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 12, 2021

markusthoemmes reviewed Feb 12, 2021

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 12, 2021

matzew reviewed Feb 15, 2021

View reviewed changes

knative-prow-robot assigned matzew Feb 15, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2021

houshengbo reviewed Feb 16, 2021

View reviewed changes

skonto added 4 commits February 17, 2021 11:42

add support for custom manifests

1fa4de2

fix style

7b55096

updates

3afeeaf

updates

83dca46

skonto force-pushed the add_custom_manifest branch from c89398c to 83dca46 Compare February 17, 2021 09:55

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2021

fix

f8b2428

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2021

matzew reviewed Feb 17, 2021

View reviewed changes

fix

e094acb

matzew approved these changes Feb 17, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2021

knative-prow-robot merged commit 353589d into knative:master Feb 17, 2021

skonto mentioned this pull request Feb 17, 2021

[RELEASE-0.20] Add support for custom manifests #481

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support for extension custom manifests #468

Adds support for extension custom manifests #468

skonto commented Feb 12, 2021 •

edited

Loading

knative-prow-robot left a comment

knative-prow-robot commented Feb 12, 2021

knative-prow-robot commented Feb 12, 2021

markusthoemmes Feb 12, 2021

skonto Feb 12, 2021

markusthoemmes Feb 12, 2021

houshengbo commented Feb 12, 2021

houshengbo commented Feb 12, 2021

skonto commented Feb 12, 2021 •

edited

Loading

matzew Feb 15, 2021

skonto Feb 15, 2021 •

edited

Loading

matzew commented Feb 15, 2021

houshengbo commented Feb 15, 2021

skonto commented Feb 15, 2021 •

edited

Loading

houshengbo Feb 16, 2021

houshengbo Feb 16, 2021

skonto Feb 16, 2021

houshengbo Feb 16, 2021

skonto commented Feb 17, 2021

skonto commented Feb 17, 2021

matzew Feb 17, 2021

skonto Feb 17, 2021

knative-metrics-robot commented Feb 17, 2021

matzew left a comment

knative-prow-robot commented Feb 17, 2021

		@@ -39,6 +40,9 @@ func NoExtension(context.Context) Extension {

		type nilExtension struct{}

		func (nilExtension) Manifests() []mf.Manifest {

Adds support for extension custom manifests #468

Adds support for extension custom manifests #468

Conversation

skonto commented Feb 12, 2021 • edited Loading

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow-robot commented Feb 12, 2021

knative-prow-robot commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

houshengbo commented Feb 12, 2021

houshengbo commented Feb 12, 2021

skonto commented Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

skonto Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

matzew commented Feb 15, 2021

houshengbo commented Feb 15, 2021

skonto commented Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Feb 17, 2021

skonto commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Feb 17, 2021

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 17, 2021

skonto commented Feb 12, 2021 •

edited

Loading

skonto commented Feb 12, 2021 •

edited

Loading

skonto Feb 15, 2021 •

edited

Loading

skonto commented Feb 15, 2021 •

edited

Loading