pipe-cd / pipecd

The One CD for All {applications, platforms, operations}
https://pipecd.dev
Apache License 2.0
1.09k stars 153 forks source link

Validate event context key #5339

Closed ffjlabo closed 1 day ago

ffjlabo commented 2 days ago

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 <ffjlabo@gmail.com>
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 https://github.com/pipe-cd/pipecd/issues/5028

Does this PR introduce a user-facing change?:

ffjlabo commented 2 days ago

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[bot] commented 2 days ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ffjlabo commented 2 days ago

@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 commented 2 days ago

@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 commented 2 days ago

@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
ffjlabo commented 2 days ago

@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"}