Skip to content
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.

Sign up for GitHub

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

Always set server acl tokens #832

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Always set server acl tokens #832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
## UNRELEASED

BUG FIXES:
* Control Plane
* ACLs: Fix issue where if one or more servers fail to have their ACL tokens set on the initial run of server-acl-init
then on subsequent re-runs of server-acl-init the tokens are never set. [[GH-825](https://github.com/hashicorp/consul-k8s/issues/825)]
* ACLs: Fix issue where if the number of Consul servers is increased, the new servers are never provisioned
an ACL token. [[GH-677](https://github.com/hashicorp/consul-k8s/issues/677)]

## 0.36.0 (November 02, 2021)

BREAKING CHANGES:
Expand Down
40 changes: 11 additions & 29 deletions control-plane/subcommand/server-acl-init/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@ import (
"sync"
"time"

"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/subcommand"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/flags"
k8sflags "github.com/hashicorp/consul-k8s/control-plane/subcommand/flags"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/go-discover"
"github.com/hashicorp/go-hclog"
Expand All @@ -24,6 +19,12 @@ import (
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"

"github.com/hashicorp/consul-k8s/control-plane/consul"
"github.com/hashicorp/consul-k8s/control-plane/subcommand"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/flags"
k8sflags "github.com/hashicorp/consul-k8s/control-plane/subcommand/flags"
)

type Command struct {
Expand Down Expand Up @@ -313,7 +314,6 @@ func (c *Command) Run(args []string) int {
scheme = "https"
}

var updateServerPolicy bool
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed. Previously this would be set when there already exists a kube bootstrap secret to ensure that we update the server policy. Now we call c.bootstrapServers, even when the kube secret exists. That function then calls setServerTokens that then calls c.setServerPolicy(consulClient). So we don't need this codepath anymore.

var bootstrapToken string

if c.flagBootstrapTokenFile != "" {
Expand All @@ -340,19 +340,13 @@ func (c *Command) Run(args []string) int {

if bootstrapToken != "" {
c.log.Info(fmt.Sprintf("ACLs already bootstrapped - retrieved bootstrap token from Secret %q", bootTokenSecretName))

// Mark that we should update the server ACL policy in case
// there are namespace related config changes. Because of the
// organization of the server token creation code, the policy
// otherwise won't be updated.
updateServerPolicy = true
} else {
c.log.Info("No bootstrap token from previous installation found, continuing on to bootstrapping")
bootstrapToken, err = c.bootstrapServers(serverAddresses, bootTokenSecretName, scheme)
if err != nil {
c.log.Error(err.Error())
return 1
}
}
bootstrapToken, err = c.bootstrapServers(serverAddresses, bootstrapToken, bootTokenSecretName, scheme)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we always call this.

if err != nil {
c.log.Error(err.Error())
return 1
}
}

Expand Down Expand Up @@ -384,18 +378,6 @@ func (c *Command) Run(args []string) int {
c.log.Info("Current datacenter", "datacenter", consulDC, "primaryDC", primaryDC)
isPrimary := consulDC == primaryDC

// With the addition of namespaces, the ACL policies associated
// with the server tokens may need to be updated if Enterprise Consul
// users upgrade to 1.7+. This updates the policy if the bootstrap
// token had previously existed, which signals a potential config change.
if updateServerPolicy {
_, err = c.setServerPolicy(consulClient)
if err != nil {
c.log.Error("Error updating the server ACL policy", "err", err)
return 1
}
}

if c.flagEnablePartitions && c.flagPartitionName == consulDefaultPartition && isPrimary {
// Partition token is local because only the Primary datacenter can have Admin Partitions.
err := c.createLocalACL("partitions", partitionRules, consulDC, isPrimary, consulClient)
Expand Down
122 changes: 112 additions & 10 deletions control-plane/subcommand/server-acl-init/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@ import (
"testing"
"time"

"github.com/hashicorp/consul-k8s/control-plane/helper/cert"
"github.com/hashicorp/consul-k8s/control-plane/helper/go-discover/mocks"
"github.com/hashicorp/consul-k8s/control-plane/helper/test"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
"github.com/hashicorp/consul/api"
"github.com/hashicorp/consul/sdk/freeport"
"github.com/hashicorp/consul/sdk/testutil"
Expand All @@ -31,6 +27,11 @@ import (
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/fake"

"github.com/hashicorp/consul-k8s/control-plane/helper/cert"
"github.com/hashicorp/consul-k8s/control-plane/helper/go-discover/mocks"
"github.com/hashicorp/consul-k8s/control-plane/helper/test"
"github.com/hashicorp/consul-k8s/control-plane/subcommand/common"
)

var ns = "default"
Expand Down Expand Up @@ -1284,6 +1285,8 @@ func TestRun_NoLeader(t *testing.T) {
numACLBootCalls++
case "/v1/agent/self":
fmt.Fprintln(w, `{"Config": {"Datacenter": "dc1", "PrimaryDatacenter": "dc1"}}`)
case "/v1/acl/tokens":
fmt.Fprintln(w, `[]`)
Comment on lines +1288 to +1289
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tests get modified because now we do a token list call to ensure we're not recreating server tokens.

default:
fmt.Fprintln(w, "{}")
}
Expand Down Expand Up @@ -1342,6 +1345,10 @@ func TestRun_NoLeader(t *testing.T) {
"PUT",
"/v1/acl/policy",
},
{
"GET",
"/v1/acl/tokens",
},
{
"PUT",
"/v1/acl/token",
Expand Down Expand Up @@ -1496,6 +1503,8 @@ func TestRun_ClientTokensRetry(t *testing.T) {
numPolicyCalls++
case "/v1/agent/self":
fmt.Fprintln(w, `{"Config": {"Datacenter": "dc1", "PrimaryDatacenter": "dc1"}}`)
case "/v1/acl/tokens":
fmt.Fprintln(w, `[]`)
default:
fmt.Fprintln(w, "{}")
}
Expand Down Expand Up @@ -1530,6 +1539,10 @@ func TestRun_ClientTokensRetry(t *testing.T) {
"PUT",
"/v1/acl/policy",
},
{
"GET",
"/v1/acl/tokens",
},
{
"PUT",
"/v1/acl/token",
Expand Down Expand Up @@ -1558,8 +1571,8 @@ func TestRun_ClientTokensRetry(t *testing.T) {
}, consulAPICalls)
}

// Test if there is an old bootstrap Secret we assume the servers were
// bootstrapped already and continue on to the next step.
// Test if there is an old bootstrap Secret we still try to create and set
// server tokens.
func TestRun_AlreadyBootstrapped(t *testing.T) {
t.Parallel()
require := require.New(t)
Expand All @@ -1581,6 +1594,8 @@ func TestRun_AlreadyBootstrapped(t *testing.T) {
switch r.URL.Path {
case "/v1/agent/self":
fmt.Fprintln(w, `{"Config": {"Datacenter": "dc1", "PrimaryDatacenter": "dc1"}}`)
case "/v1/acl/tokens":
fmt.Fprintln(w, `[]`)
default:
// Send an empty JSON response with code 200 to all calls.
fmt.Fprintln(w, "{}")
Expand Down Expand Up @@ -1629,15 +1644,27 @@ func TestRun_AlreadyBootstrapped(t *testing.T) {

// Test that the expected API calls were made.
require.Equal([]APICall{
// We only expect the calls for creating client tokens
// and updating the server policy.
// We expect calls for updating the server policy, setting server tokens,
// and updating client policy.
{
"PUT",
"/v1/acl/policy",
},
{
"GET",
"/v1/agent/self",
"/v1/acl/tokens",
},
{
"PUT",
"/v1/acl/policy",
"/v1/acl/token",
},
{
"PUT",
"/v1/agent/token/agent",
},
{
"GET",
"/v1/agent/self",
},
{
"PUT",
Expand All @@ -1650,6 +1677,81 @@ func TestRun_AlreadyBootstrapped(t *testing.T) {
}, consulAPICalls)
}

// Test if there is an old bootstrap Secret and the server token exists
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this to test that we don't re-create the server tokens if the token already exists.

// that we don't try and recreate the token.
func TestRun_AlreadyBootstrapped_ServerTokenExists(t *testing.T) {
t.Parallel()
require := require.New(t)

// First set everything up with ACLs bootstrapped.
bootToken := "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee"
k8s, testAgent := completeBootstrappedSetup(t, bootToken)
setUpK8sServiceAccount(t, k8s, ns)
defer testAgent.Stop()
k8s.CoreV1().Secrets(ns).Create(context.Background(), &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: resourcePrefix + "-bootstrap-acl-token",
},
Data: map[string][]byte{
"token": []byte(bootToken),
},
}, metav1.CreateOptions{})

consulClient, err := api.NewClient(&api.Config{
Address: testAgent.HTTPAddr,
Token: bootToken,
})
require.NoError(err)
ui := cli.NewMockUi()
cmd := Command{
UI: ui,
clientset: k8s,
}

// Create the server policy and token _before_ we run the command.
agentPolicyRules, err := cmd.agentRules()
require.NoError(err)
policy, _, err := consulClient.ACL().PolicyCreate(&api.ACLPolicy{
Name: "agent-token",
Description: "Agent Token Policy",
Rules: agentPolicyRules,
}, nil)
require.NoError(err)
_, _, err = consulClient.ACL().TokenCreate(&api.ACLToken{
Description: fmt.Sprintf("Server Token for %s", strings.Split(testAgent.HTTPAddr, ":")[0]),
Policies: []*api.ACLTokenPolicyLink{
{
Name: policy.Name,
},
},
}, nil)
require.NoError(err)

// Run the command.
cmdArgs := []string{
"-timeout=1m",
"-k8s-namespace", ns,
"-server-address", strings.Split(testAgent.HTTPAddr, ":")[0],
"-server-port", strings.Split(testAgent.HTTPAddr, ":")[1],
"-resource-prefix", resourcePrefix,
}

responseCode := cmd.Run(cmdArgs)
require.Equal(0, responseCode, ui.ErrorWriter.String())

// Check that only one server token exists, i.e. it didn't create an
// extra token.
tokens, _, err := consulClient.ACL().TokenList(nil)
require.NoError(err)
count := 0
for _, token := range tokens {
if len(token.Policies) == 1 && token.Policies[0].Name == policy.Name {
count++
}
}
require.Equal(1, count)
}

// Test if there is a provided bootstrap we skip bootstrapping of the servers
// and continue on to the next step.
func TestRun_SkipBootstrapping_WhenBootstrapTokenIsProvided(t *testing.T) {
Expand Down
Loading