Repository navigation
Revert "[cozystack-controller] Ancestor tracking webhook" - #1425
Conversation
WalkthroughThis PR removes the LineageControllerWebhook feature and its supporting lineage library, eliminates webhook-related manifests and wiring, and updates the controller deployment to run without a mutating webhook. Controller setup code that registered the webhook/controller is deleted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant KAS as K8s API Server
participant Ctrl as cozystack-controller
participant WH as (Removed) Mutating Webhook
rect rgba(230,245,255,0.6)
note over User,KAS: Previous flow (before this change)
User->>KAS: Create Pod/Secret/Service/PVC
KAS->>WH: AdmissionReview /mutate-lineage
WH->>Ctrl: (internal) Lineage mapping & ownership graph
WH-->>KAS: Patch/Allow response
KAS-->>User: Object admitted (possibly mutated)
end
rect rgba(245,235,235,0.6)
note over User,KAS: New flow (after this change)
User->>KAS: Create Pod/Secret/Service/PVC
KAS-->>User: Object admitted (no lineage mutation)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello Andrei Kvapil (@kvaps), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request performs a full reversion of the "Ancestor tracking webhook" feature. It systematically removes all components introduced by the original change, including the Go code responsible for the webhook's logic and its integration into the controller, as well as all Kubernetes deployment artifacts like Cert-Manager configurations, the MutatingWebhookConfiguration, and the webhook service. This ensures a complete rollback of the feature from the cozystack-controller.
Highlights
- Full Reversion of Ancestor Tracking Webhook: This PR completely undoes the implementation of the "Ancestor tracking webhook" feature, rolling back all associated code and configurations.
- Code Removal: All Go source code related to the
lineagecontrollerwebhookpackage and thepkg/lineagepackage, including its controller, webhook logic, types, and configuration, has been removed. - Kubernetes Manifest Deletion: All associated Kubernetes manifests, such as Cert-Manager resources, the MutatingWebhookConfiguration, and the Service for the webhook, have been deleted.
- Deployment Configuration Cleanup: The main controller deployment configuration has been updated to remove webhook-specific port, volume mounts, and volume definitions.
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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request reverts the 'Ancestor tracking webhook' feature. The changes correctly remove all code and configuration related to this feature, including the lineagecontrollerwebhook and pkg/lineage packages, and associated Kubernetes manifests. The revert appears clean and complete. I have one suggestion regarding a hardcoded namespace in deployment.yaml to improve the Helm chart's flexibility, which seems to be an issue this revert is re-introducing.
| kind: Deployment | ||
| metadata: | ||
| name: cozystack-controller | ||
| namespace: cozy-system |
There was a problem hiding this comment.
The namespace is hardcoded to cozy-system. While some other components in the system seem to rely on this namespace, it's a best practice in Helm charts to use {{ .Release.Namespace }} to allow for flexible deployment into different namespaces. The removed cert-manager.yaml and mutatingwebhookconfiguration.yaml files used {{ .Release.Namespace }}, which suggests that using the release namespace is the intended pattern. For consistency and to avoid potential deployment issues, consider using the release namespace here.
namespace: {{ .Release.Namespace }}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/cozystack-controller/main.go(0 hunks)internal/lineagecontrollerwebhook/config.go(0 hunks)internal/lineagecontrollerwebhook/controller.go(0 hunks)internal/lineagecontrollerwebhook/types.go(0 hunks)internal/lineagecontrollerwebhook/webhook.go(0 hunks)packages/system/cozystack-controller/templates/certmanager.yaml(0 hunks)packages/system/cozystack-controller/templates/deployment.yaml(1 hunks)packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml(0 hunks)packages/system/cozystack-controller/templates/service.yaml(0 hunks)pkg/lineage/lineage.go(0 hunks)pkg/lineage/lineage_test.go(0 hunks)pkg/lineage/mapper.go(0 hunks)
💤 Files with no reviewable changes (11)
- pkg/lineage/lineage_test.go
- packages/system/cozystack-controller/templates/service.yaml
- internal/lineagecontrollerwebhook/controller.go
- internal/lineagecontrollerwebhook/types.go
- packages/system/cozystack-controller/templates/mutatingwebhookconfiguration.yaml
- cmd/cozystack-controller/main.go
- packages/system/cozystack-controller/templates/certmanager.yaml
- internal/lineagecontrollerwebhook/config.go
- internal/lineagecontrollerwebhook/webhook.go
- pkg/lineage/lineage.go
- pkg/lineage/mapper.go
⏰ 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
| kind: Deployment | ||
| metadata: | ||
| name: cozystack-controller | ||
| namespace: cozy-system |
There was a problem hiding this comment.
🧩 Analysis chain
Avoid hard-coding namespace in Helm templates; use release namespace or omit.
Pinning metadata.namespace: cozy-system can desync this Deployment from its ServiceAccount/RBAC if those resources are created in .Release.Namespace (default Helm behavior). This can cause pod startup failures (“serviceaccount not found”) when installing into any namespace other than cozy-system, or when other templates don’t hard-code the same namespace.
Two safer options:
- Prefer omit: let Helm set the namespace from the release.
- Or template it explicitly.
- namespace: cozy-system
+ # namespace resolved by Helm release; avoid hard-coding to prevent drift
+ {{- /* omit explicit namespace or, if required: */ -}}
+ {{- /* namespace: {{ .Release.Namespace }} */ -}}Verify consistency across the chart:
🏁 Script executed:
#!/bin/bash
# Check if ServiceAccount/RBAC are hard-coded to a different namespace or left implicit
rg -n -C2 -g 'packages/system/cozystack-controller/templates/**' -e 'kind:\s*ServiceAccount|RoleBinding|ClusterRoleBinding|Deployment' -e 'namespace:\s*cozy-system'
# List all explicit namespace declarations in this chart
rg -n -C1 -g 'packages/system/cozystack-controller/templates/**' -e '^\s*namespace:\s*'
# Look for lingering webhook artifacts (ports, cert mounts, services) after the revert
rg -n -C2 -g 'packages/system/cozystack-controller/templates/**' -e 'webhook|9443|serving-certs|webhook-certs|MutatingWebhookConfiguration|cert-manager'Length of output: 5093
Remove hard-coded namespace; use the release namespace or template it
Hard-coded "namespace: cozy-system" exists across templates and can desync ServiceAccount/RBAC, causing "serviceaccount not found" when installing the chart into a different namespace.
- Fix namespaced resources (omit metadata.namespace so Helm uses the release namespace, or template to .Release.Namespace if you must): packages/system/cozystack-controller/templates/deployment.yaml (line 5), sa.yaml (line 5), role.yaml (lines 5, 17, 25).
- Fix cluster-scoped RBAC subjects (do not omit—template the subject namespace): packages/system/cozystack-controller/templates/rbac-bind.yaml (line 12) — replace with {{ .Release.Namespace }}.
Suggested edits (examples):
For namespaced resources:
- namespace: cozy-system
-
omit explicit namespace so Helm uses the release namespace
- {{- /* if required: namespace: "{{ .Release.Namespace }}" */ -}}
For ClusterRoleBinding subject namespace:
- namespace: cozy-system
- namespace: {{ .Release.Namespace }}
🤖 Prompt for AI Agents
packages/system/cozystack-controller/templates/deployment.yaml around line 5:
remove the hard-coded namespace entry so Helm will install into the release
namespace; either delete the metadata.namespace line entirely (preferred for
namespaced resources) or replace it with a template using .Release.Namespace if
an explicit field is required, and apply the same change to sa.yaml and
role.yaml; for cluster-scoped RBAC subjects (rbac-bind.yaml line 12) do not omit
namespace—template the subject namespace as {{ .Release.Namespace }} so bindings
point to the correct release namespace.
1 participant
Reverts #1400
Summary by CodeRabbit