DAOS-9825 control: Update Telemetry Endpoint to use HTTPS #15216

ravalsam · 2024-09-27T20:36:58Z

Adding new option for telemetry config in server, control and agent yaml file.
Telemetry endpoint can have option to run in both secure (https) and insecure (http) mode.
telemetry_config: allow_insecure: false server_cert: /etc/daos/certs/telemetryserver.crt server_key: /etc/daos/certs/telemetryserver.key ca_cert: /etc/daos/certs/daosTelemetryCA.crt
Telemetry old configuration option is supported bur recommend to use the new options.
Updated dmg to create the correct prometheus config install based on telemetry is insecure mode or not.
# cat /root/.prometheus.yml scheme: https tls_config: ca_file: /etc/daos/certs/daosTelemetryCA.crt

Features: control telemetry

Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

- Adding new option for telemetry config in server, control and agent yaml file. - Telemetry endpoint can have option to run in both secure (https) and insecure (http) mode. telemetry_config: allow_insecure: false server_cert: /etc/daos/certs/telemetryserver.crt server_key: /etc/daos/certs/telemetryserver.key ca_cert: /etc/daos/certs/daosTelemetryCA.crt - Telemetry old configuration option is supported bur recommend to use the new options. - Updated dmg to create the correct prometheus config install based on telemetry is insecure mode or not. # cat /root/.prometheus.yml scheme: https tls_config: ca_file: /etc/daos/certs/daosTelemetryCA.crt Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

github-actions · 2024-09-27T20:37:19Z

Ticket title is 'Update Telemetry Endpoint to use HTTPS'
Status is 'In Review'
Labels: 'Checkmarx,scrubbed_2.6,scrubbed_2.8'
https://daosio.atlassian.net/browse/DAOS-9825

daosbuild1 · 2024-09-27T22:01:50Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/1/execution/node/1169/log

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-10-01T02:53:31Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/5/execution/node/1507/log

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-10-01T11:36:46Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/6/execution/node/1438/log

daltonbohning

This probably needs to run with one or both of these

Features: telemetry control

daltonbohning · 2024-10-01T14:35:15Z

src/tests/ftest/util/agent_utils.py

+        self.manager.job.copy_telemetry_certificates(
+            get_log_file("daosTelemetryCA"), self._hosts)
+        self.manager.job.generate_telemetry_certificates(self._hosts, "daos_agent")


Maybe I'm missing something but how can we copy them before generating?

So the certificate will be created on two stage first to create the private CA on Admin node and copy and create the individual certificate on server/client.
I see your point on naming confusion, let me change the function name to avoid confusion.

daltonbohning · 2024-10-01T14:37:21Z

src/tests/ftest/util/agent_utils_params.py

+    def get_certificate_data(self, name_list):
+        """Get certificate data.
+
+        Args:
+            name_list (list): list of certificate attribute names.
+
+        Returns:
+            data (dict): a dictionary of parameter directory name keys and
+                value.
+
+        """
+        data = super().get_certificate_data(name_list)
+        return data
+


This function just returns the super() so it's not necessary

Suggested change

def get_certificate_data(self, name_list):

"""Get certificate data.

Args:

name_list (list): list of certificate attribute names.

Returns:

data (dict): a dictionary of parameter directory name keys and

value.

"""

data = super().get_certificate_data(name_list)

return data

Will remove it.

daltonbohning · 2024-10-01T14:41:27Z

src/tests/ftest/util/command_utils.py

+            result = run_pcmd(hosts, command, 30)
+            if result[0]['exit_status'] != 0:
+                self.log.info("    WARNING: command %s failed", command)


We should use run_remote instead of run_pcmd

Suggested change

result = run_pcmd(hosts, command, 30)

if result[0]['exit_status'] != 0:

self.log.info(" WARNING: command %s failed", command)

result = run_remote(self.log, hosts, command, 30)

if not result.passed:

self.log.info(" WARNING: command %s failed", command)

Will update it.

daltonbohning · 2024-10-01T14:42:15Z

src/tests/ftest/util/command_utils_base.py

+    """Telemetry credentials listing certificates for secure communication."""
+
+    def __init__(self, namespace, title, log_dir):
+        """Initialize a TelemetryConfig object.


Suggested change

"""Initialize a TelemetryConfig object.

"""Initialize a TelemetryCredentials object.

Will update

Telemetry has more than credential so TelemetryConfig is make sense from that context.

This comment just needs to match whatever the class name is

daltonbohning · 2024-10-01T14:43:05Z

src/tests/ftest/util/command_utils_base.py

+            title (str, optional): namespace under which to place the
+                parameters when creating the yaml file. Defaults to None.


Suggested change

title (str, optional): namespace under which to place the

parameters when creating the yaml file. Defaults to None.

title (str): namespace under which to place the

parameters when creating the yaml file.

daltonbohning · 2024-10-01T14:46:58Z

src/tests/ftest/util/server_utils_params.py

+    """Telemetry credentials listing certificates for secure communication."""
+
+    def __init__(self, log_dir=os.path.join(os.sep, "tmp")):
+        """Initialize a TelemetryConfig object."""


Suggested change

"""Initialize a TelemetryConfig object."""

"""Initialize a DaosServerTelemetryCredentials object."""

daltonbohning · 2024-10-01T14:47:14Z

src/tests/ftest/util/server_utils_params.py

+    def get_certificate_data(self, name_list):
+        """Get certificate data.
+
+        Args:
+            name_list (list): list of certificate attribute names.
+
+        Returns:
+            data (dict): a dictionary of parameter directory name keys and
+                value.
+
+        """
+        data = super().get_certificate_data(name_list)
+        return data
+


Suggested change

def get_certificate_data(self, name_list):

"""Get certificate data.

Args:

name_list (list): list of certificate attribute names.

Returns:

data (dict): a dictionary of parameter directory name keys and

value.

"""

data = super().get_certificate_data(name_list)

return data

Will remove it.

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

kjacque · 2024-10-01T15:42:17Z

src/control/cmd/daos_agent/config.go

-	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"`


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.

Right, it's just that those variables moved under the TelemetryConfig struct so from user point of view nothing has changed.

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.

I see your point, let me change the name to support that old config too.

kjacque · 2024-10-01T15:43:11Z

src/control/cmd/daos_agent/config.go

 		return nil, errors.New("telemetry_enabled requires telemetry_port")
 	}

+	if cfg.TelemetryConfig.AllowInsecure == false {


nit - slightly more idiomatic way of saying the same

Suggested change

if cfg.TelemetryConfig.AllowInsecure == false {

if !cfg.TelemetryConfig.AllowInsecure {

Will update

kjacque · 2024-10-01T15:51:29Z

utils/config/daos_agent.yml

+#  # Server certificate for use in TLS handshakes
+#  # DAOS client is the HTTPS server to open secure telemetry endpoint.
+#  server_cert: /etc/daos/certs/telemetryserver.crt


The name for these is a little confusing.

Suggested change

# # Server certificate for use in TLS handshakes

# # DAOS client is the HTTPS server to open secure telemetry endpoint.

# server_cert: /etc/daos/certs/telemetryserver.crt

# # HTTPS certificate for telemetry endpoint

# https_cert: /etc/daos/certs/telemetryserver.crt

Will Update

kjacque · 2024-10-01T15:52:39Z

utils/config/daos_agent.yml

+#  # Key portion of Server Certificate
+#  # DAOS client is the HTTPS server to open secure telemetry endpoint.
+#  server_key: /etc/daos/certs/telemetryserver.key


Suggested change

# # Key portion of Server Certificate

# # DAOS client is the HTTPS server to open secure telemetry endpoint.

# server_key: /etc/daos/certs/telemetryserver.key

# # Private key for HTTPS certificate for telemetry endpoint

# https_key: /etc/daos/certs/telemetryserver.key

Will update

kjacque · 2024-10-01T19:51:04Z

utils/config/daos_agent.yml

+#  # In order to disable transport security, uncomment and set allow_insecure
+#  # to true. Not recommended for production configurations.
+#  allow_insecure: false


I still feel unsure about having this option be explicit, rather than implied by the cert+key not being defined. I think this endpoint is fundamentally different from the transport security we use for DAOS overall. The telemetry endpoint is not authenticated via certs (or at all) the way that the component communications are. The HTTPS encryption is to guarantee the data read by prometheus (or any other tool) hasn't been tampered with.

I think this will also changes once we use the tool suggested by you. So let me try and see how that works and will change the code accordingly.

Code was modified to use the system certificate. But to be aligned with Transport config option I think it's better we can keep it. This will also gives user a chance to use specific security level with certificates.

kjacque · 2024-10-03T00:35:54Z

src/control/lib/control/telemetry.go

+	if req.AllowInsecure == false && req.CaCertPath == "" {
+		return nil, errors.New("Provide the CA certificate path")
+	}


This isn't right. The dmg config shouldn't need the user to supply a root cert if it is from a trusted CA that the OS recognizes.

I will have to check this and change the code.

kjacque · 2024-10-03T00:40:38Z

src/control/lib/control/telemetry.go

+		AllowInsecure bool   // Set the https end point secure
+		CaCertPath    string // CA Cert path for telemetry


I don't think we should need these from the dmg side.

I will have to check this and change the code.

kjacque · 2024-10-03T00:44:47Z

src/control/lib/control/http.go

+	url           *url.URL
+	getFn         httpGetFn
+	allowInsecure *bool
+	cacertpath    *string


As mentioned before, we shouldn't need to include a path to a root cert.

I will have to check this and change the code.

kjacque · 2024-10-03T00:46:06Z

src/control/lib/control/http.go

@@ -128,6 +165,22 @@ func httpGetBody(ctx context.Context, url *url.URL, get httpGetFn, timeout time.
 		return nil, errors.New("nil get function")
 	}

+	if *allowInsecure == false {


Why is it a pointer? If it's nil, dereferencing will segfault, as in C.

Changed to bool.

kjacque · 2024-10-03T00:46:52Z

src/control/lib/control/http.go

+// and return the http.Get
+func httpsGetFunc(cert []byte) (httpGetFn, error) {
+	caCertPool := x509.NewCertPool()
+	result := caCertPool.AppendCertsFromPEM(cert)


We should be able to harvest what's in the OS list of trusted certs, similar to what web browsers do. I think x509.SystemCertPool does this.

I will have to check this and change the code.

daltonbohning

ftest LGTM but will defer to Kris on the config/interface since ftest just mirrors that

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-10-08T17:22:03Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/9/execution/node/1520/log

daosbuild1 · 2024-10-08T21:42:42Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/9/execution/node/1566/log

tanabarr

Other than change requests from kjacque this LGTM. So once those concerns have been addressed I'm happy to approve.

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-11-19T20:01:23Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15216/10/testReport/

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-12-04T08:22:28Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/12/execution/node/1210/log

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-12-05T09:16:15Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-15216/16/execution/node/1211/log

daosbuild1 · 2024-12-06T02:03:47Z

Test stage Test RPMs on EL 8.6 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-15216/18/display/redirect

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-12-14T11:16:16Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15216/24/testReport/

This reverts commit f67aed2. Signed-off-by: Samir Raval <[email protected]>

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

tanabarr · 2024-12-18T10:18:57Z

need to resolve conflicts

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daosbuild1 · 2024-12-19T22:45:20Z

Test stage Functional on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-15216/28/testReport/

tanabarr

LGTM

ravalsam requested review from a team as code owners September 27, 2024 20:36

Fixed Test code.

a3bcfc9

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam force-pushed the samirrav/DAOS-9825-Final branch from fa93839 to 91ddce3 Compare September 30, 2024 17:53

Spell correction and script fix.

d3f9941

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam force-pushed the samirrav/DAOS-9825-Final branch from 91ddce3 to d3f9941 Compare September 30, 2024 17:56

ravalsam changed the title ~~DAOS-9825 control:Update Telemetry Endpoint to use HTTPS~~ DAOS-9825 control: Update Telemetry Endpoint to use HTTPS Sep 30, 2024

Few minor fix and Test case fix.

d9860a4

Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

daltonbohning reviewed Oct 1, 2024

View reviewed changes

Code modified based on review comments.

3c3bbc3

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam requested review from daltonbohning, tanabarr, kjacque and mjmac October 2, 2024 14:48

kjacque requested changes Oct 3, 2024

View reviewed changes

daltonbohning reviewed Oct 3, 2024

View reviewed changes

Code updated based on review comments.

3b9be62

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam force-pushed the samirrav/DAOS-9825-Final branch from 5ab0cf4 to 3b9be62 Compare October 7, 2024 18:06

tanabarr reviewed Nov 7, 2024

View reviewed changes

Code updated based on review comments.

82b37d1

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam added 2 commits November 19, 2024 22:54

Merge branch 'master' into samirrav/DAOS-9825-Final

f1d463a

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Code updated based on review comments.

1329d1e

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam added 2 commits December 4, 2024 16:39

Merge branch 'master' into samirrav/DAOS-9825-Final

49d5062

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/DAOS-9825-Final

29db84d

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam added 3 commits December 11, 2024 01:31

Merge branch 'master' into samirrav/DAOS-9825-Final

ca672e7

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Code updated based on review comments.

f67aed2

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/DAOS-9825-Final

b3cc471

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam added 3 commits December 16, 2024 18:07

Revert "Code updated based on review comments."

599ec1e

This reverts commit f67aed2. Signed-off-by: Samir Raval <[email protected]>

Code modified based on review comments

e7833f8

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/DAOS-9825-Final

834e532

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

ravalsam force-pushed the samirrav/DAOS-9825-Final branch from 3e5f1e0 to 834e532 Compare December 16, 2024 18:09

ravalsam requested review from kjacque, daltonbohning and tanabarr December 17, 2024 16:25

ravalsam added 2 commits December 18, 2024 16:00

Merge branch 'master' into samirrav/DAOS-9825-Final

5b5f7a7

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

Merge branch 'master' into samirrav/DAOS-9825-Final

662baa9

Features: control telemetry Required-githooks: true Signed-off-by: Samir Raval <[email protected]>

tanabarr approved these changes Dec 20, 2024

View reviewed changes

	"""Initialize a TelemetryConfig object.
	"""Initialize a TelemetryCredentials object.

		title (str, optional): namespace under which to place the
		parameters when creating the yaml file. Defaults to None.

	"""Initialize a TelemetryConfig object."""
	"""Initialize a DaosServerTelemetryCredentials object."""

	if cfg.TelemetryConfig.AllowInsecure == false {
	if !cfg.TelemetryConfig.AllowInsecure {

		AllowInsecure bool // Set the https end point secure
		CaCertPath string // CA Cert path for telemetry

DAOS-9825 control: Update Telemetry Endpoint to use HTTPS #15216

Are you sure you want to change the base?

DAOS-9825 control: Update Telemetry Endpoint to use HTTPS #15216

Conversation

ravalsam commented Sep 27, 2024

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Sep 27, 2024 • edited Loading

daosbuild1 commented Sep 27, 2024

daosbuild1 commented Oct 1, 2024

daosbuild1 commented Oct 1, 2024

daltonbohning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjacque Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daltonbohning left a comment

Choose a reason for hiding this comment

daosbuild1 commented Oct 8, 2024

daosbuild1 commented Oct 8, 2024

tanabarr left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 19, 2024

daosbuild1 commented Dec 4, 2024

daosbuild1 commented Dec 5, 2024

daosbuild1 commented Dec 6, 2024

daosbuild1 commented Dec 14, 2024

tanabarr commented Dec 18, 2024

daosbuild1 commented Dec 19, 2024

tanabarr left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 27, 2024 •

edited

Loading

kjacque Oct 1, 2024 •

edited

Loading