[TEP-0141] Platform Context Variables -- implementable

[Yongxuanzhang]

This commit adds a design for allowing platforms to inject "context" variables as PipelineRun and TaskRun parameters.
/kind tep

[JeromeJu]

WG /assign @pritidesai

[Yongxuanzhang]

Hi @afrittoli @pritidesai, it would be great if you could take a look at this section first? I want to get some early feedback on whether this proposed solution makes sense, or we need to make some changes, or seek alternatives.
Thanks!

[afrittoli]

/assign

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from afrittoli after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

• teps/OWNERS

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

[pritidesai]

is the proposal about introducing a new section called context in pipelineRun.spec to define platform context? How is this different from a param section i.e. can a param repo-url reference a context variable without defining it in a context section?

Are there any pre-defined context variables available for the users to run with? for example, as a platform provider, I have defined (somewhere out side of a pipelineRun) a few options for the users to choose from. The available options are:

STORAGECLASS-A: class-A,
STORAGECLASS-B: class-B,
STORAGECLASS-C: class-C

Will the pipeline author be able to utilize STORAGECLASS-A variable and tekton controller will be able to replace this with the right value - class-A?

[Yongxuanzhang]

is the proposal about introducing a new section called context in pipelineRun.spec to define platform context?

Yes. The field is used to accept the platform context. We add this field to PipelineRun.Spec, and platform can populate the values when creating the PipelineRun

Are there any pre-defined context variables available for the users to run with?

Yes. And I think this is up to the platforms if they want to have pre-defined values. They need to add code to populate those values to pipelinerun, and users can use $(context.platform.STORAGECLASS-A) to use class-A.

$(context.platform.STORAGECLASS-A) the syntax can also be customized by the platform. I added an optional section for this feature

[vdemeester]

I kind-of share the same question as @pritidesai. If the value of the context.platform variable has to be specified in the PipelineRun spec, what's the benefit over just using params only ?
For me, those values should be populated outside of the PipelineRun definition.

[Yongxuanzhang]

I see, how about putting it our of spec, and making it like this:

apiVersion: tekton.dev/v1
kind: PipelineRun
metadata:
  name: clone-kaniko-build-push-run
context:
  params:
  - name: repo_url
    value: https://github.com/tektoncd/pipeline
  - name: image_registry
    value: gcr.io/user
spec:
  pipelineRef:
    name: clone-kaniko-build-push
  params:
  - name: repo-url
    value: $(context.platform.repo_url)
  - name: image
    value: $(context.platform.image_registry)/myapp

what's the benefit over just using params only

The benefits are: 1) platform params won't have conflicts with users' params 2) provenance generator(Chains) would be able to tell which are internal params (platform params), which are external params (users params)

[pritidesai]

where is repo-url context defined?

[Yongxuanzhang]

That's a great question. It is not defined in Pipeline since it is a platform variable, so the pipelines used in this platform can reference to the repo_url directly. The feature is not designed for reusable tasks, pipelines, and I have mentioned this in the design evaluation section

[pritidesai]

what happens when a string parameter references an array context?

[Yongxuanzhang]

It's the same as current params? A string param references an array param will have validation errs

[pritidesai]

are there any additional way of providing context instead of or in addition to pipelineRun.context?

[Yongxuanzhang]

Sorry I may not understand this question. Are you asking alternatives of the proposed solution?

[vdemeester]

As an exercise to reduce this proposal to the smallest TL;DR I could come up with: it's essentially a way to pass another set of parameters, but covered by RBAC (aka only allow a subset of users to use that field), am I right ?

I really think this should be limited to runtime types (aka not able to refer to context.plaftorm variable from Pipeline or Task definition).

Other possible concern: What about the "size" of the object ? What if the amount of context.platform parameters one "builder" adds make the PipelineRun too big to be created ?

I think we should experiment with a project like #1083 and this approach to validate that our thoughts match the usage.

[vdemeester]

Naive question, how does a user "discover" the context.platform variable available ?

[Yongxuanzhang]

Via documentation, the vendors who want to use this would use documentation to inform users the available list of context params

[vdemeester]

I kind-of share the same question as @pritidesai. If the value of the context.platform variable has to be specified in the PipelineRun spec, what's the benefit over just using params only ?
For me, those values should be populated outside of the PipelineRun definition.

[vdemeester]

I do think this would be a mistake. As said below, this make Pipeline less/not portable. It's just there to remove one (or more) parameters (explicit) and use an implicit value that might not even be there, right ?

[Yongxuanzhang]

Why this is a mistake? This sub-feature is not designed for reusable Pipeline/Task (e.g. catalog tasks which can be used by anyone), it won't affect current reusability.

And it will add more simplicity for users who need them. Vendors will tell users this is a list of params you can use on their platform, so users can create their pipelines/tasks under the platform's context.

How about we reach this consensus: The feature is not designed to be shared across platforms.

[vdemeester]

How about we reach this consensus: The feature is not designed to be shared across platforms.

Yes, but we can limit the effect it has. If we allow to use those at Pipeline/Task definition time, we essentially allow relying on implicit value and implicitely allow "lock-in" of Pipeline or Task for a given vendor. And I do believe this (encouraging users to uses those in definition types) would be a mistake. Task / Pipeline should be as runtime-free (and vendor-free) as possible.

[vdemeester]

I am definitely worried about that as well. It's giving way too much option to refer to those. Imaging reviewing the failing PipelineRun from a user, that is using these kind of customization. You would need to be aware of a lot of context in addition to the spec itself.

[Yongxuanzhang]

If we want to propose a general solution, and different platforms may want to have different syntax (they don't like the $(contxt.platform.foo)) How can we support that?

It's just giving option to platforms not end users to use a unified syntax, and users can review the runtime values of them in the PipelineRun.Context.Params

Platform can choose whatever syntax they want, as long as it doesn't conflict with our current variable replacements.

Anyways I think this is an optional thing, if we don't want it I'm happy to remove it from the proposal.

[vdemeester]

@Yongxuanzhang but this would mean, I could write a Pipeline with a variable called $(foo.is.bar.toto) and be very dependent on where it runs.

[vdemeester]

"customized syntax" refers to what pipelineascode does, right ? (from here)

[Yongxuanzhang]

I'm not sure, I guess so. I basically copied most of the contents from Lee's original proposal. 😄
I think if we choose to propose $(context.platform.foo), it indeed adds the simplicity. (I can remove my customized syntax option since it conflicts with this :/)

[vdemeester]

Configuring connected repository (aka creating the Repository CRD) is not necessarily the responsability of a cluster operator.

[vdemeester]

Note that, technically, this proposal is doable toady, isn't it ?

[Yongxuanzhang]

Yes, it just have more cons

[chmouel]

We can support this in Pipelines as Code for the simple key/value we provide (ie: standard params as per screenshot)

We do have advanced use case we are introducing in the form of CEL expressions inside the expressions

openshift-pipelines/pipelines-as-code#1466

which i'd think would be useful for platform variable as well.. altho may need an API change we want to consider here (i.e: a type field which in this case would be cel and evaluate as CEL expression)

We need as well being able to access those variables replaced from anywhere, I think there is some limitation currently where the params substitution occur on pipelinerun?

We probably want to specify what happen after remote resolver resolution occured, for example for PaaC we need to be able to do this when we are using the remote pipeline feature or remote task if there is $(context.platforms.*) variables they would be expanded after resolution..

for the foreseeable future we would have to support both form from our side but if we get features parity with platform context variable we would try to slowly make users migrate to it by generating context variable from our {{ }} variables

let me know if you want me to expand on any of those point since i am guessing it's a comment with a lot of different topics.

[Yongxuanzhang]

Thanks for the suggestions and information!

We do have advanced use case we are introducing in the form of CEL expressions inside the expressions

Happy to add CEL if this proposal makes sense!

We need as well being able to access those variables replaced from anywhere, I think there is some limitation currently where the params substitution occur on pipelinerun?

Yes, this tep proposes that users can access those variables anywhere, the same as current params. I don't think we want the limitation to only use in pipelinerun

We probably want to specify what happen after remote resolver resolution occured

After the resolution, those $(context.platforms.*) variables would be replaced. I think we mentioned this in the tep's design details section.

for the foreseeable future we would have to support both form from our side but if we get features parity with platform context variable we would try to slowly make users migrate to it by generating context variable from our {{ }} variables

It's totally up to the vendors who want to adopt this feature. For vendors who don't features like pipeline-as-code, they can use this feature to build their own params populate logic. I don't know if it makes sense for pac users to migrate to this feature.

Since it is also gated by feature flag, and the api is omit empty, it won't affect any current users if it is disabled.

[chmouel]

At first when reading this (initial proposal) TEP I was confused between this and implicit params:

https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md#design-details

I think we may want to explain to our users when to use context and when to use implicit params, if context is supposed to be something set by the user in her PipelineRun or something that is only going to be added by platform builders that sit on top of the PipelineRun creation (i.e pac and others).

additionally and not asking for a change because i know that naming is hard, context is a over used term that may mean different things, it's really context parameters in this case.

[Yongxuanzhang]

As an exercise to reduce this proposal to the smallest TL;DR I could come up with: it's essentially a way to pass another set of parameters, but covered by RBAC (aka only allow a subset of users to use that field), am I right ?

I really think this should be limited to runtime types (aka not able to refer to context.plaftorm variable from Pipeline or Task definition).

Other possible concern: What about the "size" of the object ? What if the amount of context.platform parameters one "builder" adds make the PipelineRun too big to be created ?

I think we should experiment with a project like #1083 and this approach to validate that our thoughts match the usage.

Thanks for the reviews!

it's essentially a way to pass another set of parameters, but covered by RBAC (aka only allow a subset of users to use that field), am I right ?

RBAC is mainly used to restrict the updating permissions, so only platform controller can update those fields, make sure that other roles in the cluster don't have the permission. But users would be able to access those params via our variable substitution, same as current params.

I really think this should be limited to runtime types (aka not able to refer to context.plaftorm variable from Pipeline or Task definition).

What's the reason for that? We're hoping that this context param can do the same variable substitution as current params, so it would be great if we can support using them in pipeline and task

Other possible concern: What about the "size" of the object ? What if the amount of context.platform parameters one "builder" adds make the PipelineRun too big to be created ?

Do we currently have limitation on Params size? If we do then we can apply the same to the context.params. If we do not, then we can address them in a separate issue if that is a concern.

I think we should experiment with a project like #1083 and this approach to validate that our thoughts match the usage.

I see, I'm happy to do that if the proposal makes sense to you

[Yongxuanzhang]

At first when reading this (initial proposal) TEP I was confused between this and implicit params:

https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md#design-details

I think we may want to explain to our users when to use context and when to use implicit params, if context is supposed to be something set by the user in her PipelineRun or something that is only going to be added by platform builders that sit on top of the PipelineRun creation (i.e pac and others).

additionally and not asking for a change because i know that naming is hard, context is a over used term that may mean different things, it's really context parameters in this case.

Thanks!! It's very different from the implicit params(now it is tep107, propagate params), I know the naming are very similar! The implicit params or propatating params are used for inline pipelinespec, taskspec, that users don't need to specify params in their pipelinespec and taskspec.

And users cannot set the proposed params, the update permission is restricted to platform via RBAC. So it is going to be added by platform builders that sit on top of the PipelineRun creation

[vdemeester]

What's the reason for that? We're hoping that this context param can do the same variable substitution as current params, so it would be great if we can support using them in pipeline and task

So the worry to have it in definition types (Task and Pipeline) is that they are then dependent on the platform they run in (kind-of vendor lock-in 😛). And I don't see any use case where this would be a requirement. One can easily set a parameter that will get populated, at runtime, by that context.platform value. There is a clear cut between definition types and runtime for a reason, and Task params and workspace being as explicit as they are is by design, to not have implicit behavior, variable, … that othe CI usually have and imply (making what runs in those CI a bit vendor lock-in).

[Yongxuanzhang]

What's the reason for that? We're hoping that this context param can do the same variable substitution as current params, so it would be great if we can support using them in pipeline and task

So the worry to have it in definition types (Task and Pipeline) is that they are then dependent on the platform they run in (kind-of vendor lock-in 😛). And I don't see any use case where this would be a requirement. One can easily set a parameter that will get populated, at runtime, by that context.platform value. There is a clear cut between definition types and runtime for a reason, and Task params and workspace being as explicit as they are is by design, to not have implicit behavior, variable, … that othe CI usually have and imply (making what runs in those CI a bit vendor lock-in).

Thanks! This is actually our use case 😢 .

One can easily set a parameter that will get populated, at runtime, by that context.platform value.

This is complex to users and we want to avoid that. I know there are people like them, and some not. That's one reason we have TEP-107 for inline spec. I know the case we're discussing here is remote tasks and pipelines. But they kind like share the similar purpose

It is true that this is a bit vendor lock-in because this feature is designed for vendors. Vendors have the option to use or not use this feature. Some vendors want to have the implicit variables in pipelines and tasks (convenient for end users) and they won't share them to other vendors.

[vdemeester]

Thanks! This is actually our use case 😢 .

Can you describe the use case in more detail ? What make it "hard" to have to define explicit parameters for a Pipeline ?

This is complex to users and we want to avoid that. I know there are people like them, and some not. That's one reason we have TEP-107 for inline spec. I know the case we're discussing here is remote tasks and pipelines. But they kind like share the similar purpose

For me this is explicit, this might be verbose, but this is not complex. It is easy to understand, easy to follow, it's just verbose. Propagating parameters is a different "beast", it is there to reduce the duplication, on embedded task (because all is defined at the same place), but it doesn't do anything on usual Task reference. It doesn't "change" the "clear cut" between runtime and definition.

It is true that this is a bit vendor lock-in because this feature is designed for vendors. Vendors have the option to use or not use this feature. Some vendors want to have the implicit variables in pipelines and tasks (convenient for end users) and they won't share them to other vendors.

Is there anything vendor specific in Pod definition ? Or vendor specific things are coming from "elsewhere" ?

If a vendor would like to apply this proposal today, how would it do ? Probably writing an admission controller that looks at the definition, and replace $(my-vendor.foo) with some actual value right ? Do we want to make this easier to do for the vendor ? My opinion is not necesarrily, maybe I am wrong 😝. But if we do, I really want to limit this to the runtime part, because this is the part "close" to the vendor.

[QuanZhang-William]

I'm a bit confused, which clone-kaniko-build-push pipeline do you mean here? The one in the "Existing Workaround" or "TaskRun and PipelineRun syntax"?

Maybe it is a good idea to split inline and PipelineRef/remote pipelines scenarios to different subsections to make it easier to follow.

[QuanZhang-William]

I'm not sure why do we call it $(context.platform.foo)? Based on the below syntax and usage, I feel something like context.param.foo/ platform.param.foo could be better?

[Yongxuanzhang]

I think it is because our existing context variables, e.g. $(context.taskRun.name). So it has the context.LEVEL.NAME format

[tekton-robot]

@Yongxuanzhang: PR needs rebase.

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.