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.

Sign up for GitHub

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

chore!: DBTP-1072 Change ADDITIONAL_IP_LIST to EGRESS_IPS #179

Merged
merged 7 commits into from
Jul 5, 2024
Merged

chore!: DBTP-1072 Change ADDITIONAL_IP_LIST to EGRESS_IPS #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6f24f3d
update ssm var
gabelton Jun 19, 2024
aff84cd
restore checkov skips
gabelton Jun 19, 2024
e698773
resolve merge conflicts in readme
gabelton Jul 1, 2024
7f71551
ADDITIONAL_IP_LIST --> EGRESS_IPS
gabelton Jul 4, 2024
9aac175
resolve merge conflicts
gabelton Jul 4, 2024
dad04cd
Merge branch 'main' into DBTP-1072-As-a-developer-when-I-create-an-AP…
gabelton Jul 5, 2024
82d8937
Merge branch 'main' into DBTP-1072-As-a-developer-when-I-create-an-AP…
gabelton Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vpc/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense.

Are you confident that this rename is not backwards breaking? I.e. it won't mess up IP Filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

docs change here #179

type = "String"
value = join(",", local.nat_gateway_eips)
tags = local.tags
Expand Down
4 changes: 2 additions & 2 deletions vpc/tests/unit.tftest.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ run "aws_vpc_unit_test" {

### nat_gateway_eip aws_ssm_parameter ###
assert {
condition = aws_ssm_parameter.combined_nat_gateway_eips.name == "/vpc-test-name/ADDITIONAL_IP_LIST"
error_message = "Should be: /vpc-test-name/ADDITIONAL_IP_LIST"
condition = aws_ssm_parameter.combined_nat_gateway_eips.name == "/vpc-test-name/EGRESS_IPS"
error_message = "Should be: /vpc-test-name/EGRESS_IPS"
}

assert {
Expand Down