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

TEP0122 - Complete build instructions and parameters - implementable #894

Conversation

chitrangpatel
Copy link
Contributor

@chitrangpatel chitrangpatel commented Dec 7, 2022

This proposal was intended to be implementable but was accidentally left in proposed state. Some minor cleanup (accidental buildConfig --> invocation.Parameters) and clarifications were added to make it more readable. No major changes were introduced.

@tekton-robot tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2022
@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch from 7f50790 to f25c813 Compare December 7, 2022 15:17
@tekton-robot tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 7, 2022
@afrittoli
Copy link
Member

Maybe this PR is an opportunity to address #820 (comment)?

@chitrangpatel
Copy link
Contributor Author

chitrangpatel commented Dec 7, 2022

Maybe this PR is an opportunity to address #820 (comment)?

Good point. I will move the conversation here:

What about flags passed to the controller directly at start time?

Hmm. I didn't think of that. Do you mean https:/tektoncd/pipeline/blob/d5f1a1743ff03b5aeee0363c234016d3eaa66d75/config/controller.yaml#L63-L87? If so, yes we should be able to capture the args in materials.

Also, we have a lot of different configuration flags, so it would make sense to specify which ones shall be included.

I was thinking of including them all for completeness since there are only 16 I think.

It feels like this would cause a non-negligable increase in size of the TaskRun resources.

Hmm. Unless I'm misunderstanding, the only features that I'm thinking of are these. Surely, that list could increase over time but probably not too large? That shouldn't be an issue right?
We can definitely update this approach if its not good enough.

Is it a standard SLSA requirement to include the configuration of the build system?

I think SLSA wants reproducibility as best as we can. I think feature flags that were used to configure Tekton that produced the build is very important for reproducibility and for verification. A user may decide that the build was performed using wrong feature flags and may not want to deploy the build.

@bobcatfish
Copy link
Contributor

/approve

@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 8, 2022
@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch from f25c813 to 2386a40 Compare December 14, 2022 18:24
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2022
@chuangw6
Copy link
Member

/assign

@chuangw6
Copy link
Member

Synced offline with @chitrangpatel . We need to update the tables to reflect that service account, workspaces etc. provided in taskrun will be recorded in invocation.params instead of buildConfig.

Not sure if it fits into this pr which just moves the tep to implementable. If not, feel free to do it in another pr :d Thanks @chitrangpatel !!

@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch from 2386a40 to 2320a3b Compare December 14, 2022 20:19
@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 14, 2022
Copy link
Member

@chuangw6 chuangw6 left a comment

Choose a reason for hiding this comment

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

Thanks @chitrangpatel for updating this!

@chitrangpatel
Copy link
Contributor Author

Synced offline with @chitrangpatel . We need to update the tables to reflect that service account, workspaces etc. provided in taskrun will be recorded in invocation.params instead of buildConfig.

Not sure if it fits into this pr which just moves the tep to implementable. If not, feel free to do it in another pr :d Thanks @chitrangpatel !!

I think it fits here. Before making it implementable, its better to address these issues.

@chuangw6
Copy link
Member

chuangw6 commented Dec 14, 2022

What about flags passed to the controller directly at start time?
Is it a standard SLSA requirement to include the configuration of the build system?

I think on the surface level, the provenance aims to record the information about the build process of an artifact instead of the build system. There is indeed a field in the SLSA v0.2 provenance named predicate.builder to record the builder information, but it only records the builder identifier.

That said, the SLSA v1.0 design (draft) seems to introduce more fields about builder, and one of them is builderDependencies which seems like a better place for the flags to the controller?

So I am leaning towards making flags out of scope for SLSA v0.2, but I am open to other suggestions here.

cc @afrittoli @chitrangpatel

@chitrangpatel
Copy link
Contributor Author

@chuangw6 @afrittoli according to https://slsa.dev/provenance/v0.2, invocation.environment is meant to contain any builder-controlled inputs necessary for correctly evaluating the build. Usually only needed for reproducing the build but not evaluated as part of policy.

In slsav0.2, I think this is where config-feature-flags belong and may be also arguments that I think @afrittoli is mentioning.

@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch 2 times, most recently from c01b2f4 to 089e4be Compare December 15, 2022 19:47
@chitrangpatel
Copy link
Contributor Author

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Dec 19, 2022
@JeromeJu
Copy link
Member

/assign @afrittoli

@chuangw6
Copy link
Member

/approve

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2022
@pritidesai
Copy link
Member

API WG - @afrittoli please take a look, this approved by other reviewers. @chitrangpatel to rebase this. Thanks!

@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch from 089e4be to 4a67752 Compare January 9, 2023 19:22
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2023
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

/approve

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, chuangw6

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

teps/README.md Outdated Show resolved Hide resolved
This proposal was intended to be implementable but was accidentally left in proposed state. I don't believe there is anything else that needs to be done to make it implementable.
@chitrangpatel chitrangpatel force-pushed the TEP-0122-reproducibility-of-complete-build-instructions branch from 4a67752 to b378706 Compare January 19, 2023 19:35
@afrittoli
Copy link
Member

Approved in API WG
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2023
@tekton-robot tekton-robot merged commit 71a4fd7 into tektoncd:main Jan 23, 2023
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/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
Status: UnAssigned
Development

Successfully merging this pull request may close these issues.

8 participants