Inject certificate to http client from a configmap referenced in the …

vinokurig commented 2 months ago

…config

What does this PR do?

Add a new config to be able to define a configmap with certificate content
Add a watcher that will trigger certificate injection to http client on the configmap create / update events

What issues does this PR fix or reference?

fixes https://github.com/devfile/devworkspace-operator/issues/1248

Is it tested? How?

Define certificate configmap in the config:

config:
routing:
tlsCertificateConfigmapRef:
  name: <configmap name>
  namespace: <configmap namespace>

Create the configmap with certificate:

kind: ConfigMap
apiVersion: v1
metadata:
name: <the name from the config>
namespace: <the namespace from the config>
labels:
controller.devfile.io/watch-configmap: 'true'
data:
custom-ca-certificates.pem: <certificate pem content>

To test the http client a git provider hosted with self signed certificate is required:
- Create a public repository with parent.yaml:
```
schemaVersion: 2.2.0
metadata:
name: parent-devfile
components:
```
- name: tools attributes: controller.devfile.io/merge-contribution: true container: image: quay.io/devspaces/udi-rhel8:latest env:
  - name: PARENT_ENV_VAR value: 'true' commands:
- id: parent-command exec: label: '2. This command from the parent' component: tools commandLine: echo "Hello from parent"
- Create devfile.yaml
```
schemaVersion: 2.2.0
metadata:
name: sample-using-parent
parent:
uri: <RAW url to the parent yaml>
components:
```
- name: tools container: env:
  - name: DEVFILE_ENV_VAR value: 'true' commands:
- id: devfile-command exec: label: '1. This command from the devfile' component: tools commandLine: echo "Hello from devfile"
Start a workspace from the repository.
PR Checklist

[ ] E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
- [ ] v8-devworkspace-operator-e2e: DevWorkspace e2e test
- [ ] v8-che-happy-path: Happy path for verification integration with Che

openshift-ci[bot] commented 2 months ago

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tolusha commented 1 month ago

Since httpsClient can be changed after configuring certificates, it is better to move initialization from SetupWithManager to Reconcile

tolusha commented 1 month ago

Sounds good, need to be tested

vinokurig commented 1 month ago

Tested the latest PR version with the steps to reproduce: works as expected.

dkwon17 commented 1 month ago

I was able to successfully test this feature on CRC, by adding a plugin contribution (spec.contributions) uri hosted from CRC which is signed by a self-signed certificate:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: code-latest
spec:
  started: true
  template:
    projects:
      - name: web-nodejs-sample
        git:
          remotes:
            origin: "https://github.com/che-samples/web-nodejs-sample.git"
    components:
      - name: dev
        container:
          image: quay.io/devfile/universal-developer-image:latest
          memoryLimit: 512Mi
          memoryRequest: 256Mi
          cpuRequest: 1000m
    commands:
      - id: say-hello
        exec:
          component: dev
          commandLine: echo "Hello from $(pwd)"
          workingDir: ${PROJECT_SOURCE}/app
  contributions:
    - name: che-code
      uri: https://eclipse-che.apps-crc.testing/plugin-registry/v3/plugins/che-incubator/che-code/latest/devfile.yaml
      components:
        - name: che-code-runtime-description
          container:
            env:
              - name: CODE_HOST
                value: 0.0.0.0

AObuchow commented 1 month ago

@vinokurig thanks for the latest changes :) Unfortunately, your last commit seems to have misformated imports I believe, as the CI formatting check is still failing. Please run make fmt again when you have a moment.

AObuchow commented 1 month ago

@vinokurig Thank you for the latest changes! Please run make fmt again when you have a moment.

AObuchow commented 1 month ago

The controller env tests are currently failing because of the change from having the HTTP client be setup on controller initialization basis to a per-reconcile basis. Previously, we used to mock the HTTP transport. Before the tests which needed a mocked HTTP transport object, it would be configured by the testing suite. But now, the HTTP transport is being configured during the workspace reconcile, overriding the mocked value.

I'm not sure yet of how to elegantly solve this problem.

We might be able to modify the setupHttpClients() function to check whether a test environment is being used, and not override the mocked http.Transport object if so. Though, it seems a bit awkward to check for a testing environment in production code.

vinokurig commented 1 month ago

@AObuchow What if we revert the healthcheck http to be initialised on operator start but leave the base http client to be initialised on Reconcile?

AObuchow commented 1 month ago

@AObuchow What if we revert the healthcheck http to be initialised on operator start but leave the base http client to be initialised on Reconcile?

This would work and is a potential option we could take (i.e. something like have separate setupHealthCheckHttpClient() and setupTransportHttpClient() functions). However, I'm not the biggest fan of making this change since it moves around code that used to be executed in a single place, though I'm open to it.

Since httpsClient can be changed after configuring certificates, it is better to move initialization from SetupWithManager to Reconcile

After reconsideration, I don't think it's worth moving the HTTP client initialization logic from the operator startup to the reconciliation function, given the complications it brings to the test suite. It definitely is advantageous from a UX point of view to have the HTTP client re-initialized on reconcile, as it allows users to configure the certificates while the operator is running and have the operator pick up these changes more immediately. However, previously admins just had to restart the devworkspace-controller pod to pick up HTTP configuration changes from the DWOC, which did not seem to be a problem.

Maybe there's another detail I'm missing here?

Additionally, in the PRs current state, the global httpClient object is being modified when calling setupHttpClients() and then being retrieved later, rather than being a return value of the setupHttpClients() function. I believe this is somewhat of an anti-pattern, as we are relying on the side-effect of setupHttpClients() rather than making it clear that setupHttpClients() modifies the httpClient.

@vinokurig @tolusha @ibuziuk I am open to either keeping the HTTP client initialization logic as part of the operator initialization, or moving it to the reconcile function as proposed in this PR, but would appreciate any input given the complications that this change brings.

vinokurig commented 1 month ago

As another possible solution I propose to just update certificates on Reconcile step instead of reinitialise the http client. @AObuchow @tolusha @ibuziuk WDYT?

AObuchow commented 1 month ago

As another possible solution I propose to just update certificates on Reconcile step instead of reinitialise the http client. @AObuchow @tolusha @ibuziuk WDYT?

This sounds like the best solution so far to me and should work, great idea :)

Something to be careful with here: If we retrieve the SSL certificates from the workspace's config (and not the global config), we need to Clone() the httpClient's transport object before updating the certificates, and use that for the rest of the workspace's reconcile function instead of updating and using the global httpClient object. The reason we should clone/copy the global httpClient instead of modifying it directly during a reconcile, is because it will be shared between concurrent reconciles . If workspace A and B both configure an external DWOC that reference different SSL certificates, then workspace A and B will also need their own HTTP clients to be configured with different certs.

Since the above might add some weird complexity for a case that might never happen (are users really going to need different certs for individual workspaces?...), I think it's fine to just use the global config when updating the SSL certificates (i.e. globalConfig := config.GetGlobalConfg())

ibuziuk commented 1 month ago

/ok-to-test

vinokurig commented 1 month ago

/retest

AObuchow commented 1 month ago

@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 :) ).

AObuchow commented 1 month ago

@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.

openshift-ci[bot] commented 1 month ago

[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: - ~~[OWNERS](https://github.com/devfile/devworkspace-operator/blob/main/OWNERS)~~ [AObuchow,dkwon17] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

openshift-ci[bot] commented 1 month ago

New changes are detected. LGTM label has been removed.

devfile / devworkspace-operator