✨ feature: implemented multiple PostCreateHooks with backward compatibility #384
Conversation
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
|
@asmit27rai - thanks, I will review - in the meantime, could you pls. set the correct PR type? The CI check is failing. It should be ✨ feature: |
| hooks = append(hooks, v1alpha1.PostCreateHookUse{ | ||
| HookName: hcp.Spec.PostCreateHook, | ||
| Vars: hcp.Spec.PostCreateHookVars, | ||
| }) |
There was a problem hiding this comment.
If the same hook is specified both as the legacy field and in the new array, it may be applied twice unless handled elsewhere. Add a check to avoid duplicate execution of hooks (e.g., by hook name).
|
|
||
| // Get hook definition | ||
| pch := &v1alpha1.PostCreateHook{} | ||
| if err := r.Client.Get(ctx, client.ObjectKey{Name: hookName}, pch); err != nil { |
There was a problem hiding this comment.
On transient errors, the function immediately returns, preventing other hooks from being processed. Would be better accumulating errors and reporting them collectively, or ensure that a failed hook (transient or not) doesn’t block other hooks from being attempted
test/e2e/setup-kubeflex.sh
Outdated
| --create-namespace \ | ||
| --version 4.10.0 \ | ||
| --set controller.service.type=NodePort \ | ||
| --set controller.hostPort.enabled=true |
There was a problem hiding this comment.
the script installs ingress-nginx twice: once via Helm and once via kubectl apply. This may lead to race conditions or resource conflicts. Use only one method to install ingress-nginx.
|
Is there a documentation update that goes along with the Go code change? |
|
@asmit27rai when multiple hooks are listed... is there a consistent order by which they are executed? perhaps the order by which they are listed? |
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
Yes, hooks are executed in the exact order they are listed:
|
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
|
To be fully clear... this older YAML: postCreateHook: wds
postCreateHookVars:
ITSName: "{{ $cp.ITSName | default "" }}"
APIGroups: "{{ $cp.APIGroups | default "" }}"could be changed to this YAML after this PR merges: postCreateHooks:
- hookName: wds
vars:
ITSName: "{{ $cp.ITSName | default "" }}"
APIGroups: "{{ $cp.APIGroups | default "" }}"
|
|
Since the new |
The new type Var struct {
Name string `json:"name"`
Value string `json:"value"`
} |
|
@asmit27rai thank you for the clarification. |
|
Also one think is that! For context: kubestellar/kubestellar#2935 |
Signed-off-by: asmit27rai <raiasmit10@gmail.com>
@francostellari I added global/shared variable. |
|
/lgtm |
|
LGTM label has been added. Git tree hash: 8b8a5e7a8c802a67e798da68eb7dc7014f20b91e
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pdettori The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughThe changes introduce support for multiple post-creation hooks in the control plane specification and its CRD schema. The legacy single-hook fields are deprecated, and a new structure allows specifying multiple hooks with individualized variables and optional global variables. The post-create hook reconciliation logic is refactored to process multiple hooks in order. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller
participant API Server
participant HookTemplate
User->>API Server: Create/Update ControlPlane (with multiple PostCreateHooks)
API Server->>Controller: Notify of ControlPlane change
loop For each PostCreateHook in order
Controller->>API Server: Fetch PostCreateHook definition
Controller->>HookTemplate: Apply hook (merge default, global, user, system vars)
HookTemplate-->>Controller: Result (success/failure)
Controller->>API Server: Update ControlPlane status for applied hook
end
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
test/e2e/cleanup.sh (1)
22-26: Redirection sequence is redundant & shell options relaxed.
&> /dev/nullalready redirects both stdout and stderr; chaining another2>&1afterwards is superfluous:-kubectl config delete-context cp1 &> /dev/null 2>&1 || true +kubectl config delete-context cp1 &> /dev/null || trueSame for the other lines.
Side note: this script removed
set -e, so silent failures earlier in the file now go unnoticed. Keepingset -eand selectively using|| true(as you already do) provides clearer intent.api/v1alpha1/controlplane_types.go (2)
35-39: Mark deprecated fields with// +optionaland comment tags.
PostCreateHookandPostCreateHookVarsare flagged as deprecated but not explicitly marked optional via kubebuilder comments. To avoid validation noise in generated CRDs, append// +optional:-// Deprecated: Use PostCreateHooks instead -PostCreateHook *string `json:"postCreateHook,omitempty"` +// Deprecated: Use PostCreateHooks instead +// +optional +PostCreateHook *string `json:"postCreateHook,omitempty"`Same for
PostCreateHookVars.
40-45: Add// +optionalto new slice fields for consistency.
PostCreateHooksis omitempty-tagged but misses the kubebuilder+optionalmarker, whereasGlobalVarshas it. This keeps CRD generation symmetric and avoids unexpected “required” behaviour from some tooling.-// PostCreateHooks specifies multiple post-creation hooks to execute -PostCreateHooks []PostCreateHookUse `json:"postCreateHooks,omitempty"` +// PostCreateHooks specifies multiple post-creation hooks to execute +// +optional +PostCreateHooks []PostCreateHookUse `json:"postCreateHooks,omitempty"`config/crd/bases/tenancy.kflex.kubestellar.org_controlplanes.yaml (1)
86-115: Schema tightening forglobalVars/postCreateHooks.
globalVarsallows arbitrary key/value pairs – good – but consider adding
x-kubernetes-preserve-unknown-fields: falseto prevent nested objects (JSON schema default is permissive).For
postCreateHooksitems, setx-kubernetes-list-type: mapandx-kubernetes-list-map-keys: ["hookName"]to enforce uniqueness on the API side, mirroring your “skip duplicates” controller logic.These minor schema hints move validation closer to runtime expectations.
pkg/reconcilers/shared/postcreate_hook.go (1)
48-59: Address static-analysis warnings for deprecated fieldsThe legacy fields are intentionally referenced for backward-compatibility, but
staticcheck(SA1019) will keep flagging these lines.
Add a scoped nolint comment to silence the warning and avoid cluttering CI noise://nolint:staticcheck // needed for backward-compat until <remove-date> if hcp.Spec.PostCreateHook != nil { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/v1alpha1/controlplane_types.go(1 hunks)config/crd/bases/tenancy.kflex.kubestellar.org_controlplanes.yaml(1 hunks)config/crd/bases/tenancy.kflex.kubestellar.org_postcreatehooks.yaml(1 hunks)pkg/reconcilers/shared/postcreate_hook.go(1 hunks)test/e2e/cleanup.sh(1 hunks)test/e2e/setup-kubeflex.sh(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
pkg/reconcilers/shared/postcreate_hook.go
53-53: SA1019: hcp.Spec.PostCreateHook is deprecated: Use PostCreateHooks instead
(staticcheck)
54-54: SA1019: hcp.Spec.PostCreateHook is deprecated: Use PostCreateHooks instead
(staticcheck)
56-56: SA1019: hcp.Spec.PostCreateHook is deprecated: Use PostCreateHooks instead
(staticcheck)
57-57: SA1019: hcp.Spec.PostCreateHookVars is deprecated: Use PostCreateHooks instead
(staticcheck)
🔇 Additional comments (2)
test/e2e/setup-kubeflex.sh (1)
26-29: ```shell
#!/usr/bin/env bashLocate Helm chart definitions
echo "Chart.yaml files found:"
fd -t f Chart.yaml || trueInspect Chart.yaml contents for dependencies
for chart in $(fd -t f Chart.yaml); do
echo -e "\n=== $chart ==="
sed -n '1,200p' "$chart" || true
doneSearch for any ingress-nginx mentions in chart templates or values
echo -e "\nSearching for ingress-nginx references in the repo:"
grep -R "ingress-nginx" -n . || true</details> <details> <summary>config/crd/bases/tenancy.kflex.kubestellar.org_postcreatehooks.yaml (1)</summary> `60-70`: **👍 Description relocation improves readability.** Moving the `Var` description inside the item block matches common CRD style and helps doc generators. No further issues spotted. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| type PostCreateHookUse struct { | ||
| // Name of the PostCreateHook resource to execute | ||
| HookName *string `json:"hookName"` | ||
| // Variables to pass to the hook template | ||
| Vars map[string]string `json:"vars,omitempty"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid pointer for required hookName.
hookName is declared required in the CRD, yet the Go struct uses *string.
A plain string is simpler – no nil checks, no chance of zero-value being marshalled incorrectly:
-// Name of the PostCreateHook resource to execute
-HookName *string `json:"hookName"`
+// Name of the PostCreateHook resource to execute
+HookName string `json:"hookName"`Unless you have a compelling reason to distinguish “unset” vs “set to empty string”, prefer the value type.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type PostCreateHookUse struct { | |
| // Name of the PostCreateHook resource to execute | |
| HookName *string `json:"hookName"` | |
| // Variables to pass to the hook template | |
| Vars map[string]string `json:"vars,omitempty"` | |
| } | |
| type PostCreateHookUse struct { | |
| // Name of the PostCreateHook resource to execute | |
| HookName string `json:"hookName"` | |
| // Variables to pass to the hook template | |
| Vars map[string]string `json:"vars,omitempty"` | |
| } |
🤖 Prompt for AI Agents
In api/v1alpha1/controlplane_types.go between lines 47 and 52, change the
HookName field from a pointer to a plain string type because it is required in
the CRD. This avoids unnecessary nil checks and ensures the field is always set,
simplifying marshaling and usage. Replace *string with string for HookName and
update any related code accordingly.
| // Update status | ||
| if hcp.Status.PostCreateHooks == nil { | ||
| hcp.Status.PostCreateHooks = make(map[string]bool) | ||
| } | ||
| hcp.Status.PostCreateHooks[hookName] = true | ||
| if err := r.Client.Status().Update(ctx, hcp); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to update status for hook %s: %w", hookName, err)) | ||
| // Break on status update error to prevent inconsistent state | ||
| break | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Retry status update on conflicts
Status().Update can fail with Conflict when another controller touches the object.
Instead of aborting the entire reconciliation, wrap the update in retry.RetryOnConflict; only give up on non-conflict errors:
err := retry.RetryOnConflict(retry.DefaultBackoff, func() error {
return r.Client.Status().Update(ctx, hcp)
})
if err != nil {
errs = append(errs, fmt.Errorf("failed to update status for hook %s: %w", hookName, err))
break
}This prevents unnecessary requeues caused by transient resource-version races.
🤖 Prompt for AI Agents
In pkg/reconcilers/shared/postcreate_hook.go around lines 128 to 137, the status
update using r.Client.Status().Update can fail with a Conflict error if another
controller modifies the object concurrently. To handle this, wrap the update
call inside retry.RetryOnConflict with retry.DefaultBackoff to retry on conflict
errors. Only append the error and break the loop if the error is not a conflict
after retries. Replace the direct update call with this retry logic to avoid
unnecessary reconciliation aborts due to transient conflicts.
| var errs []error | ||
|
|
||
| for _, hook := range hooks { | ||
| hookName := *hook.HookName | ||
|
|
||
| // Skip already applied hooks | ||
| if hcp.Status.PostCreateHooks != nil && hcp.Status.PostCreateHooks[hookName] { | ||
| continue | ||
| } | ||
|
|
||
| logger.Info("Processing post-create hook", "hook", hookName) | ||
|
|
||
| // Get hook definition | ||
| pch := &v1alpha1.PostCreateHook{} | ||
| if err := r.Client.Get(ctx, client.ObjectKey{Name: hookName}, pch); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to get PostCreateHook %s: %w", hookName, err)) | ||
| continue | ||
| } | ||
|
|
||
| // Build variables with precedence: defaults -> global -> user vars -> system | ||
| vars := make(map[string]interface{}) | ||
|
|
||
| // 1. Default vars from hook spec | ||
| for _, dv := range pch.Spec.DefaultVars { | ||
| vars[dv.Name] = dv.Value | ||
| } | ||
|
|
||
| // 2. Global variables from control plane | ||
| for key, val := range hcp.Spec.GlobalVars { | ||
| vars[key] = val | ||
| } | ||
|
|
||
| // 3. User-provided vars from hook use | ||
| for key, val := range hook.Vars { | ||
| vars[key] = val | ||
| } | ||
|
|
||
| // 4. System variables | ||
| vars["Namespace"] = namespace | ||
| vars["ControlPlaneName"] = hcp.Name | ||
| vars["HookName"] = hookName | ||
|
|
||
| // Apply hook templates | ||
| if err := applyPostCreateHook(ctx, r.ClientSet, r.DynamicClient, pch, vars, hcp); err != nil { | ||
| if util.IsTransientError(err) { | ||
| errs = append(errs, fmt.Errorf("transient error applying hook %s: %w", hookName, err)) | ||
| } else { | ||
| logger.Error(err, "Permanent error applying post-create hook", "hook", hookName) | ||
| errs = append(errs, fmt.Errorf("permanent error applying hook %s: %w", hookName, err)) | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| // Update status | ||
| if hcp.Status.PostCreateHooks == nil { | ||
| hcp.Status.PostCreateHooks = make(map[string]bool) | ||
| } | ||
| hcp.Status.PostCreateHooks[hookName] = true | ||
| if err := r.Client.Status().Update(ctx, hcp); err != nil { | ||
| errs = append(errs, fmt.Errorf("failed to update status for hook %s: %w", hookName, err)) | ||
| // Break on status update error to prevent inconsistent state | ||
| break | ||
| } | ||
|
|
||
| // Propagate labels | ||
| if err := propagateLabels(pch, hcp, r.Client); err != nil { | ||
| logger.Error(err, "Failed to propagate labels from hook", "hook", hookName) | ||
| } | ||
| logger.Error(err, "Failed to apply post-create hook", "hook", *hcp.Spec.PostCreateHook) | ||
| } | ||
|
|
||
| // Update status if hook was successfully applied | ||
| if hcp.Status.PostCreateHooks == nil { | ||
| hcp.Status.PostCreateHooks = make(map[string]bool) | ||
| } | ||
| hcp.Status.PostCreateHooks[*hcp.Spec.PostCreateHook] = true | ||
|
|
||
| if err := r.Client.Status().Update(context.TODO(), hcp, &client.SubResourceUpdateOptions{}); err != nil { | ||
| return fmt.Errorf("failed to update control plane status: %w", err) | ||
| if len(errs) > 0 { | ||
| return fmt.Errorf("errors processing hooks: %v", errs) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use utilerrors.NewAggregate for cleaner multi-error handling
Collecting errors into a []error slice and then wrapping it with fmt.Errorf("%v", errs) collapses context and produces unreadable output.
K8s provides an aggregator tailor-made for this:
-import "fmt"
+import (
+ "fmt"
+ utilerrors "k8s.io/apimachinery/pkg/util/errors"
+)
...
-var errs []error
+var errs []error
...
-if len(errs) > 0 {
- return fmt.Errorf("errors processing hooks: %v", errs)
+if len(errs) > 0 {
+ return utilerrors.NewAggregate(errs)
}This keeps each underlying error intact and plays nicely with callers that want to inspect individual causes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var errs []error | |
| for _, hook := range hooks { | |
| hookName := *hook.HookName | |
| // Skip already applied hooks | |
| if hcp.Status.PostCreateHooks != nil && hcp.Status.PostCreateHooks[hookName] { | |
| continue | |
| } | |
| logger.Info("Processing post-create hook", "hook", hookName) | |
| // Get hook definition | |
| pch := &v1alpha1.PostCreateHook{} | |
| if err := r.Client.Get(ctx, client.ObjectKey{Name: hookName}, pch); err != nil { | |
| errs = append(errs, fmt.Errorf("failed to get PostCreateHook %s: %w", hookName, err)) | |
| continue | |
| } | |
| // Build variables with precedence: defaults -> global -> user vars -> system | |
| vars := make(map[string]interface{}) | |
| // 1. Default vars from hook spec | |
| for _, dv := range pch.Spec.DefaultVars { | |
| vars[dv.Name] = dv.Value | |
| } | |
| // 2. Global variables from control plane | |
| for key, val := range hcp.Spec.GlobalVars { | |
| vars[key] = val | |
| } | |
| // 3. User-provided vars from hook use | |
| for key, val := range hook.Vars { | |
| vars[key] = val | |
| } | |
| // 4. System variables | |
| vars["Namespace"] = namespace | |
| vars["ControlPlaneName"] = hcp.Name | |
| vars["HookName"] = hookName | |
| // Apply hook templates | |
| if err := applyPostCreateHook(ctx, r.ClientSet, r.DynamicClient, pch, vars, hcp); err != nil { | |
| if util.IsTransientError(err) { | |
| errs = append(errs, fmt.Errorf("transient error applying hook %s: %w", hookName, err)) | |
| } else { | |
| logger.Error(err, "Permanent error applying post-create hook", "hook", hookName) | |
| errs = append(errs, fmt.Errorf("permanent error applying hook %s: %w", hookName, err)) | |
| } | |
| continue | |
| } | |
| // Update status | |
| if hcp.Status.PostCreateHooks == nil { | |
| hcp.Status.PostCreateHooks = make(map[string]bool) | |
| } | |
| hcp.Status.PostCreateHooks[hookName] = true | |
| if err := r.Client.Status().Update(ctx, hcp); err != nil { | |
| errs = append(errs, fmt.Errorf("failed to update status for hook %s: %w", hookName, err)) | |
| // Break on status update error to prevent inconsistent state | |
| break | |
| } | |
| // Propagate labels | |
| if err := propagateLabels(pch, hcp, r.Client); err != nil { | |
| logger.Error(err, "Failed to propagate labels from hook", "hook", hookName) | |
| } | |
| logger.Error(err, "Failed to apply post-create hook", "hook", *hcp.Spec.PostCreateHook) | |
| } | |
| // Update status if hook was successfully applied | |
| if hcp.Status.PostCreateHooks == nil { | |
| hcp.Status.PostCreateHooks = make(map[string]bool) | |
| } | |
| hcp.Status.PostCreateHooks[*hcp.Spec.PostCreateHook] = true | |
| if err := r.Client.Status().Update(context.TODO(), hcp, &client.SubResourceUpdateOptions{}); err != nil { | |
| return fmt.Errorf("failed to update control plane status: %w", err) | |
| if len(errs) > 0 { | |
| return fmt.Errorf("errors processing hooks: %v", errs) | |
| } | |
| // At the top of pkg/reconcilers/shared/postcreate_hook.go | |
| -import "fmt" | |
| +import ( | |
| + "fmt" | |
| + utilerrors "k8s.io/apimachinery/pkg/util/errors" | |
| +) | |
| ... | |
| // Error accumulator | |
| var errs []error | |
| for _, hook := range hooks { | |
| // (all existing per-hook logic unchanged) | |
| } | |
| - if len(errs) > 0 { | |
| - return fmt.Errorf("errors processing hooks: %v", errs) | |
| - } | |
| + if len(errs) > 0 { | |
| + return utilerrors.NewAggregate(errs) | |
| + } |
🤖 Prompt for AI Agents
In pkg/reconcilers/shared/postcreate_hook.go around lines 75 to 147, replace the
current error aggregation approach that collects errors in a []error slice and
returns them wrapped with fmt.Errorf. Instead, use utilerrors.NewAggregate to
aggregate the errors. This change preserves each individual error's context and
improves readability and error inspection. Import the utilerrors package if not
already imported, and at the end of the hook processing loop, return
utilerrors.NewAggregate(errs) when errs is not empty.
Summary
In
api/v1alpha1/controlplane_types.goIn
pkg/reconcilers/shared/postcreate_hook.goBackward Compatibility Strategy
Old Fields: Continue to work as before but are marked deprecated.
New Fields: Allow multiple hooks via
PostCreateHooks.Reconciler: Processes both old and new fields, merging them into a single list.
This ensures existing users using
postCreateHookandpostCreateHookVarsare unaffected, while new users can adoptpostCreateHooks. The CLI can be updated separately to support multiple hooks via new flags while maintaining backward compatibility with the old ones.Fixes #225
Summary by CodeRabbit