Repository navigation
[3- De-Rooting] Move serviceaccount of root shard - #4494
Merged
Merged
Conversation
kcp-ci-bot
added
release-note-none
mjudeikis
changed the title
kcp-ci-bot
added
the
needs-rebase
mjudeikis
force-pushed
the
de.root.the.root.sa.accounts
branch
from
November 2, 2026 06:12
8e0e22c to
ce14f56
Compare
kcp-ci-bot
added
size/XL
mjudeikis
changed the title
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
Contributor
Author
|
@mjudeikis-bot review this |
mjudeikis-bot
left a comment
mjudeikis-bot
left a comment
There was a problem hiding this comment.
Two verified Warnings and two Notes; no Critical found. The routing itself (index lookup, then shards credential, then the hosting shard) matches how the other proxy clients are built and is scoped per logical cluster.
Not anchored
- Warning: no test covers service account authentication through the front proxy.
pkg/proxy/serviceaccounts_test.gocovers only the lookup type against a fake shard. Nothing buildsAuthentication.ApplyTowith aServiceAccountLookup, andcmd/sharded-test-server/frontproxy.gonever passes--service-account-key-file, so no e2e reaches this path. The wiring ofServiceAccountTokenGetterandSecretsWriteris therefore unexercised. A test that sends a signed legacy token or bound token through the authenticator against two fake shards would pin it.
Checked
- Read beyond the diff:
pkg/proxy/index/index_controller.go(Lookup,BaseURL,Start),pkg/proxy/server.go(NewServer,Run, readyz wiring,/metricsmux),pkg/proxy/metrics_authorizer.go,pkg/proxy/config.go, the kcp-dev/kubernetesserviceaccount.NewClusterGetterFromClient(the old path),cmd/sharded-test-server/frontproxy.go, anddocs/content/setup/production/metrics.md. - Correctness: the old informer factory in
authentication.gowas never started, so its listers were always empty and every lookup was already a live GET. The new code adds no extra round trips. Findings are below. An unknown cluster maps to NotFound and a missing index to ServiceUnavailable (see Note on 401). - Concurrency:
clientForguardsindexandclientswithmu; no lock-order issue. Findings: theclientsmap is never pruned (Note). - kcp specifics: every call goes through
client.Cluster(cluster.Path())afterindex.Lookup. A wildcard or path-form cluster is rejected inroutedSecrets.Cluster. The shards credential and the identity round tripper are preserved throughrest.CopyConfig. No cross-workspace read without a lookup. - API and storage: no CRD or stored-format change.
--root-kubeconfigstays as a deprecated flag; see the Warning on startup. - Security: the
:rootdelegated SubjectAccessReview for the front-proxy/metricsis removed. This matches the group-based text inmetrics.md, but the PR carriesrelease-note NONE(Note). No credential reaches an error string. - Tests: the new unit test covers index-missing, known cluster, unknown cluster, patch and list-unsupported. The Authentication wiring and
Validatechange have no test (Warning above). - Build/test:
go build ./pkg/proxy/... ./cmd/...OK;go vet ./pkg/proxy/...OK;go test ./pkg/proxy/ ./pkg/proxy/options/...OK. I did not run the linter or any e2e: the sharded e2e needs a full multi-shard setup and no existing e2e enables proxy SA auth.
Contributor
Author
|
/retest |
mjudeikis
force-pushed
the
de.root.the.root.sa.accounts
branch
from
November 2, 2026 08:01
a8652cc to
8625ed9
Compare
kcp-ci-bot
added
size/XXL
mjudeikis
force-pushed
the
de.root.the.root.sa.accounts
branch
from
November 2, 2026 11:12
8625ed9 to
57c7c88
Compare
Contributor
Author
|
/retest |
ntnn
reviewed
Nov 3, 2026
Comment on lines
+50
to
+56
| type ShardRoutedServiceAccounts struct { | ||
| shardsConfig *rest.Config | ||
|
|
||
| mu sync.Mutex | ||
| index shardIndex // nil until the index controller exists | ||
| clients map[string]shardClient // by shard name | ||
| } |
Member
There was a problem hiding this comment.
I don't see any caching at all? Wouldn't that mean that every SA request would be multiple GETs to the shard behind it?
Member
There was a problem hiding this comment.
The clients are cached but not the results. I think that would be a usecase for the shardlookup package.
Comment on lines
+42
to
+46
| // fakeIndex maps logical clusters to shards and shards to base URLs. | ||
| type fakeIndex struct { | ||
| shards map[logicalcluster.Name]string | ||
| urls map[string]string | ||
| } |
Member
There was a problem hiding this comment.
Don't we already have a fake index for the front-proxy? I think its private in another package?
I'd rather extract that/make that available then reimplementing everywhere where we use an fake index for testing.
Comment on lines
+326
to
+329
| synced, found := c.shardInformersSynced[shard.Name] | ||
| if !found { | ||
| return fmt.Errorf("informers for shard %q are not started", shard.Name) | ||
| } |
Member
There was a problem hiding this comment.
We should probably at least exclude shards that are marked for maintenance etcpp instead of taking all shards.
kcp-ci-bot
added
the
needs-rebase
mjudeikis
force-pushed
the
de.root.the.root.sa.accounts
branch
from
November 3, 2026 14:03
2adb559 to
b9d45e5
Compare
kcp-ci-bot
removed
the
needs-rebase
Contributor
|
LGTM label has been added. DetailsGit tree hash: c91249d197785dd4d8d3f733734f1e2460fc5bf0 |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ntnn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
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.
Summary
Must merge after #4493
Moves root shards off root shard so making root shard "just another shard"
What Type of PR Is This?
Related Issue(s)
Fixes #
Release Notes