Skip to content
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

Smarter Chains: check taskrun level results for Subjects #866

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

chuangw6
Copy link
Member

@chuangw6 chuangw6 commented Jul 18, 2023

Changes

/kind feature

Step 1/2 of #850

Prior, Chains only looks for pipeline results to understand what
artifacts were generated in a pipeline. That means pipeline authors need
to propagate child TaskRun results to pipeline level and name the pipeline
results in type hinting way even though the pulled tasks already produce
type hinting results.

Now, we introduced a new configmap field artifacts.pipelinerun.enable-deep-inspection
to allow Chains to inspect both pipeline results and child task results
to understand what artifacts were generated throughout a pipeline.

This way, pipeline authors no longer need to worry about the rules when
writting a pipeline as long as they pull in right tasks that produce type hinting results.
That said, users still have ability to propagate task results to pipeline
level if the tasks they referenced do not produce type hinting results.

Signed-off-by: Chuang Wang [email protected]

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)

Release Notes

Enable Chains to inspect both pipelinerun results and its child taskrun results.

Introduce a new configmap field `artifacts.pipelinerun.enable-deep-inspection` to allow users to select whether Chains should dive into child taskruns for capturing results.

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 18, 2023
pkg/chains/formats/slsa/extract/extract.go Outdated Show resolved Hide resolved
pkg/chains/formats/slsa/extract/extract.go Show resolved Hide resolved
pkg/chains/formats/slsa/extract/extract_test.go Outdated Show resolved Hide resolved
pkg/chains/objects/objects.go Outdated Show resolved Hide resolved
chuangw6 added a commit to chuangw6/chains that referenced this pull request Jul 25, 2023
Prior, we received slsa-related configs from configmap and passed them
via separate arguments down to GenerateAttestation.

Now, we pass them via context instead of separate args.

Benefits:
- make `GenerateAttestation` function interface cleaner which ends up
receiving only 2 args: ctx and TektonObject.
- be extensible to receive more slsa-related configs without having to change
downstream functions' interface i.e. config option [`ObserveMode`](tektoncd#866 (comment))
which customizes how Chains observes inputs/outputs.

Signed-off-by: Chuang Wang <[email protected]>
chuangw6 added a commit to chuangw6/chains that referenced this pull request Jul 26, 2023
Prior, we received slsa-related configs from configmap and passed them
via separate arguments down to GenerateAttestation.

Now, we pass them via a struct as a whole instead of separate args. This
will enable slsa formatters to be extensible to receive more slsa-related
configs without having to change downstream functions' interface i.e.
config option [`ObserveMode`](tektoncd#866 (comment))
which customizes how Chains observes inputs/outputs.

Signed-off-by: Chuang Wang <[email protected]>
tekton-robot pushed a commit that referenced this pull request Jul 28, 2023
…#885)

Prior, we received slsa-related configs from configmap and passed them
via separate arguments down to GenerateAttestation.

Now, we pass them via a struct as a whole instead of separate args. This
will enable slsa formatters to be extensible to receive more slsa-related
configs without having to change downstream functions' interface i.e.
config option [`ObserveMode`](#866 (comment))
which customizes how Chains observes inputs/outputs.

Signed-off-by: Chuang Wang <[email protected]>
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 59.4% 16.2

@chuangw6 chuangw6 force-pushed the smarter-chains branch 2 times, most recently from f6677c5 to 6df2fb5 Compare August 1, 2023 22:28
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 58.2% 15.0

@chuangw6 chuangw6 marked this pull request as draft August 1, 2023 22:42
@tekton-robot 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 Aug 1, 2023
@chuangw6 chuangw6 force-pushed the smarter-chains branch 2 times, most recently from c58bf20 to c7cb7b6 Compare August 2, 2023 16:31
@chuangw6 chuangw6 marked this pull request as ready for review August 2, 2023 16:36
@tekton-robot 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 Aug 2, 2023
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 65.8% 22.6

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 65.8% 22.6

case *v1beta1.PipelineRun:
subjects = subjectsFromPipelineRun(ctx, obj, slsaconfig)
case *v1beta1.TaskRun:
subjects = subjectsFromTektonObject(ctx, obj)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call it subjectsFromTaskRun? Tekton Object refers to both task and pipeline runs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subjectsFromPipelineRun also passes PipelineRunObject to subjectsFromTektonObject if pr mode is configured.

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 65.8% 22.6

docs/config.md Outdated Show resolved Hide resolved
pkg/chains/formats/slsa/extract/extract.go Outdated Show resolved Hide resolved
pkg/chains/formats/slsa/extract/extract.go Outdated Show resolved Hide resolved
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@chuangw6 chuangw6 requested a review from lcarva August 9, 2023 18:43
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lcarva

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2023
@chuangw6 chuangw6 force-pushed the smarter-chains branch 2 times, most recently from dae980f to e690eac Compare August 10, 2023 18:36
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@lcarva
Copy link
Contributor

lcarva commented Aug 10, 2023

/lgtm

1 similar comment
@chitrangpatel
Copy link
Contributor

/lgtm

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
Step 1/2 of tektoncd#850

Prior, Chains only looks for pipeline results to understand what
artifacts were generated in a pipeline. That means pipeline authors need
to propagate child TaskRun results to pipeline level and name the pipeline
results in type hinting way even though the pulled tasks already produce
type hinting results.

Now, we introduced a new configmap field `artifacts.pipelinerun.enable-deep-inspection`
to allow Chains to inspect both pipeline results and child task results
to understand what artifacts were generated throughout a pipeline.

This way, pipeline authors no longer need to worry about the rules when
writting a pipeline as long as they pull in right tasks that produce type hinting results.
That said, users still have ability to propagate task results to pipeline
level if the tasks they referenced do not produce type hinting results.

Signed-off-by: Chuang Wang <[email protected]>
@chuangw6
Copy link
Member Author

Sorry made a minor change (unit test name)

@lcarva @chitrangpatel

@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@chitrangpatel
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023
@chuangw6 chuangw6 closed this Aug 10, 2023
@chuangw6 chuangw6 reopened this Aug 10, 2023
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/chains/formats/slsa/extract/extract.go 43.2% 69.9% 26.7
pkg/config/config.go 89.8% 91.8% 2.0

@tekton-robot tekton-robot merged commit f1e2dad into tektoncd:main Aug 10, 2023
21 checks passed
@chuangw6
Copy link
Member Author

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 21, 2023
@chuangw6 chuangw6 mentioned this pull request Aug 24, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants