-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove flags for policy defaulting #30256
Changes from all commits
8740e06
5588d5c
af89b47
6d69c7d
ffe6bc9
d8d9007
53c2e38
d9d8507
0ee5602
9e4fed2
0a9279a
fc13a76
bb1e4ea
45fbf66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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". | ||
|
||
* Bootstrapping Fleet Server | ||
This bootstraps the Fleet Server to be run by this Elastic Agent. At least one Fleet Server is required in a Fleet | ||
|
@@ -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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't see the need for this option. Just make the default for |
||
FLEET_SERVER_HOST - binding host for Fleet Server HTTP (overrides the policy). By default this is 0.0.0.0. | ||
FLEET_SERVER_PORT - binding port for Fleet Server HTTP (overrides the policy) | ||
FLEET_SERVER_CERT - path to certificate to use for HTTPS endpoint | ||
|
@@ -146,6 +148,10 @@ func logError(streams *cli.IOStreams, err error) { | |
fmt.Fprintf(streams.Err, "Error: %v\n%s\n", err, troubleshootMessage()) | ||
} | ||
|
||
func logWarn(streams *cli.IOStreams, a ...interface{}) { | ||
fmt.Fprintln(streams.Out, append([]interface{}{"WARN: "}, a...)...) | ||
} | ||
|
||
func logInfo(streams *cli.IOStreams, a ...interface{}) { | ||
fmt.Fprintln(streams.Out, a...) | ||
} | ||
|
@@ -498,7 +504,10 @@ func kibanaFetchPolicy(cfg setupConfig, client *kibana.Client, streams *cli.IOSt | |
if err != nil { | ||
return nil, err | ||
} | ||
return findPolicy(cfg, policies.Items) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return findPolicy(cfg, policies.Items, streams) | ||
} | ||
|
||
func kibanaFetchToken(cfg setupConfig, client *kibana.Client, policy *kibanaPolicy, streams *cli.IOStreams, tokenName string) (string, error) { | ||
|
@@ -541,31 +550,46 @@ func kibanaClient(cfg kibanaConfig, headers map[string]string) (*kibana.Client, | |
}, 0, "Elastic-Agent") | ||
} | ||
|
||
func findPolicy(cfg setupConfig, policies []kibanaPolicy) (*kibanaPolicy, error) { | ||
func findPolicy(cfg setupConfig, policies []kibanaPolicy, streams *cli.IOStreams) (*kibanaPolicy, error) { | ||
policyID := "" | ||
policyName := cfg.Fleet.TokenPolicyName | ||
if cfg.FleetServer.Enable { | ||
policyID = cfg.FleetServer.PolicyID | ||
} | ||
|
||
for _, policy := range policies { | ||
if policyID != "" { | ||
if policyID == policy.ID { | ||
logInfo(streams, "using policy with id", policyID) | ||
return &policy, nil | ||
} | ||
} else if policyName != "" { | ||
if policyName == policy.Name { | ||
logInfo(streams, "using policy named", policyName) | ||
return &policy, nil | ||
} | ||
} else if cfg.FleetServer.Enable { | ||
if policy.IsDefaultFleetServer { | ||
if policy.ID == cfg.FleetServer.DefaultPolicyID { | ||
logWarn(streams, "using default fleet policy id:", policy.ID) | ||
return &policy, nil | ||
} | ||
} else { | ||
if policy.IsDefault { | ||
if policy.Name == cfg.Fleet.DefaultTokenPolicyName { | ||
logWarn(streams, "using default policy name:", policy.Name) | ||
return &policy, nil | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the added logging and making it explicit this will helps us. |
||
} | ||
|
||
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.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. i'll add comments for both of this situation, and the one below. |
||
} | ||
} else { | ||
if policyName == "" { | ||
logWarn(streams, "No policy name set, please set the FLEET_TOKEN_POLICY_NAME environment variable to the agent policy name.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again can you give me more detail here? Maybe a code comment would be good to explain the flow here. |
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blakerouse I need to relearn the quirks of the enrollment process, but should we just error out? |
||
return nil, fmt.Errorf(`unable to find policy named "%s"`, policyName) | ||
} | ||
|
||
|
@@ -899,11 +923,9 @@ func copyFile(destPath string, srcPath string, mode os.FileMode) error { | |
} | ||
|
||
type kibanaPolicy struct { | ||
ID string `json:"id"` | ||
Name string `json:"name"` | ||
Status string `json:"status"` | ||
IsDefault bool `json:"is_default"` | ||
IsDefaultFleetServer bool `json:"is_default_fleet_server"` | ||
ID string `json:"id"` | ||
Name string `json:"name"` | ||
Status string `json:"status"` | ||
} | ||
|
||
type kibanaPolicies struct { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package cmd | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/elastic/beats/v7/x-pack/elastic-agent/pkg/cli" | ||
|
||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
var ( | ||
defaultFleetPolicy = kibanaPolicy{ | ||
ID: "fleet-server-policy", | ||
Name: "Default Fleet Server policy", | ||
Status: "active", | ||
} | ||
defaultAgentPolicy = kibanaPolicy{ | ||
ID: "2016d7cc-135e-5583-9758-3ba01f5a06e5", | ||
Name: "Default policy", | ||
Status: "active", | ||
} | ||
nondefaultAgentPolicy = kibanaPolicy{ | ||
ID: "bc634ea6-8460-4925-babd-7540c3e7df24", | ||
Name: "Another free policy", | ||
Status: "active", | ||
} | ||
|
||
nondefaultFleetPolicy = kibanaPolicy{ | ||
ID: "7b0093d2-7eab-4862-86c8-63b3dd1db001", | ||
Name: "Some kinda dependent policy", | ||
Status: "active", | ||
} | ||
) | ||
|
||
var streams *cli.IOStreams = cli.NewIOStreams() | ||
|
||
var policies kibanaPolicies = kibanaPolicies{ | ||
Items: []kibanaPolicy{ | ||
defaultFleetPolicy, | ||
defaultAgentPolicy, | ||
nondefaultAgentPolicy, | ||
nondefaultFleetPolicy, | ||
}, | ||
} | ||
|
||
// Finding policies | ||
|
||
func TestFindPolicyById(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
PolicyID: "7b0093d2-7eab-4862-86c8-63b3dd1db001", | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.NoError(t, err) | ||
require.Equal(t, &nondefaultFleetPolicy, policy) | ||
} | ||
|
||
func TestFindPolicyByName(t *testing.T) { | ||
cfg := setupConfig{ | ||
Fleet: fleetConfig{ | ||
TokenPolicyName: "Default policy", | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.NoError(t, err) | ||
require.Equal(t, &defaultAgentPolicy, policy) | ||
} | ||
|
||
func TestFindPolicyByIdOverName(t *testing.T) { | ||
cfg := setupConfig{ | ||
Fleet: fleetConfig{ | ||
TokenPolicyName: "Default policy", | ||
}, | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
PolicyID: "7b0093d2-7eab-4862-86c8-63b3dd1db001", | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.NoError(t, err) | ||
require.Equal(t, &nondefaultFleetPolicy, policy) | ||
} | ||
|
||
func TestFindPolicyByIdMiss(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
PolicyID: "invalid id", | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.Error(t, err) | ||
require.Nil(t, policy) | ||
} | ||
|
||
func TestFindPolicyByNameMiss(t *testing.T) { | ||
cfg := setupConfig{ | ||
Fleet: fleetConfig{ | ||
TokenPolicyName: "invalid name", | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.Error(t, err) | ||
require.Nil(t, policy) | ||
} | ||
|
||
func TestFindPolicyDefaultFleet(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
DefaultPolicyID: "fleet-server-policy", | ||
}, | ||
} | ||
|
||
items := []kibanaPolicy{ | ||
defaultAgentPolicy, | ||
nondefaultAgentPolicy, | ||
nondefaultFleetPolicy, | ||
defaultFleetPolicy, | ||
} | ||
|
||
policy, err := findPolicy(cfg, items, streams) | ||
require.NoError(t, err) | ||
require.Equal(t, &defaultFleetPolicy, policy) | ||
} | ||
|
||
func TestFindPolicyAmbiguousNoDefaultFleet(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
}, | ||
} | ||
|
||
items := []kibanaPolicy{ | ||
defaultAgentPolicy, | ||
nondefaultAgentPolicy, | ||
nondefaultFleetPolicy, | ||
defaultFleetPolicy, | ||
} | ||
|
||
policy, err := findPolicy(cfg, items, streams) | ||
require.Error(t, err) | ||
require.Nil(t, policy) | ||
} | ||
|
||
func TestFindPolicyDefaultNonFleet(t *testing.T) { | ||
cfg := setupConfig{ | ||
Fleet: fleetConfig{ | ||
DefaultTokenPolicyName: "Default policy", | ||
}, | ||
FleetServer: fleetServerConfig{ | ||
Enable: false, | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.NoError(t, err) | ||
require.Equal(t, &defaultAgentPolicy, policy) | ||
} | ||
|
||
func TestFindPolicyNoMatchNonFleet(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: false, | ||
}, | ||
} | ||
|
||
policy, err := findPolicy(cfg, policies.Items, streams) | ||
require.Error(t, err) | ||
require.Nil(t, policy) | ||
} | ||
|
||
func TestFindPolicyNoMatchFleet(t *testing.T) { | ||
cfg := setupConfig{ | ||
FleetServer: fleetServerConfig{ | ||
Enable: true, | ||
}, | ||
} | ||
|
||
items := []kibanaPolicy{ | ||
defaultAgentPolicy, | ||
nondefaultAgentPolicy, | ||
nondefaultFleetPolicy, | ||
} | ||
policy, err := findPolicy(cfg, items, streams) | ||
require.Error(t, err) | ||
require.Nil(t, policy) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this extra needed? Can't the default just be there for
FLEET_TOKEN_POLICY_NAME
when one is not provided?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.