-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add AccountInfo reconciler with finalizer subroutine #308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
29f805c to
67250e4
Compare
Add a new AccountInfoReconciler that ensures AccountInfo resources cannot be deleted while APIBinding resources in the workspace still have the core.platform-mesh.io/apibinding-finalizer finalizer. - Add AccountInfoFinalizerSubroutine with security.platform-mesh.io/accountinfo-finalizer - Add AccountInfoReconciler using the lifecycle builder pattern - Register controller in operator.go - Add comprehensive unit tests Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
67250e4 to
a739841
Compare
…orization model Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request introduces support for the v1alpha2 API version, adding an AccountInfo reconciler and associated finalizer subroutine for multi-cluster reconciliation. The APIBinding controller is migrated from v1alpha1 to v1alpha2 types, and the authorization model generation logic is updated to use v1alpha2 structures. New build and deployment tasks are added to Taskfile.yaml, including version-aware golangci-lint installation and container image management for KCP clusters. Additionally, a kubeconfig update script is introduced for extracting and injecting certificate and authentication data. The .gitignore file is expanded to exclude development-related files. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@hack/update-kubeconfig.sh`:
- Line 13: The echo in update-kubeconfig.sh currently claims "Retrieving kind
kubeconfig and saving as runtime.yaml..." but the script does not perform that
action; either change the message to reflect the real behavior (e.g., "Checking
for kind kubeconfig..." or "Verifying kind cluster kubeconfig...") or implement
the missing step by invoking the correct command to export/save the kubeconfig
(e.g., using kind get kubeconfig --name <cluster> or kubectl config view
--minify > runtime.yaml) so the output matches the script behavior; locate the
exact echo string ("Retrieving kind kubeconfig and saving as runtime.yaml...")
and update it or add the kubeconfig retrieval command accordingly.
- Around line 49-55: The script currently runs yq -i edits on $KUBECONFIG_YAML
which will create a partial file if it doesn't exist; add a guard before the yq
eval calls that checks $KUBECONFIG_YAML exists and is non-empty (and optionally
readable), and if the check fails, print an error and exit non-zero to fail fast
instead of writing partial kubeconfig. Place this check immediately before the
block that echoes "Updating certificate-authority-data and user info..." and the
subsequent yq eval lines that modify
.clusters[0].cluster.certificate-authority-data,
.users[0].user.client-certificate-data, .users[0].user.client-key-data and
.clusters[0].cluster.server.
In `@internal/subroutine/accountinfo_finalizer.go`:
- Line 57: The code declares var apiBindings kcpapisv1alpha1.APIBindingList
which is inconsistent with the v1alpha2 usage elsewhere and will fail to list
APIBinding resources on clusters using v1alpha2; change the type to
kcpapisv1alpha2.APIBindingList (update the import if necessary) so the
declaration of apiBindings in accountinfo_finalizer.go matches
authorization_model_generation.go and the cluster List call uses the v1alpha2
APIBindingList type.
In `@Taskfile.yaml`:
- Around line 127-132: The IMAGE_TAG extraction may be empty if the
security-operator deployment is missing; update the Taskfile step that defines
IMAGE_TAG and IMAGE_NAME to validate the extracted value (IMAGE_TAG) before
using it: after the kubectl command that sets IMAGE_TAG, add a guard that checks
IMAGE_TAG is non-empty (and optionally matches a tag pattern), print a clear
error and exit non-zero if validation fails, and only proceed to construct
IMAGE_NAME and run the build commands when IMAGE_TAG passes validation;
reference the IMAGE_TAG and IMAGE_NAME variables in the Taskfile and the cmds
block to locate where to add this check.
🧹 Nitpick comments (3)
Taskfile.yaml (1)
133-140: Hardcoded temporary file path may cause issues with concurrent runs.Using
/tmp/kind-image.tarcould lead to conflicts if multiple instances of this task run simultaneously. Consider using a unique filename.Suggested improvement
if [ "{{.CONTAINER_RUNTIME}}" = "podman" ]; then - {{.CONTAINER_RUNTIME}} save {{.IMAGE_NAME}} -o /tmp/kind-image.tar - kind load image-archive /tmp/kind-image.tar --name {{.KIND_CLUSTER}} - rm -f /tmp/kind-image.tar + TMPFILE=$(mktemp /tmp/kind-image-XXXXXX.tar) + {{.CONTAINER_RUNTIME}} save {{.IMAGE_NAME}} -o "$TMPFILE" + kind load image-archive "$TMPFILE" --name {{.KIND_CLUSTER}} + rm -f "$TMPFILE" elsehack/update-kubeconfig.sh (1)
3-12: Add explicit dependency/version checks forkindandyq.Without a preflight check, a missing
kind/yqor a non‑v4yqwill fail with opaque errors. Consider validating presence andyqmajor version up front for clearer failure modes.Proposed guard
set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" SECRET_DIR="$PROJECT_ROOT/.secret" KCP_KUBECONFIG="$PROJECT_ROOT/../helm-charts/.secret/kcp/admin.kubeconfig" KCP_SERVER="https://localhost:8443/clusters/root:platform-mesh-system" KUBECONFIG_YAML="$SECRET_DIR/operator.yaml" + +for cmd in kind yq; do + if ! command -v "$cmd" >/dev/null 2>&1; then + echo "Error: required command '$cmd' not found in PATH" + exit 1 + fi +done + +# yq v4 syntax is required for `yq eval ...` +if ! yq --version | grep -qE '\bv4\.'; then + echo "Error: yq v4 is required" + exit 1 +fiinternal/subroutine/accountinfo_finalizer.go (1)
50-50: Type assertion discards the result.The pattern
_ = instance.(*accountv1alpha1.AccountInfo)performs a type assertion but discards the result. While this ensures type safety at runtime, consider using a more explicit approach or adding a comment explaining this is intentional type validation.Alternative: explicit type check with error
- _ = instance.(*accountv1alpha1.AccountInfo) + if _, ok := instance.(*accountv1alpha1.AccountInfo); !ok { + return ctrl.Result{}, errors.NewOperatorError(fmt.Errorf("expected AccountInfo, got %T", instance), false, false) + }
- Update APIBinding to v1alpha2 in accountinfo_finalizer for consistency - Add guard for missing operator.yaml in update-kubeconfig.sh - Add IMAGE_TAG validation in docker:kind task - Fix misleading status message in update-kubeconfig.sh Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
Signed-off-by: Bastian Echterhölter <bastian.echterhoelter@sap.com> On-behalf-of: @SAP <bastian.echterhoelter@sap.com>
Summary
AccountInfoReconcilerwithAccountInfoFinalizerSubroutineto manage deletion lifecycle ofAccountInforesourcessecurity.platform-mesh.io/accountinfo-finalizerthat blocksAccountInfodeletion until allAPIBindingresources withcore.platform-mesh.io/apibinding-finalizerare removed from the workspaceAPIBindingreferences to usev1alpha2API version in reconciler, authorization model subroutine, and associated testsgcilinter requirements