Add support for custom proxy ca

[coleenquadros]

https://issues.redhat.com/browse/RHOBS-950

This PR contains -

• Update to the https://github.com/open-cluster-management-io/api library to support custom CA in addonDeploymentconfig - 🐛 add ca bundle to addon proxy settings open-cluster-management-io/api#293
• extract the custom CA bundle from config and pass to metrics collector correctly and add it to secret observability-managed-cluster-certs and pick up the env variable containing cert when setting up MTLS transport and add it to the cert pool.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coleenquadros]

/test test-unit

[douglascamata]

I don't understand why we need isCustomCA if we already have a ToUploadCA field that is being passed to the MTLSTransport function.

Particularly what confuses me is that the logic is almost the same, only differing that if using a custom CA for proxy purposes we're getting it from an environment variable instead of file.

Could we standardise on one of the approaches to simplify the code and configuration? In both cases, a custom CA cert is passed.

I would say we can always read it from a file, as it is how it already works.

On a side note: SonarCloud complains that only about 50% of new code is being tested and the quality gate wants at least 70% of test coverage. Can you have a look at this, please?

[coleenquadros]

@douglascamata From the diff in the PR in the open-cluster-management/api here the CABundle is provided as []byte and not file path/name -
CABundle []byte json:"caBundle,omitempty"``

And metricsclient in MCO reads from the file thats why I added the flag to check for CustomCA->

multicluster-observability-operator/collectors/metrics/pkg/metricsclient/metricsclient.go

Line 376 in d56f8c4

caCert, err := os.ReadFile(filepath.Clean(caCertFile))

[douglascamata]

@douglascamata From the diff in the PR in the open-cluster-management/api here the CABundle is provided as []byte and not file path/name - CABundle []byte json:"caBundle,omitempty"``

And metricsclient in MCO reads from the file thats why I added the flag to check for CustomCA->

multicluster-observability-operator/collectors/metrics/pkg/metricsclient/metricsclient.go

Line 376 in d56f8c4

caCert, err := os.ReadFile(filepath.Clean(caCertFile))

Yep, I understood this. I believe this adds extra complications, because both are custom CAs: the custom CA is needed to properly verify a server's certificate.

The only difference between the two scenarios is that:

• In one, the custom CA will verify the proxy server's certificate.
• In the other one, the custom CA will verify the Observatorium API server's certificate.

So I believe we should reuse the same mechanism for these two scenarios.

Also, the filesystem approach has a big advantage: it's a file, which can be mounted from a secret/configmap. This means it can be managed by an operator and automatically reloaded without a restart of the collector if it ever changes. In fact, for the no-proxy scenario we already managed the certificate ourselves. For the proxy scenario, an operator like certmanager could be used.

[douglascamata]

Me and @coleenquadros had a chat and agreed that the idea I proposed seems difficult to introduce in the current codebase, even though it might seem like the right way to do it.

So we will follow with the approach that she started and only change the logic inside the metrics collectors to always prefer the CA bundle in the HTTPS_PROXY_CA env var if it's present. This should remove the need for the is-custom-ca parameter, together the code that sets it and overwrites the to-upload-ca parameter with the contents of the env var.

[douglascamata]

It looks good to me. But let's allow some time for QA to confirm this.

Ideally I would love the proxy scenario to be part of the tests in the repository itself to ensure a tight feedback loop while working on the proxy features, but I understand that it's not easy to replicate the scenario.

[coleenquadros]

/retest-required

[sonarqubecloud]

Quality Gate failed

Failed conditions

66.7% Coverage on New Code (required ≥ 70%)
B Security Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[openshift-ci]

@coleenquadros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sonarcloud	7b36a30	link	true	/test sonarcloud

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[coleenquadros]

metrics pushed successfully from managed cluster with custom-ca bundle on QE test set up -

logs

level=info caller=logger.go:50 ts=2024-01-22T15:54:03.883220811Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T15:59:04.71919974Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T15:59:04.898374548Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:04:05.810905694Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:04:05.978414178Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:09:06.710112375Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:09:06.90016687Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:14:07.692960945Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:14:07.866166876Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:19:08.630162279Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:19:08.787772885Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:24:09.599454553Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:24:09.771992413Z component=forwarder component=metricsclient msg="metrics pushed successfully"
level=debug caller=logger.go:45 ts=2024-01-22T16:29:10.573424971Z component=forwarder component=metricsclient timeseriesnumber=10093
level=info caller=logger.go:50 ts=2024-01-22T16:29:10.743076057Z component=forwarder component=metricsclient msg="metrics pushed successfull

[coleenquadros]

@subbarao-meduri @douglascamata

[douglascamata]

/lgtm

[subbarao-meduri]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coleenquadros, douglascamata, subbarao-meduri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [douglascamata,subbarao-meduri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment