introduce automated credential rotation for BDBA #1144
Conversation
|
@TuanAnh17N Thank you for your contribution. |
gardener-robot
added
the
size/m
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TuanAnh17N
force-pushed
the
bdba-cred-rotation
branch
from
758dcfc to
c703ed7
Compare
| credentials = cfg_element.credentials() | ||
| headers = {'Authorization': f'Bearer {credentials.token()}'} | ||
|
|
||
| response = requests.get( |
There was a problem hiding this comment.
damn... I think it would be better to add this to delivery-service's bdba.client. However, this would introduce a cyclic dependency :-(
Could you:
- also add this into mentioned bdba-client
- add a reference to delivery-service-repository + a todo (to deduplicate)
OTOH, from python-package's pov, the dependency would not actually be cyclic.... sooo wdyt about declaring dependency against delivery-service's bdba-package?
There was a problem hiding this comment.
not too sure about introducing a dependency towards delivery-service's bdba-package ...
this would also contain recently introduced secret-mgmt.
in my opinion it is fine to keep this specific bdba implementation in parallel, as it is not to be expected that this will be expanded in future
|
|
||
|
|
||
| def get_api_key_expiry( | ||
| cfg_element: model.bdba.BDBAConfig |
There was a problem hiding this comment.
see comment below. Also, I think we should not build such functions around our cfg-model-classes (separation of concerns). It is okay to have a util-function. however, the logic for issueing requests against bdba should be handled by our bdba-client.
| @@ -18,6 +19,9 @@ | |||
|
|
|||
| logger = logging.getLogger(__name__) | |||
|
|
|||
| MAX_RETRIES = 5 | |||
There was a problem hiding this comment.
I think we need more retries / longer delay (or (better) exponential backoff).
consider we sometimes see maintenance windows of a couple of hours for GHE. so we should make our code be able to bridge such a gap
also, instead of setting module-level-constants, and read from multiple places in (one) function, preferably set those as default values in function-parameters.
| @@ -297,7 +301,31 @@ def rotate_config_element_and_persist_in_cfg_repo( | |||
| git_helper.add_and_commit( | |||
| message=commit_message, | |||
| ) | |||
| git_helper.push('@', target_ref) | |||
| for attempt in range(1, MAX_RETRIES + 1): | |||
There was a problem hiding this comment.
I suggest to stick to using recursion (as we commonly do in other places in our codebase). like so:
def function_with_retry(retries=123):
try:
something()
except ExpectedException as e:
if retries > 0:
function_with_retry(retries=retries - 1)
| logger.info(f'Retrying in {RETRY_DELAY} seconds...') | ||
|
|
||
| # pull the latest changes and rebase | ||
| latest_commit = git_helper.fetch_head(target_ref) |
There was a problem hiding this comment.
chance for success will be greater if we do fetch + hard-reset + recreate changes
| git_helper.rebase(latest_commit.hexsha) | ||
|
|
||
| time.sleep(RETRY_DELAY) | ||
| else: |
There was a problem hiding this comment.
invert (early-exit) to decrease indent, like so:
if <error-condition>:
raise ....
# regular code (w/o indentation) goes here
| ''' | ||
| if cfg_element._type_name == 'bdba': | ||
| logger.info(f'NEW BDBA API KEY: {secret_id.get('api_key')}') | ||
| raise RuntimeError(f'Failed to push changes to GitHub after {MAX_RETRIES} attempts') |
There was a problem hiding this comment.
we might want to use a dedicated exception-type for that case "FailedToWritebackCredentials" or similar, as this might be reasonable to be handled w/ specific error-handling
|
|
||
| import cfg_mgmt | ||
| import model.bdba | ||
|
|
Release note: