Generate Terraform config from policy templates #236
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes in this pull request involve modifications to several files primarily related to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Generate
participant Generator
participant Policy
User->>Generate: request to generate config
Generate->>Generator: process templates
Generator->>Generator: check template kind
alt Template kind is policy
Generator->>Policy: create policy config
end
Generate->>User: return generated config
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (13)
.rubocop.yml (1)
23-26: New RSpec/NestedGroups configuration looks good.The addition of the
RSpec/NestedGroupscop with a maximum of 5 nested groups is a good practice. It helps maintain readable and manageable test structures by limiting excessive nesting.Some considerations:
- The max value of 5 is reasonable and aligns with RuboCop's default (which is 3). It provides a good balance between flexibility and maintainability.
- This configuration encourages better test organization and can lead to more modular and easier-to-understand specs.
Consider the following to further improve your RSpec structure:
- Use
describefor classes,contextfor different states or conditions, anditfor individual test cases.- If you find yourself needing deep nesting, it might indicate that the tested code is too complex and could benefit from refactoring.
- Use shared examples or shared contexts to reduce nesting and promote code reuse in your specs.
spec/dummy/.controlplane/templates/policy.yml (1)
1-3: Consider enhancing the identity description.The identity is well-structured, but the description could be more informative. Currently, it's identical to the name, which doesn't provide additional context.
Consider updating the description to provide more details about the purpose or role of this identity in the PostgreSQL proof of concept. For example:
description: Identity for accessing PostgreSQL proof of concept resourceslib/core/terraform_config/gvc.rb (3)
27-27: Approve the change with a minor suggestion.The modification to use
deep_underscore_keysanddeep_symbolize_keysis a good improvement. It ensures consistent key transformation throughout the entireload_balancerhash structure, including nested hashes and arrays. This change enhances the robustness of the code when dealing with complex load balancer configurations.Consider extracting this transformation logic into a separate method for better readability and potential reuse. For example:
def transform_keys(hash) hash&.deep_underscore_keys&.deep_symbolize_keys end # Usage in initialize @load_balancer = transform_keys(load_balancer)This refactoring would make the
initializemethod cleaner and provide a reusable method for similar transformations in other parts of the class or module.
Line range hint
7-19: Consider using a configuration object to reduce parameter list.The
rubocop:disable Metrics/ParameterListscomment suggests that theinitializemethod has too many parameters. This can make the code harder to maintain and use.Consider introducing a configuration object to encapsulate these parameters. This would simplify the method signature and make it easier to add or remove parameters in the future. For example:
class GvcConfig attr_accessor :name, :description, :tags, :domain, :locations, :pull_secrets, :env, :load_balancer def initialize(attributes = {}) attributes.each { |key, value| public_send("#{key}=", value) } end end class Gvc < Base def initialize(config) @config = config # ... rest of the initialization end end # Usage config = GvcConfig.new(name: 'example', description: 'A GVC', ...) gvc = Gvc.new(config)This approach would eliminate the need for the
rubocop:disablecomment and make the class more flexible to future changes.
Line range hint
46-52: Add error handling for missing required fields inload_balancer_tf.The
load_balancer_tfmethod usesfetchto access the:dedicatedkey, which will raise aKeyErrorif the key is missing. However, it silently ignores missing:trusted_proxies. This inconsistency could lead to unexpected behavior.Consider adding explicit error handling for missing required fields and consistent handling of optional fields. For example:
def load_balancer_tf return if load_balancer.nil? block :load_balancer do dedicated = load_balancer[:dedicated] if dedicated.nil? raise KeyError, "Missing required key :dedicated in load_balancer configuration" end argument :dedicated, dedicated argument :trusted_proxies, load_balancer[:trusted_proxies], optional: true end endThis change ensures that missing required fields are caught early and provides consistent handling of both required and optional fields.
spec/pathces/hash_spec.rb (6)
20-21: LGTM: Excellent addition of nested hash test.The new test case for nested hashes is crucial for verifying the enhanced "deep" functionality. It correctly checks that keys at all levels are transformed, using both string and symbol keys for comprehensive coverage.
Consider adding an additional level of nesting to further validate the "deep" aspect:
let(:hash) { { "outerCamelCase" => { innerCamelCase: { "deeperCamelCase" => "value" } } } } it "transforms keys at all levels" do expect(deep_underscored_keys_hash).to eq("outer_camel_case" => { inner_camel_case: { "deeper_camel_case" => "value" } }) end
37-37: LGTM: Keys with numbers or special characters test updated correctly.The test for keys containing numbers or special characters has been properly updated to use the new method name
deep_underscore_keys. The expectation remains correct, ensuring that these keys are transformed appropriately.Consider adding a nested hash to this test case to ensure the deep transformation works correctly with special characters at different levels:
let(:hash) { { "camelCase123" => { "nested@CaseKey" => "value" } } } it "correctly transforms keys with numbers or special characters" do expect(deep_underscored_keys_hash).to eq("camel_case123" => { "nested@case_key" => "value" }) end
45-53: LGTM: String keys tests updated correctly.The tests for string keys (camelCase, snake_case, and multiple uppercase letters) have been properly updated to use the new method name
deep_underscore_keys. The expectations remain correct, ensuring that string keys are transformed appropriately in each case.Consider adding a test case with nested string keys to ensure the deep transformation works correctly at multiple levels:
let(:hash) { { "outerCamelCase" => { "innerCamelCase" => { "deeperCamelCase" => "value" } } } } it "transforms nested string keys" do expect(deep_underscored_keys_hash).to eq( "outer_camel_case" => { "inner_camel_case" => { "deeper_camel_case" => "value" } } ) end
61-69: LGTM: Symbol keys tests updated correctly.The tests for symbol keys (camelCase, snake_case, and multiple uppercase letters) have been properly updated to use the new method name
deep_underscored_keys. The expectations remain correct, ensuring that symbol keys are transformed appropriately in each case.Consider adding a test case with nested symbol keys to ensure the deep transformation works correctly at multiple levels:
let(:hash) { { outerCamelCase: { innerCamelCase: { deeperCamelCase: "value" } } } } it "transforms nested symbol keys" do expect(deep_underscored_keys_hash).to eq( outer_camel_case: { inner_camel_case: { deeper_camel_case: "value" } } ) end
Line range hint
1-72: Consider adding more complex test cases for comprehensive coverage.The updated test suite provides good coverage for the new
deep_underscore_keysfunctionality. However, to ensure robustness, consider adding the following test cases:
A deeply nested hash with mixed string and symbol keys:
let(:hash) { { "levelOne" => { levelTwo: { "levelThree" => { levelFour: "value" } } } } }A hash with an array of hashes as a value:
let(:hash) { { "outerKey" => [{ "innerKey1" => "value1" }, { "innerKey2" => "value2" }] } }Edge cases like empty string keys or numeric keys:
let(:hash) { { "" => "empty", 123 => "numeric" } }These additional tests will help ensure that the
deep_underscore_keysmethod handles a wide variety of complex hash structures correctly.
Line range hint
1-1: Potential typo in file path.The file path
spec/pathces/hash_spec.rbappears to contain a typo. The directory name "pathces" is likely meant to be "patches".Please verify and correct the directory name from "pathces" to "patches" if it's indeed a typo. The correct path should be
spec/patches/hash_spec.rb.spec/command/terraform/generate_spec.rb (1)
102-104: LGTM! Consider adding a comment for clarity.The addition of
policies.tfto the list of app config files is correct and aligns with the PR objective of supporting policy templates. This change ensures that the test suite will verify the generation of the new policies configuration file.Consider adding a comment above the
app_config_filesmethod to explain the purpose of these files and whypolicies.tfhas been added. This will help future developers understand the context of these configuration files. For example:# Define the list of app-specific Terraform configuration files # including the newly added policies.tf for policy templates def app_config_files %w[gvc.tf identities.tf secrets.tf policies.tf].map do |config_file_path| TERRAFORM_CONFIG_DIR_PATH.join(app, config_file_path) end endspec/core/terraform_config/generator_spec.rb (1)
153-176: LGTM: Comprehensive test case for policy kind.The test case thoroughly checks all attributes of the generated Terraform config for the "policy" kind. It verifies the correct instance type, attribute values, and filename generation.
Suggestion for improvement:
Consider adding type checks for each attribute to ensure they are of the expected data type. For example:expect(tf_config.name).to be_a(String).and eq("policy-name") expect(tf_config.tags).to be_a(Hash).and eq("tag1" => "tag1_value", "tag2" => "tag2_value") expect(tf_config.target_links).to be_an(Array).and eq(%w[postgres-poc-credentials postgres-poc-entrypoint-script])This would add an extra layer of validation to ensure the generated config maintains the correct data types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- .rubocop.yml (1 hunks)
- lib/command/terraform/generate.rb (1 hunks)
- lib/core/terraform_config/generator.rb (4 hunks)
- lib/core/terraform_config/gvc.rb (1 hunks)
- lib/core/terraform_config/policy.rb (1 hunks)
- lib/core/terraform_config/secret.rb (1 hunks)
- lib/patches/hash.rb (1 hunks)
- spec/command/terraform/generate_spec.rb (1 hunks)
- spec/core/terraform_config/generator_spec.rb (1 hunks)
- spec/core/terraform_config/policy_spec.rb (1 hunks)
- spec/dummy/.controlplane/templates/policy.yml (1 hunks)
- spec/pathces/hash_spec.rb (1 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/core/terraform_config/secret.rb (1)
Learnt from: zzaakiirr PR: shakacode/control-plane-flow#235 File: lib/core/terraform_config/secret.rb:12-12 Timestamp: 2024-10-10T11:00:15.326Z Learning: Methods `Hash#underscore_keys` and `Hash#symbolize_keys` are defined in `lib/patches/hash.rb` and are available to `Hash` objects in the codebase.
🔇 Additional comments (37)
spec/dummy/.controlplane/templates/policy.yml (2)
5-16: Well-structured policy with good security practices.The policy is well-defined and follows good security practices by granting minimal (view-only) access to specific resources. The use of a template variable
{{APP_NAME}}in the principalLinks is a good approach for maintaining flexibility across different environments.
14-16: Verify the format of targetLinks.The targetLinks use a different format (//secret/) compared to the principalLinks (//gvc/). While this might be intentional, it's worth verifying that this is the correct format for referencing secrets in your system.
Could you confirm that the format //secret/ is the correct way to reference secrets in your infrastructure? If not, consider updating to match the format used in principalLinks.
spec/pathces/hash_spec.rb (3)
6-7: LGTM: Method name change accurately reflects new functionality.The change from
#underscore_keysto#deep_underscore_keysclearly indicates that the method now transforms keys at all levels of the hash, including nested hashes. This naming convention is more descriptive and aligns well with Ruby conventions.
13-13: LGTM: Empty hash test updated correctly.The test for an empty hash has been properly updated to use the new method name
deep_underscore_keys. The expectation remains correct, ensuring that an empty hash is returned unchanged.
29-29: LGTM: Already underscored keys test updated correctly.The test for already underscored keys has been properly updated to use the new method name
deep_underscore_keys. The expectation remains correct, ensuring that already underscored keys are left unchanged.lib/command/terraform/generate.rb (1)
50-50: Approved: Addition of 'policy' template kind.The change aligns with the PR objective of adding support for policy templates. Good job on extending the functionality.
However, I have a few suggestions and questions:
Consider a more dynamic approach for handling template kinds. Instead of hard-coding the accepted kinds, you could maintain a list of supported kinds elsewhere in the codebase, making it easier to add or remove supported types in the future.
The TODO comment above this line suggests that this filtering might be temporary. What's the plan for addressing this? Will all template kinds be supported in the future, eliminating the need for this check?
Here's a potential refactoring to make the code more flexible:
SUPPORTED_TEMPLATE_KINDS = %w[gvc identity secret policy].freeze # In the generate_app_config method next unless SUPPORTED_TEMPLATE_KINDS.include?(template["kind"])This approach would make it easier to manage supported template kinds across the codebase.
To ensure consistency across the codebase, let's check for other occurrences of template kind filtering:
✅ Verification successful
Verification Complete: No additional template kind filtering found.
The addition of
'policy'as a template kind inlib/command/terraform/generate.rbis the only instance of such filtering in the codebase. The change aligns with the PR objective, and no other areas are affected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of template kind filtering rg --type ruby "next unless.*include.*template\[.kind.\]"Length of output: 173
lib/core/terraform_config/secret.rb (1)
45-45: Improved data preparation with deep key transformationThe change from
underscore_keys.symbolize_keystodeep_underscore_keys.deep_symbolize_keysenhances the robustness of theprepare_datamethod. This modification ensures that all levels of nested hashes within thedataobject are properly transformed, converting keys to underscored symbols consistently throughout the structure.This improvement is particularly valuable when dealing with complex, nested data structures, providing a more thorough and consistent data preparation process.
spec/core/terraform_config/generator_spec.rb (2)
128-151: LGTM: Well-structured template definition for policy kind.The template definition for the "policy" kind is comprehensive and includes all necessary fields such as name, description, tags, target, targetKind, targetLinks, and bindings. The structure is consistent with other template definitions in the file.
126-177: Overall: Well-implemented addition of policy kind support.The new context for handling templates of kind "policy" is well-structured and consistent with the existing code. The test case is comprehensive and covers all necessary aspects of the generated Terraform config. The implementation enhances the functionality of the TerraformConfig::Generator to support policy templates alongside existing types.
Consider the earlier suggestion about adding type checks in the test case for extra robustness. Otherwise, the changes look good and are ready for integration.
lib/patches/hash.rb (1)
21-37: Approve the implementation of deep key transformation methodsThe
deep_transform_keysanddeep_transform_keys_in_objectmethods are correctly implemented to recursively transform keys in bothHashandArrayobjects. The use ofeach_with_objectfor hashes andmapfor arrays is appropriate and efficient.lib/core/terraform_config/generator.rb (4)
21-22: Add support for 'policy' kind in filename methodThe code correctly adds handling for the "policy" template kind, returning
"policies.tf"as the filename. This ensures that policy templates are appropriately named in the Terraform configuration.
30-33: Refactortf_configmethod for dynamic configuration generationThe refactoring of the
tf_configmethod improves extensibility by dynamically invoking the appropriate configuration method based ontemplate['kind']. Usingsendalong with a method name constructed from the template kind reduces the complexity of the method and makes it easier to add support for new template kinds in the future.
62-62: Use dynamic GVC name inidentity_configmethodThe
identity_configmethod now uses thegvcmethod to set the GVC name dynamically. This change ensures consistency and flexibility, especially if the GVC name needs to be altered based on different configurations.
106-109: Add helper methodgvcto dynamically reference GVC nameIntroducing the
gvcmethod provides a dynamic and consistent way to reference the GVC name based on the application's name from the configuration. This promotes reusability and ensures that any changes to the application name are reflected throughout the Terraform configuration.lib/core/terraform_config/policy.rb (17)
1-2: LGTM!The frozen string literal comment is correctly placed at the beginning of the file.
5-5: LGTM!Inheriting from the
Baseclass is a good practice to promote code reuse and maintain a consistent interface across related classes.
6-9: LGTM!The
TARGET_KINDSconstant is appropriately defined as a frozen array of valid target kinds.
11-11: LGTM!The
GVC_REQUIERD_TARGET_KINDSconstant is correctly defined to specify the target kinds that require agvcparameter.
13-13: LGTM!The
attr_readeris used appropriately to define read-only attributes for the class.
15-45: LGTM!The
initializemethod is well-structured and handles the initialization of the class attributes. The# rubocop:disableand# rubocop:enablecomments are used appropriately to disable and enable specific Rubocop checks for the method.
47-58: LGTM!The
to_tfmethod correctly generates the Terraform resource block for the policy, including the necessary arguments.
62-66: LGTM!The
validate_target_kind!method properly validates thetarget_kindattribute against theTARGET_KINDSconstant and raises anArgumentErrorwith a clear message if the validation fails.
68-72: LGTM!The
validate_gvc!method correctly checks if thegvcattribute is required based on thetarget_kindand raises anArgumentErrorwith a descriptive message if the validation fails.
74-83: LGTM!The
bindings_tfmethod properly generates the Terraform configuration for thebindingblocks, handling optional attributes correctly.
85-95: LGTM!The
target_query_tfmethod appropriately generates the Terraform configuration for thetarget_queryblock, validating thefetchtype and handling optional attributes.
97-101: LGTM!The
validate_fetch_type!method correctly validates thefetchtype against the allowed values and raises anArgumentErrorwith a clear message if the validation fails.
103-115: LGTM!The
target_query_spec_tfmethod properly generates the Terraform configuration for thespecblock within thetarget_query, validating thematchtype and handling optional attributes.
117-121: LGTM!The
validate_match_type!method correctly validates thematchtype against the allowed values and raises anArgumentErrorwith a clear message if the validation fails.
123-136: LGTM!The
target_query_spec_terms_tfmethod appropriately generates the Terraform configuration for thetermsblocks within thespec, validating each term and handling optional attributes.
138-143: LGTM!The
validate_term!method correctly validates that each term intarget_query.spec.termscontains only one of the specified attributes and raises anArgumentErrorwith a clear message if the validation fails.
145-145: LGTM!The
# rubocop:enable Metrics/ClassLengthcomment is correctly placed at the end of the class to re-enable the previously disabled Rubocop check.spec/core/terraform_config/policy_spec.rb (6)
5-6: Well-structured use ofletfor test configurationsThe combination of
base_optionsandextra_optionsusingletenhances readability and maintainability of the test suite.
36-55: Comprehensive testing withtarget_querycontextThe tests within the
with target querycontext effectively cover various configurations, ensuring that theto_tfmethod behaves as expected with differenttarget_queryinputs.
96-105: Proper handling of invalidfetch_typeThe test correctly verifies that an invalid
fetch_typeraises the appropriateArgumentError, ensuring robust input validation.
107-116: Correct validation for invalidmatch_typeThe test ensures that supplying an invalid
match_typeresults in anArgumentError, which is essential for enforcing valid configurations.
143-169: Thorough testing withouttarget_queryThe context that tests the scenario without a
target_queryensures thatto_tfgenerates the correct configuration whentarget_queryis not provided, covering important use cases.
171-177: Validating the requirement ofgvcforidentitytarget kindThe test accurately checks that an
ArgumentErroris raised whengvcis missing whiletarget_kindis set to"identity", which is crucial for enforcing configuration prerequisites.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
lib/core/terraform_config/generator.rb (1)
108-113: Good addition:policy_bindingsmethod with minor optimization suggestionThe
policy_bindingsmethod is a well-implemented helper for processing policy bindings:
- It correctly transforms the
principalLinksby removing the "//" prefix.- The use of
deleteanddelete_prefixmethods is appropriate and efficient.To slightly optimize and improve readability, consider using
transform_valuesinstead ofmapandmerge:def policy_bindings template["bindings"]&.transform_values do |data| data["principalLinks"] = data["principalLinks"]&.map { |link| link.delete_prefix("//") } data end endThis suggestion maintains the functionality while making the code more concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- lib/core/terraform_config/generator.rb (4 hunks)
- lib/core/terraform_config/policy.rb (1 hunks)
- spec/core/terraform_config/policy_spec.rb (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/core/terraform_config/policy_spec.rb
🧰 Additional context used
📓 Learnings (1)
lib/core/terraform_config/policy.rb (1)
Learnt from: zzaakiirr PR: shakacode/control-plane-flow#236 File: lib/core/terraform_config/policy.rb:4-4 Timestamp: 2024-10-16T09:35:47.327Z Learning: Disabling Rubocop rules is acceptable when refactoring can be done later.
🔇 Additional comments (5)
lib/core/terraform_config/generator.rb (5)
21-22: LGTM: New case for policy template kind addedThe addition of the "policy" case in the
filenamemethod is consistent with the existing pattern and supports the new functionality for handling policy templates.
30-33: Excellent refactoring oftf_configmethodThe refactoring of the
tf_configmethod to use dynamic method invocation is a significant improvement:
- It enhances extensibility by allowing new template kinds to be added without modifying the control flow.
- The use of Ruby's
sendmethod for dynamic dispatch is a common and effective pattern.- The error handling for unsupported template kinds is maintained.
This change makes the code more maintainable and flexible for future additions.
62-62: Good: Dynamic GVC generation inidentity_configThe use of the
gvcmethod instead of a hardcoded string improves flexibility and reduces hardcoding. This change aligns well with best practices for configuration management.
96-98: Good addition:gvcmethod for centralized GVC name generationThe introduction of the
gvcmethod is a positive change:
- It centralizes the logic for generating the GVC name.
- It improves maintainability by providing a single point of change for GVC name formatting.
- The method name clearly communicates its purpose.
This addition enhances the overall code structure and readability.
101-105: Good addition:policy_target_linksmethod for processing target linksThe
policy_target_linksmethod is a well-implemented helper:
- It correctly processes the
targetLinksfrom the template.- The transformation logic (extracting the last part of each link) is clear and concise.
- The method name accurately describes its functionality.
This addition improves code organization by separating the transformation logic from the main configuration method.
…e-flow into support-terraform-config-generation-from-policy-template
zzaakiirr
deleted the
support-terraform-config-generation-from-policy-template
branch
What does this PR do?
This PR adds support for converting templates with
type: policyto terraform config formatTerraform docs
https://registry.terraform.io/providers/controlplane-com/cpln/latest/docs/resources/policy
Examples
Transforms to:
Supported all target kinds that can be created in CPLN:
Summary by CodeRabbit
Release Notes
New Features
Policyclass for managing policy configurations.Bug Fixes
Tests
Policyclass and the handling of "policy" templates.Chores