Add Trusted CA Bundle to Build Controller #21362
Conversation
openshift-ci-robot
added
the
size/XXL
|
/hold Waiting on api and client-go to settle down. |
openshift-ci-robot
added
the
do-not-merge/hold
| for bundleName := range buildCAData { | ||
| caBundles[i] = bundleName | ||
| i++ | ||
| } |
There was a problem hiding this comment.
Main challenge is that if we mount the CAs by their ConfigMap key, we need to know which CAs we have prior to creating the build pod. My approach is to generate the CA data first, then pass the keys of the CA files into the build pod.
There was a problem hiding this comment.
UPDATE: If a ConfigMap is mounted with Items, it will only mount the keys referenced in the Items list. We will be mounting the service-signing-cert by referencing it's key, since we are relying on the kubelet to hold off creating the container until the cert is injected. Therefore, we must also reference the additional trusted CA by key name.
| glog.Warningf("Failed to read additional trusted CA bundle %s: %v", bc.additionalTrustedCA, err) | ||
| return data | ||
| } | ||
| data["additional-ca.crt"] = string(pemData) |
There was a problem hiding this comment.
Failing with a warning here if the CA bundle can't be found.
TODOs:
- Don't log if the error is a
NotFounderror? - Validate the data is PEM-encoded?
There was a problem hiding this comment.
this seems like a lot of extra logic when in the end we only have a single key and bundle we care about.
ultimately (when all the pieces are in place) i expect the configmap to have at most two keys:
- additionalCAs (optional)
- serviceCA
and we know that if (1) is going to be present, it will be populated when we create the configmap, so you don't have to do anything special in terms of specifically referencing mounting the key for (1) when you setup the configmap volume. You only need to do that for (2).
| glog.Warningf("Failed to find additional trusted CA bundle %s: %v", bc.additionalTrustedCA, err) | ||
| return data | ||
| } | ||
| pemData, err := ioutil.ReadFile(bc.additionalTrustedCA) |
There was a problem hiding this comment.
we should not have to read this data on every build. If the data changes we should expect the operator to restart the controller, so we can read it once during controller startup.
|
/refresh |
* Aligns with openshift/origin#21362
adambkaplan
force-pushed
the
buildconfig-api
branch
from
0f5a4e4
to
1ae628e
Compare
| for bundleName := range buildCAData { | ||
| caBundles[i] = bundleName | ||
| i++ | ||
| } |
There was a problem hiding this comment.
UPDATE: If a ConfigMap is mounted with Items, it will only mount the keys referenced in the Items list. We will be mounting the service-signing-cert by referencing it's key, since we are relying on the kubelet to hold off creating the container until the cert is injected. Therefore, we must also reference the additional trusted CA by key name.
|
/hold cancel @bparees one question re: failure modes reading the CA bundle. |
openshift-ci-robot
removed
the
do-not-merge/hold
* Aligns with openshift/origin#21362
adambkaplan
force-pushed
the
buildconfig-api
branch
2 times, most recently
from
b4a6cad
to
7a2ccd0
Compare
* BuildController reads additional trusted CA bundle from disk. * Trusted CA bundle mounted into build pods via a ConfigMap.
openshift-ci-robot
added
size/L
|
/retest |
|
bump @bparees |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, bparees 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@adambkaplan: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Read CA bundle from disk so it can be mounted into build pods.