CNV-79324: TLS certificate for HTTP URL source #3445
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: upalatucci 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 |
upalatucci
changed the title
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
...tableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateSection.tsx
Outdated
Show resolved
Hide resolved
src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/HTTPSource.tsx
Outdated
Show resolved
Hide resolved
| const { registryCredentials, registryURL } = bootableVolume; | ||
| const { password, username } = registryCredentials || {}; | ||
|
|
||
| const credentialsValid = (username && password) || (!username && !password); |
There was a problem hiding this comment.
I think the type of credentialsValid should be a boolean, therefor its name should be
areCredentialsValid
We can use areCredentialsValid = !username === !password
There was a problem hiding this comment.
i don't like !username === !password as its difficult to read and understand but i'll do something different
upalatucci
force-pushed
the
feature/add-bootable-volume-tls-certificate
branch
from
c570648 to
88588a4
Compare
📝 WalkthroughWalkthroughAdds TLS certificate support to AddBootableVolumeModal: new UI components for selecting/uploading certificates, new hooks for validation and fetching TLS ConfigMaps, constants and utils to create/resolve TLS ConfigMaps, and localization strings across multiple locales. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateExisting.tsx`:
- Line 57: The FormGroup label in TlsCertificateExisting.tsx currently uses the
inconsistent translation key t('TLS certification'); update it to the same term
used in TlsCertificateSection by changing the label to t('TLS certificate') (or
the shared translation key used by TlsCertificateSection) so the displayed label
and i18n key match across the TlsCertificate components; locate the FormGroup
instance in TlsCertificateExisting and replace the translation key accordingly.
- Line 29: The component TlsCertificateExisting currently ignores the third
return value from the useTlsCertConfigMaps hook; update the
TlsCertificateExisting component to capture the error (e.g. const
[tlsCertConfigMaps, certMapsLoaded, certMapsError] = useTlsCertConfigMaps(...))
and handle it by either logging (console.error or processLogger) and/or
rendering user-facing feedback (an inline Alert, helper text near the dropdown,
or a disabled dropdown with an error message) so users aren’t shown a silent
empty dropdown when the watch fails; ensure you reference tlsCertConfigMaps,
certMapsLoaded and certMapsError when deciding to show the normal dropdown vs
the error UI.
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateSection.tsx`:
- Around line 88-93: The "Add new" radio becomes selected when tlsCertSource is
undefined because useExisting is computed from tlsCertSource; to make "Use
existing" the default initialize tlsCertSource to TLS_CERT_SOURCE_EXISTING or
set it when tlsCertificateRequired is toggled on: update the component state
initialization or the handler that toggles tlsCertificateRequired to assign
tlsCertSource = TLS_CERT_SOURCE_EXISTING (or call the setter used for
tlsCertSource) so useExisting is true by default; relevant symbols:
tlsCertSource, useExisting, tlsCertificateRequired, TLS_CERT_SOURCE_EXISTING,
and the radio with id "tls-add-new".
- Around line 46-49: The UI uses inconsistent wording: the FormGroup label prop
in TlsCertificateSection is "TLS certification" but the checkbox (inside the
same component) reads "TLS certificate required"; update the FormGroup label to
use the standard term "TLS certificate" to match the checkbox and ensure
consistent terminology across the component (look for the FormGroup with
fieldId="tls-certification" and the checkbox label in TlsCertificateSection and
change the label prop accordingly).
In `@src/utils/components/AddBootableVolumeModal/utils/utils.ts`:
- Around line 240-277: getOrCreateTlsCertConfigMapName can create a new TLS
ConfigMap via createTlsCertConfigMap but if subsequent resource creation in
createHTTPDataSource (DataSource/DataVolume) fails the new ConfigMap is
orphaned; update createHTTPDataSource's error handling to track the ConfigMap
name returned by getOrCreateTlsCertConfigMapName and delete that ConfigMap in
the catch/failure path (call the inverse cleanup for createTlsCertConfigMap
using the same cluster/targetNamespace/name), or alternatively ensure
createHTTPDataSource documents that callers must clean up the ConfigMap;
reference getOrCreateTlsCertConfigMapName, createTlsCertConfigMap, and
createHTTPDataSource when making the change.
🧹 Nitpick comments (5)
locales/en/plugin__kubevirt-plugin.json (2)
676-677: Avoid near-duplicate i18n keys that only differ by trailing space.
There are now two keys for the same sentence (one with a trailing space). This increases translation churn and risks inconsistent usage. Please standardize on a single key and update call sites accordingly.
1489-1489: Use “certificate” consistently (avoid “certification”).
“TLS certification” and “Select TLS certification” read like typos given the adjacent “TLS certificate” strings. Align these to “certificate” and reuse existing keys where possible to avoid duplicate terminology.💡 Suggested cleanup (update call sites + other locales accordingly)
- "Select TLS certification": "Select TLS certification", + "Select TLS certificate": "Select TLS certificate", @@ - "TLS certification": "TLS certification",Also applies to: 1759-1761
src/utils/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx (1)
85-85: WrapresetDiskSizeinuseCallback.This arrow function is recreated every render and passed as a prop to
<SourceTypeSelection>(Line 125), which may trigger unnecessary re-renders. As per coding guidelines, use React's memoization tools to avoid unnecessary re-renders.Proposed fix
- const resetDiskSize = () => setBootableVolumeField('size')(initialBootableVolumeState.size); + const resetDiskSize = useCallback( + () => setBootableVolumeField('size')(initialBootableVolumeState.size), + [setBootableVolumeField], + );src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/HTTPSource.tsx (1)
22-23: ExtracthttpSourceHelperURLas a module-level constant.Per coding guidelines, define constants in utility files with uppercase and underscore-separated naming. This string is also recreated on every render unnecessarily.
Proposed fix
+const HTTP_SOURCE_HELPER_URL = + 'https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-9-latest.x86_64.qcow2'; + const HTTPSource: FC<HTTPSourceProps> = ({ bootableVolume, setBootableVolumeField }) => { const { t } = useKubevirtTranslation(); - - const httpSourceHelperURL = - 'https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-9-latest.x86_64.qcow2';Then update the references on Lines 37–38 to use
HTTP_SOURCE_HELPER_URL.src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateExisting.tsx (1)
59-63:getName(cm)is called twice per item — extract to a local variable.Proposed fix
- options={tlsCertConfigMaps?.map((cm) => ({ - children: getName(cm), - value: getName(cm), - }))} + options={tlsCertConfigMaps?.map((cm) => { + const name = getName(cm); + return { children: name, value: name }; + })}
...ableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateExisting.tsx
Outdated
Show resolved
Hide resolved
...ableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateExisting.tsx
Outdated
Show resolved
Hide resolved
...tableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateSection.tsx
Outdated
Show resolved
Hide resolved
...tableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateSection.tsx
Show resolved
Hide resolved
|
/retest |
1 similar comment
|
/retest |
| const cluster = bootableVolume?.bootableVolumeCluster; | ||
| const tlsCertProject = bootableVolume?.tlsCertProject || bootableVolume?.bootableVolumeNamespace; | ||
| const [projectNames, projectsLoaded] = useProjects(cluster); | ||
| const [tlsCertConfigMaps, certMapsLoaded] = useTlsCertConfigMaps(tlsCertProject, cluster); |
There was a problem hiding this comment.
Do we want to use error from useTlsCertConfigMaps?
| if (!tlsRequired) return undefined; | ||
|
|
||
| const useExisting = bootableVolume?.tlsCertSource === TLS_CERT_SOURCE_EXISTING; | ||
| const tlsCertProject = bootableVolume?.tlsCertProject; |
There was a problem hiding this comment.
You can use the tlsCertProject const (const tlsCertProject = bootableVolume?.[tlsCertProject]
| value: name, | ||
| }))} | ||
| setSelected={(name: string) => { | ||
| setBootableVolumeField('tlsCertProject')(name); |
There was a problem hiding this comment.
You can use the consts you created for tlsCertProject and tlsCertConfigMapName
|
|
||
| const useExisting = bootableVolume?.tlsCertSource === TLS_CERT_SOURCE_EXISTING; | ||
| const tlsCertProject = bootableVolume?.tlsCertProject; | ||
| const tlsCertConfigMapName = bootableVolume?.tlsCertConfigMapName?.trim(); |
There was a problem hiding this comment.
Same here [tlsCertConfigMapName]
| if (!bootableVolume?.tlsCertProject && targetNamespace) { | ||
| setBootableVolumeField(TLS_CERT_FIELD_NAMES.tlsCertProject)(targetNamespace); | ||
| } | ||
| }, [targetNamespace, bootableVolume?.tlsCertProject, setBootableVolumeField]); |
There was a problem hiding this comment.
bootableVolume?.[tlsCertProject]
| <FlexItem> | ||
| <Radio | ||
| onChange={() => { | ||
| setBootableVolumeField(TLS_CERT_FIELD_NAMES.tlsCertSource)( |
There was a problem hiding this comment.
I thing it would be cleaner to destruct tlsCertificate, tlsCertConfigMapName and tlsCertSource in the beginning of the code.
- Use existing: Project + TLS certification (ConfigMap) dropdowns, horizontal layout - Add new: FileUpload for certificate (upload file or paste PEM) Co-authored-by: Cursor <[email protected]>
upalatucci
force-pushed
the
feature/add-bootable-volume-tls-certificate
branch
from
88588a4 to
2033eef
Compare
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@upalatucci: This pull request references CNV-79324 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.22." or "openshift-4.22.", but it targets "CNV v4.22.0" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateNew.tsx`:
- Around line 29-36: The onFileInputChange async callback passed to FileUpload
can reject if file.text() throws, causing an unhandled promise rejection; update
the callback in TlsCertificateNew so you await file.text() inside a try/catch
(not just finally), catch the error, clear or reset the TLS field via
setBootableVolumeField('tlsCertificate')('') and call the existing
error/notification handler (or set an error state) and still ensure
setIsCertLoading(false) runs in finally; reference the onFileInputChange
callback, setIsCertLoading, setBootableVolumeField('tlsCertificate') and
FileUpload when making the change.
In `@src/utils/components/AddBootableVolumeModal/utils/utils.ts`:
- Around line 257-267: The current code silently returns undefined when the
fetched ConfigMap (obtained via kubevirtK8sGet) does not contain
TLS_CERT_CONFIGMAP_KEY, which allows a DataVolume to be created without TLS;
update the function to throw a clear, descriptive error (or at minimum log a
warning and throw) instead of returning undefined, including the config map name
(tlsCertConfigMapName), namespace (tlsCertProject) and the missing key in the
message, and only call createTlsCertConfigMap when certData exists; ensure
callers handle or propagate the thrown error rather than assuming a
possibly-undefined newName.
🧹 Nitpick comments (2)
src/utils/components/AddBootableVolumeModal/utils/utils.ts (1)
322-334: Cleanup deletes are fire-and-forget — failed cleanup is silently swallowed.Both
kubevirtK8sDeletecalls (lines 323 and 329) are notawaited. If either fails, the error is lost and orphaned resources remain without any signal. The DataSource delete was pre-existing, but the new ConfigMap delete follows the same pattern.This is consistent with the existing code style, so flagging as optional — consider
await-ing and wrapping in a try/catch with a logged warning for observability.src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/HTTPSource.tsx (1)
22-23: Consider extractinghttpSourceHelperURLas a module-level constant.This string is recreated on every render. Per coding guidelines, define constants with uppercase naming outside the component (or in a constants file).
+const HTTP_SOURCE_HELPER_URL = + 'https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-9-latest.x86_64.qcow2'; + const HTTPSource: FC<HTTPSourceProps> = ({ bootableVolume, setBootableVolumeField }) => { const { t } = useKubevirtTranslation(); - - const httpSourceHelperURL = - 'https://cloud.centos.org/centos/9-stream/x86_64/images/CentOS-Stream-GenericCloud-9-latest.x86_64.qcow2';As per coding guidelines: "Define constants in utility files with uppercase and underscore-separated naming."
| onFileInputChange={async (_, file: File) => { | ||
| setIsCertLoading(true); | ||
| try { | ||
| const text = await file.text(); | ||
| setBootableVolumeField('tlsCertificate')(text); | ||
| } finally { | ||
| setIsCertLoading(false); | ||
| } |
There was a problem hiding this comment.
Unhandled rejection if file.text() fails.
The async callback is invoked by PatternFly's FileUpload, which likely does not await it. If file.text() throws, the error propagates as an unhandled promise rejection. The finally block handles the loading state, but the error itself is lost.
Consider adding a catch to handle the error gracefully (e.g., clear the field or show a notification).
Suggested change
onFileInputChange={async (_, file: File) => {
setIsCertLoading(true);
try {
const text = await file.text();
setBootableVolumeField('tlsCertificate')(text);
+ } catch {
+ setBootableVolumeField('tlsCertificate')('');
} finally {
setIsCertLoading(false);
}
}}🤖 Prompt for AI Agents
In
`@src/utils/components/AddBootableVolumeModal/components/VolumeSource/components/TlsCertificate/TlsCertificateNew.tsx`
around lines 29 - 36, The onFileInputChange async callback passed to FileUpload
can reject if file.text() throws, causing an unhandled promise rejection; update
the callback in TlsCertificateNew so you await file.text() inside a try/catch
(not just finally), catch the error, clear or reset the TLS field via
setBootableVolumeField('tlsCertificate')('') and call the existing
error/notification handler (or set an error state) and still ensure
setIsCertLoading(false) runs in finally; reference the onFileInputChange
callback, setIsCertLoading, setBootableVolumeField('tlsCertificate') and
FileUpload when making the change.
| const sourceConfigMap = await kubevirtK8sGet<IoK8sApiCoreV1ConfigMap>({ | ||
| cluster, | ||
| model: ConfigMapModel, | ||
| name: tlsCertConfigMapName, | ||
| ns: tlsCertProject, | ||
| }); | ||
| const certData = sourceConfigMap?.data?.[TLS_CERT_CONFIGMAP_KEY]; | ||
| if (!certData) return undefined; | ||
| const newName = `tls-cert-${getRandomChars()}`; | ||
| await createTlsCertConfigMap(cluster, targetNamespace, newName, certData); | ||
| return newName; |
There was a problem hiding this comment.
Silent undefined return when source ConfigMap lacks the expected key.
Line 264: if the user selects an existing ConfigMap but it doesn't contain the ca.pem key, the function silently returns undefined and the DataVolume is created without TLS — no error is surfaced. This could confuse users who believe they configured a certificate.
Consider throwing an error (or at least logging a warning) so the user knows the selected ConfigMap is missing the expected key.
Suggested change
const certData = sourceConfigMap?.data?.[TLS_CERT_CONFIGMAP_KEY];
- if (!certData) return undefined;
+ if (!certData) {
+ throw new Error(
+ `ConfigMap "${tlsCertConfigMapName}" in namespace "${tlsCertProject}" does not contain key "${TLS_CERT_CONFIGMAP_KEY}"`,
+ );
+ }🤖 Prompt for AI Agents
In `@src/utils/components/AddBootableVolumeModal/utils/utils.ts` around lines 257
- 267, The current code silently returns undefined when the fetched ConfigMap
(obtained via kubevirtK8sGet) does not contain TLS_CERT_CONFIGMAP_KEY, which
allows a DataVolume to be created without TLS; update the function to throw a
clear, descriptive error (or at minimum log a warning and throw) instead of
returning undefined, including the config map name (tlsCertConfigMapName),
namespace (tlsCertProject) and the missing key in the message, and only call
createTlsCertConfigMap when certData exists; ensure callers handle or propagate
the thrown error rather than assuming a possibly-undefined newName.
3 participants
Summary
Adds optional TLS certificate support when creating a bootable volume from an HTTP URL for HTTPS sources with a custom or self-signed CA.
Changes
useTlsCertConfigMapsis now surfaced to the user via an inline alert instead of showing an empty dropdown.TlsCertificateSectionfor readability. UsedTLS_CERT_FIELD_NAMESconsts insetBootableVolumeFieldcalls. ResettlsCertSourceto "Use existing" when the TLS checkbox is toggled on.Jira: https://issues.redhat.com/browse/CNV-79324
Demo
Screen.Recording.2026-02-09.at.18.02.58.mov
Screen.Recording.2026-02-09.at.18.06.54.mov
Summary by CodeRabbit
New Features
Localization