From 0c0b890487aff7797a955c2e90248fbf0427ed8d Mon Sep 17 00:00:00 2001 From: Easton Crupper <65553218+ecrupper@users.noreply.github.com> Date: Thu, 12 Oct 2023 10:22:27 -0400 Subject: [PATCH 1/2] fix(rulesets): surface error when regexp compilation fails + some match code refactoring (#327) * fix(rulesets): surface error on regex compilation failures * add pattern to desc * lint party * pesky test files * use constant over hardcoded and * address some comments * more comment fixes * add another test * shorten up some logic mouthfuls --------- Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com> --- pipeline/build.go | 34 ++++-- pipeline/build_test.go | 41 ++++++- pipeline/container.go | 38 +++++-- pipeline/container_test.go | 4 +- pipeline/ruleset.go | 219 +++++++++++++++++-------------------- pipeline/ruleset_test.go | 33 +++++- pipeline/secret.go | 11 +- pipeline/secret_test.go | 2 +- pipeline/stage.go | 11 +- pipeline/stage_test.go | 2 +- 10 files changed, 242 insertions(+), 153 deletions(-) diff --git a/pipeline/build.go b/pipeline/build.go index 1bd5d122..55864b80 100644 --- a/pipeline/build.go +++ b/pipeline/build.go @@ -33,34 +33,54 @@ type Build struct { // function will remove the steps that have a ruleset that do not // match the provided ruledata. If both stages and steps are // provided, then an empty pipeline is returned. -func (b *Build) Purge(r *RuleData) *Build { +func (b *Build) Purge(r *RuleData) (*Build, error) { // return an empty pipeline if both stages and steps are provided if len(b.Stages) > 0 && len(b.Steps) > 0 { - return nil + return nil, fmt.Errorf("cannot have both stages and steps at the top level of pipeline") } // purge stages pipeline if stages are provided if len(b.Stages) > 0 { - b.Stages = *b.Stages.Purge(r) + pStages, err := b.Stages.Purge(r) + if err != nil { + return nil, err + } + + b.Stages = *pStages } // purge steps pipeline if steps are provided if len(b.Steps) > 0 { - b.Steps = *b.Steps.Purge(r) + pSteps, err := b.Steps.Purge(r) + if err != nil { + return nil, err + } + + b.Steps = *pSteps } // purge services in pipeline if services are provided if len(b.Services) > 0 { - b.Services = *b.Services.Purge(r) + pServices, err := b.Services.Purge(r) + if err != nil { + return nil, err + } + + b.Services = *pServices } // purge secrets in pipeline if secrets are provided if len(b.Secrets) > 0 { - b.Secrets = *b.Secrets.Purge(r) + pSecrets, err := b.Secrets.Purge(r) + if err != nil { + return nil, err + } + + b.Secrets = *pSecrets } // return the purged pipeline - return b + return b, nil } // Sanitize cleans the fields for every step in each stage so they diff --git a/pipeline/build_test.go b/pipeline/build_test.go index b43418fa..a693a89b 100644 --- a/pipeline/build_test.go +++ b/pipeline/build_test.go @@ -21,6 +21,7 @@ func TestPipeline_Build_Purge(t *testing.T) { tests := []struct { pipeline *Build want *Build + wantErr bool }{ { pipeline: testBuildStages(), @@ -64,7 +65,39 @@ func TestPipeline_Build_Purge(t *testing.T) { }, }, }, - want: nil, + want: nil, + wantErr: true, + }, + { + pipeline: &Build{ + Steps: ContainerSlice{ + { + ID: "step_github octocat._1_init", + Directory: "/home/github/octocat", + Environment: map[string]string{"FOO": "bar"}, + Image: "#init", + Name: "init", + Number: 1, + Pull: "always", + }, + { + ID: "step_github octocat._1_bad_regexp", + Directory: "/home/github/octocat", + Environment: map[string]string{"FOO": "bar"}, + Image: "alpine", + Name: "bad_regexp", + Number: 2, + Pull: "always", + Ruleset: Ruleset{ + If: Rules{Event: []string{"push"}, Branch: []string{"*-dev"}}, + Operator: "and", + Matcher: "regexp", + }, + }, + }, + }, + want: nil, + wantErr: true, }, } @@ -78,7 +111,11 @@ func TestPipeline_Build_Purge(t *testing.T) { Tag: "refs/heads/main", } - got := test.pipeline.Purge(r) + got, err := test.pipeline.Purge(r) + + if test.wantErr && err == nil { + t.Errorf("Purge should have returned an error, got: %v", got) + } if !reflect.DeepEqual(got, test.want) { t.Errorf("Purge is %v, want %v", got, test.want) diff --git a/pipeline/container.go b/pipeline/container.go index 82867548..3ada27a0 100644 --- a/pipeline/container.go +++ b/pipeline/container.go @@ -56,14 +56,19 @@ type ( // Purge removes the Containers that have a ruleset // that do not match the provided ruledata. -func (c *ContainerSlice) Purge(r *RuleData) *ContainerSlice { +func (c *ContainerSlice) Purge(r *RuleData) (*ContainerSlice, error) { counter := 1 containers := new(ContainerSlice) // iterate through each Container in the pipeline for _, container := range *c { // verify ruleset matches - if container.Ruleset.Match(r) { + match, err := container.Ruleset.Match(r) + if err != nil { + return nil, fmt.Errorf("unable to process ruleset for step %s: %w", container.Name, err) + } + + if match { // overwrite the Container number with the Container counter container.Number = counter @@ -76,7 +81,7 @@ func (c *ContainerSlice) Purge(r *RuleData) *ContainerSlice { } // return the new slice of Containers - return containers + return containers, nil } // Sanitize cleans the fields for every step in the pipeline so they @@ -138,10 +143,10 @@ func (c *Container) Empty() bool { // Execute returns true when the provided ruledata matches // the conditions when we should be running the container on the worker. -func (c *Container) Execute(r *RuleData) bool { +func (c *Container) Execute(r *RuleData) (bool, error) { // return false if the container is nil if c == nil { - return false + return false, nil } // skip evaluating path in ruleset // @@ -189,11 +194,16 @@ func (c *Container) Execute(r *RuleData) bool { // disregard the need to run the container execute = false + match, err := c.Ruleset.Match(r) + if err != nil { + return false, err + } + // check if you need to run a status failure ruleset if ((!(c.Ruleset.If.Empty() && c.Ruleset.Unless.Empty()) && !(c.Ruleset.If.NoStatus() && c.Ruleset.Unless.NoStatus())) || c.Ruleset.If.Parallel) && - c.Ruleset.Match(r) { + match { // approve the need to run the container execute = true } @@ -201,19 +211,29 @@ func (c *Container) Execute(r *RuleData) bool { r.Status = constants.StatusFailure + match, err := c.Ruleset.Match(r) + if err != nil { + return false, err + } + // check if you need to skip a status failure ruleset if strings.EqualFold(status, constants.StatusSuccess) && !(c.Ruleset.If.NoStatus() && c.Ruleset.Unless.NoStatus()) && - !(c.Ruleset.If.Empty() && c.Ruleset.Unless.Empty()) && c.Ruleset.Match(r) { + !(c.Ruleset.If.Empty() && c.Ruleset.Unless.Empty()) && match { r.Status = constants.StatusSuccess - if !c.Ruleset.Match(r) { + match, err = c.Ruleset.Match(r) + if err != nil { + return false, err + } + + if !match { // disregard the need to run the container execute = false } } - return execute + return execute, nil } // MergeEnv takes a list of environment variables and attempts diff --git a/pipeline/container_test.go b/pipeline/container_test.go index 9c94e2fc..2f814ba7 100644 --- a/pipeline/container_test.go +++ b/pipeline/container_test.go @@ -39,7 +39,7 @@ func TestPipeline_ContainerSlice_Purge(t *testing.T) { Tag: "refs/heads/main", } - got := test.containers.Purge(r) + got, _ := test.containers.Purge(r) if !reflect.DeepEqual(got, test.want) { t.Errorf("Purge is %v, want %v", got, test.want) @@ -728,7 +728,7 @@ func TestPipeline_Container_Execute(t *testing.T) { // run tests for _, test := range tests { - got := test.container.Execute(test.ruleData) + got, _ := test.container.Execute(test.ruleData) if got != test.want { t.Errorf("Container Execute %s is %v, want %v", test.container.Name, got, test.want) diff --git a/pipeline/ruleset.go b/pipeline/ruleset.go index d1b6c5b3..04880d57 100644 --- a/pipeline/ruleset.go +++ b/pipeline/ruleset.go @@ -3,6 +3,7 @@ package pipeline import ( + "fmt" "path/filepath" "regexp" "strings" @@ -65,31 +66,33 @@ type ( // When the provided if rules are empty, the function returns // true. When both the provided if and unless rules are empty, // the function also returns true. -func (r *Ruleset) Match(from *RuleData) bool { +func (r *Ruleset) Match(from *RuleData) (bool, error) { // return true when the if and unless rules are empty if r.If.Empty() && r.Unless.Empty() { - return true + return true, nil } // return false when the unless rules are not empty and match if !r.Unless.Empty() { - if r.Unless.Match(from, r.Matcher, r.Operator) { - return false + match, err := r.Unless.Match(from, r.Matcher, r.Operator) + if err != nil { + return false, err + } + + if match { + return false, nil } } // return true when the if rules are empty if r.If.Empty() { - return true + return true, nil } // return true when the if rules match - if r.If.Match(from, r.Matcher, r.Operator) { - return true - } + match, err := r.If.Match(from, r.Matcher, r.Operator) - // return false if not match is found - return false + return match, err } // NoStatus returns true if the status field is empty. @@ -122,115 +125,62 @@ func (r *Rules) Empty() bool { // ruletypes from the rules match the provided ruledata. For // both operators, when none of the ruletypes from the rules // match the provided ruledata, the function returns false. -// -//nolint:gocyclo // accepting complexity in this case -func (r *Rules) Match(from *RuleData, matcher, op string) bool { - // set defaults - status := true - +func (r *Rules) Match(from *RuleData, matcher, op string) (bool, error) { // if the path ruletype is provided if len(from.Path) > 0 { // if the "or" operator is provided in the ruleset if strings.EqualFold(op, constants.OperatorOr) { - // override the default to the "or" - if len(from.Status) != 0 { - status = r.Status.MatchOr(from.Status, matcher) - } - // iterate through each path in the ruletype for _, p := range from.Path { + matches, err := matches(r, from, matcher, p, constants.OperatorOr) + if err != nil { + return false, err + } + // return true if any ruletype matches the ruledata - if r.Branch.MatchOr(from.Branch, matcher) || - r.Comment.MatchOr(from.Comment, matcher) || - r.Event.MatchOr(from.Event, matcher) || - r.Path.MatchOr(p, matcher) || - r.Repo.MatchOr(from.Repo, matcher) || - status || - r.Tag.MatchOr(from.Tag, matcher) || - r.Target.MatchOr(from.Target, matcher) { - return true + if matches { + return true, nil } } // return false if no match is found - return false - } - - // override the default to the "and" - if len(from.Status) != 0 { - status = r.Status.MatchAnd(from.Status, matcher) + return false, nil } // iterate through each path in the ruletype for _, p := range from.Path { - // return true if every ruletype matches the ruledata - if r.Branch.MatchAnd(from.Branch, matcher) && - r.Comment.MatchAnd(from.Comment, matcher) && - r.Event.MatchAnd(from.Event, matcher) && - r.Path.MatchAnd(p, matcher) && - r.Repo.MatchAnd(from.Repo, matcher) && - status && - r.Tag.MatchAnd(from.Tag, matcher) && - r.Target.MatchAnd(from.Target, matcher) { - return true + matches, err := matches(r, from, matcher, p, constants.OperatorAnd) + if err != nil { + return false, err + } + + // return true if any ruletype matches the ruledata + if matches { + return true, nil } } // return false if no match is found - return false + return false, nil } // if the "or" operator is provided in the ruleset if strings.EqualFold(op, constants.OperatorOr) { - // override the default to the "or" - if len(from.Status) != 0 { - status = r.Status.MatchOr(from.Status, matcher) - } - // return true if any ruletype matches the ruledata - if r.Branch.MatchOr(from.Branch, matcher) || - r.Comment.MatchOr(from.Comment, matcher) || - r.Event.MatchOr(from.Event, matcher) || - r.Path.MatchOr("", matcher) || - r.Repo.MatchOr(from.Repo, matcher) || - status || - r.Tag.MatchOr(from.Tag, matcher) || - r.Target.MatchOr(from.Target, matcher) { - return true - } - - // return false if no match is found - return false - } - - // override the default to the "and" - if len(from.Status) != 0 { - status = r.Status.MatchAnd(from.Status, matcher) - } - - // return true if every ruletype matches the ruledata - if r.Branch.MatchAnd(from.Branch, matcher) && - r.Comment.MatchAnd(from.Comment, matcher) && - r.Event.MatchAnd(from.Event, matcher) && - r.Path.MatchAnd("", matcher) && - r.Repo.MatchAnd(from.Repo, matcher) && - status && - r.Tag.MatchAnd(from.Tag, matcher) && - r.Target.MatchAnd(from.Target, matcher) { - return true + return matches(r, from, matcher, "", constants.OperatorOr) } - // return false if no match is found - return false + return matches(r, from, matcher, "", constants.OperatorAnd) } -// MatchAnd returns true when the provided ruletype +// Match returns true when the provided ruletype // matches the provided ruledata. When the provided -// ruletype is empty, the function returns true. -func (r *Ruletype) MatchAnd(data, matcher string) bool { - // return true if an empty ruletype is provided +// ruletype is empty, the function returns true for +// the `and` operator and false for the `or` operator. +func (r *Ruletype) Match(data, matcher, logic string) (bool, error) { + // return true for `and`, false for `or` if an empty ruletype is provided if len(*r) == 0 { - return true + return strings.EqualFold(logic, constants.OperatorAnd), nil } // iterate through each pattern in the ruletype @@ -238,9 +188,14 @@ func (r *Ruletype) MatchAnd(data, matcher string) bool { // handle the pattern based off the matcher provided switch matcher { case constants.MatcherRegex, "regex": + regExpPattern, err := regexp.Compile(pattern) + if err != nil { + return false, fmt.Errorf("error in regex pattern %s: %w", pattern, err) + } + // return true if the regexp pattern matches the ruledata - if regexp.MustCompile(pattern).MatchString(data) { - return true + if regExpPattern.MatchString(data) { + return true, nil } case constants.MatcherFilepath: fallthrough @@ -248,44 +203,68 @@ func (r *Ruletype) MatchAnd(data, matcher string) bool { // return true if the pattern matches the ruledata ok, _ := filepath.Match(pattern, data) if ok { - return true + return true, nil } } } // return false if no match is found - return false + return false, nil } -// MatchOr returns true when the provided ruletype -// matches the provided ruledata. When the provided -// ruletype is empty, the function returns false. -func (r *Ruletype) MatchOr(data, matcher string) bool { - // return false if an empty ruletype is provided - if len(*r) == 0 { - return false - } +// matches is a helper function which leverages the Match method for all rules +// and returns `true` if the ruleset is indeed a match. +func matches(r *Rules, from *RuleData, matcher, path, logic string) (bool, error) { + status := true - // iterate through each pattern in the ruletype - for _, pattern := range *r { - // handle the pattern based off the matcher provided - switch matcher { - case constants.MatcherRegex, "regex": - // return true if the regexp pattern matches the ruledata - if regexp.MustCompile(pattern).MatchString(data) { - return true - } - case constants.MatcherFilepath: - fallthrough - default: - // return true if the pattern matches the ruledata - ok, _ := filepath.Match(pattern, data) - if ok { - return true - } + var err error + + if len(from.Status) != 0 { + status, err = r.Status.Match(from.Status, matcher, logic) + if err != nil { + return false, err } } - // return false if no match is found - return false + matchBranch, err := r.Branch.Match(from.Branch, matcher, logic) + if err != nil { + return false, err + } + + matchComment, err := r.Comment.Match(from.Comment, matcher, logic) + if err != nil { + return false, err + } + + matchEvent, err := r.Event.Match(from.Event, matcher, logic) + if err != nil { + return false, err + } + + matchPath, err := r.Path.Match(path, matcher, logic) + if err != nil { + return false, err + } + + matchRepo, err := r.Repo.Match(from.Repo, matcher, logic) + if err != nil { + return false, err + } + + matchTag, err := r.Tag.Match(from.Tag, matcher, logic) + if err != nil { + return false, err + } + + matchTarget, err := r.Target.Match(from.Target, matcher, logic) + if err != nil { + return false, err + } + + switch logic { + case constants.OperatorAnd: + return (matchBranch && matchComment && matchEvent && matchPath && matchRepo && matchTag && matchTarget && status), nil + default: + return (matchBranch || matchComment || matchEvent || matchPath || matchRepo || matchTag || matchTarget || status), nil + } } diff --git a/pipeline/ruleset_test.go b/pipeline/ruleset_test.go index fb97a696..a310ed71 100644 --- a/pipeline/ruleset_test.go +++ b/pipeline/ruleset_test.go @@ -4,6 +4,8 @@ package pipeline import ( "testing" + + "github.com/go-vela/types/constants" ) func TestPipeline_Ruleset_Match(t *testing.T) { @@ -12,6 +14,7 @@ func TestPipeline_Ruleset_Match(t *testing.T) { ruleset *Ruleset data *RuleData want bool + wantErr bool }{ // Empty {ruleset: &Ruleset{}, data: &RuleData{Branch: "main"}, want: true}, @@ -188,11 +191,31 @@ func TestPipeline_Ruleset_Match(t *testing.T) { data: &RuleData{Branch: "main", Comment: "rerun", Event: "tag", Repo: "octocat/hello-world", Status: "pending", Tag: "refs/heads/main", Target: ""}, want: false, }, + // Bad regexp + { + ruleset: &Ruleset{If: Rules{Branch: []string{"*-dev"}}, Matcher: "regexp"}, + data: &RuleData{Branch: "main", Comment: "rerun", Event: "push", Repo: "octocat/hello-world", Status: "pending", Tag: "refs/heads/main", Target: ""}, + wantErr: true, + }, + { + ruleset: &Ruleset{Unless: Rules{Branch: []string{"*-dev"}, Event: []string{"push"}}, Operator: "or", Matcher: "regexp"}, + data: &RuleData{Branch: "main", Comment: "rerun", Event: "push", Repo: "octocat/hello-world", Status: "pending", Tag: "refs/heads/main", Target: ""}, + wantErr: true, + }, } // run test for _, test := range tests { - got := test.ruleset.Match(test.data) + got, err := test.ruleset.Match(test.data) + if err != nil { + if !test.wantErr { + t.Errorf("Ruleset Match for %s operator returned err: %s", test.ruleset.Operator, err) + } + } else { + if test.wantErr { + t.Errorf("Ruleset Match should have returned an error") + } + } if got != test.want { t.Errorf("Ruleset Match for %s operator is %v, want %v", test.ruleset.Operator, got, test.want) @@ -284,7 +307,7 @@ func TestPipeline_Rules_Match_Regex_Tag(t *testing.T) { // run test for _, test := range tests { - got := test.rules.Match(test.data, "regexp", test.operator) + got, _ := test.rules.Match(test.data, "regexp", test.operator) if got != test.want { t.Errorf("Rules Match for %s operator is %v, want %v", test.operator, got, test.want) @@ -416,7 +439,7 @@ func TestPipeline_Rules_Match(t *testing.T) { // run test for _, test := range tests { - got := test.rules.Match(test.data, "filepath", test.operator) + got, _ := test.rules.Match(test.data, "filepath", test.operator) if got != test.want { t.Errorf("Rules Match for %s operator is %v, want %v", test.operator, got, test.want) @@ -506,7 +529,7 @@ func TestPipeline_Ruletype_MatchAnd(t *testing.T) { // run test for _, test := range tests { - got := test.rule.MatchAnd(test.pattern, test.matcher) + got, _ := test.rule.Match(test.pattern, test.matcher, constants.OperatorAnd) if got != test.want { t.Errorf("MatchAnd for %s matcher is %v, want %v", test.matcher, got, test.want) @@ -580,7 +603,7 @@ func TestPipeline_Ruletype_MatchOr(t *testing.T) { // run test for _, test := range tests { - got := test.rule.MatchOr(test.pattern, test.matcher) + got, _ := test.rule.Match(test.pattern, test.matcher, constants.OperatorOr) if got != test.want { t.Errorf("MatchOr for %s matcher is %v, want %v", test.matcher, got, test.want) diff --git a/pipeline/secret.go b/pipeline/secret.go index 6e51f941..94250afe 100644 --- a/pipeline/secret.go +++ b/pipeline/secret.go @@ -69,7 +69,7 @@ var ( // Purge removes the secrets that have a ruleset // that do not match the provided ruledata. -func (s *SecretSlice) Purge(r *RuleData) *SecretSlice { +func (s *SecretSlice) Purge(r *RuleData) (*SecretSlice, error) { counter := 1 secrets := new(SecretSlice) @@ -82,8 +82,13 @@ func (s *SecretSlice) Purge(r *RuleData) *SecretSlice { continue } + match, err := secret.Origin.Ruleset.Match(r) + if err != nil { + return nil, fmt.Errorf("unable to process ruleset for secret %s: %w", secret.Name, err) + } + // verify ruleset matches - if secret.Origin.Ruleset.Match(r) { + if match { // overwrite the Container number with the Container counter secret.Origin.Number = counter @@ -95,7 +100,7 @@ func (s *SecretSlice) Purge(r *RuleData) *SecretSlice { } } - return secrets + return secrets, nil } // ParseOrg returns the parts (org, key) of the secret path diff --git a/pipeline/secret_test.go b/pipeline/secret_test.go index a4ff7a10..8137e757 100644 --- a/pipeline/secret_test.go +++ b/pipeline/secret_test.go @@ -39,7 +39,7 @@ func TestPipeline_SecretSlice_Purge(t *testing.T) { Tag: "refs/heads/main", } - got := test.secrets.Purge(r) + got, _ := test.secrets.Purge(r) if !reflect.DeepEqual(got, test.want) { t.Errorf("Purge is %v, want %v", got, test.want) diff --git a/pipeline/stage.go b/pipeline/stage.go index 29966677..2325f340 100644 --- a/pipeline/stage.go +++ b/pipeline/stage.go @@ -33,7 +33,7 @@ type ( // a ruleset that do not match the provided ruledata. // If all steps from a stage are removed, then the // entire stage is removed from the pipeline. -func (s *StageSlice) Purge(r *RuleData) *StageSlice { +func (s *StageSlice) Purge(r *RuleData) (*StageSlice, error) { counter := 1 stages := new(StageSlice) @@ -43,8 +43,13 @@ func (s *StageSlice) Purge(r *RuleData) *StageSlice { // iterate through each step for the stage in the pipeline for _, step := range stage.Steps { + match, err := step.Ruleset.Match(r) + if err != nil { + return nil, fmt.Errorf("unable to process ruleset for step %s: %w", step.Name, err) + } + // verify ruleset matches - if step.Ruleset.Match(r) { + if match { // overwrite the step number with the step counter step.Number = counter @@ -69,7 +74,7 @@ func (s *StageSlice) Purge(r *RuleData) *StageSlice { } // return the new slice of stages - return stages + return stages, nil } // Sanitize cleans the fields for every step in each stage so they diff --git a/pipeline/stage_test.go b/pipeline/stage_test.go index 919803dc..3ae29b10 100644 --- a/pipeline/stage_test.go +++ b/pipeline/stage_test.go @@ -39,7 +39,7 @@ func TestPipeline_StageSlice_Purge(t *testing.T) { Tag: "refs/heads/main", } - got := test.stages.Purge(r) + got, _ := test.stages.Purge(r) if !reflect.DeepEqual(got, test.want) { t.Errorf("Purge is %v, want %v", got, test.want) From 19101a5b1346caaeb675fb5f7e5a100277381a88 Mon Sep 17 00:00:00 2001 From: claire1618 <55173466+claire1618@users.noreply.github.com> Date: Tue, 24 Oct 2023 15:11:26 -0500 Subject: [PATCH 2/2] feat(yaml/secret): adding pull tag to secrets to create a lazy secrets ability (#312) * feat: adding pull tag to secrets to create a lazy secrets ability * fixing testing errors * fixing testing errors * changing pull tag options * fixing tests * fixing linter errors * fixing some errors * fixing formatting * fixing errors --------- Co-authored-by: Claire.Nicholas Co-authored-by: Tim Huynh Co-authored-by: David May <49894298+wass3rw3rk@users.noreply.github.com> Co-authored-by: Easton Crupper <65553218+ecrupper@users.noreply.github.com> --- constants/secret.go | 6 ++++++ pipeline/secret.go | 1 + pipeline/secret_test.go | 30 ++++++++++++++++++++++++++++++ yaml/build_test.go | 6 ++++++ yaml/secret.go | 7 +++++++ yaml/secret_test.go | 11 +++++++++++ yaml/testdata/secret.yml | 1 + 7 files changed, 62 insertions(+) diff --git a/constants/secret.go b/constants/secret.go index 13628e95..bb794f02 100644 --- a/constants/secret.go +++ b/constants/secret.go @@ -4,6 +4,12 @@ package constants // Secret types. const ( + // SecretPullBuild defines the pull policy type for a secret. + SecretPullBuild = "build_start" + + // SecretPullStep defines the pull policy type for a secret. + SecretPullStep = "step_start" + // SecretOrg defines the secret type for a secret scoped to a specific org. SecretOrg = "org" diff --git a/pipeline/secret.go b/pipeline/secret.go index 94250afe..bbc5e260 100644 --- a/pipeline/secret.go +++ b/pipeline/secret.go @@ -28,6 +28,7 @@ type ( Engine string `json:"engine,omitempty" yaml:"engine,omitempty"` Type string `json:"type,omitempty" yaml:"type,omitempty"` Origin *Container `json:"origin,omitempty" yaml:"origin,omitempty"` + Pull string `json:"pull,omitempty" yaml:"pull,omitempty"` } // StepSecretSlice is the pipeline representation diff --git a/pipeline/secret_test.go b/pipeline/secret_test.go index 8137e757..bf5bfbc4 100644 --- a/pipeline/secret_test.go +++ b/pipeline/secret_test.go @@ -60,6 +60,7 @@ func TestPipeline_Secret_ParseOrg_success(t *testing.T) { Key: "octocat/foo", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", }, @@ -70,6 +71,7 @@ func TestPipeline_Secret_ParseOrg_success(t *testing.T) { Key: "octocat/๐Ÿ‘‹/๐Ÿงช/๐Ÿ”‘", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", }, @@ -108,6 +110,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat/foo", Engine: "native", Type: "org", + Pull: "build_start", }, org: "wrongorg", wantErr: ErrInvalidOrg, @@ -119,6 +122,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -130,6 +134,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat/", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -140,6 +145,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat/foo/bar", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidName, @@ -151,6 +157,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat/foo/bar", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidName, @@ -162,6 +169,7 @@ func TestPipeline_Secret_ParseOrg_failure(t *testing.T) { Key: "octocat/foo", Engine: "invalid", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidEngine, @@ -195,6 +203,7 @@ func TestPipeline_Secret_ParseRepo_success(t *testing.T) { Key: "octocat/helloworld/foo", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "helloworld", @@ -206,6 +215,7 @@ func TestPipeline_Secret_ParseRepo_success(t *testing.T) { Key: "octocat/๐Ÿ‘‹/๐Ÿงช/๐Ÿ”‘", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "๐Ÿ‘‹", @@ -253,6 +263,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld/foo", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "wrongorg", repo: "helloworld", @@ -265,6 +276,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld/foo", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "badrepo", @@ -277,6 +289,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -288,6 +301,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld", Engine: "native", Type: "org", + Pull: "build_start", }, repo: "helloworld", org: "octocat", @@ -300,6 +314,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld/", Engine: "native", Type: "org", + Pull: "build_start", }, repo: "helloworld", org: "octocat", @@ -311,6 +326,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld/foo/bar", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "helloworld", @@ -323,6 +339,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat/helloworld/foo/bar", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "helloworld", @@ -335,6 +352,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "octocat", Engine: "invalid", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidEngine, @@ -346,6 +364,7 @@ func TestPipeline_Secret_ParseRepo_failure(t *testing.T) { Key: "foo", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", repo: "helloworld", @@ -379,6 +398,7 @@ func TestPipeline_Secret_ParseShared_success(t *testing.T) { Key: "octocat/helloworld/foo", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", }, @@ -389,6 +409,7 @@ func TestPipeline_Secret_ParseShared_success(t *testing.T) { Key: "octocat/๐Ÿ‘‹/๐Ÿงช/๐Ÿ”‘", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", }, @@ -431,6 +452,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat", Engine: "native", Type: "repo", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -442,6 +464,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat", Engine: "invalid", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidEngine, @@ -453,6 +476,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat/foo", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -464,6 +488,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat/foo/", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidPath, @@ -474,6 +499,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat/foo/bar", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidName, @@ -485,6 +511,7 @@ func TestPipeline_Secret_ParseShared_failure(t *testing.T) { Key: "octocat/foo/bar", Engine: "native", Type: "org", + Pull: "build_start", }, org: "octocat", wantErr: ErrInvalidName, @@ -512,6 +539,7 @@ func testSecrets() *SecretSlice { Name: "foobar", Type: "repo", Origin: &Container{}, + Pull: "build_start", }, { Engine: "native", @@ -519,6 +547,7 @@ func testSecrets() *SecretSlice { Name: "foobar", Type: "org", Origin: &Container{}, + Pull: "build_start", }, { Engine: "native", @@ -526,6 +555,7 @@ func testSecrets() *SecretSlice { Name: "foobar", Type: "shared", Origin: &Container{}, + Pull: "build_start", }, { Name: "", diff --git a/yaml/build_test.go b/yaml/build_test.go index 4b652e98..f3576327 100644 --- a/yaml/build_test.go +++ b/yaml/build_test.go @@ -260,36 +260,42 @@ func TestYaml_Build_UnmarshalYAML(t *testing.T) { Key: "org/repo/docker/username", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "docker_password", Key: "org/repo/docker/password", Engine: "vault", Type: "repo", + Pull: "build_start", }, { Name: "docker_username", Key: "org/docker/username", Engine: "native", Type: "org", + Pull: "build_start", }, { Name: "docker_password", Key: "org/docker/password", Engine: "vault", Type: "org", + Pull: "build_start", }, { Name: "docker_username", Key: "org/team/docker/username", Engine: "native", Type: "shared", + Pull: "build_start", }, { Name: "docker_password", Key: "org/team/docker/password", Engine: "vault", Type: "shared", + Pull: "build_start", }, { Origin: Origin{ diff --git a/yaml/secret.go b/yaml/secret.go index a211a245..2c44e5ab 100644 --- a/yaml/secret.go +++ b/yaml/secret.go @@ -25,6 +25,7 @@ type ( Engine string `yaml:"engine,omitempty" json:"engine,omitempty" jsonschema:"enum=native,enum=vault,default=native,description=Name of storage backend to fetch secret from.\nReference: https://go-vela.github.io/docs/reference/yaml/secrets/#the-engine-tag"` Type string `yaml:"type,omitempty" json:"type,omitempty" jsonschema:"enum=repo,enum=org,enum=shared,default=repo,description=Type of secret to fetch from storage backend.\nReference: https://go-vela.github.io/docs/reference/yaml/secrets/#the-type-tag"` Origin Origin `yaml:"origin,omitempty" json:"origin,omitempty" jsonschema:"description=Declaration to pull secrets from non-internal secret providers.\nReference: https://go-vela.github.io/docs/reference/yaml/secrets/#the-origin-tag"` + Pull string `yaml:"pull,omitempty" json:"pull,omitempty" jsonschema:"default=build_start,description=When to pull in secrets from storage backend."` } // Origin is the yaml representation of a method @@ -55,6 +56,7 @@ func (s *SecretSlice) ToPipeline() *pipeline.SecretSlice { Engine: secret.Engine, Type: secret.Type, Origin: secret.Origin.ToPipeline(), + Pull: secret.Pull, }) } @@ -94,6 +96,11 @@ func (s *SecretSlice) UnmarshalYAML(unmarshal func(interface{}) error) error { secret.Type = constants.SecretRepo } + // implicitly set `type` field if empty + if secret.Origin.Empty() && len(secret.Pull) == 0 { + secret.Pull = constants.SecretPullBuild + } + // implicitly set `pull` field if empty if !secret.Origin.Empty() && len(secret.Origin.Pull) == 0 { secret.Origin.Pull = constants.PullNotPresent diff --git a/yaml/secret_test.go b/yaml/secret_test.go index 9cb4dae6..f70637e6 100644 --- a/yaml/secret_test.go +++ b/yaml/secret_test.go @@ -108,6 +108,7 @@ func TestYaml_SecretSlice_ToPipeline(t *testing.T) { Engine: "native", Type: "repo", Origin: Origin{}, + Pull: "build_start", }, { Name: "docker_username", @@ -139,6 +140,7 @@ func TestYaml_SecretSlice_ToPipeline(t *testing.T) { }, }, }, + Pull: "build_start", }, }, want: &pipeline.SecretSlice{ @@ -148,6 +150,7 @@ func TestYaml_SecretSlice_ToPipeline(t *testing.T) { Engine: "native", Type: "repo", Origin: &pipeline.Container{}, + Pull: "build_start", }, { Name: "docker_username", @@ -176,6 +179,7 @@ func TestYaml_SecretSlice_ToPipeline(t *testing.T) { }, }, }, + Pull: "build_start", }, }, }, @@ -207,30 +211,35 @@ func TestYaml_SecretSlice_UnmarshalYAML(t *testing.T) { Key: "bar", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "noKey", Key: "noKey", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "noType", Key: "bar", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "noEngine", Key: "bar", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "noKeyEngineAndType", Key: "noKeyEngineAndType", Engine: "native", Type: "repo", + Pull: "build_start", }, { Name: "externalSecret", @@ -262,6 +271,7 @@ func TestYaml_SecretSlice_UnmarshalYAML(t *testing.T) { }, }, }, + Pull: "", }, { Name: "", @@ -293,6 +303,7 @@ func TestYaml_SecretSlice_UnmarshalYAML(t *testing.T) { }, }, }, + Pull: "", }, }, }, diff --git a/yaml/testdata/secret.yml b/yaml/testdata/secret.yml index 72a56cb0..c432eb29 100644 --- a/yaml/testdata/secret.yml +++ b/yaml/testdata/secret.yml @@ -5,6 +5,7 @@ key: bar engine: native type: repo + pull: build_start - name: noKey engine: native type: repo