From 005dc31d33e16f1d6e5cd17099dd1ae00ac026b3 Mon Sep 17 00:00:00 2001 From: Mike Verbanic Date: Mon, 9 Aug 2021 12:23:28 -0400 Subject: [PATCH] Validate generated schemas in generator script (#6385) * fix build --push=false for missing kubeconfig * Fixed log ordering per PR comment * Added validation to schema generation * Added validation to schema generation * Updated validate function * Refactor validate method * Updated error logic * Fix linter error --- docs/content/en/schemas/v2beta19.json | 23 ++++++ hack/schemas/main.go | 19 ++++- hack/schemas/main_test.go | 31 ++++++-- .../schemas/testdata/inline-skiptrim/input.go | 42 ++++++++++ .../testdata/inline-skiptrim/output.json | 79 +++++++++++++++++++ hack/schemas/testdata/invalid-schema/input.go | 44 +++++++++++ pkg/skaffold/schema/v2beta19/config.go | 2 +- 7 files changed, 233 insertions(+), 7 deletions(-) create mode 100644 hack/schemas/testdata/inline-skiptrim/input.go create mode 100644 hack/schemas/testdata/inline-skiptrim/output.json create mode 100644 hack/schemas/testdata/invalid-schema/input.go diff --git a/docs/content/en/schemas/v2beta19.json b/docs/content/en/schemas/v2beta19.json index 06a810c17e2..1356d9baa11 100755 --- a/docs/content/en/schemas/v2beta19.json +++ b/docs/content/en/schemas/v2beta19.json @@ -914,6 +914,29 @@ "description": "describes a dependency on another skaffold configuration.", "x-intellij-html-description": "describes a dependency on another skaffold configuration." }, + "ContainerHook": { + "required": [ + "command" + ], + "properties": { + "command": { + "items": { + "type": "string" + }, + "type": "array", + "description": "command to execute.", + "x-intellij-html-description": "command to execute.", + "default": "[]" + } + }, + "preferredOrder": [ + "command" + ], + "additionalProperties": false, + "type": "object", + "description": "describes a lifecycle hook definition to execute on a container. The container name is inferred from the scope in which this hook is defined.", + "x-intellij-html-description": "describes a lifecycle hook definition to execute on a container. The container name is inferred from the scope in which this hook is defined." + }, "CustomArtifact": { "properties": { "buildCommand": { diff --git a/hack/schemas/main.go b/hack/schemas/main.go index 165b86d1f4b..3c950c083c0 100644 --- a/hack/schemas/main.go +++ b/hack/schemas/main.go @@ -32,6 +32,7 @@ import ( "sync" blackfriday "github.com/russross/blackfriday/v2" + "github.com/xeipuuv/gojsonschema" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema" ) @@ -474,7 +475,23 @@ func (g *schemaGenerator) Apply(inputPath string) ([]byte, error) { Definitions: definitions, } - return toJSON(schema) + buf, err := toJSON(schema) + if err != nil { + return nil, err + } + + if err := validate(buf); err != nil { + return nil, fmt.Errorf("invalid schema generated: %v", err.Error()) + } + + return buf, nil +} + +// Validate generated schema +func validate(data []byte) error { + schemaLoader := gojsonschema.NewBytesLoader(data) + _, err := gojsonschema.NewSchema(schemaLoader) + return err } // Make sure HTML description are not encoded diff --git a/hack/schemas/main_test.go b/hack/schemas/main_test.go index f0b33d140f1..d9dfb279d7c 100644 --- a/hack/schemas/main_test.go +++ b/hack/schemas/main_test.go @@ -23,7 +23,6 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/xeipuuv/gojsonschema" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -46,6 +45,7 @@ func TestGenerators(t *testing.T) { {name: "inline"}, {name: "inline-anyof"}, {name: "inline-hybrid"}, + {name: "inline-skiptrim"}, {name: "integer"}, } for _, test := range tests { @@ -65,10 +65,6 @@ func TestGenerators(t *testing.T) { expected = bytes.ReplaceAll(expected, []byte("\r\n"), []byte("\n")) - schemaLoader := gojsonschema.NewBytesLoader(actual) - _, err = gojsonschema.NewSchema(schemaLoader) - t.CheckNoError(err) - if diff := cmp.Diff(string(actual), string(expected)); diff != "" { t.Errorf("%T differ (-got, +want): %s\n actual:\n%s", string(expected), diff, string(actual)) return @@ -76,3 +72,28 @@ func TestGenerators(t *testing.T) { }) } } + +func TestGeneratorErrors(t *testing.T) { + tests := []struct { + name string + shouldErr bool + expectedError string + }{ + {name: "invalid-schema", shouldErr: true, expectedError: "Object has no key 'InlineStruct'"}, + } + for _, test := range tests { + testutil.Run(t, test.name, func(t *testutil.T) { + input := filepath.Join("testdata", test.name, "input.go") + + generator := schemaGenerator{ + strict: false, + } + + _, err := generator.Apply(input) + t.CheckError(test.shouldErr, err) + if test.expectedError != "" { + t.CheckErrorContains(test.expectedError, err) + } + }) + } +} diff --git a/hack/schemas/testdata/inline-skiptrim/input.go b/hack/schemas/testdata/inline-skiptrim/input.go new file mode 100644 index 00000000000..ac4e3f3f145 --- /dev/null +++ b/hack/schemas/testdata/inline-skiptrim/input.go @@ -0,0 +1,42 @@ +/* +Copyright 2021 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package latest + +// TestStruct for testing the schema generator. +type TestStruct struct { + + // RequiredField should be required + RequiredField string `yaml:"reqField" yamltags:"required"` + + // AnotherField has reference to InlineStruct + AnotherField *InlineStruct `yaml:"anotherField"` +} + +// AnotherTestStruct for testing the schema s generator. +type AnotherTestStruct struct { + InlineStruct `yaml:"inline" yamltags:"skipTrim"` +} + +// InlineStruct is embedded inline into TestStruct +type InlineStruct struct { + + // Field1 should be the first choice + Field1 string `yaml:"f1"` + + // Field2 should be the second choice + Field2 string `yaml:"f2"` +} diff --git a/hack/schemas/testdata/inline-skiptrim/output.json b/hack/schemas/testdata/inline-skiptrim/output.json new file mode 100644 index 00000000000..d2c90ecd827 --- /dev/null +++ b/hack/schemas/testdata/inline-skiptrim/output.json @@ -0,0 +1,79 @@ +{ + "type": "object", + "anyOf": [ + { + "$ref": "#/definitions/TestStruct" + } + ], + "$schema": "http://json-schema-org/draft-07/schema#", + "definitions": { + "AnotherTestStruct": { + "properties": { + "f1": { + "type": "string", + "description": "should be the first choice", + "x-intellij-html-description": "should be the first choice" + }, + "f2": { + "type": "string", + "description": "should be the second choice", + "x-intellij-html-description": "should be the second choice" + } + }, + "preferredOrder": [ + "f1", + "f2" + ], + "type": "object", + "description": "for testing the schema s generator.", + "x-intellij-html-description": "for testing the schema s generator." + }, + "InlineStruct": { + "properties": { + "f1": { + "type": "string", + "description": "should be the first choice", + "x-intellij-html-description": "should be the first choice" + }, + "f2": { + "type": "string", + "description": "should be the second choice", + "x-intellij-html-description": "should be the second choice" + } + }, + "preferredOrder": [ + "f1", + "f2" + ], + "additionalProperties": false, + "type": "object", + "description": "embedded inline into TestStruct", + "x-intellij-html-description": "embedded inline into TestStruct" + }, + "TestStruct": { + "required": [ + "reqField" + ], + "properties": { + "anotherField": { + "$ref": "#/definitions/InlineStruct", + "description": "has reference to InlineStruct", + "x-intellij-html-description": "has reference to InlineStruct" + }, + "reqField": { + "type": "string", + "description": "should be required", + "x-intellij-html-description": "should be required" + } + }, + "preferredOrder": [ + "reqField", + "anotherField" + ], + "additionalProperties": false, + "type": "object", + "description": "for testing the schema generator.", + "x-intellij-html-description": "for testing the schema generator." + } + } +} diff --git a/hack/schemas/testdata/invalid-schema/input.go b/hack/schemas/testdata/invalid-schema/input.go new file mode 100644 index 00000000000..2343648c950 --- /dev/null +++ b/hack/schemas/testdata/invalid-schema/input.go @@ -0,0 +1,44 @@ +/* +Copyright 2021 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package latest + +// TestStruct for testing the schema generator. +type TestStruct struct { + + // RequiredField should be required + RequiredField string `yaml:"reqField" yamltags:"required"` + + // AnotherField has reference to InlineStruct + AnotherField *InlineStruct `yaml:"anotherField"` +} + +// AnotherTestStruct for testing the schema s generator. +type AnotherTestStruct struct { + // Not adding yamltags:"skipTrim" causes InlineStruct to be removed from definitions + // This breaks the TestStruct reference above + InlineStruct `yaml:"inline"` +} + +// InlineStruct is embedded inline into TestStruct +type InlineStruct struct { + + // Field1 should be the first choice + Field1 string `yaml:"f1"` + + // Field2 should be the second choice + Field2 string `yaml:"f2"` +} diff --git a/pkg/skaffold/schema/v2beta19/config.go b/pkg/skaffold/schema/v2beta19/config.go index 68fba8059aa..c03f58e6b90 100755 --- a/pkg/skaffold/schema/v2beta19/config.go +++ b/pkg/skaffold/schema/v2beta19/config.go @@ -1381,7 +1381,7 @@ type ContainerHook struct { // NamedContainerHook describes a lifecycle hook definition to execute on a named container. type NamedContainerHook struct { // ContainerHook describes a lifecycle hook definition to execute on a container. - ContainerHook `yaml:",inline"` + ContainerHook `yaml:",inline" yamltags:"skipTrim"` // PodName is the name of the pod to execute the command in. PodName string `yaml:"podName" yamltags:"required"` // ContainerName is the name of the container to execute the command in.