-
Notifications
You must be signed in to change notification settings - Fork 2
ELI-578 consumer_id - campaign_config mapping #504
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
Changes from all commits
09ffcff
dfcff79
18736fa
b9fe63b
bcd1d8d
60b4954
d840389
b7561a0
aefbe33
8f4e28a
4afb122
2dacd5e
72f6d0c
bcee4b1
6029398
714433d
fcc68be
510df8d
91a2a09
4089abc
1279f2d
425f6e7
3b58fc9
7812be0
569dc5f
fbb902d
d30b4df
6591256
bca6215
5160d98
92dd24e
f25e29e
80704fe
5407820
ca0aef8
3f30502
7751ae8
b6b9242
4fea2dc
34827d1
3d1b970
a14db73
1b10f85
45522f8
c507ee2
926edc8
cbbae6f
f525d01
fa80df6
dbef36c
82ee079
2045e08
5fff049
bcadb02
6eaca30
a1ad018
6c70813
3d6b127
8afefa9
34073c6
bd8b5c2
0e6c784
fe9a2da
e7d8862
f513f31
769989b
f5a52ca
68f2cca
3435844
21c36bd
afcd901
815b435
6965840
72f41b0
6fee706
b2a1171
9b3bab6
3c9a534
c18ff42
d681b59
37c919d
684b971
a4ee06d
e0da30d
1d05812
230de1d
baf882b
54482e2
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 |
|---|---|---|
|
|
@@ -104,6 +104,60 @@ | |
| } | ||
| } | ||
|
|
||
| # Policy doc for S3 Consumer Mappings bucket | ||
| data "aws_iam_policy_document" "s3_consumer_mapping_bucket_policy" { | ||
|
Collaborator
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. lambda_consumer_mapping_read_policy might be clearer? But probably needs a wider revisit to ensure all our terraform names are sensible / explain what the purpose it - a lot of my stuff from early on is likely confusing to me now.... |
||
| statement { | ||
| sid = "AllowSSLRequestsOnly" | ||
| actions = [ | ||
| "s3:GetObject", | ||
| "s3:ListBucket", | ||
| ] | ||
| resources = [ | ||
| module.s3_consumer_mappings_bucket.storage_bucket_arn, | ||
| "${module.s3_consumer_mappings_bucket.storage_bucket_arn}/*", | ||
| ] | ||
| condition { | ||
| test = "Bool" | ||
| values = ["true"] | ||
| variable = "aws:SecureTransport" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # ensure only secure transport is allowed | ||
|
|
||
| resource "aws_s3_bucket_policy" "consumer_mapping_s3_bucket" { | ||
| bucket = module.s3_consumer_mappings_bucket.storage_bucket_id | ||
| policy = data.aws_iam_policy_document.consumer_mapping_s3_bucket_policy.json | ||
| } | ||
|
|
||
| data "aws_iam_policy_document" "consumer_mapping_s3_bucket_policy" { | ||
| statement { | ||
| sid = "AllowSslRequestsOnly" | ||
| actions = [ | ||
| "s3:*", | ||
| ] | ||
| effect = "Deny" | ||
| resources = [ | ||
| module.s3_consumer_mappings_bucket.storage_bucket_arn, | ||
| "${module.s3_consumer_mappings_bucket.storage_bucket_arn}/*", | ||
| ] | ||
| principals { | ||
| type = "*" | ||
| identifiers = ["*"] | ||
| } | ||
| condition { | ||
| test = "Bool" | ||
| values = [ | ||
| "false", | ||
| ] | ||
|
|
||
| variable = "aws:SecureTransport" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # audit bucket | ||
| resource "aws_s3_bucket_policy" "audit_s3_bucket" { | ||
| bucket = module.s3_audit_bucket.storage_bucket_id | ||
| policy = data.aws_iam_policy_document.audit_s3_bucket_policy.json | ||
|
|
@@ -136,12 +190,18 @@ | |
| } | ||
|
|
||
| # Attach s3 read policy to Lambda role | ||
| resource "aws_iam_role_policy" "lambda_s3_read_policy" { | ||
| resource "aws_iam_role_policy" "lambda_s3_rules_read_policy" { | ||
| name = "S3ReadAccess" | ||
| role = aws_iam_role.eligibility_lambda_role.id | ||
| policy = data.aws_iam_policy_document.s3_rules_bucket_policy.json | ||
| } | ||
|
|
||
| resource "aws_iam_role_policy" "lambda_s3_mapping_read_policy" { | ||
| name = "S3ConsumerMappingReadAccess" | ||
| role = aws_iam_role.eligibility_lambda_role.id | ||
| policy = data.aws_iam_policy_document.s3_consumer_mapping_bucket_policy.json | ||
| } | ||
|
|
||
| # Attach s3 write policy to kinesis firehose role | ||
| resource "aws_iam_role_policy" "kinesis_firehose_s3_write_policy" { | ||
| name = "S3WriteAccess" | ||
|
|
@@ -290,6 +350,41 @@ | |
| policy = data.aws_iam_policy_document.s3_rules_kms_key_policy.json | ||
| } | ||
|
|
||
| data "aws_iam_policy_document" "s3_consumer_mapping_kms_key_policy" { | ||
| #checkov:skip=CKV_AWS_111: Root user needs full KMS key management | ||
| #checkov:skip=CKV_AWS_356: Root user needs full KMS key management | ||
| #checkov:skip=CKV_AWS_109: Root user needs full KMS key management | ||
| statement { | ||
| sid = "EnableIamUserPermissions" | ||
| effect = "Allow" | ||
| principals { | ||
| type = "AWS" | ||
| identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"] | ||
| } | ||
| actions = ["kms:*"] | ||
| resources = ["*"] | ||
| } | ||
|
|
||
| #checkov:skip=CKV_AWS_111: Permission boundary enforces restrictions for this policy | ||
| #checkov:skip=CKV_AWS_356: Permission boundary enforces resource-level controls | ||
| #checkov:skip=CKV_AWS_109: Permission boundary governs write-access constraints | ||
| statement { | ||
| sid = "AllowLambdaDecrypt" | ||
| effect = "Allow" | ||
| principals { | ||
| type = "AWS" | ||
| identifiers = [aws_iam_role.eligibility_lambda_role.arn] | ||
| } | ||
| actions = ["kms:Decrypt"] | ||
| resources = ["*"] | ||
| } | ||
| } | ||
|
|
||
| resource "aws_kms_key_policy" "s3_consumer_mapping_kms_key" { | ||
| key_id = module.s3_consumer_mappings_bucket.storage_bucket_kms_key_id | ||
| policy = data.aws_iam_policy_document.s3_consumer_mapping_kms_key_policy.json | ||
| } | ||
|
|
||
| resource "aws_iam_role_policy" "splunk_firehose_policy" { | ||
| #checkov:skip=CKV_AWS_290: Firehose requires write access to dynamic log streams without static constraints | ||
| #checkov:skip=CKV_AWS_355: Firehose logging requires wildcard resource for CloudWatch log groups/streams | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| from typing import NewType | ||
|
|
||
| from pydantic import BaseModel, Field, RootModel | ||
|
|
||
| from eligibility_signposting_api.model.campaign_config import CampaignID | ||
|
|
||
| ConsumerId = NewType("ConsumerId", str) | ||
|
|
||
|
|
||
| class ConsumerCampaign(BaseModel): | ||
| campaign_config_id: CampaignID = Field(alias="CampaignConfigID") | ||
| description: str | None = Field(default=None, alias="Description") | ||
|
|
||
|
|
||
| class ConsumerMapping(RootModel[dict[ConsumerId, list[ConsumerCampaign]]]): | ||
| def get(self, key: ConsumerId, default: list[ConsumerCampaign] | None = None) -> list[ConsumerCampaign] | None: | ||
| return self.root.get(key, default) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| import json | ||
| from typing import Annotated, NewType | ||
|
|
||
| from botocore.client import BaseClient | ||
| from wireup import Inject, service | ||
|
|
||
| from eligibility_signposting_api.model.campaign_config import CampaignID | ||
| from eligibility_signposting_api.model.consumer_mapping import ConsumerId, ConsumerMapping | ||
|
|
||
| BucketName = NewType("BucketName", str) | ||
|
|
||
|
|
||
| @service | ||
| class ConsumerMappingRepo: | ||
| """Repository class for Consumer Mapping""" | ||
|
|
||
| def __init__( | ||
| self, | ||
| s3_client: Annotated[BaseClient, Inject(qualifier="s3")], | ||
| bucket_name: Annotated[BucketName, Inject(param="consumer_mapping_bucket_name")], | ||
| ) -> None: | ||
| super().__init__() | ||
| self.s3_client = s3_client | ||
| self.bucket_name = bucket_name | ||
|
|
||
| def get_permitted_campaign_ids(self, consumer_id: ConsumerId) -> list[CampaignID] | None: | ||
|
Collaborator
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. it's probably worth caching the consumer config, it's likely to be v. slow changing
Contributor
Author
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. We can look into this along with the "nhs-product-id" implementation userstory.
Contributor
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. It is a good idea to look at caching for both consumer mapping, and campaign config. We also have to think about cache invalidation, as we don't want some warm lambda running with old config, and new lambdas running new config.
Collaborator
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. More generally, an alternative might be a new dynamoDB table - latency would hopefully be much lower as single key lookups are v speedy vs. s3
Collaborator
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. But we might look at this in terms of revisiting where we store campaign config too
Contributor
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 think this is an interesting alternative, |
||
| objects = self.s3_client.list_objects(Bucket=self.bucket_name).get("Contents") | ||
|
Collaborator
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. Couldn't we set the filename as a constant (e.g. consumer_mapping.json) and avoid this list object (e.g. just do the get?)
Contributor
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. We have followed the same pattern that has been used for campaign config. I think, we will need to revisit this as part of a review on the management of consumer mapping and campaign config.
Collaborator
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. Agree this whole thing might need a rethink. We're likely adding another 50-100ms of latency with this MR, so still just about sits under the arbitrary 200ms. 'do we want all consumers mapped in one file, or easier to manage one file per consumer? etc.' - at the moment the approach only supports the former, so your choice is to stick with the list_objects latency and rework if we want multiple files (one per consumer) or go with the single file approach and then rework to support multiple files? I think we'd want a fast following ticket to resolve this if we leave it as is
Collaborator
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. (my temptation is read in one file into memory and deal with it - we can be smart about managing it e.g. have a mechanism to independently manage each consumer and generate a single file) |
||
|
|
||
| if not objects: | ||
| return None | ||
|
|
||
| consumer_mappings_obj = objects[0] | ||
| response = self.s3_client.get_object(Bucket=self.bucket_name, Key=consumer_mappings_obj["Key"]) | ||
| body = response["Body"].read() | ||
|
|
||
| mapping_result = ConsumerMapping.model_validate(json.loads(body)).get(consumer_id) | ||
|
|
||
| if mapping_result is None: | ||
| return None | ||
|
|
||
| return [item.campaign_config_id for item in mapping_result] | ||
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.
CONSUMER_MAPPING_BUCKET_NAME = var.eligibility_consumer_mappings_bucket_name is the change here. Rest are just lint (spacings)