Skip to content

CA Paths Fixes #1570

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

Closed
wants to merge 8 commits into from
Closed

CA Paths Fixes #1570

wants to merge 8 commits into from

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Mar 24, 2025

Explain the changes

  1. In pkg/utilutil.go - Change the /etc/ocp-injected-ca-bundle.crt to /etc/ocp-injected-ca-bundle/ca-bundle.crt.
  2. In pkg/system/reconciler.go - Rename r.CaBundleConf.Name from r.Request.Name + "-ca-inject" to "ocp-injected-ca-bundle" to align with the rename of configmap-ca-inject.yaml from noobaa-ca-inject to ocp-injected-ca-bundle in PR Modify the handling of injected OCP CA bundles #1328.
  3. In pkg/system/phase2_creating.go and pkg/system/phase4_configuring.go - the core and the endpoint change the mount path from /etc/ocp-injected-ca-bundle.crt (with file extension) to a directory (remove the extension).
  4. Rename AddToRootCAs to CombineCaBundle, add the const InjectedBundleCertCAFile which is /etc/ocp-injected-ca-bundle/ca-bundle.crt, and assign r.ApplyCAsToPods to be the new constant (will be NODE_EXTRA_CA_CERTS eventually).
  5. Change the condition of checking only the existence of the config map of the CA (configmap-ca-inject) to check this and also that we have data in it.
    The reason is that when we test the OLM (in the CI), we have the OLM and then we have the operator so the config map as it is created by the OLM, but in the first step it is empty.
    When manually looking at the config map in a real cluster there is data, that probably comes from additional needed configuration in the cluster (which is not in the scope of the test).
  6. Add a comment about ApplyCAsToPods so it would be clear that it is relevant to the endpoint and core pods.
  7. in CombineCaBundle use const instead of hard-coded string (to avoid circular dependency had to move the consts from options to util file).
  8. (this change might be temporary - need to decide) Change the ApplyCAsToPods to ServiceServingCertCAFile (previous InjectedBundleCertCAFile in comment).

Issues:

  1. Currently, when I tried to deploy with the cluster-bot I saw that NODE_EXTRA_CA_CERTS is empty, while it used to be
    NODE_EXTRA_CA_CERTS=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt.

GAPs:

  1. Tests end-to-end with certificates.

Testing Instructions:

Basic Manual Tests (more details in the comment below)

Check the value of NODE_EXTRA_CA_CERTS:

  1. I used the cluster-bot with launch 4.17 aws
  2. Installed ODF operator and MCG standalone according to the docs here.
  3. Check the env NODE_EXTRA_CA_CERTS:
  • oc exec statefulset/noobaa-core -c core -n openshift-storage -- printenv | grep NODE_EXTRA_CA_CERTS (was empty)
  • oc exec noobaa-endpoint-<characters> -n openshift-storage -- env | grep NODE_EXTRA_CA_CERTS (was empty)
  1. same steps with launch 4.14 aws in the cluster-bot (the version was chosen as a version before the changes of PR Modify the handling of injected OCP CA bundles #1328) we could see NODE_EXTRA_CA_CERTS=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt (both on core and endpoint).
  2. After the change we would see (both on core and endpoint pods):
  • NODE_EXTRA_CA_CERTS=/etc/ocp-injected-ca-bundle/ca-bundle.crt

  • NODE_EXTRA_CA_CERTS=/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt (with the commit mentioned in change number 8).

  • Doc added/updated

  • Tests added

@shirady
Copy link
Contributor Author

shirady commented Mar 24, 2025

Additional Testing Instructions:

The tests were done on the code changes in version 5.17.

1) Code change - path of cert

In pkg/utilutil.go - Change the /etc/ocp-injected-ca-bundle.crt to /etc/ocp-injected-ca-bundle/ca-bundle.crt(tested on the operator pod)

Explanation:

NODE_EXTRA_CA_CERTS is empty, since the value was assigned from:
(in the core pod)

case "NODE_EXTRA_CA_CERTS":
c.Env[j].Value = r.ApplyCAsToPods

(in the endpoint pod)
case "NODE_EXTRA_CA_CERTS":
c.Env[j].Value = r.ApplyCAsToPods

and r.ApplyCAsToPods is assigned after:
err = util.AddToRootCAs(options.ServiceServingCertCAFile)
if err == nil {
r.ApplyCAsToPods = options.ServiceServingCertCAFile
} else if !os.IsNotExist(err) {
log.Errorf("❌ NooBaa %q failed to add root CAs to system default", r.NooBaa.Name)
res.RequeueAfter = 3 * time.Second
return res, nil
}

but in the function AddToRootCAs the hard-coded path does not exist in the operator pod:
var certFiles = []string{
"/etc/ocp-injected-ca-bundle.crt",
localCertFile,
}

Before code changes:

In version 4.17 I run:

  • connect to the operator pod: oc rsh noobaa-operator-<characters>
  • check the previous path: ls -al /etc/pki/ca-trust/extracted/pem (it's a directory and it is generated from OCP - not NooBaa):
total 904
drwxr-xr-x. 3 root root    123 Sep 18  2024 .
drwxr-xr-x. 6 root root     70 Sep 18  2024 ..
-rw-r--r--. 1 root root    898 Aug 19  2024 README
dr-xr-xr-x. 2 root root  16384 Sep 18  2024 directory-hash
-r--r--r--. 1 root root 165521 Sep 18  2024 email-ca-bundle.pem
-r--r--r--. 1 root root 502506 Sep 18  2024 objsign-ca-bundle.pem
-r--r--r--. 1 root root 226489 Sep 18  2024 tls-ca-bundle.pem
  • check the code path in file level: ls -al /etc/ocp-injected-ca-bundle.crt
ls: cannot access '/etc/ocp-injected-ca-bundle.crt': No such file or directory
  • check the path in the directory level: ls -al /etc/ocp-injected-ca-bundle
total 0
drwxrwsrwx. 3 root 1000710000 81 Mar 24 10:06 .
drwxr-xr-x. 1 root root       36 Mar 24 10:06 ..
drwxr-sr-x. 2 root 1000710000 27 Mar 24 10:06 ..2025_03_24_10_06_38.1871780206
lrwxrwxrwx. 1 root 1000710000 32 Mar 24 10:06 ..data -> ..2025_03_24_10_06_38.1871780206
lrwxrwxrwx. 1 root 1000710000 20 Mar 24 10:06 ca-bundle.crt -> ..data/ca-bundle.crt
  • from this I took the file path: ls -al /etc/ocp-injected-ca-bundle/ca-bundle.crt
lrwxrwxrwx. 1 root 1000710000 20 Mar 24 10:06 /etc/ocp-injected-ca-bundle/ca-bundle.crt -> ..data/ca-bundle.crt

After code changes:

We can see the printing in the operator logs:
kubectl logs noobaa-operator-<characters>

time="2025-03-31T07:35:22Z" level=info msg="Successfuly appended \"/etc/ocp-injected-ca-bundle/ca-bundle.crt\" to RootCAs"
time="2025-03-31T07:35:22Z" level=info msg="Successfuly appended \"/var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt\" to RootCAs"

2) Code change - rename the r.CaBundleConf.Name

In pkg/system/reconciler.go - Rename r.CaBundleConf.Name from r.Request.Name + "-ca-inject" to "ocp-injected-ca-bundle"to align with the rename of configmap-ca-inject.yaml from noobaa-ca-inject to ocp-injected-ca-bundle in PR #1328

since CaBundleConf is:

CaBundleConf: util.KubeObject(bundle.File_deploy_internal_configmap_ca_inject_yaml).(*corev1.ConfigMap),

and the name is: "ocp-injected-ca-bundle"

const File_deploy_internal_configmap_ca_inject_yaml = `apiVersion: v1
kind: ConfigMap
metadata:
labels:
config.openshift.io/inject-trusted-cabundle: "true"
name: ocp-injected-ca-bundle
data: {}
`

and it doesn't match:

r.CaBundleConf.Name = r.Request.Name + "-ca-inject"

Therefore, whenever we check the name (for example):

if util.KubeCheckQuiet(r.CaBundleConf) {

it will not find it.

After code changes:

I added printing to validate that the value is true, for example (in SetDesiredCoreApp):

r.Logger.Info("SDSD in SetDesiredCoreApp - case core")
r.Logger.Info("SDSD r.CaBundleConf.Name ", r.CaBundleConf.Name)
myCheck := util.KubeCheckQuiet(r.CaBundleConf)
r.Logger.Info("SDSD myCheck in SetDesiredCoreApp - case core util.KubeCheckQuiet(r.CaBundleConf) ", myCheck)

We can see the printing in the operator logs:
kubectl logs noobaa-operator-<characters>

time="2025-03-31T07:33:23Z" level=info msg="SDSD in SetDesiredCoreApp - case core" sys=openshift-storage/noobaa
time="2025-03-31T07:33:23Z" level=info msg="SDSD r.CaBundleConf.Name ocp-injected-ca-bundle" sys=openshift-storage/noobaa
time="2025-03-31T07:33:23Z" level=info msg="SDSD myCheck in SetDesiredCoreApp - case core util.KubeCheckQuiet(r.CaBundleConf) true" sys=openshift-storage/noobaa

Note: all cases were true (checked with: kubectl logs noobaa-operator-7f6c74dd69-qqk6k | grep "util.KubeCheckQuiet(r.CaBundleConf) false" - empty output).

3) Code change - mount path of directory (don't show file extension)

In pkg/system/phase2_creating.go and pkg/system/phase4_configuring.go - the core and the endpoint change the mount path from /etc/ocp-injected-ca-bundle.crt (with file extension) to a directory (remove the extension).

seems like a file (extension):

MountPath: "/etc/ocp-injected-ca-bundle.crt",

It was (directory level - no extension):

MountPath: "/etc/pki/ca-trust/extracted/pem",

Therefore, it is suggested to remove the file extension.

Before code changes:

  • connect to the endpoint pod: oc rsh noobaa-endpoint-<characters>
  • check the previous path: ls -al /etc/pki/ca-trust/extracted/pem
total 904
drwxrwxr-x. 1 root root    123 Oct 24 12:50 .
drwxrwxr-x. 1 root root     70 Oct 24 12:50 ..
-rw-rw-r--. 1 root root    898 Aug 19  2024 README
dr-xr-xr-x. 1 root root  16384 Oct 24 12:50 directory-hash
-r--r--r--. 1 root root 165521 Oct 24 12:50 email-ca-bundle.pem
-r--r--r--. 1 root root 502506 Oct 24 12:50 objsign-ca-bundle.pem
-r--r--r--. 1 root root 226489 Oct 24 12:50 tls-ca-bundle.pem
sh-5.1# ls -al /etc/ocp-injected-ca-bundle
  • check the path in the directory level: ls -al /etc/ocp-injected-ca-bundle
ls: cannot access '/etc/ocp-injected-ca-bundle': No such file or directory

In the operator yaml we have the configmap property:

- name: ocp-injected-ca-bundle
configMap:
name: ocp-injected-ca-bundle
items:
- key: ca-bundle.crt
path: ca-bundle.crt
optional: true

but we don't have this either in the endpoint deployment or the core statefulset:
oc get statefulset noobaa-core -o yaml
oc get deployment noobaa-endpoint -o yaml

After code changes:

  • connect to the endpoint pod: oc rsh noobaa-endpoint-<characters>
    ls -al /etc/ocp-injected-ca-bundle
total 0
drwxrwsrwx. 3 root 1000710000 80 Mar 31 07:25 .
drwxrwxr-x. 1 root root      136 Mar 31 07:25 ..
drwxr-sr-x. 2 root 1000710000 27 Mar 31 07:25 ..2025_03_31_07_25_58.320495734
lrwxrwxrwx. 1 root 1000710000 31 Mar 31 07:25 ..data -> ..2025_03_31_07_25_58.320495734
lrwxrwxrwx. 1 root 1000710000 20 Mar 31 07:25 ca-bundle.crt -> ..data/ca-bundle.crt
  • same in the core pod: oc rsh noobaa-core-0

4) Code change - path of cert

Rename AddToRootCAs to CombineCaBundle, add the const InjectedBundleCertCAFile which is /etc/ocp-injected-ca-bundle/ca-bundle.crt, and assign r.ApplyCAsToPods to be the new constant (will be NODE_EXTRA_CA_CERTS eventually).

Explanation:

NODE_EXTRA_CA_CERTS is assigned from:
(in the core pod)

case "NODE_EXTRA_CA_CERTS":
c.Env[j].Value = r.ApplyCAsToPods

(in the endpoint pod)
case "NODE_EXTRA_CA_CERTS":
c.Env[j].Value = r.ApplyCAsToPods

and r.ApplyCAsToPods is assigned after:
if err == nil {
r.ApplyCAsToPods = options.ServiceServingCertCAFile
} else if !os.IsNotExist(err) {
log.Errorf("❌ NooBaa %q failed to add root CAs to system default", r.NooBaa.Name)
res.RequeueAfter = 3 * time.Second
return res, nil
}

now we added the constant, and the env NODE_EXTRA_CA_CERTS will be assigned to /etc/ocp-injected-ca-bundle/ca-bundle.crt.

Before code changes:

oc exec statefulset/noobaa-core -c core -n openshift-storage -- printenv | grep NODE_EXTRA_CA_CERTS (was empty)
oc exec noobaa-endpoint-<characters> -n openshift-storage -- env | grep NODE_EXTRA_CA_CERTS (was empty)

After code changes:

oc exec noobaa-core-0 -n openshift-storage -- env | grep NODE_EXTRA_CA_CERTS

Defaulted container "core" out of: core, noobaa-log-processor
NODE_EXTRA_CA_CERTS=/etc/ocp-injected-ca-bundle/ca-bundle.crt

oc exec noobaa-endpoint-56979564f4-pstkr -n openshift-storage -- env | grep NODE_EXTRA_CA_CERTS

NODE_EXTRA_CA_CERTS=/etc/ocp-injected-ca-bundle/ca-bundle.crt

5) Code change - Change the condition existence of the config map of the CA (configmap-ca-inject) + data in it.

The reason is that when we test the OLM (in the CI), we have the OLM and then we have the operator so the config map as it is created by the OLM, but in the first step it is empty.
When manually looking at the config map in a real cluster there is data, that probably comes from additional needed configuration in the cluster (which is not in the scope of the test).

The config map ocp-injected-ca-bundle is created by the olm after the operator is installed and gets its CA injected by the network-operator (in namespace openshift-network-operator).

I could see in a cluster:
oc logs -n openshift-network-operator deploy/network-operator

I0406 07:30:35.257033       1 log.go:245] reconciling (/v1, Kind=ConfigMap) openshift-storage/ocp-injected-ca-bundle
I0406 07:30:35.300056       1 log.go:245] Apply / Create of (/v1, Kind=ConfigMap) openshift-storage/ocp-injected-ca-bundle was successful

later this controller reconciles and there no changes, for example:

I0406 07:30:35.309980       1 log.go:245] Reconciling configmap from  openshift-storage/ocp-injected-ca-bundle
I0406 07:30:35.311865       1 log.go:245] ConfigMap openshift-storage/ocp-injected-ca-bundle ca-bundle.crt unchanged, skipping
I0406 07:30:35.326456       1 log.go:245] Reconciling configmap from  openshift-storage/ocp-injected-ca-bundle
I0406 07:30:35.328750       1 log.go:245] ConfigMap openshift-storage/ocp-injected-ca-bundle ca-bundle.crt unchanged, skipping

To summarize what is happening from my understanding:

  1. The customer adds its private CAs using open shift.
  2. When he installed ODF operator it also installed the noobaa-operator which triggers the OLM to create an empty config map ocp-injected-ca-bundle
  3. The configmap has the label config.openshift.io/inject-trusted-cabundle: "true" which the pod of the network-operator watched and reconciled (we saw it in the logs + references - upstream docs, official docs).
  4. The endpoint and core pods should be created after the operator pod and therefore will have the mount path for the CA (the condition checks that the config map exists and has data in it).

@shirady shirady requested review from Neon-White and jackyalbo March 24, 2025 15:57
@shirady shirady self-assigned this Mar 24, 2025
@shirady shirady force-pushed the check-certs branch 4 times, most recently from 47a7b8c to 1ccce7a Compare March 30, 2025 11:06
@shirady shirady force-pushed the check-certs branch 3 times, most recently from 6af0112 to c671827 Compare April 3, 2025 08:30
@nimrod-becker nimrod-becker requested a review from dannyzaken April 3, 2025 13:29
@shirady shirady marked this pull request as ready for review April 6, 2025 09:05
shirady added 8 commits April 7, 2025 18:21
…ndle/ca-bundle.crt (tested on operator pod)

Signed-off-by: shirady <[email protected]>
…-injected-ca-bundle"

to align with the rename of configmap-ca-inject.yaml from noobaa-ca-inject to ocp-injected-ca-bundle in PR 1328

Signed-off-by: shirady <[email protected]>
…CertCAFile which is /etc/ocp-injected-ca-bundle/ca-bundle.crt +

assign r.ApplyCAsToPods to be the new constant (will be NODE_EXTRA_CA_CERTS eventually)

Signed-off-by: shirady <[email protected]>
(cherry picked from commit 41215ae64ada1a5c7cd5a85322201e64a2945fda)
…t the data in the core and endpoint pods

Signed-off-by: shirady <[email protected]>
(to avoid circular dependency had to move the consts from options to util)

Signed-off-by: shirady <[email protected]>
…jectedBundleCertCAFile in comment)

Signed-off-by: shirady <[email protected]>
@shirady shirady marked this pull request as draft April 7, 2025 15:28
@shirady
Copy link
Contributor Author

shirady commented Apr 10, 2025

Updates

This PR moved to draft (might be closed and a new PR / revert PR will be open):
After the current changes, the core and the endpoint have the env NODE_EXTRA_CA_CERTS with /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt (“ServiceServingCertCAFile” in the code which contains the openshift-service-serving as an issuer) - but this cert is partial. In version 5.14 we also had the certs that were mounted in the pods by overriding the existing path:
oc get pod noobaa-core-0 -o yaml | less

    - mountPath: /etc/pki/ca-trust/extracted/pem
      name: noobaa-ca-inject

The NODE_EXTRA_CA_CERTS is a single path (we cannot pass a couple of paths), and this env cannot be set and changed while the system is running (so we cannot create a workaround for updating when the cluster is up) -Node js documentation.
Therefore, it was overridden in the past (until version 5.14) - but returning the mount path as it was might recreate the race that we used to have that PR #1328 tries to solve.

Recommendation of what the testing end-to-end can include:

  1. tests with certificates of 2 kinds: internal to the cluster (like with RGW backingstore) and external to the cluster (for a server outside of the cluster).
  2. The test plan must also include 2 cases:
  • When a (correct) certificate is added - expected to create a backingstore successfully (also must look at the logs of noobaa-core and see that "request_unsecured true protocol https"), continue to send requests from the endpoint pod and check that as well.
  • When a wrong certificate is added - expect the backingstore to fail upon creation.
  • Updating certificate - should we delete the pod and the new certificate is loaded?

@Neon-White Neon-White mentioned this pull request Apr 14, 2025
@shirady
Copy link
Contributor Author

shirady commented Apr 21, 2025

I'm closing this PR as it continues in PR #1587

@shirady shirady closed this Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant