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

Add variable for publishing egress IP addresses #240

Merged
merged 6 commits into from
May 15, 2024

Conversation

dav3r
Copy link
Member

@dav3r dav3r commented May 14, 2024

🗣 Description

This PR adds a new Terraform variable (publish_egress_ip_addresses) that specifies whether EC2 instances in the operations subnet should be tagged to indicate that their public IP addresses may be published.

💭 Motivation and context

This is useful for deconfliction purposes and it allows us to control publishing of IP addresses on a per-assessment (per-Terraform workspace) basis, which is a requirement of https://github.com/cisagov/cool-system-internal/issues/125.

IP address publishing will be handled via the code in cisagov/publish-egress-ip-lambda and cisagov/publish-egress-ip-terraform.

🧪 Testing

I tested this by running a terraform plan for an environment with:

  • publish_egress_ip_addresses = true
  • No value set for publish_egress_ip_addresses

In the first case, I confirmed that the plan wanted to set "Publish Egress" = "true" for all of the correct instances/EIPs.
In the second case, I confirmed that the plan wanted to set "Publish Egress" = "false" for all of the correct instances/EIPs.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • All new and existing tests pass.

✅ Post-merge checklist

  • Coordinate with the COOL Operations team to ensure that this new variable is used correctly.
  • Update all Staging and Production tfvars files to include publish_egress_ip_addresses

dav3r added 5 commits May 13, 2024 14:29
It is a boolean value that specifies whether EC2 instances in the operations subnet should be tagged to indicate that their public IP addresses may be published.
@dav3r dav3r added the improvement This issue or pull request will add or improve functionality, maintainability, or ease of use label May 14, 2024
@dav3r dav3r self-assigned this May 14, 2024
@dav3r dav3r marked this pull request as ready for review May 14, 2024 15:07
@dav3r dav3r requested a review from a team May 14, 2024 17:09
Copy link
Member

@mcdonnnj mcdonnnj 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 pretty solid. In addition to my one feedback item I would like to raise maybe making an issue to expand functionality to allow control over which instances of a given type have their IP information published e.g. I want Windows0 to be private but Windows1 to be published.

debiandesktop_ec2.tf Show resolved Hide resolved
pentestportal_ec2.tf Outdated Show resolved Hide resolved
@dav3r
Copy link
Member Author

dav3r commented May 15, 2024

This seems pretty solid. In addition to my one feedback item I would like to raise maybe making an issue to expand functionality to allow control over which instances of a given type have their IP information published e.g. I want Windows0 to be private but Windows1 to be published.

@mcdonnnj That's a good idea and we should keep it in mind, but until someone comes to us with that as a requirement, I don't think we need to worry too much about it.

Do I have your approval on this PR?

dav3r added a commit to cisagov/publish-egress-ip-lambda that referenced this pull request May 15, 2024
This change aligns with the code that sets this tag in cisagov/cool-assessment-terraform (see cisagov/cool-assessment-terraform#240).
dav3r added a commit to cisagov/publish-egress-ip-lambda that referenced this pull request May 15, 2024
This change aligns with the code that sets this tag in cisagov/cool-assessment-terraform (see cisagov/cool-assessment-terraform#240).
@mcdonnnj
Copy link
Member

This seems pretty solid. In addition to my one feedback item I would like to raise maybe making an issue to expand functionality to allow control over which instances of a given type have their IP information published e.g. I want Windows0 to be private but Windows1 to be published.

@mcdonnnj That's a good idea and we should keep it in mind, but until someone comes to us with that as a requirement, I don't think we need to worry too much about it.

Do I have your approval on this PR?

Yeah I meant it as a "feature idea" issue just to document the thought.

Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

LGTM ✔

@dav3r
Copy link
Member Author

dav3r commented May 15, 2024

This seems pretty solid. In addition to my one feedback item I would like to raise maybe making an issue to expand functionality to allow control over which instances of a given type have their IP information published e.g. I want Windows0 to be private but Windows1 to be published.

@mcdonnnj That's a good idea and we should keep it in mind, but until someone comes to us with that as a requirement, I don't think we need to worry too much about it.
Do I have your approval on this PR?

Yeah I meant it as a "feature idea" issue just to document the thought.

Understood - I put this idea in #242. Feel free to make any edits as you see fit.

@dav3r dav3r merged commit 26dd13b into develop May 15, 2024
5 checks passed
@dav3r dav3r deleted the improvement/add-publish-egress-ip-variable branch May 15, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants