From e46bde261337f52336eeebec42b96001b218ef3b Mon Sep 17 00:00:00 2001 From: Brandon Mabey Date: Wed, 20 Feb 2019 15:48:44 -0500 Subject: [PATCH 1/5] Add conformance tests for .metadata.generateName. --- test/conformance/generatename_test.go | 187 ++++++++++++++++++++++++++ test/service.go | 18 ++- 2 files changed, 201 insertions(+), 4 deletions(-) create mode 100644 test/conformance/generatename_test.go diff --git a/test/conformance/generatename_test.go b/test/conformance/generatename_test.go new file mode 100644 index 000000000000..c07a3880e51e --- /dev/null +++ b/test/conformance/generatename_test.go @@ -0,0 +1,187 @@ +// +build e2e + +/* +Copyright 2019 The Knative 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 conformance + +import ( + pkgTest "github.com/knative/pkg/test" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + rtesting "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" + "github.com/knative/serving/test" + "net/http" + "strings" + "testing" +) + +func setServiceGenerateName(generateName string) rtesting.ServiceOption { + return func(service *v1alpha1.Service) { + service.ObjectMeta.GenerateName = generateName + } +} + +func setConfigurationGenerateName(generateName string) rtesting.ConfigOption { + return func(config *v1alpha1.Configuration) { + config.ObjectMeta.GenerateName = generateName + } +} + +func setRouteGenerateName(generateName string) rtesting.RouteOption { + return func(route *v1alpha1.Route) { + route.ObjectMeta.GenerateName = generateName + } +} + +// getGenerateNamePrefix returns the object name to be used for testing, shorted to +// 44 characters to avoid #3236, as generateNames longer than 44 characters may cause +// some knative resources to never become ready. +func getGenerateNamePrefix(t *testing.T) string { + generateName := test.ObjectNameForTest(t) + "-" + + generateNameLength := len(generateName) + if generateNameLength > 44 { + generateNameLength = 44 + } + return generateName[0:generateNameLength] +} + +func ensureCanServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1.Route) { + t.Logf("Route %s has a domain set in its status", route.Name) + var domain string + err := test.WaitForRouteState( + clients.ServingClient, + route.Name, + func(r *v1alpha1.Route) (bool, error) { + domain = r.Status.Domain + return domain != "", nil + }, + "RouteDomain", + ) + + t.Logf("Route %s can serve the expected data at the endpoint", route.Name) + _, err = pkgTest.WaitForEndpointState( + clients.KubeClient, + t.Logf, + domain, + pkgTest.Retrying(pkgTest.EventuallyMatchesBody(helloWorldText), http.StatusNotFound), + "WaitForEndpointToServeText", + test.ServingFlags.ResolvableDomain) + if err != nil { + t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", route.Name, domain, helloWorldText, err) + } + +} + +// TestServiceGenerateName checks that knative Services MAY request names generated by +// the system using metadata.generateName. It ensures that knative Services created this way can become ready +// and serve requests. +func TestServiceGenerateName(t *testing.T) { + t.Parallel() + clients := setup(t) + + generateName := getGenerateNamePrefix(t) + names := test.ResourceNames{ + Image: helloworld, + } + + // Cleanup on test failure. + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + defer func() { test.TearDown(clients, names) }() + + // Create the service using the generate name field. If the serivce does not become ready this will fail. + t.Logf("Creating new service with generateName %s", generateName) + resources, err := test.CreateRunLatestServiceReady(t, clients, &names, &test.Options{}, setServiceGenerateName(generateName)) + if err != nil { + t.Fatalf("Failed to create service with generateName %s: %v", generateName, err) + } + + // Ensure that the name given to the service is generated from the generateName field. + t.Log("When the service is created, the name is generated using the provided generateName") + if names.Service == generateName { + t.Fatalf("Service did not recieve a randomized name, instead got %s", names.Service) + } + if !strings.HasPrefix(names.Service, generateName) { + t.Fatalf("Service name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Service) + } + + // Ensure that the service can serve requests + ensureCanServeRequests(t, clients, resources.Route) +} + +// TestRouteAndConfiguration checks that both routes and configurations MAY request names generated by +// the system using metadata.generateName. It ensures that routes and configurations created this way both: +// 1. Become ready +// 2. Can serve requests. +func TestRouteAndConfigurationGenerateName(t *testing.T) { + t.Parallel() + clients := setup(t) + + generateName := getGenerateNamePrefix(t) + names := test.ResourceNames{ + Image: helloworld, + } + + test.CleanupOnInterrupt(func() { test.TearDown(clients, names) }) + defer func() { test.TearDown(clients, names) }() + + t.Logf("Creating new configuration with generateName %s", generateName) + config, err := test.CreateConfiguration(t, clients, names, &test.Options{}, setConfigurationGenerateName(generateName)) + if err != nil { + t.Fatalf("Failed to create configuration with generateName %s: %v", generateName, err) + } + names.Config = config.Name + + // Ensure the associated revision is created. This also checks that the configuration becomes ready. + t.Log("The configuration will be updated with the name of the associated Revision once it is created.") + names.Revision, err = test.WaitForConfigLatestRevision(clients, names) + if err != nil { + t.Fatalf("Configuration %s was not updated with the new revision: %v", names.Config, err) + } + + // Ensure that the name given to the configuration is generated from the generate name field. + t.Log("When the configuration is created, the name is generated using the provided generateName") + if names.Config == generateName { + t.Fatalf("Configuration did not recieve a randomized name, instead got %s", names.Config) + } + if !strings.HasPrefix(names.Config, generateName) { + t.Fatalf("Configuration name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Config) + } + + // Create a route that maps to the revision created by the configuration above + t.Logf("Create new Route with generateName %s", generateName) + route, err := test.CreateRoute(t, clients, names, setRouteGenerateName(generateName)) + if err != nil { + t.Fatalf("Failed to create route with generateName %s: %v", generateName, err) + } + names.Route = route.Name + + t.Log("When the route is created, it will become ready") + if err := test.WaitForRouteState(clients.ServingClient, names.Route, test.IsRouteReady, "RouteIsReady"); err != nil { + t.Fatalf("Error waiting for the route %s to become ready: %v", names.Route, err) + } + + // Ensure that the name given to the route is generated from the generate name field + if names.Route == generateName { + t.Fatalf("Route did not recieve a randomized name, instead got %s", names.Route) + } + if !strings.HasPrefix(names.Route, generateName) { + t.Fatalf("Route name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Route) + } + + // Ensure that the generated route endpoint can serve requests + ensureCanServeRequests(t, clients, route) +} diff --git a/test/service.go b/test/service.go index c81b9bb939cc..0e837204861f 100644 --- a/test/service.go +++ b/test/service.go @@ -105,8 +105,8 @@ func GetResourceObjects(clients *Clients, names ResourceNames) (*ResourceObjects func CreateReleaseServiceWithLatest( t *testing.T, clients *Clients, names *ResourceNames, options *Options) (*ResourceObjects, error) { - if names.Service == "" || names.Image == "" { - return nil, fmt.Errorf("expected non-empty Service and Image name; got Service = %s, Image = %s", names.Service, names.Image) + if names.Image == "" { + return nil, fmt.Errorf("expected non-empty Image name; got Image=%v", names.Image) } t.Log("Creating a new Service as Release with @latest.") @@ -119,6 +119,11 @@ func CreateReleaseServiceWithLatest( names.Route = serviceresourcenames.Route(svc) names.Config = serviceresourcenames.Configuration(svc) + // If the Service name was not specified, populate it + if names.Service == "" { + names.Service = svc.Name + } + t.Log("Waiting for Service to transition to Ready.") if err := WaitForServiceState(clients.ServingClient, names.Service, IsServiceReady, "ServiceIsReady"); err != nil { return nil, err @@ -138,8 +143,8 @@ func CreateReleaseServiceWithLatest( // Names is updated with the Route and Configuration created by the Service and ResourceObjects is returned with the Service, Route, and Configuration objects. // Returns error if the service does not come up correctly. func CreateRunLatestServiceReady(t *testing.T, clients *Clients, names *ResourceNames, options *Options, fopt ...rtesting.ServiceOption) (*ResourceObjects, error) { - if names.Service == "" || names.Image == "" { - return nil, fmt.Errorf("expected non-empty Service and Image name; got Service=%v, Image=%v", names.Service, names.Image) + if names.Image == "" { + return nil, fmt.Errorf("expected non-empty Image name; got Image=%v", names.Image) } t.Logf("Creating a new Service %s as RunLatest.", names.Service) @@ -152,6 +157,11 @@ func CreateRunLatestServiceReady(t *testing.T, clients *Clients, names *Resource names.Route = serviceresourcenames.Route(svc) names.Config = serviceresourcenames.Configuration(svc) + // If the Service name was not specified, populate it + if names.Service == "" { + names.Service = svc.Name + } + t.Logf("Waiting for Service %q to transition to Ready.", names.Service) if err := WaitForServiceState(clients.ServingClient, names.Service, IsServiceReady, "ServiceIsReady"); err != nil { return nil, err From 3a164fd190ab9fd1b7585d9540a86443f3cfa896 Mon Sep 17 00:00:00 2001 From: Brandon Mabey Date: Thu, 21 Feb 2019 11:38:27 -0500 Subject: [PATCH 2/5] Cleanup generate name conformance tests. --- test/conformance/generatename_test.go | 72 +++++++++++++++++---------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/test/conformance/generatename_test.go b/test/conformance/generatename_test.go index c07a3880e51e..503e4f0bf800 100644 --- a/test/conformance/generatename_test.go +++ b/test/conformance/generatename_test.go @@ -19,13 +19,15 @@ limitations under the License. package conformance import ( + "fmt" + "net/http" + "regexp" + "testing" + pkgTest "github.com/knative/pkg/test" "github.com/knative/serving/pkg/apis/serving/v1alpha1" rtesting "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" "github.com/knative/serving/test" - "net/http" - "strings" - "testing" ) func setServiceGenerateName(generateName string) rtesting.ServiceOption { @@ -46,10 +48,10 @@ func setRouteGenerateName(generateName string) rtesting.RouteOption { } } -// getGenerateNamePrefix returns the object name to be used for testing, shorted to +// generateNamePrefix returns the object name to be used for testing, shorted to // 44 characters to avoid #3236, as generateNames longer than 44 characters may cause // some knative resources to never become ready. -func getGenerateNamePrefix(t *testing.T) string { +func generateNamePrefix(t *testing.T) string { generateName := test.ObjectNameForTest(t) + "-" generateNameLength := len(generateName) @@ -59,7 +61,22 @@ func getGenerateNamePrefix(t *testing.T) string { return generateName[0:generateNameLength] } -func ensureCanServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1.Route) { +// validateName checks that a name generated using a generateName is valid. It checks +// 1. The generateName is a prefix of the name, but they are not equal +// 2. 5 lowercase characters or digits are added onto generateName to create the value of name. +func validateName(generateName, name string) error { + r, err := regexp.Compile("^" + regexp.QuoteMeta(generateName) + "[a-z0-9]{5}$") + if err != nil { + return fmt.Errorf("failed to compile regex to test generated name %s: %v", name, err) + } + + if !r.MatchString(name) { + return fmt.Errorf("expected generated name to match \"%v\", got %s", r, name) + } + return nil +} + +func canServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1.Route) error { t.Logf("Route %s has a domain set in its status", route.Name) var domain string err := test.WaitForRouteState( @@ -71,19 +88,23 @@ func ensureCanServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1 }, "RouteDomain", ) + if err != nil { + return fmt.Errorf("route did not get assigned a domain: %v", err) + } t.Logf("Route %s can serve the expected data at the endpoint", route.Name) _, err = pkgTest.WaitForEndpointState( clients.KubeClient, t.Logf, domain, - pkgTest.Retrying(pkgTest.EventuallyMatchesBody(helloWorldText), http.StatusNotFound), + pkgTest.Retrying(pkgTest.MatchesAllOf(pkgTest.IsStatusOK(), pkgTest.MatchesBody(helloWorldText)), http.StatusNotFound), "WaitForEndpointToServeText", test.ServingFlags.ResolvableDomain) if err != nil { - t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", route.Name, domain, helloWorldText, err) + return fmt.Errorf("the endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", route.Name, domain, helloWorldText, err) } + return nil } // TestServiceGenerateName checks that knative Services MAY request names generated by @@ -93,7 +114,7 @@ func TestServiceGenerateName(t *testing.T) { t.Parallel() clients := setup(t) - generateName := getGenerateNamePrefix(t) + generateName := generateNamePrefix(t) names := test.ResourceNames{ Image: helloworld, } @@ -111,15 +132,15 @@ func TestServiceGenerateName(t *testing.T) { // Ensure that the name given to the service is generated from the generateName field. t.Log("When the service is created, the name is generated using the provided generateName") - if names.Service == generateName { - t.Fatalf("Service did not recieve a randomized name, instead got %s", names.Service) - } - if !strings.HasPrefix(names.Service, generateName) { - t.Fatalf("Service name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Service) + if err := validateName(generateName, names.Service); err != nil { + t.Errorf("Illegal name generated for service %s: %v", names.Service, err) } // Ensure that the service can serve requests - ensureCanServeRequests(t, clients, resources.Route) + err = canServeRequests(t, clients, resources.Route) + if err != nil { + t.Errorf("Service %s could not serve requests: %v", names.Service, err) + } } // TestRouteAndConfiguration checks that both routes and configurations MAY request names generated by @@ -130,7 +151,7 @@ func TestRouteAndConfigurationGenerateName(t *testing.T) { t.Parallel() clients := setup(t) - generateName := getGenerateNamePrefix(t) + generateName := generateNamePrefix(t) names := test.ResourceNames{ Image: helloworld, } @@ -154,11 +175,8 @@ func TestRouteAndConfigurationGenerateName(t *testing.T) { // Ensure that the name given to the configuration is generated from the generate name field. t.Log("When the configuration is created, the name is generated using the provided generateName") - if names.Config == generateName { - t.Fatalf("Configuration did not recieve a randomized name, instead got %s", names.Config) - } - if !strings.HasPrefix(names.Config, generateName) { - t.Fatalf("Configuration name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Config) + if err := validateName(generateName, names.Config); err != nil { + t.Errorf("Illegal name generated for configuration %s: %v", names.Config, err) } // Create a route that maps to the revision created by the configuration above @@ -175,13 +193,13 @@ func TestRouteAndConfigurationGenerateName(t *testing.T) { } // Ensure that the name given to the route is generated from the generate name field - if names.Route == generateName { - t.Fatalf("Route did not recieve a randomized name, instead got %s", names.Route) - } - if !strings.HasPrefix(names.Route, generateName) { - t.Fatalf("Route name is not prefixed by the generateName field, expected %s to be a prefix, got %s", generateName, names.Route) + t.Log("When the route is created, the name is generated using the provided generateName") + if err := validateName(generateName, names.Route); err != nil { + t.Errorf("Illegal name generated for route %s: %v", names.Route, err) } // Ensure that the generated route endpoint can serve requests - ensureCanServeRequests(t, clients, route) + if err := canServeRequests(t, clients, route); err != nil { + t.Errorf("Configuration %s with Route %s could not serve requests: %v", names.Config, names.Route, err) + } } From 1ed5406917fea12156c521df05fa4e8093b8da0a Mon Sep 17 00:00:00 2001 From: Brandon Mabey Date: Fri, 22 Feb 2019 16:34:37 -0500 Subject: [PATCH 3/5] Simplify name verification regex for generateName conformance tests. --- test/conformance/generatename_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/test/conformance/generatename_test.go b/test/conformance/generatename_test.go index 503e4f0bf800..71156b630797 100644 --- a/test/conformance/generatename_test.go +++ b/test/conformance/generatename_test.go @@ -63,12 +63,10 @@ func generateNamePrefix(t *testing.T) string { // validateName checks that a name generated using a generateName is valid. It checks // 1. The generateName is a prefix of the name, but they are not equal -// 2. 5 lowercase characters or digits are added onto generateName to create the value of name. +// 2. Any number of valid name characters (alphanumeric, -, and .) are added togenerateName to +// create the value of name. func validateName(generateName, name string) error { - r, err := regexp.Compile("^" + regexp.QuoteMeta(generateName) + "[a-z0-9]{5}$") - if err != nil { - return fmt.Errorf("failed to compile regex to test generated name %s: %v", name, err) - } + r := regexp.MustCompile("^" + regexp.QuoteMeta(generateName) + "[a-zA-Z0-9\\-.]+$") if !r.MatchString(name) { return fmt.Errorf("expected generated name to match \"%v\", got %s", r, name) From ed7d4a0b6ac9e1b4e4c4beb37d1aa6b3725d8b75 Mon Sep 17 00:00:00 2001 From: Brandon Mabey Date: Fri, 22 Feb 2019 16:51:42 -0500 Subject: [PATCH 4/5] Change IsStatusOk() -> IsStatusOk in generateName conformance tests. --- test/conformance/generatename_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/conformance/generatename_test.go b/test/conformance/generatename_test.go index 71156b630797..24675faf182f 100644 --- a/test/conformance/generatename_test.go +++ b/test/conformance/generatename_test.go @@ -95,7 +95,7 @@ func canServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1.Route clients.KubeClient, t.Logf, domain, - pkgTest.Retrying(pkgTest.MatchesAllOf(pkgTest.IsStatusOK(), pkgTest.MatchesBody(helloWorldText)), http.StatusNotFound), + pkgTest.Retrying(pkgTest.MatchesAllOf(pkgTest.IsStatusOK, pkgTest.MatchesBody(helloWorldText)), http.StatusNotFound), "WaitForEndpointToServeText", test.ServingFlags.ResolvableDomain) if err != nil { From 8e48797c4a51e121f7cff5ba504e781a853c0cc5 Mon Sep 17 00:00:00 2001 From: Brandon Mabey Date: Tue, 19 Mar 2019 13:44:09 -0400 Subject: [PATCH 5/5] Cleanup error messages to match Go style more closely. --- test/conformance/generatename_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/conformance/generatename_test.go b/test/conformance/generatename_test.go index 24675faf182f..24130e8ea4b0 100644 --- a/test/conformance/generatename_test.go +++ b/test/conformance/generatename_test.go @@ -69,7 +69,7 @@ func validateName(generateName, name string) error { r := regexp.MustCompile("^" + regexp.QuoteMeta(generateName) + "[a-zA-Z0-9\\-.]+$") if !r.MatchString(name) { - return fmt.Errorf("expected generated name to match \"%v\", got %s", r, name) + return fmt.Errorf("generated name = %q, want to match %q", name, r.String()) } return nil } @@ -99,7 +99,7 @@ func canServeRequests(t *testing.T, clients *test.Clients, route *v1alpha1.Route "WaitForEndpointToServeText", test.ServingFlags.ResolvableDomain) if err != nil { - return fmt.Errorf("the endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", route.Name, domain, helloWorldText, err) + return fmt.Errorf("the endpoint for Route %s at domain %s didn't serve the expected text %q: %v", route.Name, domain, helloWorldText, err) } return nil