-
Notifications
You must be signed in to change notification settings - Fork 185
sandboxclaim fix TemplateNotFound (#444) #445
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,14 +33,18 @@ import ( | |
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller" | ||
| "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" | ||
| "sigs.k8s.io/controller-runtime/pkg/handler" | ||
| "sigs.k8s.io/controller-runtime/pkg/log" | ||
| "sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
|
|
||
| v1alpha1 "sigs.k8s.io/agent-sandbox/api/v1alpha1" | ||
| sandboxcontrollers "sigs.k8s.io/agent-sandbox/controllers" | ||
| extensionsv1alpha1 "sigs.k8s.io/agent-sandbox/extensions/api/v1alpha1" | ||
| asmetrics "sigs.k8s.io/agent-sandbox/internal/metrics" | ||
| ) | ||
|
|
||
| const templateRefIndexField = ".spec.sandboxTemplateRef.name" | ||
|
|
||
| // ErrTemplateNotFound is a sentinel error indicating a SandboxTemplate was not found. | ||
| var ErrTemplateNotFound = errors.New("SandboxTemplate not found") | ||
|
|
||
|
|
@@ -637,14 +641,52 @@ func (r *SandboxClaimReconciler) getTemplate(ctx context.Context, claim *extensi | |
|
|
||
| // SetupWithManager sets up the controller with the Manager. | ||
| func (r *SandboxClaimReconciler) SetupWithManager(mgr ctrl.Manager, concurrentWorkers int) error { | ||
| if err := mgr.GetFieldIndexer().IndexField(context.Background(), &extensionsv1alpha1.SandboxClaim{}, templateRefIndexField, func(obj client.Object) []string { | ||
| claim, ok := obj.(*extensionsv1alpha1.SandboxClaim) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| return []string{claim.Spec.TemplateRef.Name} | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to create index for SandboxClaim templateRef: %w", err) | ||
| } | ||
|
|
||
| r.MaxConcurrentReconciles = concurrentWorkers | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&extensionsv1alpha1.SandboxClaim{}). | ||
| Owns(&v1alpha1.Sandbox{}). | ||
| Watches(&extensionsv1alpha1.SandboxTemplate{}, handler.EnqueueRequestsFromMapFunc(r.findClaimsForTemplate)). | ||
| WithOptions(controller.Options{MaxConcurrentReconciles: concurrentWorkers}). | ||
| Complete(r) | ||
| } | ||
|
|
||
| // findClaimsForTemplate returns reconcile requests for all SandboxClaims | ||
| // that reference the given SandboxTemplate. This ensures that when a | ||
| // SandboxTemplate is created or updated, any claims waiting for it get reconciled. | ||
| func (r *SandboxClaimReconciler) findClaimsForTemplate(ctx context.Context, obj client.Object) []reconcile.Request { | ||
| template, ok := obj.(*extensionsv1alpha1.SandboxTemplate) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here TBH, I would probably just do a |
||
| if !ok { | ||
| return nil | ||
| } | ||
|
|
||
| claimList := &extensionsv1alpha1.SandboxClaimList{} | ||
| if err := r.List(ctx, claimList, | ||
| client.InNamespace(template.Namespace), | ||
| client.MatchingFields{templateRefIndexField: template.Name}, | ||
| ); err != nil { | ||
| log.FromContext(ctx).Error(err, "failed to list SandboxClaims for template", "template", template.Name) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Contrary to the above case, I agree with this not panicking. We don't expect it, but it's maybe "less unexpected" than getting the wrong object type. 👍 for logging so we can see if it is actually happening |
||
| return nil | ||
| } | ||
|
|
||
| requests := make([]reconcile.Request, len(claimList.Items)) | ||
| for i, claim := range claimList.Items { | ||
| requests[i] = reconcile.Request{ | ||
| NamespacedName: client.ObjectKeyFromObject(&claim), | ||
| } | ||
| } | ||
| return requests | ||
| } | ||
|
|
||
| // reconcileNetworkPolicy ensures a NetworkPolicy exists for the claimed Sandbox. | ||
| func (r *SandboxClaimReconciler) reconcileNetworkPolicy(ctx context.Context, claim *extensionsv1alpha1.SandboxClaim, template *extensionsv1alpha1.SandboxTemplate) error { | ||
| logger := log.FromContext(ctx) | ||
|
|
||
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.
I'm always wary of silently ignoring errors. I would probably at least log, and in this case panic. Given that I'm suggesting a panic, I think you could just
claim := obj.(*extensionsv1alpha1.SandboxClaim)