-
Notifications
You must be signed in to change notification settings - Fork 220
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
TEP-0005: Tekton OCI Bundle #137
TEP-0005: Tekton OCI Bundle #137
Conversation
@vdemeester: GitHub didn't allow me to request PR reviews from the following users: pierretasci. Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
btw @imjasonh you can also hit this button to see the rendered version in the files view (still cant comment tho): |
btw, https://hackmd.io/RPvSTjA_RSuVOu2FEmj8bQ is where I edit this file 😉 (and you can edit it with me, comment, …) |
teps/0003-tekton-oci-bundles.md
Outdated
### Goals | ||
|
||
The goal of this TEP is to define the specification for the Tekton OCI | ||
bundles. The goal of the Tekton OCI bundles is to define a Spec on top |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a Spec
? and not a full resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The distinction is actually really important. They key use case that OCI Bundles unlocks is the ability to refer to Tasks/Pipelines outside of the cluster. This allows multiple definitions of the same-named Task/Pipeline to exist and for users to version them. Therefore, the metadata of the resource is unimportant. It is really the spec we are after as we wish to dynamically insert that at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means that we again will get the same problem as this PR was intended to fix tektoncd/pipeline#2826
By providing the full resource rather than a part of a resource the end-user has the possibility to add labels and annotations. But this was mostly a question - it may not be relevant - but it is hard to reason since the full context (e.g. use) is not specified here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Spec
here should be rewritten as specification, it doesn't have anything to do with the spec
field of a Tekton Resource 😅 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the purpose of a TEP is to get too involved into the implementation but just to entertain this...there is nothing stopping the implementation of this from propagating the metadata of the task. It is possible either way (though to be clear, my preference is to not propagate labels and annotations from tasks to the pod).
teps/0003-tekton-oci-bundles.md
Outdated
|
||
### Non-Goals | ||
|
||
How Tekton OCI bundles are used (referred to) in the different Tekton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that it is hard to reason about this feature - e.g. hard to understand what it really is - when there is no explanation about how these are used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more background, the original design doc might be useful: https://docs.google.com/document/d/1lXF_SvLwl6OqqGy8JbpSXRj4hWJ6CSImlxlIl4V9rnM/edit
teps/0003-tekton-oci-bundles.md
Outdated
|
||
Our most important capability is enabling references of remote Tasks | ||
from within TaskRuns or Pipelines and Pipelines from within | ||
PipelineRuns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggle to understand this. E.g. a new PipelineRun
is created, but it contains a reference to a Pipeline
declared within an image. So the controller - instead of accessing a resource through informers and via ApiServer and etcd - it should now start to pull images from a registry - or multiple different registries - then extract data from the images (I imaging this is a kind of unzipping or similar) and then - no caching? - so next time the run is reconciled - the same procedure is happening? This is unclear and confusing for me - with the information that is provided so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we define here is merely the contract so the implementation details such as, is the image cached, are intentionally left out. But to your earlier sentiment, that is indeed correct. Rather than use the Kubernetes API (which is how Tekton looks up references at the moment), we would like to enable external references. This solves use cases of versioning, idempotency, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to update the doc with some user stories and usage example. As @pierretasci the scope of this TEP is only to define the specification of the OCI artifact that would "transport" the tekton definitions. We can add examples of how we think they could be used by the CLI, the pipeline controller, etc… but it's out of the scope here.
I'll add those examples next week 😉
So the controller - instead of accessing a resource through informers and via ApiServer and etcd - it should now start to pull images from a registry - or multiple different registries - then extract data from the images (I imaging this is a kind of unzipping or similar) and then - no caching? - so next time the run is reconciled - the same procedure is happening? This is unclear and confusing for me - with the information that is provided so far.
This will be on the controller side to decide those things (caching, etc..). And this, most likely, will be an enhancement over this TEP or a new TEP dedicated on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think this captures the contract as originally intended by the design.
5f96456
to
082453e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm 🎉
I'd like to see some more details about how access to the bundles is authorized, but since integration into Pipelines is out of scope for this doc we can just punt the auth issue until then.
teps/0005-tekton-oci-bundles.md
Outdated
of this file) is [here](/teps/NNNN-TEP-template/README.md). | ||
|
||
--> | ||
# TEP-NNNN: Tekton OCI bundles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be 0005 now
Right, I didn't want to go too deep into this. That said, we could say that one of the benefit of this proposal is that we can re-use what already exists for OCI images. |
082453e
to
9a70e32
Compare
in-cluster functionality. You wouldn't be able to store two Tekton | ||
Tasks of the same `name` and `apiVersion` in a namespace so to make | ||
the task references easier to reason about, we enforce the same | ||
characteristic on the images. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this enforced? I don't think registries will do anything to prevent it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that registries will not enforce this. Likely, we would enforce this on pull. In particular because we rely on these annotations to understand what resource is in each layer, if it isn't specified, it is easy to reject the OCI image as it will be unusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! Another approach might be to treat these layers just like an actual image layers, and have the last one win...
Not sure which is better. I like the idea of not imposing extra semantics on what can be a valid bundle, but this is probably a rare case anyway.
teps/0005-tekton-oci-bundles.md
Outdated
|
||
- An image will store each resource as a new layer | ||
This allows us to quickly store and retrieve individual resources | ||
because we can key the layer they reside in against its metadatax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: metadata (no x) :)
this later but we can still do all of the same things with the default | ||
`MIME` type. Nothing prevents us for adding a new custom `MIME` type | ||
in the future when we are confident that most registries supports | ||
custom mime types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this, support just isn't there yet.
A few nits/questions, otherwise this looks good! |
/lgtm |
Signed-off-by: Vincent Demeester <[email protected]>
9a70e32
to
077ede1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli, pierretasci 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:
Approvers can indicate their approval by writing |
|
||
- Using OCI images and layer for what they are not intended for — at | ||
least with the usual `MIME` type — might be confusing for users. A | ||
`docker pull gcr.io/tekton-catalog/task/golang-build:0.1` will fail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a minimum, we should document the error message you'll see when you docker pull
it, so that users can find it and learn what it means (e.g., "You tried to pull a Tekton OCI bundle, which is not runnable").
Going further, do we have any say in how it fails? For instance, we could document/require (and support with tooling) that the image should actually have a valid MIME type, and when it's docker run
, it will print some useful error message? Do helm images do anything like this?
One minor non-blocking comment. Otherwise lgtm! 🎉 /lgtm |
@vdemeester I failed to notice that this TEP does not have the header part. Could you fix that as a follow-up? |
Signed-off-by: Priya Wadhwa <[email protected]> Signed-off-by: Priya Wadhwa <[email protected]>
This proposal is to be able to bundle Tasks (and Pipelines, and Resources, and potentially other future config objects) into an OCI Artifact, pushed to an image registry, and referenced from that registry.
We need to agree on a name too (cc @wlynch for the bundle part that was discussed in a complete different context 😛)
This is a early version, based on
/cc @imjasonh @pierretasci
Signed-off-by: Vincent Demeester [email protected]