From 45b1cebf8a277e7af86ff66e5d33ce824395bae6 Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 15:14:57 -0700 Subject: [PATCH 01/10] Add node id/name to config --- agent/config/builder.go | 25 ++++++++----- agent/hcp/client/client.go | 12 +++++-- agent/hcp/client/client_test.go | 53 ++++++++++++++++++++++++++++ agent/hcp/client/metrics_client.go | 2 ++ agent/hcp/client/mock_CloudConfig.go | 7 ++++ agent/hcp/config/config.go | 8 +++++ agent/hcp/deps.go | 11 +++--- agent/hcp/deps_test.go | 11 +++--- agent/setup.go | 5 ++- 9 files changed, 111 insertions(+), 23 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 665688b8c864..ec1e4360defd 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -984,7 +984,7 @@ func (b *builder) build() (rt RuntimeConfig, err error) { AutoEncryptIPSAN: autoEncryptIPSAN, AutoEncryptAllowTLS: autoEncryptAllowTLS, AutoConfig: autoConfig, - Cloud: b.cloudConfigVal(c.Cloud), + Cloud: b.cloudConfigVal(c), ConnectEnabled: connectEnabled, ConnectCAProvider: connectCAProvider, ConnectCAConfig: connectCAConfig, @@ -2541,21 +2541,28 @@ func validateAutoConfigAuthorizer(rt RuntimeConfig) error { return nil } -func (b *builder) cloudConfigVal(v *CloudConfigRaw) hcpconfig.CloudConfig { +func (b *builder) cloudConfigVal(v *Config) hcpconfig.CloudConfig { val := hcpconfig.CloudConfig{ ResourceID: os.Getenv("HCP_RESOURCE_ID"), } - if v == nil { + if v.NodeName != nil { + val.NodeName = *v.NodeName + } + if v.NodeID != nil { + val.NodeID = types.NodeID(*v.NodeID) + } + + if v.Cloud == nil { return val } - val.ClientID = stringVal(v.ClientID) - val.ClientSecret = stringVal(v.ClientSecret) - val.AuthURL = stringVal(v.AuthURL) - val.Hostname = stringVal(v.Hostname) - val.ScadaAddress = stringVal(v.ScadaAddress) + val.ClientID = stringVal(v.Cloud.ClientID) + val.ClientSecret = stringVal(v.Cloud.ClientSecret) + val.AuthURL = stringVal(v.Cloud.AuthURL) + val.Hostname = stringVal(v.Cloud.Hostname) + val.ScadaAddress = stringVal(v.Cloud.ScadaAddress) - if resourceID := stringVal(v.ResourceID); resourceID != "" { + if resourceID := stringVal(v.Cloud.ResourceID); resourceID != "" { val.ResourceID = resourceID } return val diff --git a/agent/hcp/client/client.go b/agent/hcp/client/client.go index 212647c51e87..15ee2cb0f5e4 100644 --- a/agent/hcp/client/client.go +++ b/agent/hcp/client/client.go @@ -313,9 +313,15 @@ func (t *TelemetryConfig) Enabled() (string, bool) { } // DefaultLabels returns a set of string pairs that must be added as attributes to all exported telemetry data. -func (t *TelemetryConfig) DefaultLabels(nodeID string) map[string]string { - labels := map[string]string{ - "node_id": nodeID, // used to delineate Consul nodes in graphs +func (t *TelemetryConfig) DefaultLabels(cfg CloudConfig) map[string]string { + labels := make(map[string]string) + nID, nodeName := cfg.NodeMeta() + nodeID := string(nID) + if nodeID != "" { + labels["node_id"] = nodeID + } + if nodeName != "" { + labels["node_name"] = nodeName } for k, v := range t.Labels { diff --git a/agent/hcp/client/client_test.go b/agent/hcp/client/client_test.go index 8c8a6addd70c..7e789d3d4d79 100644 --- a/agent/hcp/client/client_test.go +++ b/agent/hcp/client/client_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/hashicorp/consul/types" "github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/client/consul_telemetry_service" "github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/models" "github.com/stretchr/testify/mock" @@ -147,3 +148,55 @@ func TestConvertTelemetryConfig(t *testing.T) { }) } } + +func Test_DefaultLabels(t *testing.T) { + for name, tc := range map[string]struct { + mockCloudCfg CloudConfig + expectedLabels map[string]string + }{ + "Success": { + mockCloudCfg: MockCloudCfg{ + NodeID: types.NodeID("nodeyid"), + NodeName: "nodey", + }, + expectedLabels: map[string]string{ + "node_id": "nodeyid", + "node_name": "nodey", + }, + }, + + "NoNodeID": { + mockCloudCfg: MockCloudCfg{ + NodeID: types.NodeID(""), + NodeName: "nodey", + }, + expectedLabels: map[string]string{ + "node_name": "nodey", + }, + }, + "NoNodeName": { + mockCloudCfg: MockCloudCfg{ + NodeID: types.NodeID("nodeyid"), + NodeName: "", + }, + expectedLabels: map[string]string{ + "node_id": "nodeyid", + }, + }, + "Empty": { + mockCloudCfg: MockCloudCfg{ + NodeID: "", + NodeName: "", + }, + expectedLabels: map[string]string{ + "node_id": "nodeyid", + }, + }, + } { + t.Run(name, func(t *testing.T) { + tCfg := &TelemetryConfig{} + labels := tCfg.DefaultLabels(tc.mockCloudCfg) + require.Equal(t, labels, tc.expectedLabels) + }) + } +} diff --git a/agent/hcp/client/metrics_client.go b/agent/hcp/client/metrics_client.go index 7e19c9857a97..1aa4df0bffaf 100644 --- a/agent/hcp/client/metrics_client.go +++ b/agent/hcp/client/metrics_client.go @@ -18,6 +18,7 @@ import ( "golang.org/x/oauth2" "google.golang.org/protobuf/proto" + "github.com/hashicorp/consul/types" "github.com/hashicorp/consul/version" ) @@ -43,6 +44,7 @@ type MetricsClient interface { type CloudConfig interface { HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) Resource() (resource.Resource, error) + NodeMeta() (NodeID types.NodeID, NodeName string) } // otlpClient is an implementation of MetricsClient with a retryable http client for retries and to honor throttle. diff --git a/agent/hcp/client/mock_CloudConfig.go b/agent/hcp/client/mock_CloudConfig.go index 5f2ef50046d7..3af71233a4a6 100644 --- a/agent/hcp/client/mock_CloudConfig.go +++ b/agent/hcp/client/mock_CloudConfig.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "net/url" + "github.com/hashicorp/consul/types" hcpcfg "github.com/hashicorp/hcp-sdk-go/config" "github.com/hashicorp/hcp-sdk-go/profile" "github.com/hashicorp/hcp-sdk-go/resource" @@ -28,6 +29,8 @@ func (m *mockHCPCfg) PortalURL() *url.URL { return &url.URL{} } func (m *mockHCPCfg) Profile() *profile.UserProfile { return nil } type MockCloudCfg struct { + NodeID types.NodeID + NodeName string ConfigErr error ResourceErr error } @@ -45,3 +48,7 @@ func (m MockCloudCfg) Resource() (resource.Resource, error) { func (m MockCloudCfg) HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) { return &mockHCPCfg{}, m.ConfigErr } + +func (m MockCloudCfg) NodeMeta() (types.NodeID, string) { + return m.NodeID, m.NodeName +} diff --git a/agent/hcp/config/config.go b/agent/hcp/config/config.go index 8d1358fa4adf..a4858af075ff 100644 --- a/agent/hcp/config/config.go +++ b/agent/hcp/config/config.go @@ -6,6 +6,7 @@ package config import ( "crypto/tls" + "github.com/hashicorp/consul/types" hcpcfg "github.com/hashicorp/hcp-sdk-go/config" "github.com/hashicorp/hcp-sdk-go/resource" ) @@ -25,6 +26,9 @@ type CloudConfig struct { // TlsConfig for testing. TLSConfig *tls.Config + + NodeID types.NodeID + NodeName string } func (c *CloudConfig) WithTLSConfig(cfg *tls.Config) { @@ -35,6 +39,10 @@ func (c *CloudConfig) Resource() (resource.Resource, error) { return resource.FromString(c.ResourceID) } +func (c *CloudConfig) NodeMeta() (types.NodeID, string) { + return c.NodeID, c.NodeName +} + func (c *CloudConfig) HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) { if c.TLSConfig == nil { c.TLSConfig = &tls.Config{} diff --git a/agent/hcp/deps.go b/agent/hcp/deps.go index f4ad161daba4..eb9640aed209 100644 --- a/agent/hcp/deps.go +++ b/agent/hcp/deps.go @@ -14,7 +14,6 @@ import ( "github.com/hashicorp/consul/agent/hcp/config" "github.com/hashicorp/consul/agent/hcp/scada" "github.com/hashicorp/consul/agent/hcp/telemetry" - "github.com/hashicorp/consul/types" "github.com/hashicorp/go-hclog" ) @@ -25,10 +24,10 @@ type Deps struct { Sink metrics.MetricSink } -func NewDeps(cfg config.CloudConfig, logger hclog.Logger, nodeID types.NodeID) (Deps, error) { +func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) { client, err := hcpclient.NewClient(cfg) if err != nil { - return Deps{}, fmt.Errorf("failed to init client: %w:", err) + return Deps{}, fmt.Errorf("failed to init client: %w", err) } provider, err := scada.New(cfg, logger.Named("scada")) @@ -36,7 +35,7 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger, nodeID types.NodeID) ( return Deps{}, fmt.Errorf("failed to init scada: %w", err) } - sink := sink(client, &cfg, logger.Named("sink"), nodeID) + sink := sink(client, &cfg, logger.Named("sink")) return Deps{ Client: client, @@ -48,7 +47,7 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger, nodeID types.NodeID) ( // sink provides initializes an OTELSink which forwards Consul metrics to HCP. // The sink is only initialized if the server is registered with the management plane (CCM). // This step should not block server initialization, so errors are logged, but not returned. -func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Logger, nodeID types.NodeID) metrics.MetricSink { +func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Logger) metrics.MetricSink { ctx := context.Background() ctx = hclog.WithContext(ctx, logger) @@ -81,7 +80,7 @@ func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Lo sinkOpts := &telemetry.OTELSinkOpts{ Ctx: ctx, Reader: telemetry.NewOTELReader(metricsClient, u, telemetry.DefaultExportInterval), - Labels: telemetryCfg.DefaultLabels(string(nodeID)), + Labels: telemetryCfg.DefaultLabels(cfg), Filters: telemetryCfg.MetricsConfig.Filters, } diff --git a/agent/hcp/deps_test.go b/agent/hcp/deps_test.go index 54ec7b6de478..c880c9d8083a 100644 --- a/agent/hcp/deps_test.go +++ b/agent/hcp/deps_test.go @@ -28,7 +28,10 @@ func TestSink(t *testing.T) { }, }, nil) }, - mockCloudCfg: client.MockCloudCfg{}, + mockCloudCfg: client.MockCloudCfg{ + NodeID: types.NodeID("nodeyid"), + NodeName: "nodey", + }, expectedSink: true, }, "noSinkWhenServerNotRegisteredWithCCM": { @@ -95,12 +98,12 @@ func TestSink(t *testing.T) { c := client.NewMockClient(t) l := hclog.NewNullLogger() test.expect(c) - sinkOpts := sink(c, test.mockCloudCfg, l, types.NodeID("server1234")) + s := sink(c, test.mockCloudCfg, l) if !test.expectedSink { - require.Nil(t, sinkOpts) + require.Nil(t, s) return } - require.NotNil(t, sinkOpts) + require.NotNil(t, s) }) } } diff --git a/agent/setup.go b/agent/setup.go index bf1b4d004077..4d5d0feed7a1 100644 --- a/agent/setup.go +++ b/agent/setup.go @@ -138,7 +138,10 @@ func NewBaseDeps(configLoader ConfigLoader, logOut io.Writer, providedLogger hcl var extraSinks []metrics.MetricSink if cfg.IsCloudEnabled() { - d.HCP, err = hcp.NewDeps(cfg.Cloud, d.Logger.Named("hcp"), cfg.NodeID) + // This values is set late within newNodeIDFromConfig above + cfg.Cloud.NodeID = cfg.NodeID + + d.HCP, err = hcp.NewDeps(cfg.Cloud, d.Logger.Named("hcp")) if err != nil { return d, err } From 0701d017ddb9191ed0ab6eeb403e9441578abc84 Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 15:27:09 -0700 Subject: [PATCH 02/10] Fixing linting errors --- agent/config/builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index ec1e4360defd..5a61de81006f 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2541,7 +2541,7 @@ func validateAutoConfigAuthorizer(rt RuntimeConfig) error { return nil } -func (b *builder) cloudConfigVal(v *Config) hcpconfig.CloudConfig { +func (b *builder) cloudConfigVal(v Config) hcpconfig.CloudConfig { val := hcpconfig.CloudConfig{ ResourceID: os.Getenv("HCP_RESOURCE_ID"), } From 507286fbb7893113909e1e649e71dd1ed27cc535 Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:20:00 -0700 Subject: [PATCH 03/10] Refactor metrics client out of sink and use di --- agent/hcp/client/client.go | 2 +- agent/hcp/client/client_test.go | 17 +++++++------- agent/hcp/client/metrics_client.go | 2 +- agent/hcp/client/metrics_client_test.go | 4 ++-- agent/hcp/deps.go | 29 ++++++++++++++--------- agent/hcp/deps_test.go | 31 +++++++++---------------- 6 files changed, 41 insertions(+), 44 deletions(-) diff --git a/agent/hcp/client/client.go b/agent/hcp/client/client.go index 15ee2cb0f5e4..68833fdc84a8 100644 --- a/agent/hcp/client/client.go +++ b/agent/hcp/client/client.go @@ -313,7 +313,7 @@ func (t *TelemetryConfig) Enabled() (string, bool) { } // DefaultLabels returns a set of string pairs that must be added as attributes to all exported telemetry data. -func (t *TelemetryConfig) DefaultLabels(cfg CloudConfig) map[string]string { +func (t *TelemetryConfig) DefaultLabels(cfg config.CloudConfig) map[string]string { labels := make(map[string]string) nID, nodeName := cfg.NodeMeta() nodeID := string(nID) diff --git a/agent/hcp/client/client_test.go b/agent/hcp/client/client_test.go index 7e789d3d4d79..0292fa3fab22 100644 --- a/agent/hcp/client/client_test.go +++ b/agent/hcp/client/client_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/hashicorp/consul/agent/hcp/config" "github.com/hashicorp/consul/types" "github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/client/consul_telemetry_service" "github.com/hashicorp/hcp-sdk-go/clients/cloud-consul-telemetry-gateway/preview/2023-04-14/models" @@ -151,11 +152,11 @@ func TestConvertTelemetryConfig(t *testing.T) { func Test_DefaultLabels(t *testing.T) { for name, tc := range map[string]struct { - mockCloudCfg CloudConfig + cfg config.CloudConfig expectedLabels map[string]string }{ "Success": { - mockCloudCfg: MockCloudCfg{ + cfg: config.CloudConfig{ NodeID: types.NodeID("nodeyid"), NodeName: "nodey", }, @@ -166,7 +167,7 @@ func Test_DefaultLabels(t *testing.T) { }, "NoNodeID": { - mockCloudCfg: MockCloudCfg{ + cfg: config.CloudConfig{ NodeID: types.NodeID(""), NodeName: "nodey", }, @@ -175,7 +176,7 @@ func Test_DefaultLabels(t *testing.T) { }, }, "NoNodeName": { - mockCloudCfg: MockCloudCfg{ + cfg: config.CloudConfig{ NodeID: types.NodeID("nodeyid"), NodeName: "", }, @@ -184,18 +185,16 @@ func Test_DefaultLabels(t *testing.T) { }, }, "Empty": { - mockCloudCfg: MockCloudCfg{ + cfg: config.CloudConfig{ NodeID: "", NodeName: "", }, - expectedLabels: map[string]string{ - "node_id": "nodeyid", - }, + expectedLabels: map[string]string{}, }, } { t.Run(name, func(t *testing.T) { tCfg := &TelemetryConfig{} - labels := tCfg.DefaultLabels(tc.mockCloudCfg) + labels := tCfg.DefaultLabels(tc.cfg) require.Equal(t, labels, tc.expectedLabels) }) } diff --git a/agent/hcp/client/metrics_client.go b/agent/hcp/client/metrics_client.go index 1aa4df0bffaf..4b59e1a42a7c 100644 --- a/agent/hcp/client/metrics_client.go +++ b/agent/hcp/client/metrics_client.go @@ -56,7 +56,7 @@ type otlpClient struct { // NewMetricsClient returns a configured MetricsClient. // The current implementation uses otlpClient to provide retry functionality. -func NewMetricsClient(cfg CloudConfig, ctx context.Context) (MetricsClient, error) { +func NewMetricsClient(ctx context.Context, cfg CloudConfig) (MetricsClient, error) { if cfg == nil { return nil, fmt.Errorf("failed to init telemetry client: provide valid cloudCfg (Cloud Configuration for TLS)") } diff --git a/agent/hcp/client/metrics_client_test.go b/agent/hcp/client/metrics_client_test.go index e80996fcf5eb..1034c16bd7c6 100644 --- a/agent/hcp/client/metrics_client_test.go +++ b/agent/hcp/client/metrics_client_test.go @@ -51,7 +51,7 @@ func TestNewMetricsClient(t *testing.T) { }, } { t.Run(name, func(t *testing.T) { - client, err := NewMetricsClient(test.cfg, test.ctx) + client, err := NewMetricsClient(test.ctx, test.cfg) if test.wantErr != "" { require.Error(t, err) require.Contains(t, err.Error(), test.wantErr) @@ -95,7 +95,7 @@ func TestExportMetrics(t *testing.T) { })) defer srv.Close() - client, err := NewMetricsClient(MockCloudCfg{}, context.Background()) + client, err := NewMetricsClient(context.Background(), MockCloudCfg{}) require.NoError(t, err) ctx := context.Background() diff --git a/agent/hcp/deps.go b/agent/hcp/deps.go index eb9640aed209..c4faf5970bab 100644 --- a/agent/hcp/deps.go +++ b/agent/hcp/deps.go @@ -25,6 +25,10 @@ type Deps struct { } func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) { + logger = logger.Named("sink") + ctx := context.Background() + ctx = hclog.WithContext(ctx, logger) + client, err := hcpclient.NewClient(cfg) if err != nil { return Deps{}, fmt.Errorf("failed to init client: %w", err) @@ -35,7 +39,13 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) { return Deps{}, fmt.Errorf("failed to init scada: %w", err) } - sink := sink(client, &cfg, logger.Named("sink")) + metricsClient, err := hcpclient.NewMetricsClient(ctx, &cfg) + if err != nil { + logger.Error("failed to init metrics client", "error", err) + return Deps{}, fmt.Errorf("failed to init metrics client: %w", err) + } + + sink := sink(ctx, client, metricsClient, cfg) return Deps{ Client: client, @@ -47,10 +57,13 @@ func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) { // sink provides initializes an OTELSink which forwards Consul metrics to HCP. // The sink is only initialized if the server is registered with the management plane (CCM). // This step should not block server initialization, so errors are logged, but not returned. -func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Logger) metrics.MetricSink { - ctx := context.Background() - ctx = hclog.WithContext(ctx, logger) - +func sink( + ctx context.Context, + hcpClient hcpclient.Client, + metricsClient hcpclient.MetricsClient, + cfg config.CloudConfig, +) metrics.MetricSink { + logger := hclog.FromContext(ctx) reqCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel() @@ -71,12 +84,6 @@ func sink(hcpClient hcpclient.Client, cfg hcpclient.CloudConfig, logger hclog.Lo return nil } - metricsClient, err := hcpclient.NewMetricsClient(cfg, ctx) - if err != nil { - logger.Error("failed to init metrics client", "error", err) - return nil - } - sinkOpts := &telemetry.OTELSinkOpts{ Ctx: ctx, Reader: telemetry.NewOTELReader(metricsClient, u, telemetry.DefaultExportInterval), diff --git a/agent/hcp/deps_test.go b/agent/hcp/deps_test.go index c880c9d8083a..9a90c26d50ad 100644 --- a/agent/hcp/deps_test.go +++ b/agent/hcp/deps_test.go @@ -1,10 +1,11 @@ package hcp import ( + "context" "fmt" "testing" - "github.com/hashicorp/go-hclog" + "github.com/hashicorp/consul/agent/hcp/config" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -16,7 +17,7 @@ func TestSink(t *testing.T) { t.Parallel() for name, test := range map[string]struct { expect func(*client.MockClient) - mockCloudCfg client.CloudConfig + cloudCfg config.CloudConfig expectedSink bool }{ "success": { @@ -28,7 +29,7 @@ func TestSink(t *testing.T) { }, }, nil) }, - mockCloudCfg: client.MockCloudCfg{ + cloudCfg: config.CloudConfig{ NodeID: types.NodeID("nodeyid"), NodeName: "nodey", }, @@ -43,26 +44,13 @@ func TestSink(t *testing.T) { }, }, nil) }, - mockCloudCfg: client.MockCloudCfg{}, + cloudCfg: config.CloudConfig{}, }, "noSinkWhenCCMVerificationFails": { expect: func(mockClient *client.MockClient) { mockClient.EXPECT().FetchTelemetryConfig(mock.Anything).Return(nil, fmt.Errorf("fetch failed")) }, - mockCloudCfg: client.MockCloudCfg{}, - }, - "noSinkWhenMetricsClientInitFails": { - mockCloudCfg: client.MockCloudCfg{ - ConfigErr: fmt.Errorf("test bad hcp config"), - }, - expect: func(mockClient *client.MockClient) { - mockClient.EXPECT().FetchTelemetryConfig(mock.Anything).Return(&client.TelemetryConfig{ - Endpoint: "https://test.com", - MetricsConfig: &client.MetricsConfig{ - Endpoint: "", - }, - }, nil) - }, + cloudCfg: config.CloudConfig{}, }, "failsWithFetchTelemetryFailure": { expect: func(mockClient *client.MockClient) { @@ -96,9 +84,12 @@ func TestSink(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() c := client.NewMockClient(t) - l := hclog.NewNullLogger() + mc := client.MockMetricsClient{} + test.expect(c) - s := sink(c, test.mockCloudCfg, l) + ctx := context.Background() + + s := sink(ctx, c, mc, test.cloudCfg) if !test.expectedSink { require.Nil(t, s) return From 1d0b2dd6a237a9b2dc9852057be7467130272efe Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:24:29 -0700 Subject: [PATCH 04/10] Remove original impl of node meta fn --- agent/hcp/client/client.go | 7 +++---- agent/hcp/client/metrics_client.go | 2 -- agent/hcp/client/mock_CloudConfig.go | 4 ---- agent/hcp/config/config.go | 4 ---- 4 files changed, 3 insertions(+), 14 deletions(-) diff --git a/agent/hcp/client/client.go b/agent/hcp/client/client.go index 68833fdc84a8..1c49fd792471 100644 --- a/agent/hcp/client/client.go +++ b/agent/hcp/client/client.go @@ -315,13 +315,12 @@ func (t *TelemetryConfig) Enabled() (string, bool) { // DefaultLabels returns a set of string pairs that must be added as attributes to all exported telemetry data. func (t *TelemetryConfig) DefaultLabels(cfg config.CloudConfig) map[string]string { labels := make(map[string]string) - nID, nodeName := cfg.NodeMeta() - nodeID := string(nID) + nodeID := string(cfg.NodeID) if nodeID != "" { labels["node_id"] = nodeID } - if nodeName != "" { - labels["node_name"] = nodeName + if cfg.NodeName != "" { + labels["node_name"] = cfg.NodeName } for k, v := range t.Labels { diff --git a/agent/hcp/client/metrics_client.go b/agent/hcp/client/metrics_client.go index 4b59e1a42a7c..eb446101d1f1 100644 --- a/agent/hcp/client/metrics_client.go +++ b/agent/hcp/client/metrics_client.go @@ -18,7 +18,6 @@ import ( "golang.org/x/oauth2" "google.golang.org/protobuf/proto" - "github.com/hashicorp/consul/types" "github.com/hashicorp/consul/version" ) @@ -44,7 +43,6 @@ type MetricsClient interface { type CloudConfig interface { HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) Resource() (resource.Resource, error) - NodeMeta() (NodeID types.NodeID, NodeName string) } // otlpClient is an implementation of MetricsClient with a retryable http client for retries and to honor throttle. diff --git a/agent/hcp/client/mock_CloudConfig.go b/agent/hcp/client/mock_CloudConfig.go index 3af71233a4a6..f44b96c62f48 100644 --- a/agent/hcp/client/mock_CloudConfig.go +++ b/agent/hcp/client/mock_CloudConfig.go @@ -48,7 +48,3 @@ func (m MockCloudCfg) Resource() (resource.Resource, error) { func (m MockCloudCfg) HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) { return &mockHCPCfg{}, m.ConfigErr } - -func (m MockCloudCfg) NodeMeta() (types.NodeID, string) { - return m.NodeID, m.NodeName -} diff --git a/agent/hcp/config/config.go b/agent/hcp/config/config.go index a4858af075ff..319c39e40e94 100644 --- a/agent/hcp/config/config.go +++ b/agent/hcp/config/config.go @@ -39,10 +39,6 @@ func (c *CloudConfig) Resource() (resource.Resource, error) { return resource.FromString(c.ResourceID) } -func (c *CloudConfig) NodeMeta() (types.NodeID, string) { - return c.NodeID, c.NodeName -} - func (c *CloudConfig) HCPConfig(opts ...hcpcfg.HCPConfigOption) (hcpcfg.HCPConfig, error) { if c.TLSConfig == nil { c.TLSConfig = &tls.Config{} From f2eb143b56e77684345e8c3affaf36fbe96d2f18 Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:38:25 -0700 Subject: [PATCH 05/10] Use builder nodeName in cloud initialization --- agent/config/builder.go | 2 +- agent/hcp/client/mock_CloudConfig.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 5a61de81006f..0c032550ac97 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2546,7 +2546,7 @@ func (b *builder) cloudConfigVal(v Config) hcpconfig.CloudConfig { ResourceID: os.Getenv("HCP_RESOURCE_ID"), } if v.NodeName != nil { - val.NodeName = *v.NodeName + val.NodeName = b.nodeName(v.NodeName) } if v.NodeID != nil { val.NodeID = types.NodeID(*v.NodeID) diff --git a/agent/hcp/client/mock_CloudConfig.go b/agent/hcp/client/mock_CloudConfig.go index f44b96c62f48..5f2ef50046d7 100644 --- a/agent/hcp/client/mock_CloudConfig.go +++ b/agent/hcp/client/mock_CloudConfig.go @@ -4,7 +4,6 @@ import ( "crypto/tls" "net/url" - "github.com/hashicorp/consul/types" hcpcfg "github.com/hashicorp/hcp-sdk-go/config" "github.com/hashicorp/hcp-sdk-go/profile" "github.com/hashicorp/hcp-sdk-go/resource" @@ -29,8 +28,6 @@ func (m *mockHCPCfg) PortalURL() *url.URL { return &url.URL{} } func (m *mockHCPCfg) Profile() *profile.UserProfile { return nil } type MockCloudCfg struct { - NodeID types.NodeID - NodeName string ConfigErr error ResourceErr error } From 6fd48ddcf2b52fba7c02fb8ff9edf6d3bd66b4fc Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:40:54 -0700 Subject: [PATCH 06/10] Use the stringVal function in the config builder --- agent/config/builder.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 0c032550ac97..9c31c5b58b6a 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2549,7 +2549,7 @@ func (b *builder) cloudConfigVal(v Config) hcpconfig.CloudConfig { val.NodeName = b.nodeName(v.NodeName) } if v.NodeID != nil { - val.NodeID = types.NodeID(*v.NodeID) + val.NodeID = types.NodeID(stringVal(v.NodeID)) } if v.Cloud == nil { From c70fe1094b8e0fbbfa89677a8e9c8fd7ed324c4b Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:42:07 -0700 Subject: [PATCH 07/10] Clean up checks and rely on built in config fns --- agent/config/builder.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 9c31c5b58b6a..52b5d82fda3e 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2545,12 +2545,10 @@ func (b *builder) cloudConfigVal(v Config) hcpconfig.CloudConfig { val := hcpconfig.CloudConfig{ ResourceID: os.Getenv("HCP_RESOURCE_ID"), } - if v.NodeName != nil { - val.NodeName = b.nodeName(v.NodeName) - } - if v.NodeID != nil { - val.NodeID = types.NodeID(stringVal(v.NodeID)) - } + + nodeID := stringVal(v.NodeID) + val.NodeID = types.NodeID(nodeID) + val.NodeName = b.nodeName(v.NodeName) if v.Cloud == nil { return val From 3f8550ed7e228e602a759d0444f4e33ac49b791a Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Wed, 14 Jun 2023 16:57:34 -0700 Subject: [PATCH 08/10] Adding missed mocked metrics client --- agent/hcp/client/mock_metrics_client.go | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 agent/hcp/client/mock_metrics_client.go diff --git a/agent/hcp/client/mock_metrics_client.go b/agent/hcp/client/mock_metrics_client.go new file mode 100644 index 000000000000..a30b1f1c62c0 --- /dev/null +++ b/agent/hcp/client/mock_metrics_client.go @@ -0,0 +1,5 @@ +package client + +type MockMetricsClient struct { + MetricsClient +} From 80ff6d377566b5bb245e12d9a45e0b8a08eec0cc Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Thu, 15 Jun 2023 11:13:51 -0700 Subject: [PATCH 09/10] Adding in fixes for runtime config tests --- agent/config/runtime_test.go | 7 +++++++ agent/config/testdata/TestRuntimeConfig_Sanitize.golden | 4 +++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index c1cd85ac502f..f868ea964b18 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -619,6 +619,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.NodeName = "a" rt.TLS.NodeName = "a" rt.DataDir = dataDir + rt.Cloud.NodeName = "a" }, }) run(t, testCase{ @@ -630,6 +631,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { expected: func(rt *RuntimeConfig) { rt.NodeID = "a" rt.DataDir = dataDir + rt.Cloud.NodeID = "a" }, }) run(t, testCase{ @@ -2319,6 +2321,8 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.Cloud = hcpconfig.CloudConfig{ // ID is only populated from env if not populated from other sources. ResourceID: "env-id", + NodeName: "thehostname", + NodeID: "", } // server things @@ -2359,6 +2363,7 @@ func TestLoad_IntegrationWithFlags(t *testing.T) { rt.Cloud = hcpconfig.CloudConfig{ // ID is only populated from env if not populated from other sources. ResourceID: "file-id", + NodeName: "thehostname", } // server things @@ -6317,6 +6322,8 @@ func TestLoad_FullConfig(t *testing.T) { Hostname: "DH4bh7aC", AuthURL: "332nCdR2", ScadaAddress: "aoeusth232", + NodeID: types.NodeID("AsUIlw99"), + NodeName: "otlLxGaI", }, DNSAddrs: []net.Addr{tcpAddr("93.95.95.81:7001"), udpAddr("93.95.95.81:7001")}, DNSARecordLimit: 29907, diff --git a/agent/config/testdata/TestRuntimeConfig_Sanitize.golden b/agent/config/testdata/TestRuntimeConfig_Sanitize.golden index b6ee9a98129f..6bb08ff95fed 100644 --- a/agent/config/testdata/TestRuntimeConfig_Sanitize.golden +++ b/agent/config/testdata/TestRuntimeConfig_Sanitize.golden @@ -134,7 +134,9 @@ "ManagementToken": "hidden", "ResourceID": "cluster1", "ScadaAddress": "", - "TLSConfig": null + "TLSConfig": null, + "NodeID": "", + "NodeName": "" }, "ConfigEntryBootstrap": [], "ConnectCAConfig": {}, From d50a6aeed278b8c81e775d4984973d6caff64943 Mon Sep 17 00:00:00 2001 From: Chris Chapman Date: Fri, 16 Jun 2023 11:27:37 -0700 Subject: [PATCH 10/10] Making suggested changes --- agent/config/builder.go | 2 +- agent/hcp/deps.go | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 15ac93b2ba27..8e0bb37ef999 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -2549,7 +2549,7 @@ func (b *builder) cloudConfigVal(v Config) hcpconfig.CloudConfig { val := hcpconfig.CloudConfig{ ResourceID: os.Getenv("HCP_RESOURCE_ID"), } - + // Node id might get overriden in setup.go:142 nodeID := stringVal(v.NodeID) val.NodeID = types.NodeID(nodeID) val.NodeName = b.nodeName(v.NodeName) diff --git a/agent/hcp/deps.go b/agent/hcp/deps.go index c4faf5970bab..e3e83dec9657 100644 --- a/agent/hcp/deps.go +++ b/agent/hcp/deps.go @@ -25,7 +25,6 @@ type Deps struct { } func NewDeps(cfg config.CloudConfig, logger hclog.Logger) (Deps, error) { - logger = logger.Named("sink") ctx := context.Background() ctx = hclog.WithContext(ctx, logger) @@ -63,7 +62,7 @@ func sink( metricsClient hcpclient.MetricsClient, cfg config.CloudConfig, ) metrics.MetricSink { - logger := hclog.FromContext(ctx) + logger := hclog.FromContext(ctx).Named("sink") reqCtx, cancel := context.WithTimeout(ctx, 5*time.Second) defer cancel()