-
Notifications
You must be signed in to change notification settings - Fork 0
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
Access key check updates #50
Changes from 21 commits
c4df986
12854c1
90fb081
1df539c
e80cfce
a81a473
9e56ee3
a86a4f5
9cea755
6bac7ba
0b763a8
af0ba55
d132e65
ea55d85
151c56f
cfaaefc
e941f6c
671011a
8316534
ee2955f
052c846
ead6cb3
7701a51
a393257
c255753
3f1d755
ed5cecb
f2be23e
6f52374
7d8f5d7
b47be8a
8ed11dd
6cdf27b
3162c0e
82f718a
1e3f82a
b114846
405f948
dc080cc
624caa3
9c29a32
018e8fa
99393be
f0d0fbc
40a86f5
ecb17f8
8010f50
d55478a
ea3654b
20c49ae
7eab6b9
753c9ac
c255694
7645761
b236965
13a85ff
9585ff3
4b42cd6
c86c78f
de0b236
d301b23
d8a1017
d877581
fd3d7d8
39bafbc
d70f21a
adaefde
d1fd457
8f70fd6
513c89a
af1c2bd
dc1b376
5b1a86b
1aea9cc
a9e97a5
10b9d2b
18114ac
699435c
69f9c40
e897284
dbbf489
0b9df51
a6e4c9c
5946774
5214458
ea254e8
e0e7ca2
b416267
941d5e4
9a42c65
1868f4a
0b73ccf
0f0698e
16f970c
eb56d80
e0e197f
05e2d45
23c72e4
a91401c
fad3f4d
18abc68
4ce3ae9
d2ead82
5e91511
4ae1526
bf39b98
5f72cc3
95b2c17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,7 +70,6 @@ class AppUtilsCore(ReactApi, Routes): | |
""" | ||
|
||
CHECK_SETUP_FILE_NAME = "check_setup.json" | ||
ACCOUNTS_FILE_NAME = "accounts.json.encrypted" | ||
|
||
# Define in subclass. | ||
APP_PACKAGE_NAME = None | ||
|
@@ -119,9 +118,6 @@ def __init__(self): | |
self.sqs = SQS(self.prefix) | ||
self.check_setup_file = self._locate_check_setup_file() | ||
logger.info(f"Using check_setup file: {self.check_setup_file}") | ||
self.accounts_file = self._locate_accounts_file() | ||
if self.accounts_file: | ||
logger.info(f"Using accounts file: {self.accounts_file}") | ||
self.check_handler = CheckHandler(self.prefix, self.package_name, self.check_setup_file, self.get_default_env()) | ||
self.CheckResult = self.check_handler.CheckResult | ||
self.ActionResult = self.check_handler.ActionResult | ||
|
@@ -182,6 +178,9 @@ def init_connection(self, environ, _environments=None): | |
Returns an FSConnection object or raises an error. | ||
""" | ||
environments = self.init_environments(environ) if _environments is None else _environments | ||
if not environments: | ||
environ = self.get_default_env(); | ||
environments = self.init_environments(environ) if _environments is None else _environments | ||
logger.warning("environments = %s" % str(environments)) | ||
# if still not there, return an error | ||
if environ not in environments: | ||
|
@@ -1862,10 +1861,13 @@ def run_check_runner(self, runner_input, propogate=True): | |
Returns: | ||
dict: run result if something was run, else None | ||
""" | ||
# FYI the runner_input argument is a dict that looks something like this (2023-06-16): | ||
# {'sqs_url': 'https://sqs.us-east-1.amazonaws.com/466564410312/foursight-cgap-prod-check_queue'} | ||
# and the propogate arguments is a bootstrap.LambdaContext that instance. | ||
sqs_url = runner_input.get('sqs_url') | ||
if not sqs_url: | ||
return | ||
client = boto3.client('sqs') | ||
client = SQS.get_sqs_boto_client() | ||
response = client.receive_message( | ||
QueueUrl=sqs_url, | ||
AttributeNames=['MessageGroupId'], | ||
|
@@ -1876,7 +1878,7 @@ def run_check_runner(self, runner_input, propogate=True): | |
message = response.get('Messages', [{}])[0] | ||
|
||
# TODO/2022-12-01/dmichaels: Issue with check not running because not detecting that | ||
# dependency # has already run; for example with expset_opf_unique_files_in_experiments | ||
# dependency has already run; for example with expset_opf_unique_files_in_experiments | ||
# depending on expset_opfsets_unique_titles; seems not checking the result in S3 of the | ||
# depdendency correctly. This is what seems to be returned here if the check has a dependency, e.g.: | ||
# | ||
|
@@ -1996,7 +1998,7 @@ def run_check_runner(self, runner_input, propogate=True): | |
stage = self.stage.get_stage() | ||
if run_result['type'] == 'check' and run_result['kwargs']['queue_action'] == stage: | ||
# must also have check.action and check.allow_action set | ||
if run_result['allow_action'] and run_result['action']: | ||
if run_result['allow_action'] and run_result['action'] and not run_result.get('prevent_action'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feels like this should be a subroutine that can be independently tested i all the various cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made TODO comment to this effect for now. |
||
action_params = {'check_name': run_result['name'], | ||
'called_by': run_result['kwargs']['uuid']} | ||
try: | ||
|
@@ -2031,12 +2033,9 @@ def collect_run_info(cls, run_uuid, env, no_trailing_slash=False): | |
def _locate_check_setup_file(self) -> Optional[str]: | ||
return self._locate_config_file(AppUtilsCore.CHECK_SETUP_FILE_NAME) | ||
|
||
def _locate_accounts_file(self) -> Optional[str]: | ||
return self._locate_config_file(AppUtilsCore.ACCOUNTS_FILE_NAME) | ||
|
||
def _locate_config_file(self, file_name: str) -> Optional[str]: | ||
""" | ||
Returns the full path to the given named file (e.g. check_setup.json or accounts.json), | ||
Returns the full path to the given named file (e.g. check_setup.json), | ||
looking for the first NON-EMPTY file within these directories, in the following order; | ||
if not found then returns None. | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,13 @@ def access_key_status(connection, **kwargs): | |
check.action = 'refresh_access_keys' | ||
# TOOD: Figure this out ... Seems like, both from this code and what we are seeing in the actual action history, | ||
# that the refresh action is running everday; we only want to run a refresh if the access is expiring very soon. | ||
check.allow_action = True # always allow refresh | ||
# Always allow refresh | ||
# TODO/INPROGRESS/2023-06-16/dmichaels: | ||
# But I think if we do not want the action to run automatically then set check.action to "" (cannot be None). | ||
# No nevermind that does not work because then no action is shown at all. | ||
# Try setting allow_action to False ... | ||
# Works but then not always enabled ... need another bit ... | ||
check.allow_action = True | ||
fs_user_email, fs_user_kp = '[email protected]', 'access_key_foursight' | ||
user_props = get_metadata(f'/users/{fs_user_email}?datastore=database', key=connection.ff_keys) | ||
user_uuid = user_props['uuid'] | ||
|
@@ -41,11 +47,13 @@ def access_key_status(connection, **kwargs): | |
check.summary = (f'Application access keys will expire in less than 21 days! Please run' | ||
f' the deployment action soon') | ||
check.brief_output = check.full_output = check.summary | ||
check.prevent_action = True | ||
return check | ||
else: | ||
check.status = 'PASS' | ||
check.summary = (f'Application access keys expiration is more than 3 weeks away. All good.' | ||
f' Expiration date: {expiration_date}') | ||
check.prevent_action = True | ||
return check | ||
|
||
|
||
|
@@ -78,7 +86,6 @@ def refresh_access_keys(connection, **kwargs): | |
access_key_res = post_metadata(access_key_req, 'access-keys', key=connection.ff_keys) | ||
access_key_id = access_key_res.get('access_key_id') | ||
secret_access_key = access_key_res.get('secret_access_key') | ||
import pdb ; pdb.set_trace() | ||
if not access_key_id or not secret_access_key: | ||
# We will say these must occur in pairs; both at the top level or both within the @graph array. | ||
graph_item = access_key_res.get('@graph', [{}])[0] | ||
|
@@ -90,7 +97,11 @@ def refresh_access_keys(connection, **kwargs): | |
# clear out old keys after generating new one | ||
for access_key in access_keys: # note this search result was computed before the new key was added | ||
if access_key['status'] != 'deleted': | ||
patch_metadata(patch_item={'status': 'deleted'}, obj_id=access_key['uuid'], key=connection.ff_keys) | ||
try: | ||
patch_metadata(patch_item={'status': 'deleted'}, obj_id=access_key['uuid'], key=connection.ff_keys) | ||
except Exception as e: | ||
Comment on lines
+111
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would add a comment in this loop indicating that the email ordering above is VERY important to maintain because it is the foursight keys that are used for these operations and if you refresh them (ie: patch to deleted) out of order here, all future requests will fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, got it (after out Slack exchange). |
||
print(f"Exception while trying to delete old access key ({access_key['uuid']})") | ||
print(e) | ||
|
||
action.full_output = full_output | ||
action.status = 'DONE' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
from dcicutils.common import EnvName, ChaliceStage | ||
from dcicutils.env_manager import EnvManager | ||
from dcicutils.function_cache_decorator import function_cache | ||
from dcicutils.misc_utils import full_class_name | ||
from dcicutils.s3_utils import s3Utils | ||
from typing import Optional, List | ||
|
@@ -40,7 +41,7 @@ def __init__(self, foursight_prefix: Optional[str] = None): # the foursight_pre | |
|
||
self.prefix = prefix | ||
self.s3_connection = S3Connection(self.get_env_bucket_name()) | ||
self.cached_list_environment_names = None | ||
# self.cached_list_environment_names = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reasoning for commenting out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now using the @function_cache decorator instead; will delete commented out code. |
||
|
||
def get_env_bucket_name(self) -> Optional[str]: | ||
|
||
|
@@ -62,14 +63,15 @@ def list_unique_environment_names(self) -> List[EnvName]: | |
result.add(infer_foursight_from_env(envname=env)) | ||
return sorted(result) # a list and sorted | ||
|
||
@function_cache | ||
def list_environment_names(self) -> List[EnvName]: | ||
""" | ||
Lists all environments in the foursight-envs s3. | ||
|
||
Returns: a list of names | ||
""" | ||
if self.cached_list_environment_names: | ||
return self.cached_list_environment_names | ||
# if self.cached_list_environment_names: | ||
# return self.cached_list_environment_names | ||
|
||
environment_names = [infer_foursight_from_env(envname=env) | ||
for env in sorted(EnvManager.get_all_environments(env_bucket=self.get_env_bucket_name()))] | ||
|
@@ -83,7 +85,7 @@ def list_environment_names(self) -> List[EnvName]: | |
if modern_full_names != legacy_full_names: | ||
logger.warning(f"{full_class_name(self)}.list_environment_names has consistency problems.") | ||
|
||
self.cached_list_environment_names = environment_names | ||
# self.cached_list_environment_names = environment_names | ||
return environment_names | ||
|
||
def list_valid_schedule_environment_names(self) -> List[EnvName]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
import logging | ||
from typing import Optional | ||
from .datetime_utils import convert_utc_datetime_to_useastern_datetime_string | ||
from dcicutils.boto_s3 import boto_s3_client, boto_s3_resource | ||
|
||
logging.basicConfig() | ||
logger = logging.getLogger(__name__) | ||
|
@@ -13,7 +14,7 @@ class AwsS3: | |
def get_buckets(cls) -> list: | ||
results = [] | ||
try: | ||
s3 = boto3.resource("s3") | ||
s3 = boto_s3_resource() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whether this is a good rewrite depends on the result of the other long comment I made about what paradigm to use. I won't mark these in detail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, these are gone now, using the monkey patching thing for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a different PR? I still see them in my review copy here. |
||
results = sorted([bucket.name for bucket in s3.buckets.all()]) | ||
except Exception as e: | ||
logger.error(f"Exception getting S3 bucket list: {e}") | ||
|
@@ -23,7 +24,7 @@ def get_buckets(cls) -> list: | |
def get_bucket_keys(cls, bucket_name: str) -> list: | ||
results = [] | ||
try: | ||
s3 = boto3.client("s3") | ||
s3 = boto_s3_client() | ||
bucket_keys = s3.list_objects(Bucket=bucket_name) | ||
if bucket_keys: | ||
bucket_keys = bucket_keys.get("Contents") | ||
|
@@ -54,7 +55,7 @@ def _may_look_at_key_content(cls, bucket, key, size) -> bool: | |
@classmethod | ||
def _get_bucket_key_content_size(cls, bucket_name: str, bucket_key_name) -> int: | ||
try: | ||
s3 = boto3.client('s3') | ||
s3 = boto_s3_client() | ||
response = s3.head_object(Bucket=bucket_name, Key=bucket_key_name) | ||
size = response['ContentLength'] | ||
return size | ||
|
@@ -77,7 +78,7 @@ def get_bucket_key_contents(cls, bucket_name: str, bucket_key_name) -> Optional[ | |
size = cls._get_bucket_key_content_size(bucket_name, bucket_key_name) | ||
if size <= 0 or not cls._may_look_at_key_content(bucket_name, bucket_key_name, size): | ||
return None | ||
s3 = boto3.resource("s3") | ||
s3 = boto_s3_resource() | ||
s3_object = s3.Object(bucket_name, bucket_key_name) | ||
return s3_object.get()["Body"].read().decode("utf-8") | ||
except Exception as e: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,10 @@ | |
from dcicutils.misc_utils import future_datetime | ||
from typing import Optional, Tuple | ||
from ...app import app | ||
from .datetime_utils import convert_iso_datetime_string_to_datetime | ||
from .datetime_utils import ( | ||
convert_iso_datetime_string_to_datetime as normalize_portal_datetime, | ||
convert_utc_datetime_to_useastern_datetime_string as datetime_to_string | ||
) | ||
|
||
|
||
_PORTAL_ACCESS_KEY_NAME = "access_key_foursight" | ||
|
@@ -32,14 +35,14 @@ def get_portal_access_key_info(env: str, | |
access_key_create_date, access_key_expires_date, access_key_expires_exception = \ | ||
_get_portal_access_key_expires_date(connection_keys) | ||
if access_key_expires_date: | ||
access_key_info["created_at"] = access_key_create_date.strftime("%Y-%m-%d %H:%M:%S") | ||
access_key_info["created_at"] = datetime_to_string(access_key_create_date) | ||
if access_key_expires_date == datetime.max: | ||
access_key_info["expires_at"] = None | ||
access_key_info["expired"] = False | ||
access_key_info["invalid"] = False | ||
access_key_info["expires_soon"] = False | ||
else: | ||
access_key_info["expires_at"] = access_key_expires_date.strftime("%Y-%m-%d %H:%M:%S") | ||
access_key_info["expires_at"] = datetime_to_string(access_key_expires_date) | ||
access_key_info["expired"] = now >= access_key_expires_date | ||
access_key_info["invalid"] = access_key_info["expired"] | ||
# Note that these "test_mode_xyz" variables are for testing only and are set | ||
|
@@ -61,7 +64,7 @@ def get_portal_access_key_info(env: str, | |
if "credenti" in e or "expir" in e: | ||
access_key_info["probably_expired"] = True | ||
access_key_info["invalid"] = True | ||
access_key_info["timestamp"] = now.strftime("%Y-%m-%d %H:%M:%S") | ||
access_key_info["timestamp"] = datetime_to_string(now.strftime("%Y-%m-%d %H:%M:%S")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like not to have any calls amid other code to something like now.strftime with a random string you could get wrong. Is there not a function that does this? can we make one? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh weird, that one is just a mistake, should just be datetime_to_string(now) |
||
return access_key_info | ||
except Exception: | ||
return {} | ||
|
@@ -71,10 +74,10 @@ def _get_portal_access_key_expires_date(keys: dict) -> Tuple[datetime, Optional[ | |
try: | ||
query = f"/search/?type=AccessKey&description={_PORTAL_ACCESS_KEY_NAME}&sort=-date_created" | ||
access_key = ff_utils.search_metadata(query, key=keys)[0] | ||
access_key_create_date = convert_iso_datetime_string_to_datetime(access_key["date_created"]) | ||
access_key_create_date = normalize_portal_datetime(access_key["date_created"]) | ||
access_key_expires_date = access_key.get("expiration_date") | ||
if access_key_expires_date: | ||
access_key_expires_date = convert_iso_datetime_string_to_datetime(access_key_expires_date) | ||
access_key_expires_date = normalize_portal_datetime(access_key_expires_date) | ||
else: | ||
# There may or may not be an expiration date (e.g. for fourfront); | ||
# if not then make it the max date which is 9999-12-31 23:59:59.999999. | ||
|
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.
Did you mean to do this both unconditionally and conditionally right after that?
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.
Not sure exactly what you mean ... if no environments from init_environment we make the same call but with a different (default) environ.
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.
I see. Well, it looks like it doesn't check that the default env is in fact different, but I missed that you were looking up a (potentially) different env to use.