Remove cleanup from Link controller and move it to MonitorHCPLink

Previously, the Link Controller was responsible for cleaning up the
HCP-related files on the file system. This change makes it so
MonitorHCPLink handles this cleanup. As a result, we are able to remove
the PlacementEachServer placement strategy for the Link controller
because it no longer needs to do this per-node cleanup.
pull/20401/head
Nick Cellino 2024-01-30 17:29:44 -05:00
parent a276e68078
commit 1b5e05fb95
12 changed files with 74 additions and 203 deletions

View File

@ -1018,13 +1018,14 @@ func isV1CatalogRequest(rpcName string) bool {
}
func (s *Server) registerControllers(deps Deps, proxyUpdater ProxyUpdater) error {
hcpctl.RegisterControllers(s.controllerManager, hcpctl.ControllerDependencies{
ResourceApisEnabled: s.useV2Resources,
HCPAllowV2ResourceApis: s.hcpAllowV2Resources,
CloudConfig: deps.HCP.Config,
DataDir: deps.HCP.DataDir,
HCPManager: s.hcpManager,
})
hcpctl.RegisterControllers(
s.controllerManager, hcpctl.ControllerDependencies{
ResourceApisEnabled: s.useV2Resources,
HCPAllowV2ResourceApis: s.hcpAllowV2Resources,
CloudConfig: deps.HCP.Config,
HCPManager: s.hcpManager,
},
)
// When not enabled, the v1 tenancy bridge is used by default.
if s.useV2Tenancy {

View File

@ -18,17 +18,17 @@ import (
"strings"
"time"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/consul/agent/connect"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/lib/retry"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid"
)
const (
SubDir = "hcp-config"
CAFileName = "server-tls-cas.pem"
CertFileName = "server-tls-cert.pem"
ConfigFileName = "server-config.json"
@ -128,7 +128,7 @@ func persistAndProcessConfig(dataDir string, devMode bool, bsCfg *hcpclient.Boot
}
// Create subdir if it's not already there.
dir := filepath.Join(dataDir, SubDir)
dir := filepath.Join(dataDir, constants.SubDir)
if err := lib.EnsurePath(dir, true); err != nil {
return "", fmt.Errorf("failed to ensure directory %q: %w", dir, err)
}
@ -273,7 +273,7 @@ func LoadPersistedBootstrapConfig(dataDir string, ui UI) (*RawBootstrapConfig, b
return nil, false
}
dir := filepath.Join(dataDir, SubDir)
dir := filepath.Join(dataDir, constants.SubDir)
_, err := os.Stat(filepath.Join(dir, SuccessFileName))
if os.IsNotExist(err) {
@ -309,7 +309,7 @@ func LoadPersistedBootstrapConfig(dataDir string, ui UI) (*RawBootstrapConfig, b
}
func loadBootstrapConfigJSON(dataDir string) (string, error) {
filename := filepath.Join(dataDir, SubDir, ConfigFileName)
filename := filepath.Join(dataDir, constants.SubDir, ConfigFileName)
_, err := os.Stat(filename)
if os.IsNotExist(err) {
@ -461,7 +461,7 @@ func ValidateTLSCerts(cert, key string, caCerts []string) error {
// LoadManagementToken returns the management token, either by loading it from the persisted
// token config file or by fetching it from HCP if the token file does not exist.
func LoadManagementToken(ctx context.Context, logger hclog.Logger, client hcpclient.Client, dataDir string) (string, error) {
hcpCfgDir := filepath.Join(dataDir, SubDir)
hcpCfgDir := filepath.Join(dataDir, constants.SubDir)
token, err := loadManagementToken(hcpCfgDir)
if err != nil {

View File

@ -11,15 +11,18 @@ import (
"testing"
"time"
"github.com/mitchellh/cli"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/hashicorp/consul/tlsutil"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-uuid"
"github.com/mitchellh/cli"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)
func Test_loadPersistedBootstrapConfig(t *testing.T) {
@ -37,7 +40,7 @@ func Test_loadPersistedBootstrapConfig(t *testing.T) {
run := func(t *testing.T, tc testCase) {
dataDir := testutil.TempDir(t, "load-bootstrap-cfg")
dir := filepath.Join(dataDir, SubDir)
dir := filepath.Join(dataDir, constants.SubDir)
// Do some common setup as if we received config from HCP and persisted it to disk.
require.NoError(t, lib.EnsurePath(dir, true))
@ -264,7 +267,7 @@ func TestLoadManagementToken(t *testing.T) {
run := func(t *testing.T, tc testCase) {
dataDir := testutil.TempDir(t, "load-management-token")
hcpCfgDir := filepath.Join(dataDir, SubDir)
hcpCfgDir := filepath.Join(dataDir, constants.SubDir)
if !tc.skipHCPConfigDir {
err := os.Mkdir(hcpCfgDir, 0755)
require.NoError(t, err)

View File

@ -15,6 +15,7 @@ import (
"github.com/hashicorp/consul/agent/config"
"github.com/hashicorp/consul/agent/hcp/bootstrap"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
)
@ -161,7 +162,7 @@ func finalizeRuntimeConfig(rc *config.RuntimeConfig, cfg *bootstrap.RawBootstrap
// validatePersistedConfig attempts to load persisted config to check for errors and basic validity.
// Errors here will raise issues like referencing unsupported config fields.
func validatePersistedConfig(dataDir string) error {
filename := filepath.Join(dataDir, bootstrap.SubDir, bootstrap.ConfigFileName)
filename := filepath.Join(dataDir, constants.SubDir, bootstrap.ConfigFileName)
_, err := config.Load(config.LoadOpts{
ConfigFiles: []string{filename},
HCL: []string{

View File

@ -13,13 +13,15 @@ import (
"path/filepath"
"testing"
"github.com/mitchellh/cli"
"github.com/stretchr/testify/require"
"github.com/hashicorp/consul/agent/config"
"github.com/hashicorp/consul/agent/hcp"
"github.com/hashicorp/consul/agent/hcp/bootstrap"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/lib"
"github.com/mitchellh/cli"
"github.com/stretchr/testify/require"
)
func TestBootstrapConfigLoader(t *testing.T) {
@ -274,7 +276,7 @@ func TestLoadConfig_Persistence(t *testing.T) {
// New clusters should have received and persisted the whole suite of config.
verifyFn: func(t *testing.T, rc *config.RuntimeConfig) {
dir := filepath.Join(rc.DataDir, bootstrap.SubDir)
dir := filepath.Join(rc.DataDir, constants.SubDir)
entries, err := os.ReadDir(dir)
require.NoError(t, err)
@ -310,7 +312,7 @@ func TestLoadConfig_Persistence(t *testing.T) {
// Existing clusters should have only received and persisted the management token.
verifyFn: func(t *testing.T, rc *config.RuntimeConfig) {
dir := filepath.Join(rc.DataDir, bootstrap.SubDir)
dir := filepath.Join(rc.DataDir, constants.SubDir)
entries, err := os.ReadDir(dir)
require.NoError(t, err)
@ -347,7 +349,7 @@ func TestValidatePersistedConfig(t *testing.T) {
require.NoError(t, err)
t.Cleanup(func() { os.RemoveAll(dataDir) })
dir := filepath.Join(dataDir, bootstrap.SubDir)
dir := filepath.Join(dataDir, constants.SubDir)
require.NoError(t, lib.EnsurePath(dir, true))
if tc.configContents != "" {

View File

@ -0,0 +1,9 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
// Package constants declares some constants for use in the HCP bootstrapping
// process. It is in its own package with no other dependencies in order
// to avoid a dependency cycle.
package constants
const SubDir = "hcp-config"

View File

@ -5,9 +5,12 @@ package hcp
import (
"context"
"os"
"path/filepath"
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/agent/hcp/config"
pbhcp "github.com/hashicorp/consul/proto-public/pbhcp/v2"
@ -31,6 +34,16 @@ func MonitorHCPLink(
for watchEvent := range hcpLinkEventCh {
if watchEvent.GetDelete() != nil {
logger.Debug("HCP Link deleted, stopping HCP manager")
if dataDir != "" {
hcpConfigDir := filepath.Join(dataDir, constants.SubDir)
logger.Debug("deleting hcp-config dir", "dir", hcpConfigDir)
err := os.RemoveAll(hcpConfigDir)
if err != nil {
logger.Error("failed to delete hcp-config dir", "dir", hcpConfigDir, "err", err)
}
}
err := m.Stop()
if err != nil {
logger.Error("error stopping HCP manager", "error", err)

View File

@ -6,6 +6,8 @@ package hcp
import (
"context"
"io"
"os"
"path/filepath"
"sync"
"testing"
@ -15,10 +17,12 @@ import (
"github.com/hashicorp/go-hclog"
"github.com/hashicorp/consul/agent/hcp/bootstrap/constants"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/agent/hcp/config"
pbhcp "github.com/hashicorp/consul/proto-public/pbhcp/v2"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/hashicorp/consul/sdk/testutil"
)
func TestMonitorHCPLink_Ok(t *testing.T) {
@ -37,6 +41,9 @@ func TestMonitorHCPLink_Ok(t *testing.T) {
return "test-mgmt-token", nil
}
dataDir := testutil.TempDir(t, "test-link-controller")
os.Mkdir(filepath.Join(dataDir, constants.SubDir), os.ModeDir)
var wg sync.WaitGroup
wg.Add(1)
@ -44,8 +51,7 @@ func TestMonitorHCPLink_Ok(t *testing.T) {
defer wg.Done()
MonitorHCPLink(
ctx, hclog.New(&hclog.LoggerOptions{Output: io.Discard}), mgr, linkWatchCh, mockHcpClientFn,
loadMgmtTokenFn,
"",
loadMgmtTokenFn, dataDir,
)
}()
@ -98,6 +104,12 @@ func TestMonitorHCPLink_Ok(t *testing.T) {
// Wait for MonitorHCPLink to return before assertions run
close(linkWatchCh)
wg.Wait()
// Ensure hcp-config directory is removed
file := filepath.Join(dataDir, constants.SubDir)
if _, err := os.Stat(file); err == nil || !os.IsNotExist(err) {
require.Fail(t, "should have removed hcp-config directory")
}
}
func TestMonitorHCPLink_Ok_ReadOnly(t *testing.T) {

View File

@ -19,7 +19,6 @@ import (
"github.com/hashicorp/consul/agent/hcp/config"
"github.com/hashicorp/consul/internal/controller"
"github.com/hashicorp/consul/internal/hcp/internal/types"
"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/internal/storage"
pbhcp "github.com/hashicorp/consul/proto-public/pbhcp/v2"
"github.com/hashicorp/consul/proto-public/pbresource"
@ -44,16 +43,9 @@ func LinkController(
hcpAllowV2ResourceApis bool,
hcpClientFn HCPClientFn,
cfg config.CloudConfig,
dataDir string,
hcpManager hcp.Manager,
) *controller.Controller {
return controller.NewController("link", pbhcp.LinkType).
// Placement is configured to each server so that the HCP manager is started
// on each server. We plan to implement an alternative strategy to starting
// the HCP manager so that the controller placement will eventually only be
// on the leader.
// https://hashicorp.atlassian.net/browse/CC-7364
WithPlacement(controller.PlacementEachServer).
WithInitializer(
&linkInitializer{
cloudConfig: cfg,
@ -64,7 +56,6 @@ func LinkController(
resourceApisEnabled: resourceApisEnabled,
hcpAllowV2ResourceApis: hcpAllowV2ResourceApis,
hcpClientFn: hcpClientFn,
dataDir: dataDir,
hcpManager: hcpManager,
},
)
@ -74,7 +65,6 @@ type linkReconciler struct {
resourceApisEnabled bool
hcpAllowV2ResourceApis bool
hcpClientFn HCPClientFn
dataDir string
hcpManager hcp.Manager
}
@ -106,7 +96,7 @@ func (r *linkReconciler) Reconcile(ctx context.Context, rt controller.Runtime, r
switch {
case status.Code(err) == codes.NotFound:
rt.Logger.Trace("link has been deleted")
return cleanup(rt, r.hcpManager, r.dataDir)
return nil
case err != nil:
rt.Logger.Error("the resource service has returned an unexpected error", "error", err)
return err
@ -119,26 +109,6 @@ func (r *linkReconciler) Reconcile(ctx context.Context, rt controller.Runtime, r
return err
}
if err = addFinalizer(ctx, rt, res); err != nil {
rt.Logger.Error("error adding finalizer to link resource", "error", err)
return err
}
if resource.IsMarkedForDeletion(res) {
if err = cleanup(rt, r.hcpManager, r.dataDir); err != nil {
rt.Logger.Error("error cleaning up link resource", "error", err)
return err
}
err := ensureDeleted(ctx, rt, res)
if err != nil {
rt.Logger.Error("error deleting link resource", "error", err)
return err
}
return nil
}
// Validation - Ensure V2 Resource APIs are not enabled unless hcpAllowV2ResourceApis flag is set
var newStatus *pbresource.Status
if r.resourceApisEnabled && !r.hcpAllowV2ResourceApis {

View File

@ -6,29 +6,24 @@ package link
import (
"context"
"fmt"
"os"
"path/filepath"
"testing"
"github.com/hashicorp/go-uuid"
gnmmod "github.com/hashicorp/hcp-sdk-go/clients/cloud-global-network-manager-service/preview/2022-02-15/models"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
gnmmod "github.com/hashicorp/hcp-sdk-go/clients/cloud-global-network-manager-service/preview/2022-02-15/models"
svctest "github.com/hashicorp/consul/agent/grpc-external/services/resource/testing"
"github.com/hashicorp/consul/agent/hcp"
"github.com/hashicorp/consul/agent/hcp/bootstrap"
hcpclient "github.com/hashicorp/consul/agent/hcp/client"
"github.com/hashicorp/consul/agent/hcp/config"
"github.com/hashicorp/consul/internal/controller"
"github.com/hashicorp/consul/internal/hcp/internal/types"
"github.com/hashicorp/consul/internal/resource"
rtest "github.com/hashicorp/consul/internal/resource/resourcetest"
pbhcp "github.com/hashicorp/consul/proto-public/pbhcp/v2"
"github.com/hashicorp/consul/proto-public/pbresource"
"github.com/hashicorp/consul/sdk/testutil"
"github.com/hashicorp/consul/sdk/testutil/retry"
)
type controllerSuite struct {
@ -39,7 +34,6 @@ type controllerSuite struct {
rt controller.Runtime
tenancies []*pbresource.Tenancy
dataDir string
}
func mockHcpClientFn(t *testing.T) (*hcpclient.MockClient, HCPClientFn) {
@ -74,17 +68,6 @@ func TestLinkController(t *testing.T) {
func (suite *controllerSuite) deleteResourceFunc(id *pbresource.ID) func() {
return func() {
suite.client.MustDelete(suite.T(), id)
// Ensure hcp-config directory is removed
retry.Run(suite.T(), func(r *retry.R) {
if suite.dataDir != "" {
file := filepath.Join(suite.dataDir, bootstrap.SubDir)
if _, err := os.Stat(file); !os.IsNotExist(err) {
r.Fatalf("should have removed hcp-config directory")
}
}
})
suite.client.WaitForDeletion(suite.T(), id)
}
}
@ -99,28 +82,14 @@ func (suite *controllerSuite) TestController_Ok() {
AccessLevel: &readWrite,
}, nil)
token, err := uuid.GenerateUUID()
require.NoError(suite.T(), err)
mockClient.EXPECT().FetchBootstrap(mock.Anything).
Return(&hcpclient.BootstrapConfig{
ManagementToken: token,
ConsulConfig: "{}",
}, nil).Once()
hcpMgr := hcp.NewMockManager(suite.T())
hcpMgr.EXPECT().GetCloudConfig().Return(config.CloudConfig{})
hcpMgr.EXPECT().UpdateConfig(mock.Anything, mock.Anything)
hcpMgr.EXPECT().Start(mock.Anything).Return(nil)
hcpMgr.EXPECT().Stop().Return(nil)
dataDir := testutil.TempDir(suite.T(), "test-link-controller")
suite.dataDir = dataDir
mgr.Register(LinkController(
false,
false,
mockClientFn,
config.CloudConfig{},
dataDir,
hcpMgr,
))
mgr.SetRaftLeader(true)
@ -138,11 +107,6 @@ func (suite *controllerSuite) TestController_Ok() {
suite.T().Cleanup(suite.deleteResourceFunc(link.Id))
// Ensure finalizer was added
suite.client.WaitForResourceState(suite.T(), link.Id, func(t rtest.T, res *pbresource.Resource) {
require.True(t, resource.HasFinalizer(res, StatusKey), "link resource does not have finalizer")
})
suite.client.WaitForStatusCondition(suite.T(), link.Id, StatusKey, ConditionLinked(linkData.ResourceId))
var updatedLink pbhcp.Link
updatedLinkResource := suite.client.WaitForNewVersion(suite.T(), link.Id, link.Version)
@ -170,19 +134,12 @@ func (suite *controllerSuite) TestController_Initialize() {
hcpMgr := hcp.NewMockManager(suite.T())
hcpMgr.EXPECT().GetCloudConfig().Return(cloudCfg)
hcpMgr.EXPECT().UpdateConfig(mock.Anything, mock.Anything)
hcpMgr.EXPECT().Start(mock.Anything).Return(nil)
hcpMgr.EXPECT().Stop().Return(nil)
dataDir := testutil.TempDir(suite.T(), "test-link-controller")
suite.dataDir = dataDir
mgr.Register(LinkController(
false,
false,
mockClientFn,
cloudCfg,
dataDir,
hcpMgr,
))
mgr.SetRaftLeader(true)
@ -214,17 +171,13 @@ func (suite *controllerSuite) TestControllerResourceApisEnabled_LinkDisabled() {
// Run the controller manager
mgr := controller.NewManager(suite.client, suite.rt.Logger)
_, mockClientFunc := mockHcpClientFn(suite.T())
dataDir := testutil.TempDir(suite.T(), "test-link-controller")
suite.dataDir = dataDir
hcpMgr := hcp.NewMockManager(suite.T())
hcpMgr.EXPECT().Stop().Return(nil)
mgr.Register(LinkController(
true,
false,
mockClientFunc,
config.CloudConfig{},
dataDir,
hcpMgr,
))
mgr.SetRaftLeader(true)
@ -252,28 +205,14 @@ func (suite *controllerSuite) TestControllerResourceApisEnabledWithOverride_Link
HCPPortalURL: "http://test.com",
}, nil)
token, err := uuid.GenerateUUID()
require.NoError(suite.T(), err)
mockClient.EXPECT().FetchBootstrap(mock.Anything).
Return(&hcpclient.BootstrapConfig{
ManagementToken: token,
ConsulConfig: "{}",
}, nil).Once()
dataDir := testutil.TempDir(suite.T(), "test-link-controller")
suite.dataDir = dataDir
hcpMgr := hcp.NewMockManager(suite.T())
hcpMgr.EXPECT().GetCloudConfig().Return(config.CloudConfig{})
hcpMgr.EXPECT().UpdateConfig(mock.Anything, mock.Anything)
hcpMgr.EXPECT().Start(mock.Anything).Return(nil)
hcpMgr.EXPECT().Stop().Return(nil)
mgr.Register(LinkController(
true,
true,
mockClientFunc,
config.CloudConfig{},
dataDir,
hcpMgr,
))
@ -321,19 +260,14 @@ func (suite *controllerSuite) TestController_GetClusterError() {
mockClient, mockClientFunc := mockHcpClientFn(t)
mockClient.EXPECT().GetCluster(mock.Anything).Return(nil, tc.expectErr)
dataDir := testutil.TempDir(t, "test-link-controller")
suite.dataDir = dataDir
hcpMgr := hcp.NewMockManager(t)
hcpMgr.EXPECT().GetCloudConfig().Return(config.CloudConfig{})
hcpMgr.EXPECT().Stop().Return(nil)
mgr.Register(LinkController(
true,
true,
mockClientFunc,
config.CloudConfig{},
dataDir,
hcpMgr,
))

View File

@ -1,72 +0,0 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1
package link
import (
"context"
"os"
"path/filepath"
"github.com/hashicorp/consul/agent/hcp"
"github.com/hashicorp/consul/agent/hcp/bootstrap"
"github.com/hashicorp/consul/internal/controller"
"github.com/hashicorp/consul/internal/resource"
"github.com/hashicorp/consul/proto-public/pbresource"
)
func cleanup(rt controller.Runtime, hcpManager hcp.Manager, dataDir string) error {
rt.Logger.Trace("cleaning up link resource")
if dataDir != "" {
hcpConfigDir := filepath.Join(dataDir, bootstrap.SubDir)
rt.Logger.Debug("deleting hcp-config dir", "dir", hcpConfigDir)
err := os.RemoveAll(hcpConfigDir)
if err != nil {
rt.Logger.Error("failed to delete hcp-config dir", "dir", hcpConfigDir, "err", err)
return err
}
}
return nil
}
func addFinalizer(ctx context.Context, rt controller.Runtime, res *pbresource.Resource) error {
// The statusKey doubles as the finalizer name for the link resource
if resource.HasFinalizer(res, StatusKey) {
rt.Logger.Trace("already has finalizer")
return nil
}
// Finalizer hasn't been written, so add it.
resource.AddFinalizer(res, StatusKey)
_, err := rt.Client.Write(ctx, &pbresource.WriteRequest{Resource: res})
if err != nil {
return err
}
rt.Logger.Trace("added finalizer")
return err
}
// ensureDeleted makes sure a link is finally deleted
func ensureDeleted(ctx context.Context, rt controller.Runtime, res *pbresource.Resource) error {
// Remove finalizer if present
if resource.HasFinalizer(res, StatusKey) {
resource.RemoveFinalizer(res, StatusKey)
_, err := rt.Client.Write(ctx, &pbresource.WriteRequest{Resource: res})
if err != nil {
return err
}
rt.Logger.Trace("removed finalizer")
}
// Finally, delete the link
_, err := rt.Client.Delete(ctx, &pbresource.DeleteRequest{Id: res.Id})
if err != nil {
return err
}
// Success
rt.Logger.Trace("finally deleted")
return nil
}

View File

@ -15,7 +15,6 @@ type Dependencies struct {
CloudConfig config.CloudConfig
ResourceApisEnabled bool
HCPAllowV2ResourceApis bool
DataDir string
HCPManager *hcp.HCPManager
}
@ -25,7 +24,6 @@ func Register(mgr *controller.Manager, deps Dependencies) {
deps.HCPAllowV2ResourceApis,
link.DefaultHCPClientFn,
deps.CloudConfig,
deps.DataDir,
deps.HCPManager,
))