From 466b4d51ce29abade08ca73530a1d464112171b4 Mon Sep 17 00:00:00 2001 From: Ajita Jain Date: Thu, 8 Aug 2024 13:43:05 -0700 Subject: [PATCH 1/7] Add flag for kiln validate to specify allow list for release source types Co-authored-by: Christopher Hunter --- internal/commands/validate.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/commands/validate.go b/internal/commands/validate.go index f364fe226..b9b2fdab5 100644 --- a/internal/commands/validate.go +++ b/internal/commands/validate.go @@ -2,6 +2,7 @@ package commands import ( "fmt" + "slices" "strings" "github.com/go-git/go-billy/v5" @@ -14,6 +15,7 @@ import ( type Validate struct { Options struct { flags.Standard + ReleaseSourceTypeAllowList []string `long:"allow-release-source-type"` } FS billy.Filesystem @@ -38,6 +40,14 @@ func (v Validate) Execute(args []string) error { return fmt.Errorf("failed to load kilnfiles: %w", err) } + if len(v.Options.ReleaseSourceTypeAllowList) > 0 { + for _, s := range kf.ReleaseSources { + if !slices.Contains(v.Options.ReleaseSourceTypeAllowList, s.Type) { + return fmt.Errorf("release source type not allowed: %s", s.Type) + } + } + } + errs := cargo.Validate(kf, lock) if len(errs) > 0 { return errorList(errs) From fdea5042bbd9c97d4a8b309700f69a27b81214d2 Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Fri, 9 Aug 2024 13:50:47 -0700 Subject: [PATCH 2/7] feat(cargo): allow configuration of Validate to check allow list for release_source types Co-authored-by: Ajita Jain --- internal/commands/validate.go | 11 +---- internal/commands/validate_test.go | 66 ++++++++++++++++++++++++++++++ pkg/cargo/validate.go | 34 ++++++++++++++- pkg/cargo/validate_test.go | 31 ++++++++++++++ 4 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 internal/commands/validate_test.go diff --git a/internal/commands/validate.go b/internal/commands/validate.go index b9b2fdab5..448ac3141 100644 --- a/internal/commands/validate.go +++ b/internal/commands/validate.go @@ -2,7 +2,6 @@ package commands import ( "fmt" - "slices" "strings" "github.com/go-git/go-billy/v5" @@ -40,15 +39,7 @@ func (v Validate) Execute(args []string) error { return fmt.Errorf("failed to load kilnfiles: %w", err) } - if len(v.Options.ReleaseSourceTypeAllowList) > 0 { - for _, s := range kf.ReleaseSources { - if !slices.Contains(v.Options.ReleaseSourceTypeAllowList, s.Type) { - return fmt.Errorf("release source type not allowed: %s", s.Type) - } - } - } - - errs := cargo.Validate(kf, lock) + errs := cargo.Validate(kf, lock, cargo.ValidateResourceTypeAllowList(v.Options.ReleaseSourceTypeAllowList...)) if len(errs) > 0 { return errorList(errs) } diff --git a/internal/commands/validate_test.go b/internal/commands/validate_test.go new file mode 100644 index 000000000..d3b6dbd63 --- /dev/null +++ b/internal/commands/validate_test.go @@ -0,0 +1,66 @@ +package commands_test + +import ( + "io" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "github.com/go-git/go-billy/v5" + "github.com/go-git/go-billy/v5/memfs" + + "github.com/pivotal-cf/kiln/internal/commands" +) + +var _ = Describe("validate", func() { + var ( + validate commands.Validate + directory billy.Filesystem + ) + + BeforeEach(func() { + directory = memfs.New() + }) + + JustBeforeEach(func() { + validate = commands.NewValidate(directory) + }) + + When("the kilnfile has two release_sources", func() { + BeforeEach(func() { + f, err := directory.Create("Kilnfile") + Expect(err).NotTo(HaveOccurred()) + _, _ = io.WriteString(f, `--- +release_sources: + - type: "bosh.io" + - type: "github" +`) + _ = f.Close() + }) + + BeforeEach(func() { + f, err := directory.Create("Kilnfile.lock") + Expect(err).NotTo(HaveOccurred()) + _ = f.Close() + }) + + When("both types are in the allow list", func() { + It("it does fail", func() { + err := validate.Execute([]string{ + "--allow-release-source-type=bosh.io", + "--allow-release-source-type=github", + }) + Expect(err).NotTo(HaveOccurred()) + }) + }) + When("both one of the types is not in the allow list", func() { + It("it does fail", func() { + err := validate.Execute([]string{ + "--allow-release-source-type=bosh.io", + }) + Expect(err).To(MatchError(ContainSubstring("release source type not allowed: github"))) + }) + }) + }) + +}) diff --git a/pkg/cargo/validate.go b/pkg/cargo/validate.go index c3f036e03..94b2e7518 100644 --- a/pkg/cargo/validate.go +++ b/pkg/cargo/validate.go @@ -7,9 +7,41 @@ import ( "github.com/Masterminds/semver/v3" ) -func Validate(spec Kilnfile, lock KilnfileLock) []error { +type ValidationOptions struct { + resourceTypeAllowList []string +} + +func ValidateResourceTypeAllowList(allowList ...string) ValidationOptions { + return ValidationOptions{}.SetValidateResourceTypeAllowList(allowList) +} + +func (o ValidationOptions) SetValidateResourceTypeAllowList(allowList []string) ValidationOptions { + o.resourceTypeAllowList = allowList + return o +} + +func mergeOptions(options []ValidationOptions) ValidationOptions { + var opt ValidationOptions + for _, o := range options { + if o.resourceTypeAllowList != nil { + opt.resourceTypeAllowList = o.resourceTypeAllowList + } + } + return opt +} + +func Validate(spec Kilnfile, lock KilnfileLock, options ...ValidationOptions) []error { + opt := mergeOptions(options) var result []error + if len(opt.resourceTypeAllowList) > 0 { + for _, s := range spec.ReleaseSources { + if !slices.Contains(opt.resourceTypeAllowList, s.Type) { + result = append(result, fmt.Errorf("release source type not allowed: %s", s.Type)) + } + } + } + for index, componentSpec := range spec.Releases { if componentSpec.Name == "" { result = append(result, fmt.Errorf("release at index %d missing name in spec", index)) diff --git a/pkg/cargo/validate_test.go b/pkg/cargo/validate_test.go index bb3521367..aa23fe2a0 100644 --- a/pkg/cargo/validate_test.go +++ b/pkg/cargo/validate_test.go @@ -4,6 +4,8 @@ import ( "testing" . "github.com/onsi/gomega" + + "github.com/stretchr/testify/assert" ) const ( @@ -296,3 +298,32 @@ func TestValidate_checkComponentVersionsAndConstraint(t *testing.T) { )) }) } + +func TestValidateWithOptions(t *testing.T) { + t.Run("resource type allow list", func(t *testing.T) { + t.Run("when the types are permitted", func(t *testing.T) { + kf := Kilnfile{ + ReleaseSources: []ReleaseSourceConfig{ + {Type: "farm"}, + {Type: "orchard"}, + }, + } + kl := KilnfileLock{} + errs := Validate(kf, kl, ValidateResourceTypeAllowList("orchard", "farm")) + assert.Zero(t, errs) + }) + t.Run("when one of the types is not in the allow list", func(t *testing.T) { + kf := Kilnfile{ + ReleaseSources: []ReleaseSourceConfig{ + {Type: "farm"}, + {Type: "orchard"}, + }, + } + kl := KilnfileLock{} + errs := Validate(kf, kl, ValidateResourceTypeAllowList("orchard")) + if assert.Len(t, errs, 1) { + assert.ErrorContains(t, errs[0], "release source type not allowed: farm") + } + }) + }) +} From a9ba3220066e93afeb8a07b14fc6e62cbf56b5bf Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Fri, 9 Aug 2024 14:11:51 -0700 Subject: [PATCH 3/7] fix: add langauge annotation to test value this is just a nice to have --- internal/commands/validate_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/commands/validate_test.go b/internal/commands/validate_test.go index d3b6dbd63..b82ab3b72 100644 --- a/internal/commands/validate_test.go +++ b/internal/commands/validate_test.go @@ -30,6 +30,7 @@ var _ = Describe("validate", func() { BeforeEach(func() { f, err := directory.Create("Kilnfile") Expect(err).NotTo(HaveOccurred()) + // language=yaml _, _ = io.WriteString(f, `--- release_sources: - type: "bosh.io" From 35d3b6fd201a71fb76a6c4b9148b91ee0787f826 Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Fri, 9 Aug 2024 14:16:22 -0700 Subject: [PATCH 4/7] test(commands): add test for empty release_source type allow list --- internal/commands/validate_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/commands/validate_test.go b/internal/commands/validate_test.go index b82ab3b72..bf477b37d 100644 --- a/internal/commands/validate_test.go +++ b/internal/commands/validate_test.go @@ -12,7 +12,7 @@ import ( "github.com/pivotal-cf/kiln/internal/commands" ) -var _ = Describe("validate", func() { +var _ = FDescribe("validate", func() { var ( validate commands.Validate directory billy.Filesystem @@ -62,6 +62,11 @@ release_sources: Expect(err).To(MatchError(ContainSubstring("release source type not allowed: github"))) }) }) + When("the allow list is empty", func() { + It("it does not fail", func() { + err := validate.Execute([]string{}) + Expect(err).NotTo(HaveOccurred()) + }) + }) }) - }) From 73b3d5b8e836c3e0b329ec0dad2a0ff76a42ec1c Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Fri, 9 Aug 2024 14:27:34 -0700 Subject: [PATCH 5/7] fix: remove focus --- internal/commands/validate_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/validate_test.go b/internal/commands/validate_test.go index bf477b37d..45561d7be 100644 --- a/internal/commands/validate_test.go +++ b/internal/commands/validate_test.go @@ -12,7 +12,7 @@ import ( "github.com/pivotal-cf/kiln/internal/commands" ) -var _ = FDescribe("validate", func() { +var _ = Describe("validate", func() { var ( validate commands.Validate directory billy.Filesystem From 084efc130bd5af650152c037637219d97bc8e179 Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Fri, 9 Aug 2024 17:09:08 -0700 Subject: [PATCH 6/7] feat: add release_source field validation Co-authored-by: Ajita Jain --- pkg/cargo/validate.go | 63 +++++++- pkg/cargo/validate_test.go | 286 +++++++++++++++++++++++++++++++++++++ 2 files changed, 347 insertions(+), 2 deletions(-) diff --git a/pkg/cargo/validate.go b/pkg/cargo/validate.go index 94b2e7518..3878f72d9 100644 --- a/pkg/cargo/validate.go +++ b/pkg/cargo/validate.go @@ -2,9 +2,9 @@ package cargo import ( "fmt" - "slices" - "github.com/Masterminds/semver/v3" + "slices" + "text/template/parse" ) type ValidationOptions struct { @@ -75,6 +75,7 @@ func Validate(spec Kilnfile, lock KilnfileLock, options ...ValidationOptions) [] } result = append(result, ensureRemoteSourceExistsForEachReleaseLock(spec, lock)...) + result = append(result, ensureReleaseSourceConfiguration(spec.ReleaseSources)...) if len(result) > 0 { return result @@ -83,6 +84,64 @@ func Validate(spec Kilnfile, lock KilnfileLock, options ...ValidationOptions) [] return nil } +func ensureReleaseSourceConfiguration(sources []ReleaseSourceConfig) []error { + var errs []error + for _, source := range sources { + switch source.Type { + case BOSHReleaseTarballSourceTypeArtifactory: + if source.ArtifactoryHost == "" { + errs = append(errs, fmt.Errorf("missing required field artifactory_host")) + } + if source.Username == "" { + errs = append(errs, fmt.Errorf("missing required field username")) + } + if source.Password == "" { + errs = append(errs, fmt.Errorf("missing required field password")) + } + if source.Repo == "" { + errs = append(errs, fmt.Errorf("missing required field repo")) + } + if source.PathTemplate == "" { + errs = append(errs, fmt.Errorf("missing required field path_template")) + } else { + p := parse.New("path_template") + p.Mode |= parse.SkipFuncCheck + if _, err := p.Parse(source.PathTemplate, "", "", make(map[string]*parse.Tree)); err != nil { + errs = append(errs, fmt.Errorf("failed to parse path_template: %w", err)) + } + } + if source.Bucket != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field bucket")) + } + if source.Region != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field region")) + } + if source.AccessKeyId != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field access_key_id")) + } + if source.SecretAccessKey != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field secret_access_key")) + } + if source.RoleARN != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field role_arn")) + } + if source.Endpoint != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field endpoint")) + } + if source.Org != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field org")) + } + if source.GithubToken != "" { + errs = append(errs, fmt.Errorf("artifactory has unexpected field github_token")) + } + case BOSHReleaseTarballSourceTypeBOSHIO: + case BOSHReleaseTarballSourceTypeS3: + case BOSHReleaseTarballSourceTypeGithub: + } + } + return errs +} + func ensureRemoteSourceExistsForEachReleaseLock(spec Kilnfile, lock KilnfileLock) []error { var result []error for _, release := range lock.Releases { diff --git a/pkg/cargo/validate_test.go b/pkg/cargo/validate_test.go index aa23fe2a0..d9b419278 100644 --- a/pkg/cargo/validate_test.go +++ b/pkg/cargo/validate_test.go @@ -1,6 +1,7 @@ package cargo import ( + "github.com/stretchr/testify/require" "testing" . "github.com/onsi/gomega" @@ -326,4 +327,289 @@ func TestValidateWithOptions(t *testing.T) { } }) }) + + t.Run("when a release_source is not configured properly", func(t *testing.T) { + for _, tt := range []struct { + Name string + Sources []ReleaseSourceConfig + Error func(t *testing.T, errs []error) + }{ + { + Name: "artifactory host is empty", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + // ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "some-path", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "missing required field artifactory_host") + }, + }, + { + Name: "artifactory password is empty", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + // Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "some-path", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "missing required field password") + }, + }, + { + Name: "artifactory username is empty", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + // Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "some-path", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "missing required field username") + }, + }, + { + Name: "artifactory repo is empty", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + // Repo: "secret-stash", + PathTemplate: "some-path", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "missing required field repo") + }, + }, + { + Name: "artifactory path_template is empty", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + // PathTemplate: "some-path", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "missing required field path_template") + }, + }, + { + Name: "artifactory path_template is malformed", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "{{ loosing power", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "failed to parse path_template:") + }, + }, + { + Name: "artifactory has unexpected field bucket", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + Bucket: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field bucket") + }, + }, + { + Name: "artifactory has unexpected field region", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + Region: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field region") + }, + }, + { + Name: "artifactory has unexpected field access_key_id", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + AccessKeyId: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field access_key_id") + }, + }, + { + Name: "artifactory has unexpected field secret_access_key", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + SecretAccessKey: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field secret_access_key") + }, + }, + { + Name: "artifactory has unexpected field role_arn", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + RoleARN: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field role_arn") + }, + }, + { + Name: "artifactory has unexpected field endpoint", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + Endpoint: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field endpoint") + }, + }, + { + Name: "artifactory has unexpected field org", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + Org: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field org") + }, + }, + { + Name: "artifactory has unexpected field github_token", + Sources: []ReleaseSourceConfig{ + { + Type: BOSHReleaseTarballSourceTypeArtifactory, + ID: "", + ArtifactoryHost: "http://example.com", + Username: "bot", + Password: "beep boop", + Repo: "secret-stash", + PathTemplate: "ok", + + GithubToken: "UNEXPECTED", + }, + }, + Error: func(t *testing.T, errs []error) { + require.Len(t, errs, 1) + assert.ErrorContains(t, errs[0], "unexpected field github_token") + }, + }, + } { + t.Run(tt.Name, func(t *testing.T) { + k := Kilnfile{ + ReleaseSources: tt.Sources, + } + errs := Validate(k, KilnfileLock{}) + tt.Error(t, errs) + }) + } + }) } From bc63749d462d5f5f8d6c583a82c785fb83281fd0 Mon Sep 17 00:00:00 2001 From: Christopher Hunter Date: Mon, 12 Aug 2024 11:17:55 -0700 Subject: [PATCH 7/7] fix(cargo): add Options constructor --- pkg/cargo/validate.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/cargo/validate.go b/pkg/cargo/validate.go index 3878f72d9..11f79f519 100644 --- a/pkg/cargo/validate.go +++ b/pkg/cargo/validate.go @@ -11,8 +11,13 @@ type ValidationOptions struct { resourceTypeAllowList []string } +func NewValidateOptions() ValidationOptions { + return ValidationOptions{} +} + +// ValidateResourceTypeAllowList calls ValidationOptions.SetValidateResourceTypeAllowList on the result of NewValidateOptions func ValidateResourceTypeAllowList(allowList ...string) ValidationOptions { - return ValidationOptions{}.SetValidateResourceTypeAllowList(allowList) + return NewValidateOptions().SetValidateResourceTypeAllowList(allowList) } func (o ValidationOptions) SetValidateResourceTypeAllowList(allowList []string) ValidationOptions {