move trusted resources verification after we resolve the remote resources #6621
Conversation
|
Skipping CI for Draft Pull Request. |
|
@Yongxuanzhang: The label(s) 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. |
tekton-robot
added
do-not-merge/work-in-progress
tekton-robot
added
the
size/XL
|
/test all |
|
/kind misc |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test all |
|
The following is the coverage report on the affected files.
|
| @@ -112,9 +112,16 @@ spec: | |||
| tasks: | |||
| - name: task | |||
| taskRef: | |||
| name: %s | |||
| resolver: cluster | |||
There was a problem hiding this comment.
Use cluster resolve for e2e test, since we don't have these examples stored in remote storage, it's hard to test other resolvers.
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String()) |
There was a problem hiding this comment.
We may return other errors, so we need to return err here instead of a new convert error
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
64866e4
to
283c534
Compare
|
/test all |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
2 times, most recently
from
7ddb9a4
to
b640cef
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
b640cef
to
743f9ff
Compare
|
The following is the coverage report on the affected files.
|
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
743f9ff
to
2364433
Compare
| // Set up a fake registry to push an image to. | ||
| s := httptest.NewServer(registry.New()) | ||
| defer s.Close() | ||
| u, err := url.Parse(s.URL) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
Use oci bundle to test trusted resources, we could probably test resolver here, but I think e2e test has covered the resolver case (using cluster resolver)
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
522fee9
to
f45f6a5
Compare
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
f45f6a5
to
3bc54fb
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lbernick 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 |
tekton-robot
added
the
approved
| // OCI bundle and remote resolution pipelines will be verified by trusted resources if the feature is enabled | ||
| func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, pipelineRun *v1beta1.PipelineRun, verificationPolicies []*v1alpha1.VerificationPolicy) rprp.GetPipeline { |
There was a problem hiding this comment.
Why don't we verify the resource in the default case where in-cluster resources are resolved by the local cluster resolver?
Prior to the move, that case seems to be included for verification but after the move the case seems ignored.
Same for task reconciler.
There was a problem hiding this comment.
Thanks! Local resolver is actually not fully supported due to the mutating webhook(the verification may fail) and we didn't make is clear before.
This PR also updates the doc to highlight what we support (resources from resolver&oci bundle). I'm hesitant to update the release notes since we don't actually support it before. Let me know if you have any suggestions. 😄
There was a problem hiding this comment.
Do you mean that the in-cluster resources could be mutated by the mutation webhook when they are applied to the cluster, which might cause the verification fail?
If so, the remote resolver is also able to resolve the in-cluster resources via cluster resolver. Will there be same issue and how are we planning to handle that?
There was a problem hiding this comment.
Yes, I also mentioned this in the doc. If users want to use cluster resolver, the resources need to set all default values.
There was a problem hiding this comment.
I think currently we could just document it, then when we finish high priority features(v1 support, condition update), we can start planning solutions for local resources. Wdyt?
There was a problem hiding this comment.
STGM as long as we will support that case.
I thought it's in the scope initially b/c the TEP doc mentions that "Provide an optional mechanism for Resources (local and remote tekton tasks/ pipelines) to be signed and verified."
There was a problem hiding this comment.
yes, that's still in the scope, but just not the priority at this moment.
|
/retest |
|
This looks related to a known flake, so I've commented on the issue tracking it: #6624 |
|
/lgtm |
|
/remove-lgtm |
docs/trusted-resources.md
Outdated
| Trusted Resources is a feature which can be used to sign Tekton Resources and verify them. Details of design can be found at [TEP--0091](https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md). This feature is under `alpha` version and support `v1beta1` version of `Task` and `Pipeline`. | ||
| Trusted Resources is a feature which can be used to sign Tekton Resources and verify them. Details of design can be found at [TEP--0091](https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md). This is an alpha feature and supports `v1beta1` version of `Task` and `Pipeline`. | ||
|
|
||
| **Note**: trusted resources support verification of resources from OCI bundle or remote resolution, to use [cluster resolver](./cluster-resolver.md) make sure to set all default values for the resources before applied to cluster, otherwise the verification will fail due to the mutating webhook. |
There was a problem hiding this comment.
I feel this paragraph is a bit confusing to users for 3 reasons: 1/. oci bundle is part of remote resolution 2. as discussed, there are 2 ways resolving cluster resources i.e. remote resolver, and local resolver. 3. what should users set for default values?
How about this?
Note: Currently, trusted resources only support verifying Tekton resources that come from remote places i.e. git, OCI registry and Artifact Hub. For in-cluster resources, verification might fail because the mutation webhook will set default values for the type of the parameters and results, which implies the resources will be modified before verification. In-cluster resources will be supported later. If one wants to use trusted resources for this case, they should set those default values for the resources before applying to the cluster so that mutation webhook doesn't affect the originality of the resource.
(Please add other areas that mutation webhook will take affect on if missed above)
There was a problem hiding this comment.
Emmm, OCI bundle is another feature not part of remote resolution, this feature is supported for v1beta1 but will not be supported in v1. I can update the doc to add link. https://github.com/tektoncd/pipeline/blob/main/docs/pipelineruns.md#tekton-bundles
Thanks for the suggestion! I will update the doc.
There was a problem hiding this comment.
The link you shared is the remote resolver that resolves OCI resource. The old bundle field in PipelineRef will be deprecated for v1beta1. "Deprecated: Please use ResolverRef with the bundles resolver instead."
Generally, I think we should mention where the resource came from (i.e. remote place are well supported, in-cluster resources are not supported unless default are well set before applying to cluster), instead of mentioning remote resolution.
Thanks for the suggestion! I will update the doc.
Thanks you for your hard work.
…rces This PR moves the trusted resources verification to readRuntimeObjectAsTask and readRuntimeObjectAsPipline, the reasons we need this change include 1) unblock the work for v1, since v1 will mutate, validate and convert the resources, the mutation will break trusted resources verification thus we need to verify right after we resolve the remote resources. 2) Prepare the support for verifying different api versions. This commit also makes it clear that currently we only support verification for remote resources. Signed-off-by: Yongxuan Zhang [email protected]
Yongxuanzhang
force-pushed
the
verify-when-fetch
branch
from
3bc54fb
to
288dc00
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/ltgm |
|
/lgtm |
Changes
This PR moves the trusted resources verification to readRuntimeObjectAsTask and readRuntimeObjectAsPipline, the reasons we need this change include:
/kind misc
Closes #6618
Signed-off-by: Yongxuan Zhang [email protected]
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes