Support the platform-specific or custom transformers

[jihuin]

Proposed Changes

User can attach platform-specific or custom transformer functions into the context in main.go to perform a platform-specific or custom transform.

The minikube-operator-example directory gives an example of building platform-specific operator by injecting the platform-specific transformers.

Release Note

NONE

[jihuin]

/assign @trshafer

[jcrossley3]

I was kinda thinking we'd remove all the "platform" stuff since it's not really practical to compile all the possible platforms into this operator. That was the motivation behind #185. I still think that approach is viable as we've done quite a bit of work on the OpenShift-specific operator.

Did you have something specific in mind that triggered this PR?

[jihuin]

@jcrossley3 I'm trying to perform some gke specific transforms defined in a separate repo. With this PR, the transformer functions can be attached to the context in main.go, and then passed to the controller.

The idea is similar to https://github.com/knative/serving-operator/pull/185#issuecomment-540714673:

In order to implement a platform-specific operator, one would import the operator library and make use of various hooks to add transforms, provide status overrides, etc.

[jcrossley3]

Fair enough. But now I'm surprised the minikube-specific stuff is still in this PR. Might it be a good candidate to test your theory? Not sure how involved your GKE operator is, but the minikube stuff is pretty simple to extract.

[jihuin]

The minikube-specific stuff is still here because I don't want to change the current behavior that the minikube is supported out-of-the-box. There are two other options:

1. Always attach the minikube-specific transformer to the context in the main.go, so minikube is still supported by default and there is no need to explicitly append the minikube transformer to the Reconciler.platform.

2. Remove the minikube-specific transformer so that minikube users need to attach it by themselves.

What do you think? @jcrossley3

[jcrossley3]

What do you think? @jcrossley3

I don't think we should have any platform-specific code in this operator, and that goes for minikube, too. However, I don't think we should just remove it, effectively breaking existing minikube users. Instead we should move it. That's what #185 did.

It may live in another repo, but we need to be clear about what users need to do to successfully run knative-serving on minikube.

[trshafer]

I think the gist of this is that different platform specific operators will want hooks in various parts of the reconcile loop. I think the platform object is a good place for this logic. Possible places I can think to add hooks and their signatures are:

1. Beginning of reconcile loop (): error
2. After fetching KnativeServing (KnativeServing): (error, KnativeServing)
3. Transforming the manifest (mf): (error, mf)
4. Validation that resources were successfully created (mf, client?): error
5. Ending the reconcile loop (): error

For Minikube, this might be a good place for a second repo or a directory in this repo that shows how to create a new top level operator which uses the serving-operator controller with these hooks.

[jihuin]

/retest

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[jihuin]

Update: move the minikube folder outside of pkg directory, which acts as an example of how to inject platform-specific transformers. @jcrossley3 @trshafer

[trshafer]

This looks good to me. Putting a hold for @jcrossley3 thoughts.

/hold /lgtm

[jcrossley3]

Personally, I'd prefer to see the example in a separate repo. That would prevent the use of symlinks for the config/ files. Maybe vendoring those files would enable you to use them as they are, though, i.e. not have to add "minikube" references in some of them. Only the image in operator.yaml would need to change maybe? Not sure.

Feel free to override me if others like it in here, though.

[trshafer]

I think it's more likely the minikube example will become stale if it's in a separate repo. I agree that vendoring the knative-serving operator is likely the better approach to demonstrating the extensibility of the operator. I don't think it's likely that we will release knative, then release the operator, then update the minikube operator. I don't have strong opinions. I think it makes sense to keep for now, unblock the transformation change, and revisit in the next WG meeting.

[jcrossley3]

I think the potential for the separate repo going stale should be a factor in evaluating this approach. :smile:

[trshafer]

IMO, the main thing this PR does is support passing in Platform via the context. This also takes advantage of the new feature and allows for minikube to be separated out. Extracting minikube out of the controller code is the first step. Pulling minikube controller out into a separate repo is not made more difficult by starting off putting it in an example directory. It would be helpful to be able to merge the feature change in to unblock using that transformation feature (the PR in its current form). We bring up extracting the example into a separate repo after this feature is available. Does that work for you?

[jcrossley3]

IMO, the main thing this PR does is support passing in Platform via the context.

I agree. That's the signal, and the minikube example is mostly noise. I'm only bothered (and only slightly) by the inclusion of the example, as it reduces the S/N ratio of the PR, imo. I think the signal would be better amplified by a simple unit test rather than an entire minikube example.

I'm not trying to be difficult. I don't feel all that strongly about it either. I'm just ready to remove all references to minikube from this repo. :)

[trshafer]

Cool cool. I think we're in agreement. I agree with https://github.com/knative/serving-operator/pull/295#issuecomment-585880207 that we shouldn't just remove minikube because it's not clear without an example (here or in another repo) how users would really use the operator in minikube. Since we have the TOC this week, we can propose another repo for minikube there. Added to the agenda for Feb 20. Agree that unit tests are a great way to demonstrate the the public api of platform specific config.

@jihuin can you fast follow with a unit test, since there aren't tests right now? If yes, I'll approve.

[jihuin]

Sure. I will add the unit test soon.

[trshafer]

/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jihuin, trshafer

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](https://github.com/knative/serving-operator/blob/master/OWNERS)~~ [trshafer] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[trshafer]

/remove hold

[trshafer]

/cancel hold

[trshafer]

/hold cancel