Owning our *Container* spec

[vdemeester]

Feature request

As of today, the TaskSpec Step is embedding the k8s "upstream" Container spec. The "possible" idea here would be to migrate to owning / defining our own Container spec instead. This would allow to control some of the "leaks* that comes from k8s in our types. For example, in the buildkit-tekton experimentation, there is a few fields of the Container struct we silently ignore because they don't make sense in a "CI" workflow type.

		// Silently ignore Ports
		// Silently ignore LivenessProbe
		// Silently ignore ReadinessProbe
		// Silently ignore StartupProbe
		// Silently ignore Lifecycle
		// Silently ignore TerminationMessagePath
		// Silently ignore TerminationMessagePolicy
		// Silently ignore Resources (for now)
		// Silently ignore ImagePullPolicy
		// Silently ignore Stdin
		// Silently ignore StdinOnce
		// Silently ignore TTY

In addition it would make it "cleaner" on openapi schema generation (because we don't get all the fields).
One possible downside might be "duck typing" as, now, the Task spec might not be as "PodSpecable" that it could.

If we want to go that way, we should do it before v1, otherwise, we can ignore that issue 😛.

/cc @tektoncd/core-maintainers @mattmoor

Use case

N/A, it's more a "technical" feature request / open question/discussion

[pritidesai]

I have a limited understanding of the downside here (because of a limited knowledge of duck typing 😆 ).

Anyways, I like the idea of having our own Container spec as it provides us flexibility in introducing pipeline/task specific fields which might not be relevant/available in upstream Container spec. It provides us extensibility and decouples from the k8s resources.

[pritidesai]

/cc @lbernick pinging Lee here since we had a similar discussion two weeks back to introduce our own custom Container spec.

[lbernick]

I like the idea of owning our own container spec since it could help with issues like #4080. I think we could make this change without breaking existing working yamls but it would break the go client libraries. Priti pointed out that there is precedence for breaking go libraries w/out breaking yamls in #2826.

I don't think we should remove the container fields that can't be implemented in buildkit since users may be depending on them, and our API spec doc states that those fields aren't required for tekton conformance.

I know what duck typing is but I don't quite understand what you're saying about it here, could you elaborate?

[imjasonh]

I swear there was an issue for this, related to changes we wanted to make before v1, but I can't find it. In any case, strong +1 from me.

Fully owning the Step type means not having weirdly silently ignored fields (or mysteriously have weird behavior on, like terminationMessagePath), but also means future additions to v1.Container that might have meaning in Tekton aren't immediately adopted and surfaced to users.

Task spec might not be as "PodSpecable" that it could.

If this is a concern, we might want to recommend that Knative limits and owns its own PodSpecable definition, since it currently just reuses the core/v1 type too, which might become problematic if e.g., Deployment and DaemonSet ever meaningfully diverge in their definitions of a "pod template".

[vdemeester]

I don't think we should remove the container fields that can't be implemented in buildkit since users may be depending on them, and our API spec doc states that those fields aren't required for tekton conformance.

This is however why I tend to want to own our Container spec, to shrink it to remove all the things that are not useful for a Task container. The fact that builkit-tetkon (I should refer to it as "local execution to make sure there is no confusion with buildkit itself) ignore them (and safely) is mainly showing they have no use in the tekton task use cases (and thus I have a 90% confidence they are not needed). Ports or LivenessProbe, etc are a good example, but the list above is not what I think we should not have in Container spec for sure, it's just an example of things (but Resource, and other would make sense to still have).

I know what duck typing is but I don't quite understand what you're saying about it here, could you elaborate?

I was thinking specifically about : https://github.com/knative/pkg/blob/main/apis/duck/v1/podspec_types.go, but it turns out, today's Task spec doesn't satisfy this anyway. (https://github.com/knative/pkg/tree/main/apis/duck#duck-types for more info on duck typing in the knative world)

[lbernick]

It looks like we haven't defined a compatibility policy for our go libs (#2748) but if we decide we're ok with a breaking change to the libraries we could probably just swap to our own struct without waiting for v1 (removing the fields you're mentioning would ofc be a breaking change to the API though). I would agree that fields like LivenessProbe probably aren't relevant to tekton.

[vdemeester]

It looks like we haven't defined a compatibility policy for our go libs (#2748) but if we decide we're ok with a breaking change to the libraries we could probably just swap to our own struct without waiting for v1 (removing the fields you're mentioning would ofc be a breaking change to the API though). I would agree that fields like LivenessProbe probably aren't relevant to tekton.

Right, we can either do this for v1 and not before or do it in v1beta1 as well. The benefit of doing it only for v1 is that, consumer of the go libs will have other potential "breaking" changes in supporting the v1 API anyway, and this one would be "just one more". But I would be fine with both.

[pritidesai]

I vote for implementing this in v1 for the same reasons you have mentioned. Implementing this in v1beta1 will be easier and quicker but can break the consumers including dashboard/CLI/others.

[vdemeester]

/assign @lbernick
I think we can mark this as done, right ?

[lbernick]

I think we can mark this as done, right ?

I was thinking we could leave it open to track marking the unnecessary fields as deprecated?

[vdemeester]

sounds good 👍🏼

[lbernick]

@pierretasci mentioned that some tools like maven behave differently depending on whether or not there is a tty attached to the container. I still would like to understand this a bit better but should we consider bringing TTY back into the step definition?