Repository navigation
[vpc] Migrate subnets definition from map to array format - #2052
Conversation
Change VPC subnets from map[string]Subnet to []Subnet with explicit
name field, aligning with the vm-instance subnet format.
Map format: subnets: {mysubnet: {cidr: "x"}}
Array format: subnets: [{name: mysubnet, cidr: "x"}]
Subnet ID generation (sha256 of namespace/vpcId/subnetName) remains
unchanged — subnetName now comes from .name field instead of map key.
ConfigMap output format stays the same.
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Andrei Kvapil <[email protected]>
Add migration script that converts VPC HelmRelease values from map format to array format. The script discovers all VirtualPrivateCloud HelmReleases, reads their values Secrets, and converts subnets using yq. Idempotent: skips if subnets are already an array or null. Bumps migration targetVersion from 30 to 31. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
📝 WalkthroughWalkthroughThis PR restructures VPC subnets from a map-based format (keyed by name) to an array-based format where each subnet has an explicit name field. It includes schema updates, template modifications, value examples, and a migration script to convert existing instances. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
Summary of ChangesHello Andrei Kvapil (@kvaps), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the definition of VPC subnets within the system, transitioning from a map-based configuration to a more structured array format. This change standardizes the subnet definition to include an explicit 'name' field, aligning it with existing 'vm-instance' conventions. To facilitate a smooth transition for existing deployments, an automated migration script has been implemented, which intelligently converts stored HelmRelease values secrets to the new format without affecting existing subnet IDs or resources. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dosubot
Bot
added
size/L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the VPC subnets definition from a map to an array of objects, which improves consistency with other parts of the system. The changes are applied consistently across Helm templates, value schemas, and default values. A migration script is included to handle existing installations, which is a great addition. I've reviewed the migration script and found a few areas for improvement to make it more robust. Overall, this is a well-thought-out change.
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | ||
| -o jsonpath="{.data.${VALUES_KEY}}" 2>/dev/null | base64 -d 2>/dev/null || true) | ||
| if [ -z "$VALUES_YAML" ]; then | ||
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | ||
| -o jsonpath="{.stringData.${VALUES_KEY}}" 2>/dev/null || true) | ||
| fi |
There was a problem hiding this comment.
This block for decoding values has two issues:
- The
jsonpath{.data.${VALUES_KEY}}will fail if$VALUES_KEYcontains a dot (e.g.,values.yaml). You should use bracket notation{.data['${VALUES_KEY}']}to handle such keys correctly. - The fallback to read from
stringDatawill not work.stringDatais a write-only field for creating/updating secrets.kubectl getwill always return the data in thedatafield, base64 encoded. This makes theifblock on lines 63-66 dead code.
I suggest simplifying this block to correctly handle the key and remove the unnecessary fallback.
VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \
-o jsonpath="{.data['${VALUES_KEY}']}" 2>/dev/null | base64 -d 2>/dev/null || true)
| ') | ||
|
|
||
| # --- Patch the Secret --- | ||
| NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64) |
There was a problem hiding this comment.
The echo command adds a trailing newline to its output, which will be included in the base64 encoded string. While this may not cause issues, it's cleaner to avoid it. Additionally, the base64 command can wrap long output into multiple lines. It's more robust to use printf to avoid the extra newline and add the -w 0 flag to base64 to disable wrapping.
NEW_VALUES_B64=$(printf "%s" "$NEW_VALUES" | base64 -w 0)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@packages/apps/vpc/templates/vpc.yaml`:
- Around line 19-20: The template now ranges over .Values.subnets (changing from
a map to an array) which allows duplicate .name entries and can cause $subnetId
collisions and duplicate ConfigMap keys; add a pre-range uniqueness guard that
tracks seen names (e.g., a dict keyed by .name), fails with a clear message like
"Duplicate subnet name: <name>" if a duplicate is detected, and only proceeds to
the existing range that computes $subnetId; alternatively document/validate
using a values schema (uniqueItems) for .Values.subnets to prevent duplicates
upstream.
In `@packages/core/platform/images/migrations/migrations/30`:
- Line 101: The NEW_VALUES_B64 assignment is creating wrapped base64 output
which injects newlines into the JSON passed to jq --arg; change the base64
invocation that produces NEW_VALUES_B64 to emit a single-line (no-wrap) output
(e.g., use base64 --wrap=0 or base64 -w 0) so NEW_VALUES_B64 contains one
continuous base64 string before it's passed into jq --arg; if portability to
macOS is required, use an alternative no-wrap command (e.g., openssl base64 -A)
in the same assignment that sets NEW_VALUES_B64.
- Around line 60-66: The jsonpath expressions used to read the Secret keys treat
dotted names like values.yaml as nested fields, so update the two kubectl -o
jsonpath calls that populate VALUES_YAML (the lines referencing VALUES_KEY,
VALUES_SECRET and NAMESPACE) to use JSONPath bracket/key access (or an
equivalent jq-based lookup) so dotted keys are treated as a single literal key
(i.e., read .data and .stringData via bracket notation using VALUES_KEY),
ensuring proper quoting/escaping of the variable.
🧹 Nitpick comments (1)
packages/core/platform/images/migrations/migrations/30 (1)
43-46: Redundantkubectlcalls — consolidate into a single invocation.Lines 43–44 and 45–46 each make a separate
kubectl get hrcall for the same HelmRelease. These could be combined into one call to avoid a race condition and reduce API load, especially with many instances.♻️ Suggested consolidation
- VALUES_SECRET=$(kubectl -n "$NAMESPACE" get hr "$HR_NAME" -o json | \ - jq -r '.spec.valuesFrom // [] | map(select(.kind == "Secret" and (.name | test("cozystack-values") | not))) | .[0].name // ""') - VALUES_KEY=$(kubectl -n "$NAMESPACE" get hr "$HR_NAME" -o json | \ - jq -r '.spec.valuesFrom // [] | map(select(.kind == "Secret" and (.name | test("cozystack-values") | not))) | .[0].valuesKey // "values.yaml"') + HR_JSON=$(kubectl -n "$NAMESPACE" get hr "$HR_NAME" -o json) + VALUES_SECRET=$(echo "$HR_JSON" | \ + jq -r '.spec.valuesFrom // [] | map(select(.kind == "Secret" and (.name | test("cozystack-values") | not))) | .[0].name // ""') + VALUES_KEY=$(echo "$HR_JSON" | \ + jq -r '.spec.valuesFrom // [] | map(select(.kind == "Secret" and (.name | test("cozystack-values") | not))) | .[0].valuesKey // "values.yaml"')
| {{- range .Values.subnets }} | ||
| {{- $subnetId := print "subnet-" (print $.Release.Namespace "/" $vpcId "/" .name | sha256sum | trunc 8) }} |
There was a problem hiding this comment.
Array iteration loses the inherent uniqueness guarantee that map keys provided.
With the old map format, subnet names were unique by definition (map keys). The new array format allows duplicate name entries, which would produce duplicate Kubernetes resource names ($subnetId collision) and duplicate ConfigMap keys, leading to deployment failures or silent data loss.
Consider adding a validation check or a uniqueItems-style constraint. A simple template-level duplicate check could be added at the top of the range:
💡 Example uniqueness guard (template-level)
You could add a pre-flight check before the range loop:
{{- $seen := dict }}
{{- range .Values.subnets }}
{{- if hasKey $seen .name }}
{{- fail (printf "Duplicate subnet name: %s" .name) }}
{{- end }}
{{- $_ := set $seen .name true }}
{{- end }}🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 20-20: syntax error: could not find expected ':'
(syntax)
🤖 Prompt for AI Agents
In `@packages/apps/vpc/templates/vpc.yaml` around lines 19 - 20, The template now
ranges over .Values.subnets (changing from a map to an array) which allows
duplicate .name entries and can cause $subnetId collisions and duplicate
ConfigMap keys; add a pre-range uniqueness guard that tracks seen names (e.g., a
dict keyed by .name), fails with a clear message like "Duplicate subnet name:
<name>" if a duplicate is detected, and only proceeds to the existing range that
computes $subnetId; alternatively document/validate using a values schema
(uniqueItems) for .Values.subnets to prevent duplicates upstream.
| # --- Decode current values --- | ||
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | ||
| -o jsonpath="{.data.${VALUES_KEY}}" 2>/dev/null | base64 -d 2>/dev/null || true) | ||
| if [ -z "$VALUES_YAML" ]; then | ||
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | ||
| -o jsonpath="{.stringData.${VALUES_KEY}}" 2>/dev/null || true) | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for jsonpath usage patterns in migration scripts and shell files
echo "=== Searching for jsonpath usage with .data pattern ==="
rg -n 'jsonpath.*\.data' --type=sh packages/
echo -e "\n=== Searching for jq usage patterns in the repository ==="
rg -n 'jq' --type=sh packages/ | head -20
echo -e "\n=== Checking the specific migration file ==="
cat -n packages/core/platform/images/migrations/migrations/30 | head -80Repository: cozystack/cozystack
Length of output: 6466
🌐 Web query:
kubectl jsonpath access secret key with dot notation Kubernetes
💡 Result:
In kubectl -o jsonpath=..., a . in a Secret key name is treated as a path separator, so you must escape literal dots in the key with \..
Examples:
# key name: tls.crt
kubectl get secret my-tls -o jsonpath='{.data.tls\.crt}' | base64 -d# key name: .dockercfg (leading dot)
kubectl get secret my-secret -o jsonpath='{.data.\.dockercfg}' | base64 -dIf the key contains dots and jsonpath quoting/indexing like {.data['db.conf']} doesn’t work in your kubectl version, use jq (reliable for dotted keys):
kubectl get secret my-secret -o json | jq -r '.data["db.conf"]' | base64 -dSources: escaping dots for Secret/ConfigMap keys in jsonpath [1][2][3], leading-dot key example [4], bracket/indexing limitations for dotted map keys [5].
Fix kubectl JSONPath access for Secret keys with dots in their names.
When VALUES_KEY is values.yaml (the default), the jsonpath expression {.data.values.yaml} is parsed as nested field access (.data → values → yaml), not as a literal key lookup, returning empty and silently skipping the migration for all instances.
Use jq with bracket key access to properly handle dotted key names:
🐛 Proposed fix
- VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \
- -o jsonpath="{.data.${VALUES_KEY}}" 2>/dev/null | base64 -d 2>/dev/null || true)
- if [ -z "$VALUES_YAML" ]; then
- VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \
- -o jsonpath="{.stringData.${VALUES_KEY}}" 2>/dev/null || true)
- fi
+ VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" -o json 2>/dev/null | \
+ jq -r --arg key "$VALUES_KEY" '.data[$key] // empty' | base64 -d 2>/dev/null || true)
+ if [ -z "$VALUES_YAML" ]; then
+ VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" -o json 2>/dev/null | \
+ jq -r --arg key "$VALUES_KEY" '.stringData[$key] // empty' 2>/dev/null || true)
+ fi📝 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.
| # --- Decode current values --- | |
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | |
| -o jsonpath="{.data.${VALUES_KEY}}" 2>/dev/null | base64 -d 2>/dev/null || true) | |
| if [ -z "$VALUES_YAML" ]; then | |
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" \ | |
| -o jsonpath="{.stringData.${VALUES_KEY}}" 2>/dev/null || true) | |
| fi | |
| # --- Decode current values --- | |
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" -o json 2>/dev/null | \ | |
| jq -r --arg key "$VALUES_KEY" '.data[$key] // empty' | base64 -d 2>/dev/null || true) | |
| if [ -z "$VALUES_YAML" ]; then | |
| VALUES_YAML=$(kubectl -n "$NAMESPACE" get secret "$VALUES_SECRET" -o json 2>/dev/null | \ | |
| jq -r --arg key "$VALUES_KEY" '.stringData[$key] // empty' 2>/dev/null || true) | |
| fi |
🤖 Prompt for AI Agents
In `@packages/core/platform/images/migrations/migrations/30` around lines 60 - 66,
The jsonpath expressions used to read the Secret keys treat dotted names like
values.yaml as nested fields, so update the two kubectl -o jsonpath calls that
populate VALUES_YAML (the lines referencing VALUES_KEY, VALUES_SECRET and
NAMESPACE) to use JSONPath bracket/key access (or an equivalent jq-based lookup)
so dotted keys are treated as a single literal key (i.e., read .data and
.stringData via bracket notation using VALUES_KEY), ensuring proper
quoting/escaping of the variable.
| ') | ||
|
|
||
| # --- Patch the Secret --- | ||
| NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64) |
There was a problem hiding this comment.
base64 without -w 0 may produce line-wrapped output, corrupting the Secret patch.
On GNU/Linux, base64 wraps output at 76 characters by default. The resulting multi-line string, when passed via jq --arg, embeds literal newlines into the JSON value. While some base64 decoders tolerate whitespace, Kubernetes API behavior here is fragile.
🐛 Proposed fix
- NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64)
+ NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64 -w 0)📝 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.
| NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64) | |
| NEW_VALUES_B64=$(echo "$NEW_VALUES" | base64 -w 0) |
🤖 Prompt for AI Agents
In `@packages/core/platform/images/migrations/migrations/30` at line 101, The
NEW_VALUES_B64 assignment is creating wrapped base64 output which injects
newlines into the JSON passed to jq --arg; change the base64 invocation that
produces NEW_VALUES_B64 to emit a single-line (no-wrap) output (e.g., use base64
--wrap=0 or base64 -w 0) so NEW_VALUES_B64 contains one continuous base64 string
before it's passed into jq --arg; if portability to macOS is required, use an
alternative no-wrap command (e.g., openssl base64 -A) in the same assignment
that sets NEW_VALUES_B64.
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `packages/apps/vpc/Makefile` is the only app Makefile that calls `cozyvalues-gen` without `-r README.md`, and it has been that way since the app was added, so the README's Parameters table was hand-written and never regenerated. It still documents `subnets` as `map[string]object` and omits `peers` and `routes`, and the README example uses the map form, which `values.schema.json` rejects since #2052 made `subnets` a list. That README is also the source of the website's VPC reference page, so the published example is the invalid one. This adds `-r README.md`, regenerates the table with cozyvalues-gen v1.7.0 (the version CI uses), and rewrites the example as a list. `make generate` in `packages/apps/vpc` leaves the CRD and Go types unchanged, and the new example validates against the VPC `openAPISchema`. The draft #4813 edits this `values.yaml`, so once this lands its `make generate` will update the README as well. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: The website's VPC page is regenerated from this README by the release bot, so it needs no separate change. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note docs(vpc): document subnets, peers and routes in the VPC README as the list-based parameters the chart accepts. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated the VPC configuration guide to describe subnet entries, including names, CIDR ranges, and optional allowed source ranges. * Added guidance for configuring VPC peers with a remote VPC name and tenant namespace, and routes with a destination range and next-hop IP. * Revised the subnet example to use a list of named entries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Migrates VPC subnets definition from map format (
map[string]Subnet) to array format ([]Subnet) with an explicitnamefield. This aligns VPC subnet definitions with the vm-instance format.Before:
After:
Subnet ID generation remains unchanged — the sha256 input (
namespace/vpcId/subnetName) is the same, so existing resources are not affected.Includes a migration script (migration 30) that automatically converts existing VPC HelmRelease values Secrets from map to array format. The migration is idempotent and skips subnets that are already arrays or null.
Release note
Summary by CodeRabbit