-
Notifications
You must be signed in to change notification settings - Fork 31
fix: update XAccessPolicy status when rules are skipped during translation #236
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 |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /* | ||
| Copyright The Kubernetes Authors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package fake | ||
|
|
||
| import ( | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/watch" | ||
| fakediscovery "k8s.io/client-go/discovery/fake" | ||
| "k8s.io/client-go/testing" | ||
| ) | ||
|
|
||
| // NewClientset returns a fake Clientset seeded with the given objects. | ||
| // | ||
| // Prefer this over NewSimpleClientset (deprecated by client-gen). This constructor does not call | ||
| // NewSimpleClientset so callers avoid nesting deprecated APIs. | ||
| // | ||
| // Implementation matches generated NewSimpleClientset (testing.NewObjectTracker). Upstream | ||
| // Kubernetes fake NewClientset uses testing.NewFieldManagedObjectTracker when apply configurations | ||
| // exist; regenerate this clientset with apply configs (client-gen --with-applyconfig) to match that. | ||
| func NewClientset(objects ...runtime.Object) *Clientset { | ||
| o := testing.NewObjectTracker(scheme, codecs.UniversalDecoder()) | ||
| for _, obj := range objects { | ||
| if err := o.Add(obj); err != nil { | ||
| panic(err) | ||
| } | ||
| } | ||
|
|
||
| cs := &Clientset{tracker: o} | ||
| cs.discovery = &fakediscovery.FakeDiscovery{Fake: &cs.Fake} | ||
| cs.AddReactor("*", "*", testing.ObjectReaction(o)) | ||
| cs.AddWatchReactor("*", func(action testing.Action) (handled bool, ret watch.Interface, err error) { | ||
| var opts metav1.ListOptions | ||
| if watchAction, ok := action.(testing.WatchActionImpl); ok { | ||
| opts = watchAction.ListOptions | ||
| } | ||
| gvr := action.GetResource() | ||
| ns := action.GetNamespace() | ||
| watch, err := o.Watch(gvr, ns, opts) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
| return true, watch, nil | ||
| }) | ||
|
|
||
| return cs | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "reflect" | ||
| "strings" | ||
|
|
||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| "k8s.io/apimachinery/pkg/api/meta" | ||
|
|
@@ -40,26 +41,6 @@ import ( | |
| "sigs.k8s.io/kube-agentic-networking/pkg/constants" | ||
| ) | ||
|
|
||
| // AccessPolicyTargetRefIndex is the index name for looking up AccessPolicies by target ref (namespace/name of XBackend). | ||
| const AccessPolicyTargetRefIndex = "targetRef" | ||
|
|
||
| // accessPolicyTargetRefIndexFunc indexes AccessPolicies by each XBackend targetRef (namespace/name). | ||
| // Used by the informer cache to support O(1) lookup of policies targeting a given backend. | ||
| func accessPolicyTargetRefIndexFunc(obj interface{}) ([]string, error) { | ||
| policy, ok := obj.(*agenticv0alpha0.XAccessPolicy) | ||
| if !ok { | ||
| return nil, nil | ||
| } | ||
| var keys []string | ||
| for _, targetRef := range policy.Spec.TargetRefs { | ||
| if !isXBackendTargetRef(targetRef) { | ||
| continue | ||
| } | ||
| keys = append(keys, policy.Namespace+"/"+string(targetRef.Name)) | ||
| } | ||
| return keys, nil | ||
| } | ||
|
|
||
| func (c *Controller) setupAccessPolicyEventHandlers(accessPolicyInformer agenticinformers.XAccessPolicyInformer) error { | ||
| _, err := accessPolicyInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
| AddFunc: c.onAccessPolicyAdd, | ||
|
|
@@ -331,3 +312,60 @@ func (c *Controller) updateAccessPolicyStatus(ctx context.Context, policy *agent | |
| return err | ||
| }) | ||
| } | ||
|
|
||
| // filterEmptyStrings returns a copy of ss without "" elements. | ||
| func filterEmptyStrings(ss []string) []string { | ||
| var out []string | ||
| for _, s := range ss { | ||
| if s != "" { | ||
| out = append(out, s) | ||
| } | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| func accessPolicyHasInvalidTranslationStatus(policy *agenticv0alpha0.XAccessPolicy) bool { | ||
| for _, anc := range policy.Status.Ancestors { | ||
| cond := meta.FindStatusCondition(anc.Conditions, string(agenticv0alpha0.PolicyConditionAccepted)) | ||
| if cond != nil && cond.Status == metav1.ConditionFalse && cond.Reason == string(gwapiv1.PolicyReasonInvalid) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // reconcileAccessPolicyTranslationStatus updates XAccessPolicy status when the translator skips or | ||
| // partially applies rules (Gateway API PolicyReasonInvalid), and clears that reason by re-running | ||
| // the per-target limit check when translation succeeds again. | ||
| // | ||
| // ListAccessPoliciesAttachedToGateway defines which policies are in scope for this Gateway sync. | ||
| // The translator's issues map only contains policies that reported errors this pass; it does not | ||
| // include policies that now translate cleanly but still carry Accepted=False/Invalid from a prior | ||
| // reconciliation. Listing attachment lets us update both: set Invalid when issues[nn] is non-empty, | ||
| // and clear stale Invalid (via isPolicyUnderTargetLimit) when issues omits the policy. | ||
| func (c *Controller) reconcileAccessPolicyTranslationStatus(ctx context.Context, gateway *gwapiv1.Gateway, issues map[types.NamespacedName][]string) { | ||
| attached, err := c.translator.ListAccessPoliciesAttachedToGateway(gateway) | ||
| if err != nil { | ||
| runtime.HandleError(fmt.Errorf("list AccessPolicies attached to gateway: %w", err)) | ||
| return | ||
| } | ||
| for _, ap := range attached { | ||
| policy, err := c.agentic.accessPolicyLister.XAccessPolicies(ap.Namespace).Get(ap.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. What's the goal of getting the policy object again? Is it to ensure the latest This seems almost the same as #236 (comment). I wonder if we this step is really needed. If we can confirm this builds on the informer's cache and won't issue a request to the API server for each policy object unless the generation of the resource has changed, then perhaps there's no harm in keeping it. Otherwise, I'd probably advocate for avoiding the overhead on the API server, given another reconciliation event may have been enqueued already by the time this happens. |
||
| if err != nil { | ||
| continue | ||
| } | ||
| nn := types.NamespacedName{Namespace: policy.Namespace, Name: policy.Name} | ||
| if msgs, hasErr := issues[nn]; hasErr { | ||
| msg := strings.Join(deduplicateStrings(filterEmptyStrings(msgs)), "; ") | ||
| for _, tr := range policy.Spec.TargetRefs { | ||
| if err := c.updateAccessPolicyStatus(ctx, policy, tr, false, gwapiv1.PolicyReasonInvalid, msg); err != nil { | ||
| runtime.HandleError(fmt.Errorf("update AccessPolicy %s status for translation failure: %w", nn, err)) | ||
| } | ||
| } | ||
| continue | ||
| } | ||
| if accessPolicyHasInvalidTranslationStatus(policy) { | ||
| _ = c.isPolicyUnderTargetLimit(ctx, policy) | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.