chore(server): remove the resources_config_path boot loader flow - #1767
chore(server): remove the resources_config_path boot loader flow#1767rohilsurana wants to merge 6 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe server no longer loads deprecated resource configuration at boot. Bootstrap uses only the base schema and database-backed permissions. End-to-end tests seed custom compute permissions and roles through the admin API. ChangesResource configuration removal
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1ee7ba3 to
043bb7e
Compare
Coverage Report for CI Build 31481063487Coverage decreased (-0.02%) to 48.082%Details
Uncovered Changes
Coverage Regressions17 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
16283c9 to
5c8bc3a
Compare
rohilsurana
left a comment
There was a problem hiding this comment.
Automated code review (xhigh). Findings below, most important first. Two are behavior questions worth confirming before merge; the rest are cleanups.
| } | ||
| }() | ||
|
|
||
| // load billing plans |
There was a problem hiding this comment.
Removing the loader and its deprecation warning together leaves upgraders with no signal.
An operator who still sets resources_config_path on upgrade gets nothing. The key becomes an unknown field that config loading ignores without error, and the old logger.Warn deprecation notice is gone. Any new custom permission or role in that file is never registered, so CheckPermission and CreatePolicy for those resources fail until they reseed through the admin/reconcile API. The docs (configurations.mdx, custom-resources.mdx) still describe this field. Consider keeping a boot-time warning for one release and updating those docs.
There was a problem hiding this comment.
Docs updated in 85ed587 (removed resources_config_path from the config docs and added a banner to custom-resources.mdx; a full rewrite is coming in a separate PR). The boot-time deprecation warning is intentionally removed: reconcile and the admin API own custom permissions and roles now, and this PR is gated on a deployment running reconcile first. Leaving this open for a call on whether a one-release warning is still wanted.
| custom.Permissions = filterDefaultAppNamespacePermissions(custom.Permissions) | ||
|
|
||
| defs, err := ApplyServiceDefinitionOverAZSchema(custom, GetBaseAZSchema()) | ||
| defs, err := ApplyServiceDefinitionOverAZSchema(&schema.ServiceDefinition{}, GetBaseAZSchema()) |
There was a problem hiding this comment.
Config-defined permissions lose delete protection.
BuiltinPermissions now covers only the base schema. A permission that used to come from the config file (say compute/order:create) is no longer in the builtin set, so DeletePermission accepts it where it used to reject with FailedPrecondition. That removes the permission and its SpiceDB tuples, and boot no longer recreates it. Every role and policy referencing it silently loses the grant. Please confirm this dropped protection is intended.
There was a problem hiding this comment.
Intended. Under the reconcile model custom permissions are owned by reconcile and the admin API, so config no longer contributes to the builtin set and such a permission becomes deletable. This PR is gated on reconcile owning that state first.
| @@ -269,16 +255,8 @@ func (s Service) MigrateRoles(ctx context.Context) error { | |||
| } | |||
There was a problem hiding this comment.
Config-defined roles are no longer self-healed at boot.
Predefined roles are still recreated by migrateRole, but the loop that recreated config-defined roles is gone. So a deleted custom role now stays deleted for good, and anything bound to it loses access with no boot-time recovery. Confirm this is expected now that reconcile is the seeding path.
(anchored near the diff; refers to internal/bootstrap/service.go:249)
There was a problem hiding this comment.
Intended, same reasoning as the permission delete-guard: reconcile is the seeding path now, so config-defined roles are no longer recreated at boot. Predefined roles are still self-healed by migrateRole.
Status
The deprecation window has elapsed.
resources_config_pathwas deprecated at v0.110.0 for two minor versions, and frontier is now at v0.114.0, so the removal is due. This stays a draft on the deployment gate below: it should land only once every deployment has cut over to the reconcile flow.What
Removes the
app.resources_config_pathboot loader end to end. Custom permissions and role overrides are now managed through the reconcile flow (thePermissionandRolekinds), so the server no longer reads a resource config file at startup.cmd/serve.go: drop the resource blob load and theSchemaConfigRepositorywiring.internal/bootstrap: drop theFileService(schemaConfig) dependency.MigrateSchemanow re-applies the base schema merged with the permissions already in the DB (viaAppendSchema, which preserves them) instead of reading a config file.MigrateRolesno longer creates config-defined custom roles, since reconcile owns them.BuiltinPermissions(the permission delete-guard) reports only base-schema permissions.pkg/server/config.go: remove theresources_config_pathandresources_config_path_secretfields.internal/store/blob/schema_repository.go: removed, no longer used.config/sample.config.yaml: drop the sample entries.test/e2e: the regression and smoke suites seed custom compute resources through the admin API instead of a resource config file. The shared setup lives intestbench/helper.go.Why it is safe
Existing custom permissions survive across boots because they live in the database, and
AppendSchemamerges them back into the schema. Existing role overrides survive becauseMigrateRolesis skip-if-exists. So on a server that has already migrated, this is a no-op. On one that has not, it stops seeding from the config, which is why the cutover gate matters.Gating
resources_config_path, so removal waits out the two-minor-version deprecation window. That window is now over: deprecated at v0.110.0, past the v0.112.0 removal target, current release v0.114.0.resources_config_pathfrom its own config before taking this build.Verification
Rebased onto current main (70 commits ahead of the old base) with no conflicts.
go build ./...,go vet, and unit tests forinternal/bootstrap,pkg/server, andcore/resourcepass. gofmt and vet clean.