Validate event context key #5339

ffjlabo · 2024-11-14T08:08:56Z

What this PR does:

Added validation for event context key.

The event context is used as --trailer. The trailer key is currently allowed to use - alphabet, number, ' ', '\t' but it is unclear the actual format.
ref: https://github.com/git/git/blob/25b0f41288718625b18495de23cc066394c09a92/trailer.c#L630-L646

Also tried some formats below.

% git commit -m "Test" --trailer "github.repository: test" --trailer "github_repository: test" --trailer "github-repository: test"
% git show                                                                                         
commit bedaed31b10c454e730a7aa06abb118a4cd3c264 (HEAD -> test-trailer)
Author: Yoshiki Fujikane <[email protected]>
Date:   Tue Nov 12 18:19:55 2024 +0900

    Test

    github.repository: test:
    github_repository: test:
    github-repository: test

So I added the restriction for the event context key format like kebab case such asTest-hoge test-hoge for now.

Why we need it:

Which issue(s) this PR fixes:

Part of #5028

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-11-14T08:12:43Z

The pattern

OK (tried test-hoge)

% ./pipectl_v0.49.3_darwin_arm64 event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --contexts="test-hoge=fuga" --insecure=true
Successfully registered event	{"id": "cc64b59d-82d1-4d3d-a669-b6051e9fe33b"}

Invalid key format (tried test.hoge)

% ./pipectl_v0.49.3_darwin_arm64 event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --contexts="test.hoge=fuga" --insecure=true
2024/11/14 16:56:22 failed to register event: rpc error: code = InvalidArgument desc = invalid request: invalid RegisterEventRequest.Contexts[test.hoge]: value does not match regex pattern "^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$"

Key is nothing

% ./pipectl_v0.49.3_darwin_arm64 event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --contexts="=fuga" --insecure=true
2024/11/14 16:46:29 failed to register event: rpc error: code = InvalidArgument desc = invalid request: invalid RegisterEventRequest.Contexts[]: value length must be at least 1 rune

codecov · 2024-11-14T08:13:12Z

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 25.30%. Comparing base (249315b) to head (179eef2).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/app/pipectl/cmd/event/register.go	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5339      +/-   ##
==========================================
- Coverage   25.31%   25.30%   -0.02%     
==========================================
  Files         444      444              
  Lines       47592    47601       +9     
==========================================
- Hits        12050    12044       -6     
- Misses      34600    34615      +15     
  Partials      942      942

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Warashi

commented on regexp and nits.

IMHO, the error message like below is a bit confusing.
The validation error occurs at the key of the map, but the message says "value."

2024/11/14 16:46:29 failed to register event: rpc error: code = InvalidArgument desc = invalid request: invalid RegisterEventRequest.Contexts[]: value length must be at least 1 rune

Warashi · 2024-11-14T08:17:51Z

pkg/model/event.go

+var (
+	hashFunc = sha256.New
+
+	eventKeyFormatRegex = regexp.MustCompile(`^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$`)


[nits] This variable seems unused

Fixed 🙏 341ce20

Warashi · 2024-11-14T08:23:48Z

pkg/app/server/service/apiservice/service.proto

@@ -229,7 +229,7 @@ message RegisterEventRequest {
    string name = 1 [(validate.rules).string.min_len = 1];
    string data = 2 [(validate.rules).string.min_len = 1];
    map<string,string> labels = 3 [(validate.rules).map.keys.string.min_len = 1, (validate.rules).map.values.string.min_len = 1];
-    map<string,string> contexts = 4;
+    map<string,string> contexts = 4 [(validate.rules).map.keys.string.min_len = 1, (validate.rules).map.keys.string.pattern = "^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$", (validate.rules).map.values.string.min_len = 1];


The regexp might be wrong.
The git implementations seem to allow double or more hyphens continuously like test--hoge or start with a hyphen like -test.
The pattern might be like ^[\-a-zA-Z0-9]+$.

@Warashi
Yes, I know that.
I don't plan to do the same as validation for git.
It is because the trailer format is not so concrete.

So I wanted to express the format like a kebab case test-hoge in PipeCD.

Okay, I got your point.

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-11-14T08:55:28Z

@Warashi

commented on regexp and nits.

IMHO, the error message like below is a bit confusing.
The validation error occurs at the key of the map, but the message says "value."

I see. Thank you for finding it. This is based on the code generated by protoc-gen-validate.
In my understanding, many validations are based on it.
Is there any idea?

Warashi · 2024-11-14T09:01:37Z

@ffjlabo

I see. Thank you for finding it. This is based on the code generated by protoc-gen-validate.
In my understanding, many validations are based on it.
Is there any idea?

How about implementing validations in pipectl code with go's regexp?
In my understanding, many parts of error messages from protoc-gen-validate will not be seen by users, but this is seen by users usually. So it's better that we can control the messages.

ffjlabo · 2024-11-14T09:11:24Z

@Warashi
Thanks

In my understanding, many parts of error messages from protoc-gen-validate will not be seen by users, but this is seen by users usually. So it's better that we can control the messages.

I got it. Sounds nice!
So we would accept the message on the server side and implement the validation in pipectl.

📝 It seems that these errors occurred when using pipectl.
e.g.

./pipectl_v0.49.3_darwin_arm64 event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --labels="=fuga" --insecure=true
2024/11/14 18:06:51 failed to register event: rpc error: code = InvalidArgument desc = invalid request: invalid RegisterEventRequest.Labels[]: value length must be at least 1 runes

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo · 2024-11-14T09:34:48Z

@Warashi Fixed in 179eef2

like this

go run cmd/pipectl/main.go event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --contexts="test_hoge=fuga" --insecure=true
2024/11/14 18:32:34 failed to validate event context: invalid format key 'test_hoge', should be ^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$
exit status 1

It also works OK pattern.

go run cmd/pipectl/main.go event register --address=localhost:8080 --api-key=xxx --name=simple --data="gcr.io/pipecd/helloworld:v0.40.0" --contexts="test-hoge=fuga" --insecure=true
Successfully registered event	{"id": "c256dfc4-d301-4cad-89f7-fb5afa87f31b"}

Warashi

LGTM

khanhtc1202 · 2024-11-15T06:25:09Z

pkg/app/pipectl/cmd/event/register.go

@@ -79,3 +86,13 @@ func (r *register) run(ctx context.Context, input cli.Input) error {
 	)
 	return nil
 }
+
+func (r *register) validateEventContexts() error {


This function looks testable, please add tests for this later (another PR is ok, since this one is for rc release)

cc @ffjlabo

khanhtc1202

👌

Signed-off-by: pipecd-bot <[email protected]>

* Sort trailers to ensure the order (#5334) * Sort trailers to ensure the order Signed-off-by: Yoshiki Fujikane <[email protected]> * Fixed to work on go1.22 Signed-off-by: Yoshiki Fujikane <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Validate event context key (#5339) Signed-off-by: pipecd-bot <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]>

ffjlabo added 4 commits November 14, 2024 16:49

Add validation for event context key

e2dd765

Signed-off-by: Yoshiki Fujikane <[email protected]>

Add validation in request for RegisterEvent

0844106

Signed-off-by: Yoshiki Fujikane <[email protected]>

Fix error for failing to add event

c375105

Signed-off-by: Yoshiki Fujikane <[email protected]>

Add key format description

3bd7f8b

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo requested review from nghialv, khanhtc1202, kentakozuka, t-kikuc and Warashi as code owners November 14, 2024 08:08

github-actions bot added the area/go label Nov 14, 2024

Warashi reviewed Nov 14, 2024

View reviewed changes

Remove unused regex

341ce20

Signed-off-by: Yoshiki Fujikane <[email protected]>

Add validation to pipectl side

179eef2

Signed-off-by: Yoshiki Fujikane <[email protected]>

ffjlabo added cherry-pick v0.49.4-rc1 labels Nov 14, 2024

ffjlabo requested a review from Warashi November 14, 2024 09:37

Warashi approved these changes Nov 14, 2024

View reviewed changes

ffjlabo enabled auto-merge (squash) November 15, 2024 01:39

khanhtc1202 reviewed Nov 15, 2024

View reviewed changes

khanhtc1202 approved these changes Nov 15, 2024

View reviewed changes

ffjlabo merged commit 519d3fc into master Nov 15, 2024
16 of 18 checks passed

ffjlabo deleted the validate-event-context-key branch November 15, 2024 06:25

github-actions bot pushed a commit that referenced this pull request Nov 19, 2024

Validate event context key (#5339)

0eb357e

Signed-off-by: pipecd-bot <[email protected]>

github-actions bot mentioned this pull request Nov 19, 2024

Cherry-pick #5334 #5339 #5345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate event context key #5339

Validate event context key #5339

ffjlabo commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

Warashi left a comment

Warashi Nov 14, 2024

ffjlabo Nov 14, 2024

Warashi Nov 14, 2024

ffjlabo Nov 14, 2024

Warashi Nov 14, 2024

ffjlabo commented Nov 14, 2024

Warashi commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

Warashi left a comment

khanhtc1202 Nov 15, 2024

khanhtc1202 Nov 15, 2024

khanhtc1202 left a comment

Validate event context key #5339

Validate event context key #5339

Conversation

ffjlabo commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

The pattern

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

Warashi left a comment

Choose a reason for hiding this comment

Warashi Nov 14, 2024

Choose a reason for hiding this comment

ffjlabo Nov 14, 2024

Choose a reason for hiding this comment

Warashi Nov 14, 2024

Choose a reason for hiding this comment

ffjlabo Nov 14, 2024

Choose a reason for hiding this comment

Warashi Nov 14, 2024

Choose a reason for hiding this comment

ffjlabo commented Nov 14, 2024

Warashi commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

ffjlabo commented Nov 14, 2024

Warashi left a comment

Choose a reason for hiding this comment

khanhtc1202 Nov 15, 2024

Choose a reason for hiding this comment

khanhtc1202 Nov 15, 2024

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2024 •

edited

Loading