From 85724cf2c3c17f5e1cba9f791d9a6f6d77a56bfd Mon Sep 17 00:00:00 2001 From: Aaron Crickenberger Date: Mon, 1 Jun 2020 17:02:48 -0700 Subject: [PATCH 1/2] behaviors: fix kubeconform link kubeconform was choking on a typo in the description field, so I fixed the typo while adding friendlier logging to tell me which file was invalid I got curious why tests didn't catch this, and it turns out kubeconform and the behavior tests use different codepaths to load and validate. So I merged them together --- test/conformance/behaviors/BUILD | 12 ++- test/conformance/behaviors/behaviors.go | 74 +++++++++++++++++++ test/conformance/behaviors/behaviors_test.go | 40 +--------- .../behaviors/sig-node/pod-spec.yaml | 4 +- test/conformance/kubeconform/link.go | 31 ++------ 5 files changed, 96 insertions(+), 65 deletions(-) create mode 100644 test/conformance/behaviors/behaviors.go diff --git a/test/conformance/behaviors/BUILD b/test/conformance/behaviors/BUILD index c628970328e..5cb79459a9d 100644 --- a/test/conformance/behaviors/BUILD +++ b/test/conformance/behaviors/BUILD @@ -2,9 +2,16 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "go_default_library", - srcs = ["types.go"], + srcs = [ + "behaviors.go", + "types.go", + ], importpath = "k8s.io/kubernetes/test/conformance/behaviors", visibility = ["//visibility:public"], + deps = [ + "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library", + "//vendor/gopkg.in/yaml.v2:go_default_library", + ], ) go_test( @@ -12,9 +19,6 @@ go_test( srcs = ["behaviors_test.go"], data = glob(["*/*.yaml"]), embed = [":go_default_library"], - deps = [ - "//vendor/gopkg.in/yaml.v2:go_default_library", - ], ) filegroup( diff --git a/test/conformance/behaviors/behaviors.go b/test/conformance/behaviors/behaviors.go new file mode 100644 index 00000000000..95eba28d1b4 --- /dev/null +++ b/test/conformance/behaviors/behaviors.go @@ -0,0 +1,74 @@ +/* +Copyright 2020 The Kubernetes 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 behaviors + +import ( + "fmt" + "io/ioutil" + utilerrors "k8s.io/apimachinery/pkg/util/errors" + "os" + "path/filepath" + "regexp" + + "gopkg.in/yaml.v2" +) + +// BehaviorFileList returns a list of eligible behavior files in or under dir +func BehaviorFileList(dir string) ([]string, error) { + var behaviorFiles []string + + r, _ := regexp.Compile(".+.yaml$") + err := filepath.Walk(dir, + func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if r.MatchString(path) { + behaviorFiles = append(behaviorFiles, path) + } + return nil + }, + ) + return behaviorFiles, err +} + +// LoadSuite loads a Behavior Suite from .yaml file at path +func LoadSuite(path string) (*Suite, error) { + var suite Suite + bytes, err := ioutil.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("error loading suite %s: %v", path, err) + } + err = yaml.UnmarshalStrict(bytes, &suite) + if err != nil { + return nil, fmt.Errorf("error loading suite %s: %v", path, err) + } + return &suite, nil +} + +// ValidateSuite validates that the given suite has no duplicate behavior IDs +func ValidateSuite(suite *Suite) error { + var errs []error + behaviorsByID := make(map[string]bool) + for _, b := range suite.Behaviors { + if _, ok := behaviorsByID[b.ID]; ok { + errs = append(errs, fmt.Errorf("Duplicate behavior ID: %s", b.ID)) + } + behaviorsByID[b.ID] = true + } + return utilerrors.NewAggregate(errs) +} diff --git a/test/conformance/behaviors/behaviors_test.go b/test/conformance/behaviors/behaviors_test.go index b3f7163314e..ee49755fe16 100644 --- a/test/conformance/behaviors/behaviors_test.go +++ b/test/conformance/behaviors/behaviors_test.go @@ -17,30 +17,11 @@ limitations under the License. package behaviors import ( - "io/ioutil" - "os" - "path/filepath" - "regexp" "testing" - - "gopkg.in/yaml.v2" ) func TestValidate(t *testing.T) { - var behaviorFiles []string - - err := filepath.Walk(".", - func(path string, info os.FileInfo, err error) error { - if err != nil { - t.Errorf("%q", err.Error()) - } - - r, _ := regexp.Compile(".+.yaml$") - if r.MatchString(path) { - behaviorFiles = append(behaviorFiles, path) - } - return nil - }) + behaviorFiles, err := BehaviorFileList(".") if err != nil { t.Errorf("%q", err.Error()) } @@ -51,25 +32,12 @@ func TestValidate(t *testing.T) { } func validateSuite(path string, t *testing.T) { - var suite Suite - yamlFile, err := ioutil.ReadFile(path) + suite, err := LoadSuite(path) if err != nil { t.Errorf("%q", err.Error()) } - err = yaml.Unmarshal(yamlFile, &suite) - + err = ValidateSuite(suite) if err != nil { - t.Errorf("%q", err.Error()) - } - - behaviorIDList := make(map[string]bool) - - for _, behavior := range suite.Behaviors { - - // Ensure no behavior IDs are duplicated - if _, ok := behaviorIDList[behavior.ID]; ok { - t.Errorf("Duplicate behavior ID: %s", behavior.ID) - } - behaviorIDList[behavior.ID] = true + t.Errorf("error validating %s: %q", path, err.Error()) } } diff --git a/test/conformance/behaviors/sig-node/pod-spec.yaml b/test/conformance/behaviors/sig-node/pod-spec.yaml index c2462fbe690..7b25dacd5f5 100644 --- a/test/conformance/behaviors/sig-node/pod-spec.yaml +++ b/test/conformance/behaviors/sig-node/pod-spec.yaml @@ -42,8 +42,8 @@ behaviors: description: When hostIPC is set to false, the Pod MUST NOT use the host's inter-process communication namespace. - id: pod/spec/label/create - decription: Create a Pod with a unique label. Query for the Pod with the label as selector MUST be successful. + description: Create a Pod with a unique label. Query for the Pod with the label as selector MUST be successful. - id: pod/spec/label/patch - decription: A patch request must be able to update the pod to change the value of an existing Label. Query for the Pod with the new value for the label MUST be successful. + description: A patch request must be able to update the pod to change the value of an existing Label. Query for the Pod with the new value for the label MUST be successful. - id: pod/spec/container/resources description: Create a Pod with CPU and Memory request and limits. Pod status MUST have QOSClass set to PodQOSGuaranteed. diff --git a/test/conformance/kubeconform/link.go b/test/conformance/kubeconform/link.go index 1bdfe830830..eadb5e8c5b0 100644 --- a/test/conformance/kubeconform/link.go +++ b/test/conformance/kubeconform/link.go @@ -19,9 +19,6 @@ package main import ( "fmt" "io/ioutil" - "os" - "path/filepath" - "regexp" "gopkg.in/yaml.v2" @@ -29,30 +26,21 @@ import ( ) func link(o *options) error { - var behaviorFiles []string behaviorsMapping := make(map[string][]string) var conformanceDataList []behaviors.ConformanceData - err := filepath.Walk(o.behaviorsDir, - func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - r, _ := regexp.Compile(".+.yaml$") - if r.MatchString(path) { - behaviorFiles = append(behaviorFiles, path) - } - return nil - }) + behaviorFiles, err := behaviors.BehaviorFileList(o.behaviorsDir) if err != nil { return err } + fmt.Println() fmt.Printf("Using behaviors from these %d files:\n", len(behaviorFiles)) for _, f := range behaviorFiles { fmt.Println(" ", f) } fmt.Println() + if o.listAll { fmt.Println("All behaviors:") } else { @@ -60,17 +48,14 @@ func link(o *options) error { } for _, behaviorFile := range behaviorFiles { - var suite behaviors.Suite - - yamlFile, err := ioutil.ReadFile(behaviorFile) + suite, err := behaviors.LoadSuite(behaviorFile) if err != nil { return err } - err = yaml.UnmarshalStrict(yamlFile, &suite) + err = behaviors.ValidateSuite(suite) if err != nil { - return err + return fmt.Errorf("error validating %s: %q", behaviorFile, err.Error()) } - for _, behavior := range suite.Behaviors { behaviorsMapping[behavior.ID] = nil } @@ -78,12 +63,12 @@ func link(o *options) error { conformanceYaml, err := ioutil.ReadFile(o.testdata) if err != nil { - return err + return fmt.Errorf("%s: %v", o.testdata, err) } err = yaml.Unmarshal(conformanceYaml, &conformanceDataList) if err != nil { - return err + return fmt.Errorf("%s: %v", o.testdata, err) } for _, data := range conformanceDataList { From d960bf1903b887c89bd324c0cceb4b3b915ad970 Mon Sep 17 00:00:00 2001 From: Aaron Crickenberger Date: Wed, 3 Jun 2020 11:03:52 -0700 Subject: [PATCH 2/2] behaviors: enforce behavior id must have suite id as prefix --- test/conformance/behaviors/behaviors.go | 4 ++++ .../behaviors/sig-network/service-spec.yaml | 14 +++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/test/conformance/behaviors/behaviors.go b/test/conformance/behaviors/behaviors.go index 95eba28d1b4..518051ed9fe 100644 --- a/test/conformance/behaviors/behaviors.go +++ b/test/conformance/behaviors/behaviors.go @@ -23,6 +23,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "gopkg.in/yaml.v2" ) @@ -68,6 +69,9 @@ func ValidateSuite(suite *Suite) error { if _, ok := behaviorsByID[b.ID]; ok { errs = append(errs, fmt.Errorf("Duplicate behavior ID: %s", b.ID)) } + if !strings.HasPrefix(b.ID, suite.Suite) { + errs = append(errs, fmt.Errorf("Invalid behavior ID: %s, must have suite name as prefix: %s", b.ID, suite.Suite)) + } behaviorsByID[b.ID] = true } return utilerrors.NewAggregate(errs) diff --git a/test/conformance/behaviors/sig-network/service-spec.yaml b/test/conformance/behaviors/sig-network/service-spec.yaml index cb3228d8cdb..6beb9be2f98 100644 --- a/test/conformance/behaviors/sig-network/service-spec.yaml +++ b/test/conformance/behaviors/sig-network/service-spec.yaml @@ -1,30 +1,30 @@ suite: service/spec description: Base suite for services behaviors: -- id: service/basic-create/selector +- id: service/spec/selector/present-during-create description: When a Service resource is created with type "ClusterIP", "NodePort", or "LoadBalancer", and a selector is specified, an Endpoints object is generated based with the IPs of pods with label keys and values matching the selector. -- id: service/basic-create/no-selector +- id: service/spec/selector/absent-during-create description: When a Service resource is created and a no selector is specified, no changes are made to any corresponding Endpoints object. -- id: service/type/ClusterIP/empty +- id: service/spec/type/ClusterIP/empty description: When the Service type is specified as "ClusterIP" and the clusterIP field is empty, a cluster-internal IP address for load-balancing to endpoints is allocated. -- id: service/type/ClusterIP/static +- id: service/spec/type/ClusterIP/static description: When the Service type is specified as "ClusterIP" and the clusterIP field is specified as an IP address in the cluster service range, and that IP is not already assigned to another service, that IP is be allocated as a cluster-internal IP address for load-balancing to endpoints. -- id: service/type/ClusterIP/None +- id: service/spec/type/ClusterIP/None description: When the Service type is specified as "ClusterIP" and the clusterIP field is "None", no virtual IP is allocated and the endpoints are published as a set of endpoints rather than a stable IP. -- id: service/type/NodePort +- id: service/spec/type/NodePort description: When the Service type is specified as "NodePort" , a cluster-internal IP address for load-balancing to endpoints is allocated as for type "ClusterIP". Additionally, a cluster-wide port is allocated on every node, routing to the clusterIP. -- id: service/type/ExternalName +- id: service/spec/type/ExternalName description: When the Service type is specified as "ExternalName", the cluster DNS provider publishes a CNAME pointing from the service to the specified external name.