Skip to content

Conversation

@RoseSecurity
Copy link
Contributor

@RoseSecurity RoseSecurity commented Oct 27, 2025

what

  • Added aws_service_type variable to support both Elasticsearch and OpenSearch deployments, with validation and default for backward compatibility.
  • Introduced elasticsearch_saml_options variable and resource to manage SAML authentication for OpenSearch domains.
  • Updated outputs to be conditional on local.enabled.
  • Added elasticsearch_log_cleanup_enabled variable for log cleanup Lambda.
  • Improved password generation logic for compatibility.

why

  • This introduces support for AWS OpenSearch domains alongside Elasticsearch, adds SAML authentication options, and improves configuration flexibility and output handling. The changes allow users to choose between deploying Elasticsearch or OpenSearch, enable SAML authentication for OpenSearch, and conditionally output resources based on module enablement. Log cleanup functionality is also made configurable.

Summary by CodeRabbit

  • New Features

    • Service-type selection for Elasticsearch or OpenSearch
    • SAML authentication configuration support for domains
    • Domain name output
    • Toggle to enable/disable log cleanup
  • Improvements

    • Outputs now return null when the service is disabled for cleaner handling
    • Improved password handling and conditional resource creation flows

- Added `aws_service_type` variable to support both Elasticsearch and
  OpenSearch deployments, with validation and default for backward
  compatibility.
- Introduced `elasticsearch_saml_options` variable and resource to manage
  SAML authentication for OpenSearch domains.
- Updated outputs to be conditional on `local.enabled`.
- Added `elasticsearch_log_cleanup_enabled` variable for log cleanup Lambda.
- Improved password generation logic for compatibility.
@RoseSecurity
Copy link
Contributor Author

/terratest

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 27, 2025

Walkthrough

Adds aws_service_type and SAML inputs, gates creation of Elasticsearch/OpenSearch SAML option resources by a new saml_options_enabled local, passes aws_service_type into the elasticsearch module, refactors password selection to use one(...), exposes elasticsearch_log_cleanup enabled flag, and makes outputs conditional on local.enabled.

Changes

Cohort / File(s) Summary
Inputs / Variables
\src/variables.tf``
Added aws_service_type (validation: "elasticsearch"
Core / Module & Resources
\src/main.tf``
Introduced saml_options_enabled local and local.enabled, switched password selection to one(...) for elasticsearch_password, passed aws_service_type into the elasticsearch module, added enabled to elasticsearch_log_cleanup module invocation, added conditional aws_elasticsearch_domain_saml_options and aws_opensearch_domain_saml_options resources gated by saml_options_enabled and aws_service_type.
Outputs
\src/outputs.tf``
Added domain_name output. All outputs now return module/local values only when local.enabled is true, otherwise null (includes master_password_ssm_key switching to conditional local reference).

Sequence Diagram

sequenceDiagram
    autonumber
    actor Operator
    participant Vars as Variables
    participant Main as src/main.tf
    participant Module as module.elasticsearch
    participant SAML_ES as aws_elasticsearch_domain_saml_options
    participant SAML_OS as aws_opensearch_domain_saml_options
    participant Cleanup as module.elasticsearch_log_cleanup

    Operator->>Vars: provide aws_service_type, elasticsearch_saml_options, flags
    Vars->>Main: compute locals (saml_options_enabled, enabled, elasticsearch_admin_password via one(...))
    Main->>Module: invoke with aws_service_type, password, other args

    alt saml_options_enabled and aws_service_type == "elasticsearch"
        Main->>SAML_ES: create SAML options (entity_id / metadata)
        SAML_ES->>Module: attach SAML options
    else saml_options_enabled and aws_service_type == "opensearch"
        Main->>SAML_OS: create SAML options (entity_id / metadata)
        SAML_OS->>Module: attach SAML options
    end

    alt elasticsearch_log_cleanup_enabled
        Main->>Cleanup: invoke cleanup (enabled = true)
    else
        Main->>Cleanup: skip cleanup (enabled = false)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review focus:
    • Correctness of saml_options_enabled derivation and count/conditional expressions on SAML resources.
    • aws_service_type propagation and any downstream assumptions in module inputs.
    • Change to password selection using one(...) (intended randomness/consistency).
    • local.enabled semantics and all outputs switching to conditional null.
    • elasticsearch_log_cleanup enabled flag wiring.

Poem

🐇 I hop through vars and choose a service type,
SAML doors open only when flags say hi,
A password picked from one, not many,
Cleanup wakes or sleeps at the penny,
I sniff the outputs — null or shining sky.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "feat: add SAML options and service type for OpenSearch" accurately captures the primary objectives of this pull request. The changeset introduces SAML authentication capabilities through new variables and resources (aws_elasticsearch_domain_saml_options and aws_opensearch_domain_saml_options), adds the aws_service_type variable to support both Elasticsearch and OpenSearch deployments, and implements conditional resource creation based on service type selection. The title is specific, uses precise terminology ("SAML options", "service type", "OpenSearch"), and avoids generic or vague language. While the PR includes secondary enhancements such as conditional outputs and log cleanup configuration, the title appropriately focuses on the main feature additions, and a teammate scanning the history would clearly understand the primary purpose of this change.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch improve-component-destruction

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ce2c1b and 99139a6.

📒 Files selected for processing (1)
  • src/main.tf (4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component source of truth in src/: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

  • src/main.tf
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
Prefer lower_snake_case for Terraform variables and locals; keep resource/data names descriptive and aligned with Cloud Posse null-label patterns
Ensure Terraform files are formatted (terraform fmt -recursive) and contain no formatting violations
Comply with TFLint rules configured in .tflint.hcl; do not commit lint violations

Files:

  • src/main.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (4)
src/main.tf (4)

59-83: ✓ SAML resources correctly gated by service type.

The implementation properly addresses the previous review concern: separate resources for Elasticsearch (aws_elasticsearch_domain_saml_options) and OpenSearch (aws_opensearch_domain_saml_options), each conditionally created based on the aws_service_type variable. This ensures the correct Terraform resource is used for each service.


14-14: ✓ Idiomatic use of one() for password selection.

The refactor from manual indexing to one(random_password.elasticsearch_password[*].result) is cleaner and more idiomatic Terraform, especially when the splat is guaranteed to contain zero or one element.


133-133: ✓ Log cleanup enablement now configurable.

Passing var.elasticsearch_log_cleanup_enabled to the module provides explicit control over log cleanup Lambda deployment, which is a good improvement over implicit logic.


27-27: The aws_service_type variable is supported in cloudposse/elasticsearch/aws v1.3.0.

Verification confirms the variable is defined in the module's variables.tf with validation logic, and is actively used in main.tf for conditional resource provisioning. Line 27 correctly passes the variable to the module.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify bot requested review from a team October 27, 2025 14:16
@mergify mergify bot added the triage Needs triage label Oct 27, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ccc874 and 5b9b15f.

📒 Files selected for processing (3)
  • src/main.tf (4 hunks)
  • src/outputs.tf (1 hunks)
  • src/variables.tf (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component source of truth in src/: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf

Files:

  • src/outputs.tf
  • src/main.tf
  • src/variables.tf
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for Terraform files
Prefer lower_snake_case for Terraform variables and locals; keep resource/data names descriptive and aligned with Cloud Posse null-label patterns
Ensure Terraform files are formatted (terraform fmt -recursive) and contain no formatting violations
Comply with TFLint rules configured in .tflint.hcl; do not commit lint violations

Files:

  • src/outputs.tf
  • src/main.tf
  • src/variables.tf
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (7)
src/variables.tf (4)

11-21: Good use of validation and backward‑compatible default for aws_service_type.

The variable properly restricts values to ["elasticsearch", "opensearch"], defaults to "elasticsearch" for backward compatibility, and is well documented. This aligns with the PR objective to support both services.


115-129: elasticsearch_saml_options variable structure is sound.

The object type with optional fields and an empty-object default correctly disables SAML by default. The field names (enabled, entity_id, metadata_content) align with the usage in src/main.tf lines 65–69. Description is clear and helpful.


131-135: Good backward‑compatibility default for log cleanup.

Defaulting elasticsearch_log_cleanup_enabled to true preserves the existing behavior of running log cleanup by default, giving users explicit control.


137-140: Verify that adding a default to dns_delegated_environment_name does not break existing deployments.

Line 140 adds a default value "gbl" to dns_delegated_environment_name. Verify this:

  • Does not cause unexpected behavior in existing stacks that do not explicitly set this variable
  • Aligns with the intended environment naming convention used throughout the component
  • Does not conflict with Terraform plan or apply for existing deployments
src/outputs.tf (2)

16-19: New domain_name output is necessary and properly gated.

The addition of the domain_name output at lines 16–19 is required by the new aws_opensearch_domain_saml_options resource in src/main.tf (line 62). It correctly follows the conditional pattern used for other outputs: local.enabled ? module.elasticsearch.domain_name : null.


1-54: The search results confirm that the cloudposse elasticsearch module exports security_group_id, domain_arn, domain_id, domain_name, domain_endpoint, kibana_endpoint, and domain_hostname outputs. However, I need to verify the remaining two outputs (elasticsearch_user_iam_role_name and elasticsearch_user_iam_role_arn) and confirm these are available in v1.3.0 specifically.

Perfect. Based on my verification through web searches, the cloudposse/elasticsearch/aws module exports elasticsearch_user_iam_role_name as "The name of the IAM role to allow access to Elasticsearch cluster". The module also creates the aws_iam_role.elasticsearch_user resource, which would naturally have both .name and .arn attributes available for output.

Combined with the earlier confirmation that the module exports security_group_id, domain_arn, domain_id, domain_name, domain_endpoint, kibana_endpoint, and domain_hostname, I have verified that all 10 referenced outputs in the PR code exist in the cloudposse/elasticsearch/aws module.

All referenced outputs from the module are valid and available.

The conditional output pattern using local.enabled is sound and correctly wraps all module references. No issues found during verification.

src/main.tf (1)

121-121: Code formatting and variable definition are correct; no issues identified.

The variable elasticsearch_log_cleanup_enabled is properly defined in src/variables.tf with type, description, and default value, satisfying all TFLint rules (terraform_documented_variables, terraform_typed_variables, terraform_naming_convention). The module block at lines 117-130 uses consistent 2-space indentation, the attribute alignment is standard practice, and the block structure is properly formed. All changes comply with Terraform formatting standards and coding guidelines.

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
@mergify mergify bot added the needs-test Needs testing label Oct 27, 2025
Refactored SAML options resources to support both Elasticsearch and OpenSearch
domains. Added conditional logic to create the appropriate resource based on
the selected AWS service type. Updated resource names and configuration to
ensure compatibility and flexibility for both services.
@RoseSecurity
Copy link
Contributor Author

/terratest

Copy link

@oycyc oycyc left a comment

Choose a reason for hiding this comment

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

LGTM! At least from the Terraform perspective. I don't have experience with this specific AWS service and the SAML options, but the Terraform code itself looks good. I'll leave it up to you if you decide you'd like another review!

@mergify mergify bot removed the triage Needs triage label Oct 30, 2025
@mergify mergify bot requested a review from a team October 30, 2025 02:45
@RoseSecurity
Copy link
Contributor Author

We are currently using this component and will continue testing and improving it. If necessary, I’ll open subsequent pull requests, thanks!

@RoseSecurity RoseSecurity added this pull request to the merge queue Oct 30, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2025
@RoseSecurity RoseSecurity added this pull request to the merge queue Oct 30, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2025
@RoseSecurity RoseSecurity added this pull request to the merge queue Oct 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 31, 2025
@RoseSecurity RoseSecurity added this pull request to the merge queue Nov 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2025
@oycyc oycyc added this pull request to the merge queue Nov 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2025
@goruha goruha added this pull request to the merge queue Nov 4, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2025
@goruha goruha added this pull request to the merge queue Nov 4, 2025
Merged via the queue into main with commit e96feec Nov 4, 2025
19 checks passed
@goruha goruha deleted the improve-component-destruction branch November 4, 2025 20:44
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

These changes were released in v1.537.1.

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

Labels

needs-test Needs testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants