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

Fix spam on ICA rotator (and general upgrade) #411

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

andrewpatto
Copy link
Member

Upgrade to CDKv2
Modernise build (use uv and Makefile)

@@ -1,9 +1,6 @@
[mypy]
strict = True

[mypy-aws_cdk.*]
ignore_missing_imports = True

Copy link
Member Author

Choose a reason for hiding this comment

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

this was only here because cdk used to be missing these

@@ -1,13 +1,12 @@
import os

from aws_cdk import core as cdk
from aws_cdk import App, Environment
Copy link
Member Author

Choose a reason for hiding this comment

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

these are all the v1 -> v2 migration

env=cdk.Environment(
account=os.environ["CDK_DEFAULT_ACCOUNT"],
region=os.environ["CDK_DEFAULT_REGION"],
),
Copy link
Member Author

Choose a reason for hiding this comment

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

I took out the ability for these to be deployed account agnostic - because I don't know the use case for that?

Copy link
Member

Choose a reason for hiding this comment

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

One for Alexis

Copy link
Member

Choose a reason for hiding this comment

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

Happy to drop this

@@ -44,9 +45,9 @@ def __init__(
data_project,
workflow_projects,
ica_base_url,
"cron(0 4/12 * * ? *)",
Copy link
Member Author

Choose a reason for hiding this comment

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

Using cron expressions to give more certainty to timings

assumed_by=iam.FederatedPrincipal(
"arn:aws:iam::"
+ Stack.of(self).account
+ ":oidc-provider/token.actions.githubusercontent.com",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is better because we shouldn't need to pass in the account - the Stack already knows where it is being installed

# obviously the rotation cron schedules must in some way match up so that this window is used
if n.isoweekday() == 1:
if n.hour < 12:
send_secrets_event_to_slack(ev, slack_host_ssm_name, slack_webhook_ssm_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Some crappy logic but we can work on this as we want down the track

@victorskl
Copy link
Member

will review soon...

@andrewpatto
Copy link
Member Author

no rush - is deployed to dev now so will see it do its thing over the next week anyhow

@andrewpatto
Copy link
Member Author

I'm a bit proud of my Makefile btw. It does some fancy stuff.

Copy link
Member

@victorskl victorskl left a comment

Choose a reason for hiding this comment

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

LGTM. Nice modernising effort, agree.!

Leaving more details requirement and comment around ICAv2 to Alexis.

typecheck:
. .venv/bin/activate; mypy lambdas/notify_slack_lambda/app
. .venv/bin/activate; mypy lambdas/jwt_producer_lambda/app
. .venv/bin/activate; mypy cdk
Copy link
Member

Choose a reason for hiding this comment

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

I will try this typechecker soon - https://github.com/python/mypy
It looks promising.

Copy link
Member Author

Choose a reason for hiding this comment

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

100% - Python without mypy is like writing straight javascript

env=cdk.Environment(
account=os.environ["CDK_DEFAULT_ACCOUNT"],
region=os.environ["CDK_DEFAULT_REGION"],
),
Copy link
Member

Choose a reason for hiding this comment

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

One for Alexis

@victorskl
Copy link
Member

Somewhat related to #401

@victorskl
Copy link
Member

Hi @andrewpatto happy to merge this whenever you ready.

@andrewpatto
Copy link
Member Author

Didn't this all get replaced by a different stack @alexiswl ?

@alexiswl
Copy link
Member

alexiswl commented Aug 1, 2024

Didn't this all get replaced by a different stack @alexiswl ?

New stack only supports ICAv2, not ICAv1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants