feat: implemented status resource routing for remote cluster mode#4579
Open
abhisheksheth28 wants to merge 1 commit into
Open
feat: implemented status resource routing for remote cluster mode#4579abhisheksheth28 wants to merge 1 commit into
abhisheksheth28 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Implements remote-cluster status-resource routing so that status.gatekeeper.sh PodStatus resources live on the management cluster (alongside the Gatekeeper pod) while all other resources continue to be reconciled on the target cluster. This enables OwnerReference-based garbage collection of PodStatus on pod restart and removes the previous orphan-cleanup workaround.
Changes:
- Adds a new
pkg/routingpackage withRoutingCacheandRoutingClientthat dispatch operations to either the management or target cluster based on GVK; wires them into the manager viaNewCache/NewClientinmain.go. - Removes the
util.SetSkipPodOwnerRef/ShouldSkipPodOwnerRefmechanism and always sets the pod OwnerReference on every PodStatus type (and deletes the corresponding skip-tests). - Updates the remote-cluster E2E workflow, docs, and design doc to reflect status resources living on the management cluster.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/routing/routing.go | New helpers: IsManagementResource (group check) and ResolveGVK. |
| pkg/routing/cache.go | RoutingCache wrapping two cache.Cache instances. |
| pkg/routing/client.go | RoutingClient routing reads/writes by GVK; Status()/SubResource() not overridden. |
| pkg/routing/*_test.go | Unit tests with fake cache/client and typed/unstructured GVK cases. |
| main.go | Builds mgmtConfig from in-cluster config and provides NewCache/NewClient factories. |
| pkg/controller/controller.go | Adds RemoteClusterEnabled() helper and drops the SetSkipPodOwnerRef(true) call. |
| pkg/util/remote_cluster*.go | Deleted (replaced by routing package). |
| apis/status/v1alpha1, v1beta1/*_types.go | Always set pod OwnerReference; removed conditional skip. |
| apis/status/v1*/.../*_test.go | Removed _SkipsOwnerRefInRemoteClusterMode tests. |
| .github/workflows/remote-cluster-e2e.yaml | Splits CRD deployment per cluster; asserts PodStatuses on management cluster only, with OwnerReferences. |
| docs/design/remote-cluster-routing.md | New design document describing the routing layer. |
| website/docs/customize-startup.md | Updates RBAC guidance and adds upgrade/migration instructions. |
Comment on lines
+86
to
+91
| go func() { | ||
| if err := r.management.Start(ctx); err != nil { | ||
| log.Error(err, "management cache failed to start") | ||
| } | ||
| }() | ||
| return r.target.Start(ctx) |
| return r.target | ||
| } | ||
|
|
||
| // cacheForGVK returns the corect cache for GVK |
Comment on lines
+194
to
+197
| // Typed object not registered in scheme, no GVK set, should fall back to target. | ||
| type unknownObj struct { | ||
| client.Object | ||
| } |
Comment on lines
+19
to
+39
| type RoutingClient struct { | ||
| // Client is the target cluster client | ||
| client.Client | ||
| management client.Client | ||
| scheme *runtime.Scheme | ||
| } | ||
|
|
||
| // Compile-time interface check. | ||
| var _ client.Client = &RoutingClient{} | ||
|
|
||
| // NewRoutingClient creates a RoutingClient. In non-remote mode same client for both target and management | ||
| func NewRoutingClient(target, management client.Client, scheme *runtime.Scheme) *RoutingClient { | ||
| if target != management { | ||
| log.Info("routing enabled: status.gatekeeper.sh resources route to management cluster client") | ||
| } | ||
| return &RoutingClient{ | ||
| Client: target, | ||
| management: management, | ||
| scheme: scheme, | ||
| } | ||
| } |
Comment on lines
+252
to
+261
| // create management cluster config once if remote cluster mode is enabled, shared by RoutingCache (reads) and RoutingClient (writes) | ||
| var mgmtConfig *rest.Config | ||
| if controller.RemoteClusterEnabled() { | ||
| var err error | ||
| mgmtConfig, err = rest.InClusterConfig() | ||
| if err != nil { | ||
| setupLog.Error(err, "management cluster in-cluster config required for --enable-remote-cluster") | ||
| return 1 | ||
| } | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4579 +/- ##
==========================================
- Coverage 54.49% 44.57% -9.93%
==========================================
Files 134 284 +150
Lines 12329 20788 +8459
==========================================
+ Hits 6719 9266 +2547
- Misses 5116 10728 +5612
- Partials 494 794 +300
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Comment on lines
+10
to
+39
| // RoutingClient implements client.Client by embedding the target client and | ||
| // overriding read and write methods to route PodStatus resources | ||
| // (status.gatekeeper.sh) to the management cluster. | ||
| // | ||
| // Get, List, and all write methods route by GVK so that PodStatus operations | ||
| // always target the management cluster regardless of how controller-runtime | ||
| // configures cache-backed reads. | ||
| // | ||
| // In non-remote mode, target and management are the same client. | ||
| type RoutingClient struct { | ||
| // Client is the target cluster client | ||
| client.Client | ||
| management client.Client | ||
| scheme *runtime.Scheme | ||
| } | ||
|
|
||
| // Compile-time interface check. | ||
| var _ client.Client = &RoutingClient{} | ||
|
|
||
| // NewRoutingClient creates a RoutingClient. In non-remote mode same client for both target and management. | ||
| func NewRoutingClient(target, management client.Client, scheme *runtime.Scheme) *RoutingClient { | ||
| if target != management { | ||
| log.Info("routing enabled: status.gatekeeper.sh resources route to management cluster client") | ||
| } | ||
| return &RoutingClient{ | ||
| Client: target, | ||
| management: management, | ||
| scheme: scheme, | ||
| } | ||
| } |
Comment on lines
+86
to
+91
| go func() { | ||
| if err := r.management.Start(ctx); err != nil { | ||
| log.Error(err, "management cache failed to start") | ||
| } | ||
| }() | ||
| return r.target.Start(ctx) |
Comment on lines
+689
to
+700
| return func(config *rest.Config, opts client.Options) (client.Client, error) { | ||
| targetClient, err := client.New(config, opts) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating target client: %w", err) | ||
| } | ||
|
|
||
| mgmtClient, err := client.New(mgmtConfig, client.Options{Scheme: scheme}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating management client: %w", err) | ||
| } | ||
| return routing.NewRoutingClient(targetClient, mgmtClient, scheme), nil | ||
| } |
| return r.target | ||
| } | ||
|
|
||
| // cacheForGVK returns the corect cache for GVK. |
Comment on lines
+252
to
+261
| // create management cluster config once if remote cluster mode is enabled, shared by RoutingCache (reads) and RoutingClient (writes) | ||
| var mgmtConfig *rest.Config | ||
| if controller.RemoteClusterEnabled() { | ||
| var err error | ||
| mgmtConfig, err = rest.InClusterConfig() | ||
| if err != nil { | ||
| setupLog.Error(err, "management cluster in-cluster config required for --enable-remote-cluster") | ||
| return 1 | ||
| } | ||
| } |
Signed-off-by: Abhishek Sheth <[email protected]>
abhisheksheth28
force-pushed
the
status-resource-routing
branch
from
0563d31 to
3c76916
Compare
Comment on lines
+80
to
+92
| // Start starts both caches. | ||
| func (r *RoutingCache) Start(ctx context.Context) error { | ||
| // If both caches are the same (non-remote mode), just start once | ||
| if r.target == r.management { | ||
| return r.target.Start(ctx) | ||
| } | ||
| go func() { | ||
| if err := r.management.Start(ctx); err != nil { | ||
| log.Error(err, "management cache failed to start") | ||
| } | ||
| }() | ||
| return r.target.Start(ctx) | ||
| } |
| return r.target | ||
| } | ||
|
|
||
| // cacheForGVK returns the corect cache for GVK. |
Comment on lines
+670
to
+675
| mgmtCache, err := cache.New(mgmtConfig, cache.Options{ | ||
| Scheme: opts.Scheme, | ||
| DefaultNamespaces: map[string]cache.Config{ | ||
| util.GetNamespace(): {}, | ||
| }, | ||
| }) |
Comment on lines
+689
to
+699
| return func(config *rest.Config, opts client.Options) (client.Client, error) { | ||
| targetClient, err := client.New(config, opts) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating target client: %w", err) | ||
| } | ||
|
|
||
| mgmtClient, err := client.New(mgmtConfig, client.Options{Scheme: scheme}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating management client: %w", err) | ||
| } | ||
| return routing.NewRoutingClient(targetClient, mgmtClient, scheme), nil |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Moves Status resources to the management cluster in remote cluster mode.
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #4381
Special notes for your reviewer:
pkg/routing/: Routing layer (RoutingCache,RoutingClient) that routesstatus.gatekeeper.shresources to the management cluster. All other resources go to the target cluster.main.go: Wires routing into the manager viaNewCache/NewClientfactory functions. Returns nil in non-remote mode (zero behavior change).apis/status/: OwnerReferences are now always set on PodStatus resources (removed the remote-mode skip since PodStatus and the pod now live on the same cluster).pkg/util/remote_cluster.go: Deleted (replaced by routing package).docs/design/remote-cluster-routing.md: Design proposal.