pipeline icon indicating copy to clipboard operation
pipeline copied to clipboard

Published 20 hours ago • verified publisher icon tektoncd

[TEP-0089] - Spire Package

Open pxp928 opened this issue 3 years ago • 34 comments

Signed-off-by: pxp928 [email protected]

Changes

Spire package separated out from https://github.com/tektoncd/pipeline/pull/4759 as requested. It includes the spire interface with a mocked spire for testing. As requested by @afrittoli.

/kind feature

Submitter Checklist

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

  • [ ] Docs included if any changes are user facing
  • [x] Tests included if any functionality added or changed
  • [x] Follows the commit message standard
  • [x] Meets the Tekton contributor standards (including functionality, content, code)
  • [x] Release notes block below has been filled in (if there are no user facing changes, use release note "NONE")
Spire package that includes the controller and entrypointer API that is used in for [TEP-0089: Non-falsifiable provenance support](https://github.com/tektoncd/community/blob/main/teps/0089-nonfalsifiable-provenance-support.md). This is just the spire package. The phase 1 PR for the TEP is located here -> https://github.com/tektoncd/pipeline/pull/4759

pxp928 avatar Jun 25 '22 21:06 pxp928

Hi @pxp928. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tekton-robot avatar Jun 25 '22 21:06 tekton-robot

/ok-to-test

afrittoli avatar Jun 28 '22 14:06 afrittoli

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 0.0%
pkg/spire/entrypointer.go Do not exist 0.0%
pkg/spire/sign.go Do not exist 17.6%
pkg/spire/spire_mock.go Do not exist 85.5%
pkg/spire/verify.go Do not exist 17.3%

tekton-robot avatar Jun 28 '22 15:06 tekton-robot

Thanks @pxp928 for moving this to a separate PR!

I'm reviewing the PR, but I find the module structure and test strategy not 100% clear: In my understanding this is the role of each module is:

  • spire.go: defines two interfaces, ControllerAPIClient and EntrypointerAPIClient. (NIT: maybe we call call this clients?)
  • controller.go: implements some of ControllerAPIClient, including type, constructor and some of the required method, except for VerifyTaskRunResults
  • verify.go: implements the rest of ControllerAPIClient, specifically VerifyTaskRunResults (NIT: maybe call this controller_verify?)
  • entrypointer.go: implements some of EntrypointerAPIClient, including type, constructor and some of the required method, except for Sign
  • sign.go: implements the rest of EntrypointerAPIClient, specifically VerifyTaskRunResults, specifically VerifyTaskRunResults (NIT: maybe call this controller_verify?)esults` (NIT: maybe call this entrypointer_sign?)
  • spire_mock.go: defines a mock implementation of both interfaces. This bypasses most of the code in all the actual implementation, but it does invoke some of the actual functions, like verifyManifest and getResultValue
  • spire_mock_tests.go: unit tests for spire_mock.go. These tests verify the logic in the mocks and some private functions in the verify and sign modules, but I find this a bit problematic:
    • it's very opaque what is tested and what not by the unit tests. This will make changing the code in future a rather convoluted task as changing private functions might break mock unit tests that depend on them. We have a coding guideline of testing only exported functions, and this is doing the opposite
    • there is no unit test coverage at all for a lot of the code defined in the actual implementations

The structure I would expect for this is something like a mock client, that implements only functions that already exists in the original client, something like dial, close, get resource, list resources, create resource, delete resource. The ControllerAPIClient and EntrypointerAPIClient would only be responsible for implementing higher abstraction functions like VerifyTaskRunResults and Sign. They would take a client as input, which would be the actual client at runtime, or the mock client at unit test time.

/cc @tektoncd/core-maintainers thoughts?

afrittoli avatar Jul 04 '22 13:07 afrittoli

Thanks @pxp928 for moving this to a separate PR!

I'm reviewing the PR, but I find the module structure and test strategy not 100% clear: In my understanding this is the role of each module is:

  • spire.go: defines two interfaces, ControllerAPIClient and EntrypointerAPIClient. (NIT: maybe we call call this clients?)

  • controller.go: implements some of ControllerAPIClient, including type, constructor and some of the required method, except for VerifyTaskRunResults

  • verify.go: implements the rest of ControllerAPIClient, specifically VerifyTaskRunResults (NIT: maybe call this controller_verify?)

  • entrypointer.go: implements some of EntrypointerAPIClient, including type, constructor and some of the required method, except for Sign

  • sign.go: implements the rest of EntrypointerAPIClient, specifically VerifyTaskRunResults, specifically VerifyTaskRunResults (NIT: maybe call this controller_verify?)esults` (NIT: maybe call this entrypointer_sign?)

  • spire_mock.go: defines a mock implementation of both interfaces. This bypasses most of the code in all the actual implementation, but it does invoke some of the actual functions, like verifyManifest and getResultValue

  • spire_mock_tests.go: unit tests for spire_mock.go. These tests verify the logic in the mocks and some private functions in the verify and sign modules, but I find this a bit problematic:

    • it's very opaque what is tested and what not by the unit tests. This will make changing the code in future a rather convoluted task as changing private functions might break mock unit tests that depend on them. We have a coding guideline of testing only exported functions, and this is doing the opposite
    • there is no unit test coverage at all for a lot of the code defined in the actual implementations

The structure I would expect for this is something like a mock client, that implements only functions that already exists in the original client, something like dial, close, get resource, list resources, create resource, delete resource. The ControllerAPIClient and EntrypointerAPIClient would only be responsible for implementing higher abstraction functions like VerifyTaskRunResults and Sign. They would take a client as input, which would be the actual client at runtime, or the mock client at unit test time.

/cc @tektoncd/core-maintainers thoughts?

Let's setup a meeting and to go through this. @lumjjb and I can talk through the intent for each component of the spire package and determine what changes need to be done.

pxp928 avatar Jul 04 '22 14:07 pxp928

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 0.0%
pkg/spire/entrypointer.go Do not exist 0.0%
pkg/spire/sign.go Do not exist 17.6%
pkg/spire/spire_mock.go Do not exist 85.5%
pkg/spire/verify.go Do not exist 17.3%

tekton-robot avatar Jul 04 '22 17:07 tekton-robot

/kind feature

pxp928 avatar Jul 04 '22 17:07 pxp928

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 0.0%
pkg/spire/entrypointer.go Do not exist 0.0%
pkg/spire/sign.go Do not exist 16.7%
pkg/spire/spire_mock.go Do not exist 85.5%
pkg/spire/verify.go Do not exist 16.8%

tekton-robot avatar Jul 11 '22 16:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 0.0%
pkg/spire/entrypointer.go Do not exist 0.0%
pkg/spire/sign.go Do not exist 16.7%
pkg/spire/spire_mock.go Do not exist 85.5%
pkg/spire/verify.go Do not exist 16.8%

tekton-robot avatar Jul 11 '22 17:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 1.0%
pkg/spire/entrypointer.go Do not exist 3.0%
pkg/spire/sign.go Do not exist 16.7%
pkg/spire/spire_mock.go Do not exist 82.4%
pkg/spire/verify.go Do not exist 16.8%

tekton-robot avatar Jul 18 '22 19:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 78.6%

tekton-robot avatar Jul 20 '22 16:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 78.6%

tekton-robot avatar Jul 20 '22 16:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 81.5%

tekton-robot avatar Jul 20 '22 21:07 tekton-robot

@vdemeester @afrittoli @wlynch Should be ready for another review. @lumjjb added the injection piece and I have added the fakeworkloadapi unit tests to test the spire "sign" and "verify" functionality. I have also added some missing pieces to the spire package that are needed for phase 2 (signed TaskRun). This will allow the other two follow on PRs to utilize all the functionality from the spire package for TaskRun results and status signing and verification.

Once the review is complete. I will squash all the commits into one before it gets merged.

pxp928 avatar Jul 20 '22 21:07 pxp928

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 20 '22 21:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 27 '22 13:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 28 '22 15:07 tekton-robot

/retest

pxp928 avatar Jul 28 '22 16:07 pxp928

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 85.3%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 79.8%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 28 '22 22:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 29 '22 13:07 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Jul 29 '22 14:07 tekton-robot

/retest

pxp928 avatar Jul 29 '22 14:07 pxp928

/assign @vdemeester @jerop @pritidesai - assigning TEP approvers to the matching PR review :)

afrittoli avatar Jul 29 '22 14:07 afrittoli

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 83.6%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Aug 04 '22 20:08 tekton-robot

Is this a verbatim copy, or are we making modifications?

Yes the pkg/spire/test folder is a verbatim copy from the upstream project. No modification were made.

pxp928 avatar Aug 04 '22 22:08 pxp928

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 81.4%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Aug 05 '22 21:08 tekton-robot

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 81.4%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Aug 05 '22 21:08 tekton-robot

/retest

pxp928 avatar Aug 05 '22 23:08 pxp928

@wlynch @pritidesai changes made PTAL!

lumjjb avatar Aug 08 '22 15:08 lumjjb

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

File Old Coverage New Coverage Delta
pkg/spire/controller.go Do not exist 37.7%
pkg/spire/entrypointer.go Do not exist 88.9%
pkg/spire/sign.go Do not exist 81.4%
pkg/spire/spire_mock.go Do not exist 80.4%
pkg/spire/verify.go Do not exist 82.1%

tekton-robot avatar Aug 09 '22 00:08 tekton-robot