Adds resolver to provide task resolution for images #2137

sthaha · 2020-03-03T07:44:45Z

Changes

This partially addresses the desire to fetch tasks
from an OCI image artifact.

Issue: #1839

Signed-off-by: Sunil Thaha sthaha@redhat.com

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
[🙅‍♂ ] Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

If you are adding a new binary/image to the cmd dir, please update
the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

sthaha · 2020-03-04T00:05:41Z

pkg/remote/oci.go

+}
+
+func (o OCIResolver) readImageLayer(kind string, name string) ([]byte, error) {
+	// TODO: When this is moved into the Tekton controller, authorize this


This will be addressed in a follow up patch as the change seems rather large

Do worry too much about it, we've seen way larger changes 😉

So should this be all done in one PR or in multiple?

depend what you mean by "all". But having a full working pkg/remote/oci.go file (aka at least with the serviceAccount support, the validation too) works, then other PR to start integrating it into the reconcilier would be others.

sthaha · 2020-03-04T00:17:12Z

/test tekton-pipeline-unit-tests

vdemeester · 2020-03-04T08:06:55Z

pkg/remote/oci.go

+}
+
+func (o OCIResolver) readImageLayer(kind string, name string) ([]byte, error) {
+	// TODO: When this is moved into the Tekton controller, authorize this


Do worry too much about it, we've seen way larger changes 😉

vdemeester · 2020-03-04T08:07:26Z

pkg/remote/oci.go

+func init() {
+	// Add Tekton's resources to the K8s deserializer
+	schemeBuilder := runtime.NewSchemeBuilder(v1beta1.AddToScheme)
+	err := schemeBuilder.AddToScheme(scheme.Scheme)
+	if err != nil {
+		log.Panic(err)
+	}
+}


why is it required ? 🤔

sthaha · 2020-03-06T02:31:44Z

pkg/remote/oci.go

+// authentication.
+type KeychainProvider func() (authn.Keychain, error)
+
+// ImageResolver will attempt to fetch Tekton resources from an OCI compliant image repository.


OCIResolver

This partially addresses the desire to fetch tasks from an OCI image artifact. Issue: tektoncd#1839 Signed-off-by: Sunil Thaha <sthaha@redhat.com>

pierretasci · 2020-03-09T21:16:54Z

/assign vdemeester
/assign sbwsg

vdemeester

/meow

We will need to add more to this, most likely in the form of annotation:

support for multiple api (right now it's …v1beta1, at some point we will need to support more than one version at the same time)
support for specific version of tekton, or minimum version required

tekton-robot · 2020-03-10T08:43:12Z

@vdemeester:

In response to this:

/meow

We will need to add more to this, most likely in the form of annotation:

support for multiple api (right now it's …v1beta1, at some point we will need to support more than one version at the same time)

support for specific version of tekton, or minimum version required

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.

tekton-robot · 2020-03-10T08:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

ghost · 2020-03-10T11:21:07Z

pkg/remote/oci_test.go

+
+	imgRef, err := name.ParseReference(fmt.Sprintf("%s/test/ociresolver", u.Host))
+	if err != nil {
+		t.Errorf("undexpected error producing image reference %s", err.Error())


nit: undexpected -> unexpected

ghost · 2020-03-10T11:29:22Z

pkg/remote/oci.go

+type KeychainProvider func() (authn.Keychain, error)
+
+// OCIResolver will attempt to fetch Tekton resources from an OCI compliant image repository.
+type OCIResolver struct {


What's a typical usage of this type going to look like? If it's most often going to be instantiated and immediately used like this:

{ myresolver := remote.OCIResolver{imageRef, kp} mytask := myresolver.GetTask(foo) }

then I suggest simply exposing a GetOCITask func that takes (imageReference string, keychainProvider KeychainProvider, taskName string). WDYT?

yeah that makes a ton of sense. I guess it comes down to how this evolves when we add a resolver for Git or other remote sources. I could see an argument for either route.

I think the idea here is that it might be easier for a reconciler to use a factory to instantiate a Resolver and call it instead of having to know itself whether it should use an OCIResolver or a GitResolver or whatever else we add. We might even add a "local" resolver and embed the local task resolver into this format. Could be overkill or could be a nice abstraction. Thoughts?

I can definitely see a factory fitting that use case. I'm not sure it follows that we'd therefore need a remote.Resolver interface and creation of structs like this anywhere outside the factory's internals though.

I think it'll ultimately depend on both the kind of configuration we expect to be available and the signature we want to expose for resolution. Examples of config might be that we allow specific resolvers to be turned on/off/configured via configmap / cli flags. Examples of public signature might be a func like ResolveTaskFromRef(taskRef) that figures out the kind of resolution it needs to make without publicly exposing the Resolver type.

Not to say we need to commit to those kinds of design choices here, more that by introducing the Resolver machinery before we get to that stage we're already kind of leaning towards a design. And as often happens we might end up needing to unwind those kinds of choices based on work discovered as we dig into that stage more thoroughly. And in turn that can lead to a some extra work reviewing in future as well.

None of this is a blocker at all from my POV, just raising it from my read-through. 👍

/lgtm

Yup. Makes a ton of sense. We can always wrap the resolver in a function like you mention to give a single interface to caller but I guess the next PR that embeds this in the reconciler will show if it makes it more or less "gross" and we can go from there 😄

pierretasci · 2020-03-10T16:33:18Z

support for multiple api (right now it's …v1beta1, at some point we will need to support more than one version at the same time)

support for specific version of tekton, or minimum version required

Question for @vdemeester, do you mean that the user should specify a min-supported version of Tekton when referencing the task as in

taskRef:
  name: my-task
  image: docker.com/my-image
  minVersion: v1alpha1

or do you mean that we should ensure the version of the fetched task is not incompatible with the version of the taskrun or pipelinerun that references it?

apiVersion: v1beta1
...
taskRef:
  name: my-task
  image: docker.com/my-image # apiVersion: v1alpha1

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 3, 2020

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 3, 2020

tekton-robot requested review from imjasonh and a user March 3, 2020 07:44

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 3, 2020

sthaha force-pushed the task-resolver branch from a41636d to f2b20c2 Compare March 4, 2020 00:00

sthaha changed the title ~~WIP: Add resolver to provide task resolution for images~~ Adds resolver to provide task resolution for images Mar 4, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 4, 2020

sthaha commented Mar 4, 2020

View reviewed changes

sthaha force-pushed the task-resolver branch from f2b20c2 to 0721b59 Compare March 4, 2020 00:18

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 4, 2020

vdemeester reviewed Mar 4, 2020

View reviewed changes

vdemeester added kind/design Categorizes issue or PR as related to design. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 4, 2020

pierretasci mentioned this pull request Mar 5, 2020

Add an option to secure the test registry google/go-containerregistry#686

Closed

sthaha force-pushed the task-resolver branch from 0721b59 to 8874c6d Compare March 6, 2020 02:30

sthaha commented Mar 6, 2020

View reviewed changes

Adds resolver to provide task resolution for images

912a8a4

This partially addresses the desire to fetch tasks from an OCI image artifact. Issue: tektoncd#1839 Signed-off-by: Sunil Thaha <sthaha@redhat.com>

sthaha force-pushed the task-resolver branch from 8874c6d to 912a8a4 Compare March 9, 2020 00:55

tekton-robot assigned ghost and vdemeester Mar 9, 2020

vdemeester approved these changes Mar 10, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 10, 2020

ghost reviewed Mar 10, 2020

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2020

tekton-robot merged commit 9e8e752 into tektoncd:master Mar 10, 2020

danielhelfand mentioned this pull request Mar 26, 2020

Add ROADMAP.md 🤖 tektoncd/cli#835

Merged

4 tasks

dlorenc mentioned this pull request Jul 17, 2020

OCI Storage Backend tektoncd/chains#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds resolver to provide task resolution for images #2137

Adds resolver to provide task resolution for images #2137

sthaha commented Mar 3, 2020 •

edited

Loading

sthaha Mar 4, 2020

vdemeester Mar 4, 2020

pierretasci Mar 4, 2020

vdemeester Mar 5, 2020

sthaha commented Mar 4, 2020

vdemeester Mar 4, 2020

vdemeester Mar 4, 2020

sthaha Mar 6, 2020

pierretasci commented Mar 9, 2020

vdemeester left a comment

tekton-robot commented Mar 10, 2020

tekton-robot commented Mar 10, 2020

ghost Mar 10, 2020

ghost Mar 10, 2020

pierretasci Mar 10, 2020

ghost Mar 10, 2020

pierretasci Mar 10, 2020

pierretasci commented Mar 10, 2020

Adds resolver to provide task resolution for images #2137

Adds resolver to provide task resolution for images #2137

Conversation

sthaha commented Mar 3, 2020 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha commented Mar 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierretasci commented Mar 9, 2020

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 10, 2020

tekton-robot commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierretasci commented Mar 10, 2020

sthaha commented Mar 3, 2020 •

edited

Loading