Skip to content

Conversation

@korenyoni
Copy link
Contributor

@korenyoni korenyoni commented Oct 26, 2021

what

why

  • The new Security Group standards include an input var.associated_security_group_ids which allows additional security groups to be associated with the MSK brokers, removing the need for var.broker_node_security_groups, hence why Feat: Add Broker Node Security Groups #41 is superseded by this PR.
  • The Security Groups needs to be restricted based on which protocols are enabled.
  • Abstract unnecessary SG-related logic (especially present when dynamically enabling or disabling various protocols for both SG-based and CIDR-based SG rules) via SG module.

references

@korenyoni korenyoni changed the title Add broker_node_security_groups Feat: Use Security Group Module Oct 26, 2021
@korenyoni korenyoni changed the title Feat: Use Security Group Module Feat: Use Security Group Module; Restrict MSK Ingress Based on Enabled Protocols Oct 26, 2021
@korenyoni korenyoni added the enhancement New feature or request label Oct 26, 2021
@korenyoni korenyoni requested review from Nuru and nitrocode October 26, 2021 14:29
@korenyoni korenyoni marked this pull request as ready for review October 26, 2021 14:29
@korenyoni korenyoni requested review from a team as code owners October 26, 2021 14:29
@korenyoni korenyoni requested a review from Gowiem October 26, 2021 14:29
@korenyoni
Copy link
Contributor Author

/test all

@korenyoni korenyoni force-pushed the feat/security-group-module branch from bd52042 to dba04fc Compare October 26, 2021 14:50
@korenyoni
Copy link
Contributor Author

/test all

1 similar comment
@korenyoni
Copy link
Contributor Author

/test all

@korenyoni korenyoni force-pushed the feat/security-group-module branch from 501eb19 to df562b9 Compare October 26, 2021 16:15
@korenyoni
Copy link
Contributor Author

/test all

@korenyoni
Copy link
Contributor Author

/test all

Nuru
Nuru previously requested changes Oct 28, 2021
variable "security_group_create_before_destroy" {
type = bool

default = false
Copy link
Contributor

Choose a reason for hiding this comment

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

This should default to true. Only default to false if it would successfully allow a non-breaking upgrade. In this case, default to true and if needed (which it probably is), add an explicit instruction to set it to false in the migration document.

Suggested change
default = false
default = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nuru I was basing this on the instructions here:

https://github.com/cloudposse/terraform-aws-security-group/blob/c6e4156696ee28cad0cd927c82377fbd73156199/exports/security_group_inputs.tf#L71-L72

If the resource has to be deleted to change its security group,
then set the default to false and highlight the option to change
it to true in the release notes and migration documents.

@mergify mergify bot dismissed Nuru’s stale review November 3, 2021 13:51

This Pull Request has been updated, so we're dismissing all reviews.

@korenyoni korenyoni force-pushed the feat/security-group-module branch from f56a83b to 1c5e101 Compare November 3, 2021 14:28
@korenyoni korenyoni requested a review from Nuru November 3, 2021 14:28
@korenyoni
Copy link
Contributor Author

/test all

@korenyoni
Copy link
Contributor Author

/test all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restrict security group to tls port 9094 input variable security_groups is not being utilized

5 participants