Add Archivista Storage Backend #1316
Conversation
|
|
tekton-robot
added
size/L
|
Hi @mikhailswift. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Tagging @colek42 for visibility |
mikhailswift
force-pushed
the
archivista-storage2
branch
2 times, most recently
from
41cd51c to
986ab3b
Compare
|
/ok-to-test |
tekton-robot
added
ok-to-test
|
The following is the coverage report on the affected files.
|
|
@PuneetPunamiya do you see any issues with this PR? Let us know if you would like to discuss at a community meeting. FYSA Archivista is an intoto project which is CNCF graduated. |
| "chains.tekton.dev/archivista-gitoid": uploadResp.Gitoid, | ||
| "chains.tekton.dev/archivista-url": a.url, | ||
| } | ||
| obj.SetAnnotations(annotations) |
There was a problem hiding this comment.
@lcarva - right now we do not set any annotation on tekton objects post storage with other backend types. Is there any concern having the info captured post upload?
There was a problem hiding this comment.
I haven't reviewed the PR yet, but this is what comes to mind:
Chains will add annotations once it is done processing a Tekton object (TaskRun or PipelineRun). That happens right at the end of reconciliation. This update triggers another reconciliation but that's generally ok because Chains quickly detects the object has already been processed (based on it annotations) and skips that iteration of the reconciliation loop.
If Chains updates the Tekton object before those final annotations are added, the reconciliation that is triggered as a side-effect will cause signing and attesting to happen again. This may happen multiple times.
I would advise against changing the object during reconciliation. If needed, try tying into the already existing mechanism Chains has for adding the final annotations, see here.
NOTE: The tekton storage does update the object during reconciliation. We should probably fix that, but that's less of a concern given that that storage type is meant for test/develop.
There was a problem hiding this comment.
@lcarva Are you suggesting that tekton storage should not be used in production? Isn't it included in some of the default configuration?
There was a problem hiding this comment.
It might be best for us to remove this for now and follow up with the annotation in a separate PR. cc @mikhailswift
There was a problem hiding this comment.
Are you suggesting that tekton storage should not be used in production?
Yes, 100%. This storage stores payloads as annotations which can get quite large if you have a pipeline with many tasks, for example. You'll face etcd issues eventually.
Isn't it included in some of the default configuration?
Yes, it's not ideal. We could pursue changing this default to OCI which is a more suitable storage type that requires no configuration if you are already building container images.
There was a problem hiding this comment.
The annotations have been removed from the Archivista storage backend
There was a problem hiding this comment.
@mikhailswift could you please help with the steps a user has to perform with cosign to verify-blob-attestation or verify-attestation accessing the uploaded content from archivista?
mikhailswift
force-pushed
the
archivista-storage2
branch
from
986ab3b to
fbab4c6
Compare
|
The following is the coverage report on the affected files.
|
mikhailswift
force-pushed
the
archivista-storage2
branch
from
fbab4c6 to
d5dad8e
Compare
|
The following is the coverage report on the affected files.
|
mikhailswift
force-pushed
the
archivista-storage2
branch
from
d5dad8e to
e2bd6d3
Compare
|
The following is the coverage report on the affected files.
|
| return nil | ||
| } | ||
|
|
||
| // RetrievePayload is not implemented for Archivista. |
There was a problem hiding this comment.
I think the main thing we need here is details of what Tekton uses to retrieve the data. I looked into it a bit and I am a bit unclear on what the ShortKey is that is being used to find the payload.
Archivista can search for payloads with its GraphQL API and download the DSSE envelope. From Archivista's side, we need to make sure we are indexing the ShortKey or some other identifier that Tekton will use to retrieve the payload.
There was a problem hiding this comment.
@mikhailswift - Please lookup - https://github.com/tektoncd/chains/blob/main/pkg/config/options.go#L26C1-L36C1. Does this help?
// ShortKey is the short version of an artifact identifier. This is useful for annotation based storage
// because annotation key has limitations (https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set).
// - For OCI artifact, it is first 12 chars of the image digest.
// - For TaskRun/PipelineRun artifact, it is `<KIND>-<UID>`.
ShortKey string
There was a problem hiding this comment.
Yeah, I think this helps. One thing I'm not entirely clear on -- does the UID appear in the in-toto attestation payload?
If so archivista can easily just look for that and put it in a searchable edge off of the DSSE envelope
|
One thing I would encourage and it would be nice if you can submit a follow up pr to add a small tutorial for using the storage backend you have added for example - https://github.com/tektoncd/chains/tree/main/docs/tutorials |
mikhailswift
force-pushed
the
archivista-storage2
branch
from
e2bd6d3 to
0b90ad0
Compare
|
I addressed the few comments here -- If I can get clarification on what I can add that and a tutorial in a followup PR. |
|
The following is the coverage report on the affected files.
|
Adds the ability to store signed TaskRun and PipelineRun results in Archivista. Archivista currently only supports payloads that are wrapped in a DSSE envelope, so any signatures that are not DSSE envelopes will not be stored.
mikhailswift
force-pushed
the
archivista-storage2
branch
from
0b90ad0 to
0b46102
Compare
|
Re-pushed to address the linting job errors |
|
The following is the coverage report on the affected files.
|
Changes
Adds the ability to store signed TaskRun and PipelineRun results in Archivista. Archivista currently only supports payloads that are wrapped in a DSSE envelope, so any signatures that are not DSSE envelopes will not be stored.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes