-
Notifications
You must be signed in to change notification settings - Fork 87
Fix template path validation including linked files #1009
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
Fix template path validation including linked files #1009
Conversation
…package policy_template paths with linked files
|
Shall this also be check at input packages? 🤔 can the template be a link file at agent/input? I can't seem to find in the spec the link allowance parameter for inputs. The spec does not have an "agent" folder |
Input packages use the same definition for package-spec/spec/input/spec.yml Lines 33 to 37 in b9e2c04
And in that file it is allowed link files: package-spec/spec/integration/agent/spec.yml Lines 10 to 15 in b9e2c04
So, it should be also added for |
|
|
||
| // findPathWithPattern looks for a file matching the templatePath in the given data stream directory | ||
| // It checks for exact matches, files ending with the templatePath, or templatePath + ".link" | ||
| func findPathWithPattern(fsys fspath.FS, dsDir, templatePath string) (string, error) { |
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.
Another option here is using directly the fsys that will iterate over the linked files already "rendered".
package-spec/code/go/pkg/validator/validator.go
Lines 53 to 62 in b9e2c04
| func ValidateFromFS(location string, fsys fs.FS) error { | |
| // If we are not explicitly using the linkedfiles.FS, we wrap fsys with | |
| // a linkedfiles.BlockFS to block the use of linked files. | |
| if _, ok := fsys.(*linkedfiles.FS); !ok { | |
| fsys = linkedfiles.NewBlockFS(fsys) | |
| } | |
| pkg, err := packages.NewPackageFromFS(location, fsys) | |
| if err != nil { | |
| return err | |
| } |
So maybe it could be used fs.WalkDir and look for the template paths without checking explictely for *.link. Maybe something like this (it needs to be tested)?
func findPathWithPattern(fsys fspath.FS, dsDir, templatePath string) (string, error) {
mainPath := path.Join(dsDir, "agent", "stream")
foundFile := ""
err := fs.WalkDir(fsys, mainPath, func(p string, d fs.DirEntry, walkErr error) error {
if walkErr != nil {
return walkErr
}
if d.IsDir() {
return nil
}
base := filepath.Base(p)
if base == templatePath || strings.HasSuffix(p, templatePath) {
foundFile = base
return fs.SkipAll // found a match, stop walking
}
return nil
})
if err != nil {
return "", err
}
if foundFile == "" {
return "", errTemplateNotFound
}
return foundFile, ""
}As a advantage, there is no need to check *.link files.
As a disadvantage, it requires to iterate over all files in the directory.
WDYT @jsoriano @teresaromero ?
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.
Just checked locally and my proposed solution here does not work as I expected.
It should be added also more conditions to that if in order to work successfully:
if base == templatePath || base == templatePath+".link" || strings.HasSuffix(match, templatePath) || strings.HasSuffix(match, templatePath+".link") {
foundFile = match
break
}So there is no advantage of using WalkDir here.
The current implementation based on fs.Glob could be kept 👍
| } | ||
|
|
||
| _, err := fs.ReadFile(fsys, path.Join(dsDir, "agent", "stream", stream.TemplatePath)) | ||
| foundFile, err := findPathWithPattern(fsys, dsDir, stream.TemplatePath) |
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.
It looks like that these changes should be added also into validateAgentInputTemplatePath:
package-spec/code/go/internal/validator/semantic/validate_input_policy_template_template_path.go
Lines 87 to 98 in b9e2c04
| func validateAgentInputTemplatePath(fsys fspath.FS, tmplPath string) error { | |
| templatePath := path.Join("agent", "input", tmplPath) | |
| _, err := fs.Stat(fsys, templatePath) | |
| if err != nil { | |
| if errors.Is(err, os.ErrNotExist) { | |
| return errTemplateNotFound | |
| } | |
| return fmt.Errorf("failed to stat template file %s: %w", fsys.Path(templatePath), err) | |
| } | |
| return nil | |
| } |
since that path also allow linked files:
package-spec/spec/integration/agent/spec.yml
Lines 4 to 15 in b9e2c04
| - description: Folder containing input definitions | |
| type: folder | |
| name: input | |
| required: true | |
| additionalContents: false | |
| contents: | |
| - description: Config template file for inputs defined in the policy_templates section of the top level manifest | |
| type: file | |
| sizeLimit: 2MB | |
| pattern: '^.+\.yml\.hbs$' | |
| required: true | |
| allowLink: true |
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.
I've updated the PR with this change. Made the finder function more broad with the dir param instead of data stream dir. Updated also test cases
|
|
||
| // findPathWithPattern looks for a file matching the templatePath in the given data stream directory | ||
| // It checks for exact matches, files ending with the templatePath, or templatePath + ".link" | ||
| func findPathWithPattern(fsys fspath.FS, dsDir, templatePath string) (string, error) { |
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.
Just checked locally and my proposed solution here does not work as I expected.
It should be added also more conditions to that if in order to work successfully:
if base == templatePath || base == templatePath+".link" || strings.HasSuffix(match, templatePath) || strings.HasSuffix(match, templatePath+".link") {
foundFile = match
break
}So there is no advantage of using WalkDir here.
The current implementation based on fs.Glob could be kept 👍
| if strings.HasSuffix(match, templatePath) { | ||
| foundFile = match | ||
| break | ||
| } |
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.
It should be added a new if to validate whether or not it exists a template path with the suffix templatePath+".link" too.
In the with_links test package from elastic-package, for instance there could be a file named:
test/packages/other/with_links/data_stream/first/agent/stream/filestream.yml.hbs.link
And this implementation would fail as:
Error: checking package failed: linting package failed: found 1 validation error:
1. file "/home/user/Coding/work/elastic-package-tere/test/packages/other/with_links/manifest.yml" is invalid: policy template "sample" references input template_path: error validating input from streams "logfile": template file not found
You could create a new variable templatePathWithLink or similar to store the path adding the extension .link.
var foundFile string
templatePathWithLink := fmt.Sprintf("%s.link", templatePath)
for _, match := range matches {
base := filepath.Base(match)
if base == templatePath || base == templatePathWithLink {
foundFile = match
break
}
// fallback to check for suffix match, in case the path is prefixed
if strings.HasSuffix(match, templatePath) {
foundFile = match
break
}
if strings.HasSuffix(match, templatePathWithLink) {
foundFile = match
break
}
}
code/go/internal/validator/semantic/validate_integration_policy_template_path.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Mario Rodriguez Molins <[email protected]>
Co-authored-by: Mario Rodriguez Molins <[email protected]>
jsoriano
left a comment
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.
LGTM, added a couple of small comments.
code/go/internal/validator/semantic/validate_integration_policy_template_path.go
Outdated
Show resolved
Hide resolved
code/go/internal/validator/semantic/validate_integration_policy_template_path.go
Outdated
Show resolved
Hide resolved
| // findPathWithPattern looks for a file matching the templatePath in the given directory (dir) | ||
| // It checks for exact matches, files ending with the templatePath, or templatePath + ".link" | ||
| func findPathWithPattern(fsys fspath.FS, dir, templatePath string) (string, error) { |
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.
Nit. It would be more correct to call this findPathWithSuffix, right?
| // findPathWithPattern looks for a file matching the templatePath in the given directory (dir) | |
| // It checks for exact matches, files ending with the templatePath, or templatePath + ".link" | |
| func findPathWithPattern(fsys fspath.FS, dir, templatePath string) (string, error) { | |
| // findPathWithSuffix looks for a file ending with the suffix in the given directory (dir) | |
| // It checks for exact matches, files ending with the suffix, or suffix + ".link" | |
| func findPathWithSuffix(fsys fspath.FS, dir, suffix string) (string, error) { |
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.
i got the point for renaming, but was not sure about the suffix suggestion. The function also matches exact without being a suffix... i've renamed it to findPathAtDirectory... as the files should be inside the dir param ... WDYT?
mrodm
left a comment
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.
LGTM!
Just a minor comment related to the changelog entry description.
Co-authored-by: Mario Rodriguez Molins <[email protected]>
💚 Build Succeeded
History
|
What does this PR do?
Includes a fix at the integration packages template path validation, taking into account the existence of linked files.
A template can be shared accross data streams by using a shared file and at the agent/stream file, place a
.link.The validation previously implemented did not take into account this occurence, as the iterator was looking for specific file names. It has been spotted while running the integration tests with
elastic-package, as the test case forwith_linkspackage was correctly mapped.Why it was not detected at
package-spectests? The package representing a package with link files was not taking into account that streams are linked with the manifest policy template declaration. This PR modifies the package so the test is reliable and spots this occurance.Why is it important?
When linked files are used to share data streams templates, the validation should be able to check if the file exists, and the final "included" file exists too.
The fs handler already has embedded the link files system, so when a file is
.linkit looks for the included directly.package-spec/code/go/internal/linkedfiles/fs.go
Line 45 in 1bd98cf
Checklist
tested replacing package-spec at elastic-package and testing the error
replace github.com/elastic/package-spec/v3 v3.5.0 => ../package-spectest/packagesthat prove my change is effective.spec/changelog.yml.Related issues
Related #703
Bug introduced at #1002