-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add advanced security and domain config options #69
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
base: main
Are you sure you want to change the base?
Conversation
- Added variables for advanced security options, including anonymous auth, internal user database, and master user name, allowing more flexible Elasticsearch/Kibana security configuration. - Introduced variables for Elasticsearch domain name and subdomain names, with validation for domain name format. - Added support for enabling cold storage and node-to-node encryption via new variables. - Updated module usage to reference new and updated variables.
WalkthroughIntroduces new variables and local subdomain names, updates the elasticsearch module wiring to use those locals and variables (including cold storage and multiple advanced security and encryption flags), and adjusts a test expectation for the domain hostname. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Caller (module consumer)
participant Loc as locals (src/main.tf)
participant ES as module.elasticsearch
Dev->>Loc: provide inputs (vars & defaults)
Note right of Loc: compute\nkibana_subdomain_name\nelasticsearch_subdomain_name\n(using coalesce)
Loc->>ES: pass wired inputs\n- elasticsearch_domain_name\n- elasticsearch_subdomain_name\n- kibana_subdomain_name\n- cold_storage_enabled\n- node_to_node_encryption_enabled\n- advanced_security_* flags
ES->>ES: configure domain with provided flags
ES-->>Dev: provisioned Elasticsearch domain metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
/terratest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/variables.tf (1)
96-99: elasticsearch_subdomain_name should havedefault = nullfor consistency with kibana_subdomain_name.Currently,
elasticsearch_subdomain_name(line 96) is a required variable with no default. However,main.tf(line 13) usescoalesce(var.elasticsearch_subdomain_name, module.this.environment)to provide a fallback to the environment name. This fallback logic never executes because the variable is required.For consistency with
kibana_subdomain_name(which now hasdefault = nullat line 104), adddefault = nullto makeelasticsearch_subdomain_nameoptional:variable "elasticsearch_subdomain_name" { type = string description = "The name of the subdomain for Elasticsearch in the DNS zone (_e.g._ `elasticsearch`, `ui`, `ui-es`, `search-ui`)" + default = null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main.tf(2 hunks)src/variables.tf(3 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.tfsrc/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/main.tfsrc/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 (4)
src/main.tf (2)
12-13: New subdomain locals use consistent coalesce pattern. ✓The coalesce approach allows optional override via
var.kibana_subdomain_nameandvar.elasticsearch_subdomain_namewhile falling back tomodule.this.environment. This aligns with the pattern for optional subdomain naming.
38-42: Module inputs properly wired to new variables and security options now configurable. ✓The addition of
elasticsearch_domain_name, subdomain overrides,cold_storage_enabled, and variable-driven security options (node_to_node_encryption_enabled,advanced_security_options_*) replaces hardcoded defaults with appropriate flexibility while maintaining sensible defaults via variables.Also applies to: 49-53
src/variables.tf (2)
49-77: Security and encryption options properly parameterized with sensible defaults. ✓The new variables (
node_to_node_encryption_enabled,advanced_security_options_*) replace hardcoded values with appropriate flexibility:
node_to_node_encryption_enabled: default = truemaintains security posture from previous hard-coded behavioradvanced_security_options_enabled: default = trueenables security features by defaultadvanced_security_options_anonymous_auth_enabled: default = falseis appropriately restrictiveadvanced_security_options_internal_user_database_enabled: default = truemaintains existing behavioradvanced_security_options_master_user_name: default = "admin"provides reasonable defaultAll defaults maintain backward compatibility while enabling flexibility.
121-125: cold_storage_enabled variable added with appropriate default. ✓The new
cold_storage_enabledvariable withdefault = falseallows opt-in to cold storage support without affecting existing deployments.
Update the assertion for domainHostname in the TestBasic suite to check for the "es." prefix and ".components.cptest.test-automation.app" suffix, ensuring the output matches the expected hostname format.
|
/terratest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/component_test.go (1)
53-53: Consider verifying the subdomain portion of the hostname.The assertion now checks that
domainHostnamestarts with "es." and ends with ".components.cptest.test-automation.app", which correctly validates the subdomain-based DNS pattern introduced in this PR. However, the middle portion (the actual subdomain) is not verified.This means the test would pass for edge cases like:
es..components.cptest.test-automation.app(empty subdomain)es.wrong-subdomain.components.cptest.test-automation.app(incorrect subdomain)Consider capturing the subdomain value from the test setup and verifying it appears in the hostname, or at least assert that there is a non-empty subdomain between "es." and ".components.cptest.test-automation.app".
Example approach:
domainHostname := atmos.Output(s.T(), options, "domain_hostname") -assert.True(s.T(), strings.HasPrefix(domainHostname, "es.") && strings.HasSuffix(domainHostname, ".components.cptest.test-automation.app")) +assert.True(s.T(), strings.HasPrefix(domainHostname, "es.")) +assert.True(s.T(), strings.HasSuffix(domainHostname, ".components.cptest.test-automation.app")) +// Verify subdomain exists between prefix and suffix +parts := strings.SplitN(strings.TrimPrefix(domainHostname, "es."), ".components.cptest.test-automation.app", 2) +assert.NotEmpty(s.T(), parts[0], "subdomain portion should not be empty")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/component_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
test/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Place Go Terratest files under test/ and name them *_test.go
Files:
test/component_test.go
test/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Go Terratest with github.com/cloudposse/test-helpers and Atmos fixtures for integration tests
Files:
test/component_test.go
⏰ 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
what
elasticsearch_domain_name,elasticsearch_subdomain_name, andkibana_subdomain_nameto allow custom naming, with validation onelasticsearch_domain_namefor length and allowed characters. These are now passed to the Elasticsearch module using local values for better flexibility. (src/variables.tf,src/main.tf)advanced_security_options_enabled,advanced_security_options_anonymous_auth_enabled,advanced_security_options_internal_user_database_enabled, andadvanced_security_options_master_user_name. These replace hardcoded values, allowing users to control security features through variables. (src/variables.tf,src/main.tf)node_to_node_encryption_enabledconfigurable instead of always enabled, and added a newcold_storage_enabledoption to allow enabling cold storage support. (src/variables.tf,src/main.tf)why
references
Summary by CodeRabbit
New Features
Tests