remove flags for policy defaulting #30256

lykkin · 2022-02-08T00:31:36Z

What does this PR do?

Updates the agent policy selection logic to avoid using the is_default* flags.
Fixes #29774

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
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.

How to test this PR locally

Unit tests are added for the new code.

Related issues

Closes #29774

Use cases

Screenshots

Logs

elasticmachine · 2022-02-08T02:12:39Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-02-16T19:21:36.605+0000
Duration: 105 min 32 sec

Test stats 🧪

Test	Results
Failed	0
Passed	5484
Skipped	12
Total	5496

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

x-pack/elastic-agent/pkg/agent/cmd/container.go

…in package policy separation

elasticmachine · 2022-02-08T15:31:31Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

x-pack/elastic-agent/pkg/agent/cmd/container.go

x-pack/elastic-agent/docs/fleet-server-bootstrap.asciidoc

x-pack/elastic-agent/pkg/agent/cmd/container.go

Co-authored-by: Josh Dover <[email protected]>

…leet-policy-updates

ph

This looks good to me, I would like a second reviewer @blakerouse can you review this please.

@lykkin Thanks for the test and the clarify in the log statement.

ph · 2022-02-17T15:13:04Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

 				return &policy, nil
 			}
 		} else {
-			if policy.IsDefault {
+			if policy.Name == cfg.Fleet.DefaultTokenPolicyName {
+				logWarn(streams, "using default policy name:", policy.Name)
 				return &policy, nil
 			}
 		}


Thanks for the added logging and making it explicit this will helps us.

ph · 2022-02-17T15:20:52Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

+		if policyName == "" {
+			logWarn(streams, "No policy name set, please set the FLEET_TOKEN_POLICY_NAME environment variable to the agent policy name.")
+		}
+	}


@blakerouse I need to relearn the quirks of the enrollment process, but should we just error out?

blakerouse · 2022-02-24T16:28:34Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

@@ -81,6 +81,7 @@ The following actions are possible and grouped based on the actions.
  KIBANA_FLEET_HOST - kibana host to enable create enrollment token on [$KIBANA_HOST]
  FLEET_TOKEN_NAME - token name to use for fetching token from Kibana. This requires Kibana configs to be set.
  FLEET_TOKEN_POLICY_NAME - token policy name to use for fetching token from Kibana. This requires Kibana configs to be set.
+  DEFAULT_FLEET_TOKEN_POLICY_NAME - token policy name to use when [$FLEET_TOKEN_POLICY_NAME] is not set. Defaults to "Default policy".


Why is this extra needed? Can't the default just be there for FLEET_TOKEN_POLICY_NAME when one is not provided?

this is in response to the above conversation saying the default should be configurable. maybe i misunderstood what was being said?

it does seem a bit redundant to me though, since someone could just set the policy id up front when they are configuring their instance.

i suppose it could be useful if someone wanted to set up different defaults assuming the policy an agent is looking for disappears. not too sure how much we want to worry about that situation.

blakerouse · 2022-02-24T16:29:07Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

@@ -95,7 +96,8 @@ The following actions are possible and grouped based on the actions.
  FLEET_SERVER_ELASTICSEARCH_CA_TRUSTED_FINGERPRINT - The sha-256 fingerprint value of the certificate authority to trust
  FLEET_SERVER_ELASTICSEARCH_INSECURE - disables cert validation for communication with Elasticsearch
  FLEET_SERVER_SERVICE_TOKEN - service token to use for communication with elasticsearch
-  FLEET_SERVER_POLICY_ID - policy ID for Fleet Server to use for itself ("Default Fleet Server policy" used when undefined)
+  FLEET_SERVER_POLICY_ID - policy ID for Fleet Server to use for itself ([$DEFAULT_FLEET_SERVER_POLICY_ID] used when undefined)
+  DEFAULT_FLEET_SERVER_POLICY_ID - policy ID for Fleet Server to use for itself when none is specified. (defaults to "fleet-server-policy")


Don't see the need for this option. Just make the default for FLEET_SERVER_POLICY_ID when one is not provided.

blakerouse · 2022-02-24T16:31:40Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

+
+	if cfg.FleetServer.Enable {
+		if policyID == "" {
+			logWarn(streams, "No policy id set, please set the FLEET_SERVER_POLICY_ID environment variable to the fleet server policy id.")


Maybe I missed part of a conversation, but whats the point of this warning? I believe that Fleet Server will find the policy correctly without an ID. So is a warning here really needed?

here's what i understand this code to mean:

this would be hit if the user didn't configure either a policy id or default when hosting a fleet server under the agent. this gives more specific information to the user assuming the agent fails to find a policy to get a token from.
this coincides with a path that halts start up of the agent.

i'll add comments for both of this situation, and the one below.

blakerouse · 2022-02-24T16:32:28Z

x-pack/elastic-agent/pkg/agent/cmd/container.go

+		}
+	} else {
+		if policyName == "" {
+			logWarn(streams, "No policy name set, please set the FLEET_TOKEN_POLICY_NAME environment variable to the agent policy name.")


Again can you give me more detail here? Maybe a code comment would be good to explain the flow here.

mergify · 2022-02-24T20:19:21Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fleet-policy-updates upstream/fleet-policy-updates
git merge upstream/main
git push upstream fleet-policy-updates

ph · 2022-03-23T20:08:17Z

@lykkin can you apply the comments / changes from @blakerouse?

jlind23 · 2022-03-29T07:18:26Z

@lykkin can we close this one as it's targeting the old elastic agent folder?

jlind23 · 2022-03-31T17:09:38Z

This PR is not relevant anymore as agent was moved.

remove default flags for policy defaulting

8740e06

lykkin added enhancement Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team backport-v8.2.0 Automated backport with mergify labels Feb 8, 2022

lykkin self-assigned this Feb 8, 2022

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 8, 2022

lykkin mentioned this pull request Feb 8, 2022

[Agent] Change logic to find agent policy during Fleet enrollment #29774

Closed

lykkin changed the title ~~remove default flags for policy defaulting~~ remove flags for policy defaulting Feb 8, 2022

more tests, less useless code

5588d5c

juliaElastic reviewed Feb 8, 2022

View reviewed changes

x-pack/elastic-agent/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

more tests, added logic for fleet default, fixed a duplication issue …

af89b47

…in package policy separation

lykkin marked this pull request as ready for review February 8, 2022 15:31

lykkin added the v8.2.0 label Feb 8, 2022

added changelog entry

6d69c7d

jlind23 requested a review from a team February 8, 2022 16:45

jlind23 reviewed Feb 8, 2022

View reviewed changes

x-pack/elastic-agent/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

jlind23 requested review from ph and a team February 8, 2022 16:46

bryan added 3 commits February 9, 2022 13:32

added configurable default policy id option

ffe6bc9

formatting

d8d9007

missing quote

53c2e38

ph suggested changes Feb 10, 2022

View reviewed changes

address review comments

d9d8507

simitt mentioned this pull request Feb 14, 2022

Fix setup with Elastic Agent and Fleet elastic/apm-integration-testing#1440

Closed

bryan added 3 commits February 14, 2022 20:32

ripped out fallback logic, added logging

0ee5602

remove more cruft

9e4fed2

added agent policy default name, updated tests, updated docs

0a9279a

lykkin requested a review from ph February 16, 2022 15:02

joshdover reviewed Feb 16, 2022

View reviewed changes

x-pack/elastic-agent/docs/fleet-server-bootstrap.asciidoc Outdated Show resolved Hide resolved

x-pack/elastic-agent/pkg/agent/cmd/container.go Outdated Show resolved Hide resolved

lykkin and others added 3 commits February 16, 2022 09:59

Update x-pack/elastic-agent/docs/fleet-server-bootstrap.asciidoc

fc13a76

Co-authored-by: Josh Dover <[email protected]>

fixed docs for new env vars

bb1e4ea

Merge branch 'fleet-policy-updates' of github.com:lykkin/beats into f…

45fbf66

…leet-policy-updates

ph approved these changes Feb 17, 2022

View reviewed changes

ph requested a review from blakerouse February 17, 2022 15:21

lykkin mentioned this pull request Feb 22, 2022

Default fleet policy removal elastic/fleet-server#1157

Merged

5 tasks

blakerouse requested changes Feb 24, 2022

View reviewed changes

lykkin mentioned this pull request Mar 20, 2022

Remove flags for policy defaulting elastic/elastic-agent#226

Merged

jlind23 closed this Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove flags for policy defaulting #30256

remove flags for policy defaulting #30256

lykkin commented Feb 8, 2022 •

edited

Loading

elasticmachine commented Feb 8, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

elasticmachine commented Feb 8, 2022

ph left a comment

ph Feb 17, 2022

ph Feb 17, 2022

blakerouse Feb 24, 2022

lykkin Feb 25, 2022

blakerouse Feb 24, 2022

blakerouse Feb 24, 2022

lykkin Feb 25, 2022

blakerouse Feb 24, 2022

mergify bot commented Feb 24, 2022

ph commented Mar 23, 2022

jlind23 commented Mar 29, 2022

jlind23 commented Mar 31, 2022

remove flags for policy defaulting #30256

remove flags for policy defaulting #30256

Conversation

lykkin commented Feb 8, 2022 • edited Loading

What does this PR do?

Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

elasticmachine commented Feb 8, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

elasticmachine commented Feb 8, 2022

ph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Feb 24, 2022

ph commented Mar 23, 2022

jlind23 commented Mar 29, 2022

jlind23 commented Mar 31, 2022

lykkin commented Feb 8, 2022 •

edited

Loading

elasticmachine commented Feb 8, 2022 •

edited by jenkins-beats-ci bot

Loading