refactor beats argument parsing #41277

leehinman · 2024-10-17T00:19:49Z

Proposed commit message

Change beats argument parsing behavior.

Beats currently sets some command line flags in an init function, this means that any other project that imports beats will have conflicts if it uses the same options. This change moves this out of init.

Beats currently modifies the Argument list to offer backwards compatibility for flags in an init function. This would convert flags like -e to --e. This means if any other project imports beats, it's argument parsing would be changed. This change adds an allowed list of arguments that can be changed and moves it out of init.

Beats currently detects if it is fleet managed by looking at the command line flags. This change keeps that behavior, but adds the ability to set this directly. This allows setting the flag without the command line option.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

The goal is none, however if any flags are missed, then the user would have to call them as --flag instead of -flag.

Author's Checklist

Must be tested with these changes running under elastic-agent (main elastic agent, beats built from this PR, policy had auditd and system metrics)
Must be tested with beats imported into elastic-agent (main elastic-agent, importing beats and implementing filebeat receiver, beats builts from this PR, policy had auditd and system metrics). Verified filebeatreceiver worked, normal agentbeat worked and that -1s could be passed as an argument to --fleet-server-timeout
Must be tested with apm-server. Built apm-server using this PR, had to make changes to internal/beatcmd/beat.go and internal/beatcmd/reloader.go. This was expected for change to uuidv5 and storage of the reload registry. Also had to upgrade to badgerv4, also expected, but may be a breaking change. Tested that apm-server -E management.enabled=true -E logging.to_stderr=true still worked as expected. No changes were required to apm-server argument parsing.

How to test this PR locally

Related issues

Closes Fix libbeat argument parsing to allow beats to be imported in other code bases. #41153

Use cases

Screenshots

Logs

elasticmachine · 2024-10-17T00:20:10Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

elasticmachine · 2024-10-17T07:12:51Z

Pinging @elastic/obs-ds-hosted-services (Team:obs-ds-hosted-services)

elasticmachine · 2024-10-17T07:12:51Z

Pinging @elastic/sec-linux-platform (Team:Security-Linux Platform)

elasticmachine · 2024-10-17T07:12:51Z

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

elasticmachine · 2024-10-17T07:12:52Z

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

libbeat/cfgfile/cfgfile.go

belimawr · 2024-10-17T14:27:18Z

libbeat/cfgfile/cfgfile.go


 	"github.com/elastic/beats/v7/libbeat/common"
 	"github.com/elastic/beats/v7/libbeat/common/fleetmode"
 	"github.com/elastic/elastic-agent-libs/config"
 	"github.com/elastic/elastic-agent-libs/logp"
 )

-// Command line flags.
+// Evil package level globals


I love the honesty on this comment ❤️

Co-authored-by: Tiago Queiroz <me@tiago.life>

pierrehilbert · 2024-10-21T12:25:44Z

@qcorporation / @bturquet could we have someone from your teams to review here please?

kruskall · 2024-10-21T20:16:30Z

libbeat/cfgfile/cfgfile.go

+// HandleFlags adapts default config settings based on command line
+// flags.  This also stores if -E management.enabled=true was set on
+// command line to determine if running the Beat under agent.  It will
+// call Initialize() to register the flags like `-E`.
 func HandleFlags() error {


question: what's the guidance for downstream clients ? Do they have to call flag.Parse before calling this ?

asking mostly because of elastic/apm-server#14214 and the previous apm server issue

@kruskall I built apm-server with this PR. Had to make changes to internal/beatcmd/beat.go and internal/beatcmd/reloader.go for the change to uuidv5 and how the reload registry is stored. Also had to upgrade to badgerv4, that is probably a breaking change but I think someone from APM team will have to work on that. I tested that apm-server -E management.enabled=true -E logging.to_stderr=true and apm-server -d "*" -e work the same as before. Also apm-server --d "*" --e works the same as before.

So at least for APM server, it shouldn't require changes to how it handles flag parsing. Other clients may have to register any flags they want to have the -c to --c conversion done with cfgfile.AddAllowedBackwardsCompatibleFlag("c")

emilioalvap

LGTM

pierrehilbert · 2024-10-22T11:30:56Z

@leehinman we are good to go.
Thanks all for the time you took to review this PR.

* modify beats argument parsing to not happen during init * allowlist for backwards compatible args, to conver -arg to --arg Co-authored-by: Tiago Queiroz <me@tiago.life> (cherry picked from commit 15eae5f)

* modify beats argument parsing to not happen during init * allowlist for backwards compatible args, to conver -arg to --arg Co-authored-by: Tiago Queiroz <me@tiago.life> (cherry picked from commit 15eae5f) Co-authored-by: Lee E Hinman <57081003+leehinman@users.noreply.github.com>

leehinman requested review from a team as code owners October 17, 2024 00:19

leehinman requested review from belimawr and rdner October 17, 2024 00:19

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 17, 2024

leehinman added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-8.x Automated backport to the 8.x branch with mergify labels Oct 17, 2024

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 17, 2024

mergify bot assigned leehinman Oct 17, 2024

belimawr reviewed Oct 17, 2024

View reviewed changes

leehinman force-pushed the 41153_argument_parsing branch from 968dbf6 to ffcfe48 Compare October 17, 2024 14:42

leehinman requested a review from a team as a code owner October 17, 2024 14:42

leehinman force-pushed the 41153_argument_parsing branch 6 times, most recently from f0f6e8a to 60b1b1c Compare October 17, 2024 21:44

leehinman force-pushed the 41153_argument_parsing branch 2 times, most recently from 64f67f9 to c2f8a09 Compare October 18, 2024 18:02

leehinman and others added 5 commits October 18, 2024 17:27

start of changing argument parsing

7228fae

allowlist for backwards compatible args

a257c5c

fixes for linter

49c8c60

Update libbeat/cfgfile/cfgfile.go

36db25d

Co-authored-by: Tiago Queiroz <me@tiago.life>

update doc strings to mention when Initialize will be run

1242459

leehinman force-pushed the 41153_argument_parsing branch from 20e1daf to 1242459 Compare October 18, 2024 22:28

nfritts approved these changes Oct 21, 2024

View reviewed changes

taylor-swanson approved these changes Oct 21, 2024

View reviewed changes

kruskall reviewed Oct 21, 2024

View reviewed changes

emilioalvap approved these changes Oct 22, 2024

View reviewed changes

pierrehilbert approved these changes Oct 22, 2024

View reviewed changes

leehinman merged commit 15eae5f into elastic:main Oct 22, 2024
183 checks passed

mergify bot mentioned this pull request Oct 22, 2024

[8.x](backport #41277) refactor beats argument parsing #41373

Merged

9 tasks

ycombinator mentioned this pull request Nov 1, 2024

[Failing Test]: TestAPMConfig – no such index [traces-apm-default] elastic/elastic-agent#5890

Closed

kvalliyurnatt mentioned this pull request Nov 5, 2024

fix beats test failure elastic/cloud-on-k8s#8182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor beats argument parsing #41277

refactor beats argument parsing #41277

leehinman commented Oct 17, 2024 •

edited

Loading

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

belimawr Oct 17, 2024

pierrehilbert commented Oct 21, 2024

kruskall Oct 21, 2024

leehinman Oct 22, 2024

emilioalvap left a comment

pierrehilbert commented Oct 22, 2024

refactor beats argument parsing #41277

refactor beats argument parsing #41277

Conversation

leehinman commented Oct 17, 2024 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

elasticmachine commented Oct 17, 2024

belimawr Oct 17, 2024

Choose a reason for hiding this comment

pierrehilbert commented Oct 21, 2024

kruskall Oct 21, 2024

Choose a reason for hiding this comment

leehinman Oct 22, 2024

Choose a reason for hiding this comment

emilioalvap left a comment

Choose a reason for hiding this comment

pierrehilbert commented Oct 22, 2024

leehinman commented Oct 17, 2024 •

edited

Loading