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

PYIC-5872: Expose core's internal API in dev #1966

Merged
merged 4 commits into from
Jun 5, 2024
Merged

Conversation

Wynndow
Copy link
Contributor

@Wynndow Wynndow commented Jun 3, 2024

Proposed changes

What changed

Expose core's internal API to requets with an API key.

Why did it change

PYIC-5872: Expose internal API in dev

This change allows core's internal API to be accessed with an API key
when deployed to either of the dev accounts.

It adds a usage plan with API key to the internal API, and then makes
use of an API key required only for the dev accounts. It also adds DNS
to the API gateway to make use of the internal API easier.

Also, to allow programatic access to the API key, a new resource policy
is created to allow the OIDC identity provider enough permissions to
fetch the API key.

It removes the CoreFrontLocal parameter in favour of defaulting to
this behavour for dev accounts. This is because changing back to a
private API from the new regional version is a two step process - so not
as easy as just toggling the parameter.

It's a two step process as you can't change from regional to private if
it has custom domains associated with it. In CloudFormation there isn't
a way to specify that something should be removed before making changes,
resulting in having to remove the custom domain before changing the API
to private.

Ideally I would have liked to have conditionally created the UsagePlan -
only for the dev accounts. However there appears to be a bug in cfn-lint
that means we can't use an If function with the CreateUsagePlan
property.

The GHA workflow included in this PR is just to demonstrate that the
gateway is now available with an API key, and also how to
programatically access the API key. This would not be merged with the
PR.

PYIC-5872: Don't use SAM UsagePlan

SAM allows an easy way to create a usage plan and API key, by defining
the UsagePlan property in an Api resource. Unfortunately you can't
use an !If function with it do conditionally create it.

This removes the use of it and defines the resources requied using
plain old CloudFormation. This allows us to only create them when we
want.

Using the previous method would have caused a usage plan and api key to
be created in all envs including prod. This felt like murky ground - the
non dev api's would still be private so it wouldn't have been an issue,
but feels safer to only create them when actually needed.

PYIC-5872: Create separate API for internal testing

This creates a new API gateway to run in parallel with the original.
This means that the VPC protected internal API can still be used by
core-front without any changes, and the new regional, API token
protected API gateway can be used for testing.

cfn-lint has an issue with having an event defined in a lambda that
includes a reference to a conditionally created resource, even if that
resource is behind the same condition. This has lead to the ignore
written into the metadata of a number of resources.

Issue tracking

@Wynndow Wynndow force-pushed the pyic-5872-open-gateway branch 4 times, most recently from 83b1664 to 796c789 Compare June 3, 2024 11:00
@Wynndow Wynndow changed the title PYIC-5872: Open the gates PYIC-5872: Expose core's internal API in dev Jun 3, 2024
@Wynndow Wynndow force-pushed the pyic-5872-open-gateway branch 2 times, most recently from f2b6369 to 7391cec Compare June 3, 2024 11:49
@Wynndow Wynndow marked this pull request as ready for review June 3, 2024 11:49
@Wynndow Wynndow requested review from a team as code owners June 3, 2024 11:49
Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

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

This seems better to me, what do you think?

It makes the prod configuration cleaner, and isn't much more code than before. The main thing that's annoying is configuring the extra events and overriding that cfn-lint rule :/

deploy/template.yaml Outdated Show resolved Hide resolved
deploy/template.yaml Show resolved Hide resolved
@Wynndow
Copy link
Contributor Author

Wynndow commented Jun 3, 2024

This seems better to me, what do you think?

It makes the prod configuration cleaner, and isn't much more code than before. The main thing that's annoying is configuring the extra events and overriding that cfn-lint rule :/

I think this is ok - at some point we're going to create a new lambda and forget to add the extra testing event on. We could write a macro to do that for us and not have to define the event at all....

@Wynndow Wynndow force-pushed the pyic-5872-open-gateway branch from 5c8765d to b94e335 Compare June 3, 2024 14:36
Copy link
Contributor

@Joe-Edwards-GDS Joe-Edwards-GDS left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this, but I think we need to:

  • Remove the test pipeline code
  • Monitor this in build to make sure it doesn't cause any changes there

@Wynndow Wynndow force-pushed the pyic-5872-open-gateway branch from 828c613 to 8eade35 Compare June 5, 2024 08:56
Joe-Edwards-GDS
Joe-Edwards-GDS previously approved these changes Jun 5, 2024
Joe-Edwards-GDS
Joe-Edwards-GDS previously approved these changes Jun 5, 2024
Wynndow added 4 commits June 5, 2024 14:16
This change allows core's internal API to be accessed with an API key
when deployed to either of the dev accounts.

It adds a usage plan with API key to the internal API, and then makes
use of an API key required only for the dev accounts. It also adds DNS
to the API gateway to make use of the internal API easier.

Also, to allow programatic access to the API key, a new resource policy
is created to allow the OIDC identity provider enough permissions to
fetch the API key.

It removes the `CoreFrontLocal` parameter in favour of defaulting to
this behavour for dev accounts. This is because changing back to a
private API from the new regional version is a two step process - so not
as easy as just toggling the parameter.

It's a two step process as you can't change from regional to private if
it has custom domains associated with it. In CloudFormation there isn't
a way to specify that something should be removed before making changes,
resulting in having to remove the custom domain before changing the API
to private.

Ideally I would have liked to have conditionally created the UsagePlan -
only for the dev accounts. However there appears to be a bug in cfn-lint
that means we can't use an If function with the CreateUsagePlan
property.

The GHA workflow included in this PR is just to demonstrate that the
gateway is now available with an API key, and also how to
programatically access the API key. This would not be merged with the
PR.
SAM allows an easy way to create a usage plan and API key, by defining
the `UsagePlan` property in an `Api` resource. Unfortunately you can't
use an `!If` function with it do conditionally create it.

This removes the use of it and defines the resources requied using
plain old CloudFormation. This allows us to only create them when we
want.

Using the previous method would have caused a usage plan and api key to
be created in all envs including prod. This felt like murky ground - the
non dev api's would still be private so it wouldn't have been an issue,
but feels safer to only create them when actually needed.
This creates a new API gateway to run in parallel with the original.
This means that the VPC protected internal API can still be used by
core-front without any changes, and the new regional, API token
protected API gateway can be used for testing.

cfn-lint has an issue with having an event defined in a lambda that
includes a reference to a conditionally created resource, even if that
resource is behind the same condition. This has lead to the ignore
written into the metadata of a number of resources.
This workflow was purely to demonstrate the new gateway was function and
that the API key could be retrieved.

Removing in a separate commit rather than squashing into the previous
commit to keep a record of the workflow in git in case we want to refer
to it.
Copy link

sonarqubecloud bot commented Jun 5, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@Wynndow Wynndow merged commit ec575e5 into main Jun 5, 2024
8 checks passed
@Wynndow Wynndow deleted the pyic-5872-open-gateway branch June 5, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants