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

Update Security Group module #45

Merged
merged 34 commits into from
Jun 8, 2022
Merged

Update Security Group module #45

merged 34 commits into from
Jun 8, 2022

Conversation

milldr
Copy link
Member

@milldr milldr commented Apr 22, 2022

what

  • Updated the Security Group module version and relevant config
  • Incorporated open PRs
  • Updated test framework

why

  • Unblocking further enhancements
  • Open PRs were based on incompatible pre-release versions
  • Test framework maintenance is not automated

notes

This PR introduces breaking changes and will be released as version 2.0. Migration document is here.

references

@milldr milldr requested a review from a team as a code owner April 22, 2022 22:37
@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

versions.tf Outdated Show resolved Hide resolved
@nitrocode
Copy link
Member

you'll need to deprecate the sg inputs from before the pre release versions

see cloudposse/terraform-aws-efs#94

@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

@milldr
Copy link
Member Author

milldr commented Apr 22, 2022

/test all

@Nuru Nuru self-requested a review April 23, 2022 00:57
Nuru
Nuru previously requested changes Apr 23, 2022
Copy link

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

Use security-group-variables.tf version 2, not security_group_inputs.tf version 1.

I may have other requests for changes on top of that.

variables-deprecated.tf Outdated Show resolved Hide resolved
@mergify mergify bot dismissed Nuru’s stale review April 25, 2022 16:31

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

@nitrocode
Copy link
Member

There is a missing migration doc. Do we need one here ? There are some modules that have one and others that do not, cc: @Nuru

@milldr
Copy link
Member Author

milldr commented Apr 27, 2022

/test all

@milldr
Copy link
Member Author

milldr commented Apr 27, 2022

There is a missing migration doc. Do we need one here ?

I've added that doc

@Gowiem
Copy link
Member

Gowiem commented May 17, 2022

@nitrocode @milldr Anything holding this one up? I've got an old client who it is blocking so I'd love to see it ship for them and make an old colleague of mine happy!

@milldr
Copy link
Member Author

milldr commented May 17, 2022

@Gowiem

@nitrocode @milldr Anything holding this one up? I've got an old client who it is blocking so I'd love to see it ship for them and make an old colleague of mine happy!

Yes, @Nuru is making several additional upgrades to the upstream modules before this PR can be merged. @Nuru do you have a status update for this PR?

@Nuru Nuru requested review from aknysh and nitrocode June 4, 2022 21:09
@Nuru Nuru added the major Breaking changes (or first stable release) label Jun 4, 2022
@Nuru
Copy link

Nuru commented Jun 4, 2022

/test all

docs/migration-v1-v2.md Outdated Show resolved Hide resolved
docs/migration-v1-v2.md Outdated Show resolved Hide resolved
docs/migration-v1-v2.md Outdated Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

var.security_groups
))
)
security_groups = local.broker_security_groups
Copy link

Choose a reason for hiding this comment

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

MEDIUM   Ensure Amazon MQ Broker logging is enabled
    Resource: aws_mq_broker.default | ID: BC_AWS_LOGGING_10

Description

Amazon MQ is a broker service built on Apache ActiveMQ. As a message broker, MQ allows applications to communicate using various programming languages, operating systems and formal messaging protocols.

Amazon MQ is integrated with CloudTrail and provides a record of the Amazon MQ calls made by a user, role, or AWS service. It supports logging both the request parameters and the responses for APIs as events in CloudTrail. Logging MQ ensures developers can trace all requests and responses, and ensure they are only used for their predefined message brokering settings.

We recommend you enable Amazon MQ Broker Logging.

Benchmarks

  • HIPAA 164.312(B) Audit controls

Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

⚠️   Due to 2ddf35c - Address reviewer comments - 1 new error was added

Change details

Error ID Change Path Resource
BC_AWS_LOGGING_10 Added /main.tf aws_mq_broker.default

var.security_groups
))
)
security_groups = local.broker_security_groups
Copy link

Choose a reason for hiding this comment

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

MEDIUM   Ensure Amazon MQ Broker logging is enabled
    Resource: module.mq_broker.aws_mq_broker.default | ID: BC_AWS_LOGGING_10

Description

Amazon MQ is a broker service built on Apache ActiveMQ. As a message broker, MQ allows applications to communicate using various programming languages, operating systems and formal messaging protocols.

Amazon MQ is integrated with CloudTrail and provides a record of the Amazon MQ calls made by a user, role, or AWS service. It supports logging both the request parameters and the responses for APIs as events in CloudTrail. Logging MQ ensures developers can trace all requests and responses, and ensure they are only used for their predefined message brokering settings.

We recommend you enable Amazon MQ Broker Logging.

Benchmarks

  • HIPAA 164.312(B) Audit controls

@Nuru
Copy link

Nuru commented Jun 5, 2022

/test all

@Nuru Nuru self-requested a review June 8, 2022 22:35
@Nuru Nuru merged commit 30e9cd5 into master Jun 8, 2022
@Nuru Nuru deleted the sg-update branch June 8, 2022 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Breaking changes (or first stable release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants