[TEP-0091] add KMS field #5891

Yongxuanzhang · 2022-12-19T19:45:25Z

Changes

This commit add KMS into v1alpha1.VerificationPolicy and validation. KMS field is used to configure the KMS path and can be used to resolve keys stored in key management system by cloud providers. Before this commit we only support loading keys from file or raw data.

/kind feature

Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

Submitter Checklist

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

Has Docs included if any changes are user facing
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)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Add KMS filed into v1alpha1.VerificationPolicy

tekton-robot · 2022-12-19T19:45:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go

wlynch

/lgtm

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go

wlynch · 2022-12-19T20:02:44Z

Is there a tracking issue we can associate this with?

Yongxuanzhang · 2022-12-19T20:07:22Z

Is there a tracking issue we can associate this with?

Yes! I have this issue to track all the PRs and milestones
#5527

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go

Yongxuanzhang · 2022-12-19T22:17:23Z

/retest

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go

Yongxuanzhang · 2022-12-20T00:36:34Z

/retest

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go

bendory · 2022-12-20T15:49:27Z

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go

@@ -95,7 +95,7 @@ func TestVerificationPolicy_Invalid(t *testing.T) {
 				},
 			},
 		},
-		want: apis.ErrMissingOneOf("key[0].data", "key[0].secretref"),
+		want: apis.ErrMissingOneOf("data", "kms", "secretref").ViaFieldIndex("key", 0),


Can we make these test cases less fragile? If the given error changes in the implementation, this test will start failing. Maybe there's a way to untie them such that the test case is less fragile?
For example, what if someone has good reason to change the case from kms to KMS in the implementation -- as written, they now have to change the test code as well -- but this is not a functional change, it's just a change in the error message. Can we test for functional change instead of error message?

This comment also applies to the ordering of the error message in index 0. Can we be order-independent such that the test remains valid even if the order of validation changes in the implementation?

This is how we return err for validation in current code base, take this for example:

pipeline/pkg/apis/pipeline/v1beta1/pipelineref_validation.go

Lines 37 to 54 in 17b705c

if ref.Name != "" {

errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver"))

}

if ref.Bundle != "" {

errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver"))

}

}

if ref.Params != nil {

if ref.Name != "" {

errs = errs.Also(apis.ErrMultipleOneOf("name", "params"))

}

if ref.Bundle != "" {

errs = errs.Also(apis.ErrMultipleOneOf("bundle", "params"))

}

if ref.Resolver == "" {

errs = errs.Also(apis.ErrMissingField("resolver"))

}

errs = errs.Also(ValidateParameters(ctx, ref.Params))

I think we should keep it consistent until we want to change them all

I almost have the opposite take from Bendory-- I often give the feedback he is giving, but here I think it actually makes sense to include the text of the error message rather than apis.ErrMultipleOneOf. The reason is that sometimes these helper functions can lead to confusing error message strings, and you don't really realize what output the user is actually seeing until it's reflected in a test. This does have the downside of brittle tests, though.

Re: consistency, I think the section you're referring to is in non-test code, while the comment David is making is a separate point about brittleness of tests.

I almost have the opposite take from Bendory-- I often give the feedback he is giving, but here I think it actually makes sense to include the text of the error message rather than apis.ErrMultipleOneOf. The reason is that sometimes these helper functions can lead to confusing error message strings, and you don't really realize what output the user is actually seeing until it's reflected in a test. This does have the downside of brittle tests, though.

Re: consistency, I think the section you're referring to is in non-test code, while the comment David is making is a separate point about brittleness of tests.

Are you suggesting we should avoid using apis.ErrMissingOneOf?. It seems convenient and also used a lot in other validation functions. https://github.com/tektoncd/pipeline/search?q=apis.ErrMissingOneOf

Only in tests. For example, in #5673 I noted a confusing error message "expected exactly one, got both: spec.taskRef.name, spec.taskRef.params, spec.taskRef.resolver", which comes from ErrMultipleOneOf. These helpers are super useful in validation code but in tests IMO it's useful to be able to see the string error message the user encounters so you can check that it's not confusing.

The downside as David pointed out is that if you change "kms" in the code to "KMS", your tests will fail, but not in a way that gives you useful signal about the code. (The comment about indexing here I'm not sure I agree with, since the "0" refers to the index of the problematic field in the test data rather than an artifact of validation ordering.)

I may be going more in depth than is necessary; feel free to use your best judgment here although I'd just note our tests aren't actually that consistent (example)

Ohh I see your point. Thanks!

chitrangpatel · 2022-12-21T19:56:22Z

/test pull-tekton-pipeline-go-coverage-df

tekton-robot · 2022-12-21T19:56:24Z

@chitrangpatel: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-tekton-pipeline-alpha-integration-tests
/test pull-tekton-pipeline-build-tests
/test pull-tekton-pipeline-integration-tests
/test tekton-pipeline-unit-tests

The following commands are available to trigger optional jobs:

/test pull-tekton-pipeline-go-coverage

Use /test all to run all jobs.

In response to this:

/test pull-tekton-pipeline-go-coverage-df

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.

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go

lbernick · 2023-01-05T20:06:05Z

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go

@@ -95,7 +95,7 @@ func TestVerificationPolicy_Invalid(t *testing.T) {
 				},
 			},
 		},
-		want: apis.ErrMissingOneOf("key[0].data", "key[0].secretref"),
+		want: apis.ErrMissingOneOf("data", "kms", "secretref").ViaFieldIndex("key", 0),


I almost have the opposite take from Bendory-- I often give the feedback he is giving, but here I think it actually makes sense to include the text of the error message rather than apis.ErrMultipleOneOf. The reason is that sometimes these helper functions can lead to confusing error message strings, and you don't really realize what output the user is actually seeing until it's reflected in a test. This does have the downside of brittle tests, though.

Re: consistency, I think the section you're referring to is in non-test code, while the comment David is making is a separate point about brittleness of tests.

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go

This commit add KMS into v1alpha1.VerificationPolicy and validation. KMS field is used to configure the KMS path and can be used to resolve keys stored in key management system by cloud providers. Before this commit we only support loading keys from file or raw data. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com

tekton-robot · 2023-01-05T21:03:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, 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~~ [dibyom,wlynch]

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

lbernick · 2023-01-05T21:10:40Z

/lgtm

I wouldn't actually use a release note here because you're introducing a field that isn't usable yet, so I'm not sure there's actionable info for users.

Yongxuanzhang · 2023-01-05T21:42:29Z

/retest

tekton-robot requested review from afrittoli and dibyom December 19, 2022 19:45

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 19, 2022

Yongxuanzhang marked this pull request as ready for review December 19, 2022 19:49

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 Dec 19, 2022

tekton-robot requested review from bobcatfish and pritidesai December 19, 2022 19:49

wlynch requested changes Dec 19, 2022

View reviewed changes

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go Show resolved Hide resolved

Yongxuanzhang force-pushed the add-kms-field branch from 25ec971 to 4387863 Compare December 19, 2022 20:00

wlynch approved these changes Dec 19, 2022

View reviewed changes

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation_test.go Show resolved Hide resolved

tekton-robot assigned wlynch Dec 19, 2022

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

Yongxuanzhang mentioned this pull request Dec 19, 2022

TEP-0091: Trusted Resources milestone 1 work for Alpha #5527

Closed

15 tasks

jagathprakash reviewed Dec 19, 2022

View reviewed changes

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the add-kms-field branch from 4387863 to 085f468 Compare December 19, 2022 21:57

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022

Yongxuanzhang requested review from jagathprakash and removed request for pritidesai, bobcatfish, afrittoli and dibyom December 19, 2022 22:03

chuangw6 reviewed Dec 19, 2022

View reviewed changes

pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Outdated Show resolved Hide resolved

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the add-kms-field branch from 085f468 to 4481093 Compare December 19, 2022 23:11

bendory reviewed Dec 20, 2022

View reviewed changes

Yongxuanzhang force-pushed the add-kms-field branch 2 times, most recently from 72a590b to 634649c Compare December 20, 2022 16:55

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

Yongxuanzhang force-pushed the add-kms-field branch from 634649c to 387c0d2 Compare January 3, 2023 19:55

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2023

JeromeJu mentioned this pull request Jan 3, 2023

Add taskrun.status.cloudEvents to deprecation.md #5937

Merged

6 tasks

Yongxuanzhang force-pushed the add-kms-field branch from 387c0d2 to 0203a59 Compare January 4, 2023 17:40

dibyom approved these changes Jan 5, 2023

View reviewed changes

pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Show resolved Hide resolved

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

lbernick reviewed Jan 5, 2023

View reviewed changes

Yongxuanzhang force-pushed the add-kms-field branch from 0203a59 to 7ca8a5c Compare January 5, 2023 21:03

tekton-robot assigned lbernick Jan 5, 2023

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

tekton-robot merged commit fa2e372 into tektoncd:main Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0091] add KMS field #5891

[TEP-0091] add KMS field #5891

Yongxuanzhang commented Dec 19, 2022 •

edited

Loading

tekton-robot commented Dec 19, 2022

wlynch left a comment

wlynch commented Dec 19, 2022

Yongxuanzhang commented Dec 19, 2022

Yongxuanzhang commented Dec 19, 2022

Yongxuanzhang commented Dec 20, 2022

bendory Dec 20, 2022

bendory Dec 20, 2022

Yongxuanzhang Dec 20, 2022

lbernick Jan 5, 2023

Yongxuanzhang Jan 5, 2023

lbernick Jan 5, 2023

Yongxuanzhang Jan 5, 2023

chitrangpatel commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

lbernick Jan 5, 2023

tekton-robot commented Jan 5, 2023

lbernick commented Jan 5, 2023

Yongxuanzhang commented Jan 5, 2023

	if ref.Name != "" {
	errs = errs.Also(apis.ErrMultipleOneOf("name", "resolver"))
	}
	if ref.Bundle != "" {
	errs = errs.Also(apis.ErrMultipleOneOf("bundle", "resolver"))
	}
	}
	if ref.Params != nil {
	if ref.Name != "" {
	errs = errs.Also(apis.ErrMultipleOneOf("name", "params"))
	}
	if ref.Bundle != "" {
	errs = errs.Also(apis.ErrMultipleOneOf("bundle", "params"))
	}
	if ref.Resolver == "" {
	errs = errs.Also(apis.ErrMissingField("resolver"))
	}
	errs = errs.Also(ValidateParameters(ctx, ref.Params))

[TEP-0091] add KMS field #5891

[TEP-0091] add KMS field #5891

Conversation

Yongxuanzhang commented Dec 19, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 19, 2022

wlynch left a comment

Choose a reason for hiding this comment

wlynch commented Dec 19, 2022

Yongxuanzhang commented Dec 19, 2022

Yongxuanzhang commented Dec 19, 2022

Yongxuanzhang commented Dec 20, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitrangpatel commented Dec 21, 2022

tekton-robot commented Dec 21, 2022

Choose a reason for hiding this comment

tekton-robot commented Jan 5, 2023

lbernick commented Jan 5, 2023

Yongxuanzhang commented Jan 5, 2023

Yongxuanzhang commented Dec 19, 2022 •

edited

Loading