Repository: tektoncd/operator
Pull request #549 (2021-12-08 12:59:06)

Stop overriding system defaults in SSL_CERT_DIR
# Changes

Prior to this commit, we used to set default value of SSL_CERT_DIR in
our TaskRun, etc pods to `/tekton-custom-certs`; while this worked for
certs mounted via the bundle config maps, this broke use cases where users
relied on default locations (like `/etc/ssl/certs`,
`/etc/pki/tls/certs`, etc) for already existing or user-mounted certificates.

This commit fixes that by setting SSL_CERT_DIR as a list of directories
which contains `/tekton-custom-certs` and the system default locations
as well.

# Release Notes

```release-note
System default locations for certificates are no longer overridden by SSL_CERT_DIR
```

tekton-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **houshengbo**
You can assign the PR to them by writing `/assign @houshengbo` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["houshengbo"]} -->
tekton-robot: @concaf: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468565876432179203/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/proxy/proxy.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-go-coverage/1468565876432179200/artifacts/line-cov.html#file1) | 10.1% | 10.7% | 0.6

tekton-robot: @concaf: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468565876432179203/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @concaf: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468565876432179203/) | `/test pull-tekton-operator-unit-tests`
pull-tekton-operator-build-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-build-tests/1468565876432179201/) | `/test pull-tekton-operator-build-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @concaf: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-unit-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468565876432179203/) | `/test pull-tekton-operator-unit-tests`
pull-tekton-operator-build-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-build-tests/1468565876432179201/) | `/test pull-tekton-operator-build-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/proxy/proxy.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-go-coverage/1468568644526018563/artifacts/line-cov.html#file1) | 10.1% | 10.7% | 0.6

tekton-robot: @concaf: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-build-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-build-tests/1468565876432179201/) | `/test pull-tekton-operator-build-tests`
pull-tekton-operator-unit-tests | d5de419a74f0c1d706a62f872ec9f3cb4c6ce512 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468568644526018560/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @concaf: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-build-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-build-tests/1468565876432179201/) | `/test pull-tekton-operator-build-tests`
pull-tekton-operator-unit-tests | d5de419a74f0c1d706a62f872ec9f3cb4c6ce512 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468568644526018560/) | `/test pull-tekton-operator-unit-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @concaf: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-integration-tests | d5de419a74f0c1d706a62f872ec9f3cb4c6ce512 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-integration-tests/1468568644526018562/) | `/test pull-tekton-operator-integration-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: @concaf: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-tekton-operator-build-tests | 2cd981b7e5076c8c95eee100ef43770999cd4e02 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-build-tests/1468565876432179201/) | `/test pull-tekton-operator-build-tests`
pull-tekton-operator-unit-tests | d5de419a74f0c1d706a62f872ec9f3cb4c6ce512 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-unit-tests/1468568644526018560/) | `/test pull-tekton-operator-unit-tests`
pull-tekton-operator-integration-tests | d5de419a74f0c1d706a62f872ec9f3cb4c6ce512 | [link](https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-integration-tests/1468568644526018562/) | `/test pull-tekton-operator-integration-tests`

[Full PR test history](https://tekton-releases.appspot.com/pr/tektoncd_operator/549). [Your PR dashboard](https://prow.tekton.dev/pr/concaf).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
tekton-robot: The following is the coverage report on the affected files.
Say `/test pull-tekton-operator-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/reconciler/proxy/proxy.go](https://storage.cloud.google.com/tekton-prow/pr-logs/pull/tektoncd_operator/549/pull-tekton-operator-go-coverage/1468570532491300864/artifacts/line-cov.html#file1) | 10.1% | 10.7% | 0.6

concaf: /test pull-tekton-operator-integration-tests
nikhil-thomas: /approve
:heavy_check_mark:  lgtm
tekton-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tektoncd/operator/pull/549#issuecomment-988818255" title="Approved">nikhil-thomas</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)~~ [nikhil-thomas]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
tekton-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/tektoncd/operator/pull/549#issuecomment-988818255" title="Approved">nikhil-thomas</a>*, *<a href="https://github.com/tektoncd/operator/pull/549#pullrequestreview-826446255" title="LGTM">vdemeester</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=tektoncd%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/tektoncd/operator/blob/main/OWNERS)~~ [nikhil-thomas,vdemeester]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/openshift/common/cabundle.go b/pkg/reconciler/openshift/common/cabundle.go
index 0995018f4..88eee7223 100644
--- a/pkg/reconciler/openshift/common/cabundle.go
+++ b/pkg/reconciler/openshift/common/cabundle.go
@@ -114,7 +114,7 @@ func ApplyCABundles(u *unstructured.Unstructured) error {
 			}
 		}
 
-		// we wiil mount the certs at this location so we don't override the existing certs
+		// We will mount the certs at this location so we don't override the existing certs
 		sslCertDir := "/tekton-custom-certs"
 		for _, env := range c.Env {
 			if env.Name == "SSL_CERT_DIR" {
diff --git a/pkg/reconciler/proxy/proxy.go b/pkg/reconciler/proxy/proxy.go
index 572cb3964..90e8786fd 100644
--- a/pkg/reconciler/proxy/proxy.go
+++ b/pkg/reconciler/proxy/proxy.go
@@ -402,10 +402,13 @@ func updateVolume(pod corev1.Pod, volumeName, configmapName, key string) corev1.
 			}
 		}
 
-		// we wiil mount the certs at this location so we don't override the existing certs
+		// We will mount the certs at this location so we don't override the existing certs
 		sslCertDir := "/tekton-custom-certs"
 		certEnvAvaiable := false
+
 		for _, env := range c.Env {
+			// If SSL_CERT_DIR env var already exists, then we don't mess with
+			// it and simply carry it forward as it is
 			if env.Name == "SSL_CERT_DIR" {
 				sslCertDir = env.Value
 				certEnvAvaiable = true
@@ -413,9 +416,29 @@ func updateVolume(pod corev1.Pod, volumeName, configmapName, key string) corev1.
 		}
 
 		if !certEnvAvaiable {
+			// Here, we need to set the default value for SSL_CERT_DIR.
+			// Keep in mind that if SSL_CERT_DIR is set, then it overrides the
+			// system default, i.e. the system default directories will "NOT"
+			// be scanned for certificates. This is risky and we don't want to
+			// do this because users mount certificates at these locations or
+			// build images with certificates "in" them and expect certificates
+			// to get picked up, and rightfully so since this is the documented
+			// way of achieving this.
+			// So, let's keep the system wide default locations in place and
+			// "append" our custom location to those.
+			//
+			// Copied from https://golang.org/src/crypto/x509/root_linux.go
+			var certDirectories = []string{
+				sslCertDir,                     // /tekton-custom-certs
+				"/etc/ssl/certs",               // SLES10/SLES11, https://golang.org/issue/12139
+				"/etc/pki/tls/certs",           // Fedora/RHEL
+				"/system/etc/security/cacerts", // Android
+			}
+
 			c.Env = append(c.Env, corev1.EnvVar{
-				Name:  "SSL_CERT_DIR",
-				Value: sslCertDir,
+				Name: "SSL_CERT_DIR",
+				// SSL_CERT_DIR accepts a colon separated list of directories
+				Value: strings.Join(certDirectories, ":"),
 			})
 		}
 


Test patch:
diff --git a/pkg/reconciler/proxy/proxy_test.go b/pkg/reconciler/proxy/proxy_test.go
index 31e30c894..c626cb97c 100644
--- a/pkg/reconciler/proxy/proxy_test.go
+++ b/pkg/reconciler/proxy/proxy_test.go
@@ -39,7 +39,7 @@ func TestUpdateVolume(t *testing.T) {
 	assert.DeepEqual(t, len(podUpdated.Spec.Volumes), 1)
 	assert.DeepEqual(t, len(podUpdated.Spec.Containers[0].Env), 1)
 	assert.DeepEqual(t, podUpdated.Spec.Containers[0].Env[0].Name, "SSL_CERT_DIR")
-	assert.DeepEqual(t, podUpdated.Spec.Containers[0].Env[0].Value, "/tekton-custom-certs")
+	assert.DeepEqual(t, podUpdated.Spec.Containers[0].Env[0].Value, "/tekton-custom-certs:/etc/ssl/certs:/etc/pki/tls/certs:/system/etc/security/cacerts")
 	assert.DeepEqual(t, podUpdated.Spec.Volumes[0].Name, "testv")
 	assert.DeepEqual(t, podUpdated.Spec.Volumes[0].ConfigMap.Name, "testcm")
 	assert.DeepEqual(t, len(podUpdated.Spec.Containers[0].VolumeMounts), 1)