Skip to content

Conversation

@dimuon
Copy link
Contributor

@dimuon dimuon commented Dec 8, 2025

Closes #1312

@dimuon dimuon requested review from Copilot, nick-benoit and tobio and removed request for Copilot December 8, 2025 15:49
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 support for the host_name_format field in Fleet agent policies, allowing users to control whether the host.name field uses short hostnames or fully qualified domain names (FQDNs). The implementation leverages the underlying AgentFeatures API, specifically the "fqdn" feature flag.

Key Changes:

  • Added host_name_format schema attribute with validation and default value
  • Implemented conversion between host_name_format field and AgentFeatures API representation
  • Enhanced update logic to preserve existing agent features while managing the hostname format setting

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/fleet/agent_policy/schema.go Adds host_name_format field definition with validators and default value
internal/fleet/agent_policy/models.go Implements conversion logic between host_name_format and AgentFeatures, including merge logic
internal/fleet/agent_policy/models_test.go Unit tests for conversion and merge functions
internal/fleet/agent_policy/update.go Fetches existing policy features before update to preserve non-hostname features
internal/fleet/agent_policy/version_test.go Updates test calls to match new toAPIUpdateModel signature
internal/fleet/agent_policy/acc_test.go Acceptance tests covering create, update, and removal scenarios
internal/fleet/agent_policy/testdata/.../main.tf Test configuration files for acceptance tests
internal/fleet/agent_policy/testdata/.../variables.tf Test variable definitions for acceptance tests
examples/resources/elasticstack_fleet_agent_policy/resource.tf Documentation example showing usage
Makefile Makes RERUN_FAILS configurable

)

// apiAgentFeature is the type expected by the generated API for agent features
type apiAgentFeature = struct {
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

The type alias apiAgentFeature should be a named type rather than a type alias. Using type apiAgentFeature = struct creates an alias which may cause confusion when comparing types. Change to type apiAgentFeature struct to define a proper named type.

Suggested change
type apiAgentFeature = struct {
type apiAgentFeature struct {

Copilot uses AI. Check for mistakes.
return
}

// Read current policy to get existing AgentFeatures (so we can preserve other features)
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

This introduces an additional API call during every update operation to fetch the current policy. Consider documenting why this extra round-trip is necessary (to preserve existing agent features) and whether this could impact update performance for users with many policies.

Suggested change
// Read current policy to get existing AgentFeatures (so we can preserve other features)
// Read current policy to get existing AgentFeatures (so we can preserve other features).
//
// NOTE: This introduces an additional API call during every update operation.
// This is necessary to fetch the current agent policy so that we can preserve agent features
// that are not managed by Terraform. Without this, updating the policy would overwrite
// any features set outside of Terraform, potentially breaking existing agent functionality.
//
// Be aware: For users with many policies, this extra round-trip may impact update performance.
// However, it is required to ensure that unmanaged agent features are not lost during updates.

Copilot uses AI. Check for mistakes.
Comment on lines +554 to +560
result := make([]apiAgentFeature, 0, len(existing)+1)
found := false

for _, f := range existing {
if f.Name == newFeature.Name {
result = append(result, *newFeature)
found = true
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

The capacity allocation len(existing)+1 will always over-allocate when replacing an existing feature. Consider using len(existing) as the capacity and only growing if appending a new feature.

Suggested change
result := make([]apiAgentFeature, 0, len(existing)+1)
found := false
for _, f := range existing {
if f.Name == newFeature.Name {
result = append(result, *newFeature)
found = true
// Pre-scan to determine if we are replacing or appending
found := false
for _, f := range existing {
if f.Name == newFeature.Name {
found = true
break
}
}
// Allocate with correct capacity: len(existing) if replacing, len(existing)+1 if appending
capacity := len(existing)
if !found {
capacity++
}
result := make([]apiAgentFeature, 0, capacity)
for _, f := range existing {
if f.Name == newFeature.Name {
result = append(result, *newFeature)

Copilot uses AI. Check for mistakes.
Copy link
Member

@tobio tobio left a comment

Choose a reason for hiding this comment

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

LGTM, couple of tiny nits.

Optional: true,
Default: stringdefault.StaticString("hostname"),
Validators: []validator.String{
stringvalidator.OneOf("hostname", "fqdn"),
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
stringvalidator.OneOf("hostname", "fqdn"),
stringvalidator.OneOf(HostNameFormatHostname, HostNameFormatFQDN),

Description: "Determines the format of the host.name field in events. Can be 'hostname' (short hostname, e.g., 'myhost') or 'fqdn' (fully qualified domain name, e.g., 'myhost.example.com'). Defaults to 'hostname'.",
Computed: true,
Optional: true,
Default: stringdefault.StaticString("hostname"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Default: stringdefault.StaticString("hostname"),
Default: stringdefault.StaticString(HostNameFormatHostname),

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.

[Feature] Fleet agent policy resource - support "host name format" field

2 participants