-
Notifications
You must be signed in to change notification settings - Fork 241
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
local-run: add option to set service auth token in container environment #3922
Conversation
8f712ca
to
a93571e
Compare
paasta_tools/cli/utils.py
Outdated
def get_current_ecosystem() -> str: | ||
"""Infer ecosystem from local-run configuration""" | ||
local_run_config = load_system_paasta_config().get_local_run_config() | ||
ecosystem = local_run_config["default_cluster"].split("-", 1)[-1] | ||
return f"{ecosystem}prod" if ecosystem == "corp" else ecosystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this meant to be the ecosystem that local-run is being run in (e.g., devc for local-runs from a devboc) or the ecosystem of the instance configuration being used (e.g, stagef if someone uses -c norcal-stagef
from a local-run from a devbox)?
if it's the former, it would probably be a little more standard to read /nail/etc/ecosystem - and if it's the latter, then this is incorrect since this will return the ecosystem of the cluster configuration you'd look at if you ran local-run
without -c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the former. The fact is that this codebase does not make references to our internal filesystem structure in that manner, so I wanted to keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha - imo, referencing /nail/etc
would probably be preferable since folks wouldn't be aware that default_cluster
value in SystemPaastaConfig is being used for locating where a box is hosted without reading this code
e.g., if we were to force that users specify a --cluster arg in local-run and cleaned up the SystemPaastaConfig in puppet, we'd be in for a bit of a surprise once this stopped working
(and fwiw, we're already reading /nail/etc/runtimeenv in this file (and other /nail or /nail/etc/ filelocs in other files))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we really wanted to make this flexible, we could maybe add a little bit more SystemPaastaConfig to make some of these dirs configurable - but a lot of the concepts are pretty yelp-centric anyway (and we're really more developing in the open than anything else here :p) so it doesn't seem like we'd gain much embarking on that effort
vault_client = get_vault_client(get_vault_url(ecosystem), get_vault_ca(ecosystem)) | ||
vault_role = load_system_paasta_config().get_service_auth_vault_role() | ||
metadata_provider = InstanceMetadataProvider( | ||
iam_role_fetcher=InstanceMetadataFetcher(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious: would we need to do any sort of env var clearing for this to work if folks have any AWS_* env vars set (or if they have a default profile or anything like that)
(i've personally run into fun stuff like that when attempting to run things that used only the instance profile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, this always calls to the instance metadata endpoint, regardless of env values.
Environment stuff is never referenced:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, perfect!
Unit tests are a bit pointless since it's just a waterfall of mocks, but the implementation works: