Skip to content

Conversation

@tobio
Copy link
Member

@tobio tobio commented Nov 24, 2025

Related to #617

By itself this is just a bit of a painful breaking change, I'm splitting this overall change up to make things more easily reviewed though. The end goal here is:

  • Reduce the scope of sensitive attributes. elasticstack_fleet_integration_policy's streams_json param should not be sensitive by default. #1150 highlights some of the pain here, where streams_json is a big blob which is treated as a single sensitive string. Decomposing this into separate stream attributes allows practitioners to better understand the changes being applied.
  • Better consistency between API and TF schemas. Inputs and streams are maps in the API, it makes much more sense to treat them as maps in TF land as well.
  • Only require what's actually required. Currently all inputs and streams must be fully defined, otherwise the provider breaks. Decomposing the streams JSON blob allows us to more easily merge user provided config with the integration defaults so users only need to specify vars/streams etc if they're overriding a default value

All of that is coming in follow up PRs though. This PR is intended to only be a schema change.

Breaking change

  • The input block has been removed, and replaced with an inputs map.
  • streams_json has been replaced by a streams map attribute
  • See the changes to the acceptance test definitions (example) for examples.

By itself this is just a bit of a painful breaking change, however it allows us to provide much nicer handling for default value handling, allowing practitioners to only supply non-default input/stream variables
@tobio tobio requested review from dimuon and nick-benoit November 24, 2025 09:00
@tobio tobio self-assigned this Nov 24, 2025
Copilot AI review requested due to automatic review settings November 24, 2025 09:00
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a breaking schema change for the Fleet integration policy resource, moving from list-based input blocks to map-based input attributes. The primary goal is to improve consistency with the Fleet API, enable better handling of sensitive attributes, and allow for partial configuration where users only need to specify overrides to integration defaults.

Key changes:

  • Replace input list block with inputs map attribute keyed by input ID
  • Replace streams_json with nested streams map attribute keyed by stream ID
  • Add schema version upgraders (V0→V1→V2) to handle migration from old schemas
  • Update all test fixtures and acceptance tests to use the new schema format

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
internal/fleet/integration_policy/schema.go Updated schema to v2 with map-based inputs and streams structure
internal/fleet/integration_policy/models.go Refactored models to use maps instead of lists, removed input sorting logic
internal/fleet/integration_policy/schema_v1.go Added V1 schema definition and V1→V2 upgrade logic
internal/fleet/integration_policy/schema_v0.go Modified V0→V1 upgrader to chain into V1→V2 upgrader
internal/fleet/integration_policy/resource.go Registered both V0→V2 and V1→V2 state upgraders
internal/fleet/integration_policy/{create,read,update}.go Updated references from Input to Inputs and getInputTypeV1() to getInputsTypes()
internal/fleet/integration_policy/schema_v1_test.go Added comprehensive tests for V1→V2 upgrade logic
internal/fleet/integration_policy/schema_v0_test.go Added tests for V0→V1 upgrade logic
internal/fleet/integration_policy/models_test.go Removed tests for deleted input sorting logic
internal/fleet/integration_policy/acc_test.go Updated acceptance test assertions to use map-based paths
internal/fleet/integration_policy/testdata/* Updated all test fixtures to use new inputs map syntax
internal/fleet/integration/acc_test.go Updated integration test fixture to use new syntax
docs/resources/fleet_integration_policy.md Updated documentation to reflect map-based schema

Comment on lines +107 to +109
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "inputs.tcp-tcp.enabled", "true"),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "inputs.tcp-tcp.streams.tcp.generic.enabled", "true"),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "inputs.tcp-tcp.streams.tcp.generic.vars", `{"custom":"","data_stream.dataset":"tcp.generic","listen_address":"localhost","listen_port":8080,"ssl":"","syslog_options":"field: message","tags":[]}`),
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The test assertions don't match the expected values from the updated test fixture. The fixture shows enabled = true and listen_port : 8080, but based on the 'update' test case context (line 97 shows listen_port : 8080), these assertions should verify the actual updated values: enabled should check that input was updated and listen_port should be 8080, not the create values.

Copilot uses AI. Check for mistakes.
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "input.0.input_id", "aws_logs-aws-cloudwatch"),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "input.0.enabled", "true"),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "input.0.vars_json", ""),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "input.0.streams_json", `{"aws_logs.generic":{"enabled":true,"vars":{"api_sleep":"200ms","api_timeput":"120s","custom":"","data_stream.dataset":"aws_logs.generic","log_streams":[],"number_of_workers":1,"preserve_original_event":false,"scan_frequency":"1m","start_position":"beginning","tags":["forwarded"]}}}`),
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

This line references the old input.0.streams_json path but should be updated to use the new map-based syntax like inputs.aws_logs-aws-cloudwatch.streams.aws_logs.generic.vars. The test will fail because this attribute path no longer exists in the v2 schema.

Suggested change
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "input.0.streams_json", `{"aws_logs.generic":{"enabled":true,"vars":{"api_sleep":"200ms","api_timeput":"120s","custom":"","data_stream.dataset":"aws_logs.generic","log_streams":[],"number_of_workers":1,"preserve_original_event":false,"scan_frequency":"1m","start_position":"beginning","tags":["forwarded"]}}}`),
resource.TestCheckResourceAttr("elasticstack_fleet_integration_policy.test_policy", "inputs.aws_logs-aws-cloudwatch.streams.aws_logs.generic.vars", `{"api_sleep":"200ms","api_timeput":"120s","custom":"","data_stream.dataset":"aws_logs.generic","log_streams":[],"number_of_workers":1,"preserve_original_event":false,"scan_frequency":"1m","start_position":"beginning","tags":["forwarded"]}`),

Copilot uses AI. Check for mistakes.
"custom" : ""
inputs = {
"tcp-tcp" = {
enabled = true
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The enabled value should be false to properly test the update scenario. The old version had enabled = false (line 72 in the diff context), and an update test should verify that changes are applied correctly.

Suggested change
enabled = true
enabled = false

Copilot uses AI. Check for mistakes.
enabled = true
streams = {
"tcp.generic" = {
enabled = true
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The stream's enabled value should be false to match the update test expectations. The previous version had enabled : false for the stream, and this should be preserved to properly test updates.

Suggested change
enabled = true
enabled = false

Copilot uses AI. Check for mistakes.
enabled = true
vars = jsonencode({
"listen_address" : "localhost"
"listen_port" : 8080
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The listen_port should be 8085 to match the update test scenario. The old version used listen_port : 8085 (line 97 in diff context), which should be preserved to properly test that configuration updates are applied.

Suggested change
"listen_port" : 8080
"listen_port" : 8085

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants