From cc5259cb260a5768df79fdb79ce57bff89858fdb Mon Sep 17 00:00:00 2001 From: aahel Date: Mon, 19 Jun 2023 07:48:34 +0000 Subject: [PATCH 1/6] backport of commit 9a8c45b91dd31a08928d2de50b0b33fde8eee157 --- .../server-acl-init/create_or_update.go | 57 +++++++++---------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/control-plane/subcommand/server-acl-init/create_or_update.go b/control-plane/subcommand/server-acl-init/create_or_update.go index 085372827b..ba1e3adcf5 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update.go +++ b/control-plane/subcommand/server-acl-init/create_or_update.go @@ -312,42 +312,37 @@ func (c *Command) createOrUpdateACLPolicy(policy api.ACLPolicy, consulClient *ap // Allowing the Consul node name to be configurable also requires any sync // policy to be updated in case the node name has changed. if isPolicyExistsErr(err, policy.Name) { - if c.flagEnableNamespaces || c.flagSyncCatalog { - c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name)) + c.log.Info(fmt.Sprintf("Policy %q already exists, updating", policy.Name)) - // The policy ID is required in any PolicyUpdate call, so first we need to - // get the existing policy to extract its ID. - existingPolicies, _, err := consulClient.ACL().PolicyList(&api.QueryOptions{}) - if err != nil { - return err - } - - // Find the policy that matches our name and description - // and that's the ID we need - for _, existingPolicy := range existingPolicies { - if existingPolicy.Name == policy.Name && existingPolicy.Description == policy.Description { - policy.ID = existingPolicy.ID - } - } + // The policy ID is required in any PolicyUpdate call, so first we need to + // get the existing policy to extract its ID. + existingPolicies, _, err := consulClient.ACL().PolicyList(&api.QueryOptions{}) + if err != nil { + return err + } - // This shouldn't happen, because we're looking for a policy - // only after we've hit a `Policy already exists` error. - // The only time it might happen is if a user has manually created a policy - // with this name but used a different description. In this case, - // we don't want to overwrite the policy so we just error. - if policy.ID == "" { - return fmt.Errorf("policy found with name %q but not with expected description %q; "+ - "if this policy was created manually it must be renamed to something else because this name is reserved by consul-k8s", - policy.Name, policy.Description) + // Find the policy that matches our name and description + // and that's the ID we need + for _, existingPolicy := range existingPolicies { + if existingPolicy.Name == policy.Name && existingPolicy.Description == policy.Description { + policy.ID = existingPolicy.ID } + } - // Update the policy now that we've found its ID - _, _, err = consulClient.ACL().PolicyUpdate(&policy, &api.WriteOptions{}) - return err - } else { - c.log.Info(fmt.Sprintf("Policy %q already exists, skipping update", policy.Name)) - return nil + // This shouldn't happen, because we're looking for a policy + // only after we've hit a `Policy already exists` error. + // The only time it might happen is if a user has manually created a policy + // with this name but used a different description. In this case, + // we don't want to overwrite the policy so we just error. + if policy.ID == "" { + return fmt.Errorf("policy found with name %q but not with expected description %q; "+ + "if this policy was created manually it must be renamed to something else because this name is reserved by consul-k8s", + policy.Name, policy.Description) } + + // Update the policy now that we've found its ID + _, _, err = consulClient.ACL().PolicyUpdate(&policy, &api.WriteOptions{}) + return err } return err } From 8315f30c5e16ea5b968a39d14f3bc85bfe219351 Mon Sep 17 00:00:00 2001 From: aahel Date: Wed, 21 Jun 2023 11:28:33 +0000 Subject: [PATCH 2/6] backport of commit 80079208c1534de71c433e21a3c19a3c5d88a94a --- .changelog/2392.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/2392.txt diff --git a/.changelog/2392.txt b/.changelog/2392.txt new file mode 100644 index 0000000000..e15ef152b1 --- /dev/null +++ b/.changelog/2392.txt @@ -0,0 +1,3 @@ +```release-note:bug +control-plane: Always update ACL policies upon upgrade +``` \ No newline at end of file From ead5a0ed38c83abdbc99e3f2bd9db6941121293b Mon Sep 17 00:00:00 2001 From: aahel Date: Sun, 25 Jun 2023 18:01:13 +0000 Subject: [PATCH 3/6] backport of commit 7ab54cea7a08ea9f260322bf080a9309be13c30d --- .../server-acl-init/create_or_update_test.go | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index 5cd01fac25..708ec342e4 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -66,3 +66,93 @@ func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) { require.NoError(err) require.Equal(policyDescription, rereadPolicy.Description) } + +func TestCreateOrUpdateACLPolicy_Update(t *testing.T) { + require := require.New(t) + ui := cli.NewMockUi() + k8s := fake.NewSimpleClientset() + cmd := Command{ + UI: ui, + clientset: k8s, + log: hclog.NewNullLogger(), + } + cmd.init() + // Start Consul. + bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + svr, err := testutil.NewTestServerConfigT(t, func(c *testutil.TestServerConfig) { + c.ACL.Enabled = true + c.ACL.Tokens.InitialManagement = bootToken + }) + require.NoError(err) + defer svr.Stop() + svr.WaitForLeader(t) + + // Get a Consul client. + consul, err := api.NewClient(&api.Config{ + Address: svr.HTTPAddr, + Token: bootToken, + }) + require.NoError(err) + connectInjectRule, err := cmd.injectRules() + require.NoError(err) + aclReplRule, err := cmd.aclReplicationRules() + require.NoError(err) + policyDescription := "policy-description" + policyName := "policy-name" + policy, _, err := consul.ACL().PolicyCreate(&api.ACLPolicy{ + Name: "new-policy-name", + Description: "new-policy-desc", + }, nil) + require.NoError(err) + cases := []struct { + Name string + ID string + PolicyDescription string + PolicyName string + Rules string + Err error + ExpPolicy *api.ACLPolicy + }{ + { + Name: "create", + ID: "", + PolicyDescription: policyDescription, + PolicyName: policyName, + Rules: connectInjectRule, + Err: nil, + ExpPolicy: &api.ACLPolicy{ + Name: policyName, + Description: policyDescription, + Rules: connectInjectRule, + }, + }, + { + Name: "update", + ID: policy.ID, + PolicyDescription: policy.Description, + PolicyName: policy.Name, + Rules: aclReplRule, + Err: nil, + ExpPolicy: &api.ACLPolicy{ + Name: policyName, + Description: policyDescription, + Rules: aclReplRule, + }, + }, + } + for _, tt := range cases { + t.Run(tt.Name, func(t *testing.T) { + err = cmd.createOrUpdateACLPolicy(api.ACLPolicy{ + Name: tt.PolicyName, + Description: tt.PolicyDescription, + Rules: tt.Rules, + }, consul) + require.Equal(tt.Err, err) + if tt.ID != "" { + readPolicy, _, err := consul.ACL().PolicyRead(tt.ID, nil) + require.NoError(err) + require.Equal(tt.Rules, readPolicy.Rules) + } + }) + } +} From 9ef246aae0fb122671564a6e07913952e08f1e21 Mon Sep 17 00:00:00 2001 From: aahel Date: Sun, 25 Jun 2023 18:13:37 +0000 Subject: [PATCH 4/6] backport of commit 399853b942eb33cdbdbf06e8aaee1fd1bf6ced68 --- .../subcommand/server-acl-init/create_or_update_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index 708ec342e4..eaa0adb52e 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -67,7 +67,7 @@ func TestCreateOrUpdateACLPolicy_ErrorsIfDescriptionDoesNotMatch(t *testing.T) { require.Equal(policyDescription, rereadPolicy.Description) } -func TestCreateOrUpdateACLPolicy_Update(t *testing.T) { +func TestCreateOrUpdateACLPolicy(t *testing.T) { require := require.New(t) ui := cli.NewMockUi() k8s := fake.NewSimpleClientset() From 8b1f531e9d37a3bb4fc08da92b041c719cf82377 Mon Sep 17 00:00:00 2001 From: aahel Date: Mon, 26 Jun 2023 07:19:39 +0000 Subject: [PATCH 5/6] backport of commit ed3f58a40f514a19c1a89aff4b30a67f0ad655cd --- .../subcommand/server-acl-init/create_or_update_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index eaa0adb52e..aae22ac544 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -152,6 +152,8 @@ func TestCreateOrUpdateACLPolicy(t *testing.T) { readPolicy, _, err := consul.ACL().PolicyRead(tt.ID, nil) require.NoError(err) require.Equal(tt.Rules, readPolicy.Rules) + require.Equal(tt.PolicyName, readPolicy.Name) + require.Equal(tt.PolicyDescription, readPolicy.Description) } }) } From 041ae4fbe32ac9e7a35b24846235e348808bd555 Mon Sep 17 00:00:00 2001 From: aahel Date: Mon, 26 Jun 2023 17:15:22 +0000 Subject: [PATCH 6/6] backport of commit 55a9090a211dda1cc2004c88ec91e37ac6b7e193 --- .../server-acl-init/create_or_update_test.go | 40 ++++--------------- 1 file changed, 8 insertions(+), 32 deletions(-) diff --git a/control-plane/subcommand/server-acl-init/create_or_update_test.go b/control-plane/subcommand/server-acl-init/create_or_update_test.go index aae22ac544..fbc83f77f0 100644 --- a/control-plane/subcommand/server-acl-init/create_or_update_test.go +++ b/control-plane/subcommand/server-acl-init/create_or_update_test.go @@ -99,45 +99,23 @@ func TestCreateOrUpdateACLPolicy(t *testing.T) { require.NoError(err) policyDescription := "policy-description" policyName := "policy-name" - policy, _, err := consul.ACL().PolicyCreate(&api.ACLPolicy{ - Name: "new-policy-name", - Description: "new-policy-desc", - }, nil) - require.NoError(err) cases := []struct { Name string - ID string PolicyDescription string PolicyName string Rules string - Err error - ExpPolicy *api.ACLPolicy }{ { Name: "create", - ID: "", PolicyDescription: policyDescription, PolicyName: policyName, Rules: connectInjectRule, - Err: nil, - ExpPolicy: &api.ACLPolicy{ - Name: policyName, - Description: policyDescription, - Rules: connectInjectRule, - }, }, { Name: "update", - ID: policy.ID, - PolicyDescription: policy.Description, - PolicyName: policy.Name, + PolicyDescription: policyDescription, + PolicyName: policyName, Rules: aclReplRule, - Err: nil, - ExpPolicy: &api.ACLPolicy{ - Name: policyName, - Description: policyDescription, - Rules: aclReplRule, - }, }, } for _, tt := range cases { @@ -147,14 +125,12 @@ func TestCreateOrUpdateACLPolicy(t *testing.T) { Description: tt.PolicyDescription, Rules: tt.Rules, }, consul) - require.Equal(tt.Err, err) - if tt.ID != "" { - readPolicy, _, err := consul.ACL().PolicyRead(tt.ID, nil) - require.NoError(err) - require.Equal(tt.Rules, readPolicy.Rules) - require.Equal(tt.PolicyName, readPolicy.Name) - require.Equal(tt.PolicyDescription, readPolicy.Description) - } + require.Nil(err) + policy, _, err := consul.ACL().PolicyReadByName(tt.PolicyName, nil) + require.Nil(err) + require.Equal(tt.Rules, policy.Rules) + require.Equal(tt.PolicyName, policy.Name) + require.Equal(tt.PolicyDescription, policy.Description) }) } }