-
Notifications
You must be signed in to change notification settings - Fork 192
feat: add support for plugins #2079
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for plugins #2079
Conversation
WalkthroughThis change introduces first-class support for plugin and gRPC subgraphs across the entire stack, including schema, database, control plane, CLI, router, and studio. It adds new subgraph types, plugin lifecycle management, registry integration, plugin image handling, protobuf schema storage, new CLI commands, and comprehensive test coverage for plugin workflows and feature subgraphs. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Complexity: cross-cutting, schema and migration changes, new DB tables and enum, protobuf contract changes, new RPCs, significant router plugin lifecycle refactor (OCI image handling), CLI surface additions, and many new/updated tests. Review should include DB migration safety, protobuf compatibility, security checks for image unpacking and JWTs, RBAC and quota enforcement, and integration flows between router, control plane, and CLI. Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
…09-router-plugins-with-cosmo-integration
Router image scan passed✅ No security vulnerabilities found in image: |
…iple files and adjust related database schema and tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (4)
proto/wg/cosmo/platform/v1/platform.proto (3)
377-401:Subgraph.typeshould beoptional+ minor PluginData nit
- The same “unset vs default” problem exists here:
- SubgraphType type = 18; + optional SubgraphType type = 18;
PluginData.platformslooks like an enumeration of well-known runtimes (linux/arm64, alpine, …). Consider an enum to prevent free-form strings.
168-169: Maketypefieldoptionalfor presence detection
Same issue previously raised still applies: withoutoptional, servers cannot distinguish “unset” from the default enum value (STANDARD = 0).- SubgraphType type = 13; + optional SubgraphType type = 13;
1840-1841: MarksubgraphTypeasoptional
GraphCompositionSubgraph.subgraphTypeshould be declaredoptionalfor the same reason as above.- SubgraphType subgraphType = 7; + optional SubgraphType subgraphType = 7;controlplane/src/core/repositories/SubgraphRepository.ts (1)
542-543: Validation implemented but could be more explicit.The implementation correctly validates that plugin data is only processed for plugin-type subgraphs (lines 571-577), addressing the past review concern. However, the validation could be more explicit about rejecting invalid combinations.
The current implementation validates the subgraph type before processing plugin data:
if (data.proto.pluginData && subgraph.type === 'grpc_plugin') {This prevents plugin data insertion for non-plugin subgraphs, but it silently ignores the plugin data rather than throwing an error. Consider making this more explicit:
+ if (data.proto.pluginData && subgraph.type !== 'grpc_plugin') { + throw new Error('Plugin data can only be provided for plugin-type subgraphs'); + } + if (data.proto.pluginData && subgraph.type === 'grpc_plugin') {Also applies to: 544-544, 550-580
🧹 Nitpick comments (3)
proto/wg/cosmo/platform/v1/platform.proto (3)
370-374: Add an explicitUNSPECIFIED = 0value
WhileSTANDARD = 0currently doubles as “unspecified”, a dedicatedUNSPECIFIED(orSUBGRAPH_TYPE_UNSPECIFIED) makes intent clearer and leaves room for future re-ordering without breaking wire compatibility.
2844-2855: TreatpushTokenas sensitive – document or rename
pushTokenwill likely carry bearer-token style secrets. Make sure callers do not log this field and consider renaming topush_token(snake-case) for consistency with the rest of the schema.
[security]
3221-3223: Add idempotency annotation if RPC is read-only
IfValidateAndFetchPluginDatamerely validates & returns data without mutating state, addoption idempotency_level = NO_SIDE_EFFECTS;to the method options for better gRPC tooling support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (25)
cli/src/commands/router/commands/plugin/commands/create.ts(1 hunks)cli/src/commands/router/commands/plugin/commands/publish.ts(1 hunks)connect/src/wg/cosmo/platform/v1/platform_pb.ts(12 hunks)controlplane/migrations/0128_nifty_romulus.sql(1 hunks)controlplane/migrations/meta/_journal.json(1 hunks)controlplane/src/core/bufservices/plugin/validateAndFetchPluginData.ts(1 hunks)controlplane/src/core/bufservices/subgraph/createFederatedSubgraph.ts(7 hunks)controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts(9 hunks)controlplane/src/core/composition/composer.ts(4 hunks)controlplane/src/core/repositories/FeatureFlagRepository.ts(5 hunks)controlplane/src/core/repositories/PluginRepository.ts(1 hunks)controlplane/src/core/repositories/SubgraphRepository.ts(14 hunks)controlplane/src/core/util.ts(4 hunks)controlplane/src/db/schema.ts(3 hunks)controlplane/test/feature-flag/feature-flag-with-plugin-fs.test.ts(1 hunks)controlplane/test/feature-subgraph/create-feature-subgraph.test.ts(5 hunks)controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts(1 hunks)controlplane/test/plugin/validate-and-fetch-plugin-data.test.ts(1 hunks)controlplane/test/subgraph/create-subgraph.test.ts(16 hunks)controlplane/test/subgraph/delete-subgraph.test.ts(2 hunks)controlplane/test/subgraph/publish-subgraph.test.ts(10 hunks)proto/wg/cosmo/platform/v1/platform.proto(8 hunks)studio/src/components/subgraphs-table.tsx(8 hunks)studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/index.tsx(3 hunks)studio/src/pages/[organizationSlug]/[namespace]/subgraph/[subgraphSlug]/index.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- controlplane/migrations/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (16)
- studio/src/pages/[organizationSlug]/[namespace]/subgraph/[subgraphSlug]/index.tsx
- controlplane/test/subgraph/delete-subgraph.test.ts
- controlplane/src/core/repositories/PluginRepository.ts
- studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/compositions/[compositionId]/index.tsx
- controlplane/src/core/util.ts
- cli/src/commands/router/commands/plugin/commands/create.ts
- controlplane/src/core/repositories/FeatureFlagRepository.ts
- cli/src/commands/router/commands/plugin/commands/publish.ts
- controlplane/src/core/bufservices/plugin/validateAndFetchPluginData.ts
- controlplane/test/feature-flag/feature-flag-with-plugin-fs.test.ts
- controlplane/test/feature-subgraph/publish-feature-subgraph.test.ts
- controlplane/test/plugin/validate-and-fetch-plugin-data.test.ts
- studio/src/components/subgraphs-table.tsx
- controlplane/src/core/bufservices/subgraph/publishFederatedSubgraph.ts
- controlplane/test/feature-subgraph/create-feature-subgraph.test.ts
- controlplane/src/db/schema.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: in the cosmo codebase, database transactions are typically managed at the service layer (e.g., in bu...
Learnt from: wilsonrivera
PR: wundergraph/cosmo#1919
File: controlplane/src/core/repositories/OrganizationGroupRepository.ts:193-224
Timestamp: 2025-07-01T13:53:54.146Z
Learning: In the Cosmo codebase, database transactions are typically managed at the service layer (e.g., in buf services like deleteOrganizationGroup.ts), where repositories are instantiated with the transaction handle and all operations within those repositories are automatically part of the same transaction.
Applied to files:
controlplane/src/core/bufservices/subgraph/createFederatedSubgraph.ts
📚 Learning: in the cosmo router project, when extending json schema validation for security-sensitive fields lik...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
controlplane/src/core/composition/composer.ts
📚 Learning: in the cosmo router project, required field validation for jwks configuration (secret, algorithm, ke...
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
controlplane/src/core/composition/composer.ts
📚 Learning: in the wundergraph/cosmo project, anonymous admin access in grafana (gf_auth_anonymous_enabled=true ...
Learnt from: endigma
PR: wundergraph/cosmo#2009
File: docker-compose.yml:102-103
Timestamp: 2025-07-03T10:33:38.291Z
Learning: In the wundergraph/cosmo project, anonymous admin access in Grafana (GF_AUTH_ANONYMOUS_ENABLED=true and GF_AUTH_ANONYMOUS_ORG_ROLE=Admin) is intentionally configured for development environments in docker-compose.yml files.
Applied to files:
controlplane/test/subgraph/create-subgraph.test.ts
🧬 Code Graph Analysis (3)
controlplane/test/subgraph/create-subgraph.test.ts (5)
controlplane/test/test-util.ts (3)
SetupTest(66-417)DEFAULT_NAMESPACE(52-52)DEFAULT_SUBGRAPH_URL_ONE(49-49)controlplane/src/core/test-util.ts (4)
genID(53-55)createTestRBACEvaluator(173-175)createTestGroup(181-198)genUniqueLabel(57-59)connect-go/gen/proto/wg/cosmo/common/common.pb.go (4)
EnumStatusCode(23-23)EnumStatusCode(103-105)EnumStatusCode(107-109)EnumStatusCode(116-118)studio/src/lib/constants.ts (1)
OrganizationRole(284-284)connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go (4)
SubgraphType(73-73)SubgraphType(105-107)SubgraphType(109-111)SubgraphType(118-120)
controlplane/test/subgraph/publish-subgraph.test.ts (2)
controlplane/src/core/test-util.ts (4)
genUniqueLabel(57-59)genID(53-55)createTestRBACEvaluator(173-175)createTestGroup(181-198)controlplane/test/test-util.ts (3)
SetupTest(66-417)createSubgraph(703-715)subgraphSDL(753-757)
controlplane/src/core/repositories/SubgraphRepository.ts (2)
controlplane/src/db/models.ts (1)
DBSubgraphType(41-41)controlplane/src/types/index.ts (2)
SubgraphDTO(117-138)ProtoSubgraph(107-115)
🔇 Additional comments (44)
controlplane/src/core/bufservices/subgraph/createFederatedSubgraph.ts (6)
7-7: LGTM!The import additions properly support the new plugin subgraph functionality with appropriate type definitions and repository dependencies.
Also applies to: 26-28
48-49: LGTM!Setting a default
STANDARDtype ensures backward compatibility and defensive programming practices.
65-90: Comprehensive feature subgraph validation logic.The implementation properly validates feature subgraph requirements:
- Ensures base subgraph name is provided
- Validates base subgraph exists in the namespace
- Correctly sets baseSubgraphID and aligns feature subgraph type with base subgraph
This maintains referential integrity and type consistency for feature subgraphs.
137-137: Appropriate routing URL validation exemption.Plugin subgraphs correctly bypass routing URL validation since they execute via OCI images rather than HTTP endpoints, similar to event-driven graphs.
211-226: Proper plugin limit enforcement.The implementation correctly enforces billing plan plugin limits by:
- Counting existing plugins in the namespace
- Retrieving organization's plugin feature limit
- Handling unlimited plans appropriately
- Returning proper error response when limits are exceeded
248-248: LGTM!Properly formats and passes the subgraph type to the repository using the utility function.
controlplane/test/subgraph/publish-subgraph.test.ts (5)
17-21: Excellent use of real test data files.Reading actual proto schema, mappings, and lock files from disk provides more realistic test coverage compared to mock data.
391-404: Well-organized plugin test suite structure.The nested describe block with reusable test data definitions provides clear separation and maintainable test organization.
406-474: Comprehensive plugin publishing test coverage.The tests effectively cover both publishing scenarios:
- Publishing to existing plugin subgraph
- Creating and publishing in one step
Both tests properly validate stored plugin metadata and use appropriate billing plan configuration.
476-664: Thorough plugin validation and error handling tests.Excellent coverage of error scenarios:
- Plugin limit enforcement via billing plans
- Name conflict detection with regular subgraphs
- Type validation and mismatch handling
- Required field validation for proto data
- Version format validation
All tests use appropriate error codes and meaningful error messages.
666-762: Comprehensive RBAC test coverage for plugins.The tests effectively validate role-based permissions for:
- Plugin creation (limited to admin/developer/subgraph-admin roles)
- Publishing to existing plugins (includes subgraph-publisher role)
Clear separation between creation and publishing permissions aligns with security best practices.
controlplane/migrations/0128_nifty_romulus.sql (4)
1-1: Proper enum definition for subgraph types.The
subgraph_typeenum correctly defines the three supported types with appropriate naming conventions.
2-8: Well-designed plugin version metadata table.The
plugin_image_versionstable structure appropriately supports plugin versioning with:
- UUID primary key with auto-generation
- Foreign key relationship to schema_versions
- Flexible text version field and platform array
- Proper timestamp handling
10-17: Appropriate protobuf schema storage design.The
protobuf_schema_versionstable correctly stores protobuf data with separate fields for schema, mappings, and lock information, linked to the schema versioning system.
19-30: Robust column addition and constraint handling.The implementation correctly:
- Adds type column with backward-compatible default
- Uses exception handling for graceful constraint creation
- Implements cascade delete for proper referential integrity
The migration is resilient to re-runs and maintains data consistency.
controlplane/src/core/composition/composer.ts (4)
14-28: Proper type system extension for plugin support.The import additions and
ComposedSubgraphtype extension correctly incorporate the new plugin and gRPC subgraph variants while adding the requiredschemaVersionIdfield for consistency.Also applies to: 113-117
119-126: Proper error handling for JSON parsing.The
parseGRPCMappingfunction correctly wraps JSON parsing in try-catch with meaningful error messages, addressing potential parsing failures.
146-190: Comprehensive plugin and gRPC subgraph composition logic.The implementation properly handles different subgraph types:
- Validates required data presence for each type
- Constructs appropriate composed subgraph objects
- Builds correct ImageReference for plugin subgraphs
- Maintains type safety and error handling
The composition logic integrates well with the existing federation system.
226-226: Correct function signature update.Properly passes
federatedGraph.organizationIdto match the updated function signature required for plugin ImageReference construction.controlplane/test/subgraph/create-subgraph.test.ts (6)
3-3: Good addition of SubgraphType import.The import correctly adds support for the new plugin subgraph type enumeration, which is essential for the plugin creation tests.
12-12: Appropriate addition of genUniqueLabel utility.The
genUniqueLabelimport will be used effectively in the new plugin tests for generating unique labels, which helps prevent test interference.
52-74: Good refactoring to parameterized test.The conversion to
test.eachimproves test maintainability by eliminating code duplication while testing multiple RBAC roles consistently.
539-862: Excellent comprehensive plugin subgraph test suite.The new plugin test suite provides thorough coverage of plugin subgraph functionality including:
- ✅ Billing plan restrictions (developer vs launch plans)
- ✅ Plugin limit enforcement (max 3 on launch plan)
- ✅ Name collision prevention between plugin and regular subgraphs
- ✅ Complete RBAC permission matrix
- ✅ Multiple label support
- ✅ Verification that plugin limits are isolated from regular subgraph limits
The tests are well-structured, use appropriate test data generation, and verify both success and failure scenarios with proper error codes and messages.
A few notable strengths:
- Line 567: Proper validation that created subgraphs have the correct
GRPC_PLUGINtype- Lines 601-613: Efficient loop-based creation for testing limits
- Lines 831-858: Clever verification that plugin limits don't affect other subgraph types
- Lines 704-733, 735-770: Comprehensive RBAC coverage for both positive and negative cases
588-589: Consistent and appropriate error handling.The error messages and status codes are consistent across plugin limit scenarios, providing clear feedback about billing plan restrictions.
Also applies to: 626-627
661-663: Proper name collision error messages.The error messages clearly indicate the conflict and include both the subgraph name and namespace, which aids in debugging and user experience.
Also applies to: 697-699
proto/wg/cosmo/platform/v1/platform.proto (1)
67-69: 👍 Optional qualifiers added – resolves presence-detection problem
PublishFederatedSubgraphRequestnow marks bothtypeandprotoasoptional. This fixes the silent-default issue noted earlier.connect/src/wg/cosmo/platform/v1/platform_pb.ts (12)
33-57: Well-structured enum definition for subgraph types.The
SubgraphTypeenum is properly defined with clear semantic names and sequential numbering. The distinction betweenGRPC_PLUGINandGRPC_SERVICEprovides good separation of concerns for different gRPC-based subgraph types.
618-677: Complete protobuf message implementation for ProtoInput.The
ProtoInputmessage class is well-structured with appropriate field types and proper protobuf conventions. The field ordering is logical: schema, mappings, lock, platforms array, and version.
754-762: Appropriate optional fields for plugin support in PublishFederatedSubgraphRequest.The addition of optional
typeandprotofields maintains backward compatibility while enabling plugin subgraph publishing functionality.
783-784: Correct field definitions for optional plugin fields.The field definitions use proper protobuf field numbers (13, 14) and correct typing with optional flags.
1413-1416: Good default value for subgraph type in CreateFederatedSubgraphRequest.Setting the default to
SubgraphType.STANDARDensures backward compatibility and provides sensible defaults for existing workflows.
1437-1437: Proper non-optional field definition for required subgraph type.The
typefield is correctly defined as non-optional in CreateFederatedSubgraphRequest, ensuring type information is always present.
3040-3048: Well-designed plugin data extension to Subgraph message.The addition of
typefield with STANDARD default and optionalpluginDatafield provides a clean way to extend subgraph metadata for plugin types while maintaining backward compatibility.
3075-3076: Correct field definitions for plugin-related fields.Field numbers 18 and 19 are properly sequenced, and the types are correctly specified with appropriate optional flags.
3096-3137: Complete nested message implementation for plugin metadata.The
Subgraph_PluginDatanested message follows protobuf conventions properly with version string and platforms array. The implementation includes all necessary serialization methods.
14257-14260: Consistent subgraph type field in GraphCompositionSubgraph.Adding the
subgraphTypefield with STANDARD default maintains consistency across the platform API for composition handling.
14276-14276: Proper field definition for composition subgraph type.Field number 7 continues the sequence correctly with proper enum type reference.
22347-22449: Complete implementation of plugin validation messages.Both
ValidateAndFetchPluginDataRequestandValidateAndFetchPluginDataResponseare properly implemented with appropriate field types. The request includes name, namespace, and labels for plugin identification, while the response provides validation results and plugin metadata.controlplane/src/core/repositories/SubgraphRepository.ts (6)
12-12: LGTM! Proper imports for new type system.The imports for
DBSubgraphTypeandProtoSubgraphare correctly added to support the new subgraph type classification and protobuf schema functionality.Also applies to: 32-32
95-96: LGTM! Type parameter properly integrated.The
typeparameter is correctly added to the create method signature, stored in the subgraphs table, and included in the returned SubgraphDTO. The implementation aligns with the new subgraph type system.Also applies to: 136-137, 190-194
212-213: LGTM! Proto parameter handling is well-structured.The
protoparameter is properly added to the update method and passed toaddSchemaVersion. The conditional check forgrpc_plugintype ensures schema versions are added for plugin subgraphs regardless of schema equality, which is appropriate for plugin lifecycle management.Also applies to: 248-255
563-579: LGTM! Protobuf schema handling is well-implemented.The protobuf schema version insertion is properly scoped to gRPC service and plugin types, and the plugin image version data is correctly handled only for plugin types. The database operations are appropriately wrapped in the existing transaction.
605-605: LGTM! Return type includes new type field.The
addSchemaVersionmethod correctly includes thetypefield in the returned SubgraphDTO.
928-928: LGTM! Proto data retrieval is comprehensive.The
getSubgraphmethod properly:
- Selects the
typefield from the database- Initializes the
protovariable- Conditionally fetches protobuf schema and plugin data for gRPC types
- Includes both fields in the returned SubgraphDTO
The implementation correctly handles the relationship between protobuf schema versions and plugin image versions.
Also applies to: 943-943, 955-976, 997-998
…n in FeatureFlagRepository
…n in SubgraphRepository
…09-router-plugins-with-cosmo-integration
JivusAyrus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should modify the registry url to point to prod before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cli/src/commands/router/commands/plugin/commands/publish.ts (2)
16-19: DRY: Deduplicate thesupportedPlatformsdefinition
supportedPlatformsis declared twice (Lines 16-19 and 186-189). Define it once at module scope and reuse it to avoid divergence when the supported set changes.Also applies to: 186-198
101-103: SanitisepluginNamederived from directory name
pluginNamefalls back topath.basename(pluginDir), which can include characters disallowed by the backend or Docker tag rules (e.g., spaces, uppercase letters). Add a regex whitelist and fail fast if the name violates it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cli/src/commands/router/commands/plugin/commands/publish.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
cli/src/commands/router/commands/plugin/commands/publish.ts
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
cli/src/commands/router/commands/plugin/commands/publish.ts
📚 Learning: 2025-08-07T12:05:06.744Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2079
File: proto/wg/cosmo/platform/v1/platform.proto:39-45
Timestamp: 2025-08-07T12:05:06.744Z
Learning: In the Cosmo project, the proto fields for schema, mappings, and lock in ProtoInput are intentionally kept as string types rather than bytes because the team works with text data and wants it to be UTF-8 encoded for readability and text processing purposes.
Applied to files:
cli/src/commands/router/commands/plugin/commands/publish.ts
…09-router-plugins-with-cosmo-integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (3)
proto/wg/cosmo/platform/v1/platform.proto (3)
168-168: Make CreateFederatedSubgraphRequest.type optional for presence detection (duplicate)Without
optional, down-level clients are indistinguishable from those explicitly settingSTANDARD(0). This can defeat validation and migration logic.- SubgraphType type = 13; + optional SubgraphType type = 13;
399-401: Make Subgraph.type optional for presence detection (duplicate)Same rationale as creation: without presence, existing clients get coerced to
STANDARD(0) and servers cannot tell unset from explicitly set.- SubgraphType type = 18; + optional SubgraphType type = 18;
1842-1842: Make GraphCompositionSubgraph.subgraphType optional (duplicate)Maintains presence semantics in composition results and prevents defaulting ambiguity.
- SubgraphType subgraphType = 7; + optional SubgraphType subgraphType = 7;
🧹 Nitpick comments (6)
proto/wg/cosmo/platform/v1/platform.proto (6)
39-45: ProtoInput: keep strings (UTF-8) — add brief field docs for clarityAligned with the team decision to keep these as
string. Consider documenting expected contents and formats to aid SDK users.message ProtoInput { - string schema = 1; - string mappings = 2; - string lock = 3; - repeated string platforms = 4; - string version = 5; + // Text contents of the protobuf schema (UTF-8) + string schema = 1; + // Text contents of plugin mappings (UTF-8) + string mappings = 2; + // Text contents of the lock file (UTF-8) + string lock = 3; + // Supported build targets (e.g., "linux/amd64", "linux/arm64", "wasm") + repeated string platforms = 4; + // Plugin version (e.g., semver) + string version = 5; }
67-68: PublishFederatedSubgraphRequest: clarify relation betweentypeandprotoPlease document the precondition so clients know how to populate these fields. Server-side must enforce it.
optional bool disable_resolvability_validation = 12; - optional SubgraphType type = 13; - optional ProtoInput proto = 14; + // If type != STANDARD, 'proto' must be provided; for STANDARD subgraphs, 'proto' must be omitted. + optional SubgraphType type = 13; + optional ProtoInput proto = 14;
370-374: SubgraphType: add short comments per valueNames look good. Add one-line docs to reduce ambiguity across SDKs.
enum SubgraphType { - STANDARD = 0; - GRPC_PLUGIN = 1; - GRPC_SERVICE = 2; + // Standard GraphQL subgraph + STANDARD = 0; + // gRPC plugin running within the router + GRPC_PLUGIN = 1; + // External gRPC service subgraph + GRPC_SERVICE = 2; }
377-381: Subgraph.PluginData: add field docsMirror
ProtoInputdocs so the meaning ofplatformsandversionremains consistent across request/response types.message Subgraph { message PluginData{ - string version = 1; - repeated string platforms = 2; + // Plugin version (e.g., semver) + string version = 1; + // Supported target platforms (e.g., "linux/amd64", "linux/arm64", "wasm") + repeated string platforms = 2; }
3223-3225: Annotate RPC as NO_SIDE_EFFECTSThis RPC validates and fetches metadata; marking as no side effects can improve client behavior and proxies.
- rpc ValidateAndFetchPluginData(ValidateAndFetchPluginDataRequest) returns (ValidateAndFetchPluginDataResponse) {} + rpc ValidateAndFetchPluginData(ValidateAndFetchPluginDataRequest) returns (ValidateAndFetchPluginDataResponse) { + option idempotency_level = NO_SIDE_EFFECTS; + }
2846-2850: ValidateAndFetchPluginDataRequest: confirm call timing & strengthen identityI didn’t find any implementation or call sites for ValidateAndFetchPluginData in the service layer—please verify whether this RPC can be invoked before a subgraph record exists. If it can, adding a unique identifier (for example, a
typeor dedicatedidfield) will help avoid misclassification down-the-line.Please review
proto/wg/cosmo/platform/v1/platform.proto:message ValidateAndFetchPluginDataRequest { string name = 1; string namespace = 2; repeated Label labels = 3; // Optional: include when request may occur pre-subgraph creation optional SubgraphType type = 4; }– If the RPC is always called post-creation, confirm here.
– If pre-creation calls are possible, please add and document the new field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(12 hunks)proto/wg/cosmo/platform/v1/platform.proto(8 hunks)router-tests/go.mod(6 hunks)router/go.mod(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- router/go.mod
- router-tests/go.mod
- connect/src/wg/cosmo/platform/v1/platform_pb.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-07T12:05:06.744Z
Learnt from: StarpTech
PR: wundergraph/cosmo#2079
File: proto/wg/cosmo/platform/v1/platform.proto:39-45
Timestamp: 2025-08-07T12:05:06.744Z
Learning: In the Cosmo project, the proto fields for schema, mappings, and lock in ProtoInput are intentionally kept as string types rather than bytes because the team works with text data and wants it to be UTF-8 encoded for readability and text processing purposes.
Applied to files:
proto/wg/cosmo/platform/v1/platform.proto
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
proto/wg/cosmo/platform/v1/platform.proto
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
PR: wundergraph/cosmo#2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
proto/wg/cosmo/platform/v1/platform.proto
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: build-router
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
StarpTech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…09-router-plugins-with-cosmo-integration
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Tests
Checklist
COMPLETES ENG-7682
COMPLETES ENG-7758