Skip to content

[lineage, controller] Implement name selectors - #1477

Merged
Timofei Larkin (lllamnyp) merged 3 commits into
mainfrom
feat/secretselector-by-name
Oct 1, 2025
Merged

Timofei Larkin (lllamnyp) merged 3 commits into
mainfrom
feat/secretselector-by-name

Conversation

@lllamnyp

@lllamnyp Timofei Larkin (lllamnyp) commented Sep 30, 2025 •

Copy link
Copy Markdown
Member

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 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.

Summary by CodeRabbit

  • New Features

    • Resource selectors now support exact resource-name filtering (with templating); include/exclude selectors operate at the resource level and require both label and name matches.
  • Chores

    • Many service templates switched from label-based default exclusions to empty excludes with explicit name-based includes.
    • Updated several component image tags to latest and refreshed CRD packaging/templates.
    • Standardized tenant-resource label keys/values and adjusted secret labels in manifests.

@coderabbitai

coderabbitai Bot commented Sep 30, 2025 •

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Replaces 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

Cohort / File(s) Summary
API types & deepcopy
api/v1alpha1/cozystackresourcedefinitions_types.go, api/v1alpha1/zz_generated.deepcopy.go
Replace CozystackResourceDefinitionSecrets with CozystackResourceDefinitionResources; add CozystackResourceDefinitionResourceSelector (LabelSelector + optional resourceNames[] templating); update DeepCopy implementations and spec field types.
Lineage webhook matcher & wiring
internal/lineagecontrollerwebhook/matcher.go, internal/lineagecontrollerwebhook/webhook.go
Add templated name matching (matchName), selector helpers (matchResourceToSelector*, matchResourceToExcludeInclude), thread context/templateContext through matching, and switch matching to the new ResourceSelector type and centralized tenantresource constants.
CRD schema & Helm CRD aggregation
packages/system/cozystack-controller/crds/cozystack.io_cozystackresourcedefinitions.yaml, packages/system/cozystack-controller/templates/crds/crds.yaml
Add resourceNames array to include/exclude selector schema with examples/docs; add Helm template to aggregate CRDs from crds/*.
Helm cozyrds passthrough & per-service configs
packages/system/cozystack-api/templates/cozystack-resource-definitions/cozyrds.yaml, packages/system/cozystack-api/cozyrds/*
Keep glob inclusion; convert many cozyrds secret selectors from label-based excludes to empty excludes and add explicit resourceNames includes or structured include entries where applicable.
Templates & manifest label edits
packages/apps/*/templates/*, packages/extra/monitoring/templates/alerta/alerta.yaml
Remove tenant/internal tenant labels from many Secret metadata blocks; add apps.cozystack.io/user-secret: "true" label for RabbitMQ user secrets.
Image/tag updates & values
packages/apps/*/images/*.tag, packages/system/*/values.yaml, packages/system/kubevirt-csi-node/values.yaml, packages/system/metallb/values.yaml
Update multiple image tags/digests (several moved to :latest or new digests); update Cilium, MetalLB and other component image references.
Codegen script
hack/update-codegen.sh
Change controller-gen CRD output path from templates/crds to crds.
Core constants & registry label keys
pkg/apis/core/v1alpha1/tenantresource_types.go, pkg/registry/core/*/rest.go, pkg/registry/core/*/tenantsecret*.go
Add TenantResourceLabelKey/TenantResourceLabelValue constants and update registry packages to reference them (label keys moved to internal.cozystack.io/...).
Cozy app/service manifest adjustments
packages/system/cozystack-api/cozyrds/*, packages/apps/*/templates/*
Replace many exclude label-match rules with empty lists; add explicit resourceNames includes for specific services' secrets where needed.
Tests & call-sites
pkg/lineage/lineage_test.go
Update test invocation to pass an added mapper argument to WalkOwnershipGraph.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • klinch0
  • kvaps

Poem

"I nibble templates and stitch names just right,
CRDs and secrets hop into the night.
Labels and names I gently inspect,
Resources matched — a tidy little project. 🐇"

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely conveys that the PR implements name selectors in the lineage controller, which aligns exactly with the primary change of adding resourceNames support for secret selection. It uses clear component tags, avoids unnecessary detail, and directly reflects the main objective without ambiguity.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/secretselector-by-name

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello 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 CozystackResourceDefinitions by introducing name-based selectors for managing secret visibility. Application developers can now precisely control which secrets are exposed to end-users, leveraging both traditional label matching and new name-based criteria, including dynamic name generation through Go templates. This change provides a more robust and adaptable mechanism for secret management within the Cozystack ecosystem.

Highlights

  • Name-based Selectors for Secrets: Implemented the ability to select secrets based on their names within CozystackResourceDefinitions.spec.secrets, in addition to existing label-based selection.
  • Go Template Support: The new name selectors support Go templates, allowing dynamic name matching using variables like {{ .name }} and {{ .kind }}.
  • API and CRD Updates: Introduced a new CozystackResourceDefinitionResourceSelector type to extend metav1.LabelSelector with the ResourceNames field, and updated the Custom Resource Definition (CRD) schema accordingly.
  • Webhook Logic Enhancement: Modified the webhook's matching logic to incorporate the new name-based selection and template rendering for resource visibility.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment Gemini (@gemini-code-assist) Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

Comment thread internal/lineagecontrollerwebhook/matcher.go
Comment thread internal/lineagecontrollerwebhook/matcher.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
internal/lineagecontrollerwebhook/matcher.go (2)

15-40: Template parsing performance and error handling concerns.

The matchName function logic is correct: it returns true if any template matches the name, or if resourceNames is nil (match-all behavior). However, there are two issues already raised in previous reviews:

  1. Performance: Parsing templates inside the loop on every call is inefficient for a webhook hot path.
  2. 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 the latest tag alongside a digest. Even though the digest pins the image, using latest can 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 cozystackVersion field 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 to latest@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 of latest for clearer traceability.

packages/system/dashboard/values.yaml (1)

2-6: Separate image version bumps from name selector feature
The PR replaces explicit v0.37.0-alpha.1@sha256:… tags with latest@sha256:… for openapi-ui, openapi-ui-k8s-bff, and token-proxy in packages/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.
• If latest images are required, document the necessity in the PR description.

packages/system/kubeovn/values.yaml (1)

67-67: Separate image updates from feature changes

Updating the kube-ovn image tag from v1.14.5@… to latest@… 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 latest for 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 that matchName parses 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 tags

I see multiple charts under packages/system (seaweedfs, objectstorage-controller, kubevirt-csi-node, kubeovn-plunger, dashboard, cozystack-api, etc.) using latest@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

📥 Commits

Reviewing files that changed from the base of the PR and between 6782e98 and 8c60523.

📒 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 to latest with digest pinning.

The image reference has been updated from version 0.0.0 to latest. While using latest tags 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.0 to latest while 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 for CozystackResourceDefinitions.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: Clarify latest tag usage in s3manager.tag

The file packages/system/bucket/images/s3manager.tag was updated from a fixed v0.5.0@sha256:… to latest@sha256:…. Relying on the latest tag undermines build reproducibility, complicates rollbacks, and falls outside this PR’s scope of implementing name selectors for CozystackResourceDefinitions. Please confirm:

  1. Should this image update be part of this PR or extracted into its own?
  2. What’s the rationale for switching to latest rather than pinning a specific version?
packages/extra/monitoring/images/grafana.tag (1)

1-1: Pinning image by digest overrides the tag
The reference ghcr.io/cozystack/cozystack/grafana:latest@sha256:dc27b9d7ef42e3d59667a203cb94d410c60afc119102ff1b9a081af4ea72f52c uses the digest for resolution, rendering :latest cosmetic. To track the moving “latest” image, remove the digest; to ensure stability, drop :latest and use a specific version tag.

packages/system/cilium/values.yaml (1)

17-18: Clarify image specification: digest overrides tag

  • Remove tag: latest when pinning by digest.
  • Remove the pinned digest if you intend to follow latest (not recommended for production).
  • When bumping versions, update both tag and digest together.
packages/system/cozystack-api/values.yaml (1)

2-2: Use semantic version tags for cozystack-api image

The cozystack-api image is specified as
ghcr.io/cozystack/cozystack/cozystack-api:latest@sha256:490806568820c45d2c0ae3d2fcc349dd65ac20a4500ca86ee5094e4e3f4e24fd;
because the digest pins the exact image, the latest tag 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 .tag files 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 latest tag 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 latest with 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-driver image has been updated to latest with digest sha256:7312623d..., which matches the reference in packages/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 latest with digest pinning. Using latest in 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
Confirmed packages/system/cozystack-controller/crds exists and templates/crds/crds.yaml uses .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 the Secrets field type change
In api/v1alpha1/cozystackresourcedefinitions_types.go:54, the JSON field secrets now uses the CozystackResourceDefinitionResources type instead of CozystackResourceDefinitionSecrets. 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 resourceNames field. 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 resourceNames field. The default behavior now includes all secrets (via include: [{}]), 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 resourceNames support.

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 resourceNames support, 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 resourceNames field and template matching capabilities.

packages/system/cozystack-api/cozyrds/kafka.yaml (1)

32-33: Approve: Secret visibility migration applied consistently
All cozyrds manifests in packages/system/cozystack-api/cozyrds/ now use secrets.exclude: [] and secrets.include: [{}] with no remaining matchLabels. Consider updating documentation to inform application developers of the new resourceNames templating 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 .yaml files 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: [] and include: [{}], all secrets are now included by default, which supports the new resourceNames filtering 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: [] and include: [{}], all secrets are visible by default, allowing future name-based filtering via resourceNames if 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 resourceNames feature. By specifying postgres-{{ .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 (like nats.yaml and seaweedfs.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 resourceNames as an optional array of strings, aligned with the Go type definition in api/v1alpha1/cozystackresourcedefinitions_types.go.

Also applies to: 246-252


258-284: Consistent schema and documentation for include selector.

The include selector schema mirrors the exclude selector 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 true if any selector in the array matches. The updated signature correctly passes name and ctx to the underlying matchResourceToSelector function.


62-70: Exclude/include logic correctly implements documented precedence.

The function correctly implements the documented behavior:

  1. If resource matches any exclude selector → hidden (return false)
  2. Else if resource matches any include selector → visible (return true)
  3. 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/tenantresource nor define any Secret resources—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: [] with include: [{}] matches every secret in the namespace; unlike the Postgres CRD which explicitly lists resourceNames, you should either:

  • Specify only the NATS secrets (e.g. <Release.Name>-credentials, the natsBox.contextsSecret.name, natsBox.contentsSecret.name) under include.resourceNames
  • Or confirm that exposing all secrets is intentional for NATS deployments.

Comment thread packages/extra/bootbox/images/matchbox.tag Outdated
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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Template objects.


22-32: Missing error handling for template operations.

Errors from template.New().Parse() (line 22) and tmpl.Execute() (line 29) are ignored with TODO comments. Users will have no visibility into malformed templates, making debugging very difficult. Propagate context.Context to this function so you can log these errors using log.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. Propagate context.Context to 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 using latest@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: latest while 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) to latest. While the digest pin ensures immutability, using latest as 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 resourceNames is 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 resourceNames means 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 resourceNames behavior 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8c60523 and 66004c8.

📒 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 image

The image tag was changed from 0.0.0@sha256:5335c044… to latest@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 latest with 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 to latest@sha256:… is one of several .tag files 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.exclude rule filtering secrets with apps.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 resourceNames field 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 use apps.cozystack.io/tenantresource: "false" and will now be exposed due to exclude: [] in vm-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 resourceNames field 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 exclude list with a generic include: [{}] makes all secrets visible by default. This aligns with the PR's shift to name-based resource selectors via the new resourceNames field.

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 resourceNames support.

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 resourceNames instead 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 CozystackResourceDefinitionResourceSelector type correctly embeds metav1.LabelSelector and adds the ResourceNames field with template support. The documentation clearly explains the AND semantics and provides helpful examples.


127-137: LGTM: Correct include/exclude semantics.

The CozystackResourceDefinitionResources type correctly uses pointer slices for Exclude and Include, and the documentation clearly states that exclusions take precedence. This aligns with the matchResourceToExcludeInclude implementation in matcher.go.


54-54: Verify CRD migration is complete.

The Secrets field now uses the new CozystackResourceDefinitionResources type with ResourceSelector support. 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 LabelSelector and creates an independent copy of the ResourceNames slice.


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 Exclude and Include slices, 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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66004c8 and 80fc345.

📒 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 include list. 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 explicit resourceNames for 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.TenantResourceLabelKey and TenantResourceLabelValue) 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.io domain 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 strings package is used for strings.ToLower on Line 137, and the corev1alpha1 alias 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) in templateLabels flow as templateContext into matchName and 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 generating ferretdb-{{ .name }}-credentials secrets in this repo. Please confirm that the upstream FerretDB chart actually defines secrets following that naming pattern and that .name is provided in the template context.

packages/system/cozystack-api/cozyrds/mysql.yaml (1)

32-35: Approve include selector pattern

The selector mysql-{{ .name }}-credentials correctly matches the Helm chart’s secret naming ({{ .Release.Name }}-credentials where Release.Name is mysql- + 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 secrets

Switching both exclude and include to empty lists risks changing selector semantics—if include is 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 expected resourceNames so we don’t regress behavior).

Comment thread internal/lineagecontrollerwebhook/matcher.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) on ResourceNames in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 80fc345 and cf29d1e.

📒 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 resourceNames support. 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 configuration

The 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 .name is populated in templateLabels (internal/lineagecontrollerwebhook/webhook.go:138), so the ferretdb-{{ .name }}-credentials template 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 }}-credentials correctly uses the .name variable 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 }}-credentials correctly uses the .name variable 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 }} and bucket-{{ .name }}-credentials correctly use the .name variable 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 strings import is correctly added to support strings.ToLower for the kind field in templateLabels (line 137).


21-22: LGTM! Import for centralized label constants.

The corev1alpha1 import 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 matching
  • name: 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() and templateLabels to matchResourceToExcludeInclude (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 Secrets remains unchanged while the type evolves from CozystackResourceDefinitionSecrets to CozystackResourceDefinitionResources, 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.

@lllamnyp
Timofei Larkin (lllamnyp) force-pushed the feat/secretselector-by-name branch 2 times, most recently from f2cee70 to d3e3483 Compare October 1, 2025 13:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

  3. Template security (duplicate of major-priority comment): No timeout, memory limit, or size constraints on template execution. While templateContext appears controlled, unbounded template execution in a webhook poses DoS risk.

Required fixes:

  • Pre-parse templates when CozystackResourceDefinition is loaded in the controller's Reconcile loop
  • Store parsed *template.Template in runtimeConfig
  • Pass pre-parsed templates to matchName instead of raw strings
  • Implement clear failure mode (recommend fail-closed: return false on 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 Secrets but now uses the CozystackResourceDefinitionResources type. 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 .namespace in templateLabels
Templates receive the resource’s namespace (o.GetNamespace(), line 139) but the CRD docs only document .name and .kind. Either switch to obj.GetNamespace() if you meant the application’s namespace or add .namespace to the CRD template-variable documentation to avoid confusion.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2cee70 and d3e3483.

📒 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 }}-credentials correctly references the .name variable 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-secret label 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 .namespace variable 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 the templateContext["name"] set in internal/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 strings import (line 8) is used for strings.ToLower at line 137, and the corev1alpha1 import (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.TenantResourceLabelKey and corev1alpha1.TenantResourceLabelValue is a good practice that ensures consistency across the codebase.


153-153: LGTM! Function call updated correctly.

The call to matchResourceToExcludeInclude now includes the resource name and templateLabels, 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: bytes for template buffers, context for logging, text/template for Go template parsing, cozyv1alpha1 for the ResourceSelector type, and log for 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

Comment thread api/v1alpha1/cozystackresourcedefinitions_types.go
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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 CozystackResourceDefinition is created/updated to catch syntax errors early.

Example approach: Add a validation webhook that attempts to parse each resourceNames entry 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 resourceNames field 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: array

Note: 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 exclude and include array 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

📥 Commits

Reviewing files that changed from the base of the PR and between d3e3483 and 8d50dfb.

📒 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 resourceNames selector correctly implements name-based secret matching for ClickHouse credentials. The pattern clickhouse-{{ .name }}-credentials follows 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 resourceNames selector correctly implements name-based secret matching for Redis authentication. The pattern redis-{{ .name }}-auth follows 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: .namespace available in templateContext
The computeLabels function in internal/lineagecontrollerwebhook/webhook.go populates templateContext["namespace"] with o.GetNamespace(), so {{ .namespace }} is guaranteed when rendering resourceNames templates.

internal/lineagecontrollerwebhook/webhook.go (2)

136-140: LGTM! Template context correctly populated with all documented variables.

The templateLabels map includes all three template variables documented in the API types (.kind, .name, .namespace), properly addressing the concern raised in previous reviews about missing .namespace support.


148-153: Good use of centralized constants.

Replacing hardcoded label keys and values with corev1alpha1.TenantResourceLabelKey and corev1alpha1.TenantResourceLabelValue improves maintainability and consistency across the codebase.

api/v1alpha1/cozystackresourcedefinitions_types.go (1)

98-126: Documentation is now accurate.

The .namespace template 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.

Comment on lines +17 to +43
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
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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:

  1. Pre-parse templates in the controller's Reconcile method when loading CozystackResourceDefinition CRDs
  2. Store parsed *template.Template objects in runtimeConfig alongside the CRD data
  3. Update matchName to accept pre-parsed templates instead of raw strings
  4. Treat template parse failures at load time as reconciliation errors
  5. 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.

Comment thread internal/lineagecontrollerwebhook/matcher.go
@lllamnyp
Timofei Larkin (lllamnyp) merged commit 295fdf1 into main Oct 1, 2025
20 checks passed
@lllamnyp
Timofei Larkin (lllamnyp) deleted the feat/secretselector-by-name branch October 1, 2025 14:35
@github-actions github-actions Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress label Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants