-
Notifications
You must be signed in to change notification settings - Fork 91
Validation rules: build/source mode enforcement #1178
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
Changes from 41 commits
c2b6ed7
1166296
892c661
a5c8cfc
30cfd76
d482f31
2769edc
324c4a0
d999602
fe83a0a
491658c
15750f5
fcaf8c2
f4e266e
e9d0013
cbd3b7c
0f275c0
d1efd03
4233331
da2a325
8539209
b0049ff
d4508b1
8d441f1
15cc91e
cc5e273
2a3fef8
d8a2a6c
ebac0c7
d895e49
6fd467f
2abdb49
ab9ad08
73326a2
9345254
cc99311
0c34f3f
7b72caf
561b526
8ccac7b
148aebf
8c60420
a52f51a
5b78784
44f5383
b66a1f3
4206c50
dedc2ff
c29ab54
2fdf404
1219cd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,11 +20,11 @@ import ( | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| const ( | ||||||||||||||||||||||
| defaultStreamTemplatePath = "stream.yml.hbs" | ||||||||||||||||||||||
| packageTypeIntegration = "integration" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type policyTemplateInput struct { | ||||||||||||||||||||||
| Type string `yaml:"type"` | ||||||||||||||||||||||
| Package string `yaml:"package"` | ||||||||||||||||||||||
| TemplatePath string `yaml:"template_path"` | ||||||||||||||||||||||
| TemplatePaths []string `yaml:"template_paths"` | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -41,6 +41,7 @@ type integrationPackageManifest struct { // package manifest | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| type stream struct { | ||||||||||||||||||||||
| Input string `yaml:"input"` | ||||||||||||||||||||||
| Package string `yaml:"package"` | ||||||||||||||||||||||
| TemplatePath string `yaml:"template_path"` | ||||||||||||||||||||||
| TemplatePaths []string `yaml:"template_paths"` | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
@@ -78,7 +79,7 @@ func ValidateIntegrationPolicyTemplates(fsys fspath.FS) specerrors.ValidationErr | |||||||||||||||||||||
| specerrors.NewStructuredErrorf("file \"%s\" is invalid: %w", fsys.Path(manifestPath), errFailedToParseManifest)} | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if manifest.Type != packageTypeIntegration { | ||||||||||||||||||||||
| if manifest.Type != integrationPackageType { | ||||||||||||||||||||||
| return nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -110,6 +111,14 @@ func ValidateIntegrationPolicyTemplates(fsys fspath.FS) specerrors.ValidationErr | |||||||||||||||||||||
| // under agent/input when template_paths or template_path is set (Fleet: template_paths first). | ||||||||||||||||||||||
| func validateIntegrationPolicyTemplateInputs(fsys fspath.FS, policyTemplate integrationPolicyTemplate) error { | ||||||||||||||||||||||
| for _, input := range policyTemplate.Inputs { | ||||||||||||||||||||||
| // Composable inputs reference an input package via 'package:'. When no | ||||||||||||||||||||||
| // explicit template_path/template_paths is set, all templates come from | ||||||||||||||||||||||
| // the dependency and are only present after build. Skip those. | ||||||||||||||||||||||
| // If the composable input defines its own template_path or template_paths | ||||||||||||||||||||||
| // (overlay templates that live in the source package), those are validated. | ||||||||||||||||||||||
| if input.Package != "" && input.TemplatePath == "" && len(input.TemplatePaths) == 0 { | ||||||||||||||||||||||
| continue | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this exception needed? I see two options for that:
Also, unit tests pass without this code.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i removed the check. i prefer the validation runs on source and build. source is run on elastic-package lint - build - install; if there is any missing file we will notice before getting to the bundle. |
||||||||||||||||||||||
| if len(input.TemplatePaths) > 0 { | ||||||||||||||||||||||
| for _, tp := range input.TemplatePaths { | ||||||||||||||||||||||
| if err := validateAgentInputTemplatePath(fsys, tp); err != nil { | ||||||||||||||||||||||
|
|
@@ -141,6 +150,17 @@ func validateAllDataStreamStreamTemplates(fsys fspath.FS, dsMap map[string]dataS | |||||||||||||||||||||
| dsManifestPath := path.Join(dsDir, "manifest.yml") | ||||||||||||||||||||||
| manifest := dsMap[dsDir] | ||||||||||||||||||||||
| for _, s := range manifest.Streams { | ||||||||||||||||||||||
| // Composable streams reference an input package via 'package:'. When | ||||||||||||||||||||||
| // no explicit template_path/template_paths is set on the stream, all | ||||||||||||||||||||||
| // templates come from the dependency and are only present after build. | ||||||||||||||||||||||
| // Skip those — ValidateStreamInputMaterialized enforces that 'package:' | ||||||||||||||||||||||
| // is replaced by 'input:' in build mode. | ||||||||||||||||||||||
| // However, if the composable stream defines its own template_path or | ||||||||||||||||||||||
| // template_paths, those files must exist in the source package and are | ||||||||||||||||||||||
| // validated here. | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit. A bit too verbose.
Suggested change
|
||||||||||||||||||||||
| if s.Package != "" && s.TemplatePath == "" && len(s.TemplatePaths) == 0 { | ||||||||||||||||||||||
| continue | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case I guess we need to maintain this condition if we want to validate templates at source packages. This case is different because a default template is expected if none is defined, and for composable packages it would be fine to don't have one in the source.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, i am keeping this as the source validation is mantained |
||||||||||||||||||||||
| if err := validateSingleDataStreamStreamTemplates(fsys, dsDir, s); err != nil { | ||||||||||||||||||||||
| errs = append(errs, specerrors.NewStructuredErrorf( | ||||||||||||||||||||||
| "file \"%s\" is invalid: data stream \"%s\" stream input %q: %w", | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| // or more contributor license agreements. Licensed under the Elastic License; | ||
| // you may not use this file except in compliance with the Elastic License. | ||
|
|
||
| package semantic | ||
|
|
||
| import ( | ||
| "io/fs" | ||
|
|
||
| "github.com/elastic/package-spec/v3/code/go/internal/fspath" | ||
| "github.com/elastic/package-spec/v3/code/go/pkg/specerrors" | ||
| ) | ||
|
|
||
| // ValidateNoDevFolder errors for any _dev/ directory found in the package. | ||
| // _dev/ is a source-only artifact used during development (tests, deploy | ||
| // configs, build manifests). It must not appear in a built package that is | ||
| // validated with ModeBuild. | ||
| func ValidateNoDevFolder(fsys fspath.FS) specerrors.ValidationErrors { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There would be any way to define this in the spec? Maybe folder spec should be also aware of the validation mode, and it should not accept files marked with some "source" marker. And we could reuse it for link files, or other files that should be there only in source packages.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. implemented this from the spec, both dev and .link files are checked based on mode on the spec. i was suggested to use the |
||
| var errs specerrors.ValidationErrors | ||
| walkErr := fs.WalkDir(fsys, ".", func(p string, d fs.DirEntry, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if d.IsDir() && d.Name() == "_dev" { | ||
| errs = append(errs, specerrors.NewStructuredErrorf( | ||
| "file %q: _dev directory is not allowed in built packages", | ||
| fsys.Path(p), | ||
| )) | ||
| // Skip the subtree to avoid generating child errors for each | ||
| // file inside the _dev directory. | ||
| return fs.SkipDir | ||
| } | ||
| return nil | ||
| }) | ||
| if walkErr != nil { | ||
| errs = append(errs, specerrors.NewStructuredError(walkErr, specerrors.UnassignedCode)) | ||
| } | ||
| return errs | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| // Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| // or more contributor license agreements. Licensed under the Elastic License; | ||
| // you may not use this file except in compliance with the Elastic License. | ||
|
|
||
| package semantic | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/elastic/package-spec/v3/code/go/internal/fspath" | ||
| ) | ||
|
|
||
| func TestValidateNoDevFolder(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| dirs []string // directories to create relative to the temp root | ||
| expectErrors bool | ||
| errorContains []string | ||
| }{ | ||
| { | ||
| name: "no _dev directory", | ||
| dirs: []string{"data_stream/foo/fields"}, | ||
| expectErrors: false, | ||
| }, | ||
| { | ||
| name: "_dev at package root", | ||
| dirs: []string{"_dev/build"}, | ||
| expectErrors: true, | ||
| errorContains: []string{"_dev", "_dev directory is not allowed in built packages"}, | ||
| }, | ||
| { | ||
| name: "_dev inside data_stream", | ||
| dirs: []string{"data_stream/foo/_dev/test"}, | ||
| expectErrors: true, | ||
| errorContains: []string{"_dev", "_dev directory is not allowed in built packages"}, | ||
| }, | ||
| { | ||
| name: "multiple _dev directories", | ||
| dirs: []string{ | ||
| "_dev/build", | ||
| "data_stream/foo/_dev/test", | ||
| }, | ||
| expectErrors: true, | ||
| errorContains: []string{"_dev directory is not allowed in built packages"}, | ||
| }, | ||
| { | ||
| name: "directory named _devtools is not rejected", | ||
| dirs: []string{"_devtools"}, | ||
| expectErrors: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
|
|
||
| for _, d := range tc.dirs { | ||
| require.NoError(t, os.MkdirAll(filepath.Join(tempDir, d), 0o755)) | ||
| } | ||
|
|
||
| fsys := fspath.DirFS(tempDir) | ||
| errs := ValidateNoDevFolder(fsys) | ||
|
|
||
| if !tc.expectErrors { | ||
| assert.Nil(t, errs, "expected no errors but got: %v", errs) | ||
| return | ||
| } | ||
|
|
||
| require.NotNil(t, errs, "expected validation errors but got none") | ||
| var sb strings.Builder | ||
| for _, e := range errs { | ||
| sb.WriteString(e.Error()) | ||
| sb.WriteString("\n") | ||
| } | ||
| combined := sb.String() | ||
| for _, substr := range tc.errorContains { | ||
| assert.Contains(t, combined, substr) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation of this change? Can we have anything under
dataStreamDirthat is not a directory?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by the spec definition there are only directories on the datastream dir, but, fs.ReadDir is gathering directories and files, so i thought it was interesting to add this guard at this layer to ensure it. based on the docs https://pkg.go.dev/io/fs#DirEntry the direntry interface could be one or the other...