search

kubevirt / containerized-data-importer

Data Import Service for kubernetes, designed with kubevirt in mind.
Apache License 2.0
395 stars 249 forks source link

Expose Upload Proxy certificate in CDI Config status #3314

Closed EduardGomezEscandell closed 2 weeks ago

EduardGomezEscandell commented 1 month ago

What this PR does / why we need it: As a VM owner I want to upload a VM disk to my cluster without needing to use an unsecured TLS connection. I want to import the upload proxy cert into my client.

Which issue(s) this PR fixes https://issues.redhat.com/browse/CNV-21214

Special notes for your reviewer: None of the config controller code is covered by tests, I tried to add test for watchUploadProxyCA but it was too much boilerplate and I didn't know what I was doing.

The reconciler is well tested (at least the happy paths) so I could add my own test easily.

Release note:

Expose the upload-proxy's certificate in the CDI config status
kubevirt-bot commented 1 month ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

EduardGomezEscandell commented 1 month ago

/test all

EduardGomezEscandell commented 1 month ago

/cc @awels

EduardGomezEscandell commented 1 month ago

/retest

EduardGomezEscandell commented 1 month ago

/hold Don't want to merge with expanded permissions

EduardGomezEscandell commented 1 month ago

/retest

EduardGomezEscandell commented 1 month ago

/unhold /retest

EduardGomezEscandell commented 1 month ago

/retest

EduardGomezEscandell commented 4 weeks ago

Seems like fossa is complaining about docker's licence? I did not add or update this dependency :man_shrugging:

EduardGomezEscandell commented 4 weeks ago

Anyways this is ready for review @awels

EduardGomezEscandell commented 3 weeks ago

/retest pull-cdi-goveralls

kubevirt-bot commented 3 weeks ago

@EduardGomezEscandell: The /retest command does not accept any targets. The following commands are available to trigger required jobs:

The following commands are available to trigger optional jobs:

Use /test all to run all jobs.

In response to [this](https://github.com/kubevirt/containerized-data-importer/pull/3314#issuecomment-2188790230): >/retest pull-cdi-goveralls 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-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
EduardGomezEscandell commented 3 weeks ago

/test pull-cdi-goveralls

awels commented 3 weeks ago

/test pull-cdi-goveralls

coveralls commented 3 weeks ago

Coverage Status

coverage: 59.083% (+0.01%) from 59.069% when pulling 71bca29669876e60e41a154e2a488652ec232091 on EduardGomezEscandell:uploadproxy-ca-certificate into b887d879436400b15da98e295e448b455a8ba76e on kubevirt:main.

EduardGomezEscandell commented 3 weeks ago

Looks pretty good to me, I am just missing the regular K8S Ingress. We do look up the Ingress certificate in VMExport.

We cover this implicitly I believe. The UploadProxyURL that we use is set right before in reconcileUploadProxyURL. It is set to the route if present, and otherwise the ingress.

awels commented 3 weeks ago

Right but the Ingress CA cert is in a different config map IIRC. I would have to look at the export code to see what I did there. I do remember the logic being quite different from what I did for Routes.

awels commented 3 weeks ago

I am talking about this bit of code in vmexport. The code assuming the secret exists in the namespace kubevirt is installed in. In this case it would be the namespace CDI is installed in.

awels commented 3 weeks ago

/test pull-containerized-data-importer-e2e-ceph

kubevirt-bot commented 3 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awels

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/kubevirt/containerized-data-importer/blob/main/OWNERS)~~ [awels] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
awels commented 2 weeks ago

/retest-required

akalenyu commented 2 weeks ago

/test pull-containerized-data-importer-e2e-nfs