Code updated based on review comments.

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>
daos-stack · Oct 7, 2024 · 3b9be62 · 3b9be62
1 parent 3c3bbc3
commit 3b9be62
Show file tree

Hide file tree

Showing 33 changed files with 201 additions and 181 deletions.
diff --git a/docs/admin/deployment.md b/docs/admin/deployment.md
@@ -831,16 +831,16 @@ subject=CN = wolf-170
 Getting CA Private Key
 Required Server Certificate Files:
     .//daosTelemetryCA.crt
-    .//telemetryserver.key
-    .//telemetryserver.crt
+    .//telemetry.key
+    .//telemetry.crt
 $ ls -l
 total 20
 -rw-r--r-- 1 root       daos_daemons 1460 Sep 27 17:18 daosTelemetryCA.crt
 -rw-r--r-- 1 root       root           41 Sep 27 17:19 daosTelemetryCA.srl
 -rw-r--r-- 1 root       root            0 Sep 27 17:18 index.txt
 -rw-r--r-- 1 root       root            3 Sep 27 17:18 serial.txt
--rw-r--r-- 1 daos_agent daos_agent   1302 Sep 27 17:19 telemetryserver.crt
--r-------- 1 daos_agent daos_agent   1675 Sep 27 17:19 telemetryserver.key
+-rw-r--r-- 1 daos_agent daos_agent   1302 Sep 27 17:19 telemetry.crt
+-r-------- 1 daos_agent daos_agent   1675 Sep 27 17:19 telemetry.key
 ```
 
 Below example is ran with daos_server user on server node
@@ -858,16 +858,16 @@ subject=CN = wolf-173
 Getting CA Private Key
 Required Server Certificate Files:
     .//daosTelemetryCA.crt
-    .//telemetryserver.key
-    .//telemetryserver.crt
+    .//telemetry.key
+    .//telemetry.crt
 $ ls -l
 total 20
 -rw-r--r-- 1 root        daos_daemons 1460 Sep 27 17:24 daosTelemetryCA.crt
 -rw-r--r-- 1 root        root           41 Sep 27 17:24 daosTelemetryCA.srl
 -rw-r--r-- 1 root        root            0 Sep 27 17:24 index.txt
 -rw-r--r-- 1 root        root            3 Sep 27 17:24 serial.txt
--rw-r--r-- 1 daos_server daos_server  1302 Sep 27 17:24 telemetryserver.crt
--r-------- 1 daos_server daos_server  1679 Sep 27 17:24 telemetryserver.key
+-rw-r--r-- 1 daos_server daos_server  1302 Sep 27 17:24 telemetry.crt
+-r-------- 1 daos_server daos_server  1679 Sep 27 17:24 telemetry.key
 ```
 
 You can copy this certificates on /etc/daos/certs/ or someother secure location
@@ -884,9 +884,9 @@ telemetry_config:
   # Set the server telemetry endpoint port number
   port: 9191
   # Server certificate for use in TLS handshakes
-  server_cert: /etc/daos/certs/telemetryserver.crt
+  https_cert: /etc/daos/certs/telemetry.crt
   # Key portion of Server Certificate
-  server_key: /etc/daos/certs/telemetryserver.key
+  https_key: /etc/daos/certs/telemetry.key
 ```
 
 ```yaml
@@ -901,9 +901,9 @@ telemetry_config:
   # Retain client telemetry for a period of time after the client process exits.
   retain: 30s
   # Server certificate for use in TLS handshakes
-  server_cert: /etc/daos/certs/telemetryserver.crt
+  https_cert: /etc/daos/certs/telemetry.crt
   # Key portion of Server Certificate
-  server_key: /etc/daos/certs/telemetryserver.key
+  https_key: /etc/daos/certs/telemetry.key
 ```
 
 ```yaml

diff --git a/src/control/cmd/daos_agent/config.go b/src/control/cmd/daos_agent/config.go
@@ -58,6 +58,10 @@ type Config struct {
 	FabricInterfaces    []*NUMAFabricConfig        `yaml:"fabric_ifaces,omitempty"`
 	ProviderIdx         uint                       // TODO SRS-31: Enable with multiprovider functionality
 	TelemetryConfig     *security.TelemetryConfig  `yaml:"telemetry_config"`
+	// Support Old config options.
+	TelemetryPort    int           `yaml:"telemetry_port,omitempty"`
+	TelemetryEnabled bool          `yaml:"telemetry_enabled,omitempty"`
+	TelemetryRetain  time.Duration `yaml:"telemetry_retain,omitempty"`
 }
 
 // TelemetryExportEnabled returns true if client telemetry export is enabled.
@@ -97,6 +101,19 @@ func LoadConfig(cfgPath string) (*Config, error) {
 		return nil, fmt.Errorf("invalid system name: %s", cfg.SystemName)
 	}
 
+	// Support Old config options and copy it to the underline new structure value.
+	if cfg.TelemetryRetain > 0 {
+		cfg.TelemetryConfig.Retain = cfg.TelemetryRetain
+	}
+
+	if cfg.TelemetryPort != 0 {
+		cfg.TelemetryConfig.Port = cfg.TelemetryPort
+	}
+
+	if cfg.TelemetryEnabled {
+		cfg.TelemetryConfig.Enabled = cfg.TelemetryEnabled
+	}
+
 	if cfg.TelemetryConfig.Retain > 0 && cfg.TelemetryConfig.Port == 0 {
 		return nil, errors.New("telemetry_retain requires telemetry_port")
 	}
@@ -105,9 +122,9 @@ func LoadConfig(cfgPath string) (*Config, error) {
 		return nil, errors.New("telemetry_enabled requires telemetry_port")
 	}
 
-	if cfg.TelemetryConfig.AllowInsecure == false {
-		if cfg.TelemetryConfig.ServerCert == "" || cfg.TelemetryConfig.ServerKey == "" {
-			return nil, errors.New("For secure mode, server_cert and server_key required under telemetry_config")
+	if !cfg.TelemetryConfig.AllowInsecure {
+		if cfg.TelemetryConfig.HttpsCert == "" || cfg.TelemetryConfig.HttpsKey == "" {
+			return nil, errors.New("For secure mode, https_cert and https_key required under telemetry_config")
 		}
 	}
 

diff --git a/src/control/cmd/daos_agent/config_test.go b/src/control/cmd/daos_agent/config_test.go
@@ -98,8 +98,8 @@ control_log_mask: debug
 transport_config:
   allow_insecure: true
 telemetry_config:
-  retain: 1
-  port: 0
+  telemetry_retain: 1m
+  telemetry_port: 0
 `)
 
 	telemetryEnabledWithBadPort := test.CreateTestFile(t, dir, `
@@ -112,11 +112,11 @@ control_log_mask: debug
 transport_config:
   allow_insecure: true
 telemetry_config:
-  enabled: true
-  port: 0
+  telemetry_enabled: true
+  telemetry_port: 0
 `)
 
-	telemetryWithoutServerCert := test.CreateTestFile(t, dir, `
+	telemetryWithoutHttpsCert := test.CreateTestFile(t, dir, `
 name: shire
 access_points: ["one:10001", "two:10001"]
 port: 4242
@@ -127,10 +127,10 @@ transport_config:
   allow_insecure: true
 telemetry_config:
   allow_insecure: false
-  server_cert: ""
+  https_cert: ""
 `)
 
-	telemetryWithoutServerKey := test.CreateTestFile(t, dir, `
+	telemetryWithoutHttpsKey := test.CreateTestFile(t, dir, `
 name: shire
 access_points: ["one:10001", "two:10001"]
 port: 4242
@@ -141,7 +141,7 @@ transport_config:
   allow_insecure: true
 telemetry_config:
   allow_insecure: false
-  server_key: ""
+  https_key: ""
 `)
 
 	for name, tc := range map[string]struct {
@@ -173,12 +173,12 @@ telemetry_config:
 			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"),
+			path:   telemetryWithoutHttpsCert,
+			expErr: errors.New("For secure mode, https_cert and https_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"),
+			path:   telemetryWithoutHttpsKey,
+			expErr: errors.New("For secure mode, https_cert and https_key required under telemetry_config"),
 		},
 		"without optional items": {
 			path: withoutOptCfg,

diff --git a/src/control/cmd/daos_agent/telemetry.go b/src/control/cmd/daos_agent/telemetry.go
@@ -20,8 +20,8 @@ func startPrometheusExporter(ctx context.Context, log logging.Logger, cs *promex
 		Port:          cfg.TelemetryConfig.Port,
 		Title:         "DAOS Client Telemetry",
 		AllowInsecure: cfg.TelemetryConfig.AllowInsecure,
-		HttpsCert:     cfg.TelemetryConfig.ServerCert,
-		HttpsKey:      cfg.TelemetryConfig.ServerKey,
+		HttpsCert:     cfg.TelemetryConfig.HttpsCert,
+		HttpsKey:      cfg.TelemetryConfig.HttpsKey,
 		Register: func(ctx context.Context, log logging.Logger) error {
 			c, err := promexp.NewClientCollector(ctx, log, cs, &promexp.CollectorOpts{
 				RetainDuration: cfg.TelemetryConfig.Retain,

diff --git a/src/control/cmd/dmg/auto_test.go b/src/control/cmd/dmg/auto_test.go
@@ -593,9 +593,9 @@ disable_hugepages: false
 control_log_mask: INFO
 control_log_file: /tmp/daos_server.log
 telemetry_config:
-  allow_insecure: false
-  server_cert: /etc/daos/certs/telemetryserver.crt
-  server_key: /etc/daos/certs/telemetryserver.key
+  allow_insecure: true
+  https_cert: /etc/daos/certs/telemetry.crt
+  https_key: /etc/daos/certs/telemetry.key
   ca_cert: /etc/daos/certs/daosTelemetryCA.crt
 core_dump_filter: 19
 name: daos_server

diff --git a/src/control/lib/control/http.go b/src/control/lib/control/http.go
@@ -39,16 +39,16 @@ type httpGetter interface {
 	retryer
 	getURL() *url.URL
 	getBody(context.Context) ([]byte, error)
-	getAllowInsecure() *bool
+	getAllowInsecure() bool
 	getCaCertPath() *string
 }
 
 type httpReq struct {
 	url           *url.URL
 	getFn         httpGetFn
-	allowInsecure *bool
+	allowInsecure bool
 	cacertpath    *string
-	getBodyFn     func(context.Context, *url.URL, httpGetFn, time.Duration, *bool, *string) ([]byte, error)
+	getBodyFn     func(context.Context, *url.URL, httpGetFn, time.Duration, bool, *string) ([]byte, error)
 }
 
 func (r *httpReq) canRetry(err error, cur uint) bool {
@@ -83,7 +83,7 @@ func (r *httpReq) getURL() *url.URL {
 	return r.url
 }
 
-func (r *httpReq) getAllowInsecure() *bool {
+func (r *httpReq) getAllowInsecure() bool {
 	return r.allowInsecure
 }
 
@@ -152,7 +152,7 @@ func httpsGetFunc(cert []byte) (httpGetFn, error) {
 
 // httpGetBody executes a simple HTTP GET request to a given URL and returns the
 // content of the response body.
-func httpGetBody(ctx context.Context, url *url.URL, get httpGetFn, timeout time.Duration, allowInsecure *bool, cacertpath *string) ([]byte, error) {
+func httpGetBody(ctx context.Context, url *url.URL, get httpGetFn, timeout time.Duration, allowInsecure bool, cacertpath *string) ([]byte, error) {
 	if url == nil {
 		return nil, errors.New("nil URL")
 	}
@@ -165,7 +165,7 @@ func httpGetBody(ctx context.Context, url *url.URL, get httpGetFn, timeout time.
 		return nil, errors.New("nil get function")
 	}
 
-	if *allowInsecure == false {
+	if !allowInsecure {
 		if cacertpath == nil {
 			return nil, errors.New("Provide the CA certificate path")
 		}

diff --git a/src/control/lib/control/http_test.go b/src/control/lib/control/http_test.go
@@ -133,7 +133,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		timeout       time.Duration
 		cancelCtx     bool
 		getFn         httpGetFn
-		allowInsecure *bool
+		allowInsecure bool
 		caCertPath    *string
 		expResult     []byte
 		expErr        error
@@ -143,25 +143,25 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"empty URL": {
 			url:           &url.URL{},
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			expErr:        errors.New("host address is required"),
 		},
 		"nil getFn": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			expErr:        errors.New("nil get function"),
 		},
 		"getFn error": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return nil, errors.New("mock getFn")
 			},
 			expErr: errors.New("mock getFn"),
 		},
 		"http.Response error": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: http.StatusNotFound,
@@ -172,7 +172,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"empty body": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: http.StatusOK,
@@ -183,7 +183,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"success with body": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: http.StatusOK,
@@ -194,7 +194,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"failure with body in secure mode without CA certificate path": {
 			url:           defaultURL,
-			allowInsecure: &falseAllowInsecure,
+			allowInsecure: falseAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: http.StatusOK,
@@ -205,7 +205,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"failure with body in secure mode with bad CA certificate": {
 			url:           defaultURL,
-			allowInsecure: &falseAllowInsecure,
+			allowInsecure: falseAllowInsecure,
 			caCertPath:    &badCertPerm,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
@@ -217,7 +217,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"failure with body in secure mode with bad CA certificate path": {
 			url:           defaultURL,
-			allowInsecure: &falseAllowInsecure,
+			allowInsecure: falseAllowInsecure,
 			caCertPath:    &badCertPath,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
@@ -229,7 +229,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"reading body fails": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			getFn: func(_ string) (*http.Response, error) {
 				return &http.Response{
 					StatusCode: http.StatusOK,
@@ -240,7 +240,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"request times out": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			timeout:       5 * time.Millisecond,
 			getFn: func(_ string) (*http.Response, error) {
 				time.Sleep(1 * time.Second)
@@ -253,7 +253,7 @@ func TestControl_httpGetBody(t *testing.T) {
 		},
 		"request canceled": {
 			url:           defaultURL,
-			allowInsecure: &defaultAllowInsecure,
+			allowInsecure: defaultAllowInsecure,
 			cancelCtx:     true,
 			getFn: func(_ string) (*http.Response, error) {
 				time.Sleep(1 * time.Second)
@@ -324,9 +324,8 @@ func (r *mockHTTPGetter) getURL() *url.URL {
 	}
 }
 
-func (r *mockHTTPGetter) getAllowInsecure() *bool {
-	allowInsecure := true
-	return &allowInsecure
+func (r *mockHTTPGetter) getAllowInsecure() bool {
+	return true
 }
 
 func (r *mockHTTPGetter) getCaCertPath() *string {

diff --git a/src/control/lib/control/telemetry.go b/src/control/lib/control/telemetry.go
@@ -114,7 +114,7 @@ func MetricsList(ctx context.Context, req *MetricsListReq) (*MetricsListResp, er
 	}
 
 	req.url = getMetricsURL(req.Host, req.Port, req.AllowInsecure)
-	req.allowInsecure = &req.AllowInsecure
+	req.allowInsecure = req.AllowInsecure
 	req.cacertpath = &req.CaCertPath
 
 	scraped, err := scrapeMetrics(ctx, req)
@@ -176,7 +176,7 @@ func MetricsQuery(ctx context.Context, req *MetricsQueryReq) (*MetricsQueryResp,
 	}
 
 	req.url = getMetricsURL(req.Host, req.Port, req.AllowInsecure)
-	req.allowInsecure = &req.AllowInsecure
+	req.allowInsecure = req.AllowInsecure
 	req.cacertpath = &req.CaCertPath
 
 	scraped, err := scrapeMetrics(ctx, req)