Repository navigation
[lineage, controller] Implement name selectors - #1477
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces secret-level label selectors with resource-level selectors that add optional templated exact-name matching; updates API types, deepcopy, CRD schema, webhook matcher logic, many cozyrds templates/manifests, centralized tenantresource constants, some image tags, and codegen output path. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CRD as CRD Spec (Resources)
participant Webhook as Lineage Webhook
participant Matcher as Selector Matcher
participant Template as Go Template Engine
participant K8s as Label selector engine
Note over CRD: include[] / exclude[] of ResourceSelector\n- LabelSelector\n- resourceNames[] (optional, templated)
Webhook->>Webhook: Build template context {kind, name, namespace}
Webhook->>Matcher: matchResourceToExcludeInclude(ctx, name, tmplCtx, labels, exclude, include)
par Evaluate Exclude
loop each sel in exclude
Matcher->>K8s: Evaluate LabelSelector => labelMatch
alt sel.resourceNames present
Matcher->>Template: Render each resourceName with tmplCtx
Template-->>Matcher: renderedNames
Matcher->>Matcher: nameMatch = name in renderedNames
else
Matcher->>Matcher: nameMatch = true
end
Matcher-->>Webhook: if labelMatch && nameMatch => excluded
end
and Evaluate Include
loop each sel in include
Matcher->>K8s: Evaluate LabelSelector => labelMatch
alt sel.resourceNames present
Matcher->>Template: Render resourceNames with tmplCtx
Template-->>Matcher: renderedNames
Matcher->>Matcher: nameMatch = name in renderedNames
else
Matcher->>Matcher: nameMatch = true
end
Matcher-->>Webhook: if labelMatch && nameMatch => included
end
end
alt Any exclude matched
Webhook-->>Webhook: Mark object excluded
else Any include matched
Webhook-->>Webhook: Mark object eligible
else
Webhook-->>Webhook: Not eligible
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 Timofei Larkin (@lllamnyp), 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 significantly enhances the flexibility of Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces name-based selectors for resources, complementing the existing label-based selectors. This is a valuable feature that allows for more flexible resource selection, especially for resources with predictable names but unpredictable labels. The implementation looks solid, with updated API types, CRD definitions, and matching logic. I have a couple of suggestions for the matcher.go file to improve performance and error handling, which are important for a webhook component.
| } | ||
|
|
||
| for _, templateStr := range resourceNames { | ||
| tmpl, err := template.New("resourceName").Parse(templateStr) |
There was a problem hiding this comment.
Parsing templates with template.New().Parse() inside a loop is inefficient and can become a performance bottleneck in a high-traffic webhook. Since these templates come from CozystackResourceDefinition CRDs, which don't change frequently, they can be parsed once and cached.
I recommend pre-parsing the templates when the CozystackResourceDefinition is loaded or updated in the controller's Reconcile loop (internal/lineagecontrollerwebhook/controller.go) and storing the parsed *template.Template objects in your runtimeConfig. You can then pass these pre-parsed templates to the matching functions to avoid re-parsing on every call.
6782e98 to
8c60523
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/lineagecontrollerwebhook/matcher.go (2)
15-40: Template parsing performance and error handling concerns.The
matchNamefunction logic is correct: it returnstrueif any template matches the name, or ifresourceNamesis nil (match-all behavior). However, there are two issues already raised in previous reviews:
- Performance: Parsing templates inside the loop on every call is inefficient for a webhook hot path.
- Error handling: Template parse and execution errors are silently ignored, making debugging difficult for users with malformed templates.
These issues were previously flagged and should be addressed by:
- Pre-parsing templates during CRD reconciliation and caching them
- Logging errors with context for debugging
42-51: AND logic correctly implemented, but error handling needs improvement.The function correctly implements the documented behavior: a resource matches only if BOTH label conditions AND name conditions are satisfied. However, the error from
metav1.LabelSelectorAsSelector()on line 44 is silently ignored, which was previously flagged.An invalid label selector in the CRD will cause matching to fail silently. This error should be logged with context for debugging.
🧹 Nitpick comments (11)
packages/core/installer/values.yaml (1)
2-2: Use explicit semantic version tag for installer image The installer image is referenced with thelatesttag alongside a digest. Even though the digest pins the image, usinglatestcan cause confusion—replace it with a specific semantic version tag to improve traceability.packages/system/cozystack-controller/values.yaml (1)
5-5: Consider traceability implications of "latest" version string.The
cozystackVersionfield is now set to"latest"rather than a specific semantic version. If this value is used in logs, metrics, or user-facing output, it may reduce traceability and make debugging or auditing more difficult.Consider maintaining a specific version string (e.g., git commit SHA or semantic version) for better observability, even if the image tag remains
latest.packages/system/kubeovn-plunger/values.yaml (1)
3-3: Separate the image bump into its own PR
The change updates the plunger image from a pinned pre-release tolatest@sha256:…, which isn’t related to the name-selector feature. Move this image version upgrade into a dedicated PR and use an explicit version tag (e.g. vX.Y.Z) instead oflatestfor clearer traceability.packages/system/dashboard/values.yaml (1)
2-6: Separate image version bumps from name selector feature
The PR replaces explicitv0.37.0-alpha.1@sha256:…tags withlatest@sha256:…for openapi-ui, openapi-ui-k8s-bff, and token-proxy inpackages/system/dashboard/values.yaml, which increases deployment risk and obscures version history.
• Move these image updates into a standalone PR.
• Preserve explicit version tags for production deployments.
• Iflatestimages are required, document the necessity in the PR description.packages/system/kubeovn/values.yaml (1)
67-67: Separate image updates from feature changesUpdating the kube-ovn image tag from
v1.14.5@…tolatest@…in the same PR as the name-selector feature mixes unrelated concerns.
- Move all image-version bumps into a dedicated PR.
- Use explicit semantic version tags rather than
latestfor core components.packages/apps/http-cache/images/nginx-cache.tag (1)
1-1: Separate image version bumps into a dedicated PR
This change updates 11 image tags across the repo (e.g.packages/apps/http-cache/images/nginx-cache.tag), mixing infrastructure version bumps with the name selector feature. Please move all image updates into a standalone PR to keep feature work and version upgrades distinct.packages/system/objectstorage-controller/values.yaml (1)
3-3: Consider the implications of using the "latest" tag.While the SHA256 digest ensures immutability for this deployment, the "latest" tag can create confusion during debugging and may complicate rollbacks. Consider whether a semantic version tag would be more appropriate for production deployments.
packages/apps/kubernetes/images/ubuntu-container-disk.tag (1)
1-1: Consider the implications of using the "latest" tag.The move from a versioned tag (v1.32) to "latest" reduces traceability. While the SHA256 digest provides immutability, the "latest" tag can complicate debugging and version tracking.
packages/system/kamaji/values.yaml (1)
6-6: Consider the implications of using the "latest" tag.The transition from a versioned tag (v0.37.0-alpha.1) to "latest" may complicate version tracking and rollbacks, despite the SHA256 digest providing immutability.
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
98-125: LGTM – template rendering is validated Verified thatmatchNameparses and executes each resource name template and gracefully skips malformed templates without panicking (internal/lineagecontrollerwebhook/matcher.go:21–24; 28–32). Consider replacing the TODO comments with actual warning logs.packages/system/kubeovn-webhook/values.yaml (1)
3-3: Align image tag strategy across charts; consider using semantic version tagsI see multiple charts under
packages/system(seaweedfs, objectstorage-controller, kubevirt-csi-node, kubeovn-plunger, dashboard, cozystack-api, etc.) usinglatest@sha256. While the digest immutably pins the image, pairing the digest with an explicit semantic version tag across all charts would improve traceability and reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)hack/update-codegen.sh(1 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/clickhouse/images/clickhouse-backup.tag(1 hunks)packages/apps/http-cache/images/nginx-cache.tag(1 hunks)packages/apps/kubernetes/images/cluster-autoscaler.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-csi-driver.tag(1 hunks)packages/apps/kubernetes/images/ubuntu-container-disk.tag(1 hunks)packages/apps/mysql/images/mariadb-backup.tag(1 hunks)packages/core/installer/values.yaml(1 hunks)packages/core/testing/values.yaml(1 hunks)packages/extra/bootbox/images/matchbox.tag(1 hunks)packages/extra/monitoring/images/grafana.tag(1 hunks)packages/extra/seaweedfs/images/objectstorage-sidecar.tag(1 hunks)packages/system/bucket/images/s3manager.tag(1 hunks)packages/system/cilium/values.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/monitoring.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml(1 hunks)packages/system/cozystack-api/values.yaml(1 hunks)packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml(3 hunks)packages/system/cozystack-controller/templates/crds/crds.yaml(1 hunks)packages/system/cozystack-controller/values.yaml(1 hunks)packages/system/dashboard/templates/configmap.yaml(1 hunks)packages/system/dashboard/values.yaml(1 hunks)packages/system/kamaji/values.yaml(2 hunks)packages/system/kubeovn-plunger/values.yaml(1 hunks)packages/system/kubeovn-webhook/values.yaml(1 hunks)packages/system/kubeovn/values.yaml(1 hunks)packages/system/kubevirt-csi-node/values.yaml(1 hunks)packages/system/metallb/values.yaml(1 hunks)packages/system/objectstorage-controller/values.yaml(1 hunks)packages/system/seaweedfs/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/system/cozystack-api/cozyrds/tenant.yaml
- packages/system/cozystack-api/cozyrds/redis.yaml
- packages/system/seaweedfs/values.yaml
- packages/system/cozystack-api/cozyrds/info.yaml
- packages/apps/kubernetes/images/kubevirt-cloud-provider.tag
- packages/extra/seaweedfs/images/objectstorage-sidecar.tag
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/lineagecontrollerwebhook/webhook.go
🧰 Additional context used
🧬 Code graph analysis (2)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (2)
CozystackResourceDefinitionResourceSelector(119-125)CozystackResourceDefinitionResources(127-137)
internal/lineagecontrollerwebhook/matcher.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionResourceSelector(119-125)
🪛 YAMLlint (1.37.1)
packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-controller/templates/crds/crds.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (45)
packages/apps/clickhouse/images/clickhouse-backup.tag (1)
1-1: Image tag updated tolatestwith digest pinning.The image reference has been updated from version
0.0.0tolatest. While usinglatesttags can introduce non-deterministic behavior, the inclusion of the SHA256 digest ensures that the exact image version is pinned, maintaining build reproducibility.packages/apps/mysql/images/mariadb-backup.tag (1)
1-1: Clarify the relationship of this change to the PR objectives.This file updates the image tag from
0.0.0tolatestwhile keeping the digest pinned. The AI summary mentions this is part of a broader pattern of tag updates across the repository. However, the PR objectives focus on implementing name selectors forCozystackResourceDefinitions.spec.secrets.Can you confirm whether this image tag change is intentionally bundled with the name selectors feature, or if it should be separated into a different PR?
packages/system/metallb/values.yaml (1)
7-7: LGTM! Image digests updated correctly.The image tag updates for both metallb-controller and metallb-speaker follow the correct format (version@sha256:digest) and are syntactically valid.
Note: These metallb image updates appear unrelated to the PR's stated objective of implementing name selectors for CozystackResourceDefinitions.spec.secrets. They may be incidental updates bundled with the main feature work.
Also applies to: 11-11
packages/system/bucket/images/s3manager.tag (1)
1-1: Clarifylatesttag usage in s3manager.tagThe file
packages/system/bucket/images/s3manager.tagwas updated from a fixedv0.5.0@sha256:…tolatest@sha256:…. Relying on thelatesttag undermines build reproducibility, complicates rollbacks, and falls outside this PR’s scope of implementing name selectors for CozystackResourceDefinitions. Please confirm:
- Should this image update be part of this PR or extracted into its own?
- What’s the rationale for switching to
latestrather than pinning a specific version?packages/extra/monitoring/images/grafana.tag (1)
1-1: Pinning image by digest overrides the tag
The referenceghcr.io/cozystack/cozystack/grafana:latest@sha256:dc27b9d7ef42e3d59667a203cb94d410c60afc119102ff1b9a081af4ea72f52cuses the digest for resolution, rendering:latestcosmetic. To track the moving “latest” image, remove the digest; to ensure stability, drop:latestand use a specific version tag.packages/system/cilium/values.yaml (1)
17-18: Clarify image specification: digest overrides tag
- Remove
tag: latestwhen pinning by digest.- Remove the pinned
digestif you intend to followlatest(not recommended for production).- When bumping versions, update both
taganddigesttogether.packages/system/cozystack-api/values.yaml (1)
2-2: Use semantic version tags for cozystack-api imageThe cozystack-api image is specified as
ghcr.io/cozystack/cozystack/cozystack-api:latest@sha256:490806568820c45d2c0ae3d2fcc349dd65ac20a4500ca86ee5094e4e3f4e24fd;
because the digest pins the exact image, thelatesttag no longer provides automatic updates. For clearer release tracking, switch to an explicit semantic version tag (e.g.vX.Y.Z@sha256:…) alongside the digest.packages/apps/kubernetes/images/cluster-autoscaler.tag (1)
1-1: Cluster autoscaler image uses “latest” tag with digest. The 4 image references in packages/apps/kubernetes/images/*.tag pin a digest to:latest, which can obscure the actual version; for operational clarity, use explicit version tags.
Image tag updates unrelated to PR objectives. These 4.tagfiles only update image references, yet this PR implements name-based selectors; move these tag updates to a separate PR, document their testing purpose, or revert if accidental.packages/system/dashboard/templates/configmap.yaml (1)
3-3: Verify if "latest" is appropriate for user-facing version text.The tenant text default has been changed from a specific version string (
v0.37.0-alpha.1) to"latest". While this aligns with the broader image tag updates across the PR, consider whether end users benefit more from seeing a specific version number versus the generic "latest" string for branding/informational purposes.packages/system/cozystack-controller/values.yaml (1)
2-2: Image digest is pinned despite "latest" tag.The image reference uses the
latesttag but includes a sha256 digest, ensuring immutability. This approach provides reproducibility while allowing the tag to float. Verify this is the intended deployment strategy for the controller.packages/apps/kubernetes/images/kubevirt-csi-driver.tag (1)
1-1: Image reference updated to latest with digest pinning.The image tag has been updated to
latestwith a new sha256 digest. The digest ensures the specific image version is locked, providing immutability despite the floating tag.packages/system/kubevirt-csi-node/values.yaml (1)
3-3: Image reference updated consistently with tag file.The
kubevirt-csi-driverimage has been updated tolatestwith digestsha256:7312623d..., which matches the reference inpackages/apps/kubernetes/images/kubevirt-csi-driver.tag. The digest pinning ensures immutability.packages/core/testing/values.yaml (1)
2-2: Testing image updated to latest with digest.The e2e sandbox image has been updated to
latestwith digest pinning. Usinglatestin testing configurations is generally more acceptable than in production, as it allows for easier integration of recent changes while the digest still ensures reproducibility.hack/update-codegen.sh (1)
56-56: Verified CRD output path references
Confirmedpackages/system/cozystack-controller/crdsexists andtemplates/crds/crds.yamluses.Files.Glob "crds/*".packages/system/kamaji/values.yaml (1)
16-16: LGTM! Migrate-image updated consistently.The migrate-image argument has been updated to match the main image tag, ensuring consistency.
api/v1alpha1/cozystackresourcedefinitions_types.go (2)
127-137: LGTM! Include/Exclude arrays enable flexible resource selection.The Resources type correctly uses pointer slices for optional selectors and maintains clear precedence (exclude overrides include).
54-54: Verify backward compatibility for theSecretsfield type change
In api/v1alpha1/cozystackresourcedefinitions_types.go:54, the JSON fieldsecretsnow uses theCozystackResourceDefinitionResourcestype instead ofCozystackResourceDefinitionSecrets. Confirm that the new type includes all previously exposed fields or implement a conversion/defaulting path to migrate existing CRs without data loss.api/v1alpha1/zz_generated.deepcopy.go (2)
177-195: LGTM! Generated deepcopy correctly handles ResourceSelector.The deepcopy implementation properly handles:
- The embedded LabelSelector via
in.LabelSelector.DeepCopyInto(&out.LabelSelector)- The ResourceNames slice with nil check and copy
198-232: LGTM! Generated deepcopy correctly handles Resources type.The deepcopy implementation properly handles both Include and Exclude slices with correct type references (
*CozystackResourceDefinitionResourceSelector), nil checks, and element-wise deep copying.packages/system/cozystack-api/cozyrds/mysql.yaml (1)
32-33: LGTM! Migration to name-based selector model.The change from label-based exclusion to an empty exclude list aligns with the PR's objective to support name-based resource selectors via the new
resourceNamesfield. The include pattern[{}]preserves the default inclusion behavior.packages/system/cozystack-api/cozyrds/bucket.yaml (1)
33-34: LGTM! Consistent with resource selector migration.The removal of label-based exclusion rules and adoption of an empty exclude list is consistent with the broader migration to support name-based resource selectors introduced in this PR.
packages/system/cozystack-api/cozyrds/virtual-machine.yaml (1)
33-34: LGTM! Aligns with the resource selector refactoring.This change is consistent with the systematic migration from label-based to name-based resource selection across all CozystackResourceDefinition manifests.
packages/system/cozystack-api/cozyrds/bootbox.yaml (1)
32-33: LGTM! Completes the systematic migration pattern.This change completes the consistent pattern observed across all reviewed CozystackResourceDefinition files, removing label-based exclusions in preparation for the new
resourceNames-based selector functionality.packages/system/cozystack-api/cozyrds/ingress.yaml (1)
33-34: LGTM! Consistent with resource selector migration.The change from label-based exclusion to an empty exclude list aligns with the PR's objective to support name-based resource selection via the new
resourceNamesfield. The default behavior now includes all secrets (viainclude: [{}]), with no exclusions.packages/system/cozystack-api/cozyrds/http-cache.yaml (1)
33-34: LGTM! Consistent migration pattern.This change mirrors the selector migration across other CozystackResourceDefinition manifests, removing label-based exclusions in favor of the new resource selector mechanism with optional
resourceNamessupport.packages/system/cozystack-api/cozyrds/vpn.yaml (1)
32-33: LGTM! Consistent with PR-wide refactoring.The removal of label-based exclusions is consistent with the broader migration to resource selectors with
resourceNamessupport, enabling template-based name matching for secret visibility control.packages/system/cozystack-api/cozyrds/rabbitmq.yaml (1)
32-33: LGTM! Aligned with resource selector refactoring.This change removes tenant/resource label-based exclusions, consistent with the PR's introduction of name-based resource selection via the new
resourceNamesfield and template matching capabilities.packages/system/cozystack-api/cozyrds/kafka.yaml (1)
32-33: Approve: Secret visibility migration applied consistently
All cozyrds manifests inpackages/system/cozystack-api/cozyrds/now usesecrets.exclude: []andsecrets.include: [{}]with no remainingmatchLabels. Consider updating documentation to inform application developers of the newresourceNamestemplating support.packages/system/cozystack-api/cozyrds/kubernetes.yaml (1)
32-34: LGTM! Secrets configuration aligned with resource selector migration.The change from a nested matchLabels-based exclusion to an empty exclude list aligns with the PR's migration to ResourceSelector-based targeting. The
include: [{}]preserves the default allow-all behavior while the empty exclusion list ensures no resources are filtered out by default.packages/system/cozystack-api/cozyrds/vm-instance.yaml (1)
32-34: LGTM! Consistent with the resource selector migration.The secrets configuration change matches the pattern across other CozyRDS resources in this PR, removing label-based exclusions in favor of the new ResourceSelector mechanism with resourceNames support.
packages/system/cozystack-api/cozyrds/clickhouse.yaml (1)
30-32: LGTM! Configuration aligned with PR-wide resource selector changes.The secrets configuration follows the consistent pattern of migrating from label-based exclusions to the new ResourceSelector approach, supporting the introduction of name-based filtering via resourceNames.
packages/system/cozystack-api/cozyrds/vm-disk.yaml (1)
32-34: LGTM! Secrets selector configuration updated consistently.This change maintains consistency with the other CozyRDS resources, migrating to the ResourceSelector-based approach that supports both label and name-based filtering.
packages/system/cozystack-controller/templates/crds/crds.yaml (1)
1-4: LGTM! CRD aggregation template is correctly structured.The Helm template logic correctly iterates over all CRD files in the
crds/directory and concatenates them with proper YAML document separators. This dynamic approach aligns with the PR's goal of supporting the expanded ResourceSelector capabilities across all CRDs.Note: The YAMLlint syntax error is a false positive—this file contains Helm template syntax, not pure YAML, and will be correctly processed during Helm rendering.
packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml (1)
1-4: LGTM! Template logic is correct.This Helm template dynamically aggregates all CRD files from the
cozyrds/directory, which is a clean approach for managing multiple resource definitions. The YAMLlint syntax error is a false positive—the tool doesn't recognize Helm's Go template syntax ({{-,}}, etc.), which is expected for.yamlfiles containing templates.packages/system/cozystack-api/cozyrds/tcp-balancer.yaml (1)
32-33: Consistent migration to resource selector model.The change from label-based exclusion to an empty exclude list aligns with the PR's shift toward name-based resource selection. With
exclude: []andinclude: [{}], all secrets are now included by default, which supports the newresourceNamesfiltering approach introduced in this PR.packages/system/cozystack-api/cozyrds/monitoring.yaml (1)
33-36: Excellent example of the new resourceNames feature.This demonstrates the PR's core functionality: name-based resource selection. By specifying
resourceNames: ["grafana-admin-password"], only this specific secret will be visible to end users, providing precise control over resource visibility without relying on label selectors.packages/system/cozystack-api/cozyrds/etcd.yaml (1)
33-34: Consistent with the resource selector migration.Like other CRDs in this PR, the label-based exclusion has been removed. The
include: [{}]matcher ensures all secrets are visible, which is appropriate for the etcd application where no specific secret filtering is needed.packages/system/cozystack-api/cozyrds/ferretdb.yaml (1)
32-33: Migration complete and consistent.The ferretdb CRD completes the consistent pattern across all reviewed resources: removing label-based exclusions in favor of the new resource selector model. With
exclude: []andinclude: [{}], all secrets are visible by default, allowing future name-based filtering viaresourceNamesif needed.packages/system/cozystack-api/cozyrds/postgres.yaml (1)
40-43: Excellent implementation of name-based secret selector.This configuration demonstrates the intended use of the new
resourceNamesfeature. By specifyingpostgres-{{ .name }}-app, only the application-specific secret is exposed to end users while keeping other PostgreSQL secrets (e.g., superuser, replication credentials) hidden. The template variable{{ .name }}correctly references the application name.This approach is more secure than using
include: [{}], which would expose all secrets. Consider applying similar patterns to other resource definitions (likenats.yamlandseaweedfs.yaml) if specific secrets should be exposed rather than all secrets.packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml (2)
178-203: Comprehensive documentation for resourceNames in exclude selector.The CRD schema additions are well-documented with clear descriptions of:
- The AND logic between label selectors and resource names
- Available template variables (
{{ .name }}and{{ .kind }})- Concrete YAML examples showing usage patterns
The schema correctly defines
resourceNamesas an optional array of strings, aligned with the Go type definition inapi/v1alpha1/cozystackresourcedefinitions_types.go.Also applies to: 246-252
258-284: Consistent schema and documentation for include selector.The
includeselector schema mirrors theexcludeselector implementation, maintaining consistency across the CRD. The documentation quality and structure are identical, which aids understanding.Also applies to: 327-333
internal/lineagecontrollerwebhook/matcher.go (2)
53-60: OR logic across selector array correctly implemented.The function properly implements OR semantics: returns
trueif any selector in the array matches. The updated signature correctly passesnameandctxto the underlyingmatchResourceToSelectorfunction.
62-70: Exclude/include logic correctly implements documented precedence.The function correctly implements the documented behavior:
- If resource matches any exclude selector → hidden (return
false)- Else if resource matches any include selector → visible (return
true)- Else → hidden by default (return
false)This precedence ensures that exclusions cannot be overridden by inclusions, which is the expected security behavior.
packages/system/cozystack-api/cozyrds/seaweedfs.yaml (1)
33-34: Ignore tenantresource exclusion suggestion.This chart file doesn’t reference
apps.cozystack.io/tenantresourcenor define anySecretresources—no exclusion was removed here.Likely an incorrect or invalid review comment.
packages/system/cozystack-api/cozyrds/nats.yaml (1)
32-33: Clarify the NATS secrets filter
exclude: []withinclude: [{}]matches every secret in the namespace; unlike the Postgres CRD which explicitly listsresourceNames, you should either:
- Specify only the NATS secrets (e.g.
<Release.Name>-credentials, thenatsBox.contextsSecret.name,natsBox.contentsSecret.name) underinclude.resourceNames- Or confirm that exposing all secrets is intentional for NATS deployments.
This patch implements name-based selectors for
`CozystackResourceDefinitions.spec.secrets`. Application developers may
now specify secrets that should or should not be visible to end users by
specifying a `resourceNames` field with a string slice of acceptable
names. This will, for instance, let developers exclude a secret like
`postgres-dbname-superuser` that has a predictable name even if it does
not have predictable labels. Simple templates are supported, so
`postgres-{{ .name }}-superuser` is also a valid entry under
`resourceNames`.
```release-note
[lineage, controller] Let application developers determine resource
visibility for end users by name, as well as by labels.
```
Signed-off-by: Timofei Larkin <[email protected]>
8c60523 to
66004c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
internal/lineagecontrollerwebhook/matcher.go (3)
21-21: Performance: Template parsing in loop.Parsing templates with
template.New().Parse()inside the loop is inefficient and becomes a bottleneck in high-traffic webhooks. Since templates come from CRDs that change infrequently, consider pre-parsing them in the controller's reconcile loop and caching the*template.Templateobjects.
22-32: Missing error handling for template operations.Errors from
template.New().Parse()(line 22) andtmpl.Execute()(line 29) are ignored with TODO comments. Users will have no visibility into malformed templates, making debugging very difficult. Propagatecontext.Contextto this function so you can log these errors usinglog.FromContext(ctx).
44-47: Missing error handling for invalid label selectors.The error from
metav1.LabelSelectorAsSelector()is ignored. An invalid label selector in the CRD will cause matching to fail silently, making debugging difficult. Propagatecontext.Contextto this function and log the error.
🧹 Nitpick comments (7)
packages/extra/monitoring/images/grafana.tag (1)
1-1: Document bulk image refresh and unify tagging strategy
- This PR includes coordinated updates to multiple image tags across
packages/*/images/*.tag; please note this batch refresh in the PR description for clarity.- Consider adopting a consistent tagging policy—either semantic version tags (e.g.,
vX.Y.Z) or uniformly usinglatest@sha256—to improve reproducibility and release management.packages/system/cilium/values.yaml (1)
17-18: Clarify the use of "latest" tag with pinned digest.The image uses
tag: latestwhile maintaining a pinned digest. Since the digest takes precedence over the tag, the effective image is immutable. However, using "latest" as the tag can be confusing and doesn't clearly communicate the intended version. Consider using a semantic version tag that matches the digest for better clarity and maintenance.packages/apps/kubernetes/images/ubuntu-container-disk.tag (1)
1-1: Reconsider using "latest" tag for container disk images.The image reference was changed from a semantic version (
v1.32) tolatest. While the digest pin ensures immutability, usinglatestas the tag reduces version traceability and makes it harder to identify which Ubuntu version is deployed without inspecting the digest. For container disk images used in VMs, clear version identification is particularly important for troubleshooting and rollback scenarios.packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml (4)
183-203: Clarify behavior when resourceNames is omitted.The documentation states "A resource matches this selector only if it satisfies ALL criteria" and lists resourceNames as one criterion. However, it's unclear what happens when
resourceNamesis omitted or empty. Does it match all names (no name filtering) or match none?Based on the "(if specified)" clause on line 186, it appears that omitting
resourceNamesmeans no name filtering is applied. Consider making this more explicit in the main description.Example clarification:
CozystackResourceDefinitionResourceSelector extends metav1.LabelSelector with resourceNames support. A resource matches this selector only if it satisfies ALL criteria: - Label selector conditions (matchExpressions and matchLabels) - - AND has a name that matches one of the names in resourceNames (if specified) + - If resourceNames is specified, the resource name must match one of the provided names (after template expansion) + - If resourceNames is omitted, no name filtering is applied
264-284: Same clarification needed for include section.The same documentation ambiguity about omitted
resourceNamesbehavior exists here as in the exclude section (lines 183-203). Consider applying the same clarification.
246-252: Clarify "exact names" with template expansion.The description states "the resource must have one of these exact names" but templates like
"{{ .name }}-secret"are expanded first. Consider clarifying that exact matching occurs after template expansion.Suggested revision:
resourceNames: description: |- ResourceNames is a list of resource names to match - If specified, the resource must have one of these exact names to match the selector + If specified, templates are expanded first, then the resource name must exactly match one of the resulting names
327-333: Same clarification needed for include section.Apply the same template expansion clarification here as suggested for the exclude section (lines 246-252).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)api/v1alpha1/zz_generated.deepcopy.go(1 hunks)hack/update-codegen.sh(1 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/clickhouse/images/clickhouse-backup.tag(1 hunks)packages/apps/kubernetes/images/cluster-autoscaler.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-cloud-provider.tag(1 hunks)packages/apps/kubernetes/images/kubevirt-csi-driver.tag(1 hunks)packages/apps/kubernetes/images/ubuntu-container-disk.tag(1 hunks)packages/apps/mysql/images/mariadb-backup.tag(1 hunks)packages/extra/monitoring/images/grafana.tag(1 hunks)packages/system/cilium/values.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/monitoring.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml(1 hunks)packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml(3 hunks)packages/system/cozystack-controller/templates/crds/crds.yaml(1 hunks)packages/system/kubevirt-csi-node/values.yaml(1 hunks)packages/system/metallb/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/system/cozystack-api/cozyrds/redis.yaml
- packages/system/cozystack-api/cozyrds/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/bootbox.yaml
- packages/system/cozystack-api/cozyrds/mysql.yaml
- packages/system/cozystack-api/cozyrds/info.yaml
- packages/system/cozystack-api/cozyrds/kubernetes.yaml
- packages/apps/kubernetes/images/kubevirt-csi-driver.tag
- packages/system/cozystack-api/cozyrds/vm-instance.yaml
- packages/system/cozystack-api/cozyrds/ferretdb.yaml
- packages/system/cozystack-api/cozyrds/tenant.yaml
- packages/system/cozystack-api/cozyrds/http-cache.yaml
- packages/apps/kubernetes/images/cluster-autoscaler.tag
- packages/system/cozystack-api/cozyrds/postgres.yaml
- packages/system/metallb/values.yaml
- packages/system/cozystack-api/cozyrds/clickhouse.yaml
- packages/system/cozystack-api/cozyrds/monitoring.yaml
- packages/system/cozystack-api/cozyrds/tcp-balancer.yaml
- packages/system/cozystack-api/cozyrds/etcd.yaml
- packages/apps/clickhouse/images/clickhouse-backup.tag
- packages/system/cozystack-api/cozyrds/ingress.yaml
- packages/system/cozystack-api/cozyrds/kafka.yaml
- packages/system/cozystack-api/cozyrds/vpn.yaml
- internal/lineagecontrollerwebhook/webhook.go
- hack/update-codegen.sh
🧰 Additional context used
🧬 Code graph analysis (2)
api/v1alpha1/zz_generated.deepcopy.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (2)
CozystackResourceDefinitionResourceSelector(119-125)CozystackResourceDefinitionResources(127-137)
internal/lineagecontrollerwebhook/matcher.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionResourceSelector(119-125)
🪛 YAMLlint (1.37.1)
packages/system/cozystack-controller/templates/crds/crds.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (18)
packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml (1)
1-4: Helm aggregation loop reads clean.Thanks for keeping the cozyrd bundle template minimal and consistent—nothing to change here.
packages/apps/kubernetes/images/kubevirt-cloud-provider.tag (1)
1-1: Confirm intentional update of kubevirt-cloud-provider imageThe image tag was changed from
0.0.0@sha256:5335c044…tolatest@sha256:190b7d23…, which appears unrelated to the name-selector work. Although the digest ensures immutability, use a semantic version tag for clearer tracking.
- Is this update required for the PR, or should it be split into a separate change?
- Can you replace
latestwith a specific version tag instead?packages/apps/mysql/images/mariadb-backup.tag (1)
1-1: Drop the separation suggestion for this tag update. The bump tolatest@sha256:…is one of several.tagfiles updated in this PR as a batch change.Likely an incorrect or invalid review comment.
packages/system/cozystack-api/cozyrds/bucket.yaml (1)
33-34: Verify the removal of tenantresource label-based exclusion.Similar to other cozyrds configurations in this PR, the
secrets.excluderule filtering secrets withapps.cozystack.io/tenantresource: "false"has been removed. This change makes all secrets visible to end users by default.Confirm that this behavioral change aligns with the new resource selector design and that no secrets requiring exclusion are inadvertently exposed. If needed, leverage the new
resourceNamesfield for targeted exclusions.packages/system/cozystack-controller/templates/crds/crds.yaml (1)
1-4: LGTM!The Helm template correctly aggregates CRD files from the
crds/directory, outputting them with proper YAML document separators. This approach is idiomatic for Helm charts and aligns with the updated CRD generation path.Note: The YAMLlint syntax error is a false positive—the tool doesn't recognize Helm template syntax.
packages/system/cozystack-api/cozyrds/vm-disk.yaml (1)
33-34: Verify tenantresource-based secret exclusion removal
Secrets defined in the following templates still useapps.cozystack.io/tenantresource: "false"and will now be exposed due toexclude: []invm-disk.yaml:
- packages/apps/vpn/templates/secret.yaml
- packages/apps/mysql/templates/backup-script.yaml
- packages/apps/postgres/templates/init-script.yaml
- packages/apps/clickhouse/templates/backup-script.yaml
- packages/extra/monitoring/templates/alerta/alerta.yaml
Confirm this is intentional. If certain secrets must remain hidden, reintroduce name-based exclusions via the new
resourceNamesfield or update the selector accordingly.packages/system/cozystack-api/cozyrds/virtual-machine.yaml (1)
32-34: LGTM: Secrets filtering now uses resource-selector semantics.The change from label-based exclusion to an empty
excludelist with a genericinclude: [{}]makes all secrets visible by default. This aligns with the PR's shift to name-based resource selectors via the newresourceNamesfield.packages/system/cozystack-api/cozyrds/nats.yaml (1)
31-33: LGTM: Consistent secrets filtering update.This change mirrors the pattern in other cozyrds YAMLs, removing label-based exclusion in favor of the new resource-selector approach with
resourceNamessupport.packages/system/cozystack-api/cozyrds/seaweedfs.yaml (1)
32-34: LGTM: Secrets filtering updated.Consistent with the broader refactoring to use resource-level selectors with
resourceNamesinstead of label-based exclusions.internal/lineagecontrollerwebhook/matcher.go (2)
53-60: LGTM: Correct OR semantics for selector arrays.The function correctly implements "match any" logic for selector arrays by iterating and returning true on the first match.
62-70: LGTM: Correct exclude/include precedence.The function correctly implements the documented semantics: exclusions take precedence, then inclusions are checked, with a default of false (not visible).
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
98-125: LGTM: Well-documented resource selector type.The
CozystackResourceDefinitionResourceSelectortype correctly embedsmetav1.LabelSelectorand adds theResourceNamesfield with template support. The documentation clearly explains the AND semantics and provides helpful examples.
127-137: LGTM: Correct include/exclude semantics.The
CozystackResourceDefinitionResourcestype correctly uses pointer slices forExcludeandInclude, and the documentation clearly states that exclusions take precedence. This aligns with thematchResourceToExcludeIncludeimplementation inmatcher.go.
54-54: Verify CRD migration is complete.The
Secretsfield now uses the newCozystackResourceDefinitionResourcestype withResourceSelectorsupport. This is a breaking API change. Ensure all existing CozystackResourceDefinition CRs have been updated to the new format (as evidenced by the cozyrds YAML updates in this PR).api/v1alpha1/zz_generated.deepcopy.go (4)
177-185: LGTM: Correct deepcopy implementation for ResourceSelector.The deepcopy logic properly handles the embedded
LabelSelectorand creates an independent copy of theResourceNamesslice.
188-195: LGTM: Standard DeepCopy pattern.Follows the expected controller-gen pattern with proper nil handling.
198-222: LGTM: Correct deepcopy for pointer slices.The implementation properly deep copies both
ExcludeandIncludeslices, with appropriate nil checks for the slices and individual elements.
224-232: LGTM: Updated to Resources type.The DeepCopy function correctly reflects the renamed type and follows the standard pattern.
This patch refactors the secret selectors to use the `internal.cozystack.io/tenantresource` label for managing secret visibility and removes any selectors based on it or the previous `apps.cozystack.io/tenantresource` label, the idea being that this label will only ever be set by the controller. ``` [controller,api] Refactor labels for the secret selector. ``` Signed-off-by: Timofei Larkin <[email protected]>
f87c07e to
80fc345
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/clickhouse/templates/backup-script.yaml(0 hunks)packages/apps/mysql/templates/backup-script.yaml(0 hunks)packages/apps/postgres/templates/init-script.yaml(0 hunks)packages/apps/rabbitmq/templates/rabbitmq.yaml(1 hunks)packages/apps/vpn/templates/secret.yaml(0 hunks)packages/extra/monitoring/templates/alerta/alerta.yaml(0 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)pkg/apis/core/v1alpha1/tenantresource_types.go(1 hunks)pkg/registry/core/tenantmodule/rest.go(1 hunks)pkg/registry/core/tenantsecret/rest.go(2 hunks)pkg/registry/core/tenantsecretstable/rest.go(2 hunks)
💤 Files with no reviewable changes (5)
- packages/apps/postgres/templates/init-script.yaml
- packages/extra/monitoring/templates/alerta/alerta.yaml
- packages/apps/clickhouse/templates/backup-script.yaml
- packages/apps/mysql/templates/backup-script.yaml
- packages/apps/vpn/templates/secret.yaml
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/system/cozystack-api/cozyrds/vm-instance.yaml
- packages/system/cozystack-api/cozyrds/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/vpn.yaml
- pkg/registry/core/tenantmodule/rest.go
- packages/system/cozystack-api/cozyrds/http-cache.yaml
- packages/system/cozystack-api/cozyrds/etcd.yaml
- packages/system/cozystack-api/cozyrds/tcp-balancer.yaml
- packages/system/cozystack-api/cozyrds/nats.yaml
- packages/system/cozystack-api/cozyrds/vm-disk.yaml
- packages/system/cozystack-api/cozyrds/clickhouse.yaml
- pkg/registry/core/tenantsecret/rest.go
- packages/system/cozystack-api/cozyrds/info.yaml
- packages/system/cozystack-api/cozyrds/ingress.yaml
- packages/system/cozystack-api/cozyrds/postgres.yaml
- packages/system/cozystack-api/cozyrds/tenant.yaml
- packages/apps/rabbitmq/templates/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/seaweedfs.yaml
- packages/system/cozystack-api/cozyrds/bucket.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/registry/core/tenantsecretstable/rest.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (2)
TenantResourceLabelKey(3-3)TenantResourceLabelValue(4-4)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (2)
TenantResourceLabelKey(3-3)TenantResourceLabelValue(4-4)
🪛 GitHub Actions: Pull Request
internal/lineagecontrollerwebhook/matcher.go
[error] 11-11: Go build failed: unused import 'sigs.k8s.io/controller-runtime' (ctrl) in internal/lineagecontrollerwebhook/matcher.go.
🔇 Additional comments (16)
packages/system/cozystack-api/cozyrds/virtual-machine.yaml (1)
33-34: Validate virtual-machine secrets visibility with empty include.We now have an empty
includelist. In the old scheme we matched by labels so VM users still received their kubeconfig/credentials secrets. Please verify whether the new name-based matcher interprets an empty include as “select nothing” or “select everything,” and add explicitresourceNamesfor the VM secrets if they should stay visible.pkg/registry/core/tenantsecretstable/rest.go (1)
3-3: LGTM! Centralized label constants improve maintainability.The refactoring from hardcoded label strings to centralized constants (
corev1alpha1.TenantResourceLabelKeyandTenantResourceLabelValue) is a good practice that reduces duplication and improves maintainability across the codebase.Also applies to: 32-33
pkg/apis/core/v1alpha1/tenantresource_types.go (1)
3-4: LGTM! Well-defined label constants.The constants are appropriately named and use the
internal.cozystack.iodomain prefix, clearly indicating internal use. These centralized definitions improve maintainability and are correctly referenced by other files in the PR.internal/lineagecontrollerwebhook/webhook.go (3)
8-8: LGTM! Imports correctly added for new functionality.The
stringspackage is used forstrings.ToLoweron Line 137, and thecorev1alpha1alias provides access to the centralized label constants. Both imports are justified and necessary.Also applies to: 21-22
148-153: matchResourceToExcludeInclude signature verified
The function in matcher.go correctly accepts (ctx, name, templateContext, labels, exclude, include), aligning with the new call site.
136-140: Template labels correctly passed through to matcher for evaluation. All keys (kind,name,namespace) intemplateLabelsflow astemplateContextintomatchNameand are available for Go template execution.packages/system/cozystack-api/cozyrds/ferretdb.yaml (1)
32-35: Inconclusive FerretDB secret pattern — manual verification required
Local searches didn’t reveal any Helm templates generatingferretdb-{{ .name }}-credentialssecrets in this repo. Please confirm that the upstream FerretDB chart actually defines secrets following that naming pattern and that.nameis provided in the template context.packages/system/cozystack-api/cozyrds/mysql.yaml (1)
32-35: Approve include selector patternThe selector
mysql-{{ .name }}-credentialscorrectly matches the Helm chart’s secret naming ({{ .Release.Name }}-credentialswhereRelease.Nameismysql-+ CR name).internal/lineagecontrollerwebhook/matcher.go (4)
18-44: Error handling improvements look good; consider template caching for performance.The error handling concerns from the previous review have been addressed—template parsing and execution errors are now properly logged with context. The function correctly handles nil resourceNames and validates templates at runtime.
However, the performance suggestion from the earlier review remains valid: parsing templates inside a loop on every webhook invocation can be a bottleneck. Consider pre-parsing and caching templates when CozystackResourceDefinitions are loaded in the controller's reconcile loop, as suggested in the previous review.
46-55: LGTM: Error handling properly implemented.The label selector conversion error is now properly logged, addressing the concern from the previous review. The AND logic correctly requires both label and name criteria to match.
57-64: LGTM!The function correctly implements OR semantics across the selector array and properly propagates the new parameters.
66-74: LGTM!The exclude/include logic correctly prioritizes exclusions over inclusions and properly implements the selection semantics.
api/v1alpha1/cozystackresourcedefinitions_types.go (3)
98-126: Excellent documentation and type definition.The comprehensive documentation clearly explains the AND semantics, available template variables, and provides a realistic example. The type cleanly extends metav1.LabelSelector with the ResourceNames field, and the implementation correctly matches the documented behavior.
128-138: LGTM!The type definition clearly documents the exclude-takes-precedence semantics and uses appropriate pointer slices for the selector arrays. The implementation in matcher.go correctly reflects this behavior.
54-54: LGTM: API change aligns with PR objectives.The field type change from the previous secret selector structure to the new CozystackResourceDefinitionResources type correctly implements the name-based selection feature described in the PR objectives.
packages/system/cozystack-api/cozyrds/bootbox.yaml (1)
32-33: Confirm BootBox still exposes its intended secretsSwitching both
excludeandincludeto empty lists risks changing selector semantics—ifincludeis now empty, the controller may stop surfacing any BootBox-specific secrets. Please double-check this manifest still keeps the same secrets visible (or explicitly list the expectedresourceNamesso we don’t regress behavior).
80fc345 to
cf29d1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
98-126: Document and validate Go template usage in ResourceNames
- Add a CRD-level validation marker (e.g.
+kubebuilder:validation:Pattern) onResourceNamesin api/v1alpha1/cozystackresourcedefinitions_types.go to enforce basic template syntax rather than relying solely on runtime parsing in the webhook.- In the API docs, note that templates are parsed (template.New…Parse) and executed (tmpl.Execute) at admission time in internal/lineagecontrollerwebhook/matcher.go.
- Clarify that the only variables available to those templates—
.name,.kind, and.namespace—are populated from the unstructured object’s metadata (obj.GetName(), strings.ToLower(obj.GetKind()), o.GetNamespace()).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/rabbitmq/templates/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)pkg/lineage/lineage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/system/cozystack-api/cozyrds/etcd.yaml
- packages/system/cozystack-api/cozyrds/tcp-balancer.yaml
- packages/system/cozystack-api/cozyrds/redis.yaml
- packages/apps/rabbitmq/templates/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/ingress.yaml
- packages/system/cozystack-api/cozyrds/kafka.yaml
- packages/system/cozystack-api/cozyrds/bootbox.yaml
- packages/system/cozystack-api/cozyrds/vpn.yaml
- packages/system/cozystack-api/cozyrds/vm-disk.yaml
- packages/system/cozystack-api/cozyrds/virtual-machine.yaml
- packages/system/cozystack-api/cozyrds/clickhouse.yaml
- packages/system/cozystack-api/cozyrds/http-cache.yaml
- packages/system/cozystack-api/cozyrds/vm-instance.yaml
- packages/system/cozystack-api/cozyrds/kubernetes.yaml
- packages/system/cozystack-api/cozyrds/mysql.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (2)
TenantResourceLabelKey(3-3)TenantResourceLabelValue(4-4)
internal/lineagecontrollerwebhook/matcher.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionResourceSelector(120-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (17)
packages/system/cozystack-api/cozyrds/tenant.yaml (1)
30-31: LGTM! Consistent with resource-level selector migration.The secrets filtering has been updated to use empty include/exclude lists, aligning with the broader migration from secret-level label-based selectors to resource-level selectors with optional
resourceNamessupport. For tenant resources, having no default secret filtering is appropriate.packages/system/cozystack-api/cozyrds/seaweedfs.yaml (1)
33-34: LGTM! Consistent migration to resource-level selectors.The secrets filtering has been updated to empty include/exclude lists, consistent with the broader migration to resource-level selectors. For SeaweedFS (a tenant module), having no default secret filtering is appropriate.
packages/system/cozystack-api/cozyrds/rabbitmq.yaml (1)
32-37: Approve RabbitMQ secret selectors configurationThe
apps.cozystack.io/user-secret: "true"label appears in both packages/apps/rabbitmq/templates/rabbitmq.yaml (line 62) and packages/system/cozystack-api/cozyrds/rabbitmq.yaml (line 37), confirming consistent usage.packages/system/cozystack-api/cozyrds/ferretdb.yaml (1)
32-35: Approve template-based resourceNames configuration
Confirmed.nameis populated intemplateLabels(internal/lineagecontrollerwebhook/webhook.go:138), so theferretdb-{{ .name }}-credentialstemplate will render correctly.pkg/lineage/lineage_test.go (1)
49-49: Resolved: stubMapper correctly implements AppMapper
stubMapper is defined in pkg/lineage/mapper.go and satisfies the AppMapper interface; WalkOwnershipGraph’s signature accepts it as the fourth argument, so no changes needed.packages/system/cozystack-api/cozyrds/info.yaml (1)
33-37: LGTM! Resource-level selector with name-based templates.The migration from label-based exclusion to resourceNames-based inclusion is correct. The templates
kubeconfig-{{ .namespace }}and{{ .namespace }}will match secrets by name using the namespace context provided by the webhook's templateLabels.packages/system/cozystack-api/cozyrds/nats.yaml (1)
32-35: LGTM! Correct resourceNames pattern for NATS credentials.The template
nats-{{ .name }}-credentialscorrectly uses the.namevariable from templateLabels to match NATS credential secrets. Consistent with the PR's shift to resource-level name-based selectors.packages/system/cozystack-api/cozyrds/postgres.yaml (1)
40-43: LGTM! Correct resourceNames pattern for PostgreSQL credentials.The template
postgres-{{ .name }}-credentialscorrectly uses the.namevariable from templateLabels to match PostgreSQL credential secrets. Consistent with the broader PR pattern.packages/system/cozystack-api/cozyrds/bucket.yaml (1)
33-37: LGTM! Correct resourceNames patterns for bucket secrets.The templates
bucket-{{ .name }}andbucket-{{ .name }}-credentialscorrectly use the.namevariable to match both the bucket secret and its credentials. Consistent with the PR's resource-level selector approach.internal/lineagecontrollerwebhook/webhook.go (4)
8-8: LGTM! Required import for template label conversion.The
stringsimport is correctly added to supportstrings.ToLowerfor the kind field in templateLabels (line 137).
21-22: LGTM! Import for centralized label constants.The
corev1alpha1import correctly references the centralized tenant resource label constants (TenantResourceLabelKey and TenantResourceLabelValue) from pkg/apis/core/v1alpha1/tenantresource_types.go.Based on learnings.
136-140: LGTM! Template labels correctly constructed.The templateLabels map correctly provides:
kind: lowercased application kind for case-insensitive matchingname: application name (from ancestor)namespace: current object's namespace (appropriate for namespace-scoped secrets)These variables align with the template patterns used in the cozyrds YAML files (e.g.,
{{ .name }},{{ .namespace }}).
148-153: LGTM! Correct integration of resource-level matching.The changes correctly:
- Use centralized tenant resource label constants (lines 148, 150)
- Pass
o.GetName()andtemplateLabelstomatchResourceToExcludeInclude(line 153) to enable template-based name matching alongside label-based filtering- Align with the broader PR shift from secret-level to resource-level selectors
api/v1alpha1/cozystackresourcedefinitions_types.go (2)
54-54: LGTM! Field type change is backward compatible.The field name
Secretsremains unchanged while the type evolves fromCozystackResourceDefinitionSecretstoCozystackResourceDefinitionResources, maintaining JSON compatibility.
128-138: LGTM! Include/Exclude semantics are clear.The documentation clearly states the precedence (Exclude overrides Include) and the matching logic for tenant resources.
internal/lineagecontrollerwebhook/matcher.go (2)
45-54: LGTM! Matching logic correctly implements documented semantics.The function correctly:
- Combines label selector AND name matching (line 53) as documented in types.go lines 99-101
- Logs label selector conversion errors (line 48), addressing a previous review concern
- Returns false on selector conversion failure (safe default)
65-73: LGTM! Exclude precedence is correctly implemented.The function correctly implements the precedence documented in types.go lines 131-137: resources matching Exclude selectors are hidden regardless of Include matches.
f2cee70 to
d3e3483
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/lineagecontrollerwebhook/matcher.go (1)
17-43: Template parsing performance and security issues remain unaddressed.Previous reviews identified critical concerns with this implementation that have not been resolved:
Performance bottleneck (duplicate of high-priority comment): Templates are still parsed on every webhook call (line 24). In a webhook hot path, this will cause significant latency and CPU overhead under load.
Fail-open error handling (duplicate of medium-priority comment): Parse/execute errors are logged but matching continues with
continue(lines 27, 34), causing unpredictable resource visibility when templates are malformed.Template security (duplicate of major-priority comment): No timeout, memory limit, or size constraints on template execution. While
templateContextappears controlled, unbounded template execution in a webhook poses DoS risk.Required fixes:
- Pre-parse templates when
CozystackResourceDefinitionis loaded in the controller'sReconcileloop- Store parsed
*template.TemplateinruntimeConfig- Pass pre-parsed templates to
matchNameinstead of raw strings- Implement clear failure mode (recommend fail-closed: return
falseon errors)- Add timeout/size limits on template execution
- Validate template syntax at CRD admission time
🧹 Nitpick comments (2)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
53-54: Consider clarifying the field name or comment.The field is named
Secretsbut now uses theCozystackResourceDefinitionResourcestype. While the semantics remain secret-focused, the comment "Secret selectors" could be expanded to "Resource selectors for secrets" to clarify that the selection mechanism now operates at the resource level.Apply this diff to clarify the comment:
- // Secret selectors + // Resource selectors for secrets Secrets CozystackResourceDefinitionResources `json:"secrets,omitempty"`internal/lineagecontrollerwebhook/webhook.go (1)
136-140: Clarify.namespacein templateLabels
Templates receive the resource’s namespace (o.GetNamespace(), line 139) but the CRD docs only document.nameand.kind. Either switch toobj.GetNamespace()if you meant the application’s namespace or add.namespaceto the CRD template-variable documentation to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/rabbitmq/templates/rabbitmq.yaml(1 hunks)packages/extra/info/templates/dashboard-resourcemap.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)packages/system/dashboard/templates/keycloakclient.yaml(2 hunks)pkg/lineage/lineage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/system/cozystack-api/cozyrds/seaweedfs.yaml
- packages/system/cozystack-api/cozyrds/vpn.yaml
- packages/system/cozystack-api/cozyrds/etcd.yaml
- packages/system/cozystack-api/cozyrds/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/postgres.yaml
- packages/system/cozystack-api/cozyrds/clickhouse.yaml
- packages/system/cozystack-api/cozyrds/bootbox.yaml
- packages/system/cozystack-api/cozyrds/tcp-balancer.yaml
- packages/system/cozystack-api/cozyrds/kafka.yaml
- packages/system/cozystack-api/cozyrds/ferretdb.yaml
- packages/system/cozystack-api/cozyrds/vm-disk.yaml
- packages/system/cozystack-api/cozyrds/vm-instance.yaml
- packages/system/cozystack-api/cozyrds/ingress.yaml
- packages/system/cozystack-api/cozyrds/redis.yaml
- packages/extra/info/templates/dashboard-resourcemap.yaml
- packages/system/cozystack-api/cozyrds/kubernetes.yaml
- packages/system/cozystack-api/cozyrds/http-cache.yaml
- packages/system/cozystack-api/cozyrds/virtual-machine.yaml
- packages/system/cozystack-api/cozyrds/tenant.yaml
- packages/system/cozystack-api/cozyrds/bucket.yaml
- pkg/lineage/lineage_test.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (2)
TenantResourceLabelKey(3-3)TenantResourceLabelValue(4-4)
internal/lineagecontrollerwebhook/matcher.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionResourceSelector(120-126)
🔇 Additional comments (11)
packages/system/cozystack-api/cozyrds/mysql.yaml (1)
31-35: LGTM! Clean migration to resource-level selectors.The shift from label-based filtering to resourceNames-based inclusion aligns with the PR objectives. The template syntax
mysql-{{ .name }}-credentialscorrectly references the.namevariable documented in the CozystackResourceDefinitionResourceSelector type.packages/apps/rabbitmq/templates/rabbitmq.yaml (1)
61-62: LGTM! Label addition supports resource selection.Adding the
apps.cozystack.io/user-secretlabel enables filtering user credentials from other secrets. This complements the resourceNames-based selection introduced in this PR and provides an additional dimension for resource matching.api/v1alpha1/cozystackresourcedefinitions_types.go (1)
128-138: LGTM! Clear precedence semantics.The exclude-takes-precedence design is well-documented, and using pointer slices allows for proper nil handling. The "tenant resource" terminology aligns with the broader system concepts.
packages/system/dashboard/templates/keycloakclient.yaml (1)
48-76: LGTM! Good defensive practice for optional CRDs.The capability check ensures the KeycloakClient manifest is only rendered when the corresponding CRD is installed, preventing deployment failures in environments where the Keycloak operator isn't present. The conditional wrapping is correctly scoped.
packages/system/cozystack-api/cozyrds/info.yaml (1)
32-37: LGTM! Verify template variable support.The resourceNames entries correctly use the
{{ .namespace }}template variable for namespace-scoped secret selection. This aligns with the kubeconfig use case where secrets are named after their namespace.Note: This relies on the same
.namespacevariable verification requested in the types file review.packages/system/cozystack-api/cozyrds/nats.yaml (1)
31-35: LGTM! Template context alignment confirmed.The resourceNames template references
{{ .name }}, which correctly aligns with thetemplateContext["name"]set ininternal/lineagecontrollerwebhook/webhook.go(line 138). The structure follows the new ResourceSelector pattern introduced in this PR.internal/lineagecontrollerwebhook/webhook.go (3)
8-8: LGTM! Necessary imports added.The
stringsimport (line 8) is used forstrings.ToLowerat line 137, and thecorev1alpha1import (line 22) provides the centralized tenant resource label constants used at lines 148 and 150.Also applies to: 21-22
148-150: LGTM! Centralized constants improve maintainability.Replacing hardcoded label strings with
corev1alpha1.TenantResourceLabelKeyandcorev1alpha1.TenantResourceLabelValueis a good practice that ensures consistency across the codebase.
153-153: LGTM! Function call updated correctly.The call to
matchResourceToExcludeIncludenow includes the resource name andtemplateLabels, enabling the new template-based name matching while preserving existing label-based matching.internal/lineagecontrollerwebhook/matcher.go (2)
4-6: LGTM! Necessary imports for template-based matching.All imports are used:
bytesfor template buffers,contextfor logging,text/templatefor Go template parsing,cozyv1alpha1for the ResourceSelector type, andlogfor error logging.Also applies to: 8-8, 11-11
56-73: LGTM! Selector array and exclude/include logic correct.The updated functions correctly:
- Iterate over ResourceSelector arrays with name and templateContext support
- Apply exclude-before-include precedence (line 66-68)
- Propagate context for logging throughout the call chain
d3e3483 to
852fc49
Compare
This patch carries the selectors for secrets to be shown to end users
over from the legacy dashboard-resourcemap roles into the new
CozystackResourceDefinition selectors. Also a {{ .namespace }} template
variable is added to the variables supported in the `resourceNames`
field in the selector.
```release-note
[controller,api] Support {{ .namespace }} in `resourceNames` resource
selectors, add whitelist of secrets to show to end-users.
```
Signed-off-by: Timofei Larkin <[email protected]>
852fc49 to
8d50dfb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
120-126: Consider adding webhook validation for template syntax.While the type definition is solid, there's no compile-time or admission-time validation of template syntax in
ResourceNames. Invalid templates will only be caught at webhook runtime, potentially causing resources to be unexpectedly included or excluded.Consider adding a validating webhook or CRD validation rule that pre-parses templates when the
CozystackResourceDefinitionis created/updated to catch syntax errors early.Example approach: Add a validation webhook that attempts to parse each
resourceNamesentry as a Go template during CRD admission, rejecting invalid syntax before it reaches the lineage webhook.packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml (2)
247-253: Consider adding OpenAPI validation for template syntax.The
resourceNamesfield allows arbitrary strings without validation. While runtime template parsing will catch errors, adding a regex pattern or webhook validation would provide earlier feedback to users creating malformed templates.Example OpenAPI pattern to catch basic template syntax errors:
resourceNames: description: |- ResourceNames is a list of resource names to match If specified, the resource must have one of these exact names to match the selector items: type: string pattern: '^[a-zA-Z0-9._\-{}{{ }}.]+$' type: arrayNote: This is a low-priority enhancement; the current implementation is functional.
183-204: Documentation duplication across exclude and include sections.The extensive documentation block (lines 183-204 and 265-286) is duplicated verbatim between the
excludeandincludearray item schemas. While functionally correct, this increases maintenance burden if the documentation needs updates.Consider using YAML anchors and aliases to DRY up the schema:
x-resource-selector-doc: &resource-selector-doc description: |- CozystackResourceDefinitionResourceSelector extends metav1.LabelSelector with resourceNames support. [rest of documentation]Then reference it:
items: <<: *resource-selector-doc properties: # ...Note: This is a minor maintenance improvement and not required for functionality.
Also applies to: 265-286
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
api/v1alpha1/cozystackresourcedefinitions_types.go(2 hunks)internal/lineagecontrollerwebhook/matcher.go(1 hunks)internal/lineagecontrollerwebhook/webhook.go(3 hunks)packages/apps/rabbitmq/templates/rabbitmq.yaml(1 hunks)packages/extra/info/templates/dashboard-resourcemap.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bootbox.yaml(1 hunks)packages/system/cozystack-api/cozyrds/bucket.yaml(1 hunks)packages/system/cozystack-api/cozyrds/clickhouse.yaml(1 hunks)packages/system/cozystack-api/cozyrds/etcd.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ferretdb.yaml(1 hunks)packages/system/cozystack-api/cozyrds/http-cache.yaml(1 hunks)packages/system/cozystack-api/cozyrds/info.yaml(1 hunks)packages/system/cozystack-api/cozyrds/ingress.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kafka.yaml(1 hunks)packages/system/cozystack-api/cozyrds/kubernetes.yaml(1 hunks)packages/system/cozystack-api/cozyrds/mysql.yaml(1 hunks)packages/system/cozystack-api/cozyrds/nats.yaml(1 hunks)packages/system/cozystack-api/cozyrds/postgres.yaml(1 hunks)packages/system/cozystack-api/cozyrds/rabbitmq.yaml(1 hunks)packages/system/cozystack-api/cozyrds/redis.yaml(1 hunks)packages/system/cozystack-api/cozyrds/seaweedfs.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tcp-balancer.yaml(1 hunks)packages/system/cozystack-api/cozyrds/tenant.yaml(1 hunks)packages/system/cozystack-api/cozyrds/virtual-machine.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-disk.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vm-instance.yaml(1 hunks)packages/system/cozystack-api/cozyrds/vpn.yaml(1 hunks)packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml(3 hunks)packages/system/dashboard/templates/keycloakclient.yaml(2 hunks)pkg/lineage/lineage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/system/cozystack-api/cozyrds/kafka.yaml
- packages/system/cozystack-api/cozyrds/vm-disk.yaml
- packages/system/cozystack-api/cozyrds/ferretdb.yaml
- packages/system/cozystack-api/cozyrds/vpn.yaml
- packages/system/cozystack-api/cozyrds/mysql.yaml
- packages/system/cozystack-api/cozyrds/tcp-balancer.yaml
- packages/system/cozystack-api/cozyrds/seaweedfs.yaml
- packages/system/cozystack-api/cozyrds/rabbitmq.yaml
- packages/system/cozystack-api/cozyrds/postgres.yaml
- packages/system/cozystack-api/cozyrds/bootbox.yaml
- packages/system/cozystack-api/cozyrds/vm-instance.yaml
- packages/system/cozystack-api/cozyrds/http-cache.yaml
- packages/system/cozystack-api/cozyrds/virtual-machine.yaml
- packages/system/cozystack-api/cozyrds/nats.yaml
- packages/system/cozystack-api/cozyrds/kubernetes.yaml
- packages/system/cozystack-api/cozyrds/bucket.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
internal/lineagecontrollerwebhook/webhook.go (1)
pkg/apis/core/v1alpha1/tenantresource_types.go (2)
TenantResourceLabelKey(3-3)TenantResourceLabelValue(4-4)
internal/lineagecontrollerwebhook/matcher.go (1)
api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionResourceSelector(120-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (13)
packages/apps/rabbitmq/templates/rabbitmq.yaml (1)
61-62: LGTM!The label addition enables resource-level selector matching for user secrets, aligning with the PR's goal of implementing name-based selectors.
packages/system/cozystack-api/cozyrds/ingress.yaml (1)
33-34: LGTM!Removing secret filtering for ingress resources is appropriate, as ingress resources typically don't require secret visibility control. This aligns with the PR's shift from label-based to resource-name-based selectors.
packages/system/cozystack-api/cozyrds/clickhouse.yaml (1)
31-34: LGTM!The templated
resourceNamesselector correctly implements name-based secret matching for ClickHouse credentials. The patternclickhouse-{{ .name }}-credentialsfollows the expected naming convention and achieves the PR's objective of enabling predictable secret selection.packages/system/cozystack-api/cozyrds/redis.yaml (1)
32-35: LGTM!The templated
resourceNamesselector correctly implements name-based secret matching for Redis authentication. The patternredis-{{ .name }}-authfollows the expected naming convention and is consistent with the implementation across other resource definitions in this PR.pkg/lineage/lineage_test.go (1)
49-49: stubMapper correctly implements AppMapper. Confirmed stubMapper is defined in pkg/lineage/mapper.go and provides the required Map(*helmv2.HelmRelease) signature matching AppMapper.packages/system/dashboard/templates/keycloakclient.yaml (1)
48-76: LGTM! Defensive capability check added.The capability guard ensures the KeycloakClient resource is only rendered when the v1.edp.epam.com/v1 API is available, preventing deployment failures when the CRD is absent.
packages/system/cozystack-api/cozyrds/etcd.yaml (1)
33-34: LGTM! Aligns with resource-level selector semantics.The shift from label-based matchLabels to empty exclude/include lists is consistent with the new resource-level selector model introduced in this PR.
packages/system/cozystack-api/cozyrds/tenant.yaml (1)
30-31: LGTM! Consistent with resource-level selector migration.The removal of matchLabels filtering and shift to empty lists aligns with the new resource selector model, consistent with similar changes across other CozyRDS definitions.
packages/extra/info/templates/dashboard-resourcemap.yaml (1)
13-17: No action needed – the Role’s resourceNames now correctly reference the Secret named{{ .Release.Namespace }}as defined in serviceaccount.yaml.packages/system/cozystack-api/cozyrds/info.yaml (1)
35-37: Resolved:.namespaceavailable in templateContext
ThecomputeLabelsfunction in internal/lineagecontrollerwebhook/webhook.go populatestemplateContext["namespace"]witho.GetNamespace(), so{{ .namespace }}is guaranteed when renderingresourceNamestemplates.internal/lineagecontrollerwebhook/webhook.go (2)
136-140: LGTM! Template context correctly populated with all documented variables.The
templateLabelsmap includes all three template variables documented in the API types (.kind,.name,.namespace), properly addressing the concern raised in previous reviews about missing.namespacesupport.
148-153: Good use of centralized constants.Replacing hardcoded label keys and values with
corev1alpha1.TenantResourceLabelKeyandcorev1alpha1.TenantResourceLabelValueimproves maintainability and consistency across the codebase.api/v1alpha1/cozystackresourcedefinitions_types.go (1)
98-126: Documentation is now accurate.The
.namespacetemplate variable mentioned in the documentation (line 106) is correctly populated in the webhook implementation (internal/lineagecontrollerwebhook/webhook.go, line 139), resolving the concern from the previous review.
| func matchName(ctx context.Context, name string, templateContext map[string]string, resourceNames []string) bool { | ||
| if resourceNames == nil { | ||
| return true | ||
| } | ||
|
|
||
| logger := log.FromContext(ctx) | ||
| for _, templateStr := range resourceNames { | ||
| tmpl, err := template.New("resourceName").Parse(templateStr) | ||
| if err != nil { | ||
| logger.Error(err, "failed to parse resource name template", "template", templateStr) | ||
| continue | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| err = tmpl.Execute(&buf, templateContext) | ||
| if err != nil { | ||
| logger.Error(err, "failed to execute resource name template", "template", templateStr, "context", templateContext) | ||
| continue | ||
| } | ||
|
|
||
| if buf.String() == name { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
Critical: Template parsing on every webhook call creates performance bottleneck.
Parsing Go templates on every webhook invocation (line 24) is inefficient and will degrade performance under load. Since CozystackResourceDefinition CRDs change infrequently, templates should be pre-parsed once during the controller's Reconcile loop and cached in runtimeConfig as *template.Template objects.
Additionally, the current error handling silently skips failed templates (lines 27, 34 with continue), making it difficult for users to debug misconfigured selectors.
Recommended solution:
- Pre-parse templates in the controller's
Reconcilemethod when loadingCozystackResourceDefinitionCRDs - Store parsed
*template.Templateobjects inruntimeConfigalongside the CRD data - Update
matchNameto accept pre-parsed templates instead of raw strings - Treat template parse failures at load time as reconciliation errors
- For execution errors, decide on a consistent failure mode (recommend fail-closed: return false)
This approach will:
- Eliminate repeated parsing overhead
- Surface template syntax errors at CRD admission/reconciliation time
- Provide clear failure semantics for users
Based on learnings: This addresses the high-priority performance concern and major error-handling issues flagged in previous reviews.
🤖 Prompt for AI Agents
internal/lineagecontrollerwebhook/matcher.go lines 17-43: matchName currently
parses templates on every webhook call which is inefficient and hides parse
errors; instead, change the controller Reconcile to pre-parse all resource name
templates into *template.Template instances and cache them on runtimeConfig
(treat parse failures as reconciliation errors), update matchName to accept a
slice of pre-parsed *template.Template (not strings), remove template.New/Parse
from this function, execute each cached template against templateContext and if
execution returns an error log it and treat it as a fail-closed condition
(return false for that match), and only return true when a template executes
successfully and equals the name; ensure runtimeConfig type and call sites are
updated accordingly.
What this PR does
This patch implements name-based selectors for
CozystackResourceDefinitions.spec.secrets. Application developers may now specify secrets that should or should not be visible to end users by specifying aresourceNamesfield with a string slice of acceptable names. This will, for instance, let developers exclude a secret likepostgres-dbname-superuserthat has a predictable name even if it does not have predictable labels. Simple templates are supported, sopostgres-{{ .name }}-superuseris also a valid entry underresourceNames.Release note
Summary by CodeRabbit
New Features
Chores