[TEP-0089] - Spire Package #5039

pxp928 · 2022-06-25T21:15:41Z

Signed-off-by: pxp928 parth.psu@gmail.com

Changes

Spire package separated out from #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
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 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

tekton-robot · 2022-06-25T21:15:57Z

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.

afrittoli · 2022-06-28T14:43:16Z

/ok-to-test

tekton-robot · 2022-06-28T15:16:30Z

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
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%

afrittoli · 2022-07-04T10:56:47Z

pkg/spire/controller.go

+func NewSpireControllerAPIClient(c spireconfig.SpireConfig) ControllerAPIClient {
+	if c.MockSpire {
+		return &MockClient{}
+	}
+	return &spireControllerAPIClient{
+		config: c,
+	}
+}


I would prefer to have two different factories here, one the tests and one for the actual usage, instead of embedding a MockSpire attribute in the client struct spireControllerAPIClient, it would be more consistent with all the other clients used in the controllers, like Tekton API clients, cloudevents client and cache client.

For instance, if you look at the https://github.com/tektoncd/pipeline/tree/main/pkg/reconciler/events/cache package, there are two clients defined there, the regular one and the mock one.

The knative/pkg injection machinery makes sure that the correct client (regular or mock) is injected into the context, so that when you get the client from the context you get a different one, depending on whether it's unit tests or actual running code.

@mattmoor may want to have a say on this too 🙏

Hmm I see. I might need some help setting this up.

If you don't want to deal with client injection, you could just pass a client rather than the config options in for the reconciler options.

Or since this client creation isn't actually doing too much, test clients could also just override this after the reconciler is created.

seems do-able, does this look about right? https://github.com/pxp928/pipeline/pull/26/files

Thanks @lumjjb - that's definitely in the right direction!

The only question I have is whether we could get the config from the context, so that when the client are setup the config is added directly, and we don't need to add it later on. Ideally we should add the client config once, unless we need to support reloading configuration changes at run time. Wdyt?

Thanks @afrittoli ! It's in a weird spot with the initialization of the TaskRun reconciler, since some of the options come in as CLI arguments. Do you have any recommendations to handle this?

afrittoli · 2022-07-04T13:15:11Z

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

A few comments, I've not completed the review yet, but I would like to clarify the overall code structure first.

pkg/spire/entrypointer.go

pkg/spire/config/config.go

pkg/spire/controller.go

pkg/spire/spire.go

pkg/spire/sign.go

pkg/spire/verify.go

pxp928 · 2022-07-04T14:11:22Z

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.

tekton-robot · 2022-07-04T17:17:55Z

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
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%

pxp928 · 2022-07-04T17:27:50Z

/kind feature

pkg/spire/controller.go

wlynch · 2022-07-06T18:55:41Z

pkg/spire/controller.go

+func NewSpireControllerAPIClient(c spireconfig.SpireConfig) ControllerAPIClient {
+	if c.MockSpire {
+		return &MockClient{}
+	}
+	return &spireControllerAPIClient{
+		config: c,
+	}
+}


If you don't want to deal with client injection, you could just pass a client rather than the config options in for the reconciler options.

Or since this client creation isn't actually doing too much, test clients could also just override this after the reconciler is created.

pkg/spire/entrypointer.go

pkg/spire/sign.go

pkg/spire/spire.go

pkg/spire/spire_mock.go

wlynch · 2022-07-06T19:03:13Z

pkg/spire/verify.go

+		return nil, err
+	}
+	trustPool := x509.NewCertPool()
+	for _, c := range x509Bundle[0].X509Authorities() {


Should we be looping over all bundles? Would also help protect if len(x509Bundle) == 0

A trust bundle is a collection of one or more CA root certificates that the workload should consider trustworthy. So it depends on how the spire server is configured. Hence the looping. Added the len(x509Bundle) check

I could have phrased that better - I meant should we be looping over x509Bundle (as opposed to just the authorities in first one) i.e.

trustPool := x509.NewCertPool() for _, bundle := range x509Bundle { for _, c := range bundle.X509Authorities() { trustPool.AddCert(c) } return trustPool, nil }

Yup. Added.

pkg/spire/verify.go

vdemeester

Agreeing with @wlynch and @afrittoli, in term of code organization especially for test.

vdemeester · 2022-07-07T08:06:40Z

pkg/spire/config/config.go

+	}
+	if len(unset) > 0 {
+		sort.Strings(unset)
+		return fmt.Errorf("found unset image flags: %s", unset)


I don't think the error is correct here (no image involevd)

This is part of the phase 1 PR. This PR is missing this context as this is just the breakout for the spire package. The flags are coming from the pipeline controller when its image is initialized.

flag.StringVar(&opts.SpireConfig.TrustDomain, "spire-trust-domain", "example.org", "Experimental: The SPIRE Trust domain to use.") flag.StringVar(&opts.SpireConfig.SocketPath, "spire-socket-path", "/spiffe-workload-api/spire-agent.sock", "Experimental: The SPIRE agent socket for SPIFFE workload API.") flag.StringVar(&opts.SpireConfig.ServerAddr, "spire-server-addr", "spire-server.spire.svc.cluster.local:8081", "Experimental: The SPIRE server address for workload/node registration.") flag.StringVar(&opts.SpireConfig.NodeAliasPrefix, "spire-node-alias-prefix", "/tekton-node/", "Experimental: The SPIRE node alias prefix to use.")

I think it will be great to make this error more descriptive. This error will be met when SpireConfig is missing all four required flags which in turn means no image initialized 🤔

changed to "found unset spire configuration flags"

tekton-robot · 2022-07-11T16:49:56Z

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
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 · 2022-07-11T17:22:48Z

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
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 · 2022-07-18T19:33:54Z

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
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 · 2022-08-09T00:54:34Z

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
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%

pxp928 · 2022-08-09T01:46:36Z

/retest

lumjjb · 2022-08-09T13:12:51Z

/retest

lumjjb · 2022-08-12T13:31:06Z

Hi @pritidesai we've made your requested! Can you take a look at the PR again - would like to get this across the line! Thanks!

Signed-off-by: pxp928 <parth.psu@gmail.com>

tekton-robot · 2022-08-31T14:16:24Z

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
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%

wlynch

/lgtm

@pritidesai can you take another look at this for approval? 👀

vdemeester

/hold
@pritidesai @afrittoli one last review before removing the hold 🙏🏼

tekton-robot · 2022-09-15T08:48:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester, wlynch

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:

~~OWNERS~~ [vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wlynch · 2022-09-19T17:02:57Z

Since this PR already has an LGTM from me and has been sitting without additional feedback for weeks, I'm going to go ahead and remove the hold to let this submit.

Nothing in this PR is a one-way door decision and is not wired up to the controller yet, so there's not much risk in submitting this even if there is additional feedback. Feel free to continue to leave feedback on this PR - we can follow up with additional PRs if needed!

/remove-hold

afrittoli · 2022-09-20T16:21:46Z

Great to see this moving forward! I'm sorry I wasn't able to re-review before, but I'm glad to see progress.

tekton-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2022

tekton-robot requested review from jerop and lbernick June 25, 2022 21:15

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 25, 2022

pxp928 mentioned this pull request Jun 25, 2022

[TEP-0089] - Phase 1 Signed TaskRun Results #4759

Closed

5 tasks

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2022

afrittoli added this to the Pipelines v0.38 milestone Jun 28, 2022

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 4, 2022

afrittoli reviewed Jul 4, 2022

View reviewed changes

pxp928 force-pushed the package-spire branch from 325e445 to e5b5ed1 Compare July 4, 2022 17:13

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 4, 2022

wlynch requested changes Jul 6, 2022

View reviewed changes

vdemeester reviewed Jul 7, 2022

View reviewed changes

pxp928 force-pushed the package-spire branch from e5b5ed1 to c733cc1 Compare July 11, 2022 16:44

pxp928 force-pushed the package-spire branch 2 times, most recently from f58c601 to 5a536d4 Compare July 11, 2022 17:18

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 11, 2022

pxp928 requested a review from pritidesai August 17, 2022 20:06

pritidesai modified the milestones: Pipelines v0.39, Pipelines v0.40 Aug 19, 2022

added spire package with unit tests

3e5be56

Signed-off-by: pxp928 <parth.psu@gmail.com>

pxp928 force-pushed the package-spire branch from 44f265a to 3e5be56 Compare August 31, 2022 14:11

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

wlynch approved these changes Sep 2, 2022

View reviewed changes

tekton-robot assigned wlynch Sep 2, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2022

vdemeester approved these changes Sep 15, 2022

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 15, 2022

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2022

vdemeester modified the milestones: Pipelines v0.40, Pipelines v0.41 Sep 19, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2022

tekton-robot merged commit 2272ded into tektoncd:main Sep 19, 2022

jagathprakash mentioned this pull request Feb 22, 2023

[TEP-0089] SPIRE for non-falsifiable provenance. #6160

Closed

7 tasks

This was referenced Apr 12, 2023

[TEP-0089] SPIRE for non-falsifiable provenance - IsSpireEnabled #6524

Merged

[TEP-0089] Add CSI volumes to the Pods which provide the SPIRE workload API #6539

Merged

This was referenced Apr 28, 2023

[TEP-0089] Enforce non-falsifiable provenance using SPIRE #6597

Open

[TEP-0089] SPIRE for non-falsifiable provenance. Setup the test environment. #6553

Merged

[TEP-0089] - Spire Package #5039

[TEP-0089] - Spire Package #5039

Conversation

pxp928 commented Jun 25, 2022 • edited by afrittoli Loading

Changes

Submitter Checklist

tekton-robot commented Jun 25, 2022

afrittoli commented Jun 28, 2022

tekton-robot commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxp928 Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli commented Jul 4, 2022

afrittoli left a comment

Choose a reason for hiding this comment

pxp928 commented Jul 4, 2022 • edited Loading

tekton-robot commented Jul 4, 2022

pxp928 commented Jul 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pxp928 Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

wlynch Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Jul 11, 2022

tekton-robot commented Jul 11, 2022

tekton-robot commented Jul 18, 2022

tekton-robot commented Aug 9, 2022

pxp928 commented Aug 9, 2022

lumjjb commented Aug 9, 2022

lumjjb commented Aug 12, 2022

tekton-robot commented Aug 31, 2022

wlynch left a comment

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Sep 15, 2022

wlynch commented Sep 19, 2022

afrittoli commented Sep 20, 2022

pxp928 commented Jun 25, 2022 •

edited by afrittoli

Loading

pxp928 Jul 4, 2022 •

edited

Loading

pxp928 commented Jul 4, 2022 •

edited

Loading

pxp928 Jul 11, 2022 •

edited

Loading

wlynch Jul 12, 2022 •

edited

Loading