From 8f21e026e3e65b6cf2c3ae6ca634c2bbb362a3cd Mon Sep 17 00:00:00 2001 From: Martin Angers Date: Tue, 1 Nov 2022 15:22:45 -0400 Subject: [PATCH] Fix bug with fleetctl apply for teams, clear agent options only if key is present (#8508) --- changes/issue-8336-fix-fleetctl-apply-teams | 1 + cmd/fleetctl/apply_test.go | 31 ++++++++++++++++++- ee/server/service/teams.go | 22 ++++++++++--- pkg/spec/spec.go | 5 --- server/fleet/teams.go | 18 ++++++++--- server/service/integration_core_test.go | 11 +++++++ server/service/integration_enterprise_test.go | 31 ++++++++++++++----- 7 files changed, 97 insertions(+), 22 deletions(-) create mode 100644 changes/issue-8336-fix-fleetctl-apply-teams diff --git a/changes/issue-8336-fix-fleetctl-apply-teams b/changes/issue-8336-fix-fleetctl-apply-teams new file mode 100644 index 0000000000..20dcd118d4 --- /dev/null +++ b/changes/issue-8336-fix-fleetctl-apply-teams @@ -0,0 +1 @@ +* Fixed a bug in `fleetctl apply` for teams, where a missing `agent_options` key in the YAML spec file would clear the existing agent options for the team (now it leaves it unchanged). If the key is present but empty, then it clears the agent options. diff --git a/cmd/fleetctl/apply_test.go b/cmd/fleetctl/apply_test.go index 9fba524fb4..261499bb9f 100644 --- a/cmd/fleetctl/apply_test.go +++ b/cmd/fleetctl/apply_test.go @@ -189,6 +189,8 @@ spec: require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", filename})) assert.Equal(t, []*fleet.EnrollSecret{{Secret: "AAA"}}, enrolledSecretsCalled[uint(42)]) assert.False(t, ds.ApplyEnrollSecretsFuncInvoked) + // agent options not provided, so left unchanged + assert.JSONEq(t, string(newAgentOpts), string(*teamsByName["team1"].Config.AgentOptions)) filename = writeTmpYml(t, ` apiVersion: v1 @@ -209,6 +211,19 @@ spec: assert.JSONEq(t, string(newAgentOpts), string(*teamsByName["team1"].Config.AgentOptions)) assert.Equal(t, []*fleet.EnrollSecret{{Secret: "BBB"}}, enrolledSecretsCalled[uint(42)]) assert.True(t, ds.ApplyEnrollSecretsFuncInvoked) + + filename = writeTmpYml(t, ` +apiVersion: v1 +kind: team +spec: + team: + agent_options: + name: team1 +`) + + require.Equal(t, "[+] applied 1 teams\n", runAppForTest(t, []string{"apply", "-f", filename})) + // agent options provided but empty, clears the value + assert.Nil(t, teamsByName["team1"].Config.AgentOptions) } func writeTmpYml(t *testing.T, contents string) string { @@ -226,6 +241,10 @@ func TestApplyAppConfig(t *testing.T) { return userRoleSpecList, nil } + ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activityType string, details *map[string]interface{}) error { + return nil + } + ds.UserByEmailFunc = func(ctx context.Context, email string) (*fleet.User, error) { if email == "admin1@example.com" { return userRoleSpecList[0], nil @@ -233,8 +252,13 @@ func TestApplyAppConfig(t *testing.T) { return userRoleSpecList[1], nil } + defaultAgentOpts := json.RawMessage(`{"config":{"foo":"bar"}}`) ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { - return &fleet.AppConfig{OrgInfo: fleet.OrgInfo{OrgName: "Fleet"}, ServerSettings: fleet.ServerSettings{ServerURL: "https://example.org"}}, nil + return &fleet.AppConfig{ + OrgInfo: fleet.OrgInfo{OrgName: "Fleet"}, + ServerSettings: fleet.ServerSettings{ServerURL: "https://example.org"}, + AgentOptions: &defaultAgentOpts, + }, nil } var savedAppConfig *fleet.AppConfig @@ -256,6 +280,8 @@ spec: require.NotNil(t, savedAppConfig) assert.False(t, savedAppConfig.Features.EnableHostUsers) assert.False(t, savedAppConfig.Features.EnableSoftwareInventory) + // agent options were not modified, since they were not provided + assert.Equal(t, string(defaultAgentOpts), string(*savedAppConfig.AgentOptions)) name = writeTmpYml(t, `--- apiVersion: v1 @@ -264,12 +290,15 @@ spec: features: enable_host_users: true enable_software_inventory: true + agent_options: `) assert.Equal(t, "[+] applied fleet config\n", runAppForTest(t, []string{"apply", "-f", name})) require.NotNil(t, savedAppConfig) assert.True(t, savedAppConfig.Features.EnableHostUsers) assert.True(t, savedAppConfig.Features.EnableSoftwareInventory) + // agent options were cleared, provided but empty + assert.Nil(t, savedAppConfig.AgentOptions) } func TestApplyAppConfigDryRunIssue(t *testing.T) { diff --git a/ee/server/service/teams.go b/ee/server/service/teams.go index caf109d4bb..7567782432 100644 --- a/ee/server/service/teams.go +++ b/ee/server/service/teams.go @@ -1,6 +1,7 @@ package service import ( + "bytes" "context" "database/sql" "encoding/json" @@ -376,6 +377,8 @@ func (svc *Service) ModifyTeamEnrollSecrets(ctx context.Context, teamID uint, se return newSecrets, nil } +var jsonNull = json.RawMessage(`null`) + func (svc *Service) ApplyTeamSpecs(ctx context.Context, specs []*fleet.TeamSpec, applyOpts fleet.ApplySpecOptions) error { if err := svc.authz.Authorize(ctx, &fleet.Team{}, fleet.ActionRead); err != nil { return err @@ -435,8 +438,8 @@ func (svc *Service) ApplyTeamSpecs(ctx context.Context, specs []*fleet.TeamSpec, return err } - if spec.AgentOptions != nil { - if err := fleet.ValidateJSONAgentOptions(*spec.AgentOptions); err != nil { + if len(spec.AgentOptions) > 0 && !bytes.Equal(spec.AgentOptions, jsonNull) { + if err := fleet.ValidateJSONAgentOptions(spec.AgentOptions); err != nil { err = fleet.NewUserMessageError(err, http.StatusBadRequest) if applyOpts.Force && !applyOpts.DryRun { level.Info(svc.logger).Log("err", err, "msg", "force-apply team agent options with validation errors") @@ -490,8 +493,8 @@ func (svc *Service) ApplyTeamSpecs(ctx context.Context, specs []*fleet.TeamSpec, } func (svc Service) createTeamFromSpec(ctx context.Context, spec *fleet.TeamSpec, defaults *fleet.AppConfig, secrets []*fleet.EnrollSecret) (*fleet.Team, error) { - agentOptions := spec.AgentOptions - if agentOptions == nil { + agentOptions := &spec.AgentOptions + if len(spec.AgentOptions) == 0 { agentOptions = defaults.AgentOptions } @@ -518,7 +521,16 @@ func (svc Service) createTeamFromSpec(ctx context.Context, spec *fleet.TeamSpec, func (svc Service) editTeamFromSpec(ctx context.Context, team *fleet.Team, spec *fleet.TeamSpec, secrets []*fleet.EnrollSecret) error { team.Name = spec.Name - team.Config.AgentOptions = spec.AgentOptions + + // if agent options are not provided, do not change them + if len(spec.AgentOptions) > 0 { + if bytes.Equal(spec.AgentOptions, jsonNull) { + // agent options provided but null, clear existing agent option + team.Config.AgentOptions = nil + } else { + team.Config.AgentOptions = &spec.AgentOptions + } + } // replace (don't merge) the features with the new ones, using a config // that has the global defaults applied. diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index ff173a4dfc..3d5fee4a26 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -36,11 +36,6 @@ type Metadata struct { Spec json.RawMessage `json:"spec"` } -// TeamSpec holds a spec to be applied to a team. -type TeamSpec struct { - Team *fleet.TeamSpec `json:"team"` -} - // GroupFromBytes parses a Group from concatenated YAML specs. func GroupFromBytes(b []byte) (*Group, error) { specs := &Group{} diff --git a/server/fleet/teams.go b/server/fleet/teams.go index fd2575a657..d584030268 100644 --- a/server/fleet/teams.go +++ b/server/fleet/teams.go @@ -250,8 +250,18 @@ const ( ) type TeamSpec struct { - Name string `json:"name"` - AgentOptions *json.RawMessage `json:"agent_options"` - Secrets []EnrollSecret `json:"secrets"` - Features *json.RawMessage `json:"features"` + Name string `json:"name"` + + // We need to distinguish between the agent_options key being present but + // "empty" or being absent, as we leave the existing agent options unmodified + // if it is absent, and we clear it if present but empty. + // + // If the agent_options key is not provided, the field will be nil (Go nil). + // If the agent_options key is present but empty in the YAML, will be set to + // "null" (JSON null). Otherwise, if the key is present and set, it will be + // set to the agent options JSON object. + AgentOptions json.RawMessage `json:"agent_options,omitempty"` // marshals as "null" if omitempty is not set + + Secrets []EnrollSecret `json:"secrets"` + Features *json.RawMessage `json:"features"` } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 271f263d50..650033416c 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -4430,6 +4430,17 @@ func (s *integrationTestSuite) TestAppConfig() { require.NotEqual(t, fleet.ActivityTypeEditedAgentOptions, listActivities.Activities[0].Type) } + // and it did not update the appconfig + s.DoJSON("GET", "/api/latest/fleet/config", nil, http.StatusOK, &acResp) + require.Contains(t, string(*acResp.AgentOptions), `"logger_plugin": "tls"`) // default agent options has this setting + + // test a change that does clear the agent options (the field is provided but empty). + s.DoJSON("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ + "agent_options": {} + }`), http.StatusOK, &acResp) + s.DoJSON("GET", "/api/latest/fleet/config", nil, http.StatusOK, &acResp) + require.Equal(t, string(*acResp.AgentOptions), "{}") + // test a change that does modify the agent options. s.DoJSON("PATCH", "/api/latest/fleet/config", json.RawMessage(`{ "agent_options": { "config": {"views": {"foo": "bar"}} } diff --git a/server/service/integration_enterprise_test.go b/server/service/integration_enterprise_test.go index 4734770c08..fa0f2e8923 100644 --- a/server/service/integration_enterprise_test.go +++ b/server/service/integration_enterprise_test.go @@ -83,12 +83,11 @@ func (s *integrationEnterpriseTestSuite) TestTeamSpecs() { "enable_software_inventory": false, "additional_queries": {"foo": "bar"} }`) - teamSpecs := applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts, Features: &features}}} + teamSpecs := applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts, Features: &features}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK) team, err := s.ds.TeamByName(context.Background(), teamName) require.NoError(t, err) - assert.Len(t, team.Secrets, 1) require.JSONEq(t, string(agentOpts), string(*team.Config.AgentOptions)) require.Equal(t, fleet.Features{ @@ -107,7 +106,7 @@ func (s *integrationEnterpriseTestSuite) TestTeamSpecs() { // dry-run with invalid agent options agentOpts = json.RawMessage(`{"config": {"nope": 1}}`) - teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts}}} + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusBadRequest, "dry_run", "true") // dry-run with empty body @@ -129,16 +128,34 @@ func (s *integrationEnterpriseTestSuite) TestTeamSpecs() { // dry-run with valid agent options agentOpts = json.RawMessage(`{"config": {"views": {"foo": "qux"}}}`) - teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts}}} + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK, "dry_run", "true") team, err = s.ds.TeamByName(context.Background(), teamName) require.NoError(t, err) require.Contains(t, string(*team.Config.AgentOptions), `"foo": "bar"`) // unchanged + // apply without agent options specified + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName}}} + s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK) + + // agent options are unchanged, not cleared + team, err = s.ds.TeamByName(context.Background(), teamName) + require.NoError(t, err) + require.Contains(t, string(*team.Config.AgentOptions), `"foo": "bar"`) // unchanged + + // apply with agent options specified but null + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: json.RawMessage(`null`)}}} + s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK) + + // agent options are cleared + team, err = s.ds.TeamByName(context.Background(), teamName) + require.NoError(t, err) + require.Nil(t, team.Config.AgentOptions) + // force with invalid agent options agentOpts = json.RawMessage(`{"config": {"foo": "qux"}}`) - teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts}}} + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK, "force", "true") team, err = s.ds.TeamByName(context.Background(), teamName) @@ -157,12 +174,12 @@ func (s *integrationEnterpriseTestSuite) TestTeamSpecs() { // invalid agent options command-line flag agentOpts = json.RawMessage(`{"command_line_flags": {"nope": 1}}`) - teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts}}} + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusBadRequest) // valid agent options command-line flag agentOpts = json.RawMessage(`{"command_line_flags": {"enable_tables": "abcd"}}`) - teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: &agentOpts}}} + teamSpecs = applyTeamSpecsRequest{Specs: []*fleet.TeamSpec{{Name: teamName, AgentOptions: agentOpts}}} s.Do("POST", "/api/latest/fleet/spec/teams", teamSpecs, http.StatusOK) team, err = s.ds.TeamByName(context.Background(), teamName)