Add Trusted CA to Proxy API type #377
Conversation
openshift-ci-robot
added
the
size/XS
| // present, consumers are responsible for reading ConfigMapNameReference, copying the ConfigMap | ||
| // content and using the CA as needed. The namespace for this ConfigMap is "openshift-config". | ||
| // +optional | ||
| TrustedCA ConfigMapNameReference `json:"trustedCA,omitempty"` |
There was a problem hiding this comment.
this was my proposal but i'd like to see @derekwaynecarr or another architect second it before we make it official.
also this presumably means the installer has to be able to collect CAs from the user and populate this field+configmap during install.
There was a problem hiding this comment.
@abhinavdahiya what are your thoughts on the installer requirements @bparees highlights above?
There was a problem hiding this comment.
the configmap is useful when it can be used as a volume to a pod. like the cloudproviderconfig configmap..
For most consumers of this api, that's not possible.. and most of them will need to fetch (latest) proxy object (latest) configmap and then make the request using proxy..
this also means that configmap contents can change and clients need to be reacting to that too..
so personally i think since CAs are not secret, the bundle in the object itself is more ergonomic, but depends if @deads2k and other approvers agree if that's a good API design.
There was a problem hiding this comment.
The value in making it a configmap reference is that we have numerous places in our cluster config resources where we need a list of CAs. Making it a reference to a configmap allows an admin to share those CAs if they so choose. Requiring the CAs be part of each resource (proxy config, build config, image config, etc) requires duplicating the values.
There was a problem hiding this comment.
in anycase @abhinavdahiya i think the real question to you is, do you foresee issues w/ the installer collecting CAs and populating additional cluster config w/ the CA values (in addition to the proxy config the installer was already going to be collecting and populating)
There was a problem hiding this comment.
The advantage of a fixed configmap would be that we can mount it statically into pods, including operators.
don't we need to copy it into every operator's namespace anyway to mount it? I wasn't aware you can mount a configmap from another namespace.
Anyway this is not the data being mounted as it is not in a directly consumable form. This data is going to be aggregated along w/ the system CAs, into a new config map in openshift-config-managed that has the structure of /etc/pki. If there is value in having that configmap have a static name, we can do so.
I question the value of making this a configmap reference in the proxy object.
Because you don't believe we'd have reason to reuse the same CA configmap in multiple places? We already have APIs that consume CAs via a configmap reference, so if this API does not also do that, we are ensuring people must duplicate their CAs.
There was a problem hiding this comment.
Instead of a ConfigMap this really needs to be the CA string for bootstrap. In the bootstrap process the CA will need to be added the host's ca trust before trying to pull the images.
There was a problem hiding this comment.
Instead of a ConfigMap this really needs to be the CA string for bootstrap. In the bootstrap process the CA will need to be added the host's ca trust before trying to pull the images.
nevermind
There was a problem hiding this comment.
Another point: we have the configmap reference here in the spec, but not in the status. Also the config map can be edited by the admin without any validation through the controller.
Again since there is a controller that will be responsible for taking this "user provided CA configmap" and converting it into the "/etc/pki merged configmap" that pods actually consume, that controller can validate the CA/proxy config before it update the "/etc/pki merged configmap" that lives in openshift-config-managed.
There was a problem hiding this comment.
I like having a configmap reference. ConfigMaps are easy for cluster admins to build and manage. We use references heavily in the rest of our cluster config resources. Giving flexibility to a cluster-admin to choose locations and specificity for post-processing is a pattern that has worked effectively for other cluster-admin inputs so far.
openshift-ci-robot
added
the
needs-rebase
|
ping @danehans we still need this right? |
|
@abhinavdahiya I'm waiting for confirmation whether this field is required for 4.2. Hold off until I provide an update. |
|
@abhinavdahiya we need something like this. Looking for @derekwaynecarr @deads2k @sttts to provide feedback based on the proxy ca discussions from here |
openshift-ci-robot
added
size/S
|
@abhinavdahiya @deads2k @bparees should the field name being added by this PR be the same as openshift/installer#2055 for consistency between the api's? |
my vote would be "nice but not critical/necessary". others may feel more strongly. i'm giving this an approval since i think the field is correct, but the godoc still needs cleanup per my comments in slack: https://coreos.slack.com/archives/CLJSH16J0/p1563834237009100 /approve |
openshift-ci-robot
added
the
approved
|
@bparees I updated the PR based on your Slack feedback, PTAL. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, danehans The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8 participants
Adds
TrustedCAto the proxy.spec for MITM HTTPS/TLS proxy support.@bparees @derekwaynecarr @mrunalp PTAL.