Skip to content

Add Trusted CA to Proxy API type #377

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Jul 23, 2019
Merged

Add Trusted CA to Proxy API type #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions config/v1/types_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,29 @@ type ProxySpec struct {
// readinessEndpoints is a list of endpoints used to verify readiness of the proxy.
// +optional
ReadinessEndpoints []string `json:"readinessEndpoints,omitempty"`

// trustedCA is a reference to a ConfigMap containing a CA certificate bundle used
// for client egress HTTPS connections. The certificate bundle must be from the CA
// that signed the proxy's certificate and be signed for everything. trustedCA should
// only be consumed by a proxy validator. The validator is responsible for reading
// ConfigMapNameReference, validating the certificate and copying "ca-bundle.crt"
// from data to a ConfigMap in the namespace of an operator configured for proxy.
// The namespace for this ConfigMap is "openshift-config-managed". Here is an example
// ConfigMap (in yaml):
//
// apiVersion: v1
// kind: ConfigMap
// metadata:
// name: proxy-ca
// namespace: openshift-config-managed
// data:
// ca-bundle.crt: |
// -----BEGIN CERTIFICATE-----
// Custom CA certificate bundle.
// -----END CERTIFICATE-----
//
// +optional
TrustedCA ConfigMapNameReference `json:"trustedCA,omitempty"`
Copy link
Contributor

@bparees bparees Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@danehans danehans Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhinavdahiya what are your thoughts on the installer requirements @bparees highlights above?

Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bparees bparees Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@bparees bparees Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@bparees bparees Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jcpowermac jcpowermac Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jcpowermac jcpowermac Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@bparees bparees Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@deads2k deads2k Jul 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// ProxyStatus shows current known state of the cluster proxy.
Expand Down
1 change: 1 addition & 0 deletions config/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions config/v1/zz_generated.swagger_doc_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.