-
Notifications
You must be signed in to change notification settings - Fork 91
validator: introduce mode-aware constructors and validation API #1177
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
base: main
Are you sure you want to change the base?
Changes from 11 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
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 |
|---|---|---|
|
|
@@ -6,3 +6,5 @@ fuzz | |
| /build/ | ||
| .vscode/ | ||
| .DS_Store | ||
|
|
||
| .scratch/ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| // 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 modes | ||
|
|
||
| // Mode represents the validation mode used when validating a package. | ||
| type Mode string | ||
|
|
||
| // Validation modes. The public API re-exports these as validator.Mode* constants. | ||
| const ( | ||
| Legacy Mode = "legacy" | ||
| Source Mode = "source" | ||
| Build Mode = "build" | ||
| ) | ||
|
|
||
| // Valid reports whether m is a recognised validation mode. | ||
| func (m Mode) Valid() bool { | ||
| switch m { | ||
| case Legacy, Source, Build: | ||
| return true | ||
| } | ||
| return false | ||
| } |
|
Member
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. "api" is redundant here, any public method is going to be part of the package API 🙂 This could belong to |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,203 @@ | ||||||||||||||
| // 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 validator | ||||||||||||||
|
|
||||||||||||||
| import ( | ||||||||||||||
| "archive/zip" | ||||||||||||||
| "errors" | ||||||||||||||
| "fmt" | ||||||||||||||
| "io" | ||||||||||||||
| "io/fs" | ||||||||||||||
| "os" | ||||||||||||||
|
|
||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/linkedfiles" | ||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/packages" | ||||||||||||||
| internalvalidator "github.com/elastic/package-spec/v3/code/go/internal/validator" | ||||||||||||||
| "github.com/elastic/package-spec/v3/code/go/internal/validator/common" | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| // Validator holds the configuration for a package validation run. | ||||||||||||||
| // Create one with NewFromPath, NewFromZip, or NewFromFS, then call Validate. | ||||||||||||||
| type Validator struct { | ||||||||||||||
| mode Mode | ||||||||||||||
| location string | ||||||||||||||
| fsys fs.FS | ||||||||||||||
| warningsAsErrors bool | ||||||||||||||
| // closer is non-nil when the Validator owns a resource (e.g. a zip reader) | ||||||||||||||
| // that must be released after Validate returns. | ||||||||||||||
| closer io.Closer | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Option configures a Validator. | ||||||||||||||
| type Option func(*Validator) | ||||||||||||||
|
|
||||||||||||||
| // WithWarningsAsErrors makes validation warnings count as errors when enabled is true. | ||||||||||||||
| func WithWarningsAsErrors(enabled bool) Option { | ||||||||||||||
| return func(v *Validator) { v.warningsAsErrors = enabled } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromPath returns a Validator for the package rooted at packageRootPath. | ||||||||||||||
| // | ||||||||||||||
| // For ModeLegacy and ModeSource the filesystem honours linked (.link) files; | ||||||||||||||
| // for ModeBuild linked files are blocked (matching a built package artifact). | ||||||||||||||
| func NewFromPath(mode Mode, packageRootPath string, opts ...Option) (*Validator, error) { | ||||||||||||||
|
Member
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. Configuration structs are usually easier to discover than functional options, and use to be simpler.
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. This config is used for the warnings, which is read from the env, so i removed the options/config and read it directly from the env. Changed the test case too to use t.Env instead of a local variable.
Member
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. Part of the original design in #549 was actually to introduce the "Option C" API shape, a new "Validator" object that can be somehow customized with different options. With current code we are back to something like "Option A": methods with the mode as parameter. Only that we have one method per mode instead of having the mode as a parameter. I still think it would be valuable to introduce this "Validator" object. Having the object we can add configuration options later without needing to add more functions to the package, or parameters to these functions. For this initial version it could have |
||||||||||||||
| if !mode.Valid() { | ||||||||||||||
| return nil, fmt.Errorf("invalid validation mode %q", mode) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| info, err := os.Stat(packageRootPath) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("invalid package path %q: %w", packageRootPath, err) | ||||||||||||||
| } | ||||||||||||||
| if !info.IsDir() { | ||||||||||||||
| return nil, fmt.Errorf("invalid package path %q: not a directory", packageRootPath) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| var fsys fs.FS | ||||||||||||||
| if mode == ModeBuild { | ||||||||||||||
| fsys = linkedfiles.NewBlockFS(os.DirFS(packageRootPath)) | ||||||||||||||
| } else { | ||||||||||||||
| // ModeLegacy and ModeSource: resolve linked files transparently. | ||||||||||||||
| fsys = linkedfiles.NewFS(packageRootPath, os.DirFS(packageRootPath)) | ||||||||||||||
| } | ||||||||||||||
| return buildValidator(mode, packageRootPath, fsys, nil, opts), nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromZip returns a Validator for the package stored in the zip file at zipPath. | ||||||||||||||
| // | ||||||||||||||
| // Zip files always contain built packages — they are the output format produced | ||||||||||||||
| // by elastic-package build and consumed by the package registry and Fleet. | ||||||||||||||
| // Validation always runs in ModeBuild; source-only artifacts (_dev/, .link files, | ||||||||||||||
| // external: ecs references) are therefore rejected. | ||||||||||||||
| // | ||||||||||||||
| // NOTE: ModeBuild-specific validation rules are not yet implemented; ModeBuild | ||||||||||||||
| // and ModeLegacy currently produce identical rule sets. This is intentional — | ||||||||||||||
| // the mode is set here so that future PRs can attach build-only rules without | ||||||||||||||
| // changing the public API. | ||||||||||||||
| // | ||||||||||||||
| // The returned Validator owns the underlying zip reader; calling Validate closes it. | ||||||||||||||
| // Do not call Validate more than once on a Validator created by NewFromZip. | ||||||||||||||
| func NewFromZip(zipPath string, opts ...Option) (_ *Validator, err error) { | ||||||||||||||
| r, openErr := zip.OpenReader(zipPath) | ||||||||||||||
| if openErr != nil { | ||||||||||||||
| return nil, fmt.Errorf("failed to open zip file (%s): %w", zipPath, openErr) | ||||||||||||||
| } | ||||||||||||||
| // Close the reader on any error path; on success the Validator takes ownership. | ||||||||||||||
| defer func() { | ||||||||||||||
| if err != nil { | ||||||||||||||
| if cerr := r.Close(); cerr != nil { | ||||||||||||||
| err = errors.Join(err, cerr) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| }() | ||||||||||||||
|
|
||||||||||||||
| dirs, err := fs.ReadDir(r, ".") | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("failed to read root directory in zip file (%s): %w", zipPath, err) | ||||||||||||||
| } | ||||||||||||||
| if len(dirs) != 1 { | ||||||||||||||
| return nil, fmt.Errorf("a single directory is expected in zip file, %d found", len(dirs)) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| subDir, err := fs.Sub(r, dirs[0].Name()) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, err | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Zip archives contain built packages; linked files are always blocked. | ||||||||||||||
| fsys := linkedfiles.NewBlockFS(subDir) | ||||||||||||||
| return buildValidator(ModeBuild, zipPath, fsys, r, opts), nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // NewFromFS returns a Validator for the package accessible through fsys at location. | ||||||||||||||
| // | ||||||||||||||
| // Linked-file handling depends on mode: | ||||||||||||||
| // - ModeSource: if fsys is not already a *linkedfiles.FS it is wrapped with | ||||||||||||||
| // linkedfiles.NewFS so .link files are resolved transparently. | ||||||||||||||
| // - ModeLegacy: a pre-wrapped *linkedfiles.FS is preserved as-is (links | ||||||||||||||
| // resolved); any other filesystem is wrapped with BlockFS. This matches the | ||||||||||||||
| // behaviour of the deprecated ValidateFromFS. | ||||||||||||||
| // - ModeBuild: BlockFS is always applied, even if fsys is already a | ||||||||||||||
| // *linkedfiles.FS, so linked files are unconditionally rejected. | ||||||||||||||
| func NewFromFS(mode Mode, location string, fsys fs.FS, opts ...Option) (*Validator, error) { | ||||||||||||||
|
jsoriano marked this conversation as resolved.
Outdated
|
||||||||||||||
| if !mode.Valid() { | ||||||||||||||
| return nil, fmt.Errorf("invalid validation mode %q", mode) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| info, err := fs.Stat(fsys, ".") | ||||||||||||||
| if err != nil { | ||||||||||||||
| return nil, fmt.Errorf("invalid package filesystem at %q: %w", location, err) | ||||||||||||||
| } | ||||||||||||||
| if !info.IsDir() { | ||||||||||||||
| return nil, fmt.Errorf("invalid package filesystem at %q: root is not a directory", location) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| _, isLinkedFS := fsys.(*linkedfiles.FS) | ||||||||||||||
| switch mode { | ||||||||||||||
| case ModeSource: | ||||||||||||||
| if !isLinkedFS { | ||||||||||||||
| fsys = linkedfiles.NewFS(location, fsys) | ||||||||||||||
|
Member
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. On what cases we receive a linked FS here? Nit. Maybe |
||||||||||||||
| } | ||||||||||||||
| case ModeBuild: | ||||||||||||||
| // Always block, even a pre-wrapped *linkedfiles.FS. | ||||||||||||||
| fsys = linkedfiles.NewBlockFS(fsys) | ||||||||||||||
|
Member
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. Umm, not sure if we should do this at this level. Could we instead reject Or we need to do this because the spec always receives the link files resolved?
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 am not sure i understand this, or the lines have moved around. here i am validating the fsys, not any linked files. The fs is checked dependeing on the mode "selected" if it allows or not .link files. For build validation there link should be resolved.
Member
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. When validating a built package there should not be any I was wondering how we are ensuring that, and if the different FSs used was because of that. |
||||||||||||||
| default: // ModeLegacy | ||||||||||||||
| // Preserve a pre-wrapped *linkedfiles.FS; block everything else. | ||||||||||||||
| // Matches the old validateFromFS behaviour. | ||||||||||||||
|
Member
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. Not sure if this would be needed now. I think this was used before because
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 need to review this whole refactor, as i am thinking it will be better to have direct constructors for modes, instead of using the mode as a switch... |
||||||||||||||
| if !isLinkedFS { | ||||||||||||||
| fsys = linkedfiles.NewBlockFS(fsys) | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
| return buildValidator(mode, location, fsys, nil, opts), nil | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // buildValidator is the shared internal constructor. | ||||||||||||||
| func buildValidator(mode Mode, location string, fsys fs.FS, closer io.Closer, opts []Option) *Validator { | ||||||||||||||
| v := &Validator{ | ||||||||||||||
| mode: mode, | ||||||||||||||
| location: location, | ||||||||||||||
| fsys: fsys, | ||||||||||||||
| warningsAsErrors: common.IsDefinedWarningsAsErrors(), | ||||||||||||||
| closer: closer, | ||||||||||||||
| } | ||||||||||||||
| for _, opt := range opts { | ||||||||||||||
| opt(v) | ||||||||||||||
| } | ||||||||||||||
| return v | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Validate runs package validation and returns any errors encountered. | ||||||||||||||
| // | ||||||||||||||
| // If the Validator was created by NewFromZip it owns an open zip reader; | ||||||||||||||
| // Validate closes it on return, so Validate must not be called more than once | ||||||||||||||
| // on such a Validator. | ||||||||||||||
| func (v *Validator) Validate() (err error) { | ||||||||||||||
| if v.closer != nil { | ||||||||||||||
| defer func() { | ||||||||||||||
| if cerr := v.closer.Close(); cerr != nil { | ||||||||||||||
| err = errors.Join(err, cerr) | ||||||||||||||
| } | ||||||||||||||
| }() | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| pkg, err := packages.NewPackageFromFS(v.location, v.fsys) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| if pkg.SpecVersion == nil { | ||||||||||||||
| return errors.New("could not determine specification version for package") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| spec, err := internalvalidator.NewSpec(*pkg.SpecVersion, v.mode) | ||||||||||||||
| if err != nil { | ||||||||||||||
| return err | ||||||||||||||
| } | ||||||||||||||
| spec.WarningsAsErrors = v.warningsAsErrors | ||||||||||||||
|
|
||||||||||||||
| if errs := spec.ValidatePackage(*pkg); len(errs) > 0 { | ||||||||||||||
| return errs | ||||||||||||||
| } | ||||||||||||||
| return nil | ||||||||||||||
| } | ||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.