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

Add node id/name to config #17750

Merged
merged 11 commits into from
Jun 16, 2023
Merged

Add node id/name to config #17750

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
45b1ceb
Add node id/name to config
Jun 14, 2023
0701d01
Fixing linting errors
Jun 14, 2023
507286f
Refactor metrics client out of sink and use di
Jun 14, 2023
1d0b2dd
Remove original impl of node meta fn
Jun 14, 2023
f2eb143
Use builder nodeName in cloud initialization
Jun 14, 2023
6fd48dd
Use the stringVal function in the config builder
Jun 14, 2023
c70fe10
Clean up checks and rely on built in config fns
Jun 14, 2023
3f8550e
Adding missed mocked metrics client
Jun 14, 2023
80ff6d3
Adding in fixes for runtime config tests
Jun 15, 2023
896e04e
Merge branch 'main' into chappie/CC-5423
chapmanc Jun 15, 2023
d50a6ae
Making suggested changes
Jun 16, 2023
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
23 changes: 14 additions & 9 deletions agent/config/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -2545,21 +2545,26 @@ 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 {

nodeID := stringVal(v.NodeID)
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit - feel free to ignore if preferred. Might be good to comment that this value gets overwritten and can change later on in setup.go

val.NodeID = types.NodeID(nodeID)
val.NodeName = b.nodeName(v.NodeName)

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
Expand Down
7 changes: 7 additions & 0 deletions agent/config/runtime_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -2326,6 +2328,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
Expand Down Expand Up @@ -2366,6 +2370,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
Expand Down Expand Up @@ -6324,6 +6329,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,
Expand Down
4 changes: 3 additions & 1 deletion agent/config/testdata/TestRuntimeConfig_Sanitize.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@
"ManagementToken": "hidden",
"ResourceID": "cluster1",
"ScadaAddress": "",
"TLSConfig": null
"TLSConfig": null,
"NodeID": "",
"NodeName": ""
},
"ConfigEntryBootstrap": [],
"ConnectCAConfig": {},
Expand Down
11 changes: 8 additions & 3 deletions agent/hcp/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,14 @@ func (t *TelemetryConfig) Enabled() (string, bool) {
}

// DefaultLabels returns a set of <key, value> 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 config.CloudConfig) map[string]string {
labels := make(map[string]string)
jmurret marked this conversation as resolved.
Show resolved Hide resolved
nodeID := string(cfg.NodeID)
if nodeID != "" {
Copy link
Contributor

@Achooo Achooo Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: don't think this should ever be empty since it gets generated in setup.go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah just being safe here.

labels["node_id"] = nodeID
}
if cfg.NodeName != "" {
labels["node_name"] = cfg.NodeName
}

for k, v := range t.Labels {
Expand Down
52 changes: 52 additions & 0 deletions agent/hcp/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ 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"
"github.com/stretchr/testify/mock"
Expand Down Expand Up @@ -147,3 +149,53 @@ func TestConvertTelemetryConfig(t *testing.T) {
})
}
}

func Test_DefaultLabels(t *testing.T) {
for name, tc := range map[string]struct {
cfg config.CloudConfig
expectedLabels map[string]string
}{
"Success": {
cfg: config.CloudConfig{
NodeID: types.NodeID("nodeyid"),
NodeName: "nodey",
},
expectedLabels: map[string]string{
"node_id": "nodeyid",
"node_name": "nodey",
},
},

"NoNodeID": {
cfg: config.CloudConfig{
NodeID: types.NodeID(""),
NodeName: "nodey",
},
expectedLabels: map[string]string{
"node_name": "nodey",
},
},
"NoNodeName": {
cfg: config.CloudConfig{
NodeID: types.NodeID("nodeyid"),
NodeName: "",
},
expectedLabels: map[string]string{
"node_id": "nodeyid",
},
},
"Empty": {
cfg: config.CloudConfig{
NodeID: "",
NodeName: "",
},
expectedLabels: map[string]string{},
},
} {
t.Run(name, func(t *testing.T) {
tCfg := &TelemetryConfig{}
labels := tCfg.DefaultLabels(tc.cfg)
require.Equal(t, labels, tc.expectedLabels)
})
}
}
2 changes: 1 addition & 1 deletion agent/hcp/client/metrics_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,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)")
}
Expand Down
4 changes: 2 additions & 2 deletions agent/hcp/client/metrics_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,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)
Expand Down Expand Up @@ -118,7 +118,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()
Expand Down
5 changes: 5 additions & 0 deletions agent/hcp/client/mock_metrics_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package client

type MockMetricsClient struct {
MetricsClient
}
4 changes: 4 additions & 0 deletions agent/hcp/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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) {
Expand Down
36 changes: 21 additions & 15 deletions agent/hcp/deps.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -25,18 +24,28 @@ 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) {
logger = logger.Named("sink")
Copy link
Contributor

@Achooo Achooo Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: moving this up here would make the Scada init logs have a named prefix of sink.scada. We could move it to line 41 before MetricsClient initialization

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)
return Deps{}, fmt.Errorf("failed to init client: %w", err)
}

provider, err := scada.New(cfg, logger.Named("scada"))
if err != nil {
return Deps{}, fmt.Errorf("failed to init scada: %w", err)
}

sink := sink(client, &cfg, logger.Named("sink"), nodeID)
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulling out the metrics client instantion allows us to pass the configuration in as a CloudConfig instead of an interface as is done for the NewMetricsClient. This allows us to actually access configuration values stored in the config struct.

Copy link
Contributor

@Achooo Achooo Jun 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. We could honestly even remove the interface for the MetricsClient. In my initial implementation, it felt like the easiest way to test, but I have some alternate ideas. We could do that in a follow up PR maybe! Ideally we also cleanup / refactor and test the config object (a rabbit hole :p)!


return Deps{
Client: client,
Expand All @@ -48,10 +57,13 @@ 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 {
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()

Expand All @@ -72,16 +84,10 @@ 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),
Labels: telemetryCfg.DefaultLabels(string(nodeID)),
Labels: telemetryCfg.DefaultLabels(cfg),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct?

Copy link
Contributor Author

@chapmanc chapmanc Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we build up the labels based off the configuration passed in. Likely we will need more configuration values in the future so I opted to pass the whole cfg object now so as more are requested we can just update the cfg and the key inside the DefaultLabels func

Filters: telemetryCfg.MetricsConfig.Filters,
}

Expand Down
38 changes: 16 additions & 22 deletions agent/hcp/deps_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

Expand All @@ -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": {
Expand All @@ -28,7 +29,10 @@ func TestSink(t *testing.T) {
},
}, nil)
},
mockCloudCfg: client.MockCloudCfg{},
cloudCfg: config.CloudConfig{
NodeID: types.NodeID("nodeyid"),
NodeName: "nodey",
},
expectedSink: true,
},
"noSinkWhenServerNotRegisteredWithCCM": {
Expand All @@ -40,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": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This no longer applies since we don't initialize the metrics client inside the sink.

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) {
Expand Down Expand Up @@ -93,14 +84,17 @@ 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)
sinkOpts := sink(c, test.mockCloudCfg, l, types.NodeID("server1234"))
ctx := context.Background()

s := sink(ctx, c, mc, test.cloudCfg)
if !test.expectedSink {
require.Nil(t, sinkOpts)
require.Nil(t, s)
return
}
require.NotNil(t, sinkOpts)
require.NotNil(t, s)
})
}
}
Loading