Bugfix: Turing API should process experiment engine passkey only if client selection enabled (#196)

* Bugfix: Passkey should not be processed if client selection disabled

* Update hardcoded sample plugin to use experiment variables, consistent with the runner

* Update RPC plugin example and docs

* Correct numbering in doc and plugin name change

* Add debug message

* Update Deployment controller to consider if client selection enabled

* Add another unit test case for TestIsClientSelectionEnabled

Co-authored-by: Krithika Sundararajan <[email protected]>
(cherry picked from commit 83a0c6c)

Author: krithika369

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/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{

api/turing/service/mocks/experiments_service.go

@@ -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/docs/assets/example_experiment_manager_router_ui.png

@@ -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/docs/rpc_plugins.md

@@ -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

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

@@ -2,12 +2,14 @@ package hardcoded
 
 import (
 	"encoding/json"
+
 	"github.com/gojek/turing/engines/experiment/manager"
 )
 
 type ExperimentManager struct {
 	*manager.BaseStandardExperimentManager
 	experiments map[string]Experiment
+	variables   map[string][]manager.Variable
 }
 
 func (e *ExperimentManager) Configure(cfg json.RawMessage) error {
@@ -23,6 +25,7 @@ func (e *ExperimentManager) Configure(cfg json.RawMessage) error {
 	for _, exp := range config.Experiments {
 		e.experiments[exp.Name] = exp
 	}
+	e.variables = config.Variables
 	return nil
 }
 
@@ -39,6 +42,16 @@ func (e *ExperimentManager) ListExperimentsForClient(manager.Client) ([]manager.
 	return e.ListExperiments()
 }
 
+func (e *ExperimentManager) ListVariablesForExperiments(exps []manager.Experiment) (map[string][]manager.Variable, error) {
+	variableMap := map[string][]manager.Variable{}
+	for _, exp := range exps {
+		if variables, ok := e.variables[exp.ID]; ok {
+			variableMap[exp.ID] = variables
+		}
+	}
+	return variableMap, nil
+}
+
 func (e ExperimentManager) GetExperimentRunnerConfig(cfg json.RawMessage) (json.RawMessage, error) {
 	standardExpCfg, err := manager.ParseStandardExperimentConfig(cfg)
 	if err != nil {