Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate event context key #5339

Merged
merged 6 commits into from
Nov 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion pkg/app/pipectl/cmd/event/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import (
"context"
"fmt"
"regexp"

"github.com/spf13/cobra"
"go.uber.org/zap"
Expand All @@ -25,6 +26,8 @@
"github.com/pipe-cd/pipecd/pkg/cli"
)

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

type register struct {
root *command

Expand All @@ -47,7 +50,7 @@
cmd.Flags().StringVar(&r.name, "name", r.name, "The name of event.")
cmd.Flags().StringVar(&r.data, "data", r.data, "The string value of event data.")
cmd.Flags().StringToStringVar(&r.labels, "labels", r.labels, "The list of labels for event. Format: key=value,key2=value2")
cmd.Flags().StringToStringVar(&r.contexts, "contexts", r.contexts, "The list of the values for the event context. Format: key=value,key2=value2")
cmd.Flags().StringToStringVar(&r.contexts, "contexts", r.contexts, "The list of the values for the event context. Format: key=value,key2=value2. The Key Format is [a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$")

Check warning on line 53 in pkg/app/pipectl/cmd/event/register.go

View check run for this annotation

Codecov / codecov/patch

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

Added line #L53 was not covered by tests

cmd.MarkFlagRequired("name")
cmd.MarkFlagRequired("data")
Expand All @@ -56,6 +59,10 @@
}

func (r *register) run(ctx context.Context, input cli.Input) error {
if err := r.validateEventContexts(); err != nil {
return fmt.Errorf("failed to validate event context: %w", err)
}

Check warning on line 64 in pkg/app/pipectl/cmd/event/register.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipectl/cmd/event/register.go#L62-L64

Added lines #L62 - L64 were not covered by tests

cli, err := r.root.clientOptions.NewClient(ctx)
if err != nil {
return fmt.Errorf("failed to initialize client: %w", err)
Expand All @@ -79,3 +86,13 @@
)
return nil
}

func (r *register) validateEventContexts() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for key := range r.contexts {
if !eventKeyFormatRegex.MatchString(key) {
return fmt.Errorf("invalid format key '%s', should be ^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$", key)
}

Check warning on line 94 in pkg/app/pipectl/cmd/event/register.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipectl/cmd/event/register.go#L90-L94

Added lines #L90 - L94 were not covered by tests
}

return nil

Check warning on line 97 in pkg/app/pipectl/cmd/event/register.go

View check run for this annotation

Codecov / codecov/patch

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

Added line #L97 was not covered by tests
}
490 changes: 247 additions & 243 deletions pkg/app/server/service/apiservice/service.pb.go

Large diffs are not rendered by default.

50 changes: 49 additions & 1 deletion pkg/app/server/service/apiservice/service.pb.validate.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/app/server/service/apiservice/service.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]+$.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I got your point.

}

message RegisterEventResponse {
Expand Down
2 changes: 1 addition & 1 deletion pkg/datastore/eventstore.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (s *eventStore) Add(ctx context.Context, e model.Event) error {
e.UpdatedAt = now
}
if err := e.Validate(); err != nil {
return err
return fmt.Errorf("failed to validate event: %w: %w", ErrInvalidArgument, err)
}
return s.ds.Create(ctx, s.col, e.Id, &e)
}
Expand Down
58 changes: 31 additions & 27 deletions pkg/model/event.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 28 additions & 1 deletion pkg/model/event.pb.validate.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion pkg/model/event.proto
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,10 @@ message Event {
EventStatus status = 8 [(validate.rules).enum.defined_only = true];
string status_description = 9;
// The key/value pairs that are attached to event.
// The key is like 'test-hoge-fuga'
// This is intended to add more information from event trigger side.
// E.g. send the app code commit hash to Deployment.
map<string,string> contexts = 10;
map<string,string> contexts = 10 [(validate.rules).map.keys.string.pattern = "^[a-zA-Z0-9]+(-[a-zA-Z0-9]+)*$"];

// Unix time when the event was handled.
int64 handled_at = 13;
Expand Down
Loading