Issue667 - Add global_data_tags to fleet agent policies

[Zyther]

Whew.

Picking up from where @mholttech left off, this implements global data tags on the agent policy resource.

This change:

• implements Issue 667
• Modifies CRUD of agent policies to include a global_data_tags block, if it exists
• asks for a nested attribute object at the hcl/tf resource level
• updates kibana.gen.go to 3757e641278a5186919e35a0f980ac3cda7e8ccd
• links the global data tags schemas together (post, put, get) so they can be used to marshal/unmarshal universally

Thanks! Let me know what else is needed.

[cla-checker-service]

💚 CLA has been signed

[Zyther]

oi, need to fix this. apologies

[mholttech]

@tobio can you review the updates provided? Would love to get this merged and a new release generated by the end of the week if possible.

[Zyther]

this still persists without it
resource_test.go:72: Step 6/6 error: Error running apply: exit status 1

    Error: Provider produced inconsistent result after apply

[Zyther]

based on this, i will readd

[tobio]

this still persists without it resource_test.go:72: Step 6/6 error: Error running apply: exit status 1

    Error: Provider produced inconsistent result after apply

This sounds like a bug that needs to be fixed. RequiresReplace will create usability issues and should only be in place if it's truly required.

[tobio]

This sounds like a bug that needs to be fixed

Looking at the test failure, I suspect what's happening is:

• We create an agent policy with global data tags
• The tags are then removed from TF. This updates the policy, by simply omits the global data tags from the update request. Most of the stack update API's work more closely to PATCH semantics, which means the existing tags remain on the policy rather than them being removed.
• TF then re-reads the policy, expecting null tags, but gets the existing tags back.

You can probably fix this by updating the global data tags schema to be computed, with a UseStateForUnknown plan modifier. Removing existing global data tags likely means setting global_data_tags = {} rather than just removing the attribute entirely.

[Zyther]

if i understand this correctly, I think global_data_tags needs to exist empty on the payload going out on update -- if there are no tags. If this doesnt work, i may need to modify the apiUpdate to have an empty array if there are none

[Zyther]

how do we feel about this?

[tobio]

I'm taking a look here, I think this behaviour (unsetting global data tags when they're removed from the TF definition) is different to our other resources and it would be good to keep things consistent.

[mholttech]

Can you clarify what you mean here Toby? I would expect that if the agent policy is being managed in TF then every data tag in the target policy is added and removed based on the TF definition. If you are doing a resource import of an existing policy then then during a plan or apply operation TF will let you know if the policy has tags that are not defined in TF through the normal resource diff.

[tobio]

Yep, that's a reasonable expectation. IIRC that's not the behaviour for the other resources in the Elastic stack provider which follow the Elasticsearch API behaviour, i.e a field retains its current value unless explicitly set to null.

[mholttech]

Interesting... I hadn't noticed that behavior but I'm going to test out a few scenarios. If that is the case, though, I feel like the terraform provider is not following expected usage patterns of terraform and can easily result in orphan settings without the engineer knowing

[mholttech]

Hi @tobio,

I haven't had the chance to review the way that other resources in the provider function, however after talking through this with the team I feel strongly that the way this is implemented is correct and is the normal behavior expected when using Terraform.

From a Terraform design perspective, the current implementation follows the standard declarative approach where your configuration represents the complete desired state. The typical behavior across mature Terraform providers is that:

1. When you define a resource in Terraform, all manageable properties should be represented
2. If a property is removed from your Terraform configuration, it should be removed from the actual resource
3. The resource state should exactly match what's defined in your configuration

This approach prevents configuration drift and maintains Terraform as the single source of truth. If we were to preserve tags not defined in Terraform, or require forcing them to null to remove, it would create situations where resource properties exist outside of Terraform's management scope, making it difficult to track what is actually controlled by code versus manual changes.

While I understand your concern about consistency with other Elastic Stack resources that may behave differently, I believe following Terraform's conventional approach to resource management provides a more predictable and maintainable user experience in the long run.

[mholttech]

I just finished reviewing the tests again and we did find one missing assertion that @Zyther just added. Overall the tests are showing the exact behavior that I expect when utilizing a terraform provider.

[tobio]

Couple of small things we can improve.

[tobio]

It looks like we copy this between create/update. Can we pull it out and DRY this code up a little?

[tobio]

Suggested change

	resource.TestCheckNoResourceAttr("elasticstack_fleet_agent_policy.test_policy", "global_data_tags.tag1"),
	resource.TestCheckNoResourceAttr("elasticstack_fleet_agent_policy.test_policy", "global_data_tags.tag2"),
	resource.TestCheckResourceAttr("elasticstack_fleet_agent_policy.test_policy", "global_data_tags.#", "0"),

It's perhaps more clear/accurate to assert that there are no global data tags defined.

[tobio]

While I understand your concern about consistency with other Elastic Stack resources that may behave differently, I believe following Terraform's conventional approach to resource management provides a more predictable and maintainable user experience in the long run.

Ultimately everything you've said is correct, and I certainly don't disagree. Critically it doesn't look like this is the first case we exhibit the 'correct' behaviour within the provider, which makes it a pretty simple decision in this case :) Let's keep this as is, there's a couple of small issues to fix up but otherwise thanks for all the work making these changes!