Adding design for automatically registering core RRTs#11610
Adding design for automatically registering core RRTs#11610kachawla wants to merge 1 commit intoradius-project:mainfrom
Conversation
Signed-off-by: Karishma Chawla <kachawla@microsoft.com>
kachawla
force-pushed
the
rrt-auto-registration
branch
from
f165ae2 to
4177f2d
Compare
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
There was a problem hiding this comment.
Pull request overview
Adds a design note proposing automated default registration of Radius resource types by embedding selected manifests from resource-types-contrib into the Radius binary and registering them during UCP initialization.
Changes:
- Introduces a design for using
resource-types-contribas a Go module dependency and embedding default manifests viago:embed. - Proposes a centralized
defaults.yaml+go generateworkflow to control which manifests ship as defaults. - Describes initializer/runtime behavior, error handling, and a test/CI validation plan for generated embed lists.
| │ defaults.yaml ─── lists paths ──► go generate │ | ||
| │ │ │ | ||
| │ Compute/containers/containers.yaml ▼ │ | ||
| │ Compute/routes/routes.yaml manifests_gen.go │ | ||
| │ Security/secrets/secrets.yaml (//go:embed directives) │ |
There was a problem hiding this comment.
The architecture diagram shows defaults.yaml “lists paths” and then examples of file paths (e.g., Compute/containers/containers.yaml), but later sections (e.g., the defaults.yaml format example) use logical resource type names like Radius.Compute/containers. Pick one representation (paths vs names) and update the diagram text/examples to match the proposed approach so the embedding/generation workflow is unambiguous.
|
|
||
| #### Option 2: Central `defaults.yaml` + `go generate` (Proposed) | ||
|
|
||
| A `defaults.yaml` file at the repo root lists which manifest paths should be default-registered. A `go generate` script reads this file and produces `manifests_gen.go` with `//go:embed` directives for exactly those files (plus `defaults.yaml` itself). At runtime, `RegisterFS` reads `defaults.yaml` from the embedded FS to know which paths to load. |
There was a problem hiding this comment.
This section says defaults.yaml “lists which manifest paths should be default-registered”, but the proposed defaults.yaml format later uses logical resource type identifiers (e.g., Radius.Compute/containers) and the Open Questions discuss resolving names → paths. Clarify whether the proposed design is path-based or name-based here (and align wording throughout).
| A `defaults.yaml` file at the repo root lists which manifest paths should be default-registered. A `go generate` script reads this file and produces `manifests_gen.go` with `//go:embed` directives for exactly those files (plus `defaults.yaml` itself). At runtime, `RegisterFS` reads `defaults.yaml` from the embedded FS to know which paths to load. | |
| A `defaults.yaml` file at the repo root lists which resource type identifiers should be default-registered (for example, `Radius.Compute/containers`), not raw manifest paths. A `go generate` script reads this file, resolves each resource type identifier to its manifest path in `resource-types-contrib`, and produces `manifests_gen.go` with `//go:embed` directives for exactly those resolved files (plus `defaults.yaml` itself). At runtime, `RegisterFS` reads `defaults.yaml` from the embedded FS to know which resource types to register and loads the corresponding embedded manifests. |
| │ └─► radius_compute.yaml (REMOVED, now embedded) │ | ||
| │ └─► radius_security.yaml (REMOVED, now embedded) │ |
There was a problem hiding this comment.
deploy/manifest/built-in-providers/ in this repo is split into dev/ and self-hosted/. If the intent is to remove radius_compute.yaml / radius_security.yaml, call out the concrete paths (e.g., deploy/manifest/built-in-providers/{dev,self-hosted}/radius_compute.yaml) so the change is clear and doesn’t imply there are top-level files directly under built-in-providers/.
| │ └─► radius_compute.yaml (REMOVED, now embedded) │ | |
| │ └─► radius_security.yaml (REMOVED, now embedded) │ | |
| │ └─► dev/radius_compute.yaml (REMOVED, now embedded) │ | |
| │ └─► dev/radius_security.yaml (REMOVED, now embedded) │ | |
| │ └─► self-hosted/radius_compute.yaml (REMOVED, now embedded) │ | |
| │ └─► self-hosted/radius_security.yaml (REMOVED, now embedded)│ |
| ## Compatibility (optional) | ||
|
|
||
| - **No breaking changes**: The existing `ManifestDirectory` config continues to work. Directory-based manifests are registered after embedded manifests, so existing deployments that set a custom manifest directory will continue to function. | ||
| - **Removed files**: `radius_compute.yaml` and `radius_security.yaml` are removed from `deploy/manifest/built-in-providers/`. Any tooling or scripts that reference these files directly would need to be updated. The `copy-manifests` Makefile target will copy fewer files but continues to work. |
There was a problem hiding this comment.
This “Removed files” bullet references deploy/manifest/built-in-providers/ as if the manifests live directly under it, but in this repo they live under deploy/manifest/built-in-providers/dev/ and .../self-hosted/. Update the paths here to match the actual layout so readers can find the affected files/targets easily.
| - **Removed files**: `radius_compute.yaml` and `radius_security.yaml` are removed from `deploy/manifest/built-in-providers/`. Any tooling or scripts that reference these files directly would need to be updated. The `copy-manifests` Makefile target will copy fewer files but continues to work. | |
| - **Removed files**: `radius_compute.yaml` and `radius_security.yaml` are removed from `deploy/manifest/built-in-providers/dev/` and `deploy/manifest/built-in-providers/self-hosted/`. Any tooling or scripts that reference these files directly would need to be updated. The `copy-manifests` Makefile target will copy fewer files but continues to work. |
|
|
||
| - **No breaking changes**: The existing `ManifestDirectory` config continues to work. Directory-based manifests are registered after embedded manifests, so existing deployments that set a custom manifest directory will continue to function. | ||
| - **Removed files**: `radius_compute.yaml` and `radius_security.yaml` are removed from `deploy/manifest/built-in-providers/`. Any tooling or scripts that reference these files directly would need to be updated. The `copy-manifests` Makefile target will copy fewer files but continues to work. | ||
| - **Redundant registration**: If a deployment provides the same resource type via both embedded manifests and a directory-based manifest, the directory-based one will overwrite the embedded one (UCP uses `CreateOrUpdate`). This is harmless and provides an escape hatch. |
There was a problem hiding this comment.
The note that directory-based manifests overwriting embedded ones is “harmless” seems inaccurate: the manifest registration code uses CreateOrUpdate semantics for the location resource with a ResourceTypes map built from the current manifest, which can effectively drop previously-registered type entries if the overriding manifest is a subset. Consider clarifying the semantics (full replace vs merge) and/or requiring that an overriding directory manifest include the complete intended provider/type set.
| - **Redundant registration**: If a deployment provides the same resource type via both embedded manifests and a directory-based manifest, the directory-based one will overwrite the embedded one (UCP uses `CreateOrUpdate`). This is harmless and provides an escape hatch. | |
| - **Redundant registration**: If a deployment provides the same provider namespace via both embedded manifests and a directory-based manifest, the directory-based registration runs last and uses UCP `CreateOrUpdate` semantics for the location resource. This should be treated as replacing the provider/type set represented by that manifest rather than merging with previously registered embedded types. As a result, an overriding directory manifest must include the complete intended set of resource types for that provider namespace; supplying only a subset can drop type entries that were registered earlier from embedded manifests. This still provides an escape hatch, but it is a full override rather than a harmless partial overlay. |
| # Automated Default Registration of Resource Types from resource-types-contrib | ||
|
|
||
| * **Author**: Karishma Chawla (@kachawla) | ||
|
|
||
| ## Overview |
There was a problem hiding this comment.
PR description still contains placeholder Fixes: #issue_number and the Type of change / checklist items aren’t selected. Please update the PR description to link the approved issue/design-doc PR (or mark items N/A) so reviewers can validate scope and tracking.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11610 +/- ##
=======================================
Coverage 51.38% 51.38%
=======================================
Files 699 699
Lines 44114 44114
=======================================
Hits 22666 22666
Misses 19279 19279
Partials 2169 2169 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Please explain the changes you've made.
Type of change
Fixes: #issue_number
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: