-
Notifications
You must be signed in to change notification settings - Fork 631
clear stale policy status after the policy is orphaned #12883
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
Changes from all commits
29f4143
bdcefcf
5e1cdb6
b6b27ea
a0d74fe
58bf6cd
5b8199d
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 |
|---|---|---|
|
|
@@ -257,6 +257,13 @@ func (s *ProxySyncer) Init(ctx context.Context, krtopts krtutil.KrtOptions) { | |
| s.backendPolicyReport = krt.NewSingleton(func(kctx krt.HandlerContext) *report { | ||
| backends := krt.Fetch(kctx, finalBackendsWithPolicyStatus) | ||
| merged := GenerateBackendPolicyReport(backends) | ||
|
|
||
| for _, plugin := range s.plugins.ContributesPolicies { | ||
| if plugin.ProcessPolicyStaleStatusMarkers != nil && plugin.ProcessBackend != nil { | ||
| plugin.ProcessPolicyStaleStatusMarkers(kctx, &merged) | ||
| } | ||
| } | ||
|
|
||
| return &report{merged} | ||
| }, krtopts.ToOptions("BackendsPolicyReport")...) | ||
|
|
||
|
|
@@ -271,6 +278,12 @@ func (s *ProxySyncer) Init(ctx context.Context, krtopts krtutil.KrtOptions) { | |
| objStatus := krt.Fetch(kctx, s.commonCols.Routes.GetHTTPRouteStatusMarkers()) | ||
| s.commonCols.Routes.ProcessHTTPRouteStatusMarkers(objStatus, merged) | ||
|
|
||
| for _, plugin := range s.plugins.ContributesPolicies { | ||
| if plugin.ProcessPolicyStaleStatusMarkers != nil && plugin.ProcessBackend == nil { | ||
|
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. is this done to exclude backend policy plugins?
Contributor
Author
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. Yes, backend policy and traffic policies uses different status reporting queue
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. I have a small concern with this approach, if we have plugins that contribute both backends and policy will we potentially lose this functionality? Wondering how we can make this more explicit. One option would be to have an explicit What do you think? Is that overkill?
Contributor
Author
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. I think the functionality will be fine as long as we make sure to report / generate its status in either backend or general status report, and given its gonna contribute to backend most likely it will be reported as a part of backend status reporting, which is what I have here. We might need to have explicit processBackendPolicyMarkers vs processPolicyMarkers and detailed if check in that case, but right now I didn't want to add more fields into the plugin unless unnecessary, so I went with a check here. Not entirely a fan of checking the ProcessBackend either so if you think have separate is better we can do that
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. I agree with your assessment. Given it's not likely we'll hit this specific scenario anytime soon, I think we can move forward with this as-is. |
||
| plugin.ProcessPolicyStaleStatusMarkers(kctx, &merged) | ||
| } | ||
| } | ||
|
|
||
| return &report{merged} | ||
| }) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.