Fix bug with fleetctl apply for teams, clear agent options only if key is present (#8508)

2022-11-01 15:22:45 -04:00
parent bcfd000adf
commit 8f21e026e3
7 changed files with 97 additions and 22 deletions
@@ -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.
@@ -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) {
@@ -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.
@@ -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{}
@@ -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"`
 }
@@ -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"}} }
@@ -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)
				`@@ -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.