-
Notifications
You must be signed in to change notification settings - Fork 55
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
Inject certificate to http client from a configmap referenced in the … #1259
Conversation
Hi @vinokurig. Thanks for your PR. I'm waiting for a devfile member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
9220acb
to
458d2d9
Compare
b10e11e
to
a13aa5a
Compare
df0d849
to
a523bd8
Compare
Since httpsClient can be changed after configuring certificates, it is better to move initialization from |
@dkwon17 please have a look at this PR when you have a moment & I encourage you to test it on CRC if possible (though if you are caught up with other tasks, don't worry -- having another set of eyes on the code is what matters most right now :) ). |
9f99904
to
98df927
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @dkwon17 please have a look when you have a moment.
InjectCertificates(k8s, logger) | ||
} | ||
|
||
func InjectCertificates(k8s client.Client, logger logr.Logger) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exported functions are generally placed above non-exported functions (in the code). However, this is just a style nitpick that I forgot to mention and can be changed later on as I don't want to delay merging this PR due to time zone differences and the upcoming DWO 0.28.0 upstream release that we will hopefully start tomorrow.
controllers/workspace/http.go
Outdated
func injectCertificates(certsPem []byte, transport *http.Transport) { | ||
caCertPool := transport.TLSClientConfig.RootCAs | ||
if caCertPool == nil { | ||
caCertPool = x509.NewCertPool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to [1]
// RootCAs defines the set of root certificate authorities
// that clients use when verifying server certificates.
// If RootCAs is nil, TLS uses the host's root CA set.
@vinokurig should we use SystemCertPool instead?
caCertPool = x509.NewCertPool() | |
systemCertPool, err := x509.SystemCertPool() | |
caCertPool = systemCertPool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @dkwon17.
I'm under the impression we should be using the SystemCertPool as well. If the DWOC is configured to use specific certs, a DevWorkspace that tries to resolve a URI that uses another certificate will fail (when this should normally would work). For example, https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml
can normally be resolved, but cannot when custom certs are configured with the functionality this PR introduces.
To reproduce this, the following devworkspace can be used:
apiVersion: workspace.devfile.io/v1alpha2
kind: DevWorkspace
metadata:
name: code-latest
namespace: openshift-operators
spec:
contributions:
- components:
- container:
env:
- name: CODE_HOST
value: 0.0.0.0
name: che-code-runtime-description
name: che-code
uri: 'https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml'
started: false
template:
commands:
- exec:
commandLine: echo "Hello from $(pwd)"
component: dev
workingDir: '${PROJECT_SOURCE}/app'
id: say-hello
components:
- container:
cpuRequest: 1000m
image: 'quay.io/devfile/universal-developer-image:latest'
memoryLimit: 512Mi
memoryRequest: 256Mi
sourceMapping: /projects
name: dev
projects:
- git:
remotes:
origin: 'https://github.com/che-samples/web-nodejs-sample.git'
name: web-nodejs-sample
The above devworkspace fails with:
status:
conditions:
- lastTransitionTime: '2024-05-22T19:45:49Z'
message: 'Error processing devfile: failed to resolve component che-code by URI: failed to fetch file from https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml: Get "https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml": tls: failed to verify certificate: x509: certificate signed by unknown authority'
reason: BadRequest
status: 'True'
type: FailedStart
If the call to x509.SystemCertPool()
returns an error, we could either log the error and abandon trying to inject the certs, or we could create a new cert pool with x509.NewCertPool()
and log the error. The latter option is probably preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinokurig please consider @dkwon17's request to use the System Cert Pool instead of a new Cert Pool. If you have any opposing thoughts to this, however, please bring them up .
Also, for reference, here's how I've been testing this (thanks to @dkwon17 for the help with figuring this out):
- Install DWO with the changes from this PR using olm: Set the
DWO_IMG
environment variable to your quay repo, do amake docker
, then set theDWO_BUNDLE_IMG
,DWO_INDEX_IMG
&DEFAULT_DWO_IMG
variables to your quay repos. Note:DEFAULT_DWO_IMG
==DWO_IMG
. Finally, do amake generate_olm_bundle_yaml build_bundle_and_index register_catalogsource
while logged into an OpenShift cluster. - Install Che from OperatorHub, create a Che Cluster CR and wait for Che to be started (i.e. the Che Dashboard URL is ready and accessible)
- Find the Che plugin registry URL (can be seen on the Che Cluster CR in OpenShift UI), something like:
https://eclipse-che.apps.ci-ln-f2dgggk-72292.origin-ci-int-gce.dev.rhcloud.com/plugin-registry/v3
oc apply -f ./samples/code-latest.yaml
(i.e. this sample devworkspace)- Modify the code-latest devworkspace so that the contribution URI points to
<che-plugin-registry-url>/plugins/che-incubator/che-code/latest/devfile.yaml
. For example:https://eclipse-che.apps.ci-ln-f2dgggk-72292.origin-ci-int-gce.dev.rhcloud.com/plugin-registry/v3/plugins/che-incubator/che-code/latest/devfile.yaml
. @dkwon17's earlier comment gives an example of this. - The DevWorkspace should fail to resolve due to
tls: failed to verify certificate: x509: certificate signed by unknown authority'
- Visit the Che Plugin registry URL that hosts
che-code/latest/devfile.yaml
(from step 5.) and download the .cert from your browser. - Create a configmap using the downloaded cert data, and reference it in the DWOC
- Set the
code-latest
DevWorkspacespec.started
totrue
. - The DevWorkspace should now resolve the che-code component, and start up eventually.
To verify that the default system pool certs are working for other certificate authorities, repeat step 5. but use https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml
for the uri of the DevWorkspace's che-code contribution:
spec:
contributions:
- components:
- container:
env:
- name: CODE_HOST
value: 0.0.0.0
name: che-code-runtime-description
name: che-code
uri: 'https://eclipse-che.github.io/che-plugin-registry/main/v3/plugins/che-incubator/che-code/latest/devfile.yaml'
The DevWorkspace should start up successfully.
@vinokurig thanks for the latest updates :) Will begin testing shortly. If all seems well on both my end and @dkwon17's end, we'll squash your fixup commits and have this merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected and code looks good to me :) Amazing work @vinokurig!
@dkwon17 gave me his +1 on the final changes over slack.
I tested this PR using the steps I mentioned here and everything works as expected.
Will squash the fixup commits and have this merged.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AObuchow, dkwon17, ibuziuk, tolusha, vinokurig 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 |
New changes are detected. LGTM label has been removed. |
…config Signed-off-by: ivinokur <[email protected]>
Signed-off-by: Igor Vinokur <[email protected]>
…config
What does this PR do?
What issues does this PR fix or reference?
fixes #1248
Is it tested? How?
parent.yaml
:devfile.yaml
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-path
to trigger)v8-devworkspace-operator-e2e
: DevWorkspace e2e testv8-che-happy-path
: Happy path for verification integration with Che