-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Promote Remote Resolution to beta #4710
Comments
Potential for integrating remote resolution as a direct feature of Tekton PipelinesThis comment outlines improvements to the system design of remote resolution if Pipelines directly integrated the functionality into its codebase. Simpler codeA lot of resolution's codebase exists to provide abstractions and helpers that hide the resolution specifics from clients (like Pipelines) and resolvers (like the git resolver). Examples of these are:
A lot of this code exists in service of the idea that Tekton Resolution could be leveraged by clients other than Tekton Pipelines. But without concrete examples of other clients this abstraction is unnecessary and probably just confusing. Integrating remote resolution's concepts directly into Pipelines would likely mean a lot of this could be removed entirely. ... and easier dependenciesCurrently Tekton Resolution shares a lot of the same dependencies as Tekton Pipelines. Examples include knative, go-containerregistry + k8schain, various k8s.io modules, etc. Since Tekton Pipelines depends on Resolution for some of its client code pulling in updates from resolution can cause a bit of churn in Pipeline's Re-using existing conceptsReduced to its essence a "resolver" is really the same thing as Pipeline's CustomTask concept. The only real differences are that resolvers have a fixed interface (the Given the similarity between Resolvers and CustomTasks it would make sense to eliminate Resolvers entirely (while still enforcing the specific CRD and helpful "sdk" parts that are useful to make remote resolution work). Duck typing would be another approach: register a shape of object that Pipelines will accept for remote resolution and allow resolvers to meet that shape however they want. Changes in system designHow resolution works todayCurrently Tekton Resolution maintains a "core" reconciler whose responsibility is to monitor all Here is approximately the order of operations today for a new sequenceDiagram
Tekton Pipelines->>ResolutionRequest: CREATE request for git resource
ResolutionRequest-->>Tekton Resolution: WATCH triggers reconcile
Tekton Resolution->>ResolutionRequest: UPDATE with initial status
ResolutionRequest-->>GitResolver: WATCH triggers reconcile
GitResolver->>Git Repo: Clone & get file contents
GitResolver->>ResolutionRequest: PATCH with contents of file from git
ResolutionRequest-->>Tekton Resolution: WATCH triggers reconcile
Tekton Resolution->>ResolutionRequest: UPDATE with Succeeded status
ResolutionRequest-->>Tekton Pipelines: WATCH triggers reconcile of pipelines
... and this is the simplest case where a How it could work if pipelines did remote resolution itselfIn a world where Tekton Pipelines merges remote resolution directly into its codebase, the basic flow of control would be simplified quite a bit: sequenceDiagram
Tekton Pipelines->>ResolutionRequest: CREATE request for git resource w/ initial status
ResolutionRequest-->>GitResolver: WATCH triggers reconcile
GitResolver->>Git Repo: Clone & get file contents
GitResolver->>ResolutionRequest: UPDATE with contents of file + Succeeded status
ResolutionRequest-->>Tekton Pipelines: WATCH triggers reconcile of pipeline
|
/assign |
Note: we also need to support Remote Resolution for |
Followup to tektoncd#4596, needed for tektoncd#4710. remote resolution, both in explicitly created `TaskRun`s and in `PipelineRun`s' `PipelineTask`s, from public git repositories using tektoncd/resolution. This is still in alpha. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Followup to tektoncd#4596, needed for tektoncd#4710. remote resolution, both in explicitly created `TaskRun`s and in `PipelineRun`s' `PipelineTask`s, from public git repositories using tektoncd/resolution. This is still in alpha. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Followup to tektoncd#4596, needed for tektoncd#4710. Enables remote resolution of `Task`s, both in explicitly created `TaskRun`s and in `PipelineRun`s' `PipelineTask`s, from public git repositories using tektoncd/resolution. Also modifies `resolution.Resolver` to take both owner and optional name/namespace. This is needed because we don't necessarily have a `TaskRun` yet when calling `GetTaskFunc` in the `PipelineRun` reconciler, but still need to ensure that we only make one remote resolution request for a `PipelineTask` via the `PipelineRun` and `TaskRun` reconcilers. Therefore, we must have the same deterministically-generated resolution request name in both places, using the predictable eventual `TaskRun` name and namespace. We also want to make sure that the created `ResolutionRequest` has the appropriate owner reference, hence still passing a `kmeta.OwnerRefable` to `NewResolver` as well as the name and namespace. This is still in alpha. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Followup to #4596, needed for #4710. Enables remote resolution of `Task`s, both in explicitly created `TaskRun`s and in `PipelineRun`s' `PipelineTask`s, from public git repositories using tektoncd/resolution. Also modifies `resolution.Resolver` to take both owner and optional name/namespace. This is needed because we don't necessarily have a `TaskRun` yet when calling `GetTaskFunc` in the `PipelineRun` reconciler, but still need to ensure that we only make one remote resolution request for a `PipelineTask` via the `PipelineRun` and `TaskRun` reconcilers. Therefore, we must have the same deterministically-generated resolution request name in both places, using the predictable eventual `TaskRun` name and namespace. We also want to make sure that the created `ResolutionRequest` has the appropriate owner reference, hence still passing a `kmeta.OwnerRefable` to `NewResolver` as well as the name and namespace. This is still in alpha. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Potential improvement for remote resolution: If you try to use remote resolution without both the resolution project and the correct resolver installed, your pipelinerun will just hang until it times out. I'm not sure if it's possible for the pipelinerun/taskrun reconcilers to know if you have resolution installed, or for the resolution reconciler to know if you have the correct resolvers installed, but if so it would be helpful to return an error in these cases instead. |
At the API WG call we discussed the possibility of integrating tekton resolution into pipeline. I dont think anyone at the WG expressed ideas against it. If anything, the team wanted to merge the two projects. Since many team members were away this week, the decision was not finalized (I still need to send out an email to the maintainers of |
This would definitely solve the problem of the user needing to install the resolution project separately, but it would still be nice to return an error if they are trying to use a resolver they don't have installed. |
I think this is not about where the code lives but how the api/controller behave. Today, if you submit a |
Yeah, |
Oh wait, https://github.com/tektoncd/resolution/blob/408cd52add36156c4cbbb0819404ea9daee1cfc2/pkg/reconciler/resolutionrequest/resourcerequest.go#L59-L61 - there is a global timeout of 1 minute currently for |
Ok, another thing Resolution needs - unit tests. The reconciler has none, so while I've verified that a |
Prompted by discussion on tektoncd#4710, though I should have thought of this earlier! This can't verify that the `ResolutionRequest` is handled correctly by its own reconciler, since that's still in a separate project (for now!), but at least we can be sure that the `PipelineRun` and `TaskRun` reconcilers will respond correctly to a failed `ResolutionRequest`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
I'm summarizing the action items here (cc @jerop @vdemeester @abayer @lbernick @dibyom @sbwsg): Feel free to suggest additional items and correct me if I've made an error summarizing the items 😄 . Criteria for Promotion
|
Some should probably (cluster task / pipeline, maybe the git resolver), but most might be better off living outside indeed.
I don't think it's about "informing" but just about "erroring" out properly in case we are trying to use a resolver that doesn't exists. We might even want to do this in the admission controller.
Do you mean "Remote resolution" here instead of Custom Task ? |
I agree.
Yes, I meant remote resolution. I updated it. |
@jerop and I chatted about the resolvers question on Friday - I'd advocate for moving the existing git and bundle resolvers into Pipeline, along with an eventual catalog resolver, since those all seem pretty fundamental. But additional resolvers going forward (like the GitHub-specific one I intend to write eventually, inspired by tektoncd/resolution#42) should, IMO, be outside the Pipeline repo. That does bring up another matter relating to the catalog - we do need a way to provide catalogs of custom tasks and resolvers in the catalog. But that's not relevant to Remote Resolution going to beta. =) |
Prompted by discussion on #4710, though I should have thought of this earlier! This can't verify that the `ResolutionRequest` is handled correctly by its own reconciler, since that's still in a separate project (for now!), but at least we can be sure that the `PipelineRun` and `TaskRun` reconcilers will respond correctly to a failed `ResolutionRequest`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Added this to the v1 project as I would really want to have this in beta at least so that we can deprecate |
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
I'm rather late to the discussion here, but I still wanted to mention that something that I think we should have in remote resolution is support for bundles, i.e. a way for a parent resource to use "relative references" for children, as long as they belong to the same bundle. That was one of the most requested changes to the OCI bundles feature that we have today. This is something we can add after the beta as well, as I don't expect it would really require changes to the API surface, but I wanted to add it here to start gathering feedback about it. |
Interesting - that'll need new syntax for "this |
Could it be something the resolver support itself ? For example, a In a gist, I think it is a feature on the resolvers themselves, and not necessary on "tekton resolution", and I don't think it should even bubble in the API at all. |
So this is a bunch of interesting questions! I'm pretty sure (I'm going to add e2e tests for this today) that if you've got a It gets a bit more complex for resolvers other than The |
(and down the road, it may make sense for a |
Also, I keep thinking that it may make sense to eventually add real fields (with relevant structs) to
Rather than the more elaborate generic params approach of
We could only support the I'm gonna open an issue for this. =) |
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 Based on discussions around promoting features last week, I'm opting to skip the need for a `beta` flag and take remote resolution straight to `stable`. I'm confident in the API stability, so I don't think there's a reason to wait since v1 hasn't happened yet. And for that matter, v1 is expecting remote resolution to be on by default anyway. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 There are two major aspects to this change: * Creating a `v1beta1` for `ResolutionRequest`, changing from `parameters` as a `map[string]string` to `params` (field name rename for `openapi-gen` API rule validation reasons) as a `[]pipelinev1beta1.Param`. This is being done to match `ResolverRef.Params`, which is a `[]pipelinev1beta1.Param`. We originally left `ResolutionRequest.Parameters` as `map[string]string` since it already was that in `v1alpha1` and changing it would require `ResolutionRequest` `v1beta1`, hence doing this now. Changing to `[]pipelinev1beta1.Param` is needed to support array and object params for resolvers, and we're going with a slice rather than a map because that's how you're supposed to do this in Kubernetes. =) * Promoting the remote resolution functionality from `alpha` to `beta`, meaning on by default for `v1beta1` `Pipeline`s etc, and requiring `enable-api-fields: beta` for `v1` `Pipeline`s etc. Additionally, we're undoing the questionable decision to split the resolvers out into `resolvers.yaml`, separate from `release.yaml`, and enabling the built-in resolvers. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes #4710 There are two major aspects to this change: * Creating a `v1beta1` for `ResolutionRequest`, changing from `parameters` as a `map[string]string` to `params` (field name rename for `openapi-gen` API rule validation reasons) as a `[]pipelinev1beta1.Param`. This is being done to match `ResolverRef.Params`, which is a `[]pipelinev1beta1.Param`. We originally left `ResolutionRequest.Parameters` as `map[string]string` since it already was that in `v1alpha1` and changing it would require `ResolutionRequest` `v1beta1`, hence doing this now. Changing to `[]pipelinev1beta1.Param` is needed to support array and object params for resolvers, and we're going with a slice rather than a map because that's how you're supposed to do this in Kubernetes. =) * Promoting the remote resolution functionality from `alpha` to `beta`, meaning on by default for `v1beta1` `Pipeline`s etc, and requiring `enable-api-fields: beta` for `v1` `Pipeline`s etc. Additionally, we're undoing the questionable decision to split the resolvers out into `resolvers.yaml`, separate from `release.yaml`, and enabling the built-in resolvers. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Closes tektoncd#4710 There are two major aspects to this change: * Creating a `v1beta1` for `ResolutionRequest`, changing from `parameters` as a `map[string]string` to `params` (field name rename for `openapi-gen` API rule validation reasons) as a `[]pipelinev1beta1.Param`. This is being done to match `ResolverRef.Params`, which is a `[]pipelinev1beta1.Param`. We originally left `ResolutionRequest.Parameters` as `map[string]string` since it already was that in `v1alpha1` and changing it would require `ResolutionRequest` `v1beta1`, hence doing this now. Changing to `[]pipelinev1beta1.Param` is needed to support array and object params for resolvers, and we're going with a slice rather than a map because that's how you're supposed to do this in Kubernetes. =) * Promoting the remote resolution functionality from `alpha` to `beta`, meaning on by default for `v1beta1` `Pipeline`s etc, and requiring `enable-api-fields: beta` for `v1` `Pipeline`s etc. Additionally, we're undoing the questionable decision to split the resolvers out into `resolvers.yaml`, separate from `release.yaml`, and enabling the built-in resolvers. Deprecation of the legacy `taskRef.bundle` and `pipelineRef.bundle` syntax will be handled in a separate PR (tektoncd#5514). Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
Feature request
Remote resolution is currently behind the alpha feature gate in Tekton Pipelines. This issue is intended to track the work required to promote resolution into beta, removing the feature gate.
Use case
Platforms and users would like to access Tekton resources in locations other than OCI registries or the cluster their taskruns and pipelineruns execute in.
Criteria for Promotion
The text was updated successfully, but these errors were encountered: