[TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. #6627

jagathprakash · 2023-05-05T15:08:37Z

This PR injects the spireControllerAPIClient into the pipelines controller and the taskrun reconciler. It makes it available in these objects to be used for signing and verification of the taskrunResults and the taskrun object itself.

Before this change the spireAPIController object was not injected into the taskRun and as such SPIRE was not available to be used.

After this change,

spireApiController will be available to be used by the pipeline controller and the taskrun object.
The spireApiController will be update with the spire config whenever the config changes.

This commit is part of a series of PRs to implement TEP-0089. The implementation of TEP-0089 is tracked in the issue #6597.

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including functionality, content, code)
Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

jagathprakash · 2023-05-05T15:09:01Z

/kind feature

jagathprakash · 2023-05-05T15:09:27Z

/assign @jerop , @chuangw6

tekton-robot · 2023-05-05T15:14:00Z

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/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-05T15:15:20Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

pkg/spire/controller.go

pkg/spire/spire_mock.go

pkg/spire/spire_test.go

pkg/spire/controller.go

chuangw6 · 2023-05-08T22:52:02Z

pkg/spire/spire_test.go

+	}
+	OnStore(ctx, logger)(pconf.GetSpireConfigName(), &want)
+	got := *GetControllerAPIClient(ctx).(*spireControllerAPIClient).config
+	if got != want {


It would be helpful to add some test cases for which the configmap will not be updated i.e. invalid values.

As explained in OnStore comment, we will never get bad values here in the normal flow. We could get default values if the validation fails.

tekton-robot · 2023-05-09T16:01:06Z

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/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-09T16:02:13Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

dibyom

one question and a minor comment but otherwise LGTM

pkg/reconciler/taskrun/controller.go

dibyom · 2023-05-11T19:31:03Z

pkg/spire/controller.go

@@ -45,14 +47,30 @@ func init() {
 // controllerKey is a way to associate the ControllerAPIClient from inside the context.Context
 type controllerKey struct{}

+// OnStore stores the changed spire config into the SpireClientApi
+func OnStore(ctx context.Context, logger *zap.SugaredLogger) func(name string, value interface{}) {


question: why do we need to implement OnStore? Are we supporting changing the SPIRE client without a controller restart if the config changes?

The configStore reads in the config and stores it into its local structures. Spire controller maintains its own local config. This does not get updated with the changes in the actual configMap. Essentially the Spire ConfigMap does not make it to the Spire Controller without this callback.

One way to get around this without the callback could be to load from the ConfigStore into the SpireCntroller at the beginning of the reconciler as done for other configs whcih get copied into the. context.

pipeline/pkg/client/injection/reconciler/pipeline/v1beta1/taskrun/reconciler.go

Line 190 in 8f76a04

if r.configStore != nil {

which will result in something like this.
// If configStore is set, attach the frozen configuration to the context.
if r.configStore != nil {
ctx = r.configStore.ToContext(ctx)
r.SpireClient.SetConfig(ctx)
}

WDYT?

ah I see...I think the current impl is fine

tekton-robot · 2023-05-11T19:31:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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~~ [dibyom]

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

dibyom · 2023-05-11T19:32:48Z

@jagathprakash the coverage for pkg/spire/controller.go seems low (42%) - is there a PR that will increase this?

jagathprakash · 2023-05-16T17:12:14Z

There is no PR planned for this, but I can create one which increases the coverage.

tekton-robot · 2023-05-16T17:15: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	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-16T17:17:43Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

chuangw6 · 2023-05-16T18:38:59Z

Can we clean up the PR description i.e. the descriptions before and after #changes?

jagathprakash · 2023-05-18T15:22:37Z

Can we clean up the PR description i.e. the descriptions before and after #changes?

@chuangw6 I have updated the description. PTAL.

chuangw6 · 2023-05-18T17:36:44Z

@jagathprakash Thank you!

If you don't mind, can we just use the following as both commit message and the PR description (remove the text before the title #change)?

This PR injects the spireControllerAPIClient into the pipelines controller and the taskrun reconciler. It makes it available in these objects to be used for signing and verification of the taskrunResults and the taskrun object itself.

Before this change the spireAPIController object was not injected into the taskRun and as such SPIRE was not available to be used.

After this change,
- spireApiController will be available to be used by the pipeline controller and the taskrun object.
- The spireApiController will be update with the spire config whenever the config changes.

This commit is part of a series of PRs to implement TEP-0089. The implementation of TEP-0089 is tracked in the issue https://github.com/tektoncd/pipeline/issues/6597.

jagathprakash · 2023-05-18T19:16:58Z

@jagathprakash Thank you!

If you don't mind, can we just use the following as both commit message and the PR description (remove the text before the title #change)?

This PR injects the spireControllerAPIClient into the pipelines controller and the taskrun reconciler. It makes it available in these objects to be used for signing and verification of the taskrunResults and the taskrun object itself.

Before this change the spireAPIController object was not injected into the taskRun and as such SPIRE was not available to be used.

After this change,
- spireApiController will be available to be used by the pipeline controller and the taskrun object.
- The spireApiController will be update with the spire config whenever the config changes.

This commit is part of a series of PRs to implement TEP-0089. The implementation of TEP-0089 is tracked in the issue https://github.com/tektoncd/pipeline/issues/6597.

Done.

tekton-robot · 2023-05-18T19:21:46Z

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/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-18T19:26:17Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

…r and reconciler This PR injects the spireControllerAPIClient into the pipelines controller and the taskrun reconciler. It makes it available in these objects to be used for signing and verification of the taskrunResults and the taskrun object itself. Before this change the spireAPIController object was not injected into the taskRun and as such SPIRE was not available to be used. After this change, - spireApiController will be available to be used by the pipeline controller and the taskrun object. - The spireApiController will be update with the spire config whenever the config changes. This commit is part of a series of PRs to implement TEP-0089. The implementation of TEP-0089 is tracked in the issue https://github.com/tektoncd/pipeline/issues/6597.[TEP-0089] SPIRE for non-falsifiable provenance. Inject SpireControllerAPIClient into the controller and the taskrun reconciler. This commit is part of a series of PRs to implement TEP-0089. The implementation of TEP-0089 is tracked in the issue [tektoncd#6597](tektoncd#6597).

tekton-robot · 2023-05-18T19:41: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	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-18T19:44:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

chuangw6 · 2023-05-18T19:44:55Z

/lgtm

jagathprakash · 2023-05-18T21:39:25Z

/retest

chuangw6 · 2023-05-19T15:01:23Z

/retest

maybe try close and reopen the pr?

tekton-robot · 2023-05-19T15:44:07Z

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/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

tekton-robot · 2023-05-19T15:45:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/taskrun/controller.go	95.0%	95.2%	0.2
pkg/spire/controller.go	37.7%	42.9%	5.1

jagathprakash · 2023-05-19T19:34:07Z

/retest

maybe try close and reopen the pr?

That worked! Thanks!

tekton-robot added the release-note-none Denotes a PR that doesnt merit a release note. label May 5, 2023

tekton-robot requested review from bobcatfish and dibyom May 5, 2023 15:08

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 5, 2023

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 5, 2023

jagathprakash changed the title ~~[TEP-0089] SPIRE for non-falsifiable provenance.~~ [TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. May 5, 2023

jagathprakash mentioned this pull request May 5, 2023

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

Open

10 tasks

jerop assigned jerop and chuangw6 May 5, 2023

jerop reviewed May 8, 2023

View reviewed changes

pkg/spire/controller.go Outdated Show resolved Hide resolved

chuangw6 reviewed May 8, 2023

View reviewed changes

jagathprakash force-pushed the spire-onstore branch from d44084e to c6f0e13 Compare May 9, 2023 15:53

jagathprakash requested review from jerop and chuangw6 May 11, 2023 15:05

dibyom approved these changes May 11, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2023

jagathprakash force-pushed the spire-onstore branch from c6f0e13 to 810803d Compare May 16, 2023 17:10

jagathprakash requested a review from dibyom May 16, 2023 17:12

jagathprakash force-pushed the spire-onstore branch from 810803d to 72d1dc5 Compare May 18, 2023 19:16

jagathprakash force-pushed the spire-onstore branch from 72d1dc5 to 4f00e51 Compare May 18, 2023 19:34

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2023

jagathprakash closed this May 19, 2023

jagathprakash reopened this May 19, 2023

tekton-robot merged commit 4c9880d into tektoncd:main May 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. #6627

[TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. #6627

jagathprakash commented May 5, 2023 •

edited

Loading

jagathprakash commented May 5, 2023

jagathprakash commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

chuangw6 May 8, 2023

jagathprakash May 9, 2023

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

dibyom left a comment

dibyom May 11, 2023

jagathprakash May 16, 2023

dibyom May 16, 2023

tekton-robot commented May 11, 2023

dibyom commented May 11, 2023

jagathprakash commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

chuangw6 commented May 16, 2023

jagathprakash commented May 18, 2023

chuangw6 commented May 18, 2023

jagathprakash commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

chuangw6 commented May 18, 2023

jagathprakash commented May 18, 2023

chuangw6 commented May 19, 2023

tekton-robot commented May 19, 2023

tekton-robot commented May 19, 2023

jagathprakash commented May 19, 2023

[TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. #6627

[TEP-0089] Inject SpireControllerAPIClient into the Taskrun controller and reconciler. #6627

Conversation

jagathprakash commented May 5, 2023 • edited Loading

Submitter Checklist

Release Notes

jagathprakash commented May 5, 2023

jagathprakash commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

chuangw6 May 8, 2023

Choose a reason for hiding this comment

jagathprakash May 9, 2023

Choose a reason for hiding this comment

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

dibyom left a comment

Choose a reason for hiding this comment

dibyom May 11, 2023

Choose a reason for hiding this comment

jagathprakash May 16, 2023

Choose a reason for hiding this comment

dibyom May 16, 2023

Choose a reason for hiding this comment

tekton-robot commented May 11, 2023

dibyom commented May 11, 2023

jagathprakash commented May 16, 2023

tekton-robot commented May 16, 2023

tekton-robot commented May 16, 2023

chuangw6 commented May 16, 2023

jagathprakash commented May 18, 2023

chuangw6 commented May 18, 2023

jagathprakash commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

tekton-robot commented May 18, 2023

chuangw6 commented May 18, 2023

jagathprakash commented May 18, 2023

chuangw6 commented May 19, 2023

tekton-robot commented May 19, 2023

tekton-robot commented May 19, 2023

jagathprakash commented May 19, 2023

jagathprakash commented May 5, 2023 •

edited

Loading