Switch for disabling validation

[mtojek]

Issue: #653

This PR introduces a switch for disabling package validation. In public image builds of package-storage the validation will be shifted from runtime to build time.

Changes:

• added switch to disable package validation
• refactored Validate() methods to become idempotent

Unit tests are passing.

[elasticmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: Pull request #667 updated

• Start Time: 2021-01-04T14:15:00.106+0000

• Duration: 7 min 12 sec

Test stats 🧪

Test	Results
Failed	0
Passed	169
Skipped	0
Total	169

[ruflin]

Should it be enabled or disabled by default? I wonder if we have less reported issues if we enable it by default. If disabled by default, the registry might have issues because packages are not valid.

Everyone just getting started with the registry and running it for some of the packages should have validation enabled. The only place we need to disable it is for the production environment because we know there, we already have validation in place.

That is why I would switch the defaults.

[mtojek]

I think we can go this way too, might be more intuitive and backwards compatible. I adjusted the PR and it's description.

[ruflin]

LGTM. This also cleans up the code around the validation which is great.

[ruflin]

Quite a long name for a flag but no better suggestion.

[mtojek]

It's one of these feature flags that you should be discouraged to use :)

[ruflin]

This is kind of a mix between validation and building the data model. I wonder if we should clean it up later or keep it as is. It would probably mean doing parts of it twice.

[mtojek]

I'm not sure if I understand your concerns. Any code that modifies a package or a datastream is placed in constructors (e.g. NewDataStream). Are you referring to this part of code:

package-registry/util/data_stream.go

Lines 161 to 183 in b20850d

	if d.Elasticsearch != nil && d.Elasticsearch.IngestPipelineName == "" {
	// Check that no ingest pipeline exists in the directory except default
	for _, path := range paths {
	if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
	d.Elasticsearch.IngestPipelineName = DefaultPipelineName
	// TODO: remove because of legacy
	d.IngestPipeline = DefaultPipelineName
	break
	}
	}
	// TODO: Remove, only here for legacy
	} else if d.IngestPipeline == "" {
	// Check that no ingest pipeline exists in the directory except default
	for _, path := range paths {
	if filepath.Base(path) == DefaultPipelineNameJSON || filepath.Base(path) == DefaultPipelineNameYAML {
	d.IngestPipeline = DefaultPipelineName
	break
	}
	}
	}
	if d.IngestPipeline == "" && len(paths) > 0 {
	return nil, fmt.Errorf("unused pipelines in the package (dataset: %s): %s", d.Dataset, strings.Join(paths, ","))
	}

? If so, then we should come back here and refactor, as the DataStream.IngestPipeline is marked as deprecated.

I suppose we can strip this part of at all, unless there is Kibana logic using it.

[ruflin]

What I initially assume could be an issue does not seem to be one after quickly checking the code. I was worried that for example the pipeline checks do not happen if only Validate is run. But this is not the case as before validate is run, the constructor is used which runs all these additional checks always to build the actual package object. So all good.

[mtojek]

Yeah, I know the design is not really clear as we have to sneak the condition into the Validate function, but I didn't see any Option to disable validation at Unpack.

[ruflin]

I think I know the person who built this initially and is to blame for the design 😆