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

chore(DMVP-1806): In api-gateway module resolve checkov errors #274

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ module "alb-logs-lambda" {
| [aws_s3_bucket.bucket](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket) | resource |
| [aws_s3_bucket_notification.logs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_notification) | resource |
| [aws_s3_bucket_policy.s3](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_policy) | resource |
| [aws_s3_bucket_public_access_block.access](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_public_access_block) | resource |
| [aws_caller_identity.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) | data source |
| [aws_elb_service_account.main](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/elb_service_account) | data source |
| [aws_s3_bucket.selected](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/s3_bucket) | data source |
Expand All @@ -72,7 +73,11 @@ module "alb-logs-lambda" {
| <a name="input_alb_log_bucket_prefix"></a> [alb\_log\_bucket\_prefix](#input\_alb\_log\_bucket\_prefix) | n/a | `string` | `""` | no |
| <a name="input_create_alb_log_bucket"></a> [create\_alb\_log\_bucket](#input\_create\_alb\_log\_bucket) | wether or no to create alb s3 logs bucket | `bool` | `true` | no |
| <a name="input_create_lambda"></a> [create\_lambda](#input\_create\_lambda) | n/a | `bool` | `true` | no |
| <a name="input_enabled"></a> [enabled](#input\_enabled) | enabled | `bool` | `false` | no |
| <a name="input_kms_key_id"></a> [kms\_key\_id](#input\_kms\_key\_id) | (Optional) The ARN of the KMS Key to use when encrypting log data. Please note, after the AWS KMS CMK is disassociated from the log group, AWS CloudWatch Logs stops encrypting newly ingested data for the log group. All previously ingested data remains encrypted, and AWS CloudWatch Logs requires permissions for the CMK whenever the encrypted data is requested. | `string` | `null` | no |
| <a name="input_logging"></a> [logging](#input\_logging) | logging | `list(any)` | `[]` | no |
| <a name="input_region"></a> [region](#input\_region) | Default region | `string` | `"us-east-1"` | no |
| <a name="input_sse_algorithm"></a> [sse\_algorithm](#input\_sse\_algorithm) | sse\_algorithm - (Required) The server-side encryption algorithm to use. Valid values are AES256 and aws:kms | `string` | `null` | no |

## Outputs

Expand Down
26 changes: 26 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/bucket.tf
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,32 @@ resource "aws_s3_bucket" "bucket" {
count = var.create_alb_log_bucket ? 1 : 0

bucket = var.alb_log_bucket_name
versioning {
enabled = var.enabled
}
server_side_encryption_configuration {
rule {
apply_server_side_encryption_by_default {
sse_algorithm = var.sse_algorithm
}
}
}
dynamic "logging" {
for_each = var.logging
content {
target_bucket = logging.value["target_bucket"]
target_prefix = "log/${var.alb_log_bucket_name}"
}
}
}

resource "aws_s3_bucket_public_access_block" "access" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let the users pass these "true" values dynamically and not have it hardcoded here. Also, you can set "true" the default values for those variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think no special need to have this options controller from outside as this access is for blocking public access to bucket always,
we have to decide and test to make sure this is ok, as I see this bucket should be accessible to write and read from lambda and cloudwatch and this services should be able to get access to do those actions?

bucket = aws_s3_bucket.bucket[0].id

block_public_acls = true
ignore_public_acls = true
restrict_public_buckets = true
block_public_policy = true
}

resource "aws_s3_bucket_policy" "s3" {
Expand Down
7 changes: 4 additions & 3 deletions modules/alb-logs-to-s3-to-cloudwatch/logs-to-cloudwatch.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ data "aws_s3_bucket" "selected" {
}

resource "aws_cloudwatch_log_group" "log" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest deleting this module from the repo, cause it's already exists in eks module.

count = var.create_lambda ? 1 : 0
name = "alb-${var.alb_log_bucket_name}"
retention_in_days = 365
count = var.create_lambda ? 1 : 0
name = "alb-${var.alb_log_bucket_name}"
kms_key_id = var.kms_key_id
retention_in_days = 365
}

module "alb_logs_to_cloudwatch" {
Expand Down
23 changes: 23 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/tests/basic/0-setup.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
terraform {
required_providers {
test = {
source = "terraform.io/builtin/test"
}

aws = {
source = "hashicorp/aws"
version = ">= 1.0.7"
}
}

required_version = ">= 1.0.7"
}

/**
* set the following env vars so that aws provider will get authenticated before apply:
export AWS_ACCESS_KEY_ID=xxxxxxxxxxxxxxxxxxxxxxxx
export AWS_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxx
*/
provider "aws" {
region = "us-east-1"
}
6 changes: 6 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/tests/basic/1-example.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module "alb-logs-lambda" {
source = "../../"
alb_log_bucket_name = "aws-cloudtrail-logs-565580475168-fb3dbb26"
account_id = "565580475168"
create_alb_log_bucket = false
}
9 changes: 9 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/tests/basic/2-assert.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
resource "test_assertions" "dummy" {
component = "this"

equal "scheme" {
description = "As module does not have any output and data just make sure the case runs. Probably can be thrown away."
got = "all good"
want = "all good"
}
}
36 changes: 36 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/tests/basic/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# basic

<!-- BEGINNING OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
## Requirements

| Name | Version |
|------|---------|
| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.0.7 |
| <a name="requirement_aws"></a> [aws](#requirement\_aws) | >= 1.0.7 |

## Providers

| Name | Version |
|------|---------|
| <a name="provider_test"></a> [test](#provider\_test) | n/a |

## Modules

| Name | Source | Version |
|------|--------|---------|
| <a name="module_alb-logs-lambda"></a> [alb-logs-lambda](#module\_alb-logs-lambda) | ../../ | n/a |

## Resources

| Name | Type |
|------|------|
| test_assertions.dummy | resource |

## Inputs

No inputs.

## Outputs

No outputs.
<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->
25 changes: 25 additions & 0 deletions modules/alb-logs-to-s3-to-cloudwatch/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,32 @@ variable "region" {
default = "us-east-1"
description = "Default region"
}

variable "account_id" {
type = string
default = ""
}

variable "kms_key_id" {
type = string
default = null
description = " (Optional) The ARN of the KMS Key to use when encrypting log data. Please note, after the AWS KMS CMK is disassociated from the log group, AWS CloudWatch Logs stops encrypting newly ingested data for the log group. All previously ingested data remains encrypted, and AWS CloudWatch Logs requires permissions for the CMK whenever the encrypted data is requested."
}

variable "sse_algorithm" {
type = string
default = null
description = "sse_algorithm - (Required) The server-side encryption algorithm to use. Valid values are AES256 and aws:kms"
}

variable "enabled" {
Copy link
Contributor

Choose a reason for hiding this comment

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

the name and description of variable is not descriptive,
at one look one can think that this option controls whether alb-log-to-s3-to-cloudwatch is enabled, but actually seems it just enables versioning on logs bucket,

please have descriptive name/descriptions for variables

type = bool
default = false
description = "enabled"
}

variable "logging" {
type = list(any)
default = []
description = "logging"
}
6 changes: 5 additions & 1 deletion modules/api-gateway/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,21 +191,25 @@ provider "aws" {
|------|-------------|------|---------|:--------:|
| <a name="input_access_logs_format"></a> [access\_logs\_format](#input\_access\_logs\_format) | The access logs format to sync into cloudwatch log group | `string` | `"{ \"requestId\":\"$context.requestId\", \"resourcePath\":\"$context.resourcePath\", \"httpMethod\":\"$context.httpMethod\", \"responseLength\":\"$context.responseLength\", \"responseLatency\":\"$context.responseLatency\", \"status\":\"$context.status\", \"protocol\":\"$context.protocol\", \"extendedRequestId\":\"$context.extendedRequestId\", \"ip\": \"$context.identity.sourceIp\", \"caller\":\"$context.identity.caller\", \"user\":\"$context.identity.user\", \"userAgent\":\"$context.identity.userAgent\", \"requestTime\":\"$context.requestTime\"}\n"` | no |
| <a name="input_body"></a> [body](#input\_body) | An OpenAPI/Sagger specification json string with description of paths/resources/methods, check AWS docs for more info: https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-import-api.html | `string` | `null` | no |
| <a name="input_cache_cluster_enabled"></a> [cache\_cluster\_enabled](#input\_cache\_cluster\_enabled) | Whether a cache cluster is enabled for the stage | `bool` | `true` | no |
| <a name="input_create_cloudwatch_log_role"></a> [create\_cloudwatch\_log\_role](#input\_create\_cloudwatch\_log\_role) | This allows to create cloudwatch role which is one per aws account and is not region specific | `bool` | `false` | no |
| <a name="input_create_iam_user"></a> [create\_iam\_user](#input\_create\_iam\_user) | Whether to create specific api access user to api gateway./[''871]. | `bool` | `true` | no |
| <a name="input_custom_domain_additional_options"></a> [custom\_domain\_additional\_options](#input\_custom\_domain\_additional\_options) | Additional route53 configs in this list for using along side to custom\_domain listing | <pre>list(list(object({<br> set_identifier = string<br> geolocation_routing_policy = any<br> })))</pre> | `[]` | no |
| <a name="input_custom_domains"></a> [custom\_domains](#input\_custom\_domains) | Allows to setup/attach custom domain to api gateway setup, it will create also r53 record and certificate. Note that all keys of object are required to pass when you need one | <pre>list(object({<br> name = string # this is just first/prefix/subdomain part of domain without zone part<br> zone_name = string<br> }))</pre> | `[]` | no |
| <a name="input_enable_access_logs"></a> [enable\_access\_logs](#input\_enable\_access\_logs) | Weather enable or not the access logs on stage | `bool` | `true` | no |
| <a name="input_enable_monitoring"></a> [enable\_monitoring](#input\_enable\_monitoring) | n/a | `bool` | `true` | no |
| <a name="input_endpoint_config_type"></a> [endpoint\_config\_type](#input\_endpoint\_config\_type) | API Gateway config type. Valid values: EDGE, REGIONAL or PRIVATE | `string` | `"REGIONAL"` | no |
| <a name="input_kms_key_id"></a> [kms\_key\_id](#input\_kms\_key\_id) | The ARN of the KMS Key to use when encrypting log data. Please note, after the AWS KMS CMK is disassociated from the log group, AWS CloudWatch Logs stops encrypting newly ingested data for the log group. All previously ingested data remains encrypted, and AWS CloudWatch Logs requires permissions for the CMK whenever the encrypted data is requested. | `string` | `null` | no |
| <a name="input_method_path"></a> [method\_path](#input\_method\_path) | n/a | `string` | `"*/*"` | no |
| <a name="input_monitoring_settings"></a> [monitoring\_settings](#input\_monitoring\_settings) | n/a | `map` | <pre>{<br> "data_trace_enabled": true,<br> "logging_level": "INFO",<br> "metrics_enabled": true,<br> "throttling_burst_limit": 50,<br> "throttling_rate_limit": 100<br>}</pre> | no |
| <a name="input_monitoring_settings"></a> [monitoring\_settings](#input\_monitoring\_settings) | n/a | `map` | <pre>{<br> "cache_data_encrypted": true,<br> "caching_enabled": true,<br> "data_trace_enabled": true,<br> "logging_level": "INFO",<br> "metrics_enabled": true,<br> "throttling_burst_limit": 50,<br> "throttling_rate_limit": 100<br>}</pre> | no |
| <a name="input_name"></a> [name](#input\_name) | The name of API gateway | `string` | n/a | yes |
| <a name="input_pgp_key"></a> [pgp\_key](#input\_pgp\_key) | Either a base-64 encoded PGP public key, or a keybase username in the form `keybase:username`. Used to encrypt password and access key. `pgp_key` is required when `create_iam_user_login_profile` is set to `true` | `string` | `null` | no |
| <a name="input_retention_in_days"></a> [retention\_in\_days](#input\_retention\_in\_days) | retention\_in\_days - (Optional) Specifies the number of days you want to retain log events in the specified log group. Possible values are: 1, 3, 5, 7, 14, 30, 60, 90, 120, 150, 180, 365, 400, 545, 731, 1827, 2192, 2557, 2922, 3288, 3653, and 0. If you select 0, the events in the log group are always retained and never expire. | `number` | `90` | no |
| <a name="input_root_resource_configs"></a> [root\_resource\_configs](#input\_root\_resource\_configs) | The methods/methods\_responses/integrations configs for root '/' resource, the key is HTTPS method like ANY/POST/GET | `any` | <pre>{<br> "POST": {<br> "api_key_required": true,<br> "authorization": "NONE",<br> "integration": {<br> "endpoint_uri": "https://www.google.de",<br> "integration_http_method": null,<br> "request_parameters": {<br> "integration.request.header.x-api-key": "method.request.header.x-api-key"<br> },<br> "type": "HTTP"<br> },<br> "request_parameters": {},<br> "response": {<br> "models": null,<br> "status_code": "200"<br> }<br> }<br>}</pre> | no |
| <a name="input_set_account_settings"></a> [set\_account\_settings](#input\_set\_account\_settings) | The account setting is important to have per account region level set before enabling logging as it have important setting set for cloudwatch role arn, also cloudwatch role should be created before enabling setting | `bool` | `false` | no |
| <a name="input_stage_name"></a> [stage\_name](#input\_stage\_name) | n/a | `string` | `"api-stage"` | no |
| <a name="input_usage_plan_values"></a> [usage\_plan\_values](#input\_usage\_plan\_values) | n/a | `any` | <pre>{<br> "quota_limit": 10000,<br> "quota_period": "MONTH",<br> "throttle_burst_limit": 1000,<br> "throttle_rate_limit": 500,<br> "usage_plan_description": "my description",<br> "usage_plan_name": "my-usage-plan"<br>}</pre> | no |
| <a name="input_xray_tracing_enabled"></a> [xray\_tracing\_enabled](#input\_xray\_tracing\_enabled) | Whether active tracing with X-ray is enabled. Defaults to false | `bool` | `true` | no |

## Outputs

Expand Down
3 changes: 2 additions & 1 deletion modules/api-gateway/cloudwatch.tf
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
resource "aws_cloudwatch_log_group" "access_logs" {
count = var.enable_access_logs ? 1 : 0

name = "api-gateway-${var.name}-${var.stage_name}-logs"
name = "api-gateway-${var.name}-${var.stage_name}-logs"
retention_in_days = var.retention_in_days
}
2 changes: 2 additions & 0 deletions modules/api-gateway/custom-domain/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ No requirements.
| <a name="input_api_id"></a> [api\_id](#input\_api\_id) | The API Gateway id | `string` | n/a | yes |
| <a name="input_custom_domain_additional_options"></a> [custom\_domain\_additional\_options](#input\_custom\_domain\_additional\_options) | Additional route53 configs in this list for using along side to custom\_domain listing | <pre>list(list(object({<br> set_identifier = string<br> geolocation_routing_policy = any<br> })))</pre> | `[]` | no |
| <a name="input_custom_domains"></a> [custom\_domains](#input\_custom\_domains) | Allows to setup/attach custom domain to api gateway setup, it will create also r53 record and certificate. Note that all keys of object are required to pass when you need one | <pre>list(object({<br> name = string # this is just first/prefix/subdomain part of domain without zone part<br> zone_name = string<br> }))</pre> | `[]` | no |
| <a name="input_enabled"></a> [enabled](#input\_enabled) | (Optional, Deprecated) A configuration of the S3 bucket versioning state. See Versioning below for details. Terraform will only perform drift detection if a configuration value is provided. Use the resource aws\_s3\_bucket\_versioning instead. | `bool` | `true` | no |
| <a name="input_endpoint_config_type"></a> [endpoint\_config\_type](#input\_endpoint\_config\_type) | API Gateway config type. Valid values: EDGE, REGIONAL or PRIVATE | `string` | `"REGIONAL"` | no |
| <a name="input_security_policy"></a> [security\_policy](#input\_security\_policy) | (Optional) Transport Layer Security (TLS) version + cipher suite for this DomainName. Valid values are TLS\_1\_0 and TLS\_1\_2. Must be configured to perform drift detection. | `string` | `"TLS_1_2"` | no |
| <a name="input_stage_name"></a> [stage\_name](#input\_stage\_name) | The API Gateway stage name | `string` | n/a | yes |

## Outputs
Expand Down
2 changes: 1 addition & 1 deletion modules/api-gateway/custom-domain/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ resource "aws_api_gateway_domain_name" "custom_domains" {
regional_certificate_arn = try(module.certificate_regional[each.key].arn, null)
certificate_arn = try(module.certificate_edge[each.key].arn, null)
domain_name = each.key

security_policy = var.security_policy
0katrinpetrosyan0 marked this conversation as resolved.
Show resolved Hide resolved
endpoint_configuration {
types = [var.endpoint_config_type]
}
Expand Down
6 changes: 6 additions & 0 deletions modules/api-gateway/custom-domain/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,9 @@ variable "endpoint_config_type" {
default = "REGIONAL"
description = "API Gateway config type. Valid values: EDGE, REGIONAL or PRIVATE"
}

variable "security_policy" {
type = string
default = "TLS_1_2"
description = "(Optional) Transport Layer Security (TLS) version + cipher suite for this DomainName. Valid values are TLS_1_0 and TLS_1_2. Must be configured to perform drift detection."
}
14 changes: 11 additions & 3 deletions modules/api-gateway/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ resource "aws_api_gateway_rest_api" "this" {
endpoint_configuration {
types = [var.endpoint_config_type]
}
lifecycle {
create_before_destroy = true
}
}

# root resource methods configs
Expand Down Expand Up @@ -65,8 +68,11 @@ resource "aws_api_gateway_integration_response" "root_methods_integration_respon
resource "aws_api_gateway_stage" "stage" {
stage_name = var.stage_name

deployment_id = aws_api_gateway_deployment.deployment.id
rest_api_id = aws_api_gateway_rest_api.this.id
deployment_id = aws_api_gateway_deployment.deployment.id
rest_api_id = aws_api_gateway_rest_api.this.id
xray_tracing_enabled = var.xray_tracing_enabled
cache_cluster_enabled = var.cache_cluster_enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

It'll be better to have a map of these 2 variables.

cache_cluster_size = var.cache_cluster_size

dynamic "access_log_settings" {
for_each = aws_cloudwatch_log_group.access_logs
Expand Down Expand Up @@ -133,8 +139,10 @@ resource "aws_api_gateway_method_settings" "general_settings" {
# Limit the rate of calls to prevent abuse and unwanted charges
throttling_rate_limit = var.monitoring_settings.throttling_rate_limit
throttling_burst_limit = var.monitoring_settings.throttling_burst_limit
}

caching_enabled = var.monitoring_settings.caching_enabled
cache_data_encrypted = var.monitoring_settings.cache_data_encrypted
}
depends_on = [
module.account_settings,
aws_api_gateway_stage.stage
Expand Down
23 changes: 23 additions & 0 deletions modules/api-gateway/tests/basic/0-setup.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
terraform {
required_providers {
test = {
source = "terraform.io/builtin/test"
}

aws = {
source = "hashicorp/aws"
version = ">= 1.0.7"
}
}

required_version = ">= 1.0.7"
}

/**
* set the following env vars so that aws provider will get authenticated before apply:
export AWS_ACCESS_KEY_ID=xxxxxxxxxxxxxxxxxxxxxxxx
export AWS_SECRET_ACCESS_KEY=xxxxxxxxxxxxxxxxxxxxxxxx
*/
provider "aws" {
region = "eu-central-1"
}
40 changes: 40 additions & 0 deletions modules/api-gateway/tests/basic/1-example.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
module "api_gateway" {
source = "../../"

name = "api_gw"
endpoint_config_type = "REGIONAL"
stage_name = "api-stage"

root_resource_configs = {
ANY = {
authorization = "NONE"
api_key_required = true

integration = {
type = "HTTP"
endpoint_uri = "https://www.google.de"
integration_http_method = "ANY"
request_parameters = { "integration.request.header.x-api-key" = "method.request.header.x-api-key" }
}
}
}

usage_plan_values = {}

providers = {
aws.virginia = aws.virginia
}
}

provider "aws" {
alias = "virginia"
region = "us-east-1"
}

output "access_key_id" {
value = module.api_gateway.access_key_id
}

output "access_secret_key" {
value = nonsensitive(module.api_gateway.access_secret_key)
}
9 changes: 9 additions & 0 deletions modules/api-gateway/tests/basic/2-assert.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
resource "test_assertions" "dummy" {
component = "this"

equal "scheme" {
description = "As module does not have any output and data just make sure the case runs. Probably can be thrown away."
got = "all good"
want = "all good"
}
}
Loading