Improved KEP-0017 design

[zen-dog]

What this PR does / why we need it:

This is a minor change to KEP-0017 by moving the pipe section to the tasks instead of it being at the level of individual resource. This aligns much better with our goal to encapsulate different types of tasks in the future.

Related: #774

[gerred]

Is there a useful Go interface here for different task types? Visitor or something, and then the main control loop's job is to Visit each type of resource (with some general context/info), leaving it up to different task types to define their implementations.

[gerred]

I know we don't need to change that much right now, but it IS relevant here in that you will eventually have to detect this resource type and stuff gencert.yaml into an initContainer, already breaking the resource pass-through effect we assume in the controller right now.

[zen-dog]

Ok, so I reverted my changes, embracing the schema @alenkacz proposed below:

tasks:
  deploy:
    pipe:
      file: xxx.txt
      kind: Secret
      key: {{Key}}
    resources:
      - gencert.yaml

This way resources stay simply resources and the abstraction moves to the task level. Additional task types will be represented by having new blocks on the pipe level e.g.:

tasks:
  deploy:
    pipe:
      ...
    outputVars:
      ...
    moreTypes:
      ...
    resources:
      - gencert.yaml
---
type TaskSpec struct {
	Resources []string
	Pipe PipeSpec
	OutputVars OutputVarsSpec
        ....
}

where only one of the block can be != nil similar to e.g. Protobuf Oneof pattern. If none are set, the resources list is used as-is.

[gerred]

I think this is ok for now but is going to lead us in a fairly complicated direction - the code for actually running a task is going to get larger for every task type, and we will have to switch on what "type" of task it is there to figure out what to do in the controller.

A nice alternative here would be to have something like:

type Tasker interface { Run() error }

and then have a struct for PipelineTaskSpec, ResourceTaskSpec, OutputVarTaskSpec, etc., with just the members they need.

Yes, you will still need to coerce this from the YAML, but now you're doing that as part of the parsing step and can seal it off, while making the running control loop a lot more clean (iterate through a bunch of Taskers and Run them).

[gerred]

BY the way, this also allows in the future for even more interesting things, like a type of MultiTaskSpec, which could contain other Taskers. In that world, it's just another type of Tasker that contains Taskers.

[gerred]

we need to figure out versioning our operator repo so we can make changes like this a lot less disruptive once we have more operators

[alenkacz]

I've just left one fresh morning idea that I had when reading this :) will continue the review based on what we all think about it...

[alenkacz]

I still feel kind of weird about the fact that pipe is property of resource, it's in my head still kind of a task property when I think about it and now knowing the implementation... You want to define task and that task is piping something to somewhere...

When I was thinking how to approach that I was kind of thinking if we should not rather create a special type of task for pipes, these tasks would have to have one resource (that anyway has to look totally different than any other resource, because it's not full k8s resource) and then the pipe will be property of that Task... Basically Task would either be

tasks:
  deploy:
    resources:
      - service.yaml
      - pdb.yaml
      - configmap.yaml
      - metrics-config.yaml
      - health-check.yaml
      - statefulset.yaml

or

tasks:
  deploy:
    pipe:
      file: xxx.txt
      kind: Secret
      key: {{Key}}
    resources:
      - gencert.yaml

Then we would have a simple validation that if pipe is inside task, only one resource can be present. That way we don't even have to break the API. And from user perspective it feels more natural to me because its a pipe TASK not a resource in a task...

Open to hear what you guys think :)

[zen-dog]

Yeah, as you can see, my initial proposal has pipe being a property of the task and not the resource. However, that would've created this limitation, where a task can only have this one ContainerSpec resource (which I'm ok with). A similar issue exists with e.g. output variables which are also per resource and not per task. And as @gerred mentioned above, having tasks/resources of different type bring the problem of polymorphic unmarshaling.

That all being said, I still like your proposal - pipe on the task level makes more sense than on the resource level:

tasks:
  deploy:
    pipe:
      file: xxx.txt
      kind: Secret
      key: {{Key}}
    resources:
      - gencert.yaml

Will adjust the PR 😉

[gerred]

I'm ok with this too, it's a lot simpler and the restriction seems ok to me. Need some resources? Use it as another task! That way a task does one thing and one thing only.

[zen-dog]

Fun fact: going to the above-described schema, will basically mean, reverting this whole PR 🎉 So, I'm not going to revert all code changes, leaving only KEP changes for further discussion.