Add refreshes and retries to server-acl-init job

.changelog/3137.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+control-plane: fixes an issue with the server-acl-init job where the job would fail on upgrades due to consul server ip address changes.
+```

control-plane/consul/dynamic.go

@@ -0,0 +1,65 @@
+package consul
+
+import (
+	"time"
+
+	capi "github.com/hashicorp/consul/api"
+)
+
+type DynamicClient struct {
+	ConsulClient *capi.Client
+	Config       *Config
+	watcher      ServerConnectionManager
+}
+
+func NewDynamicClientFromConnMgr(config *Config, watcher ServerConnectionManager) (*DynamicClient, error) {
+	client, err := NewClientFromConnMgr(config, watcher)
+	if err != nil {
+		return nil, err
+	}
+	return &DynamicClient{
+		ConsulClient: client,
+		Config:       config,
+		watcher:      watcher,
+	}, nil
+}
+
+func (d *DynamicClient) RefreshClient() error {
+	var err error
+	var client *capi.Client
+	// If the watcher is not set then we did not create the client using NewDynamicClientFromConnMgr and are using it in
+	// testing
+	// TODO: Use watcher in testing ;)
+	if d.watcher == nil {
+		return nil
+	}
+	client, err = NewClientFromConnMgr(d.Config, d.watcher)
+	if err != nil {
+		return err
+	}
+	d.ConsulClient = client
+	return nil
+}
+
+func NewDynamicClientWithTimeout(config *capi.Config, consulAPITimeout time.Duration) (*DynamicClient, error) {
+	client, err := NewClient(config, consulAPITimeout)
+	if err != nil {
+		return nil, err
+	}
+	return &DynamicClient{
+		ConsulClient: client,
+		Config: &Config{
+			APIClientConfig: config,
+		},
+	}, nil
+}
+
+func NewDynamicClient(config *capi.Config) (*DynamicClient, error) {
+	// defaultTimeout is taken from flags.go..
+	defaultTimeout := 5 * time.Second
+	client, err := NewDynamicClientWithTimeout(config, defaultTimeout)
+	if err != nil {
+		return nil, err
+	}
+	return client, nil
+}

control-plane/consul/dynamic_test.go

@@ -0,0 +1,73 @@
+package consul
+
+import (
+	"fmt"
+	"net"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strconv"
+	"testing"
+
+	"github.com/hashicorp/consul-server-connection-manager/discovery"
+	capi "github.com/hashicorp/consul/api"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRefreshDynamicClient(t *testing.T) {
+	// Create a server
+	consulServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintln(w, "\"leader\"")
+	}))
+	defer consulServer.Close()
+
+	serverURL, err := url.Parse(consulServer.URL)
+	require.NoError(t, err)
+	serverIP := net.ParseIP(serverURL.Hostname())
+
+	port, err := strconv.Atoi(serverURL.Port())
+	require.NoError(t, err)
+
+	connMgr := &MockServerConnectionManager{}
+
+	// Use a bad IP so that the client call fails
+	badState := discovery.State{
+		Address: discovery.Addr{
+			TCPAddr: net.TCPAddr{
+				IP:   net.ParseIP("126.0.0.1"),
+				Port: port,
+			},
+		},
+	}
+
+	goodState := discovery.State{
+		Address: discovery.Addr{
+			TCPAddr: net.TCPAddr{
+				IP:   serverIP,
+				Port: port,
+			},
+		},
+	}
+
+	// testify/mock has a weird behaviour when returning function calls. You cannot update On("State") to return
+	// something different but instead need to load up the returns. Here we are simulating a bad consul server manager
+	// state and then a good one
+	connMgr.On("State").Return(badState, nil).Once()
+	connMgr.On("State").Return(goodState, nil).Once()
+
+	cfg := capi.DefaultConfig()
+	client, err := NewDynamicClientFromConnMgr(&Config{APIClientConfig: cfg, HTTPPort: port, GRPCPort: port}, connMgr)
+	require.NoError(t, err)
+
+	// Make a request to the bad ip of the server
+	_, err = client.ConsulClient.Status().Leader()
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "connection refused")
+
+	// Refresh the client and make a call to the server now that consul-server-connection-manager state is good
+	err = client.RefreshClient()
+	require.NoError(t, err)
+	leader, err := client.ConsulClient.Status().Leader()
+	require.NoError(t, err)
+	require.Equal(t, "leader", leader)
+}

control-plane/subcommand/server-acl-init/anonymous_token.go

@@ -4,6 +4,7 @@
 package serveraclinit
 
 import (
+	"github.com/hashicorp/consul-k8s/control-plane/consul"
 	"github.com/hashicorp/consul/api"
 )
 
@@ -14,8 +15,8 @@ const (
 
 // configureAnonymousPolicy sets up policies and tokens so that Consul DNS and
 // cross-datacenter Consul connect calls will work.
-func (c *Command) configureAnonymousPolicy(consulClient *api.Client) error {
-	exists, err := checkIfAnonymousTokenPolicyExists(consulClient)
+func (c *Command) configureAnonymousPolicy(client *consul.DynamicClient) error {
+	exists, err := checkIfAnonymousTokenPolicyExists(client)
 	if err != nil {
 		c.log.Error("Error checking if anonymous token policy exists", "err", err)
 		return err
@@ -40,7 +41,7 @@ func (c *Command) configureAnonymousPolicy(consulClient *api.Client) error {
 
 	err = c.untilSucceeds("creating anonymous token policy - PUT /v1/acl/policy",
 		func() error {
-			return c.createOrUpdateACLPolicy(anonPolicy, consulClient)
+			return c.createOrUpdateACLPolicy(anonPolicy, client)
 		})
 	if err != nil {
 		return err
@@ -55,13 +56,17 @@ func (c *Command) configureAnonymousPolicy(consulClient *api.Client) error {
 	// Update anonymous token to include this policy
 	return c.untilSucceeds("updating anonymous token with policy",
 		func() error {
-			_, _, err := consulClient.ACL().TokenUpdate(&aToken, &api.WriteOptions{})
+			err := client.RefreshClient()
+			if err != nil {
+				c.log.Error("could not refresh client", err)
+			}
+			_, _, err = client.ConsulClient.ACL().TokenUpdate(&aToken, &api.WriteOptions{})
 			return err
 		})
 }
 
-func checkIfAnonymousTokenPolicyExists(consulClient *api.Client) (bool, error) {
-	token, _, err := consulClient.ACL().TokenRead(anonymousTokenAccessorID, nil)
+func checkIfAnonymousTokenPolicyExists(client *consul.DynamicClient) (bool, error) {
+	token, _, err := client.ConsulClient.ACL().TokenRead(anonymousTokenAccessorID, nil)
 	if err != nil {
 		return false, err
 	}

control-plane/subcommand/server-acl-init/anonymous_token_test.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/consul-k8s/control-plane/consul"
 	"github.com/hashicorp/consul/api"
 	"github.com/mitchellh/cli"
 	"github.com/stretchr/testify/require"
@@ -39,16 +40,16 @@ func Test_configureAnonymousPolicy(t *testing.T) {
 	require.Equal(t, 0, responseCode, ui.ErrorWriter.String())
 
 	bootToken := getBootToken(t, k8s, resourcePrefix, ns)
-	consul, err := api.NewClient(&api.Config{
+	client, err := consul.NewDynamicClient(&api.Config{
 		Address: consulHTTPAddr,
 		Token:   bootToken,
 	})
 	require.NoError(t, err)
 
-	err = cmd.configureAnonymousPolicy(consul)
+	err = cmd.configureAnonymousPolicy(client)
 	require.NoError(t, err)
 
-	policy, _, err := consul.ACL().PolicyReadByName(anonymousTokenPolicyName, nil)
+	policy, _, err := client.ConsulClient.ACL().PolicyReadByName(anonymousTokenPolicyName, nil)
 	require.NoError(t, err)
 
 	testPolicy := api.ACLPolicy{
@@ -57,13 +58,13 @@ func Test_configureAnonymousPolicy(t *testing.T) {
 		Description: "Anonymous token Policy",
 		Rules:       `acl = "read"`,
 	}
-	readOnlyPolicy, _, err := consul.ACL().PolicyUpdate(&testPolicy, &api.WriteOptions{})
+	readOnlyPolicy, _, err := client.ConsulClient.ACL().PolicyUpdate(&testPolicy, &api.WriteOptions{})
 	require.NoError(t, err)
 
-	err = cmd.configureAnonymousPolicy(consul)
+	err = cmd.configureAnonymousPolicy(client)
 	require.NoError(t, err)
 
-	actualPolicy, _, err := consul.ACL().PolicyReadByName(anonymousTokenPolicyName, nil)
+	actualPolicy, _, err := client.ConsulClient.ACL().PolicyReadByName(anonymousTokenPolicyName, nil)
 	require.NoError(t, err)
 
 	// assert policy is still same.