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

[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]

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!

[mholttech]

@tobio Looks like all the work here is done and all the tests passed, when can we get this merged and released?

[tobio]

@tobio Looks like all the work here is done and all the tests passed, when can we get this merged and released?

This comment is still unresolved.

[Zyther]

Thought i committed this! sorry. all set.

[tobio]

LGTM, thanks for persisting with this!