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

feat: new AWS IAM module #82

Open
wants to merge 11 commits into
base: rc
Choose a base branch
from
Open

feat: new AWS IAM module #82

wants to merge 11 commits into from

Conversation

lstadnik
Copy link
Contributor

@lstadnik lstadnik commented Oct 4, 2024

Description

The PR introduces new module for AWS repo - It supports pre-defined collection of used IAM policies and custom made policies as well.

Motivation and Context

#53 issue presents required changes for major refactor of existing code.

How Has This Been Tested?

The IAM modules should be tested against all examples.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes if appropriate.
  • All new and existing tests passed.

@lstadnik lstadnik requested a review from a team as a code owner October 4, 2024 11:16
@lstadnik lstadnik marked this pull request as draft October 4, 2024 11:19
modules/iam/README.md Outdated Show resolved Hide resolved
modules/iam/main.tf Outdated Show resolved Hide resolved
modules/iam/main.tf Show resolved Hide resolved
account_id = data.aws_caller_identity.this.account_id
delicense_param = try(startswith(var.delicense_ssm_param_name, "/") ? var.delicense_ssm_param_name : "/${var.delicense_ssm_param_name}", null)

lambda_execute_policy = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lambda_execute_policy = {
lambda_execute_policies = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, from AWS perspective - it is a one policy with many statements

}
}

lambda_delicense_policy = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lambda_delicense_policy = {
lambda_delicense_policies = {

default = false
}

variable "create_bootrap_policy" {
Copy link
Contributor

Choose a reason for hiding this comment

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

As with instance profile - proposal to either merge this into a single var or rely on bucket name.

modules/iam/main.tf Outdated Show resolved Hide resolved
modules/iam/main.tf Outdated Show resolved Hide resolved
modules/iam/main.tf Outdated Show resolved Hide resolved
modules/iam/main.tf Show resolved Hide resolved
Copy link
Contributor Author

@lstadnik lstadnik left a comment

Choose a reason for hiding this comment

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

Thanks @michalbil for suggestion - "plurals" and "create" we can rediscuss offline

modules/iam/main.tf Outdated Show resolved Hide resolved
Comment on lines 28 to 38
variable "create_instance_profile" {
description = "Create an instance profile."
type = bool
default = false
}

variable "profile_instance_name" {
description = "A profile instance name."
type = string
default = null
}
Copy link
Contributor Author

@lstadnik lstadnik Oct 24, 2024

Choose a reason for hiding this comment

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

I think - using create variable - gives more flexibility. The module can use data source for existing instance_profile_name = 'name' - The name providing only is less initiative and also different modules are using this approach.

modules/iam/variables.tf Outdated Show resolved Hide resolved
@lstadnik lstadnik requested a review from michalbil October 30, 2024 14:52
@lstadnik lstadnik marked this pull request as ready for review October 31, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants