From 9aedab861404856b41742ab73a21043eefe3e004 Mon Sep 17 00:00:00 2001 From: Ewe Zi Yi <36802364+deadlycoconuts@users.noreply.github.com> Date: Mon, 19 Sep 2022 10:59:31 +0800 Subject: [PATCH] Apply new Standard Ensembler routing policy with Custom Experiment Engines (#237) * Add route_name_path to standard ensembler config * Update ensembler struct in test data to reflect latest schema * Add additional validation tests to ensure that experiment mappings and route name path are not both set * Add additional test case to verify the route name path is valid * Refactor request parsing test * Remove redundant regex route path name check * Update regex expression to allow empty route_name_path string * Remove required tag from EnsemblerStandardConfig.RouteNamePath * Add e2e tests for std ensemblers with route_name_path * Add additional conditional to e2e tests to check for ensembler pods only when docker/pyfunc types are used * Fix e2e test data * Add positive test to verify valid router config creation * Fix router api test data * Add additional unit tests for router service creation with std ensemblers with route name path * Replace deprecated ioutil functions with corresponding io or os ones * Fix lint comments on indenting conditional statements * Fix e2e tests after changes from rebasing * Fix bug in e2e test for std ensembler with route name path * Fix sdk e2e tests * Remove experiment mappings as a required field and update regex for route name path * Add custom validator for standard ensembler config * Remove standard ensembler config schema checks from request verification layer * Make standard ensembler only parse non nil configs to fiber config * Remove unnecessary route name path field in json request * Remove duplicated error messages from EnsemblerStandardConfig custom validation checks * Remove regex expression for route_name_path --- api/api/openapi.bundle.yaml | 9 +- api/api/specs/routers.yaml | 4 +- api/e2e/test/00_all_e2e_test.go | 4 + api/e2e/test/01_create_router_test.go | 35 +++--- api/e2e/test/02_update_router_invalid_test.go | 3 +- api/e2e/test/03_create_router_version_test.go | 29 ++--- api/e2e/test/04_undeploy_router_test.go | 4 +- api/e2e/test/06_deploy_valid_config_test.go | 4 +- ...with_route_name_path_std_ensembler_test.go | 55 +++++++++ api/e2e/test/helpers_api_test.go | 15 +-- api/e2e/test/helpers_test.go | 11 +- ...er_std_ensembler_route_name_path.json.tmpl | 73 +++++++++++ api/turing/cluster/servicebuilder/router.go | 8 +- .../cluster/servicebuilder/router_test.go | 110 ++++++++++++++++- api/turing/internal/testutils/utils.go | 4 +- .../middleware/openapi_validation_test.go | 6 +- api/turing/models/ensembler.go | 3 +- api/turing/service/alert_service_test.go | 4 +- api/turing/service/mlp_service_test.go | 5 +- api/turing/service/pod_log_service_test.go | 14 +-- ...igmap_std_ensembler_with_exp_mappings.yml} | 0 ...map_std_ensembler_with_route_name_path.yml | 34 ++++++ .../router_version_missing_bigquery.json | 43 ++++--- ...cess_std_ensembler_with_exp_mappings.json} | 0 ...ss_std_ensembler_with_route_name_path.json | 114 ++++++++++++++++++ api/turing/utils/merge.go | 3 +- api/turing/validation/validator.go | 17 +++ api/turing/validation/validator_test.go | 78 ++++++++++++ infra/e2e/turing.values.yaml | 2 + sdk/e2e/01_create_router_test.py | 10 +- sdk/e2e/02_update_router_invalid_test.py | 2 +- sdk/e2e/03_create_router_version_test.py | 2 +- 32 files changed, 604 insertions(+), 101 deletions(-) create mode 100644 api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go create mode 100644 api/e2e/test/testdata/create_router_std_ensembler_route_name_path.json.tmpl rename api/turing/testdata/cluster/servicebuilder/{router_configmap_standard_ensembler.yml => router_configmap_std_ensembler_with_exp_mappings.yml} (100%) create mode 100644 api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_route_name_path.yml rename api/turing/testdata/cluster/servicebuilder/{router_version_success_standard_ensembler.json => router_version_success_std_ensembler_with_exp_mappings.json} (100%) create mode 100644 api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_route_name_path.json diff --git a/api/api/openapi.bundle.yaml b/api/api/openapi.bundle.yaml index 05ae12eb8..57201d662 100644 --- a/api/api/openapi.bundle.yaml +++ b/api/api/openapi.bundle.yaml @@ -1980,6 +1980,7 @@ components: timeout: timeout updated_at: 2000-01-23T04:56:07.000+00:00 standard_config: + route_name_path: route_name_path experiment_mappings: - treatment: treatment-1 route: route-1 @@ -2275,6 +2276,7 @@ components: timeout: timeout updated_at: 2000-01-23T04:56:07.000+00:00 standard_config: + route_name_path: route_name_path experiment_mappings: - treatment: treatment-1 route: route-1 @@ -2335,6 +2337,7 @@ components: EnsemblerStandardConfig: description: ensembler config when ensembler type is standard example: + route_name_path: route_name_path experiment_mappings: - treatment: treatment-1 route: route-1 @@ -2348,8 +2351,8 @@ components: items: $ref: '#/components/schemas/EnsemblerStandardConfig_experiment_mappings' type: array - required: - - experiment_mappings + route_name_path: + type: string type: object EnsemblerDockerConfig: description: ensembler config when ensembler type is docker @@ -2626,6 +2629,7 @@ components: timeout: timeout updated_at: 2000-01-23T04:56:07.000+00:00 standard_config: + route_name_path: route_name_path experiment_mappings: - treatment: treatment-1 route: route-1 @@ -2781,6 +2785,7 @@ components: timeout: timeout updated_at: 2000-01-23T04:56:07.000+00:00 standard_config: + route_name_path: route_name_path experiment_mappings: - treatment: treatment-1 route: route-1 diff --git a/api/api/specs/routers.yaml b/api/api/specs/routers.yaml index 0b496bde2..f5600d3af 100644 --- a/api/api/specs/routers.yaml +++ b/api/api/specs/routers.yaml @@ -794,8 +794,6 @@ components: description: "ensembler config when ensembler type is standard" type: "object" nullable: true - required: - - "experiment_mappings" properties: experiment_mappings: type: "array" @@ -818,6 +816,8 @@ components: description: "route id of the routes configured in the router" type: "string" example: "route-1" + route_name_path: + type: "string" EnsemblerDockerConfig: description: "ensembler config when ensembler type is docker" diff --git a/api/e2e/test/00_all_e2e_test.go b/api/e2e/test/00_all_e2e_test.go index 1fcbb3be8..26b7ab1ac 100644 --- a/api/e2e/test/00_all_e2e_test.go +++ b/api/e2e/test/00_all_e2e_test.go @@ -79,6 +79,10 @@ func TestEndToEnd(t *testing.T) { t.Parallel() t.Run("DeployRouter", TestDeployRouterWithTrafficRules) }) + t.Run("TestStandardEnsemblerWithRouteNamePath", func(t *testing.T) { + t.Parallel() + t.Run("DeployRouter", TestDeployRouterWithRouteNamePathInStandardEnsembler) + }) } func TestMain(m *testing.M) { diff --git a/api/e2e/test/01_create_router_test.go b/api/e2e/test/01_create_router_test.go index 8c0116ac8..a91a19097 100644 --- a/api/e2e/test/01_create_router_test.go +++ b/api/e2e/test/01_create_router_test.go @@ -49,20 +49,21 @@ func TestCreateRouter(t *testing.T) { response.StatusCode, string(responsePayload)) actualResponse := gjson.GetBytes(responsePayload, "response").String() expectedResponse := `{ - "experiment": { - "configuration": { - "foo":"bar" - } - }, - "route_responses": [ - { - "data": { - "version": "control" - }, - "is_default": false, - "route": "control" - } - ] + "experiment": { + "configuration": { + "foo":"bar", + "route_name":"treatment-a" + } + }, + "route_responses": [ + { + "data": { + "version": "control" + }, + "is_default": false, + "route": "control" + } + ] }` assert.JSONEq(t, expectedResponse, actualResponse) }) @@ -94,7 +95,8 @@ func TestCreateRouter(t *testing.T) { "response": { "experiment": { "configuration": { - "foo": "bar" + "foo": "bar", + "route_name":"treatment-a" } }, "route_responses": [ @@ -120,7 +122,8 @@ func TestCreateRouter(t *testing.T) { "response": { "experiment": { "configuration": { - "bar": "baz" + "bar": "baz", + "route_name":"control" } }, "route_responses": [ diff --git a/api/e2e/test/02_update_router_invalid_test.go b/api/e2e/test/02_update_router_invalid_test.go index 005790d12..b96bed8ba 100644 --- a/api/e2e/test/02_update_router_invalid_test.go +++ b/api/e2e/test/02_update_router_invalid_test.go @@ -145,7 +145,8 @@ func TestUpdateRouterInvalidConfig(t *testing.T) { expectedResponse := `{ "experiment": { "configuration": { - "foo":"bar" + "foo":"bar", + "route_name":"treatment-a" } }, "route_responses": [ diff --git a/api/e2e/test/03_create_router_version_test.go b/api/e2e/test/03_create_router_version_test.go index 2bf12c66d..47d22ae88 100644 --- a/api/e2e/test/03_create_router_version_test.go +++ b/api/e2e/test/03_create_router_version_test.go @@ -130,20 +130,21 @@ func TestCreateRouterVersion(t *testing.T) { response.StatusCode, string(responsePayload)) actualResponse := gjson.GetBytes(responsePayload, "response").String() expectedResponse := `{ - "experiment": { - "configuration": { - "foo":"bar" - } - }, - "route_responses": [ - { - "data": { - "version": "control" - }, - "is_default": false, - "route": "control" - } - ] + "experiment": { + "configuration": { + "foo":"bar", + "route_name":"treatment-a" + } + }, + "route_responses": [ + { + "data": { + "version": "control" + }, + "is_default": false, + "route": "control" + } + ] }` assert.JSONEq(t, expectedResponse, actualResponse) }) diff --git a/api/e2e/test/04_undeploy_router_test.go b/api/e2e/test/04_undeploy_router_test.go index d30af731b..bd6da1509 100644 --- a/api/e2e/test/04_undeploy_router_test.go +++ b/api/e2e/test/04_undeploy_router_test.go @@ -5,7 +5,7 @@ package e2e import ( "context" "fmt" - "io/ioutil" + "io" "net/http" "testing" "time" @@ -42,7 +42,7 @@ func TestUndeployRouter(t *testing.T) { response, err := globalTestContext.httpClient.Do(req) require.NoError(t, err) assert.Equal(t, http.StatusOK, response.StatusCode) - responseBody, err := ioutil.ReadAll(response.Body) + responseBody, err := io.ReadAll(response.Body) defer response.Body.Close() require.NoError(t, err) t.Log("Undeploy Response:", string(responseBody)) diff --git a/api/e2e/test/06_deploy_valid_config_test.go b/api/e2e/test/06_deploy_valid_config_test.go index f50e5dd2b..b354284c7 100644 --- a/api/e2e/test/06_deploy_valid_config_test.go +++ b/api/e2e/test/06_deploy_valid_config_test.go @@ -5,7 +5,7 @@ package e2e import ( "context" "fmt" - "io/ioutil" + "io" "net/http" "testing" @@ -42,7 +42,7 @@ func TestDeployValidConfig(t *testing.T) { response, err := globalTestContext.httpClient.Do(req) require.NoError(t, err) assert.Equal(t, http.StatusAccepted, response.StatusCode, readBody(t, response)) - responseBody, err := ioutil.ReadAll(response.Body) + responseBody, err := io.ReadAll(response.Body) require.NoError(t, err) t.Log("Deploy Response:", string(responseBody)) diff --git a/api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go b/api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go new file mode 100644 index 000000000..4ba2fc229 --- /dev/null +++ b/api/e2e/test/09_deploy_router_with_route_name_path_std_ensembler_test.go @@ -0,0 +1,55 @@ +//go:build e2e + +package e2e + +import ( + "net/http" + "path/filepath" + "testing" + + "github.com/caraml-dev/turing/api/turing/models" + "github.com/stretchr/testify/assert" +) + +/* +Steps: +Create a new router with valid config for the router. +a. Test GET router immediately > empty config +b. Wait for success response from deployment +c. Test GET router version > status shows "deployed" +d. Test GET router > config section shows version 1, status "deployed" +e. Test cluster that deployments exist +f. Make a request to the router, validate the response. +*/ +func TestDeployRouterWithRouteNamePathInStandardEnsembler(t *testing.T) { + // Create router + t.Log("Creating router") + data := makeRouterPayload( + filepath.Join("testdata", "create_router_std_ensembler_route_name_path.json.tmpl"), + globalTestContext) + + withDeployedRouter(t, data, + func(router *models.Router) { + t.Log("Testing router endpoint: POST " + router.Endpoint) + withRouterResponse(t, + http.MethodPost, + router.Endpoint, + http.Header{ + "Content-Type": {"application/json"}, + "X-Mirror-Body": {"true"}, + }, + `{"client": {"id": 4}}`, + func(response *http.Response, responsePayload []byte) { + assert.Equal(t, http.StatusOK, response.StatusCode, + "Unexpected response (code %d): %s", + response.StatusCode, string(responsePayload)) + actualResponse := string(responsePayload) + expectedResponse := `{ + "version" : "treatment-a" + }` + assert.JSONEq(t, expectedResponse, actualResponse) + }) + }, + nil, + ) +} diff --git a/api/e2e/test/helpers_api_test.go b/api/e2e/test/helpers_api_test.go index 119a9a990..36e501b37 100644 --- a/api/e2e/test/helpers_api_test.go +++ b/api/e2e/test/helpers_api_test.go @@ -6,7 +6,7 @@ import ( "bytes" "encoding/json" "fmt" - "io/ioutil" + "io" "net/http" "testing" "time" @@ -34,7 +34,7 @@ func getRouter( } defer resp.Body.Close() - respBytes, err := ioutil.ReadAll(resp.Body) + respBytes, err := io.ReadAll(resp.Body) if err != nil { return nil, err } @@ -61,7 +61,7 @@ func getRouterByName( defer resp.Body.Close() routers := make([]*models.Router, 0) - data, err := ioutil.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) if err != nil { return nil, err } @@ -96,7 +96,7 @@ func getRouterVersion( } defer resp.Body.Close() - respBytes, err := ioutil.ReadAll(resp.Body) + respBytes, err := io.ReadAll(resp.Body) if err != nil { return nil, err } @@ -143,7 +143,7 @@ func waitDeployVersion( } func getPodLogs(t *testing.T, resp *http.Response) []service.PodLog { - data, err := ioutil.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) if err != nil { t.Error(err) } @@ -175,7 +175,7 @@ func withDeployedRouter( require.NoError(t, err) defer resp.Body.Close() - responsePayload, err := ioutil.ReadAll(resp.Body) + responsePayload, err := io.ReadAll(resp.Body) require.NoError(t, err) created := models.Router{} @@ -275,7 +275,8 @@ func withDeployedRouter( globalTestContext.ProjectName, fmt.Sprintf("%s-turing-enricher-%d-0-deployment", router.Name, routerVersion.Version))) } - if router.CurrRouterVersion.Ensembler != nil { + if router.CurrRouterVersion.Ensembler != nil && + router.CurrRouterVersion.Ensembler.Type != models.EnsemblerStandardType { require.True(t, isDeploymentExists( globalTestContext.clusterClients, globalTestContext.ProjectName, diff --git a/api/e2e/test/helpers_test.go b/api/e2e/test/helpers_test.go index 6165a2193..8de4550c7 100644 --- a/api/e2e/test/helpers_test.go +++ b/api/e2e/test/helpers_test.go @@ -4,8 +4,9 @@ package e2e import ( "bytes" - "io/ioutil" + "io" "net/http" + "os" "testing" "text/template" @@ -16,7 +17,7 @@ func readBody(t *testing.T, resp *http.Response) string { if resp.Body == nil { return "" } - data, err := ioutil.ReadAll(resp.Body) + data, err := io.ReadAll(resp.Body) if err != nil { t.Fatal(err) } @@ -25,7 +26,7 @@ func readBody(t *testing.T, resp *http.Response) string { // make payload to create new router from a template file func makeRouterPayload(payloadTemplateFile string, args TestContext) []byte { - data, err := ioutil.ReadFile(payloadTemplateFile) + data, err := os.ReadFile(payloadTemplateFile) if err != nil { panic(err) } @@ -52,7 +53,7 @@ func withRouterResponse( body string, assertion func(response *http.Response, responsePayload []byte)) { - req, err := http.NewRequest(method, url, ioutil.NopCloser(bytes.NewReader([]byte(body)))) + req, err := http.NewRequest(method, url, io.NopCloser(bytes.NewReader([]byte(body)))) require.NoError(t, err) req.Header = headers @@ -61,7 +62,7 @@ func withRouterResponse( require.NoError(t, err) defer resp.Body.Close() - responseBytes, err := ioutil.ReadAll(resp.Body) + responseBytes, err := io.ReadAll(resp.Body) require.NoError(t, err) assertion(resp, responseBytes) diff --git a/api/e2e/test/testdata/create_router_std_ensembler_route_name_path.json.tmpl b/api/e2e/test/testdata/create_router_std_ensembler_route_name_path.json.tmpl new file mode 100644 index 000000000..dc031aa73 --- /dev/null +++ b/api/e2e/test/testdata/create_router_std_ensembler_route_name_path.json.tmpl @@ -0,0 +1,73 @@ +{ + "environment_name": "{{.DeploymentEnvironment}}", + "name": "e2e-std-ensembler-test-{{.TestID}}", + "config": { + "routes": [ + { + "id": "control", + "type": "PROXY", + "endpoint": "{{.MockserverEndpoint}}/control", + "timeout": "5s" + }, + { + "id": "treatment-a", + "type": "PROXY", + "endpoint": "{{.MockserverEndpoint}}/treatment-a", + "timeout": "5s" + } + ], + "experiment_engine": { + "type": "proprietary", + "config": { + "client": { + "id": "1", + "username": "test", + "passkey": "test" + }, + "experiments": [ + { + "id": "001", + "name": "exp_1" + } + ], + "variables": { + "experiment_variables": { + "001": [ + { + "name": "client_id", + "type": "unit", + "required": true + } + ] + }, + "config": [ + { + "name": "client_id", + "required": true, + "field": "client.id", + "field_source": "payload" + } + ] + } + } + }, + "resource_request": { + "min_replica": 1, + "max_replica": 1, + "cpu_request": "200m", + "memory_request": "250Mi" + }, + "timeout": "5s", + "log_config": { + "result_logger_type": "nop" + }, + "ensembler": { + "type": "standard", + "standard_config": { + "experiment_mappings": [], + "route_name_path": "route_name" + } + }, + "default_route_id": "control" + } +} diff --git a/api/turing/cluster/servicebuilder/router.go b/api/turing/cluster/servicebuilder/router.go index 166258cb8..66a202557 100644 --- a/api/turing/cluster/servicebuilder/router.go +++ b/api/turing/cluster/servicebuilder/router.go @@ -491,7 +491,13 @@ func buildFiberConfigMap( } if ver.Ensembler != nil && ver.Ensembler.Type == models.EnsemblerStandardType { - propsMap["experiment_mappings"] = ver.Ensembler.StandardConfig.ExperimentMappings + if ver.Ensembler.StandardConfig.ExperimentMappings != nil && + len(ver.Ensembler.StandardConfig.ExperimentMappings) != 0 { + propsMap["experiment_mappings"] = ver.Ensembler.StandardConfig.ExperimentMappings + } + if ver.Ensembler.StandardConfig.RouteNamePath != "" { + propsMap["route_name_path"] = ver.Ensembler.StandardConfig.RouteNamePath + } } properties, err := json.Marshal(propsMap) diff --git a/api/turing/cluster/servicebuilder/router_test.go b/api/turing/cluster/servicebuilder/router_test.go index 86d6aa76b..a88c95e78 100644 --- a/api/turing/cluster/servicebuilder/router_test.go +++ b/api/turing/cluster/servicebuilder/router_test.go @@ -46,7 +46,13 @@ func TestNewRouterService(t *testing.T) { require.NoError(t, err) cfgmapEnsembling, err := tu.ReadFile(filepath.Join(testDataBasePath, "router_configmap_ensembling.yml")) require.NoError(t, err) - cfgmapStandardEnsemble, err := tu.ReadFile(filepath.Join(testDataBasePath, "router_configmap_standard_ensembler.yml")) + cfgmapStdEnsemblerWithExpMappings, err := tu.ReadFile( + filepath.Join(testDataBasePath, "router_configmap_std_ensembler_with_exp_mappings.yml"), + ) + require.NoError(t, err) + cfgmapStdEnsemblerWithRouteNamePath, err := tu.ReadFile( + filepath.Join(testDataBasePath, "router_configmap_std_ensembler_with_route_name_path.yml"), + ) require.NoError(t, err) cfgmapTrafficSplitting, err := tu.ReadFile(filepath.Join(testDataBasePath, "router_configmap_traffic_splitting.yml")) require.NoError(t, err) @@ -257,8 +263,104 @@ func TestNewRouterService(t *testing.T) { UserContainerLimitRequestFactor: 1.5, }, }, - "success | standard ensembler": { - filePath: filepath.Join(testDataBasePath, "router_version_success_standard_ensembler.json"), + "success | standard ensembler with experiment mappings": { + filePath: filepath.Join(testDataBasePath, "router_version_success_std_ensembler_with_exp_mappings.json"), + expRawConfig: json.RawMessage(expRunnerConfig), + expected: &cluster.KnativeService{ + BaseService: &cluster.BaseService{ + Name: "test-svc-turing-router-1", + Namespace: "test-project", + Image: "asia.gcr.io/gcp-project-id/turing-router:latest", + CPURequests: resource.MustParse("400m"), + MemoryRequests: resource.MustParse("512Mi"), + LivenessHTTPGetPath: "/v1/internal/live", + ReadinessHTTPGetPath: "/v1/internal/ready", + ConfigMap: &cluster.ConfigMap{ + Name: "test-svc-turing-fiber-config-1", + FileName: "fiber.yml", + Data: string(cfgmapStdEnsemblerWithExpMappings), + Labels: map[string]string{ + "app": "test-svc", + "environment": "", + "orchestrator": "turing", + "stream": "test-stream", + "team": "test-team", + }, + }, + Envs: []corev1.EnvVar{ + {Name: "APP_NAME", Value: "test-svc-1.test-project"}, + {Name: "APP_ENVIRONMENT", Value: "test-env"}, + {Name: "ROUTER_TIMEOUT", Value: "5s"}, + {Name: "APP_JAEGER_COLLECTOR_ENDPOINT", Value: "jaeger-endpoint"}, + {Name: "ROUTER_CONFIG_FILE", Value: "/app/config/fiber.yml"}, + {Name: "APP_SENTRY_ENABLED", Value: "true"}, + {Name: "APP_SENTRY_DSN", Value: "sentry-dsn"}, + {Name: "APP_LOGLEVEL", Value: "INFO"}, + {Name: "APP_CUSTOM_METRICS", Value: "false"}, + {Name: "APP_JAEGER_ENABLED", Value: "false"}, + {Name: "APP_RESULT_LOGGER", Value: "bigquery"}, + {Name: "APP_FIBER_DEBUG_LOG", Value: "false"}, + {Name: "APP_GCP_PROJECT", Value: "gcp-project-id"}, + {Name: "APP_BQ_DATASET", Value: "dataset_id"}, + {Name: "APP_BQ_TABLE", Value: "turing_log_test"}, + {Name: "APP_BQ_BATCH_LOAD", Value: "false"}, + {Name: "GOOGLE_APPLICATION_CREDENTIALS", Value: "/var/secret/router-service-account.json"}, + }, + Labels: map[string]string{ + "app": "test-svc", + "environment": "", + "orchestrator": "turing", + "stream": "test-stream", + "team": "test-team", + }, + Volumes: []corev1.Volume{ + { + Name: routerConfigMapVolume, + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: "test-svc-turing-fiber-config-1", + }, + }, + }, + }, + { + Name: secretVolume, + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: "service-account", + Items: []corev1.KeyToPath{ + { + Key: secretKeyNameRouter, + Path: secretKeyNameRouter, + }, + }, + }, + }, + }, + }, + VolumeMounts: []corev1.VolumeMount{ + { + Name: routerConfigMapVolume, + MountPath: routerConfigMapMountPath, + }, + { + Name: secretVolume, + MountPath: secretMountPath, + }, + }, + }, + ContainerPort: 8080, + MinReplicas: 2, + MaxReplicas: 4, + AutoscalingMetric: "rps", + AutoscalingTarget: "100", + QueueProxyResourcePercentage: 20, + UserContainerLimitRequestFactor: 1.5, + }, + }, + "success | standard ensembler with route name path": { + filePath: filepath.Join(testDataBasePath, "router_version_success_std_ensembler_with_route_name_path.json"), expRawConfig: json.RawMessage(expRunnerConfig), expected: &cluster.KnativeService{ BaseService: &cluster.BaseService{ @@ -272,7 +374,7 @@ func TestNewRouterService(t *testing.T) { ConfigMap: &cluster.ConfigMap{ Name: "test-svc-turing-fiber-config-1", FileName: "fiber.yml", - Data: string(cfgmapStandardEnsemble), + Data: string(cfgmapStdEnsemblerWithRouteNamePath), Labels: map[string]string{ "app": "test-svc", "environment": "", diff --git a/api/turing/internal/testutils/utils.go b/api/turing/internal/testutils/utils.go index e22ce2100..d4b3f0bd6 100644 --- a/api/turing/internal/testutils/utils.go +++ b/api/turing/internal/testutils/utils.go @@ -2,7 +2,7 @@ package testutils import ( "encoding/json" - "io/ioutil" + "io" "os" "testing" @@ -19,7 +19,7 @@ func ReadFile(filepath string) ([]byte, error) { } defer fileObj.Close() // Read contents - byteValue, err := ioutil.ReadAll(fileObj) + byteValue, err := io.ReadAll(fileObj) if err != nil { return nil, err } diff --git a/api/turing/middleware/openapi_validation_test.go b/api/turing/middleware/openapi_validation_test.go index 107839b6a..aeaf2cdcf 100644 --- a/api/turing/middleware/openapi_validation_test.go +++ b/api/turing/middleware/openapi_validation_test.go @@ -1,8 +1,8 @@ package middleware import ( - "io/ioutil" "net/http" + "os" "strings" "testing" ) @@ -136,7 +136,7 @@ func TestOpenAPIValidationValidate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d, err := ioutil.ReadFile("../../api/openapi.bundle.yaml") + d, err := os.ReadFile("../../api/openapi.bundle.yaml") if err != nil { t.Error(err) } @@ -189,7 +189,7 @@ func TestNewOpenAPIValidation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - d, err := ioutil.ReadFile(tt.openapiYamlFile) + d, err := os.ReadFile(tt.openapiYamlFile) if err != nil { t.Error(err) } diff --git a/api/turing/models/ensembler.go b/api/turing/models/ensembler.go index 4f4a4960e..0418d0f19 100644 --- a/api/turing/models/ensembler.go +++ b/api/turing/models/ensembler.go @@ -32,7 +32,8 @@ const ( ) type EnsemblerStandardConfig struct { - ExperimentMappings []ExperimentMapping `json:"experiment_mappings" validate:"required,dive"` + ExperimentMappings []ExperimentMapping `json:"experiment_mappings" validate:"dive"` + RouteNamePath string `json:"route_name_path"` } type EnsemblerDockerConfig struct { diff --git a/api/turing/service/alert_service_test.go b/api/turing/service/alert_service_test.go index 325ec1ae3..8fd4a45a7 100644 --- a/api/turing/service/alert_service_test.go +++ b/api/turing/service/alert_service_test.go @@ -3,7 +3,7 @@ package service import ( "errors" "fmt" - "io/ioutil" + "io" "net/http" "net/http/httptest" "strings" @@ -519,7 +519,7 @@ func newMockGitlabServer() (mockGitlab *httptest.Server, requestRecords map[stri records := make(map[string]string) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { record := fmt.Sprintf("%s %s", r.Method, r.URL.Path) - body, err := ioutil.ReadAll(r.Body) + body, err := io.ReadAll(r.Body) if body != nil && err == nil { records[record] = string(body) } else { diff --git a/api/turing/service/mlp_service_test.go b/api/turing/service/mlp_service_test.go index 2097b75b5..e39afc7a2 100644 --- a/api/turing/service/mlp_service_test.go +++ b/api/turing/service/mlp_service_test.go @@ -2,7 +2,6 @@ package service import ( "context" - "io/ioutil" "net/http" "os" "reflect" @@ -48,12 +47,12 @@ func testSetupEnvForGoogleCredentials(t *testing.T) (reset func()) { "token_uri": "https://oauth2.googleapis.com/token" }`) - file, err := ioutil.TempFile("", "dummy-service-account") + file, err := os.CreateTemp("", "dummy-service-account") if err != nil { t.Fatal(err) } - err = ioutil.WriteFile(file.Name(), serviceAccountKey, 0644) + err = os.WriteFile(file.Name(), serviceAccountKey, 0644) if err != nil { t.Fatal(err) } diff --git a/api/turing/service/pod_log_service_test.go b/api/turing/service/pod_log_service_test.go index 0a1bbf712..c4249a8de 100644 --- a/api/turing/service/pod_log_service_test.go +++ b/api/turing/service/pod_log_service_test.go @@ -3,7 +3,7 @@ package service import ( "bytes" "errors" - "io/ioutil" + "io" "testing" "time" @@ -119,24 +119,24 @@ func TestPodLogServiceListPodLogs(t *testing.T) { controller. On("ListPodLogs", mock.Anything, "namespace", "json-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true, SinceTime: &sinceTimeV1Minus1Sec}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T06:59:59Z {"foo":"bar", "baz": 5} + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T06:59:59Z {"foo":"bar", "baz": 5} 2020-07-07T07:00:05Z {"foo":"bar", "baz": 5} 2020-07-07T07:00:10Z {"foo":"bar", "baz": 10}`)), nil) controller. On("ListPodLogs", mock.Anything, "namespace", "json-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T06:59:59Z {"foo":"bar", "baz": 5} + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T06:59:59Z {"foo":"bar", "baz": 5} 2020-07-07T07:00:05Z {"foo":"bar", "baz": 5} 2020-07-07T07:00:10Z {"foo":"bar", "baz": 10}`)), nil) controller. On("ListPodLogs", mock.Anything, "namespace", "json-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true, TailLines: &tailLines}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T07:00:05Z {"foo":"bar", "baz": 5} + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T07:00:05Z {"foo":"bar", "baz": 5} 2020-07-07T07:00:10Z {"foo":"bar", "baz": 10}`)), nil) controller. On("ListPodLogs", mock.Anything, "namespace", "text-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true, SinceTime: &sinceTimeV1Minus1Sec}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 2020-07-07T08:00:10Z line2 invalidtimestamp line3 @@ -144,7 +144,7 @@ invalidtimestamp line3 controller. On("ListPodLogs", mock.Anything, "namespace", "text-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true, TailLines: &tailLines}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 2020-07-07T08:00:10Z line2 invalidtimestamp line3 @@ -152,7 +152,7 @@ invalidtimestamp line3 controller. On("ListPodLogs", mock.Anything, "namespace", "text-payload", &corev1.PodLogOptions{Container: "user-container", Timestamps: true}). - Return(ioutil.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 + Return(io.NopCloser(bytes.NewBufferString(`2020-07-07T08:00:05Z line1 2020-07-07T08:00:10Z line2 invalidtimestamp line3 diff --git a/api/turing/testdata/cluster/servicebuilder/router_configmap_standard_ensembler.yml b/api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_exp_mappings.yml similarity index 100% rename from api/turing/testdata/cluster/servicebuilder/router_configmap_standard_ensembler.yml rename to api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_exp_mappings.yml diff --git a/api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_route_name_path.yml b/api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_route_name_path.yml new file mode 100644 index 000000000..13765275a --- /dev/null +++ b/api/turing/testdata/cluster/servicebuilder/router_configmap_std_ensembler_with_route_name_path.yml @@ -0,0 +1,34 @@ +id: test-svc +routes: +- endpoint: http://example.com/treatment-b + id: treatment-b + timeout: 2s + type: PROXY +- endpoint: http://example.com/treatment-a + id: treatment-a + timeout: 2s + type: PROXY +- endpoint: http://www.mocky.io/v2/5e4caccc310000e2cad8c071 + id: control + timeout: 2s + type: PROXY +strategy: + properties: + default_route_id: control + experiment_engine: exp-engine-2 + experiment_engine_properties: + client_id: client_id + endpoint: exp-engine:8080 + experiments: + - experiment_name: exp_exp_test_experiment_1 + segmentation_field: customer_id + segmentation_field_source: payload + segmentation_unit: customer + timeout: 500ms + user_data: + app_version: + field: appVer + field_source: header + route_name_path: policy.route_name + type: fiber.DefaultTuringRoutingStrategy +type: EAGER_ROUTER diff --git a/api/turing/testdata/cluster/servicebuilder/router_version_missing_bigquery.json b/api/turing/testdata/cluster/servicebuilder/router_version_missing_bigquery.json index e3c2e97ff..470892147 100644 --- a/api/turing/testdata/cluster/servicebuilder/router_version_missing_bigquery.json +++ b/api/turing/testdata/cluster/servicebuilder/router_version_missing_bigquery.json @@ -62,25 +62,28 @@ }, "ensembler": { "id": 300, - "image": "asia.gcr.io/gcp-project-id/echo:1.0.2", - "resource_request": { - "min_replica": 1, - "max_replica": 2, - "cpu_request": "200m", - "memory_request": "256Mi" - }, - "autoscaling_policy": { - "metric": "memory", - "target": "90" - }, - "endpoint": "echo?delay=10ms", - "timeout": "2s", - "port": 8080, - "env": [ - { - "name": "TEST_ENV", - "value": "ensembler" - } - ] + "type": "docker", + "docker_config": { + "image": "asia.gcr.io/gcp-project-id/echo:1.0.2", + "resource_request": { + "min_replica": 1, + "max_replica": 2, + "cpu_request": "200m", + "memory_request": "256Mi" + }, + "autoscaling_policy": { + "metric": "memory", + "target": "90" + }, + "endpoint": "echo?delay=10ms", + "timeout": "2s", + "port": 8080, + "env": [ + { + "name": "TEST_ENV", + "value": "ensembler" + } + ] + } } } \ No newline at end of file diff --git a/api/turing/testdata/cluster/servicebuilder/router_version_success_standard_ensembler.json b/api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_exp_mappings.json similarity index 100% rename from api/turing/testdata/cluster/servicebuilder/router_version_success_standard_ensembler.json rename to api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_exp_mappings.json diff --git a/api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_route_name_path.json b/api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_route_name_path.json new file mode 100644 index 000000000..d17282b44 --- /dev/null +++ b/api/turing/testdata/cluster/servicebuilder/router_version_success_std_ensembler_with_route_name_path.json @@ -0,0 +1,114 @@ +{ + "router": { + "project_id": 10, + "environment_name": "id-dev", + "name": "test-svc" + }, + "version": 1, + "status": "pending", + "image": "asia.gcr.io/gcp-project-id/turing-router:latest", + "routes": [ + { + "id": "treatment-b", + "type": "PROXY", + "endpoint": "http://example.com/treatment-b", + "timeout": "2s" + }, + { + "id": "treatment-a", + "type": "PROXY", + "endpoint": "http://example.com/treatment-a", + "timeout": "2s" + }, + { + "id": "control", + "type": "PROXY", + "endpoint": "http://www.mocky.io/v2/5e4caccc310000e2cad8c071", + "timeout": "2s" + } + ], + "default_route_id": "control", + "experiment_engine": { + "type": "exp-engine-2", + "config": { + "deployment": { + "endpoint": "exp-engine:8080", + "timeout": "500ms" + }, + "client": { + "id": "1", + "username": "client_id", + "passkey": "xyz" + }, + "experiments": [ + { + "id": "2", + "name": "exp_exp_test_experiment_1", + "client_id": "1" + } + ], + "variables": { + "client_variables": [ + { + "name": "app_version", + "required": false, + "type": "filter" + } + ], + "experiment_variables": { + "2": [ + { + "name": "customer", + "required": true, + "type": "unit" + } + ] + }, + "config": [ + { + "name": "customer", + "required": true, + "field": "customer_id", + "field_source": "payload" + }, + { + "name": "app_version", + "required": false, + "field": "appVer", + "field_source": "header" + } + ] + } + } + }, + "resource_request": { + "min_replica": 2, + "max_replica": 4, + "cpu_request": "400m", + "memory_request": "512Mi" + }, + "autoscaling_policy": { + "metric": "rps", + "target": "100" + }, + "timeout": "5s", + "log_config": { + "log_level": "INFO", + "custom_metrics_enabled": false, + "fiber_debug_log_enabled": false, + "jaeger_enabled": false, + "result_logger_type": "bigquery", + "bigquery_config": { + "table": "gcp-project-id.dataset_id.turing_log_test", + "service_account_name": "test-svc-account", + "batch_load": false + } + }, + "ensembler": { + "id": 300, + "type": "standard", + "standard_config": { + "route_name_path": "policy.route_name" + } + } +} diff --git a/api/turing/utils/merge.go b/api/turing/utils/merge.go index 2ae902f6d..0f132871c 100644 --- a/api/turing/utils/merge.go +++ b/api/turing/utils/merge.go @@ -3,7 +3,6 @@ package utils import ( "bytes" "fmt" - "io/ioutil" "os" "gopkg.in/yaml.v3" @@ -83,7 +82,7 @@ func MergeMaps(originalMap, override map[string]interface{}) (map[string]interfa } func readYAML(filepath string) (map[string]interface{}, error) { - file, err := ioutil.ReadFile(filepath) + file, err := os.ReadFile(filepath) if err != nil { return nil, err } diff --git a/api/turing/validation/validator.go b/api/turing/validation/validator.go index ef584d220..45fcf07d7 100644 --- a/api/turing/validation/validator.go +++ b/api/turing/validation/validator.go @@ -33,6 +33,9 @@ func NewValidator(expSvc service.ExperimentsService) (*validator.Validate, error request.ExperimentEngineConfig{}, ) } + + instance.RegisterStructValidation(validateEnsemblerStandardConfig, models.EnsemblerStandardConfig{}) + instance.RegisterStructValidation(validateRouterConfig, request.RouterConfig{}) // register common.RuleConditionOperator type to use its String representation for validation @@ -46,6 +49,20 @@ func NewValidator(expSvc service.ExperimentsService) (*validator.Validate, error return instance, nil } +func validateEnsemblerStandardConfig(sl validator.StructLevel) { + ensemblerStandardConfig := sl.Current().Interface().(models.EnsemblerStandardConfig) + // Verify that the ExperimentMappings and RouteNamePath are not both empty at the same time + if (len(ensemblerStandardConfig.ExperimentMappings) == 0) && ensemblerStandardConfig.RouteNamePath == "" { + sl.ReportError(ensemblerStandardConfig.ExperimentMappings, + "ExperimentMappings", "ExperimentMappings", "required when RouteNamePath is not set", "") + } + // Verify that the ExperimentMappings and RouteNamePath are not both set at the same time + if len(ensemblerStandardConfig.ExperimentMappings) > 0 && ensemblerStandardConfig.RouteNamePath != "" { + sl.ReportError(ensemblerStandardConfig.ExperimentMappings, + "ExperimentMappings", "ExperimentMappings", "excluded when RouteNamePath is set", "") + } +} + func validateLogConfig(sl validator.StructLevel) { field := sl.Current().Interface().(request.LogConfig) switch field.ResultLoggerType { diff --git a/api/turing/validation/validator_test.go b/api/turing/validation/validator_test.go index 2c0c58601..51cf31b35 100644 --- a/api/turing/validation/validator_test.go +++ b/api/turing/validation/validator_test.go @@ -20,6 +20,84 @@ import ( "github.com/stretchr/testify/require" ) +func TestValidateEnsemblerStandardConfig(t *testing.T) { + tt := map[string]struct { + input models.EnsemblerStandardConfig + err string + }{ + "failure | experiment mappings and route name path undefined": { + input: models.EnsemblerStandardConfig{}, + err: "Key: 'EnsemblerStandardConfig.ExperimentMappings' Error:Field validation for 'ExperimentMappings' " + + "failed on the 'required when RouteNamePath is not set' tag", + }, + "failure | experiment mappings and route name path empty": { + input: models.EnsemblerStandardConfig{ + ExperimentMappings: []models.ExperimentMapping{}, + RouteNamePath: "", + }, + err: "Key: 'EnsemblerStandardConfig.ExperimentMappings' Error:Field validation for 'ExperimentMappings' " + + "failed on the 'required when RouteNamePath is not set' tag", + }, + "failure | experiment mappings empty and route name path undefined": { + input: models.EnsemblerStandardConfig{ + ExperimentMappings: []models.ExperimentMapping{}, + }, + err: "Key: 'EnsemblerStandardConfig.ExperimentMappings' Error:Field validation for 'ExperimentMappings' " + + "failed on the 'required when RouteNamePath is not set' tag", + }, + "failure | experiment mappings undefined and route name path empty": { + input: models.EnsemblerStandardConfig{ + RouteNamePath: "", + }, + err: "Key: 'EnsemblerStandardConfig.ExperimentMappings' Error:Field validation for 'ExperimentMappings' " + + "failed on the 'required when RouteNamePath is not set' tag", + }, + "failure | experiment mappings and route name path defined": { + input: models.EnsemblerStandardConfig{ + ExperimentMappings: []models.ExperimentMapping{ + { + Experiment: "experiment-1", + Treatment: "treatment-1", + Route: "route-1", + }, + }, + RouteNamePath: "route-1", + }, + err: "Key: 'EnsemblerStandardConfig.ExperimentMappings' Error:Field validation for 'ExperimentMappings' " + + "failed on the 'excluded when RouteNamePath is set' tag", + }, + "success | only experiment mappings defined": { + input: models.EnsemblerStandardConfig{ + ExperimentMappings: []models.ExperimentMapping{ + { + Experiment: "experiment-1", + Treatment: "treatment-1", + Route: "route-1", + }, + }, + }, + }, + "success | only route name path defined": { + input: models.EnsemblerStandardConfig{ + RouteNamePath: "route-1", + }, + }, + } + + for name, tc := range tt { + t.Run(name, func(t *testing.T) { + validate, err := validation.NewValidator(nil) + assert.NoError(t, err) + err = validate.Struct(tc.input) + if tc.err == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tc.err) + } + }) + } +} + func TestValidateLogConfig(t *testing.T) { tt := map[string]struct { input request.LogConfig diff --git a/infra/e2e/turing.values.yaml b/infra/e2e/turing.values.yaml index b8ab47441..5c139ebe3 100644 --- a/infra/e2e/turing.values.yaml +++ b/infra/e2e/turing.values.yaml @@ -37,10 +37,12 @@ turing: traffic: 0.85 treatment_configuration: foo: bar + route_name: treatment-a treatment-1: traffic: 0.15 treatment_configuration: bar: baz + route_name: control postgresql: &postgresql persistence: diff --git a/sdk/e2e/01_create_router_test.py b/sdk/e2e/01_create_router_test.py index f6da7c468..903a3e4a8 100644 --- a/sdk/e2e/01_create_router_test.py +++ b/sdk/e2e/01_create_router_test.py @@ -137,7 +137,7 @@ def test_create_router(): ) assert response.status_code == 200 expected_response = { - "experiment": {"configuration": {"foo": "bar"}}, + "experiment": {"configuration": {"foo": "bar", "route_name": "treatment-a"}}, "route_responses": [ {"data": {"version": "control"}, "is_default": False, "route": "control"} ], @@ -161,7 +161,9 @@ def test_create_router(): "data": { "request": {"client": {"id": 4}}, "response": { - "experiment": {"configuration": {"foo": "bar"}}, + "experiment": { + "configuration": {"foo": "bar", "route_name": "treatment-a"} + }, "route_responses": [ { "data": {"version": "control"}, @@ -177,7 +179,9 @@ def test_create_router(): "data": { "request": {"client": {"id": 7}}, "response": { - "experiment": {"configuration": {"bar": "baz"}}, + "experiment": { + "configuration": {"bar": "baz", "route_name": "control"} + }, "route_responses": [ { "data": {"version": "control"}, diff --git a/sdk/e2e/02_update_router_invalid_test.py b/sdk/e2e/02_update_router_invalid_test.py index fc959bd36..13a5919b3 100644 --- a/sdk/e2e/02_update_router_invalid_test.py +++ b/sdk/e2e/02_update_router_invalid_test.py @@ -75,7 +75,7 @@ def test_update_router_invalid_config(): ) assert response.status_code == 200 expected_response = { - "experiment": {"configuration": {"foo": "bar"}}, + "experiment": {"configuration": {"foo": "bar", "route_name": "treatment-a"}}, "route_responses": [ {"data": {"version": "control"}, "is_default": False, "route": "control"} ], diff --git a/sdk/e2e/03_create_router_version_test.py b/sdk/e2e/03_create_router_version_test.py index 987f4ddea..b3110bba9 100644 --- a/sdk/e2e/03_create_router_version_test.py +++ b/sdk/e2e/03_create_router_version_test.py @@ -92,7 +92,7 @@ def test_create_router_version(): ) assert response.status_code == 200 expected_response = { - "experiment": {"configuration": {"foo": "bar"}}, + "experiment": {"configuration": {"foo": "bar", "route_name": "treatment-a"}}, "route_responses": [ {"data": {"version": "control"}, "is_default": False, "route": "control"} ],