Bugfix: Turing API should process experiment engine passkey only if client selection enabled

api/e2e/test/helpers_api_test.go

@@ -41,7 +41,7 @@ func getRouter(
 
 	err = json.Unmarshal(respBytes, &router)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("Could not unmarshal: %v\n %s", err, string(respBytes))
 	}
 
 	return &router, err

api/go.sum

@@ -198,9 +198,8 @@ github.com/aws/aws-sdk-go v1.27.1/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN
 github.com/aws/aws-sdk-go v1.28.0/go.mod h1:KmX6BPdI08NWTb3/sm4ZGu5ShLoqVDhKgpiN924inxo=
 github.com/aws/aws-sdk-go v1.29.11/go.mod h1:1KvfttTE3SPKMpo8g2c6jL3ZKfXtFvKscTgahTma5Xg=
 github.com/aws/aws-sdk-go v1.29.32/go.mod h1:1KvfttTE3SPKMpo8g2c6jL3ZKfXtFvKscTgahTma5Xg=
+github.com/aws/aws-sdk-go v1.29.34 h1:yrzwfDaZFe9oT4AmQeNNunSQA7c0m2chz0B43+bJ1ok=
 github.com/aws/aws-sdk-go v1.29.34/go.mod h1:1KvfttTE3SPKMpo8g2c6jL3ZKfXtFvKscTgahTma5Xg=
-github.com/aws/aws-sdk-go v1.43.44 h1:t+97cY4ScE/czlNlK5iikUGi7w1fC0uop1OUalDIRT4=
-github.com/aws/aws-sdk-go v1.43.44/go.mod h1:y4AeaBuwd2Lk+GepC1E9v0qOiTws0MIWAX4oIKwKHZo=
 github.com/aws/aws-xray-sdk-go v1.0.0-rc.5/go.mod h1:XtMKdBQfpVut+tJEwI7+dJFRxxRdxHDyVNp2tHXRq04=
 github.com/bazelbuild/bazel-gazelle v0.0.0-20181012220611-c728ce9f663e/go.mod h1:uHBSeeATKpVazAACZBDPL/Nk/UhQDDsJWDlqYJo8/Us=
 github.com/bazelbuild/buildtools v0.0.0-20180226164855-80c7f0d45d7e/go.mod h1:5JP0TXzWDHXv8qvxRC4InIazwdyDseBDbzESUMKk1yU=
@@ -861,11 +860,8 @@ github.com/jinzhu/now v1.1.1/go.mod h1:d3SSVoowX0Lcu0IBviAWJpolVfI5UJVZZ7cO71lE/
 github.com/jmespath/go-jmespath v0.0.0-20160202185014-0b12d6b521d8/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
 github.com/jmespath/go-jmespath v0.0.0-20160803190731-bd40a432e4c7/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
 github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af/go.mod h1:Nht3zPeWKUH0NzdCt2Blrr5ys8VGpn0CEB0cQHVjt7k=
+github.com/jmespath/go-jmespath v0.3.0 h1:OS12ieG61fsCg5+qLJ+SsW9NicxNkg3b25OyT2yCeUc=
 github.com/jmespath/go-jmespath v0.3.0/go.mod h1:9QtRXoHjLGCJ5IBSaohpXITPlowMeeYCZ7fLUTSywik=
-github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
-github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo=
-github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8=
-github.com/jmespath/go-jmespath/internal/testify v1.5.1/go.mod h1:L3OGu8Wl2/fWfCI6z80xFu9LTZmf1ZRjMHUOPmWr69U=
 github.com/jmoiron/sqlx v1.2.0/go.mod h1:1FEQNm3xlJgrMD+FBdI9+xvCksHtbpVBBw5dYhBSsks=
 github.com/joefitzgerald/rainbow-reporter v0.1.0/go.mod h1:481CNgqmVHQZzdIbN52CupLJyoVwB10FQ/IQlF1pdL8=
 github.com/joho/godotenv v1.3.0/go.mod h1:7hK45KPybAkOC6peb+G5yklZfMxEjkZhHbwpqxOKXbg=
@@ -1584,8 +1580,6 @@ golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v
 golang.org/x/net v0.0.0-20210405180319-a5a99cb37ef4/go.mod h1:p54w0d4576C0XHj96bSt6lcn1PtDYWL6XObtHCRCNQM=
 golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f h1:OfiFi4JbukWwe3lzw+xunroH1mnC1e2Gy5cxNJApiSY=
 golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
-golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd h1:O7DYs+zxREGLKzKoMQrtrEacpb0ZVXA5rIwylE2Xchk=
-golang.org/x/net v0.0.0-20220127200216-cd36cc0744dd/go.mod h1:CfG3xpIq0wQ8r1q4Su4UZFWDARRcnwPjda9FqA0JpMk=
 golang.org/x/oauth2 v0.0.0-20180603041954-1e0a3fa8ba9a/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20180724155351-3d292e4d0cdc/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
@@ -1697,13 +1691,10 @@ golang.org/x/sys v0.0.0-20210603081109-ebe580a85c40/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20210806184541-e5e7981a1069/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20211216021012-1d35b9e2eb4e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220209214540-3681064d5158 h1:rm+CHSpPEEW2IsXUib1ThaHIjuBVZjxNgSKmBLFfD4c=
 golang.org/x/sys v0.0.0-20220209214540-3681064d5158/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 h1:v+OssWQX+hTHEmOBgwxdZxK4zHq3yOs8F9J7mk0PY8E=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
-golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 h1:JGgROgKl9N8DuW20oFS5gxc+lE67/N3FcwmBPMe7ArY=
-golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/text v0.0.0-20160726164857-2910a502d2bf/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

api/turing/api/deployment_controller.go

@@ -192,7 +192,11 @@ func (c RouterDeploymentController) deployRouterVersion(
 	expSvc := c.BaseController.AppContext.ExperimentsService
 	if routerVersion.ExperimentEngine.Type != models.ExperimentEngineTypeNop {
 		experimentConfig = routerVersion.ExperimentEngine.Config
-		if expSvc.IsStandardExperimentManager(routerVersion.ExperimentEngine.Type) {
+		isClientSelectionEnabled, err := expSvc.IsClientSelectionEnabled(routerVersion.ExperimentEngine.Type)
+		if err != nil {
+			return "", c.updateRouterVersionStatusToFailed(err, routerVersion)
+		}
+		if isClientSelectionEnabled {
 			// Convert the config to the standard type
 			standardExperimentConfig, err := manager.ParseStandardExperimentConfig(experimentConfig)
 			if err != nil {

api/turing/api/deployment_controller_test.go

@@ -143,9 +143,9 @@ func TestDeployVersionSuccess(t *testing.T) {
 			exps := &mocks.ExperimentsService{}
 			exps.
 				On(
-					"IsStandardExperimentManager",
+					"IsClientSelectionEnabled",
 					data.routerVersion.ExperimentEngine.Type,
-				).Return(true)
+				).Return(true, nil)
 			exps.
 				On(
 					"GetExperimentRunnerConfig",
@@ -278,7 +278,7 @@ func TestRollbackVersionSuccess(t *testing.T) {
 	es.On("Save", mock.Anything).Return(nil)
 
 	exps := &mocks.ExperimentsService{}
-	exps.On("IsStandardExperimentManager", "nop").Return(false)
+	exps.On("IsClientSelectionEnabled", "nop").Return(false, nil)
 
 	// Create test controller
 	ctrl := RouterDeploymentController{

api/turing/api/request/request.go

@@ -206,9 +206,12 @@ func (r RouterConfig) BuildExperimentEngineConfig(
 	expSvc service.ExperimentsService,
 ) (json.RawMessage, error) {
 	rawExpConfig := r.ExperimentEngine.Config
-
-	// Handle missing passkey / encrypt it in Standard experiment config
-	if expSvc.IsStandardExperimentManager(r.ExperimentEngine.Type) {
+	// Handle missing passkey / encrypt it, if Standard experiment config using client selection
+	isClientSelectionEnabled, err := expSvc.IsClientSelectionEnabled(r.ExperimentEngine.Type)
+	if err != nil {
+		return nil, err
+	}
+	if isClientSelectionEnabled {
 		// Convert the new config to the standard type
 		expConfig, err := manager.ParseStandardExperimentConfig(rawExpConfig)
 		if err != nil {

api/turing/api/request/request_test.go

@@ -411,7 +411,7 @@ func TestRequestBuildRouterVersionWithDefaults(t *testing.T) {
 
 	// Set up mock Experiment service
 	expSvc := &mocks.ExperimentsService{}
-	expSvc.On("IsStandardExperimentManager", mock.Anything).Return(true)
+	expSvc.On("IsClientSelectionEnabled", mock.Anything).Return(true, nil)
 
 	// Set up mock Ensembler service
 	ensemblerSvc := &mocks.EnsemblersService{}
@@ -468,8 +468,8 @@ func TestBuildExperimentEngineConfig(t *testing.T) {
 		Return("passkey-enc", nil)
 
 	es := &mocks.ExperimentsService{}
-	es.On("IsStandardExperimentManager", "standard-manager").Return(true)
-	es.On("IsStandardExperimentManager", "custom-manager").Return(false)
+	es.On("IsClientSelectionEnabled", "standard-manager").Return(true, nil)
+	es.On("IsClientSelectionEnabled", "custom-manager").Return(false, nil)
 
 	// Define tests
 	tests := map[string]struct {

api/turing/service/experiment_service.go

@@ -24,6 +24,9 @@ const (
 type ExperimentsService interface {
 	// IsStandardExperimentManager checks if the experiment manager is of the standard type
 	IsStandardExperimentManager(engine string) bool
+	// IsClientSelectionEnabled checks if the experiment manager is of the standard type and
+	// has clients
+	IsClientSelectionEnabled(engine string) (bool, error)
 	// ListEngines returns a list of the experiment engines available
 	ListEngines() []manager.Engine
 	// ListClients returns a list of the clients registered on the given experiment engine
@@ -118,6 +121,20 @@ func (es *experimentsService) IsStandardExperimentManager(engine string) bool {
 	return manager.IsStandardExperimentManager(expManager)
 }
 
+func (es *experimentsService) IsClientSelectionEnabled(engine string) (bool, error) {
+	expManager, err := es.getExperimentManager(engine)
+	if err != nil {
+		return false, err
+	}
+	engineInfo, err := expManager.GetEngineInfo()
+	if err != nil {
+		return false, err
+	}
+	return manager.IsStandardExperimentManager(expManager) &&
+		engineInfo.StandardExperimentManagerConfig != nil &&
+		engineInfo.StandardExperimentManagerConfig.ClientSelectionEnabled, nil
+}
+
 func (es *experimentsService) ListEngines() []manager.Engine {
 	engines := []manager.Engine{}
 

api/turing/service/experiment_service_test.go

@@ -8,18 +8,147 @@ import (
 	"testing"
 	"time"
 
-	"github.com/gojek/turing/engines/experiment/manager"
-	"github.com/gojek/turing/engines/experiment/manager/mocks"
-	"github.com/gojek/turing/engines/experiment/pkg/request"
 	"github.com/patrickmn/go-cache"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
+
+	"github.com/gojek/turing/engines/experiment/manager"
+	"github.com/gojek/turing/engines/experiment/manager/mocks"
+	"github.com/gojek/turing/engines/experiment/pkg/request"
 )
 
 var standardExperimentManagerConfig = manager.Engine{Type: manager.StandardExperimentManagerType}
 var customExperimentManagerConfig = manager.Engine{Type: manager.CustomExperimentManagerType}
 
+func TestIsStandardExperimentManager(t *testing.T) {
+	tests := map[string]struct {
+		engineInfo    manager.Engine
+		engineInfoErr string
+		expected      bool
+	}{
+		"standard": {
+			engineInfo: manager.Engine{
+				Name: "standard-engine-1",
+				Type: manager.StandardExperimentManagerType,
+			},
+			expected: true,
+		},
+		"error": {
+			engineInfo: manager.Engine{
+				Name: "standard-engine-2",
+				Type: manager.StandardExperimentManagerType,
+			},
+			engineInfoErr: "test error",
+		},
+		"custom": {
+			engineInfo: manager.Engine{
+				Name: "custom-engine",
+				Type: manager.CustomExperimentManagerType,
+			},
+		},
+	}
+
+	for name, data := range tests {
+		t.Run(name, func(t *testing.T) {
+			// Set up mock API call
+			expMgr := &mocks.ExperimentManager{}
+			var err error
+			if data.engineInfoErr != "" {
+				err = errors.New(data.engineInfoErr)
+			}
+			expMgr.On("GetEngineInfo").Return(data.engineInfo, err)
+			// Set up experiment service
+			svc := &experimentsService{
+				experimentManagers: map[string]manager.ExperimentManager{
+					data.engineInfo.Name: expMgr,
+				},
+				cache: cache.New(time.Second, time.Second),
+			}
+			// Validate
+			isStdEngine := svc.IsStandardExperimentManager(data.engineInfo.Name)
+			assert.Equal(t, data.expected, isStdEngine)
+		})
+	}
+}
+
+func TestIsClientSelectionEnabled(t *testing.T) {
+	// Set up mock experiment managers
+	tests := map[string]struct {
+		engineInfo    manager.Engine
+		engineInfoErr string
+		expectedErr   string
+		expected      bool
+	}{
+		"standard | no client selection": {
+			engineInfo: manager.Engine{
+				Name: "standard-engine-1",
+				Type: manager.StandardExperimentManagerType,
+				StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{
+					ClientSelectionEnabled: false,
+				},
+			},
+		},
+		"standard | no standard config": {
+			engineInfo: manager.Engine{
+				Name: "standard-engine-2",
+				Type: manager.StandardExperimentManagerType,
+			},
+		},
+		"standard | with client selection": {
+			engineInfo: manager.Engine{
+				Name: "standard-engine-3",
+				Type: manager.StandardExperimentManagerType,
+				StandardExperimentManagerConfig: &manager.StandardExperimentManagerConfig{
+					ClientSelectionEnabled: true,
+				},
+			},
+			expected: true,
+		},
+		"custom": {
+			engineInfo: manager.Engine{
+				Name: "custom-engine-1",
+				Type: manager.CustomExperimentManagerType,
+			},
+		},
+		"error": {
+			engineInfo: manager.Engine{
+				Name: "custom-engine-2",
+				Type: manager.CustomExperimentManagerType,
+			},
+			engineInfoErr: "test error",
+			expectedErr:   "test error",
+		},
+	}
+
+	for name, data := range tests {
+		t.Run(name, func(t *testing.T) {
+			// Set up mock API call
+			expMgr := &mocks.ExperimentManager{}
+			var err error
+			if data.engineInfoErr != "" {
+				err = errors.New(data.engineInfoErr)
+			}
+			expMgr.On("GetEngineInfo").Return(data.engineInfo, err)
+			// Set up experiment service
+			svc := &experimentsService{
+				experimentManagers: map[string]manager.ExperimentManager{
+					data.engineInfo.Name: expMgr,
+				},
+				cache: cache.New(time.Second, time.Second),
+			}
+			// Validate
+			isClientSelectionEnabled, err := svc.IsClientSelectionEnabled(data.engineInfo.Name)
+			if data.expectedErr != "" {
+				assert.EqualError(t, err, data.expectedErr)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, data.expected, isClientSelectionEnabled)
+			}
+		})
+	}
+}
+
 func TestListEngines(t *testing.T) {
 	// Set up mock Experiment Managers
 	standardEngineInfo := manager.Engine{

engines/experiment/docs/rpc_plugins.md

@@ -254,16 +254,18 @@ func (*FooBarExperimentRunner) Configure(cfg json.RawMessage) error {
 ExperimentRunner's configuration is stored in the Turing database together with the rest of the Router configuration. I.e:
 
 **Router Configuration Stage:**
-  1. Turing Server calls `ExperimentEngine plugin`::`ExperimentManager`::`GetExperimentRunnerConfig` to retrieve  
-     ExperimentRunner's configuration
+  1. Based on the type of the experiment manager (standard or custom) and the engine properties, the router's experiment data may be configured.
   2. Turing Server saves this data into the database as part of the Router configuration 
-     (`router_versions.experiment_engine`) 
+     (`router_versions.experiment_engine`).
+The screenshot below shows the configuration UI for the standard experiment engine defined in [`configs/plugin_config_example.yaml`](../examples/plugins/hardcoded/configs/plugin_config_example.yaml)
+
+![example_experiment_manager_ui](./assets/example_experiment_manager_router_ui.png)
 
 **Router (Re-)Deployment Stage:**
   1. Turing Server retrieves Router's configuration from the Database
-  2. Turing Server creates required resources in the k8s cluster (deployments, services, config maps and secrets)
-  3. Turing Router is being deployed and during the initialization, it establishes the connection with the 
-     ExperimentEngine plugin and passes the configuration into `ExperimentRunner`'s `Configure` method.
+  2. Turing Server calls `ExperimentEngine plugin`::`ExperimentManager`::`GetExperimentRunnerConfig` with the saved configuration from the DB, to create the configuration used by the `ExperimentRunner`
+  3. Turing Server creates required resources in the k8s cluster (deployments, services, config maps and secrets)
+  4. Turing Router is being deployed and during the initialization, it establishes the connection with the ExperimentEngine plugin and passes the configuration from step 2 into `ExperimentRunner`'s `Configure` method.
 
 ### Logging
 In order for Turing Server/Router to include log messages from the Experiment Engine plugin, you can use 

engines/experiment/examples/plugins/hardcoded/config.go

@@ -25,9 +25,9 @@ type Experiment struct {
 }
 
 type ManagerConfig struct {
-	Engine      manager.Engine      `json:"engine"`
-	Experiments []Experiment        `json:"experiments"`
-	Variables   []manager.Variables `json:"variables"`
+	Engine      manager.Engine                `json:"engine"`
+	Experiments []Experiment                  `json:"experiments"`
+	Variables   map[string][]manager.Variable `json:"variables"`
 }
 
 type RunnerConfig struct {

engines/experiment/examples/plugins/hardcoded/configs/plugin_config_example.yaml

@@ -5,7 +5,7 @@ engine:
   type: standard
   standard_experiment_manager_config:
     client_selection_enabled: false
-    experiment_selection_enabled: false
+    experiment_selection_enabled: true
 experiments:
   - id: '001'
     name: exp_1
@@ -21,3 +21,8 @@ experiments:
         traffic: 0.15
         treatment_configuration:
           bar: baz
+variables:
+  '001':
+    - name: client
+      required: true
+      type: unit