-
Notifications
You must be signed in to change notification settings - Fork 302
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.
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
DAOS-9825 control: Update Telemetry Endpoint to use HTTPS #15216
base: master
Are you sure you want to change the base?
Changes from 4 commits
7cf9875
a3bcfc9
d3f9941
d9860a4
3c3bbc3
3b9be62
82b37d1
f1d463a
1329d1e
49d5062
29db84d
ca672e7
f67aed2
b3cc471
599ec1e
e7833f8
834e532
5b5f7a7
662baa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -57,14 +57,12 @@ type Config struct { | |||||
ExcludeFabricIfaces common.StringSet `yaml:"exclude_fabric_ifaces,omitempty"` | ||||||
FabricInterfaces []*NUMAFabricConfig `yaml:"fabric_ifaces,omitempty"` | ||||||
ProviderIdx uint // TODO SRS-31: Enable with multiprovider functionality | ||||||
TelemetryPort int `yaml:"telemetry_port,omitempty"` | ||||||
TelemetryEnabled bool `yaml:"telemetry_enabled,omitempty"` | ||||||
TelemetryRetain time.Duration `yaml:"telemetry_retain,omitempty"` | ||||||
TelemetryConfig *security.TelemetryConfig `yaml:"telemetry_config"` | ||||||
} | ||||||
|
||||||
// TelemetryExportEnabled returns true if client telemetry export is enabled. | ||||||
func (c *Config) TelemetryExportEnabled() bool { | ||||||
return c.TelemetryPort > 0 | ||||||
return c.TelemetryConfig.Port > 0 | ||||||
} | ||||||
|
||||||
// NUMAFabricConfig defines a list of fabric interfaces that belong to a NUMA | ||||||
|
@@ -99,14 +97,20 @@ func LoadConfig(cfgPath string) (*Config, error) { | |||||
return nil, fmt.Errorf("invalid system name: %s", cfg.SystemName) | ||||||
} | ||||||
|
||||||
if cfg.TelemetryRetain > 0 && cfg.TelemetryPort == 0 { | ||||||
if cfg.TelemetryConfig.Retain > 0 && cfg.TelemetryConfig.Port == 0 { | ||||||
return nil, errors.New("telemetry_retain requires telemetry_port") | ||||||
} | ||||||
|
||||||
if cfg.TelemetryEnabled && cfg.TelemetryPort == 0 { | ||||||
if cfg.TelemetryConfig.Enabled && cfg.TelemetryConfig.Port == 0 { | ||||||
return nil, errors.New("telemetry_enabled requires telemetry_port") | ||||||
} | ||||||
|
||||||
if cfg.TelemetryConfig.AllowInsecure == false { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - slightly more idiomatic way of saying the same
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will update |
||||||
if cfg.TelemetryConfig.ServerCert == "" || cfg.TelemetryConfig.ServerKey == "" { | ||||||
return nil, errors.New("For secure mode, server_cert and server_key required under telemetry_config") | ||||||
} | ||||||
} | ||||||
|
||||||
return cfg, nil | ||||||
} | ||||||
|
||||||
|
@@ -121,5 +125,6 @@ func DefaultConfig() *Config { | |||||
LogLevel: common.DefaultControlLogLevel, | ||||||
TransportConfig: security.DefaultAgentTransportConfig(), | ||||||
CredentialConfig: &security.CredentialConfig{}, | ||||||
TelemetryConfig: security.DefaultClientTelemetryConfig(), | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,62 @@ transport_config: | |
allow_insecure: true | ||
`) | ||
|
||
telemetryRetainWithBadPort := test.CreateTestFile(t, dir, ` | ||
name: shire | ||
access_points: ["one:10001", "two:10001"] | ||
port: 4242 | ||
runtime_dir: /tmp/runtime | ||
log_file: /home/frodo/logfile | ||
control_log_mask: debug | ||
transport_config: | ||
allow_insecure: true | ||
telemetry_config: | ||
retain: 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the config file example, this is shown as "1m". Is that valid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes as it's time.duration but will change it to 1m as better example |
||
port: 0 | ||
`) | ||
|
||
telemetryEnabledWithBadPort := test.CreateTestFile(t, dir, ` | ||
name: shire | ||
access_points: ["one:10001", "two:10001"] | ||
port: 4242 | ||
runtime_dir: /tmp/runtime | ||
log_file: /home/frodo/logfile | ||
control_log_mask: debug | ||
transport_config: | ||
allow_insecure: true | ||
telemetry_config: | ||
enabled: true | ||
port: 0 | ||
`) | ||
|
||
telemetryWithoutServerCert := test.CreateTestFile(t, dir, ` | ||
name: shire | ||
access_points: ["one:10001", "two:10001"] | ||
port: 4242 | ||
runtime_dir: /tmp/runtime | ||
log_file: /home/frodo/logfile | ||
control_log_mask: debug | ||
transport_config: | ||
allow_insecure: true | ||
telemetry_config: | ||
allow_insecure: false | ||
server_cert: "" | ||
`) | ||
|
||
telemetryWithoutServerKey := test.CreateTestFile(t, dir, ` | ||
name: shire | ||
access_points: ["one:10001", "two:10001"] | ||
port: 4242 | ||
runtime_dir: /tmp/runtime | ||
log_file: /home/frodo/logfile | ||
control_log_mask: debug | ||
transport_config: | ||
allow_insecure: true | ||
telemetry_config: | ||
allow_insecure: false | ||
server_key: "" | ||
`) | ||
|
||
for name, tc := range map[string]struct { | ||
path string | ||
expResult *Config | ||
|
@@ -108,6 +164,22 @@ transport_config: | |
path: emptyFile, | ||
expResult: DefaultConfig(), | ||
}, | ||
"telemetry retain with no port": { | ||
path: telemetryRetainWithBadPort, | ||
expErr: errors.New("telemetry_retain requires telemetry_port"), | ||
}, | ||
"telemetry enabled with no port": { | ||
path: telemetryEnabledWithBadPort, | ||
expErr: errors.New("telemetry_enabled requires telemetry_port"), | ||
}, | ||
"telemetry with secure mode with no server certificate": { | ||
path: telemetryWithoutServerCert, | ||
expErr: errors.New("For secure mode, server_cert and server_key required under telemetry_config"), | ||
}, | ||
"telemetry with secure mode with no server key": { | ||
path: telemetryWithoutServerKey, | ||
expErr: errors.New("For secure mode, server_cert and server_key required under telemetry_config"), | ||
}, | ||
"without optional items": { | ||
path: withoutOptCfg, | ||
expResult: &Config{ | ||
|
@@ -122,6 +194,7 @@ transport_config: | |
AllowInsecure: true, | ||
CertificateConfig: DefaultConfig().TransportConfig.CertificateConfig, | ||
}, | ||
TelemetryConfig: security.DefaultClientTelemetryConfig(), | ||
}, | ||
}, | ||
"bad log mask": { | ||
|
@@ -154,6 +227,7 @@ transport_config: | |
AllowInsecure: true, | ||
CertificateConfig: DefaultConfig().TransportConfig.CertificateConfig, | ||
}, | ||
TelemetryConfig: security.DefaultClientTelemetryConfig(), | ||
ExcludeFabricIfaces: common.NewStringSet("ib3"), | ||
FabricInterfaces: []*NUMAFabricConfig{ | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we'd still want to support the old telemetry config format as well, for at least one version, so we don't force people to change config files without warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's just that those variables moved under the TelemetryConfig struct so from user point of view nothing has changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An old config file using
telemetry_port
no longer works, though. A previously-working config file will just stop working, and they will need to change it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, let me change the name to support that old config too.