✨ Add automated schema generation and validation for registry+v1 bundle configuration #2454
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
anik120
requested review from
joelanford and
perdasilva
and removed request for
camilamacedo86 and
thetechnick
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
1367534 to
2cd3619
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements Phase 1 of the DeploymentConfig feature to achieve feature parity with OLMv0's SubscriptionConfig for registry+v1 bundles. It adds automated JSON schema generation and validation infrastructure for the deploymentConfig field in ClusterExtension.
Changes:
- Added reflection-based schema generator tool that introspects
v1alpha1.SubscriptionConfigfrom the operator-framework/api package - Implemented embedded JSON schema validation infrastructure with comprehensive test coverage
- Integrated deploymentConfig schema into bundle config validation with make target for regeneration
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hack/tools/schema-generator/main.go | Implements reflection-based generator that introspects Go types and parses AST to extract documentation, handling nested k8s types and inline fields |
| hack/tools/schema-generator/main_test.go | Comprehensive test suite validating schema structure, selector field exclusion, and comparing generated vs committed schemas |
| internal/operator-controller/rukpak/bundle/schema/validator.go | Validation infrastructure with embedded schema compilation and multi-format input support (JSON bytes, strings, structs) |
| internal/operator-controller/rukpak/bundle/schema/validator_test.go | Test coverage for valid and invalid deployment configurations including edge cases |
| internal/operator-controller/rukpak/bundle/schema/deploymentconfig.json | Generated JSON Schema Draft 7 document (1862 lines) defining all SubscriptionConfig fields except selector |
| internal/operator-controller/rukpak/bundle/schema/README.md | Documentation explaining schema purpose, included/excluded fields, and regeneration workflow |
| internal/operator-controller/rukpak/bundle/registryv1.go | Integration of deploymentConfig schema into bundle config validation |
| internal/operator-controller/config/config.go | Added GetDeploymentConfig accessor method with nil handling |
| hack/tools/update-deploymentconfig-schema.sh | Shell script for regenerating schema from vendor dependencies |
| Makefile | Added update-deploymentconfig-schema make target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/rukpak/bundle/schema/validator_test.go
Outdated
Show resolved
Hide resolved
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
2cd3619 to
56d10e2
Compare
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
56d10e2 to
fce993a
Compare
anik120
force-pushed
the
deploymentconfig-json-schema
branch
2 times, most recently
from
cd32996 to
5bfc310
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Write to file | ||
| if err := os.WriteFile(outputFile, data, 0600); err != nil { |
There was a problem hiding this comment.
The file permission 0600 makes the generated schema file read/write for owner only. Since this is a JSON schema file that needs to be committed to the repository and read by the build process, it should use more permissive permissions like 0644 (readable by all, writable by owner).
| if err := os.WriteFile(outputFile, data, 0600); err != nil { | |
| if err := os.WriteFile(outputFile, data, 0644); err != nil { |
There was a problem hiding this comment.
0644 is what I started out with, but the linter fails with
hack/tools/schema-generator/main.go:106:12 gosec G306: Expect WriteFile permissions to be 0600 or less
But if I change this to 0600, Copilot says it should be 0644.
The machines are fighting with each other, and I'm caught in between.
There was a problem hiding this comment.
Use 644 and add a //nolint:gosec comment.
There was a problem hiding this comment.
There's only one user in the pod. There's no need to give group and other users access. 0600 has been used in many other places.
There was a problem hiding this comment.
let's start off with 0600 and see if more is required in that case then
internal/operator-controller/rukpak/bundle/schema/validator_test.go
Outdated
Show resolved
Hide resolved
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
5bfc310 to
64e7f6b
Compare
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
64e7f6b to
6b8cb00
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2454 +/- ##
==========================================
+ Coverage 69.49% 73.22% +3.72%
==========================================
Files 101 103 +2
Lines 7754 8131 +377
==========================================
+ Hits 5389 5954 +565
+ Misses 1930 1713 -217
- Partials 435 464 +29
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:
|
|
@anik120 have you considered approaching this by having the schema-generator generate the base registry+v1 bundle configuration schema including watchNamespace? Then the process of generating the final schema for a bundle could be: Also, you could delete the deploymentConfig from the schema if the featuregate isn't enabled... |
hack/tools/schema-generator/main.go
Outdated
| Maximum *int `json:"maximum,omitempty"` | ||
| } | ||
|
|
||
| type schemaGenerator struct { |
There was a problem hiding this comment.
Did you consider pulling the OpenAPI definition for the Deployment and pruning it? Maybe that would make life a little easier?
There was a problem hiding this comment.
Turns out, it does make life a lot easier. Updated the PR to pull in the OpenAPI definition and use that to compose the schema, PTAL
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
52133b9 to
0b4f640
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/rukpak/bundle/schema/registryv1bundleconfig.json
Show resolved
Hide resolved
internal/operator-controller/rukpak/bundle/schema/registryv1bundleconfig.json
Show resolved
Hide resolved
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
0b4f640 to
543430c
Compare
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
543430c to
234ec81
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
234ec81 to
430e19a
Compare
| return &spec, nil | ||
| } | ||
|
|
||
| func parseSubscriptionConfig(filepath string) ([]FieldInfo, error) { |
There was a problem hiding this comment.
Don't hate me, but have you considered using JSONSchema library like jsonschema-go, or something like that to deserialize the schema, manipulate it through the APIs then serialize it again? It could be easier than dealing with the AST.
Again, please don't hate me XDDD
There was a problem hiding this comment.
There was a problem hiding this comment.
When I was playing with this, I think I used: https://github.com/santhosh-tekuri/jsonschema
There was a problem hiding this comment.
We're extracting field information from the SubscriptionConfig Go struct (field names, types, whether they're slices/maps/pointers, etc.) and mapping those fields to their corresponding kube openAPI v3 schemas(that have been fetched). This requires parsing go source code, which is why AST is required.
I dug around to see if a JSON schema library like santhosh-tekuri/jsonschema/v6 (since it's already used by the config package for validation) could help us manipulate the OpenAPI schemas better, but looks like this library is designed for validating data against schemas (not for building/manipulating schemas)
The library's types (like Schema, Types, etc.) are optimized for the validation engine - eg, the Types field is a custom int type (bit field) that requires calling Add() methods rather than simple assignment. Using it for schema construction would actually be more verbose and awkward than working with raw maps.
However, this exploration did make me realize that the building of the schema can be a little cleaner by using custom typed structures instead of manipulating raw map[string]interface{}, which I have done in this new commit, PTAL.
Again, AST is unavoidable.
| return &str | ||
| } | ||
|
|
||
| // GetDeploymentConfig returns the deploymentConfig value if present in the configuration. |
There was a problem hiding this comment.
Not for this PR. But at some point we need to start splitting the architecture s.t. the configuration handling is bundle agnostic. E.g. at the config level we define some interface for getting the schema and validating it against the configuration given by the user, and some kind of Render interface that take the arbitrary or map[string]interface{} configuration and "deals with it". I.e. what separate what the applier has to do, and what the bundles have to do more clearly.
There was a problem hiding this comment.
I'm happy to tackle this if I can get all the work for config api done before time
| if [[ ! -f "${SUBSCRIPTION_TYPES}" ]]; then | ||
| echo "Error: ${SUBSCRIPTION_TYPES} not found. Run 'go mod vendor' first." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
We don't vendor things in upstream.
There was a problem hiding this comment.
Is there another approach that could be taken?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
13732f6 to
dacc042
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
c488b3c to
e3dba45
Compare
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
e3dba45 to
74bb1ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…le configuration Summary: Implements Phase 1 of the variable configuration feature to achieve feature parity with OLMv0's SubscriptionConfig for registry+v1 bundles. RFC: https://docs.google.com/document/d/18O4qBvu5I4WIJgo5KU1opyUKcrfgk64xsI3tyXxmVEU/edit?tab=t.0#heading=h.x3tfh25grvnv Details: This PR adds infrastructure for JSON schema-based validation of registry+v1 bundle configuration in ClusterExtension, including both `watchNamespace` and `deploymentConfig` fields: - **Schema Generation Tool**: Parses SubscriptionConfig using AST to extract field names/types - Fetches Kubernetes OpenAPI v3 specs from GitHub releases - Maps fields to official Kubernetes schemas - Generates schema with `watchNamespace` (for operator scope) and `deploymentConfig` (for deployment customization) - Excludes `selector` field (unused in OLMv0) - **Runtime Schema Customization**: The base schema is loaded and modified at runtime based on operator install modes: `watchNamespace` validation rules are customized per operator's supported install modes - **Validation Infrastructure**: Validation integrated into bundle config unmarshaling via `config.UnmarshalConfig()` - **Regeneration Workflow**: Added `make update-registryv1-bundle-schema` target to regenerate schema when upstream SubscriptionConfig changes. When the upstream `v1alpha1.SubscriptionConfig` adds new fields (e.g., new k8s corev1 types), running the regeneration target will automatically include them in the schema without manual updates. Addresses OPRUN-4112 (Phase 1) Future PRs will implement: - Phase 2: Renderer integration to apply deploymentConfig to Deployments - Phase 3: Provider integration to extract bundle configuration from bundles - Phase 4: Documentation
Switch from raw map[string]interface{} manipulation to custom typed
structures (Schema and SchemaField) for building JSON schemas. This
provides:
- Type-safe schema construction with compile-time checking
- Clearer, more maintainable code compared to raw map manipulation
The generated schema is functionally identical to the previous version
(all tests pass), with only cosmetic differences in JSON field ordering.
anik120
force-pushed
the
deploymentconfig-json-schema
branch
from
74bb1ba to
6f5561c
Compare
5 participants
Summary:
Implements Phase 1 of the variable configuration feature to achieve feature parity with
OLMv0's SubscriptionConfig for registry+v1 bundles.
RFC: https://docs.google.com/document/d/18O4qBvu5I4WIJgo5KU1opyUKcrfgk64xsI3tyXxmVEU/edit?tab=t.0#heading=h.x3tfh25grvnv
Details:
This PR adds infrastructure for JSON schema-based validation of registry+v1 bundle configuration
in ClusterExtension, including both
watchNamespaceanddeploymentConfigfields:Schema Generation Tool: Parses SubscriptionConfig using AST to extract field names/types
watchNamespace(for operator scope) anddeploymentConfig(for deployment customization)selectorfield (unused in OLMv0)Runtime Schema Customization: The base schema is loaded and modified at runtime based on
operator install modes:
watchNamespacevalidation rules are customized per operator's supported install modesValidation Infrastructure: Validation integrated into bundle config unmarshaling via
config.UnmarshalConfig()Regeneration Workflow: Added
make update-registryv1-bundle-schematarget to regenerate schema whenupstream SubscriptionConfig changes.
When the upstream
v1alpha1.SubscriptionConfigadds new fields (e.g., new k8s corev1 types), running theregeneration target will automatically include them in the schema without manual updates.
Addresses OPRUN-4112 (Phase 1)
Future PRs will implement: