Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Add env var for exp engine secret path #287

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Nov 16, 2022

Context

In PR #274, changes were made to the way experiment engine secrets were passed to the plugin manager of the experiment engine. One of these secrets is the Google service account token (stored in a json file), which gets mounted as a volume within a Turing Router deployment. The experiment engine plugin (run as a subprocess of the Turing Router and hence is run within the same container as it) is then supposed to access this file using a file path stored in an environment variable.

However, it is possible for the Turing Router and the experiment engine to have separate Google service accounts instead of the same one. Unfortunately, PR #274 has missed out specifying a file path to the service account token of the experiment engine. While the file path of the router's service account token remains stored in the env variable GOOGLE_APPLICATION_CREDENTIALS, there is a need to create another environment variable that points to the location of the experiment engine service account.

This PR does just that and injects an additional environment variable EXP_GOOGLE_APPLICATION_CREDENTIALS for the said purpose.

@deadlycoconuts deadlycoconuts self-assigned this Nov 16, 2022
@deadlycoconuts deadlycoconuts force-pushed the add_exp_engine_env_vars_to_router branch 7 times, most recently from d295d39 to 3387ca1 Compare November 23, 2022 19:43
@deadlycoconuts deadlycoconuts added the type: bug Something isn't working label Nov 23, 2022
@deadlycoconuts deadlycoconuts marked this pull request as ready for review November 23, 2022 19:59
@deadlycoconuts deadlycoconuts changed the title Add env var for exp engine secret path Bugfix: Add env var for exp engine secret path Nov 23, 2022
@deadlycoconuts deadlycoconuts requested a review from a team November 24, 2022 01:27
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this fix.

envRouterConfigFile = "ROUTER_CONFIG_FILE"
envRouterProtocol = "ROUTER_PROTOCOL"
envGoogleApplicationCredentials = "GOOGLE_APPLICATION_CREDENTIALS"
envExpGoogleApplicationCredentials = "EXP_GOOGLE_APPLICATION_CREDENTIALS"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit: can this be GOOGLE_APPLICATION_CREDENTIALS_EXPERIMENT_ENGINE ? I think it's nicer if the name starts with GOOGLE_APPLICATION_CREDENTIALS and replacing EXP with EXPERIMENT_ENGINE would be clearer.

@deadlycoconuts deadlycoconuts force-pushed the add_exp_engine_env_vars_to_router branch from 3387ca1 to 6cab482 Compare December 6, 2022 16:44
@deadlycoconuts deadlycoconuts merged commit 5f67f82 into caraml-dev:main Dec 6, 2022
@deadlycoconuts deadlycoconuts deleted the add_exp_engine_env_vars_to_router branch December 6, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants