chore!: DBTP-1072 Change ADDITIONAL_IP_LIST to EGRESS_IPS #179
Conversation
| @@ -113,7 +113,7 @@ locals { | |||
| resource "aws_ssm_parameter" "combined_nat_gateway_eips" { | |||
| # checkov:skip=CKV_AWS_337:Ensure SSM parameters are using KMS CMK. Related ticket: https://uktrade.atlassian.net/browse/DBTP-946 | |||
| # checkov:skip=CKV2_AWS_34:AWS SSM Parameter should be Encrypted. Related ticket: https://uktrade.atlassian.net/browse/DBTP-946 | |||
| name = "/${var.arg_name}/ADDITIONAL_IP_LIST" | |||
| name = "/${var.arg_name}/EGRESS_IPS" | |||
There was a problem hiding this comment.
So this is a rename, but the contents have not changed.
I may have misunderstood, but I think @acodeninja said that ADDITIONAL_IP_LIST might contain other things. How would that get populated and where is it used?
There was a problem hiding this comment.
The offline command just needs to allow these egress ips access behind the maintenance page. ADDITIONAL_IP_LIST, as used by ip-filter, however, may contain more than just these egress ips. So if we used the ADDITIONAL_IP_LIST var for the holding page, we might inadvertently be allowing things to bypass the maintenance page, when we only want them bypassing ip-filter.
I think for now at least we have to go back to the old process of manually creating that ADDITIONAL_IP_LIST secret per env with whatever we need it to contain. But the default could be to just use the EGRESS_IPS var and only create the ADDITIONAL_IP_LIST param if requiring extra ips be whitelisted
There was a problem hiding this comment.
That makes sense.
Are you confident that this rename is not backwards breaking? I.e. it won't mess up IP Filter.
There was a problem hiding this comment.
I doubt many services will have moved over to this new setup yet, but I'm just testing how it works with creating the new var and whether it replaces the old one or adds another.
There was a problem hiding this comment.
I doubt many services will have moved over to this new setup yet
We would need to be 100% confident of that. Perhaps worth searching https://github.com/orgs/uktrade/repositories.
There was a problem hiding this comment.
I've just tested to confirm, but it definitely destroys the previous var, so it's a breaking change. I've added a release path and will update the docs repo
There was a problem hiding this comment.
docs change here #179
…I-and-a-frontend-service-in-the-same-environment-and-put-the-frontend-service-behind-the-IP-Filter-I-want-the-api-to-be-able-to-access-the-frontend-service
…I-and-a-frontend-service-in-the-same-environment-and-put-the-frontend-service-behind-the-IP-Filter-I-want-the-api-to-be-able-to-access-the-frontend-service
gabelton
changed the title
WillGibson
changed the title
gabelton
deleted the
DBTP-1072-As-a-developer-when-I-create-an-API-and-a-frontend-service-in-the-same-environment-and-put-the-frontend-service-behind-the-IP-Filter-I-want-the-api-to-be-able-to-access-the-frontend-service
branch
ip-filter expects an
ADDITIONAL_IP_LISTenv var. Often this list just contains the NAT gateway IPs generated in the vpc module, hence calling the tf modules var by the same name. But sometimesADDITIONAL_IP_LISTwill need to include other IPs and so we should differentiate between the two vars to avoid confusion.Upgrade path for release notes
If you only need the egress ips, you can update references to
<vpc_name>/ADDITIONAL_IP_LISTto<vpc_name>/EGRESS_IPS. However, if youripfiltersidecar instance needsADDITIONAL_IP_LISTenv var, we recommend manually setting this on a per environment basis withcopilot secret init.