[kubevirt] Enable VMExport feature #808
Conversation
|
Warning Rate limit exceeded@kvaps has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
""" WalkthroughThe changes update the NGINX Ingress Controller version from 1.4.0 to 1.5.0. A new parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Helm
participant Kubernetes
participant IngressController
participant virtExportProxyService
Helm->>Kubernetes: Process values.yaml with virtExportProxy parameter
Kubernetes->>Kubernetes: Render and apply vm-exportproxy.yaml
Kubernetes->>IngressController: Create Ingress resource for virt export proxy
IngressController->>virtExportProxyService: Route traffic based on Ingress rules
sequenceDiagram
participant User
participant KubeVirtAPI
participant VMExportModule
User->>KubeVirtAPI: Send VM export request
KubeVirtAPI->>VMExportModule: Check VMExport feature (feature gate enabled)
VMExportModule-->>User: Process and return export output
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
22be951 to
1cc4185
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/extra/ingress/README.md (1)
7-15: Addition of thevirtExportProxyParameter
The new table row introducing thevirtExportProxyparameter (defaulting tofalse) is clearly documented and correctly formatted. Ensure that downstream configuration and documentation are updated accordingly so that users understand this option’s purpose and default behavior.packages/extra/ingress/vm-exportproxy.yaml (1)
1-7: Helm Templating and Conditional Block Initialization
Lines 1–7 correctly initialize variables using Helm’s templating (e.g., fetching the ConfigMap and Namespace details) and use the conditional block to ensure that this Ingress resource is generated only when.Values.virtExportProxyis enabled.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/values.schema.json(1 hunks)packages/extra/ingress/values.yaml(1 hunks)packages/extra/ingress/vm-exportproxy.yaml(1 hunks)packages/extra/versions_map(1 hunks)packages/system/kubevirt/templates/kubevirt-cr.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/ingress/vm-exportproxy.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (7)
packages/extra/ingress/Chart.yaml (1)
6-6: Version bump to 1.5.0 aligns with versions_map updateThe version update from 1.4.0 to 1.5.0 is consistent with the changes made in the versions_map file and reflects the addition of the new VM export proxy functionality.
packages/system/kubevirt/templates/kubevirt-cr.yaml (1)
20-20: VMExport feature gate enables the VM export functionalityThe addition of the VMExport feature gate is crucial for enabling the VM export functionality that will be utilized by the new ingress configuration. This change aligns perfectly with the PR title.
packages/extra/ingress/values.yaml (1)
34-35: New virtExportProxy parameter for KubeVirt export proxyThe new parameter is properly documented with a comment following the same pattern as the existing
cdiUploadProxyparameter. Setting the default tofalsemaintains backward compatibility for existing deployments.packages/extra/ingress/values.schema.json (1)
38-42: Schema updated with virtExportProxy propertyThe schema has been correctly updated to include the new
virtExportProxyproperty with appropriate type, description, and default value, ensuring schema validation for the new parameter.packages/extra/versions_map (1)
19-20: Version map updated for ingress 1.4.0 and 1.5.0The version map has been properly updated to pin ingress 1.4.0 to a specific commit and add the new 1.5.0 version pointing to HEAD. This is a good practice for version management.
According to the AI summary, there should also be a new file
vm-exportproxy.yamlthat defines an Ingress resource for the virtual export proxy. I don't see this file in the review set, but it would be important to ensure it's included with proper conditional creation based on thevirtExportProxyparameter.packages/extra/ingress/vm-exportproxy.yaml (2)
8-17: Ingress Metadata and Conditional Annotations
The Ingress metadata is well defined, with annotations for HTTPS backend protocol and cert-manager integration. However, within the conditional branch starting at line 14, if the issuer type is"cloudflare", no annotation is emitted. Please verify whether this “empty” branch is intentional, or if it should output an alternative annotation specific to Cloudflare certificate management.
18-32: Ingress Spec and Routing Rules Configuration
The Ingress spec (lines 18–32) – including setting theingressClassName, defining the host rule for routing HTTP traffic to thevirt-exportproxyservice on port 443, and specifying the path configuration – follows standard practices. Confirm that using the release namespace as the ingress class is the desired behavior in your cluster.
| tls: | ||
| - hosts: | ||
| virt-exportproxy.{{ $host }} | ||
| secretName: virt-exportproxy-{{ .Release.Namespace }}-tls | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TLS Block Formatting and YAML List Structure
The TLS configuration block is intended to declare the host for TLS and the corresponding secret name. However, the hosts field appears to be missing the YAML list marker for its entry. To meet the standard Ingress YAML spec, the hosts value should be a list. Consider updating it as follows:
- - hosts:
- virt-exportproxy.{{ $host }}
+ - hosts:
+ - virt-exportproxy.{{ $host }}Also, note that the static analysis hint from YAMLlint regarding a syntax error on line 1 is likely a false positive caused by the Helm templating syntax. You might consider configuring YAMLlint to ignore these Helm template directives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tls: | |
| - hosts: | |
| virt-exportproxy.{{ $host }} | |
| secretName: virt-exportproxy-{{ .Release.Namespace }}-tls | |
| {{- end }} | |
| tls: | |
| - hosts: | |
| - virt-exportproxy.{{ $host }} | |
| secretName: virt-exportproxy-{{ .Release.Namespace }}-tls | |
| {{- end }} |
1cc4185 to
f357209
Compare
ef63b86 to
5259b59
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/extra/ingress/vm-exportproxy.yaml (1)
33-37: TLS Block Formatting and YAML List Structure
Thehostsfield undertlsmust be a list, but the current indentation is missing the list marker for the host entry.- tls: - - hosts: - virt-exportproxy.{{ $host }} - secretName: virt-exportproxy-{{ .Release.Namespace }}-tls + tls: + - hosts: + - virt-exportproxy.{{ $host }} + secretName: virt-exportproxy-{{ .Release.Namespace }}-tls
🧹 Nitpick comments (2)
hack/gen_versions_map.sh (2)
33-41: Expose Git errors for invalid commit checks
Removing the2>/dev/nullredirection makes underlyinggit showerrors (e.g., nonexistent commits or missing Chart.yaml) visible, which improves debuggability. However, withset -eactive, a failinggit showwill abort the script immediately—potentially before your custom “not valid” message is printed.
Consider explicitly capturing and handling the exit status ofgit showto provide a clearer, contextual error, for example:- if [ "$(git show "${commit}:./${chart}/Chart.yaml" | awk '$1 == "version:" {print $2}')" != "${version}" ]; then + # Fetch Chart.yaml contents or error out with a clear message + chart_data=$(git show "${commit}:./${chart}/Chart.yaml" 2>&1) || { + echo "Failed to retrieve Chart.yaml for $chart at commit $commit" >&2 + exit 1 + } + fetched_version=$(printf '%s' "$chart_data" | awk '$1 == "version:" {print $2}') + if [ "$fetched_version" != "${version}" ]; then echo "Commit $commit for $chart $version is not valid" >&2 exit 1 fi
46-51: Consolidate version‐extraction logic for tag lookup
You’ve also removed2>/dev/nullin the tag loop to surface lookup errors. To avoid code duplication and ensure uniform error handling, consider refactoring the version retrieval into a small helper function. For example:get_chart_version_at_ref() { ref=$1; chart=$2 git show "${ref}:./${chart}/Chart.yaml" 2>&1 \ | awk '$1 == "version:" {print $2}' } … for tag in $search_commits; do tag_version=$(get_chart_version_at_ref "$tag" "$chart") if [ "$tag_version" = "$version" ]; then found_tag=$(git rev-parse --short "$tag") break fi doneThis will centralize error visibility and make future adjustments simpler.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
hack/gen_versions_map.sh(2 hunks)packages/extra/ingress/Chart.yaml(1 hunks)packages/extra/ingress/README.md(1 hunks)packages/extra/ingress/values.schema.json(1 hunks)packages/extra/ingress/values.yaml(1 hunks)packages/extra/ingress/vm-exportproxy.yaml(1 hunks)packages/extra/versions_map(1 hunks)packages/system/kubevirt/templates/kubevirt-cr.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extra/ingress/values.schema.json
- packages/extra/versions_map
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/system/kubevirt/templates/kubevirt-cr.yaml
- packages/extra/ingress/Chart.yaml
- packages/extra/ingress/values.yaml
- packages/extra/ingress/README.md
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/extra/ingress/vm-exportproxy.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (1)
packages/extra/ingress/vm-exportproxy.yaml (1)
26-30:❓ Verification inconclusive
Verify backend service availability and port configuration.
The Ingress directs traffic to thevirt-exportproxyservice on port443over HTTPS. Please confirm that this service is deployed in thecozy-kubevirtnamespace and exposes TLS on port 443, otherwise the Ingress will be unable to route traffic.Run:
and verify
.spec.ports[]includes port 443 with the correcttargetPort.
🏁 Script executed:
kubectl get svc virt-exportproxy -n cozy-kubevirt -o yamlLength of output: 157
🏁 Script executed:
#!/bin/bash # Search for the virt-exportproxy service definition and its ports in the repository rg -n "virt-exportproxy" -t yaml -A5Length of output: 1916
Action Required: Confirm
virt-exportproxyService Definition and HTTPS Port
The Ingress inpackages/extra/ingress/vm-exportproxy.yaml(lines 26–30):- backend: service: name: virt-exportproxy port: number: 443routes traffic to a
virt-exportproxyService on port 443. No Service manifest forvirt-exportproxywas found in this repository, so please verify in thecozy-kubevirtnamespace that:
- A
virt-exportproxyService exists- Its
.spec.ports[]includesport: 443(and the correcttargetPort)- TLS is properly exposed on port 443
Run:
kubectl get svc virt-exportproxy -n cozy-kubevirt -o yamland confirm the configuration.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | ||
| {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure ConfigMap presence and safe default for clusterissuer.
The template uses lookup to fetch the cozystack ConfigMap and then indexes into .data.clusterissuer. If the ConfigMap is missing or the key isn’t defined, the chart will render with an error. Use Helm’s required to enforce the ConfigMap exists and apply default directly on the indexed value for clarity.
- {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }}
- {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }}
+ {{- $cozyConfig := required "ConfigMap 'cozystack' not found in namespace 'cozy-system'" (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }}
+ {{- $issuerType := default "http01" (index $cozyConfig.data "clusterissuer") }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} | |
| {{- $issuerType := (index $cozyConfig.data "clusterissuer") | default "http01" }} | |
| {{- $cozyConfig := required "ConfigMap 'cozystack' not found in namespace 'cozy-system'" (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }} | |
| {{- $issuerType := default "http01" (index $cozyConfig.data "clusterissuer") }} |
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
bfa22e4 to
6edffc0
Compare
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
6edffc0 to
0ddaff9
Compare
Summary by CodeRabbit