Move to schemaless backend#295
Conversation
| CustomResourceDefinitionSource = SchemaSource("CustomResourceDefinition.v1.apiextensions.k8s.io") | ||
| ) | ||
|
|
||
| var schemaSourceAliases = map[string]string{ |
There was a problem hiding this comment.
Add TODO: This is tight now, but we can relax in the future
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
from
2133317 to
71082b5
Compare
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
2 times, most recently
from
bf7b6c0 to
7632399
Compare
| continue | ||
| } | ||
| // Collect all schemas for hashing. | ||
| // TODO(mjudeikis) Scope is same for all crds so we keep stamping it over. We might want to change this |
There was a problem hiding this comment.
This gonna be simplified in follow-up pr + added validation for the case when APIExport container mixed scopes it will not allow it for now.
| k8s.io/sample-controller => github.com/kcp-dev/kubernetes/staging/src/k8s.io/sample-controller v0.0.0-20250816165010-ffe1d7c8649b | ||
| ) | ||
|
|
||
| replace sigs.k8s.io/multicluster-runtime => github.com/mjudeikis/sigs-multicluster-runtime v0.0.0-20250818101434-d8ebc45e169b |
There was a problem hiding this comment.
We need a runnable PR to be merged into upstream for this. This is temporarly until we agree on direction in mcr
kubernetes-sigs/multicluster-runtime#62
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the backend to be fully schema-less by removing the kube-bind APIResourceSchema CRD and renaming BoundAPIResourceSchema to BoundSchema. The backend now uses dynamic clients and can work with any CRD-compliant schema source, making APIServiceBinding 1:1 with APIServiceExport.
Key changes:
- Remove APIResourceSchema CRD and replace with dynamic schema source handling
- Rename BoundAPIResourceSchema to BoundSchema for consistency
- Update API structure to use GroupResource references instead of schema name references
Reviewed Changes
Copilot reviewed 68 out of 70 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/framework/clients.go | Adds NewRESTConfig helper function for test setup |
| test/e2e/framework/browser.go | Updates browser path checking to use contains instead of exact match |
| test/e2e/framework/backend.go | Updates CRD creation to use boundschemas instead of boundapiresourceschemas |
| test/e2e/bind/happy-case_test.go | Updates test comments to reflect CRD usage instead of APIResourceSchema |
| sdk/client/listers/kubebind/v1alpha2/*.go | Renames and updates listers from BoundAPIResourceSchema to BoundSchema |
| sdk/client/informers/externalversions/kubebind/v1alpha2/*.go | Updates informers to use BoundSchema instead of BoundAPIResourceSchema |
| sdk/apis/kubebind/v1alpha2/*.go | Major type refactoring: removes APIResourceSchema, renames BoundAPIResourceSchema to BoundSchema |
| pkg/konnector/controllers/cluster/*.go | Updates controllers to work with BoundSchema and new API structure |
| backend/*.go | Updates backend to support dynamic schema sources and removes APIResourceSchema handling |
| deploy/crd/*.yaml | Updates CRD definitions to reflect the new BoundSchema structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
from
7632399 to
0a5b301
Compare
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: @SAP mangirdas.judeikis@sap.com
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
from
0a5b301 to
1b19d69
Compare
|
@coderabbitai can you review this? |
|
@sttts I'll review the changes in this PR. This looks like a significant architectural refactoring to move to a schemaless backend approach. ✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the project from APIResourceSchema/BoundAPIResourceSchema to BoundSchema across APIs, controllers, clients, informers, listers, helpers, RBAC, and manifests. Adds healthz endpoint, updates options and server wiring, removes crd2apiresourceschema CLI, adjusts indexers and tests, and introduces KCP example resources and CRDs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Backend as Backend Controller
participant Cache as Manager Cache
participant K8s as Kubernetes API
participant BS as BoundSchema
Note over User,Backend: ServiceExportRequest reconciliation (BoundSchema-first)
User->>Backend: Create APIServiceExportRequest
Backend->>K8s: List CRDs (per schemaSource)
K8s-->>Backend: CRD list
Backend->>Backend: CRD->BoundSchema (convert)
Backend->>K8s: Create missing BoundSchemas
K8s-->>Backend: Created/Exists
Backend->>K8s: Ensure APIServiceExport (group/resource + versions)
K8s-->>Backend: APIServiceExport upserted
Note over Backend,BS: Provider in-sync based on BoundSchemasSpecHash
Backend->>Cache: getBoundSchema(namespace,name)
Cache-->>Backend: *BoundSchema
Backend->>Backend: Compute BoundSchemasSpecHash
Backend->>K8s: Update APIServiceExport annotations/status
K8s-->>Backend: Updated
sequenceDiagram
autonumber
participant Export as APIServiceExport
participant RBAC as RBAC Reconciler
participant Cache as Cache
participant K8s as Kubernetes API
participant BS as BoundSchema
Note over Export,RBAC: ClusterBinding RBAC from BoundSchema
Export->>RBAC: Reconcile
RBAC->>Cache: Collect resources (group/resource)
Cache-->>RBAC: Keys sorted
loop For each key
RBAC->>Cache: getBoundSchema(ns, key)
Cache-->>RBAC: *BoundSchema
RBAC->>RBAC: Build PolicyRule (verbs, apiGroups, resources)
end
RBAC->>K8s: Apply ClusterRole/Binding
K8s-->>RBAC: Upserted
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
I notice in the go.mod changes that Could you clarify:
I'd recommend avoiding personal forks in dependencies unless absolutely critical, and if needed, having a clear plan to remove them by upstreaming the required changes. |
|
@sttts You're absolutely right to raise this concern. The personal fork is indeed a temporary measure solely to unblock current development. To address your questions:
I want to be crystal clear: this personal fork dependency will NOT persist beyond upstream availability. The moment the upstream PR is merged, we'll update our go.mod to use the official release. This is strictly a temporary development unblock and not a long-term dependency strategy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
kcp/README.md (1)
90-96: Fix grammar and clarify step purpose.Improves readability.
-9. Now we gonna initiate consumer: +9. Initialize the consumer workspace:backend/options/options.go (1)
158-159: Fix user-facing help string typo."on th bind screen" → "on the bind screen".
-fs.StringVar(&options.TestingAutoSelect, "testing-auto-select", options.TestingAutoSelect, "<resource>.<group> that is automatically selected on th bind screen for testing") +fs.StringVar(&options.TestingAutoSelect, "testing-auto-select", options.TestingAutoSelect, "<resource>.<group> that is automatically selected on the bind screen for testing")backend/controllers/serviceexport/serviceexport_controller.go (2)
70-77: BoundSchema is namespaced; lookup must include namespaceUsing NamespacedName{Name: name} will fail or collide across namespaces. Pass namespace to the helper and use it in the key.
Apply:
- getBoundSchema: func(ctx context.Context, cache cache.Cache, name string) (*kubebindv1alpha2.BoundSchema, error) { - var schema kubebindv1alpha2.BoundSchema - key := types.NamespacedName{Name: name} + getBoundSchema: func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error) { + var schema kubebindv1alpha2.BoundSchema + key := types.NamespacedName{Namespace: namespace, Name: name} if err := cache.Get(ctx, key, &schema); err != nil { return nil, err } return &schema, nil },Note: This change implies updating the reconciler’s getBoundSchema signature and its call sites to pass the namespace.
145-167: Indexer/mapper key mismatch — fix required
- BoundSchema is namespaced (sdk/apis/kubebind/v1alpha2/boundchema_types.go).
- The indexer IndexServiceExportByBoundSchema returns keys of the form "resource.group" (pkg/indexers/serviceexport.go).
- The mapper currently uses boundSchema.Name (backend/controllers/serviceexport/serviceexport_controller.go) — this both mismatches the indexer and can cause cross-namespace collisions for namespaced BoundSchema objects.
Actionable fixes (choose one):
Option A (recommended): make both sides namespace-aware.
- Change IndexServiceExportByBoundSchema to return keys including the export namespace, e.g. "namespace/resource.group".
- Change the mapper to build the same namespace-aware key from the BoundSchema (fmt.Sprintf("%s/%s.%s", boundSchema.Namespace, , )).
- Fix getBoundSchema (it currently does key := types.NamespacedName{Name: name}) to use a real Namespace + Name (types.NamespacedName{Namespace: ns, Name: name}) or accept namespace/name separately.
Option B: keep the indexer as-is and make the mapper compute the identical "resource.group" keys from BoundSchema.Spec (e.g. use BoundSchema.Spec.Names.Plural and Spec.Group), listing/merging results for each key.
Do NOT apply the original one-line diff (only changing boundSchemaKey to namespace/name) without also updating the indexer and getBoundSchema — it will leave the indexer/mapper inconsistent or still broken.
Files to change: pkg/indexers/serviceexport.go, backend/controllers/serviceexport/serviceexport_controller.go (mapper + getBoundSchema), and validate BoundSchema usage in sdk/apis/kubebind/v1alpha2/boundchema_types.go.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
197-202: Logic bug: non-cluster scope deletes CRB, then immediately recreates itGuard the whole create/update block with scope, or return after delete.
- if r.scope != kubebindv1alpha2.ClusterScope { - if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) - } - } + if r.scope != kubebindv1alpha2.ClusterScope { + if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) + } + return nil + }pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
189-250: updateCRD error is swallowed; metadata clobbered on update
- On error from updateCRD, the function returns nil, masking failures.
- Overwriting ObjectMeta drops labels/annotations/owner refs produced by conversion.
func (r *reconciler) ensureCRDsFromBoundSchema(ctx context.Context, binding *kubebindv1alpha2.APIServiceBinding, schema *kubebindv1alpha2.BoundSchema) error { @@ - crd.ObjectMeta = existing.ObjectMeta - if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil - } else if errors.IsInvalid(err) { + // Preserve resourceVersion, merge labels/annotations/owners + crd.ResourceVersion = existing.ResourceVersion + if len(existing.Labels) > 0 { + if crd.Labels == nil { crd.Labels = map[string]string{} } + for k, v := range existing.Labels { crd.Labels[k] = v } + } + if len(existing.Annotations) > 0 { + if crd.Annotations == nil { crd.Annotations = map[string]string{} } + for k, v := range existing.Annotations { crd.Annotations[k] = v } + } + if !kubebindhelpers.IsOwnedByBinding(binding.Name, binding.UID, existing.OwnerReferences) { + crd.OwnerReferences = append(crd.OwnerReferences, newReference) + } else { + crd.OwnerReferences = existing.OwnerReferences + } + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err + } else if errors.IsInvalid(err) { conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionConnected, "CustomResourceDefinitionUpdateFailed", conditionsapi.ConditionSeverityError, - "CustomResourceDefinition %s cannot be updated: %s", - binding.Name, err, + "CustomResourceDefinition %s cannot be updated: %s", + crd.Name, err, ) return nil }pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (1)
389-392: Update error message to reflect new terminologyThe error message still references "BoundAPIResourceSchemas" which should be updated to "BoundSchemas" for consistency.
conditions.MarkFalse( export, kubebindv1alpha2.APIServiceExportConditionConnected, - "BoundAPIResourceSchemasNotValid", + "BoundSchemasNotValid", conditionsapi.ConditionSeverityWarning, - "One or more BoundAPIResourceSchemas are not valid", + "One or more BoundSchemas are not valid", )backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (2)
219-223: Don’t flip Succeeded to Failed based on age after success.This overrides a successful outcome and will incorrectly mark old but successful requests as failed.
Apply one of:
- Remove the block:
- if time.Since(req.CreationTimestamp.Time) > time.Minute { - req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed - req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) - }
- Or only apply when ExportsReady is False and before creation.
225-228: Fix klog usage and message content.Info doesn’t do formatting. Also message says “service binding request” in a service export reconciler.
Apply:
- logger.Info("Deleting service binding request %s/%s", req.Namespace, req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time)) + logger.Info("Deleting APIServiceExportRequest", "namespace", req.Namespace, "name", req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time))
♻️ Duplicate comments (5)
test/e2e/framework/browser.go (1)
31-31: Substring path match acknowledged; keep as-is for kcp tests.Prior feedback about potential false positives was noted and accepted for this test-only helper.
backend/options/options.go (1)
119-124: Typos and grammar in TODO and comment."mostrly" → "mostly"; minor grammar tweak.
-// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 -// We should relax these once we happy they work with any schema. +// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 +// We should relax these once we are happy they work with any schema. @@ - CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests + CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostly for e2e testsbackend/kubernetes/resources/rbac.go (1)
107-109: Resource name corrected (boundschemas).Previous typo noted in an earlier review is resolved.
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
76-78: Alias comment fixed and type alias is appropriateMatches prior feedback; no further action.
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
175-177: Validate scope consistency across all BoundSchemas.Currently the last schema “wins”. If scopes differ, surface a clear condition and fail the request.
🧹 Nitpick comments (36)
kcp/deploy/examples/cowboy.yaml (2)
8-9: Avoid setting status in example manifest.Users cannot update status via the main endpoint when the status subresource is enabled; including it here may confuse or fail on apply.
Apply this diff to keep the example focused on spec:
status: - result: "ready to ride"
9-9: Add a trailing newline.Fixes YAML lint warning.
- result: "ready to ride" + result: "ready to ride" +cli/pkg/kubectl/bind/plugin/bind.go (1)
161-163: Clarify mismatch wording in version error.Consider “got …, want …” for quick scanning.
- return fmt.Errorf("unsupported binding provider version: %q != %q", provider.APIVersion, kubebindv1alpha2.GroupVersion) + return fmt.Errorf("unsupported binding provider version: got=%q, want=%q", provider.APIVersion, kubebindv1alpha2.GroupVersion)kcp/deploy/examples/apiresourceschema.yaml (1)
48-48: Add a trailing newline.Fixes YAML lint warning.
- status: {} + status: {} +kcp/deploy/examples/apiexport.yaml (1)
11-11: Add a trailing newline.Fixes YAML lint warning.
- crd: {} + crd: {} +test/e2e/framework/clients.go (1)
55-59: Mark helper and include kubeconfig path in the failure.Improves test diagnostics without changing behavior.
func NewRESTConfig(t *testing.T, kubeconfig string) *rest.Config { - config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) - require.NoError(t, err, "Failed to build config from kubeconfig file") + t.Helper() + config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) + require.NoErrorf(t, err, "failed to build REST config from kubeconfig file: %s", kubeconfig) return config }kcp/README.md (4)
35-49: Standardize on “kubectl ws” instead of alias “k”.Reduces ambiguity for readers without shell aliases.
-k ws use :root:kube-bind +kubectl ws use :root:kube-bind
52-58: Minor: ensure step numbering is sequential.Steps jump from 5 to 7; renumber to avoid confusion.
76-80: Prefer apply over create for idempotent setup.Allows re-running the README steps safely.
-kubectl create -f kcp/deploy/examples/apiexport.yaml -kubectl create -f kcp/deploy/examples/apiresourceschema.yaml +kubectl apply -f kcp/deploy/examples/apiexport.yaml +kubectl apply -f kcp/deploy/examples/apiresourceschema.yaml # recursive bind kubectl kcp bind apiexport root:provider:cowboys-stable
144-145: Avoid using the “k” alias in debug examples.Use full “kubectl” for copy-paste friendliness.
-k -s "$(kubectl get apiexportendpointslice kube-bind.io -o jsonpath="{.status.endpoints[0].url}")/clusters/*" api-resources -k -s "$(kubectl get apiexportendpointslice kube-bind.io -o jsonpath="{.status.endpoints[0].url}")/clusters/*" get crd +kubectl -s "$(kubectl get apiexportendpointslice kube-bind.io -o jsonpath="{.status.endpoints[0].url}")/clusters/*" api-resources +kubectl -s "$(kubectl get apiexportendpointslice kube-bind.io -o jsonpath="{.status.endpoints[0].url}")/clusters/*" get crdbackend/options/options.go (3)
58-60: Update comment to match the actual KCP version.The example says v1alpha2, but the code uses v1alpha1.
-// APIResourceSchema.v1alpha2.apis.kcp.io +// APIResourceSchema.v1alpha1.apis.kcp.io
146-154: Make help output deterministic and de-duplicated.Collecting map values can produce duplicates and non-deterministic order. Deduplicate and sort for stable UX.
- values := make([]string, 0, len(schemaSourceAliases)) - for _, v := range schemaSourceAliases { - values = append(values, v) - } + valuesSet := sets.New[string]() + for _, v := range schemaSourceAliases { + valuesSet.Insert(v) + } + values := sets.List(valuesSet) // sorted
244-252: Be forgiving with alias casing.Optionally allow lowercase aliases without breaking canonical values.
- schemaSource := schemaSourceAliases[options.SchemaSource] + schemaSource := schemaSourceAliases[options.SchemaSource] + if schemaSource == "" { + schemaSource = schemaSourceAliases[strings.ToLower(options.SchemaSource)] + }kcp/go.mod (1)
192-195: Forked module replaces: track and remove when upstream is ready.Replacements to personal forks unblock work but hurt reproducibility. Keep an issue to revert to upstream (sigs.k8s.io/multicluster-runtime, github.com/kcp-dev/multicluster-provider) before tagging.
kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
40-44: Confirm scope for apiresourceschemas permission claim.Do we need full "*" verbs, or would get/list/watch suffice? Principle of least privilege if write is not required.
backend/kubernetes/resources/rbac.go (2)
112-114: Tighten verbs on status subresource (drop list).Status subresources typically need get/patch/update; list is unnecessary.
- Resources: []string{"boundschemas/status"}, - Verbs: []string{"get", "list", "patch", "update"}, + Resources: []string{"boundschemas/status"}, + Verbs: []string{"get", "patch", "update"},
106-114: Avoid RBAC update churn due to order-sensitive equality.rulesEqual/policyRuleEqual compare slices by order; ClusterRoles often differ only by element order, causing perpetual updates. Normalize before compare.
Example normalization approach (outside this hunk):
func normalizeRule(r rbacv1.PolicyRule) rbacv1.PolicyRule { cp := r slices.Sort(cp.APIGroups) slices.Sort(cp.Resources) slices.Sort(cp.ResourceNames) slices.Sort(cp.NonResourceURLs) slices.Sort(cp.Verbs) return cp } func policyRuleEqual(a, b rbacv1.PolicyRule) bool { na, nb := normalizeRule(a), normalizeRule(b) return slices.Equal(na.APIGroups, nb.APIGroups) && slices.Equal(na.Resources, nb.Resources) && slices.Equal(na.Verbs, nb.Verbs) && slices.Equal(na.ResourceNames, nb.ResourceNames) && slices.Equal(na.NonResourceURLs, nb.NonResourceURLs) }backend/http/handler.go (1)
119-125: Health endpoint nit: add HEAD support, Content-Type, and a tiny body.Improves probe compatibility and clarity with negligible cost.
- mux.HandleFunc("/healthz", h.handleHealthz).Methods("GET") + mux.HandleFunc("/healthz", h.handleHealthz).Methods("GET", "HEAD") @@ func (h *handler) handleHealthz(w http.ResponseWriter, r *http.Request) { prepareNoCache(w) - w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok\n")) }pkg/indexers/serviceexport.go (1)
39-53: Harden index key construction; skip invalid entries.Guard against empty Resource/Group to avoid malformed keys and noisy index entries.
func IndexServiceExportByBoundSchema(obj client.Object) []string { export, ok := obj.(*v1alpha2.APIServiceExport) if !ok { return nil } names := make([]string, 0, len(export.Spec.Resources)) for _, res := range export.Spec.Resources { - name := res.Resource + "." + res.Group - names = append(names, name) + if res.Resource == "" || res.Group == "" { + continue + } + names = append(names, res.Resource+"."+res.Group) } return names }Also: same composition (res.Resource + "." + res.Group) appears in pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go, pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go, pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go, backend/controllers/clusterbinding/clusterbinding_reconcile.go, backend/controllers/serviceexport/serviceexport_reconcile.go, and backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go — consider applying the same guard where BoundSchema names are constructed/used.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (5)
155-158: Error message drops resource name and format verbInclude the BoundSchema name and proper format string.
- return fmt.Errorf("failed to get BoundSchema %w", err) + return fmt.Errorf("failed to get BoundSchema %q: %w", name, err)
179-185: Avoid churn: compare RBAC rules deterministicallyreflect.DeepEqual is order-sensitive; listServiceExports order may vary, causing perpetual updates. Sort rules and verbs before comparing.
I can provide a helper to sort []rbacv1.PolicyRule by (APIGroups, Resources, Verbs).
183-184: Misleading error text ("create" on update)- return fmt.Errorf("failed to create ClusterRole %s: %w", role.Name, err) + return fmt.Errorf("failed to update ClusterRole %s: %w", role.Name, err)
243-245: Misleading error text and wrong identifier on update- return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Namespace, err) + return fmt.Errorf("failed to update ClusterRoleBinding %s: %w", name, err)
275-286: Misleading error text and wrong identifier on RoleBinding update- if err := r.updateRoleBinding(ctx, client, binding); err != nil { - return fmt.Errorf("failed to create RoleBinding %s: %w", expected.Namespace, err) - } + if err := r.updateRoleBinding(ctx, client, binding); err != nil { + return fmt.Errorf("failed to update RoleBinding %s: %w", binding.Name, err) + }pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
153-187: Name no longer matches behaviorreferenceBoundAPIResourceSchema now works with BoundSchema. Consider renaming to referenceBoundSchema for clarity. No functional impact.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (3)
64-64: NewschemaSourceparameter is unused in the reconciler.The
schemaSourcestring parameter is added to the constructor and stored in the reconciler struct (line 85), but it's never used in the reconciliation logic. Consider removing it if it's not needed, or document its intended future use.
86-93: Consider reusing the namespace from the parent reconciler.The
getBoundSchemaclosure function recreates theNamespacedNamekey with its own namespace parameter, but this namespace could be derived from the parent context or passed from the reconciler's configuration.
184-184: Remove obsolete RBAC comment about APIResourceSchemas.The RBAC comment still references
apiresourceschemaswhich are no longer used in this refactored version. This should be removed or updated to reflect BoundSchema usage.-//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiresourceschemas,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch;create;update;patch;deletedeploy/crd/kube-bind.io_apiserviceexports.yaml (1)
507-509: Clarify the comment about CRD permissionsThe comment mentions "you can not ask for permissions for resource provided by a CRD not provided by an service binding export" but has a minor grammatical issue.
- Note: it is worth noting that you can not ask for permissions for resource provided by a CRD - not provided by an service binding export. + Note: you cannot ask for permissions for resources provided by a CRD + that is not provided by a service binding export.pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile_test.go (1)
113-119: Verify context handling in test helpersThe newGetBoundSchema function accepts a context parameter that isn't used. Consider whether this is intentional for future extensions or should be removed.
If the context parameter is not needed, consider simplifying:
-func newGetBoundSchema(_ context.Context, boundSchema *kubebindv1alpha2.BoundSchema) func(ctx context.Context, name string) (*kubebindv1alpha2.BoundSchema, error) { +func newGetBoundSchema(boundSchema *kubebindv1alpha2.BoundSchema) func(ctx context.Context, name string) (*kubebindv1alpha2.BoundSchema, error) {And update the usage:
- getRemoteBoundSchema: newGetBoundSchema(ctx, tt.boundSchema), + getRemoteBoundSchema: newGetBoundSchema(tt.boundSchema),backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (2)
93-107: Avoid N× identical List calls; list once and reuse.You list exported CRDs inside the per-resource loop, repeating the same API call. List once, index by group/plural, then iterate resources. Improves performance and reduces API pressure.
76-86: Parse GVK robustly and validate once.Relying on a “Kind.Version.Group” string is brittle. Consider passing a typed schema.GroupVersionKind into the reconciler, or at least validate and precompute outside the loop.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go (1)
59-71: Minor: consider adding a doc on required RBAC.A short comment noting needed verbs on boundschemas (get/list/watch/create/update/patch/delete) would help consumers.
sdk/apis/kubebind/v1alpha2/boundchema_types.go (3)
220-227: Comment says “Ready/DriftDetected” but constants are Valid/Invalid.Align the comments with the actual names to avoid confusion.
Apply:
-// BoundSchemaReady indicates that the API resource schema is ready. -// It is set to true when the API resource schema is accepted and there are no drifts detected. +// BoundSchemaValid indicates the bound schema is valid (accepted, no drift). ... -// BoundSchemaDriftDetected indicates that there is a drift between the consumer's API and the expected API. -// It is set to true when the API resource schema is not accepted or there are drifts detected. +// BoundSchemaInvalid indicates the bound schema is invalid (rejected or drift detected).
162-165: Optional: define ConversionStrategyType constants.Adds type-safety for callers.
Example:
const ( ConversionStrategyNone ConversionStrategyType = "None" ConversionStrategyWebhook ConversionStrategyType = "Webhook" )
1-1: Filename typo: boundchema_types.go → boundschema_types.go.Fix for consistency and discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (69)
backend/controllers/clusterbinding/clusterbinding_controller.go(1 hunks)backend/controllers/clusterbinding/clusterbinding_reconcile.go(2 hunks)backend/controllers/serviceexport/serviceexport_controller.go(3 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(2 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(5 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(3 hunks)backend/http/handler.go(1 hunks)backend/kubernetes/manager.go(1 hunks)backend/kubernetes/resources/rbac.go(1 hunks)backend/options/options.go(2 hunks)backend/server.go(1 hunks)cli/cmd/crd2apiresourceschema/cmd/crd2apiresourceschema.go(0 hunks)cli/cmd/crd2apiresourceschema/main.go(0 hunks)cli/pkg/crd2apiresourceschema/cmd/cmd.go(0 hunks)cli/pkg/crd2apiresourceschema/plugin/crd2apiresourceschema.go(0 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_apiresourceschemas.yaml(0 hunks)deploy/crd/kube-bind.io_apiserviceexports.yaml(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)deploy/examples/apiresourceschema.yaml(1 hunks)deploy/patches/kube-bind.io_boundapiresourceschemas.yaml-patch(0 hunks)deploy/patches/kube-bind.io_boundschemas.yaml-patch(1 hunks)kcp/README.md(3 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-apiresourceschemas.kube-bind.io.yaml(0 hunks)kcp/deploy/resources/apiresourceschema-apiserviceexports.kube-bind.io.yaml(2 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)kcp/go.mod(4 hunks)pkg/indexers/serviceexport.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(6 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile_test.go(5 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go(1 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go(8 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile_test.go(4 hunks)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/boundapiresourceschema_types.go(0 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(4 hunks)sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema_test.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundapiresourceschema.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)sdk/apis/kubebind/v1alpha2/register.go(1 hunks)sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go(5 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundapiresourceschema.go(0 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_kubebind_client.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/generated_expansion.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/kubebind_client.go(2 hunks)sdk/client/informers/externalversions/generic.go(1 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/interface.go(2 hunks)sdk/client/listers/kubebind/v1alpha2/apiresourceschema.go(0 hunks)sdk/client/listers/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/listers/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/listers/kubebind/v1alpha2/expansion_generated.go(1 hunks)test/e2e/bind/fixtures/provider/apiresourceschema-foo.yaml(0 hunks)test/e2e/bind/fixtures/provider/apiresourceschema-mangodb.yaml(0 hunks)test/e2e/bind/happy-case_test.go(1 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/bind.go(0 hunks)test/e2e/framework/browser.go(2 hunks)test/e2e/framework/clients.go(2 hunks)
💤 Files with no reviewable changes (19)
- test/e2e/framework/bind.go
- test/e2e/bind/fixtures/provider/apiresourceschema-mangodb.yaml
- test/e2e/bind/fixtures/provider/apiresourceschema-foo.yaml
- deploy/patches/kube-bind.io_boundapiresourceschemas.yaml-patch
- sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema_test.go
- sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundapiresourceschema.go
- cli/cmd/crd2apiresourceschema/cmd/crd2apiresourceschema.go
- deploy/crd/kube-bind.io_apiresourceschemas.yaml
- cli/cmd/crd2apiresourceschema/main.go
- sdk/client/listers/kubebind/v1alpha2/boundapiresourceschema.go
- sdk/apis/kubebind/v1alpha2/helpers/boundapiresourceschema.go
- sdk/client/listers/kubebind/v1alpha2/apiresourceschema.go
- cli/pkg/crd2apiresourceschema/cmd/cmd.go
- sdk/apis/kubebind/v1alpha2/boundapiresourceschema_types.go
- sdk/client/informers/externalversions/kubebind/v1alpha2/boundapiresourceschema.go
- cli/pkg/crd2apiresourceschema/plugin/crd2apiresourceschema.go
- sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema.go
- kcp/deploy/resources/apiresourceschema-apiresourceschemas.kube-bind.io.yaml
- sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundapiresourceschema.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/apiresourceschema.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiexport.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/cowboy.yaml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.27.2)
kcp/README.md
[high] 47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (53)
deploy/examples/apiresourceschema.yaml (1)
5-6: Good: explicit “exported” label added.This helps controllers select exported schemas. Ensure any label selectors in controllers/tests match this exact key.
kcp/go.mod (2)
7-12: Local replaces are fine for dev; confirm plan for merge.Relative replaces to ../sdk/apis, ../sdk/client, ../cli tie this module to the monorepo. Ensure they remain intentional for main, or switch to a go.work workspace and drop replaces before release.
3-3: Toolchain consistency check."go 1.24.0" requires matching toolchains in CI and other modules. Verify the repo builds under 1.24 across all submodules.
kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
61-74: Schema name sync.Ensure the referenced schema IDs (apiserviceexports, boundschemas) are created by the corresponding APIResourceSchema manifests in this PR set.
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (2)
25-48: Schemaless intent: confirm preserve-unknown-fields posture.The version schema sets x-kubernetes-preserve-unknown-fields: true and keeps rich descriptions. Verify this aligns with controllers and validation expectations (e.g., CEL rules and conversion=None).
5-19: Rename looks consistent.Kind/plural/listKind and shortNames updated to BoundSchema/boundschemas; metadata.name matches APIExport references.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go (1)
28-53: Generated fake aligns with BoundSchema.Resource, kind, list types, and namespace wiring look correct.
test/e2e/bind/happy-case_test.go (1)
68-70: Log message update matches schemaless flow."Creating CRD on provider side" better reflects the new path.
deploy/patches/kube-bind.io_boundschemas.yaml-patch (1)
1-6: No-op content change is fine.Retains conversion.strategy=None; newline-only change is harmless.
test/e2e/framework/backend.go (2)
63-71: CRD bootstrap list updated correctly.Removed old (apiresource/boundapiresource) schemas; added boundschemas.
90-92: Test forces CRD schema source explicitly.Good to keep e2e deterministic while generalizing runtime.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/generated_expansion.go (1)
29-29: Rename to BoundSchemaExpansion looks consistent with the API surface.Verified: only BoundSchemaExpansion is defined and referenced; no APIResourceSchemaExpansion remains. Locations: sdk/client/clientset/versioned/typed/kubebind/v1alpha2/generated_expansion.go:29, sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go:51
sdk/client/informers/externalversions/generic.go (1)
78-79: Approve — v1alpha2 informer wiring looks correct.BoundSchemas().Informer() exists at sdk/client/informers/externalversions/generic.go:79 and no occurrences of WithResource("apiresourceschemas") or WithResource("boundapiresourceschemas") were found.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_kubebind_client.go (1)
48-50: Approve: BoundSchemas(...) delegates to newFakeBoundSchemasnewFakeBoundSchemas exists at sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go:34 and is called from sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_kubebind_client.go:49 — rename is consistent, no issues.
backend/controllers/serviceexport/serviceexport_controller.go (2)
61-63: Indexer migration to BoundSchema: looks goodIndexer name and function align with the BoundSchema path.
174-176: Watching BoundSchema via factory mapper: OKWiring matches the multicluster builder pattern.
backend/server.go (1)
182-183: SchemaSource plumbed into ServiceExportRequest reconciler: OKKeeps handler and controller in sync on schema source.
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
81-88: Resources validations added: OKRequired, MinItems=1, and immutability via CEL harden the API.
sdk/apis/kubebind/v1alpha2/register.go (1)
49-51: Register BoundSchema types: OKTypes added to the scheme correctly.
sdk/client/listers/kubebind/v1alpha2/expansion_generated.go (1)
49-56: Generated lister expansions renamed to BoundSchema: OKConsistent with the type rename.
pkg/indexers/serviceexport.go (1)
27-27: LGTM on the new index key constant.Name aligns with the BoundSchema-based flow.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/kubebind_client.go (1)
36-36: Generated BoundSchema client + companions present.
BoundSchemasGetter/BoundSchemaInterface and newBoundSchemas exist; typed client, fake client, informer, and lister files found at sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go, sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go, sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go, sdk/client/listers/kubebind/v1alpha2/boundschema.go.backend/controllers/clusterbinding/clusterbinding_controller.go (1)
80-86: Include namespace in error and verify controller watches BoundSchema changesAdd namespace to the error for faster triage.
File: backend/controllers/clusterbinding/clusterbinding_controller.go (getBoundSchema)
Apply:
- return nil, fmt.Errorf("failed to get BoundSchema %q: %w", name, err) + return nil, fmt.Errorf("failed to get BoundSchema %s/%s: %w", namespace, name, err)Repo search returned no matches for a watch or immutability hints — verify SetupWithManager.Watches includes BoundSchema or confirm BoundSchema specs are immutable; if the controller must react to spec changes, ensure RBAC permits it.
pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go (1)
103-111: Wrap remote errors with resource identity — verify CRD status subresource & RBACWrap Get/UpdateStatus errors with namespace/name (see diff). I attempted to verify the CRD/RBAC but the earlier script failed to find the deploy/crd paths; please run the checks below from the repo root and paste results.
- getRemoteBoundSchema: func(ctx context.Context, name string) (*kubebindv1alpha2.BoundSchema, error) { - return providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).Get(ctx, name, metav1.GetOptions{}) + getRemoteBoundSchema: func(ctx context.Context, name string) (*kubebindv1alpha2.BoundSchema, error) { + bs, err := providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return nil, fmt.Errorf("get BoundSchema %s/%s: %w", providerNamespace, name, err) + } + return bs, nil }, - updateRemoteBoundSchema: func(ctx context.Context, boundSchema *kubebindv1alpha2.BoundSchema) error { - _, err := providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).UpdateStatus(ctx, boundSchema, metav1.UpdateOptions{}) - return err + updateRemoteBoundSchema: func(ctx context.Context, boundSchema *kubebindv1alpha2.BoundSchema) error { + _, err := providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).UpdateStatus(ctx, boundSchema, metav1.UpdateOptions{}) + if err != nil { + return fmt.Errorf("update BoundSchema status %s/%s: %w", providerNamespace, boundSchema.Name, err) + } + return nil },Run these to verify CRD status subresource and RBAC (no path restrictions):
- rg -nP -C2 'kind:\sCustomResourceDefinition[\s\S]name:\sboundschemas.kube-bind.io[\s\S]subresources:\s\n\sstatus:' -S
- rg -nP -C2 'boundschemas/status' -S
Ensure fmt is imported where you add fmt.Errorf.
backend/kubernetes/manager.go (1)
196-215: Remove commented-out CreateBoundSchema stub.Schemaless backend makes this dead code; keep history in git, not as comments.
File: backend/kubernetes/manager.go (lines 196-215)
Apply:
-/* func (m *Manager) CreateBoundSchema(ctx context.Context, cluster string, name string, u *unstructured.Unstructured) error { - ... -} -*/Verification:
rg -n 'ListAPIResourceSchemas|CreateAPIResourceSchema'returned no matches.pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (2)
118-131: Condition reason/message updated correctlySwitch to BoundSchema semantics looks consistent; the error path marks Connected=false with a clear reason.
266-283: ```shell
#!/bin/bash
set -euo pipefailecho "1) Search for 'BoundSchema' symbol (any case-sensitive matches)..."
rg -n --hidden --no-ignore -S '\bBoundSchema\b' -C3 || trueecho "2) Search for a type declaration 'type BoundSchema' in Go files..."
rg -n --hidden --no-ignore -S 'type\s+BoundSchema\b' --type=go -C3 || trueecho "3) Search for getBoundSchema function usage/definition..."
rg -n --hidden --no-ignore -S 'getBoundSchema(' -C3 || trueecho "4) Show the reconciler file (where the snippet came from) if present..."
if [ -f pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go ]; then
sed -n '1,400p' pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go
else
echo "File pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go not found"
fiecho "5) Search for lowercase 'boundschema' (CRD names, YAML, manifests)..."
rg -n --hidden --no-ignore -S 'boundschema' -C3 || trueecho "6) Search for APIServiceExport usages and resource concatenation patterns..."
rg -n --hidden --no-ignore -S '\bAPIServiceExport\b' -C3 --type=go || true
rg -n --hidden --no-ignore -S '.Resource.+.".".+..Group' -C3 || trueecho "Done."
</blockquote></details> <details> <summary>sdk/client/listers/kubebind/v1alpha2/boundschema.go (1)</summary><blockquote> `45-48`: **Confirm plural resource name for BoundSchema** rg produced no matches; verification inconclusive. Generated listers typically use the plural resource name. File: sdk/client/listers/kubebind/v1alpha2/boundschema.go (lines 45–48). Confirm whether Resource("boundschema") should be Resource("boundschemas"); if so, update and regenerate. </blockquote></details> <details> <summary>sdk/client/informers/externalversions/kubebind/v1alpha2/interface.go (2)</summary><blockquote> `35-37`: **LGTM: BoundSchemas informer added consistently** Interface and version wiring for BoundSchemas look correct. --- `72-75`: **LGTM: BoundSchemas() constructor** Constructor aligns with other informers; no issues. </blockquote></details> <details> <summary>backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)</summary><blockquote> `152-178`: **LGTM! Clean refactoring to BoundSchema-based mapping.** The mapper function is correctly updated to work with BoundSchema objects instead of APIResourceSchema. The indexing key construction and field references are properly aligned with the new model. --- `242-245`: **LGTM! Watches correctly updated for BoundSchema.** The controller setup properly watches BoundSchema resources with the appropriate mapper function. </blockquote></details> <details> <summary>backend/controllers/serviceexport/serviceexport_reconcile.go (2)</summary><blockquote> `55-68`: **LGTM! Clean implementation of BoundSchema-based hashing.** The logic correctly constructs the BoundSchema key using resource and group, retrieves schemas, and handles not-found errors appropriately. --- `70-79`: **Good refactoring to use centralized hash function.** Using `helpers.BoundSchemasSpecHash` instead of inline hashing logic improves maintainability and consistency across the codebase. </blockquote></details> <details> <summary>kcp/deploy/resources/apiresourceschema-apiserviceexports.kube-bind.io.yaml (4)</summary><blockquote> `5-5`: **Version bump in APIResourceSchema name.** The name changed from `v250809-5ed76a1` to `v250902-878858c`, which appears to be a date-based versioning with git hash. Ensure this aligns with your versioning strategy and migration path. --- `491-522`: **LGTM! Well-structured resource specification for v1alpha2.** The new resource structure with `group`, `resource`, and `versions` fields provides a cleaner API design. The validation patterns and immutability rules are appropriately defined. --- `495-499`: **Good pattern validation for API group names.** The regex pattern correctly validates DNS-like API group names and allows empty string for core groups. --- `520-522`: **Excellent addition of immutability validation.** The x-kubernetes-validations rule ensures resources cannot be modified after creation, which is crucial for maintaining consistency. </blockquote></details> <details> <summary>sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go (2)</summary><blockquote> `36-47`: **LGTM! Namespace-scoped informer correctly implemented.** The BoundSchemaInformer properly includes namespace field and passes it to all List/Watch operations. --- `17-19`: **BoundSchema generated code is namespace-scoped and consistent.** Verified informers, listers, clientset and fake implementations use namespace parameters and namespaced indexers/list/watch (examples: sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go; sdk/client/listers/kubebind/v1alpha2/boundschema.go; sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go; sdk/client/informers/externalversions/generic.go). </blockquote></details> <details> <summary>sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)</summary><blockquote> `55-57`: **Good validation for multi-version CRDs.** The check ensures that conversion strategies are properly defined when multiple versions exist. --- `2-2`: **Copyright year is 2025.** The copyright year is set to 2025, which appears to be in the future. This should be 2024 or the actual year of creation. ```diff -Copyright 2025 The Kube Bind Authors. +Copyright 2024 The Kube Bind Authors.Likely an incorrect or invalid review comment.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile_test.go (2)
37-38: Test refactoring aligns well with the schemaless backend migrationThe test updates correctly reflect the migration from BoundAPIResourceSchema to BoundSchema, including:
- Proper type updates to use the unified BoundSchema
- Consistent resource reference changes to APIServiceExportRequestResource
- Updated NotFound error resource names from "boundapiresourceschemas" to "boundschemas"
Also applies to: 48-51, 61-63, 82-86, 110-127
129-140: InformerScope is validated in the API and respected at runtime — no change requiredCRD-level XValidation enforces immutability and the scope relationship, and runtime/CLI code honors the field. See: sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (XValidation/InformerScope), backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (sets InformerScope), pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (uses InformerScope), backend/options/options.go (consumer-scope validation).
deploy/crd/kube-bind.io_apiserviceexports.yaml (2)
497-520: Well-structured resource definition for schemaless approachThe new resource structure with group/resource/versions fields provides a clean, flexible API surface that aligns well with the schemaless backend design. The pattern validations and default values are appropriate.
523-525: Good immutability constraint on resourcesThe validation rule
self == oldSelfensures resources cannot be changed after creation, which is important for maintaining consistency in the binding lifecycle.deploy/crd/kube-bind.io_boundschemas.yaml (2)
6-6: CRD rename properly executedThe rename from BoundAPIResourceSchema to BoundSchema is consistently applied throughout the CRD definition, including all metadata, naming conventions, and descriptions.
Also applies to: 14-19, 29-29, 49-49, 352-352
117-119: Well-structured validation for webhook configurationThe validation rule correctly ensures that webhook configuration is required when strategy is set to "Webhook" and prohibited when strategy is "None".
pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (2)
61-63: Clean migration to BoundSchema-based operationsThe replacement of APIResourceSchema operations with BoundSchema operations (getRemoteBoundSchema/updateRemoteBoundSchema) is properly implemented.
142-142: Updated error messages are clear and accurateThe error messages correctly reference "BoundSchema" instead of the old terminology, maintaining consistency with the new architecture.
Also applies to: 206-206
pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile_test.go (1)
53-55: Test data structures properly aligned with new APIThe test correctly uses APIServiceExportRequestResource with GroupResource fields, properly testing the new schemaless backend approach.
Also applies to: 129-140
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go (1)
33-52: LGTM: new BoundSchema client surface looks correct.Resource path, namespace scoping, and UpdateStatus are wired properly and match the CRD’s subresource.
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (1)
422-426: No change needed — APIServiceExportResource is an alias of APIServiceExportRequestResource.
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go declarestype APIServiceExportResource = APIServiceExportRequestResource, so the generated deepcopy using APIServiceExportRequestResource is correct.
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
kcp/go.mod (1)
3-3: Invalid go directive.Use “major.minor” (no patch). This breaks tooling.
-go 1.24.0 +go 1.24kcp/deploy/resources/apiresourceschema-apiserviceexports.kube-bind.io.yaml (1)
424-427: Fix misleading validation message.Rule checks name format, but message claims “informerScope is immutable”.
- - message: informerScope is immutable - rule: self.metadata.name == self.spec.names.plural+"."+self.spec.group + - message: metadata.name must be <plural>.<group> + rule: self.metadata.name == self.spec.names.plural+"."+self.spec.grouptest/e2e/framework/backend.go (1)
132-134: Use WithTimeout instead of metav1.Now().Add for deadline.metav1.Time doesn’t expose Add like time.Time on the struct; simpler to use a timeout.
- ctx, cancel := context.WithDeadline(context.Background(), metav1.Now().Add(10*time.Second)) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)kcp/README.md (1)
1-1: Temporary multicluster-runtime fork — create tracking issue + CI guard; remove replaces before merging main.Replace directives pointing to personal forks were found: ./go.mod (line 186) and kcp/go.mod (lines 192, 194) — they replace sigs.k8s.io/multicluster-runtime and github.com/kcp-dev/multicluster-provider with github.com/mjudeikis forks. Open a tracking issue, add a CI job that fails if any go.mod contains "=> github.com/mjudeikis/", and remove these replace entries from main prior to merging.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
180-185: RBAC still references apiresourceschemas; update to boundschemas.Controller now watches and gets BoundSchema, but RBAC grants are for apiresourceschemas. Update to avoid authorization failures.
-//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiresourceschemas,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watchI can scan other controllers for the same RBAC mismatch if helpful.
backend/controllers/serviceexport/serviceexport_controller.go (2)
92-95: Add RBAC for boundschemas read access.Controller watches and reads BoundSchema; grant get/list/watch.
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiserviceexports/finalizers,verbs=update +//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch
70-77: Bug: BoundSchema must be fetched by namespace+name (avoid collisions).getBoundSchema in backend/controllers/serviceexport/serviceexport_controller.go uses types.NamespacedName{Name: name} — BoundSchema is namespaced; this will collide across namespaces and break namespaced watch/index caches.
- getBoundSchema: func(ctx context.Context, cache cache.Cache, name string) (*kubebindv1alpha2.BoundSchema, error) { - var schema kubebindv1alpha2.BoundSchema - key := types.NamespacedName{Name: name} + getBoundSchema: func(ctx context.Context, cache cache.Cache, namespace, name string) (*kubebindv1alpha2.BoundSchema, error) { + var schema kubebindv1alpha2.BoundSchema + key := types.NamespacedName{Namespace: namespace, Name: name} if err := cache.Get(ctx, key, &schema); err != nil { return nil, err } return &schema, nil },Action items (required):
- Change the function signature/type in backend/controllers/serviceexport/serviceexport_controller.go and backend/controllers/serviceexport/serviceexport_reconcile.go to accept (namespace, name) and update all r.getBoundSchema(...) call sites to pass the correct namespace (e.g., export.Namespace or req.Namespace).
- Update getBoundSchemaMapper (backend/controllers/serviceexport/serviceexport_controller.go) to use boundSchema.Namespace when querying/listing, or adjust the matcher to include namespace.
- Update the indexer in pkg/indexers/serviceexport.go (and any other indexers that index by BoundSchema name-only) to index by "namespace/name" and update all MatchingFields calls to use the new key format.
- Run tests and verify informer/indexer behavior (namespaced watches).
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
219-222: Phase flips from Succeeded to Failed after 1 minuteThis downgrades successful requests. Only fail if still Pending (or not ready).
- if time.Since(req.CreationTimestamp.Time) > time.Minute { + if req.Status.Phase == kubebindv1alpha2.APIServiceExportRequestPhasePending && + time.Since(req.CreationTimestamp.Time) > time.Minute { req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) }pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
235-247: Bug: update error is swallowed; wrong object name in message.
- On update, any non-Invalid error returns nil, masking failures.
- Error message uses binding.Name instead of crd.Name.
Apply:
- if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil - } else if errors.IsInvalid(err) { + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err + } else if errors.IsInvalid(err) { conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionConnected, "CustomResourceDefinitionUpdateFailed", conditionsapi.ConditionSeverityError, - "CustomResourceDefinition %s cannot be updated: %s", - binding.Name, err, + "CustomResourceDefinition %s cannot be updated: %s", + crd.Name, err, ) return nil }pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (2)
162-175: Controller key parsing breaks for names containing dots.You key controllers as "<schema.Name>.<export.Name>", but schema.Name is "." and includes dots. The teardown logic splits on ".", expects exactly 2 parts, and incorrectly derives schemaName, causing stale controllers to linger or be stopped erroneously.
Apply:
- for key, c := range r.syncContext { - parts := strings.Split(key, ".") - if len(parts) == 2 && parts[1] == export.Name { - schemaName := parts[0] - if !processedSchemas[schemaName] { + for key, c := range r.syncContext { + if strings.HasSuffix(key, "."+export.Name) { + schemaName := strings.TrimSuffix(key, "."+export.Name) + if !processedSchemas[schemaName] { logger.V(1).Info("Stopping APIServiceExport resource sync", "key", key, "reason", "Schema no longer referenced") c.cancel() delete(r.syncContext, key) } } }This also matches your delete path which already uses HasSuffix(".<export.Name>").
Also applies to: 132-159
183-191: Compare generations consistently (schema vs export).You store schema.Generation in syncContext but compare it to export.Generation when deciding reuse, causing unnecessary restarts.
Apply:
- if found { - if c.generation == export.Generation { + if found { + if c.generation == schema.Generation { r.lock.Unlock() return nil } logger.V(1).Info("Stopping APIServiceExport resource sync", "key", key, "reason", "GenerationChanged", "generation", schema.Generation) c.cancel() delete(r.syncContext, key) } ... r.syncContext[key] = syncContext{ - generation: schema.Generation, + generation: schema.Generation, cancel: cancel, }Also applies to: 300-308
♻️ Duplicate comments (4)
test/e2e/framework/browser.go (1)
31-37: Reduce false positives: use suffix/boundary match instead of substring; fix log message.Contains() can match unintended paths; suffix/boundary checks keep flexibility without broad matches. Also the success log says “waiting” even after a match. Consider:
- if strings.Contains(browser.Url().Path, path) { - t.Logf("Browser is at %s, waiting for path %s", browser.Url(), path) + u := browser.Url() + p := strings.TrimSuffix(u.Path, "/") + exp := strings.Trim(path, "/") + if p == exp || strings.HasSuffix(p, "/"+exp) { + t.Logf("Browser reached expected path. url=%s expected=%s", u, path) return true } - t.Logf("Waiting for browser to be at %s, current URL: %s", path, browser.Url()) + t.Logf("Waiting for browser path to match. expected suffix=/%s current=%s", exp, u) return false - }, wait.ForeverTestTimeout, time.Millisecond*100, "Browser is not at path %s", path) + }, wait.ForeverTestTimeout, time.Millisecond*100, "Browser did not match expected path %s", path)If substring semantics are strictly required for kcp tests, confirm and we’ll keep Contains() but still fix the log lines.
backend/kubernetes/resources/rbac.go (1)
107-109: Resource name fix LGTMCorrectly switched to boundschemas; resolves prior typo feedback.
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
76-78: Alias doc fixed—LGTM.Comment now accurately states it’s a type alias.
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
153-178: Validate mixed scopes instead of overwriting last seen scopeYou overwrite scope on each iteration and assume uniform scope. Collect scopes and fail fast if more than one unique value is present.
- // Collect all schemas for hashing. - // TODO(mjudeikis) Scope is same for all crds so we keep stamping it over. We might want to change this - scope = boundSchema.Spec.Scope - schemas = append(schemas, boundSchema) + // Collect all schemas for hashing and validate uniform scope. + schemas = append(schemas, boundSchema)Add just before building export:
// ensureExports: validate uniform scope scopes := map[apiextensionsv1.ResourceScope]struct{}{} for _, s := range schemas { scopes[s.Spec.Scope] = struct{}{} } if len(scopes) > 1 { conditions.MarkFalse(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady, "MixedResourceScopes", conditionsapi.ConditionSeverityError, "Requested resources have mixed scopes (Cluster and Namespaced) which is not supported in one export") return fmt.Errorf("mixed resource scopes") } for s := range scopes { scope = s; break }If needed, I can wire this into a small helper.
🧹 Nitpick comments (37)
test/e2e/framework/clients.go (1)
55-59: Make this a proper test helper and support default-loading fallbackMark as test helper and allow empty kubeconfig to use default loading rules (KUBECONFIG, ~/.kube/config). This makes the helper more flexible for local and CI runs. Also unify the error message style.
func NewRESTConfig(t *testing.T, kubeconfig string) *rest.Config { - config, err := clientcmd.BuildConfigFromFlags("", kubeconfig) - require.NoError(t, err, "Failed to build config from kubeconfig file") - return config + t.Helper() + var config *rest.Config + var err error + if kubeconfig == "" { + loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() + overrides := &clientcmd.ConfigOverrides{} + config, err = clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, overrides).ClientConfig() + } else { + config, err = clientcmd.BuildConfigFromFlags("", kubeconfig) + } + require.NoError(t, err, "failed to build rest.Config from kubeconfig or defaults") + return config }Optional: consider applying t.Helper() to the other helpers in this file for clearer test failure locations:
func DynamicClient(t *testing.T, config *rest.Config) dynamic.Interface { t.Helper() // ... }deploy/patches/kube-bind.io_boundschemas.yaml-patch (1)
5-5: Make the conversion patch resilient across CRD variantsUsing JSON6902
op: addon/spec/conversionwill fail if that field already exists in upstream CRDs. Prefer a replace or a strategic-merge patch to be idempotent across versions.If you stick with JSON6902, consider replacing the whole node:
- op: add - path: /spec/conversion - value: - strategy: None + op: replace + path: /spec/conversion + value: + strategy: NoneAlternatively switch this file to a strategic-merge patch (
patchesStrategicMerge) with:spec: conversion: strategy: Nonetest/e2e/bind/happy-case_test.go (1)
68-68: Nit: pluralize log for accuracyBootstrap creates at least one CRD; plural reads better.
- t.Logf("Creating CRD on provider side") + t.Logf("Creating CRDs on provider side")kcp/deploy/examples/cowboy.yaml (2)
8-9: Avoid setting status in example manifestsStatus is typically controller-managed; including it in examples can confuse users or be rejected.
-status: - result: "ready to ride"
9-9: Add trailing newlineSatisfy linters and POSIX conventions by ending the file with a newline.
backend/kubernetes/resources/rbac.go (1)
111-114: Tighten verbs on status subresource
listisn’t typically used on*/status; trim to least privilege.- Verbs: []string{"get", "list", "patch", "update"}, + Verbs: []string{"get", "patch", "update"},backend/kubernetes/manager.go (1)
196-215: Remove commented-out dead code or track with TODO/issueKeeping large commented blocks adds drift. Either delete or replace with a brief TODO and a link to a tracking issue.
-/* func (m *Manager) CreateBoundSchema(ctx context.Context, cluster string, name string, u *unstructured.Unstructured) error { - ... - return c.Create(ctx, boundSchema) -} -*/backend/options/options.go (3)
59-60: Fix schema-source version mismatch (comment vs constant).Comment mentions v1alpha2 while the constant is v1alpha1. Align to avoid confusion.
- // APIResourceSchema.v1alpha2.apis.kcp.io + // APIResourceSchema.v1alpha1.apis.kcp.ioAlso applies to: 115-116
119-125: Polish TODO and alias comment typos.Minor grammar/typo fixes.
-// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 -// We should relax these once we happy they work with any schema. +// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 +// We should relax these once we are happy they work with any schema. -var schemaSourceAliases = map[string]string{ - CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests +var schemaSourceAliases = map[string]string{ + CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostly for e2e tests
146-154: Deduplicate and stabilize the “Possible values” list.Current help text order is non-deterministic and may include duplicates. Build a set, then list.
- values := make([]string, 0, len(schemaSourceAliases)) - for _, v := range schemaSourceAliases { - values = append(values, v) - } + valuesSet := sets.Set[string]{} + for _, v := range schemaSourceAliases { + valuesSet.Insert(v) + } + values := sets.List(valuesSet)kcp/go.mod (1)
5-12: Use consistent replaces in go.mod — local during PR, released versions for releasego mod graph/tidy shows both local replaces and resolved release/pseudo-versions (e.g. sdk/apis@v0.4.8, cli@v0.0.0-20250515145715...), which can cause surprising resolution; pick one approach, update kcp/go.mod, and verify with go mod tidy. Files: kcp/go.mod lines 5-12 (also 19-31, 81-83).
kcp/deploy/resources/apiresourceschema-apiserviceexports.kube-bind.io.yaml (1)
491-506: Tighten wording in resources.item descriptions.Minor clarity/grammar improvements.
- resource: - description: |- - resource is the name of the resource. - Note: it is worth noting that you can not ask for permissions for resource provided by a CRD - not provided by an service binding export. + resource: + description: |- + resource is the name of the resource. + Note: you cannot request permissions for a resource defined by a CRD + that is not provided by a service binding export.kcp/deploy/examples/apiexport.yaml (1)
1-11: Add trailing newline.Satisfies linters and keeps diffs cleaner.
apiVersion: apis.kcp.io/v1alpha2 kind: APIExport metadata: name: cowboys-stable spec: resources: - group: wildwest.dev name: cowboys schema: today.cowboys.wildwest.dev storage: - crd: {} + crd: {} +backend/http/handler.go (1)
119-125: Health check route added—consider tiny UX nit.Optionally return a short plaintext body to ease manual checks and some probes.
func (h *handler) handleHealthz(w http.ResponseWriter, r *http.Request) { prepareNoCache(w) - w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok\n")) }backend/controllers/clusterbinding/clusterbinding_controller.go (1)
80-85: Include namespace in error for easier triage.Add the namespace to the error message to disambiguate lookups across namespaces.
- return nil, fmt.Errorf("failed to get BoundSchema %q: %w", name, err) + return nil, fmt.Errorf("failed to get BoundSchema %s/%s: %w", namespace, name, err)kcp/README.md (1)
60-72: Step numbering and claim list clarity.Numbers jump and may confuse readers; also group claims are long—consider splitting with line comments.
-5. Bind the APIExport to the workspace +5. Bind the APIExport to the workspace @@ - --accept-permission-claim rolebindings.rbac.authorization.k8s.io \ - --accept-permission-claim apiresourceschemas.apis.kcp.io + --accept-permission-claim rolebindings.rbac.authorization.k8s.io \ + --accept-permission-claim apiresourceschemas.apis.kcp.ioAnd renumber the next header from “7. Create CRD” to “6. Create CRD”.
Happy to fix the numbering throughout the file.
kcp/deploy/examples/apiresourceschema.yaml (1)
48-48: Add trailing newline.Many tools and linters expect a newline at EOF.
backend/controllers/serviceexport/serviceexport_reconcile.go (2)
57-60: Verify BoundSchema naming derivation.Using
res.Resource + "." + res.Groupmay not match actual BoundSchema names (e.g., generational prefixes like “today.cowboys.wildwest.dev”). Confirm the canonical name source and consider storing it explicitly in APIServiceExport.Spec.Resources.I can add a small helper to compute/validate the expected BoundSchema name.
70-79: Deterministic hashing and persistence of annotation update.
- Ensure
helpers.BoundSchemasSpecHashis order-independent; otherwise, sort input deterministically before hashing.- The annotation is mutated in-memory; confirm the caller persists the change with
client.Update. If not, return a signal or perform the update here.- hash := helpers.BoundSchemasSpecHash(schemas) + // Optionally: sort by namespace/name before hashing to guarantee stability. + hash := helpers.BoundSchemasSpecHash(schemas)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (2)
266-273: Schemaless: confirm implications for pruning/defaulting and subresources.Using a bare object with x-kubernetes-preserve-unknown-fields=true disables pruning and may limit server-side defaulting/validation. Double-check that:
- Any status or scale subresource assumptions still hold.
- CEL/validation rules elsewhere don’t rely on pruned shapes.
If intentional, consider documenting this in release notes for operators.
124-131: Nit: duplicated enum in allOf block.The informerScope enum is listed twice under allOf with identical values. Safe to de-duplicate to reduce noise.
informerScope: - allOf: - - enum: - - Cluster - - Namespaced - - enum: - - Cluster - - Namespaced + enum: + - Cluster + - Namespaced description: |- InformerScope indicates whether the informer for defined custom resource is cluster- or namespace-scoped. type: stringbackend/controllers/serviceexport/serviceexport_controller.go (1)
131-138: Compare and update only status to avoid unnecessary updates.DeepEqual on the whole object may trigger updates unrelated to status. Compare Status only.
- if !equality.Semantic.DeepEqual(original, apiServiceExport) { - err := client.Status().Update(ctx, apiServiceExport) + if !equality.Semantic.DeepEqual(original.Status, apiServiceExport.Status) { + err := client.Status().Update(ctx, apiServiceExport) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err) } logger.Info("APIServiceExport status updated", "namespace", apiServiceExport.Namespace, "name", apiServiceExport.Name) }pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go (1)
108-111: Consider adding retry logic for status updates.The
updateRemoteBoundSchemafunction performs a status update that might fail due to conflicts in concurrent reconciliation scenarios. Consider implementing retry logic with exponential backoff for resilience.updateRemoteBoundSchema: func(ctx context.Context, boundSchema *kubebindv1alpha2.BoundSchema) error { - _, err := providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).UpdateStatus(ctx, boundSchema, metav1.UpdateOptions{}) - return err + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + _, err := providerBindClient.KubeBindV1alpha2().BoundSchemas(providerNamespace).UpdateStatus(ctx, boundSchema, metav1.UpdateOptions{}) + return err + }) },Note: You'll need to import
k8s.io/client-go/util/retryif not already imported.pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile_test.go (1)
110-127: Add a case for empty versions in APIServiceExportRequestCurrent tests don’t cover when a resource omits versions (provider-defaulted). Recommend adding a test to ensure the reconciler still succeeds in that scenario.
I can draft the test table entry if you want.
Also applies to: 129-140
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (3)
164-168: Reason/message still refers to APIResourceSchemaUpdate reason and message to the new term to avoid confusion.
- "APIResourceSchemaNotFound", + "BoundSchemaNotFound", @@ - "APIResourceSchema %s in the service provider cluster not found", + "BoundSchema %s in the service provider cluster not found",
225-228: Use structured logging; avoid printf-style placeholdersklog ignores %s placeholders. Provide fields explicitly.
- logger.Info("Deleting service binding request %s/%s", req.Namespace, req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time)) + logger.Info("Deleting service binding request", "namespace", req.Namespace, "name", req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time))
76-86: Validate schemaSource format early or switch to explicit GVK configParsing "Kind.Version.Group" is unconventional and brittle. Either validate once at reconciler init, or accept explicit GVK fields.
Want me to add a small parser/validator and unit test?
deploy/crd/kube-bind.io_boundschemas.yaml (2)
26-50: Add a name validation like CRD naming conventionTo prevent drift, enforce metadata.name == spec.names.plural + "." + spec.group.
schema: openAPIV3Schema: description: BoundSchema properties: apiVersion: @@ type: object + x-kubernetes-validations: + - message: "metadata.name must be <plural>.<group>" + rule: self.metadata.name == self.spec.names.plural + "." + self.spec.group
126-137: Duplicate enum under informerScopeYou have two identical enum entries under allOf. Collapse to a single enum for clarity.
- informerScope: - allOf: - - enum: - - Cluster - - Namespaced - - enum: - - Cluster - - Namespaced + informerScope: + enum: + - Cluster + - Namespacedpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (3)
135-141: Avoid redundant GETs for BoundSchema.You fetch each BoundSchema in getSchemasFromExport and then fetch it again in referenceBoundAPIResourceSchema. Pass the already-fetched schema to avoid a second API call.
Apply:
- if err := r.referenceBoundAPIResourceSchema(ctx, binding, schema.Name); err != nil { + if err := r.referenceBoundAPIResourceSchema(ctx, binding, schema); err != nil {and change the helper:
-func (r *reconciler) referenceBoundAPIResourceSchema(ctx context.Context, binding *kubebindv1alpha2.APIServiceBinding, name string) error { - boundSchema, err := r.getBoundSchema(ctx, name) - if err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to get BoundSchema %s: %w", name, err) - } - if boundSchema == nil { return nil } +func (r *reconciler) referenceBoundAPIResourceSchema(_ context.Context, binding *kubebindv1alpha2.APIServiceBinding, boundSchema *kubebindv1alpha2.BoundSchema) error {Also applies to: 153-165
189-201: Preserve/merge metadata deterministically.You construct a new CRD, append an ownerRef, but then overwrite ObjectMeta entirely with existing.ObjectMeta on update, losing any new labels/annotations set by BoundSchemaToCRD. Prefer merging required labels/annotations and ensuring a single controller OwnerReference.
Example:
- crd.ObjectMeta = existing.ObjectMeta + // keep server-managed fields and existing labels/annotations, but ensure required labels/annotations present + crd.SetResourceVersion(existing.GetResourceVersion()) + crd.UID = existing.UID + // merge labels + if crd.Labels == nil { crd.Labels = map[string]string{} } + for k, v := range existing.Labels { if _, ok := crd.Labels[k]; !ok { crd.Labels[k] = v } } + // merge annotations + if crd.Annotations == nil { crd.Annotations = map[string]string{} } + for k, v := range existing.Annotations { if _, ok := crd.Annotations[k]; !ok { crd.Annotations[k] = v } }Also applies to: 222-235
266-276: Tighten error wording.The returned error says “Schema” but this code paths deals with BoundSchema objects. Adjust for clarity.
- return nil, fmt.Errorf("failed to get Schema %s: %w", name, err) + return nil, fmt.Errorf("failed to get BoundSchema %s: %w", name, err)pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (2)
215-217: Avoid OrDie in reconciliation path.Panic on transient config issues will crash the controller. Prefer returning an error and letting the queue requeue.
- dynamicConsumerClient := dynamicclient.NewForConfigOrDie(r.consumerConfig) - dynamicProviderClient := dynamicclient.NewForConfigOrDie(r.providerConfig) + dynamicConsumerClient, err := dynamicclient.NewForConfig(r.consumerConfig) + if err != nil { return err } + dynamicProviderClient, err := dynamicclient.NewForConfig(r.providerConfig) + if err != nil { return err }
379-395: Update messages/reasons to BoundSchema terminology and signal invalidity correctly.The reason and message still reference BoundAPIResourceSchemas. Also, allValid only flips on update errors, not on actual CRD invalid conditions.
- "BoundAPIResourceSchemasNotValid", - "One or more BoundAPIResourceSchemas are not valid", + "BoundSchemasNotValid", + "One or more BoundSchemas are not valid",Optional: derive allValid from CRD conditions (e.g., Established=True, NamesAccepted=True), not only update errors.
Also applies to: 319-377
sdk/apis/kubebind/v1alpha2/boundchema_types.go (3)
220-227: Doc/constant mismatch: “Ready” vs “Valid”.The comment mentions BoundSchemaReady but the exported constant is BoundSchemaValid. Align naming or wording to avoid confusion.
- // BoundSchemaReady indicates that the API resource schema is ready. + // BoundSchemaValid indicates that the API resource schema is valid.Alternatively rename the constant to BoundSchemaReady if that’s the intended condition type.
276-283: ListMeta JSON tag parity.Kubernetes lists typically use
json:"metadata,omitempty". Consider adding omitempty for consistency.- metav1.ListMeta `json:"metadata"` + metav1.ListMeta `json:"metadata,omitempty"`
19-27: File name typo (“boundchema”).Consider renaming this file to boundschema_types.go for consistency and discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
kcp/go.sumis excluded by!**/*.sum
📒 Files selected for processing (69)
backend/controllers/clusterbinding/clusterbinding_controller.go(1 hunks)backend/controllers/clusterbinding/clusterbinding_reconcile.go(2 hunks)backend/controllers/serviceexport/serviceexport_controller.go(3 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(2 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(5 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(3 hunks)backend/http/handler.go(1 hunks)backend/kubernetes/manager.go(1 hunks)backend/kubernetes/resources/rbac.go(1 hunks)backend/options/options.go(2 hunks)backend/server.go(1 hunks)cli/cmd/crd2apiresourceschema/cmd/crd2apiresourceschema.go(0 hunks)cli/cmd/crd2apiresourceschema/main.go(0 hunks)cli/pkg/crd2apiresourceschema/cmd/cmd.go(0 hunks)cli/pkg/crd2apiresourceschema/plugin/crd2apiresourceschema.go(0 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_apiresourceschemas.yaml(0 hunks)deploy/crd/kube-bind.io_apiserviceexports.yaml(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)deploy/examples/apiresourceschema.yaml(1 hunks)deploy/patches/kube-bind.io_boundapiresourceschemas.yaml-patch(0 hunks)deploy/patches/kube-bind.io_boundschemas.yaml-patch(1 hunks)kcp/README.md(3 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-apiresourceschemas.kube-bind.io.yaml(0 hunks)kcp/deploy/resources/apiresourceschema-apiserviceexports.kube-bind.io.yaml(2 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)kcp/go.mod(4 hunks)pkg/indexers/serviceexport.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(6 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile_test.go(5 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go(1 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go(8 hunks)pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile_test.go(4 hunks)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go(1 hunks)sdk/apis/kubebind/v1alpha2/boundapiresourceschema_types.go(0 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(4 hunks)sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema_test.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundapiresourceschema.go(0 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)sdk/apis/kubebind/v1alpha2/register.go(1 hunks)sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go(5 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundapiresourceschema.go(0 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_kubebind_client.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/generated_expansion.go(1 hunks)sdk/client/clientset/versioned/typed/kubebind/v1alpha2/kubebind_client.go(2 hunks)sdk/client/informers/externalversions/generic.go(1 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/informers/externalversions/kubebind/v1alpha2/interface.go(2 hunks)sdk/client/listers/kubebind/v1alpha2/apiresourceschema.go(0 hunks)sdk/client/listers/kubebind/v1alpha2/boundapiresourceschema.go(0 hunks)sdk/client/listers/kubebind/v1alpha2/boundschema.go(1 hunks)sdk/client/listers/kubebind/v1alpha2/expansion_generated.go(1 hunks)test/e2e/bind/fixtures/provider/apiresourceschema-foo.yaml(0 hunks)test/e2e/bind/fixtures/provider/apiresourceschema-mangodb.yaml(0 hunks)test/e2e/bind/happy-case_test.go(1 hunks)test/e2e/framework/backend.go(1 hunks)test/e2e/framework/bind.go(0 hunks)test/e2e/framework/browser.go(2 hunks)test/e2e/framework/clients.go(2 hunks)
💤 Files with no reviewable changes (19)
- deploy/patches/kube-bind.io_boundapiresourceschemas.yaml-patch
- sdk/client/informers/externalversions/kubebind/v1alpha2/boundapiresourceschema.go
- test/e2e/framework/bind.go
- sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema_test.go
- sdk/apis/kubebind/v1alpha2/helpers/boundapiresourceschema.go
- cli/cmd/crd2apiresourceschema/main.go
- kcp/deploy/resources/apiresourceschema-apiresourceschemas.kube-bind.io.yaml
- sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundapiresourceschema.go
- test/e2e/bind/fixtures/provider/apiresourceschema-foo.yaml
- cli/cmd/crd2apiresourceschema/cmd/crd2apiresourceschema.go
- sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundapiresourceschema.go
- test/e2e/bind/fixtures/provider/apiresourceschema-mangodb.yaml
- cli/pkg/crd2apiresourceschema/plugin/crd2apiresourceschema.go
- sdk/client/listers/kubebind/v1alpha2/apiresourceschema.go
- cli/pkg/crd2apiresourceschema/cmd/cmd.go
- deploy/crd/kube-bind.io_apiresourceschemas.yaml
- sdk/apis/kubebind/v1alpha2/boundapiresourceschema_types.go
- sdk/apis/kubebind/v1alpha2/helpers/apiresourceschema.go
- sdk/client/listers/kubebind/v1alpha2/boundapiresourceschema.go
🧰 Additional context used
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/cowboy.yaml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiexport.yaml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.27.2)
kcp/README.md
[high] 47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (46)
test/e2e/framework/browser.go (1)
20-20: Import looks good.strings is needed for path checks here.
test/e2e/framework/clients.go (1)
28-28: LGTM: import needed for kubeconfig loadingThe new clientcmd import is appropriate for NewRESTConfig.
deploy/examples/apiresourceschema.yaml (1)
5-6: Label addition LGTMLabel key/value look correct for selectors and tooling that rely on string values.
cli/pkg/kubectl/bind/plugin/bind.go (1)
162-162: Version-mismatch message improvement LGTMIncluding both actual and expected versions is clearer for users.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_kubebind_client.go (1)
48-49: LGTM: BoundSchemas fake accessor aligns with the rename.Matches the new v1alpha2.BoundSchemaInterface and keeps parity with other namespaced accessors.
kcp/go.mod (1)
192-195: Add TODO + revert plan for temporary go.mod replace entries (upstream PR #62 still open)PR #62 remains open and I couldn't find a released tag that includes it; add the TODOs and a revert plan referencing PR #62 in kcp/go.mod. (github.com)
+// TODO: Temporary replace to unblock dev; revert once upstream merges: +// https://github.com/kubernetes-sigs/multicluster-runtime/pull/62 replace sigs.k8s.io/multicluster-runtime => github.com/mjudeikis/sigs-multicluster-runtime v0.0.0-20250818101434-d8ebc45e169b +// TODO: Temporary replace; track upstream status and remove when possible. replace github.com/kcp-dev/multicluster-provider => github.com/mjudeikis/kcp-multicluster-provider v0.0.0-20250818102159-3d31cbb06ebetest/e2e/framework/backend.go (1)
113-119: Incorrect: grpc.NewClient is public and the recommended API (gRPC‑Go v1.63+); do not switch to grpc.Dial.
The diff in the original comment is wrong — keep grpc.NewClient at both call sites (lines 113–119 and 134–137).Likely an incorrect or invalid review comment.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/generated_expansion.go (1)
29-30: LGTM: Expansion type rename is consistent.BoundSchemaExpansion matches the API rename.
sdk/client/informers/externalversions/generic.go (1)
78-79: BoundSchemas informer wiring looks correct — verify legacy lookupsCase matches the new plural and uses the v1alpha2 informer; the ripgrep run returned no matches (empty output). Because that isn't definitive, re-run the search locally or confirm there are no remaining occurrences of "apiresourceschemas" or "boundapiresourceschemas" to avoid runtime "no informer found" errors.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go (1)
106-108: Switch to namespaced BoundSchema getter—LGTM.Namespace scoping via providerNamespace aligns with the new model. Ensure callers pass the canonical name (resource.group).
sdk/apis/kubebind/v1alpha2/register.go (1)
49-51: Registering BoundSchema types—LGTM.Types are added to the scheme; no further action.
sdk/client/listers/kubebind/v1alpha2/expansion_generated.go (1)
49-56: BoundSchema expansion interfaces—LGTM.Names and scoping match the new resource.
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/kubebind_client.go (1)
36-36: LGTM — BoundSchemas getter and generated client pieces present.
BoundSchemaInterface and newBoundSchemas found in sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go (lines 40, 60).sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
81-88: Resources field validations—LGTM.Required + MinItems + immutability via CEL are appropriate for this list.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
43-48: Reconciler inputs updated for BoundSchema—LGTM.Signatures and intent align with the refactor.
backend/server.go (1)
176-183: Plumbed SchemaSource looks fine; add validation and defaults.Ensure
c.Options.SchemaSourceis validated early (allowed set, e.g., "apiresourceschemas", "boundschemas", etc.) and has a sane default to avoid misconfig at runtime.Would you like me to add an options.Validate() and wire a default into options parsing?
sdk/client/clientset/versioned/typed/kubebind/v1alpha2/fake/fake_boundschema.go (1)
28-53: BoundSchema fake wiring LGTM.Resource/kind and list plumbing via gentype looks correct and namespace-aware.
kcp/README.md (1)
76-80: Confirm workflow order and --schema-source alias
- The flag accepts the alias "apiresourceschemas" and canonicalizes it to "APIResourceSchema.v1alpha1.apis.kcp.io" (backend/options/options.go — SchemaSource values + schemaSourceAliases). Default is "CustomResourceDefinition.v1.apiextensions.k8s.io".
- kcp/README.md (lines 76–80) doesn’t show passing --schema-source and doesn’t document waiting for the APIResourceSchema / APIExport to be Ready before running the recursive bind; no automatic readiness check was found in the quick code search — add a pre-bind wait or document the required ordering.
sdk/client/informers/externalversions/kubebind/v1alpha2/interface.go (2)
35-36: New BoundSchemas() accessor: LGTM.Wiring matches other informers and returns the typed boundSchemaInformer.
72-75: Namespace-aware informer is appropriate.Using namespace in the informer aligns with BoundSchema being namespaced.
Please confirm all call sites were updated to use BoundSchemas() instead of the removed BoundAPIResourceSchemas().
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (4)
64-65: Threading schemaSource through the reconciler: LGTM.No issues spotted with the new parameter.
86-93: BoundSchema cache get uses NamespacedName: LGTM.Correct for a namespaced BoundSchema.
235-249: Watch wiring for BoundSchema: LGTM.Mapper function and watch registration match the new type.
153-161: Incorrect — boundSchema.Name already matches the index key; no change required.IndexServiceExportRequestByGroupResource builds keys as "resource.group" (pkg/indexers/serviceexportrequest.go). BoundSchema.Name is set to that same format when created in ensureBoundSchemas (backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go: boundSchema.Name = res.Resource + "." + res.Group). The mapper using boundSchema.Name is correct.
Likely an incorrect or invalid review comment.
backend/controllers/serviceexport/serviceexport_controller.go (2)
61-64: Indexer registration for ServiceExportByBoundSchema: LGTM.Good to pre-index for efficient reverse lookups.
143-168: Mapper key should include namespace (align with index key).File: backend/controllers/serviceexport/serviceexport_controller.go Lines: 143-168
Use namespace/name or the same compound key used by the ServiceExportByBoundSchema index.
- boundSchema := obj.(*kubebindv1alpha2.BoundSchema) - boundSchemaKey := boundSchema.Name + boundSchema := obj.(*kubebindv1alpha2.BoundSchema) + boundSchemaKey := fmt.Sprintf("%s/%s", boundSchema.Namespace, boundSchema.Name)Confirm IndexServiceExportByBoundSchema uses the same key format; adjust both sides if not.
sdk/client/listers/kubebind/v1alpha2/boundschema.go (1)
50-53: Namespace lister wiring: LGTM.Namespaced indexer embedding matches the generated pattern.
pkg/konnector/controllers/cluster/serviceexport/serviceexport_controller.go (2)
102-104: LGTM!The
getCRDfunction maintains the same signature and behavior as before, correctly returning a CustomResourceDefinition by name.
105-107: LGTM!The new
getRemoteBoundSchemafunction correctly fetches a BoundSchema from the provider namespace, aligning with the migration from APIResourceSchema to BoundSchema.pkg/indexers/serviceexport.go (2)
26-28: LGTM!The constant rename from
ServiceExportByAPIResourceSchematoServiceExportByBoundSchemacorrectly reflects the migration to BoundSchema.
39-53: No action required — 'resource.group' index keys match consumers.Index registration and lookups use the same key format: index registered in backend/controllers/serviceexport/serviceexport_controller.go and queried with boundSchemaKey := boundSchema.Name (boundSchema.Name is set to res.Resource + "." + res.Group in backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go and other reconciles). Repo-wide grep shows many sites constructing/consuming keys as res.Resource + "." + res.Group — no incompatible usages found.
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
33-54: LGTM!The
CRDToBoundSchemafunction correctly transforms a CRD into a BoundSchema with proper type metadata and spec initialization.sdk/client/informers/externalversions/kubebind/v1alpha2/boundschema.go (3)
36-41: LGTM!The BoundSchemaInformer interface is correctly defined with appropriate methods for accessing the informer and lister.
43-47: LGTM!The boundSchemaInformer struct correctly includes the namespace field to support namespace-scoped informers, aligning with the refactor to make BoundSchema namespace-scoped.
49-91: LGTM!The informer constructor functions are correctly updated to work with namespace-scoped BoundSchema resources. The ListWatch implementation properly targets the correct namespace.
pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile_test.go (3)
113-120: LGTM!The
newGetBoundSchemahelper function correctly simulates fetching a BoundSchema and returns appropriate NotFound errors with the correct resource type.
122-127: LGTM!The
newUpdateBoundSchemahelper correctly captures the updated BoundSchema for test verification.
129-140: GroupResource fields verified — no action required.
kubebindv1alpha2.GroupResource defines Group (string) and Resource (string) and is inlined by APIServiceExportRequestResource (sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go); the test's kubebindv1alpha2.GroupResource{Group: "example.com", Resource: "foos"} is correct.sdk/client/clientset/versioned/typed/kubebind/v1alpha2/boundschema.go (1)
33-69: Generated client rename looks consistentResource path, types, and namespace-scoping align with BoundSchema. No issues spotted.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile_test.go (1)
48-51: Tests align with new BoundSchema and APIServiceExport.Resources shapeTest wiring and NotFound paths updated correctly. Good coverage for create vs foreign CRD cases.
Also applies to: 61-64, 82-87
deploy/crd/kube-bind.io_boundschemas.yaml (1)
14-20: Rename to BoundSchema CRD: looks correctKind, listKind, plural/singular updates and shortName retained. No issues.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
38-43: Wiring for getBoundSchema looks good.New dependency points are clear and cohesive with the BoundSchema-centric flow.
pkg/konnector/controllers/cluster/serviceexport/serviceexport_reconcile.go (1)
59-64: Remote BoundSchema hooks look correct.The new getters/updaters align with the schemaless/remote status propagation design.
sdk/apis/kubebind/v1alpha2/zz_generated.deepcopy.go (1)
31-58: Generated deepcopy aligns with new types.APICRDSpec/BoundSchema/BoundSchemaList/Status deep-copies look correct and consistent with the new API surface. Ensure codegen is re-run after any further type tweaks.
Also applies to: 690-709, 719-749, 769-783, 786-813
sdk/apis/kubebind/v1alpha2/boundchema_types.go (2)
33-36: CRD annotations look good.Namespaced scope and shortName=bas are reasonable defaults.
54-65: Optional cross-field validation for Scope vs InformerScope.If Namespaced CRDs should never use Cluster informer (or vice versa), add XValidation to enforce allowed combinations. If mixed modes are intentional, ignore.
Would you like me to propose XValidation rules to enforce the desired combinations?
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
3 times, most recently
from
4092bd7 to
fa981c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
40-43: Overly broad permission claim on apiresourceschemas.Reduce from
'*'to read-only verbs unless writes are required.kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (1)
5-5: Include namespace in BoundSchema event mappers
- Verified: getBoundSchema implementations use cache.Get with types.NamespacedName (examples: backend/controllers/serviceexport/serviceexport_controller.go:70–73; backend/controllers/serviceexportrequest/serviceexportrequest_controller.go:86–89; backend/controllers/clusterbinding/clusterbinding_controller.go:80–83).
- Issue: getBoundSchemaMapper builds boundSchemaKey := boundSchema.Name (examples: backend/controllers/serviceexport/serviceexport_controller.go:156–159; backend/controllers/serviceexportrequest/serviceexportrequest_controller.go:152–156) and omits namespace — this can cause cross-namespace collisions when enqueuing.
- Fix: Change mappers to include namespace (e.g., use types.NamespacedName{Namespace: boundSchema.Namespace, Name: boundSchema.Name} or otherwise ensure the enqueued request contains the namespace to match cache lookups).
🧹 Nitpick comments (7)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml (1)
48-48: Add trailing newline.A missing EOF newline trips some linters/formatters.
sdk/apis/kubebind/v1alpha2/boundchema_types.go (2)
137-165: Conversion webhook config lacks Service field and mutual‑exclusion validation.Parity with Kubernetes requires supporting in‑cluster Service references and enforcing “exactly one of service or url”.
Apply within WebhookClientConfig:
-// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// Exactly one of service or url must be specified. +// +kubebuilder:validation:XValidation:rule="(has(self.service) && !has(self.url)) || (!has(self.service) && has(self.url))",message="exactly one of service or url must be specified" type WebhookClientConfig struct { + // service references a service within the cluster. Either service or url must be set. + // +optional + Service *ServiceReference `json:"service,omitempty"` // url gives the location of the webhook, in standard URL form // (`scheme://host:port/path`). @@ // +kubebuilder:validation:Format=uri URL *string `json:"url,omitempty"` @@ CABundle []byte `json:"caBundle,omitempty"` }Add the type (outside this hunk):
+// ServiceReference holds a reference to a service for contacting a webhook. +// Mirrors admissionregistration.k8s.io/v1 ServiceReference (subset). +type ServiceReference struct { + // namespace is the namespace of the service. + // +kubebuilder:validation:MinLength=1 + Namespace string `json:"namespace"` + // name is the name of the service. + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + // path is an optional URL path. + // +optional + Path *string `json:"path,omitempty"` + // port is an optional service port. Defaults to 443. + // +optional + Port *int32 `json:"port,omitempty"` +}
225-232: Fix comment to match constant name.“BoundSchemaReady” → “BoundSchemaValid” to avoid confusion.
- // BoundSchemaReady indicates that the API resource schema is ready. + // BoundSchemaValid indicates that the API resource schema is valid.kcp/deploy/examples/cowboy.yaml (1)
5-5: Add trailing newline.Keeps YAML linters happy.
kcp/deploy/examples/sheriff.yaml (1)
4-4: Add trailing newline.Minor formatting nit.
kcp/deploy/examples/apiexport.yaml (1)
16-16: Add trailing newline.Minor formatting fix.
kcp/deploy/examples/apiresourceschema-cowboys.yaml (1)
48-48: Add a newline at the end of the file.YAMLlint flagged that the file is missing a newline at the end.
Apply this diff to add the newline:
subresources: status: {} +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/controllers/clusterbinding/clusterbinding_reconcile.go(3 hunks)backend/controllers/serviceexport/serviceexport_controller.go(5 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(1 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(6 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(3 hunks)backend/http/handler.go(2 hunks)backend/kubernetes/manager.go(0 hunks)backend/options/options.go(2 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-cowboys.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-sheriffs.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-cluster.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-namespaced.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/examples/sheriff.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)pkg/konnector/controllers/cluster/cluster_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(7 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(5 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/kubernetes/manager.go
🚧 Files skipped from review as they are similar to previous changes (11)
- kcp/deploy/examples/apiserviceexport-namespaced.yaml
- pkg/konnector/controllers/cluster/cluster_controller.go
- backend/controllers/serviceexport/serviceexport_controller.go
- backend/http/handler.go
- pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go
- backend/options/options.go
- deploy/crd/kube-bind.io_boundschemas.yaml
- backend/controllers/clusterbinding/clusterbinding_reconcile.go
- kcp/deploy/examples/apiserviceexport-cluster.yaml
- backend/controllers/serviceexportrequest/serviceexportrequest_controller.go
- backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
📚 Learning: 2025-09-12T08:55:41.816Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
backend/controllers/serviceexport/serviceexport_reconcile.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.gosdk/apis/kubebind/v1alpha2/boundchema_types.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml
📚 Learning: 2025-09-12T09:05:29.743Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
backend/controllers/serviceexport/serviceexport_reconcile.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.gokcp/deploy/resources/apiexport-kube-bind.io.yamlsdk/apis/kubebind/v1alpha2/boundchema_types.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml
📚 Learning: 2025-09-12T08:40:15.272Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
backend/controllers/serviceexport/serviceexport_reconcile.gopkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gokcp/deploy/resources/apiexport-kube-bind.io.yamlsdk/apis/kubebind/v1alpha2/boundchema_types.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml
🧬 Code graph analysis (5)
cli/pkg/kubectl/bind/plugin/bind.go (1)
sdk/apis/kubebind/v1alpha2/register.go (1)
GroupVersion(35-35)
backend/controllers/serviceexport/serviceexport_reconcile.go (5)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (3)
APIServiceExport(55-66)APIServiceExportConditionProviderInSync(36-36)SourceSpecHashAnnotationKey(27-27)sdk/apis/third_party/conditions/util/conditions/setter.go (2)
MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemasSpecHash(117-125)
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (10)
BoundSchema(36-42)NamespacedScope(51-51)BoundSchemaSpec(60-70)InformerScope(47-47)APICRDSpec(72-104)CustomResourceConversion(108-119)ConversionStrategyType(168-168)WebhookConversion(122-135)WebhookClientConfig(138-165)APIResourceVersion(171-223)sdk/apis/kubebind/v1alpha2/register.go (1)
SchemeGroupVersion(39-39)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (6)
sdk/apis/kubebind/v1alpha1/apiservicebinding_types.go (2)
APIServiceBinding(63-73)APIServiceBindingConditionConnected(37-37)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
APIServiceExportRequest(46-60)APIServiceExportRequestPhaseFailed(154-154)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/third_party/conditions/util/conditions/setter.go (1)
MarkFalse(120-122)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemaToCRD(127-196)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (2)
ConditionType(48-48)Conditions(92-92)
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/apiexport.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-cowboys.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/cowboy.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/sheriff.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (4)
- GitHub Check: verify
- GitHub Check: lint
- GitHub Check: go-test-e2e
- GitHub Check: go-test
🔇 Additional comments (7)
cli/pkg/kubectl/bind/plugin/bind.go (1)
162-162: Clearer version mismatch message — LGTM. Also confirm APIVersion semantics.Nice improvement. Please double‑check that
provider.APIVersionis a plain version string (e.g., "v1alpha2") to matchkubebindv1alpha2.GroupVersion; if it’s "group/version", compare againstSchemeGroupVersion.String()instead.sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
117-125: Resolved — call sites handle the returned error.Both call sites (backend/controllers/serviceexport/serviceexport_reconcile.go:72–74 and backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:216–218) assign (hash, err) and check err; no ignored errors or mismatched assigns found.
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (1)
13-19: Good migration to BoundSchema.The migration from BoundAPIResourceSchema to BoundSchema is implemented correctly with properly updated kind names, plural/singular forms, and descriptions. The resource is appropriately set as Namespaced.
backend/controllers/serviceexport/serviceexport_reconcile.go (1)
49-90: LGTM! Clean migration to BoundSchema with proper namespace handling.The refactoring correctly:
- Updates the function signature to include namespace parameter
- Properly handles NotFound errors with clear condition messages
- Uses the new BoundSchemasSpecHash helper for hash calculation
- Maintains the annotation update logic appropriately
The namespace-aware
getBoundSchemacall aligns with BoundSchema being a namespaced resource.pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (3)
82-95: Good error handling for failed export requests.Excellent addition to check the APIServiceExportRequest status and propagate failure messages to the binding. This provides better visibility when export creation fails on the provider side.
282-297: Clean migration to BoundSchema-based schema fetching.The getSchemasFromExport method correctly uses the new BoundSchema type and properly handles errors when fetching schemas for each resource in the export.
169-203: Resolved — getBoundSchema uses controller-configured providerNamespace. getBoundSchema calls BoundSchemas(providerNamespace).Get(...), so the namespace is the controller's providerNamespace (see pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go lines 110–112).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
backend/http/handler.go (3)
255-258: Bug: base64 decode uses StdEncoding but state is encoded with URLEncoding.This breaks OAuth state decoding for characters like '-'/'_'. Use URLEncoding to match the encoder.
Apply this diff:
- decoded, err := base64.StdEncoding.DecodeString(state) + decoded, err := base64.URLEncoding.DecodeString(state)
268-269: Sign/verify OAuth2 state to prevent CSRF/state swapping.The TODO leaves a real security gap. Sign state (HMAC) or use securecookie to encode+sign; verify on callback.
I can wire this using an HMAC over the state payload with
cookieSigningKeyand reject invalid/missing signatures.
293-301: Secure cookies only in dev; gate by TLS/X-Forwarded-Proto.Hardcoding
secure := falserisks production cookies over HTTP. Derive from request and allow override.Apply this diff:
- // setting to false so it works over http://localhost - secure := false + // Use Secure cookies when behind TLS or a trusted proxy signaling HTTPS. + isTLS := r.TLS != nil || strings.EqualFold(r.Header.Get("X-Forwarded-Proto"), "https") + secure := isTLSpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (2)
209-217: Do not overwrite ObjectMeta; it drops ownerRefs and annotations.Assigning the whole ObjectMeta discards the new OwnerReference and any annotations/labels set earlier. Preserve metadata and only carry over what’s required for update.
newReference := metav1.OwnerReference{ @@ crd.OwnerReferences = append(crd.OwnerReferences, newReference) @@ - crd.ObjectMeta = existing.ObjectMeta + // Preserve existing server-side metadata without dropping desired fields set on crd. + crd.ResourceVersion = existing.ResourceVersion + crd.Labels = existing.Labels + crd.Annotations = existing.Annotations + // existing must already be owned by the binding (checked below), so keep existing ownerRefs to avoid dupes. + crd.OwnerReferences = existing.OwnerReferencesAlso applies to: 250-252
251-261: Swallowed update error; and wrong name in error message.
- On non-Invalid errors you return nil, hiding failures. Return the error.
- Use crd.Name in the message, not binding.Name.
- if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err } else if errors.IsInvalid(err) { @@ - "CustomResourceDefinition %s cannot be updated: %s", - binding.Name, err, + "CustomResourceDefinition %s cannot be updated: %s", + crd.Name, err,backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (2)
179-204: Cluster-scoped isolation decision uses only the last schema’s scope.Mixed resources can misconfigure isolation. Track if any schema is cluster-scoped.
- var schemas []*kubebindv1alpha2.BoundSchema - var scope apiextensionsv1.ResourceScope + var schemas []*kubebindv1alpha2.BoundSchema + hasClusterScoped := false @@ - // Collect all schemas for hashing. - // TODO(mjudeikis) Scope is same for all crds so we keep stamping it over. We might want to change this - scope = boundSchema.Spec.Scope + // Collect all schemas for hashing and detect if any are cluster-scoped. + if boundSchema.Spec.Scope == apiextensionsv1.ClusterScoped { + hasClusterScoped = true + } schemas = append(schemas, boundSchema) @@ - if scope == apiextensionsv1.ClusterScoped { + if hasClusterScoped { export.Spec.ClusterScopedIsolation = r.clusterScopedIsolation }Also applies to: 232-234
257-260: Don’t overwrite Succeeded with Failed after 1 minute.This flips successful requests to Failed unconditionally.
- if time.Since(req.CreationTimestamp.Time) > time.Minute { - req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed - req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) - } + if time.Since(req.CreationTimestamp.Time) > time.Minute && + req.Status.Phase != kubebindv1alpha2.APIServiceExportRequestPhaseSucceeded { + req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed + req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) + }
♻️ Duplicate comments (3)
backend/controllers/serviceexport/serviceexport_controller.go (1)
92-97: RBAC API group mismatch (kubebind.k8s.io vs kube-bind.io).Types register group "kube-bind.io"; RBAC uses "kubebind.k8s.io". Align to the canonical group.
-//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch -//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas/status,verbs=get;update;patch +//+kubebuilder:rbac:groups=kube-bind.io,resources=boundschemas,verbs=get;list;watch +//+kubebuilder:rbac:groups=kube-bind.io,resources=boundschemas/status,verbs=get;update;patchbackend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
218-225: Phase‑only status update on error acknowledged.Leaving as‑is per prior discussion; revisit later if conditions/messages need persistence.
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
137-165: Add Service to WebhookClientConfig and enforce “exactly one of service or url”.Parity with k8s admissionregistration improves in-cluster resolution and validation.
-// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// Exactly one of service or url must be specified. +// +kubebuilder:validation:XValidation:rule="(has(self.service) && !has(self.url)) || (!has(self.service) && has(self.url))",message="exactly one of service or url must be specified" type WebhookClientConfig struct { + // service references a service within the cluster. Either service or url must be set. + // +optional + Service *ServiceReference `json:"service,omitempty"` @@ CABundle []byte `json:"caBundle,omitempty"` } + +// ServiceReference holds a reference to a service for contacting a webhook. +// Mirrors admissionregistration.k8s.io/v1 ServiceReference (subset). +type ServiceReference struct { + // namespace is the namespace of the service. + // +kubebuilder:validation:MinLength=1 + Namespace string `json:"namespace"` + // name is the name of the service. + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + // path is an optional URL path which will be sent in any request to this service. + // +optional + Path *string `json:"path,omitempty"` + // port is an optional service port. If unspecified, defaults to 443. + // +optional + Port *int32 `json:"port,omitempty"` +}
🧹 Nitpick comments (21)
kcp/deploy/examples/cowboy.yaml (1)
5-5: Add trailing newline at EOF.Tiny hygiene fix; avoids lint noise.
backend/http/handler.go (3)
118-125: Health endpoint: return a small body and content-type (and optionally support HEAD).Many probes/tools expect a body; also nice to advertise content-type.
Apply this diff:
func (h *handler) handleHealthz(w http.ResponseWriter, r *http.Request) { prepareNoCache(w) - w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("ok\n")) }Optionally also register HEAD:
- mux.HandleFunc("/healthz", h.handleHealthz).Methods("GET") + mux.HandleFunc("/healthz", h.handleHealthz).Methods("GET", "HEAD")
391-396: Scope filter: avoid panic on unexpected types; simplify condition.Guard the type assertion and only filter when not cluster-scoped.
Apply this diff:
- if !strings.EqualFold(h.scope.String(), scope.(string)) && h.scope != kubebindv1alpha2.ClusterScope { - continue - } + if s, ok := scope.(string); ok { + if h.scope != kubebindv1alpha2.ClusterScope && !strings.EqualFold(h.scope.String(), s) { + continue + } + } else if h.scope != kubebindv1alpha2.ClusterScope { + continue + }
571-574: Error message: this lists dynamic resources, not CRDs.Tighten wording to ease debugging.
Apply this diff:
- return nil, fmt.Errorf("failed to list crds: %w", err) + return nil, fmt.Errorf("failed to list dynamic resources: %w", err)kcp/deploy/examples/apiresourceschema-sheriffs.yaml (1)
48-48: Add trailing newline at EOF.Silences YAML lint warning.
kcp/deploy/examples/apiresourceschema-cowboys.yaml (1)
48-48: Add trailing newline at EOF.Minor lint fix.
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (3)
54-60: Validate exactly one storage=true version.Guard against zero or multiple storage versions to prevent invalid CRDs round‑tripping.
if len(crd.Spec.Versions) == 0 { return nil, fmt.Errorf("no versions specified for CRD %q", crd.Name) } +var storageCount int +for _, v := range crd.Spec.Versions { + if v.Storage { + storageCount++ + } +} +if storageCount != 1 { + return nil, fmt.Errorf("expected exactly one storage version for CRD %q, got %d", crd.Name, storageCount) +} if len(crd.Spec.Versions) > 1 && crd.Spec.Conversion == nil { return nil, fmt.Errorf("multiple versions specified for CRD %q but no conversion strategy", crd.Name) }
117-125: Make hash deterministic and spec‑only.Hashing entire objects (incl. metadata/status) and relying on input order causes unnecessary churn. Sort by a stable key and encode only Spec.
-func BoundSchemasSpecHash(schemas []*kubebindv1alpha2.BoundSchema) (string, error) { - hash := sha256.New() - for _, schema := range schemas { - if err := json.NewEncoder(hash).Encode(schema); err != nil { - return "", fmt.Errorf("failed to encode schema %s: %w", schema.Name, err) - } - } - return fmt.Sprintf("%x", hash.Sum(nil)), nil -} +func BoundSchemasSpecHash(schemas []*kubebindv1alpha2.BoundSchema) (string, error) { + // copy and sort by <plural>.<group> to avoid prefix/metadata noise + ss := make([]*kubebindv1alpha2.BoundSchema, len(schemas)) + copy(ss, schemas) + sort.Slice(ss, func(i, j int) bool { + li := ss[i].Spec.Names.Plural + "." + ss[i].Spec.Group + lj := ss[j].Spec.Names.Plural + "." + ss[j].Spec.Group + return li < lj + }) + h := sha256.New() + enc := json.NewEncoder(h) + for _, s := range ss { + if err := enc.Encode(s.Spec); err != nil { + return "", fmt.Errorf("failed to encode spec for %s: %w", s.Name, err) + } + } + return fmt.Sprintf("%x", h.Sum(nil)), nil +}Add import outside this hunk:
import "sort"
164-173: Avoid setting empty Subresources.Setting
&version.Subresourceseven when empty needlessly populates CRD. Only set when non‑zero.- AdditionalPrinterColumns: version.AdditionalPrinterColumns, - Subresources: &version.Subresources, + AdditionalPrinterColumns: version.AdditionalPrinterColumns, } +if version.Subresources.Scale != nil || version.Subresources.Status != nil { + crdVersion.Subresources = &version.Subresources +}kcp/deploy/examples/sheriff.yaml (1)
4-4: Add trailing newline.Satisfy linters and POSIX text-file convention.
kcp/deploy/examples/apiexport.yaml (1)
16-16: Add trailing newline.Minor formatting cleanup for linters.
backend/controllers/serviceexport/serviceexport_controller.go (1)
133-143: Use Patch for annotations to reduce conflicts.Replace full Update with a merge/strategic Patch to persist only metadata deltas.
backend/controllers/serviceexport/serviceexport_reconcile.go (1)
51-70: Stabilize hash input ordering.Sort schemas by
<plural>.<group>before hashing so adding resources in different orders yields the same hash.- schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources)) + schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources)) for _, res := range export.Spec.Resources { name := res.ResourceGroupName() schema, err := r.getBoundSchema(ctx, cache, export.Namespace, name) ... schemas = append(schemas, schema) } + sort.Slice(schemas, func(i, j int) bool { + li := schemas[i].Spec.Names.Plural + "." + schemas[i].Spec.Group + lj := schemas[j].Spec.Names.Plural + "." + schemas[j].Spec.Group + return li < lj + })Add
import "sort"if not present.pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (4)
82-95: Fix printf-style args in condition message to avoid "%!(EXTRA ...)" noise.You're passing TerminalMessage as the format string plus an extra arg, which renders awkwardly. Use an explicit format.
- conditions.MarkFalse( - binding, - kubebindv1alpha2.APIServiceBindingConditionConnected, - "APIServiceExportFailed", - conditionsapi.ConditionSeverityError, - request.Status.TerminalMessage, - binding.Name, - ) + conditions.MarkFalse( + binding, + kubebindv1alpha2.APIServiceBindingConditionConnected, + "APIServiceExportFailed", + conditionsapi.ConditionSeverityError, + "APIServiceExportRequest %s failed: %s", + binding.Name, + request.Status.TerminalMessage, + )
222-236: Handle create race: tolerate AlreadyExists.Between get and create, another actor may create the CRD. Treat AlreadyExists as non-fatal.
- if _, err := r.createCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return err + if _, err := r.createCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + if errors.IsAlreadyExists(err) { + return nil + } + return err }
289-291: Clarify error: it's a BoundSchema, not “Schema”.Improve message precision.
- return nil, fmt.Errorf("failed to get Schema %s: %w", + return nil, fmt.Errorf("failed to get BoundSchema %s: %w", res.ResourceGroupName(), err)
169-201: Rename for clarity: referenceBoundAPIResourceSchema → referenceBoundSchema.The function no longer deals with APIResourceSchema. Rename to match intent (update callers).
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (3)
188-195: Reason/message still reference APIResourceSchema; rename to BoundSchema.Keeps terminology consistent with the new model.
- "APIResourceSchemaNotFound", + "BoundSchemaNotFound", @@ - "APIResourceSchema %s in the service provider cluster not found", + "BoundSchema %s in the service provider cluster not found",
263-265: klog structured logging misuse.Avoid printf in message and supply key/value pairs.
- logger.Info("Deleting service binding request %s/%s", req.Namespace, req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time)) + logger.Info("Deleting service binding request", + "namespace", req.Namespace, "name", req.Name, + "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time))
166-168: Tolerate AlreadyExists on BoundSchema create.Races can happen; treat as success.
- if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { - return err - } + if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { + if apierrors.IsAlreadyExists(err) { + continue + } + return err + }sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
225-232: Condition comments don’t match names.Rename comments to reflect “Valid”/“Invalid”.
-// BoundSchemaReady indicates that the API resource schema is ready. -// It is set to true when the API resource schema is accepted and there are no drifts detected. +// BoundSchemaValid indicates the schema is accepted and no drift detected. @@ -// BoundSchemaDriftDetected indicates that there is a drift between the consumer's API and the expected API. -// It is set to true when the API resource schema is not accepted or there are drifts detected. +// BoundSchemaInvalid indicates the schema is rejected or drift detected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/controllers/clusterbinding/clusterbinding_reconcile.go(3 hunks)backend/controllers/serviceexport/serviceexport_controller.go(5 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(1 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(6 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(3 hunks)backend/http/handler.go(2 hunks)backend/kubernetes/manager.go(0 hunks)backend/options/options.go(2 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-cowboys.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-sheriffs.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-cluster.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-namespaced.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/examples/sheriff.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)pkg/konnector/controllers/cluster/cluster_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(7 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(5 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/kubernetes/manager.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cli/pkg/kubectl/bind/plugin/bind.go
- kcp/deploy/examples/apiserviceexport-cluster.yaml
- kcp/deploy/examples/apiserviceexport-namespaced.yaml
- pkg/konnector/controllers/cluster/cluster_controller.go
- pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go
- backend/options/options.go
- kcp/deploy/resources/apiexport-kube-bind.io.yaml
- backend/controllers/clusterbinding/clusterbinding_reconcile.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-12T08:55:41.816Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
sdk/apis/kubebind/v1alpha2/helpers/boundschema.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlsdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T09:05:29.743Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
sdk/apis/kubebind/v1alpha2/helpers/boundschema.godeploy/crd/kube-bind.io_boundschemas.yamlbackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:40:15.272Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
backend/controllers/serviceexportrequest/serviceexportrequest_controller.godeploy/crd/kube-bind.io_boundschemas.yamlbackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gobackend/controllers/serviceexport/serviceexport_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
🧬 Code graph analysis (8)
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (10)
BoundSchema(36-42)NamespacedScope(51-51)BoundSchemaSpec(60-70)InformerScope(47-47)APICRDSpec(72-104)CustomResourceConversion(108-119)ConversionStrategyType(168-168)WebhookConversion(122-135)WebhookClientConfig(138-165)APIResourceVersion(171-223)sdk/apis/kubebind/v1alpha2/register.go (1)
SchemeGroupVersion(39-39)
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)pkg/indexers/serviceexportrequest.go (1)
ServiceExportRequestByGroupResource(26-26)
backend/http/handler.go (1)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
ClusterScope(50-50)
backend/controllers/serviceexport/serviceexport_reconcile.go (5)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (3)
APIServiceExport(55-66)APIServiceExportConditionProviderInSync(36-36)SourceSpecHashAnnotationKey(27-27)sdk/apis/third_party/conditions/util/conditions/setter.go (2)
MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemasSpecHash(117-125)
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (6)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (3)
BoundSchema(36-42)ClusterScope(50-50)InformerScope(47-47)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (4)
APIServiceExport(55-66)SourceSpecHashAnnotationKey(27-27)APIServiceExportSpec(80-104)APIServiceExportResource(77-77)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (5)
APIServiceExportRequest(46-60)APIServiceExportRequestConditionExportsReady(31-31)APIServiceExportRequestPhaseFailed(154-154)APIServiceExportRequestPhasePending(151-151)GroupResource(127-143)sdk/apis/third_party/conditions/util/conditions/setter.go (4)
SetSummary(126-128)Set(41-78)MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/util/conditions/getter.go (1)
GetMessage(94-99)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)
UnstructuredToBoundSchema(109-115)BoundSchemasSpecHash(117-125)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
APIServiceExportRequest(46-60)APIServiceExportRequestPhaseFailed(154-154)sdk/apis/third_party/conditions/util/conditions/setter.go (1)
MarkFalse(120-122)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemaToCRD(127-196)
backend/controllers/serviceexport/serviceexport_controller.go (3)
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (2)
APIServiceExport(55-66)APIServiceExportList(147-152)pkg/indexers/serviceexport.go (2)
ServiceExportByBoundSchema(27-27)IndexServiceExportByBoundSchema(40-52)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (2)
ConditionType(48-48)Conditions(92-92)
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiexport.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-cowboys.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/cowboy.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/sheriff.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (8)
deploy/crd/kube-bind.io_boundschemas.yaml (1)
6-20: Rename and surface look good; CRD reads consistent with schemaless goal.Names, kinds, shortNames, and scopes align with the BoundSchema migration.
Please ensure
make generate(or equivalent) has been run so the client/informer/lister code matches these names; the known lister singular/plural generator quirk is tracked separately per your note.sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
109-115: LGTM: unstructured conversion.The conversion path is correct and minimal.
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (2)
13-19: Rename alignment looks good.Kinds/plurals/singulars now consistently use BoundSchema/boundschemas/boundschema.
190-198: Schema block: preserve‑unknown‑fields is appropriate.Spec enforces presence of versions[].schema with atomic+preserve; matches schemaless intent.
backend/controllers/serviceexport/serviceexport_reconcile.go (1)
72-85: Error propagation LGTM; condition handling is correct.Hash failures bubble up; missing schema marks ProviderInSync=false and exits early, relying on BoundSchema watch to requeue.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
180-185: Add RBAC for BoundSchema; keep APIResourceSchema only if unused
- Add these markers (use the same API group as the rest of the file, not kube-bind.io):
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch;create;update;patch;delete
//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas/status,verbs=get;update;patch- Because lister-gen currently emits a singular name ("boundschema") in some generated code, also add RBAC for the singular resource if you observe that in the generated client.
- Do NOT replace the group with "kube-bind.io" — keep "kubebind.k8s.io" to match the codebase.
- Remove the apiresourceschemas marker only after confirming this controller no longer reads/creates APIResourceSchema (search for APIResourceSchema/BoundAPIResourceSchema usages); otherwise keep it.
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
54-57: Drop trivial InformerScope.String if unused — verify firstDefined at sdk/apis/kubebind/v1alpha2/boundchema_types.go:54–57; remove to reduce surface only after confirming no references (search for InformerScope and InformerScope.String across the repo).
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
75-88: Upstream validation present — no change required. CompletedOptions.Validate() (backend/options/options.go) canonicalizes schemaSource via schemaSourceAliases and enforces strings.SplitN(options.SchemaSource, ".", 3) == 3; backend/http/handler.go has a similar defensive check.
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
from
fa981c5 to
57e3a32
Compare
Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt> On-behalf-of: @SAP mangirdas.judeikis@sap.com
mjudeikis
force-pushed
the
mjudeikis/shemaless.backend
branch
2 times, most recently
from
15544f0 to
b02f846
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (2)
108-115: Heartbeat drift check is inverted; it flags recent heartbeats as “drift”.time.Since(lastHeartbeat) < 10s is normal, not drift. To detect future timestamps, check for a negative duration less than -10s.
Apply:
- } else if ago < time.Second*10 { + } else if ago < -10*time.Second { conditions.MarkFalse(clusterBinding, kubebindv1alpha2.ClusterBindingConditionHealthy, "HeartbeatTimeDrift", conditionsapi.ConditionSeverityWarning, - "Clocks of consumer cluster and service account cluster seem to be off by more than 10s", + "Consumer clock appears ahead by more than 10s", )
208-213: Don’t recreate ClusterRoleBinding after deletion when scope != Cluster.The function deletes the binding and then continues to (re)create it. Early-return when not ClusterScope.
Apply:
if r.scope != kubebindv1alpha2.ClusterScope { if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) } + return nil }backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
273-276: Incorrectly flipping Succeeded requests to Failed after 1 minute. Remove.This unconditionally overwrites Phase to Failed right after success.
- if time.Since(req.CreationTimestamp.Time) > time.Minute { - req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed - req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) - }pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
250-253: Swallowed update error; should be returned to requeue.Returning nil on update failure loses the error and may stall reconciliation.
- if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err } else if errors.IsInvalid(err) {
♻️ Duplicate comments (6)
kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
40-43: Narrow verbs on apiresourceschemas to least privilege.Use read-only verbs unless writes are required.
Apply:
- - group: apis.kcp.io - resource: apiresourceschemas - verbs: - - '*' + - group: apis.kcp.io + resource: apiresourceschemas + verbs: + - get + - list + - watchbackend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
218-224: Optional: compare full Status on error, not just Phase.Prevents losing condition/message updates when Phase is unchanged.
- if !reflect.DeepEqual(original.Status.Phase, apiServiceExportRequest.Status.Phase) { + if !reflect.DeepEqual(original.Status, apiServiceExportRequest.Status) { if err := client.Status().Update(ctx, apiServiceExportRequest); err != nil {Consider a light retry-on-conflict loop.
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (1)
13-19: Rename + namespacing look good; ensure all controllers use namespaced lookups.Kind/plural/singular updates to BoundSchema and scope=Namespaced align with the PR goals. Verify all controllers/indexes use namespace in cache keys and queries to avoid collisions. This was raised earlier and still applies.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
169-170: Build break: slices.Collect used on []string.maps.Keys returns []string; slices.Collect expects an iterator.
Apply:
- keys := slices.Collect(maps.Keys(grSet)) + keys := maps.Keys(grSet)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
137-166: WebhookClientConfig parity: add Service and enforce url xor service.Without Service, in-cluster webhooks are blocked. Add ServiceReference and an XValidation to require exactly one of url or service.
-// WebhookClientConfig contains the information to make a TLS connection with the webhook. -type WebhookClientConfig struct { +// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// Exactly one of service or url must be specified. +// +kubebuilder:validation:XValidation:rule="(has(self.service) && !has(self.url)) || (!has(self.service) && has(self.url))",message="exactly one of service or url must be specified" +type WebhookClientConfig struct { + // service references a service within the cluster. Either service or url must be set. + // +optional + Service *ServiceReference `json:"service,omitempty"` @@ URL *string `json:"url,omitempty"` @@ CABundle []byte `json:"caBundle,omitempty"` } + +// ServiceReference holds a reference to a service for contacting a webhook. +// Mirrors admissionregistration.k8s.io/v1 ServiceReference (subset). +type ServiceReference struct { + // namespace is the namespace of the service. + // +kubebuilder:validation:MinLength=1 + Namespace string `json:"namespace"` + // name is the name of the service. + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + // path is an optional URL path which will be sent in any request to this service. + // +optional + Path *string `json:"path,omitempty"` + // port is an optional service port. If unspecified, defaults to 443. + // +optional + Port *int32 `json:"port,omitempty"` +}sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
75-83: Round-trip loss of webhook Service config — add ServiceReference to the API and map it both ways.Only URL/CABundle are mapped; WebhookClientConfig currently has no Service field (sdk/apis/kubebind/v1alpha2/boundchema_types.go:138–165). Update the API to add ServiceReference and then update mappings in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (lines 75–83 and 157–163) as shown below — apply after the API type is extended:
@@ - if crd.Spec.Conversion.Webhook.ClientConfig != nil { - crConversion.Webhook.ClientConfig = &kubebindv1alpha2.WebhookClientConfig{ - URL: crd.Spec.Conversion.Webhook.ClientConfig.URL, - CABundle: crd.Spec.Conversion.Webhook.ClientConfig.CABundle, - } - } + if cc := crd.Spec.Conversion.Webhook.ClientConfig; cc != nil { + crConversion.Webhook.ClientConfig = &kubebindv1alpha2.WebhookClientConfig{ + URL: cc.URL, + CABundle: cc.CABundle, + } + if cc.Service != nil { + crConversion.Webhook.ClientConfig.Service = &kubebindv1alpha2.ServiceReference{ + Namespace: cc.Service.Namespace, + Name: cc.Service.Name, + Path: cc.Service.Path, + Port: cc.Service.Port, + } + } + } @@ - if schema.Spec.Conversion.Webhook.ClientConfig != nil { - crConversion.Webhook.ClientConfig = &apiextensionsv1.WebhookClientConfig{ - URL: schema.Spec.Conversion.Webhook.ClientConfig.URL, - CABundle: schema.Spec.Conversion.Webhook.ClientConfig.CABundle, - } - } + if cc := schema.Spec.Conversion.Webhook.ClientConfig; cc != nil { + crConversion.Webhook.ClientConfig = &apiextensionsv1.WebhookClientConfig{ + URL: cc.URL, + CABundle: cc.CABundle, + } + if cc.Service != nil { + crConversion.Webhook.ClientConfig.Service = &apiextensionsv1.ServiceReference{ + Namespace: cc.Service.Namespace, + Name: cc.Service.Name, + Path: cc.Service.Path, + Port: cc.Service.Port, + } + } + }
🧹 Nitpick comments (23)
backend/options/options.go (6)
115-117: Use typed consts for schema sources (immutable, clearer API).These are fixed identifiers; make them consts instead of vars.
-var ( - KCPAPIResourceSchemaSource = SchemaSource("APIResourceSchema.v1alpha1.apis.kcp.io") - CustomResourceDefinitionSource = SchemaSource("CustomResourceDefinition.v1.apiextensions.k8s.io") -) +const ( + KCPAPIResourceSchemaSource SchemaSource = "APIResourceSchema.v1alpha1.apis.kcp.io" + CustomResourceDefinitionSource SchemaSource = "CustomResourceDefinition.v1.apiextensions.k8s.io" +)
119-121: Nit: polish TODO phrasing.Minor grammar.
-// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 -// We should relax these once we happy they work with any schema. +// TODO(mjudeikis): https://github.com/kube-bind/kube-bind/issues/298 +// We should relax these once we are happy they work with any schema.
122-126: Alias map: keep canonical self-keys; add backward‑compat singular aliases; fix typo.Adds UX resilience without changing behavior.
- CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests - "customresourcedefinitions": CustomResourceDefinitionSource.String(), - "apiresourceschemas": KCPAPIResourceSchemaSource.String(), + CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostly for e2e tests + "customresourcedefinitions": CustomResourceDefinitionSource.String(), + "customresourcedefinition": CustomResourceDefinitionSource.String(), // backward-compat alias + "apiresourceschemas": KCPAPIResourceSchemaSource.String(), + "apiresourceschema": KCPAPIResourceSchemaSource.String(), // backward-compat alias KCPAPIResourceSchemaSource.String(): KCPAPIResourceSchemaSource.String(),
245-249: Accept case-insensitive alias inputs (without breaking canonical exact matches).Fallback lookup improves CLI ergonomics while preserving canonical exact keys.
- schemaSource := schemaSourceAliases[options.SchemaSource] + schemaSource := schemaSourceAliases[options.SchemaSource] + if schemaSource == "" { + schemaSource = schemaSourceAliases[strings.ToLower(options.SchemaSource)] + }
147-155: Deduplicate/sort help values; show stable, clean list.Current code may list duplicates due to multiple aliases pointing to the same canonical value.
- values := make([]string, 0, len(schemaSourceAliases)) - for _, v := range schemaSourceAliases { - values = append(values, v) - } + canonicalVals := sets.New[string]() + for _, v := range schemaSourceAliases { + canonicalVals.Insert(v) + } + values := sets.List(canonicalVals) fs.StringVar(&options.SchemaSource, "schema-source", options.SchemaSource, - fmt.Sprintf("Defines the source of the schema in Kind.Version.Group format for the bind screen. Defaults to CustomResourceDefinition.v1.apiextensions.k8s.io. Possible values are: %v", - values), + fmt.Sprintf("Defines the source of the schema in Kind.Version.Group format for the bind screen. Defaults to %s. Possible values (aliases accepted): %v", + CustomResourceDefinitionSource.String(), values), )
159-160: Nit: fix user-facing typo.“th” → “the”.
- fs.StringVar(&options.TestingAutoSelect, "testing-auto-select", options.TestingAutoSelect, "<resource>.<group> that is automatically selected on th bind screen for testing") + fs.StringVar(&options.TestingAutoSelect, "testing-auto-select", options.TestingAutoSelect, "<resource>.<group> that is automatically selected on the bind screen for testing")kcp/deploy/examples/apiexport.yaml (1)
16-16: Add newline at EOF.Fix lint error by adding a trailing newline.
kcp/deploy/examples/apiresourceschema-sheriffs.yaml (1)
48-48: Add newline at EOF.Resolve YAML lint warning.
kcp/deploy/examples/sheriff.yaml (1)
4-4: Add newline at EOF.Fix lint.
kcp/deploy/examples/cowboy.yaml (1)
5-5: Add newline at EOF.Fix lint.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (5)
187-197: Fix error message: says “create” on update path.Minor copy/paste bug.
Apply:
- if err := r.updateClusterRole(ctx, client, role); err != nil { - return fmt.Errorf("failed to create ClusterRole %s: %w", role.Name, err) + if err := r.updateClusterRole(ctx, client, role); err != nil { + return fmt.Errorf("failed to update ClusterRole %s: %w", role.Name, err) }
254-256: Wrong identifier and verb in error message.ClusterRoleBinding is cluster-scoped; expected.Namespace is empty. Also message says “create” on update.
Apply:
- if err := r.updateClusterRoleBinding(ctx, client, binding); err != nil { - return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Namespace, err) - } + if err := r.updateClusterRoleBinding(ctx, client, binding); err != nil { + return fmt.Errorf("failed to update ClusterRoleBinding %s: %w", binding.Name, err) + }
263-266: Use the same constant for the RoleBinding name.Avoid drift by using kuberesources.ServiceAccountName in the getter too.
Apply:
- binding, err := r.getRoleBinding(ctx, cache, clusterBinding.Namespace, "kube-binder") + binding, err := r.getRoleBinding(ctx, cache, clusterBinding.Namespace, kuberesources.ServiceAccountName)
295-297: Fix error message: wrong identifier and verb.Use the binding’s name and “update”.
Apply:
- if err := r.updateRoleBinding(ctx, client, binding); err != nil { - return fmt.Errorf("failed to create RoleBinding %s: %w", expected.Namespace, err) - } + if err := r.updateRoleBinding(ctx, client, binding); err != nil { + return fmt.Errorf("failed to update RoleBinding %s: %w", binding.Name, err) + }
162-185: Optional: de-duplicate RBAC rules across all exports, not just per-export.If multiple exports list the same resource, rules may duplicate. Consider collecting a union across all exports before appending.
backend/controllers/serviceexport/serviceexport_reconcile.go (1)
53-70: Stabilize hash ordering.Hash depends on the order of export.Spec.Resources. To make it order-insensitive, sort schemas by name before hashing.
Apply:
@@ - for _, res := range export.Spec.Resources { + for _, res := range export.Spec.Resources { name := res.ResourceGroupName() schema, err := r.getBoundSchema(ctx, cache, export.Namespace, name) @@ schemas = append(schemas, schema) } + sort.Slice(schemas, func(i, j int) bool { return schemas[i].Name < schemas[j].Name })Add import:
import ( "context" + "sort"backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (3)
181-183: Handle AlreadyExists on BoundSchema create (race-safe idempotency).A concurrent creator can win. Treat AlreadyExists as success.
- if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { - return err - } + if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { + if apierrors.IsAlreadyExists(err) { + continue + } + return err + }
279-282: Fix klog usage: Info doesn’t support printf formatting.Use structured fields.
- logger.Info("Deleting service binding request %s/%s", req.Namespace, req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time)) + logger.Info("Deleting service export request", "namespace", req.Namespace, "name", req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time))
75-107: Avoid N× full-listing of schemaSource per resource. List once and index.You list all exported CRDs for every requested resource. List once, build a GR->item map, then loop req.Spec.Resources.
Also applies to: 98-105
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
121-129: Hash only Spec to avoid noisy re‑exports on metadata/status drift.Current hash includes metadata/status; use Spec only.
- for _, schema := range schemas { - if err := json.NewEncoder(hash).Encode(schema); err != nil { + for _, schema := range schemas { + if err := json.NewEncoder(hash).Encode(schema.Spec); err != nil { return "", fmt.Errorf("failed to encode schema %s: %w", schema.Name, err) } }pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (2)
84-95: Condition message formatting: avoid stray fmt args.TerminalMessage is used as format string but an extra arg is passed; drop it to prevent accidental formatting.
- conditions.MarkFalse( + conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionConnected, "APIServiceExportFailed", conditionsapi.ConditionSeverityError, - request.Status.TerminalMessage, - binding.Name, + request.Status.TerminalMessage, )
289-291: Clarify error wording (“BoundSchema”).Message says “Schema”; be explicit.
- return nil, fmt.Errorf("failed to get Schema %s: %w", + return nil, fmt.Errorf("failed to get BoundSchema %s: %w", res.ResourceGroupName(), err)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
99-104: Optional: validate exactly one storage version in Versions.Add XValidation to ensure one and only one version has storage=true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/controllers/clusterbinding/clusterbinding_reconcile.go(3 hunks)backend/controllers/serviceexport/serviceexport_controller.go(5 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(1 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(6 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(2 hunks)backend/http/handler.go(2 hunks)backend/kubernetes/manager.go(0 hunks)backend/options/options.go(2 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-cowboys.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-sheriffs.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-cluster.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-namespaced.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/examples/sheriff.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)pkg/konnector/controllers/cluster/cluster_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(7 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(5 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/kubernetes/manager.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cli/pkg/kubectl/bind/plugin/bind.go
- kcp/deploy/examples/apiserviceexport-cluster.yaml
- kcp/deploy/examples/apiserviceexport-namespaced.yaml
- pkg/konnector/controllers/cluster/cluster_controller.go
- pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go
- backend/http/handler.go
- backend/controllers/serviceexport/serviceexport_controller.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
📚 Learning: 2025-09-12T09:05:29.743Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gokcp/deploy/resources/apiexport-kube-bind.io.yamldeploy/crd/kube-bind.io_boundschemas.yamlpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.go
📚 Learning: 2025-09-12T08:40:15.272Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gokcp/deploy/resources/apiexport-kube-bind.io.yamldeploy/crd/kube-bind.io_boundschemas.yamlpkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:55:41.816Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.go
🧬 Code graph analysis (7)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
APIServiceExport(55-66)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/register.go (2)
GroupName(32-32)Resource(42-44)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
GroupResource(127-143)
backend/controllers/serviceexport/serviceexport_reconcile.go (5)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (3)
APIServiceExport(55-66)APIServiceExportConditionProviderInSync(36-36)SourceSpecHashAnnotationKey(27-27)sdk/apis/third_party/conditions/util/conditions/setter.go (2)
MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemasSpecHash(121-129)
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (6)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (3)
BoundSchema(36-42)ClusterScope(50-50)InformerScope(47-47)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (4)
APIServiceExport(55-66)SourceSpecHashAnnotationKey(27-27)APIServiceExportSpec(80-104)APIServiceExportResource(77-77)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (5)
APIServiceExportRequest(46-60)APIServiceExportRequestConditionExportsReady(31-31)APIServiceExportRequestPhaseFailed(154-154)APIServiceExportRequestPhasePending(151-151)GroupResource(127-143)sdk/apis/third_party/conditions/util/conditions/setter.go (4)
SetSummary(126-128)Set(41-78)MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/util/conditions/getter.go (1)
GetMessage(94-99)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)
UnstructuredToBoundSchema(113-119)BoundSchemasSpecHash(121-129)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (6)
sdk/apis/kubebind/v1alpha1/apiservicebinding_types.go (2)
APIServiceBinding(63-73)APIServiceBindingConditionConnected(37-37)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
APIServiceExportRequest(46-60)APIServiceExportRequestPhaseFailed(154-154)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/third_party/conditions/util/conditions/setter.go (1)
MarkFalse(120-122)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemaToCRD(131-200)
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)pkg/indexers/serviceexportrequest.go (1)
ServiceExportRequestByGroupResource(26-26)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (2)
ConditionType(48-48)Conditions(92-92)
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (9)
BoundSchema(36-42)InformerScope(47-47)BoundSchemaSpec(60-70)APICRDSpec(72-104)CustomResourceConversion(108-119)ConversionStrategyType(168-168)WebhookConversion(122-135)WebhookClientConfig(138-165)APIResourceVersion(171-223)
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/cowboy.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiexport.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-cowboys.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/sheriff.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
⏰ 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). (5)
- GitHub Check: lint
- GitHub Check: verify
- GitHub Check: go-test
- GitHub Check: go-test-e2e
- GitHub Check: go-test-e2e
🔇 Additional comments (18)
backend/options/options.go (1)
59-60: Doc update matches current reality (v1alpha1).Good to explicitly document the canonical KCP value used today.
kcp/deploy/examples/apiresourceschema-sheriffs.yaml (2)
14-14: Cluster scope intent check.Scope is Cluster; the example CR (sheriff.yaml) omits namespace accordingly. Confirm this is intended for your scoping/isolation story.
1-2: Confirmed: APIResourceSchema v1alpha1 is compatible with APIExport v1alpha2.
APIExport is available as apis.kcp.io/v1alpha2 (v1alpha1 still present) and APIResourceSchema remains apis.kcp.io/v1alpha1 — the mix is supported; no change required.kcp/deploy/examples/sheriff.yaml (1)
1-4: LGTM — cluster-scoped example aligns with schema.Matches cluster scope (no namespace).
kcp/deploy/examples/cowboy.yaml (1)
1-5: LGTM — minimal namespaced example.Clear and consistent with typical CR patterns.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (4)
85-94: LGTM — cache-backed BoundSchema getter.Typed get via NamespacedName is correct.
152-161: LGTM — mapper key matches indexer format.Using
<plural>.<group>aligns with ServiceExportRequestByGroupResource.
250-252: LGTM — watch BoundSchema; ensure RBAC aligns.Watcher added; confirm RBAC for boundschemas is updated as above.
64-65: Constructor gained schemaSource — call sites updated.
Verified: the only constructor invocation found (backend/server.go) passes c.Options.SchemaSource.kcp/deploy/examples/apiexport.yaml (1)
7-16: Resolved — APIResourceSchemas present and match group/plural.
today.cowboys.wildwest.dev and today.sheriffs.wildwest.dev exist in kcp/deploy/examples; both have spec.group: wildwest.dev and names.plural: cowboys / sheriffs respectively.kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (2)
27-27: Description-only change acknowledged.The version description was updated to “BoundSchema”. No functional impact.
47-48: Spec/Status docstrings updated.Descriptions now reference BoundSchemaSpec/BoundSchemaStatus. Looks consistent with the rename.
Also applies to: 350-351
deploy/crd/kube-bind.io_boundschemas.yaml (3)
6-20: Public naming aligned with BoundSchema.CRD name and resource names (kind/plural/singular/shortNames) look correct for BoundSchema.
352-353: Status description rename LGTM.BoundSchemaStatus wording matches the type.
468-468: Add a trailing newline.YAML linter flagged missing newline at EOF.
[raise_nitpick_refactor]
Apply:- status: {} + status: {} +kcp/deploy/examples/apiresourceschema-cowboys.yaml (1)
1-48: Add a trailing newline.Fix the “no newline at end of file” lint error.
[raise_nitpick_refactor]
Apply:- status: {} + status: {} +backend/controllers/serviceexport/serviceexport_reconcile.go (2)
58-65: Condition message is good; keep it concise.Using the looked-up name avoids nil deref. Severity=Error is acceptable.
49-90: Persist metadata changes when hash mismatches — verified persisted by controller.ensureSchema mutates export.Annotations and returns nil; the outer Reconcile compares original.Annotations to apiServiceExport.Annotations and calls client.Update(ctx, apiServiceExport) when different (backend/controllers/serviceexport/serviceexport_controller.go:136–141), so the annotation/hash will be persisted and the controller will requeue.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
208-213: ClusterRoleBinding recreated after deletion; return early.When scope != ClusterScope you delete the binding but continue to recreate it.
Apply:
- if r.scope != kubebindv1alpha2.ClusterScope { - if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { - return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) - } - } + if r.scope != kubebindv1alpha2.ClusterScope { + if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) + } + return nil + }backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
180-185: RBAC marker still targets APIResourceSchemas; add BoundSchemasController watches BoundSchema; ensure generated RBAC grants list/watch (and create if needed). Replace apiresourceschemas with boundschemas and correct group.
-//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiresourceschemas,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=kube-bind.io,resources=boundschemas,verbs=get;list;watch;create;update;patch;deletepkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
205-266: Bug: updateCRD errors are swallowed; return the error.Non-Invalid errors should bubble up; current code returns nil on failure.
- if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err } else if errors.IsInvalid(err) { @@ - "CustomResourceDefinition %s cannot be updated: %s", - binding.Name, err, + "CustomResourceDefinition %s cannot be updated: %s", + crd.Name, err,backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
279-282: Fix klog structured logging: don’t use printf-style formatting in message.
Current call treats “%s” literally; use key/value pairs instead.- logger.Info("Deleting service binding request %s/%s", req.Namespace, req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time)) + logger.Info("Deleting service binding request", "namespace", req.Namespace, "name", req.Name, "reason", "timeout", "age", time.Since(req.CreationTimestamp.Time))
♻️ Duplicate comments (8)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
169-171: Build break: slices.Collect used on []string.maps.Keys already returns []string; slices.Collect expects an iterator.
Apply:
- keys := slices.Collect(maps.Keys(grSet)) - slices.Sort(keys) + keys := maps.Keys(grSet) + slices.Sort(keys)kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
40-43: Wildcard verbs on apiresourceschemasEarlier feedback suggested narrowing to read-only verbs; acknowledged decision to keep broad for now.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
218-224: On-error status update compares only PhasePreviously discussed; leaving as-is per maintainer’s note.
backend/controllers/serviceexport/serviceexport_controller.go (1)
95-97: RBAC group mismatch: reconcile kubebind.k8s.io vs kube-bind.io.Types register kube-bind.io; RBAC annotations use kubebind.k8s.io. Pick one canonical group and make them consistent across controllers and CRDs.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go (1)
57-58: Wire APIServiceExportRequest informer events.You accept serviceExportRequestInformer but never enqueue on its add/update/delete. SER phase transitions won’t reconcile bindings.
@@ func NewController( - serviceExportInformer bindinformers.APIServiceExportInformer, - serviceExportRequestInformer bindinformers.APIServiceExportRequestInformer, + serviceExportInformer bindinformers.APIServiceExportInformer, + serviceExportRequestInformer bindinformers.APIServiceExportRequestInformer, @@ if _, err := serviceExportInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ }); err != nil { return nil, err } + + if _, err := serviceExportRequestInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: func(obj any) { c.enqueueServiceExport(logger, obj) }, + UpdateFunc: func(_, newObj any) { c.enqueueServiceExport(logger, newObj) }, + DeleteFunc: func(obj any) { c.enqueueServiceExport(logger, obj) }, + }); err != nil { + return nil, err + }backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (2)
98-105: Hardcoded exported-CRD label should be configurable.
Keeping the label key in code makes future migrations harder; consider wiring this via config or a const in a single central package.
194-219: Cluster-scoped isolation computed from only the last schema; fix to “any cluster-scoped”.
Overwriting scope in the loop misses earlier cluster-scoped resources. Track a boolean and set isolation if any BoundSchema is cluster-scoped.- var schemas []*kubebindv1alpha2.BoundSchema - var scope apiextensionsv1.ResourceScope + var schemas []*kubebindv1alpha2.BoundSchema + hasClusterScoped := false @@ - // Collect all schemas for hashing. - // TODO(mjudeikis) Scope is same for all crds so we keep stamping it over. We might want to change this - scope = boundSchema.Spec.Scope + // Collect all schemas for hashing and detect cluster-scoped resources. + if boundSchema.Spec.Scope == apiextensionsv1.ClusterScoped { + hasClusterScoped = true + } schemas = append(schemas, boundSchema) @@ - if scope == apiextensionsv1.ClusterScoped { - export.Spec.ClusterScopedIsolation = r.clusterScopedIsolation - } + if hasClusterScoped { + export.Spec.ClusterScopedIsolation = r.clusterScopedIsolation + }Also applies to: 248-250
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
137-166: Parity gap: WebhookClientConfig lacks Service; enforce exactly-one-of service/url.
Kubernetes supports in-cluster service refs; add ServiceReference and an XValidation rule for exclusivity.-// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// WebhookClientConfig contains the information to make a TLS connection with the webhook. +// Exactly one of service or url must be specified. +// +kubebuilder:validation:XValidation:rule="(has(self.service) && !has(self.url)) || (!has(self.service) && has(self.url))",message="exactly one of service or url must be specified" type WebhookClientConfig struct { + // service references a service within the cluster. Either service or url must be set. + // +optional + Service *ServiceReference `json:"service,omitempty"` // url gives the location of the webhook, in standard URL form @@ // +optional CABundle []byte `json:"caBundle,omitempty"` } + +// ServiceReference holds a reference to a service for contacting a webhook. +// Mirrors admissionregistration.k8s.io/v1 ServiceReference (subset). +type ServiceReference struct { + // namespace is the namespace of the service. + // +kubebuilder:validation:MinLength=1 + Namespace string `json:"namespace"` + // name is the name of the service. + // +kubebuilder:validation:MinLength=1 + Name string `json:"name"` + // path is an optional URL path which will be sent in any request to this service. + // +optional + Path *string `json:"path,omitempty"` + // port is an optional service port. If unspecified, defaults to 443. + // +optional + Port *int32 `json:"port,omitempty"` +}
🧹 Nitpick comments (22)
kcp/deploy/examples/cowboy.yaml (1)
5-5: Add a trailing newline.Fix YAML lint warning by ending the file with a newline.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (3)
136-141: Stabilize rule ordering: sort exports by name.Ordering currently depends on list order; reflect.DeepEqual may churn updates.
Apply (and add import "sort"):
import ( "context" "fmt" "maps" "reflect" "slices" + "sort" "time" )exports, err := r.listServiceExports(ctx, cache, clusterBinding.Namespace) if err != nil { return fmt.Errorf("failed to list APIServiceExports: %w", err) } + sort.Slice(exports, func(i, j int) bool { return exports[i].Name < exports[j].Name })
194-196: Fix error message: it's an update, not create.Accurate message aids debugging.
- if err := r.updateClusterRole(ctx, client, role); err != nil { - return fmt.Errorf("failed to create ClusterRole %s: %w", role.Name, err) - } + if err := r.updateClusterRole(ctx, client, role); err != nil { + return fmt.Errorf("failed to update ClusterRole %s: %w", role.Name, err) + }
254-256: Fix error message and wrong placeholder.Use update and the binding name.
- if err := r.updateClusterRoleBinding(ctx, client, binding); err != nil { - return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Namespace, err) - } + if err := r.updateClusterRoleBinding(ctx, client, binding); err != nil { + return fmt.Errorf("failed to update ClusterRoleBinding %s: %w", binding.Name, err) + }sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
121-129: Hash only Spec to avoid churn from metadata/status.Including ObjectMeta/Status can cause spurious hash changes.
- for _, schema := range schemas { - if err := json.NewEncoder(hash).Encode(schema); err != nil { + for _, schema := range schemas { + if err := json.NewEncoder(hash).Encode(schema.Spec); err != nil { return "", fmt.Errorf("failed to encode schema %s: %w", schema.Name, err) } }kcp/deploy/examples/sheriff.yaml (1)
4-4: Add a trailing newline.Fix YAML lint warning by ending the file with a newline.
kcp/deploy/examples/apiexport.yaml (1)
16-16: Add a trailing newline.Fix YAML lint warning by ending the file with a newline.
pkg/konnector/controllers/cluster/cluster_controller.go (1)
212-239: Informer sync timeout check likely wrong contextThe timeout branch selects on ctx.Done() instead of waitCtx.Done(), so the “InformerSyncTimeout” path won’t trigger on deadline expiry. Switch to waitCtx.Done().
Apply:
- select { - case <-ctx.Done(): + select { + case <-waitCtx.Done(): // timeout logger.Info("informers did not sync in time", "timeout", heartbeatInterval/2) c.updateServiceBindings(ctx, func(binding *kubebindv1alpha2.APIServiceBinding) { conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionInformersSynced, "InformerSyncTimeout", conditionsapi.ConditionSeverityError, "Informers did not sync within %s", heartbeatInterval/2, ) }) return false, nil default: return true, nil }backend/options/options.go (2)
119-126: Tiny doc/comment polish in aliasesFix small typo and keep comments crisp.
- CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests + CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostly for e2e tests
147-155: Flag help lists values, not accepted inputs; prefer alias keys for UXThe help currently prints the map values (canonical forms). Show the accepted inputs (keys) instead.
- values := make([]string, 0, len(schemaSourceAliases)) - for _, v := range schemaSourceAliases { - values = append(values, v) - } + keys := sets.List(sets.Set[string](sets.StringKeySet(schemaSourceAliases))) fs.StringVar(&options.SchemaSource, "schema-source", options.SchemaSource, - fmt.Sprintf("Defines the source of the schema in Kind.Version.Group format for the bind screen. Defaults to CustomResourceDefinition.v1.apiextensions.k8s.io. Possible values are: %v", - values), + fmt.Sprintf("Defines the source of the schema in Kind.Version.Group format for the bind screen. Defaults to CustomResourceDefinition.v1.apiextensions.k8s.io. Possible values are: %v", + keys), )kcp/deploy/examples/apiresourceschema-sheriffs.yaml (1)
48-48: Add trailing newlineSatisfy linters and POSIX tooling by ending the file with a newline.
backend/controllers/serviceexport/serviceexport_controller.go (2)
133-142: Persisting annotation changes is good; add conflict retry.Wrap client.Update in a RetryOnConflict to avoid transient failures on concurrent writes.
- if err := client.Update(ctx, apiServiceExport); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport: %w", err) - } + if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + // refetch to get latest RV + cur := &kubebindv1alpha2.APIServiceExport{} + if err := client.Get(ctx, req.NamespacedName, cur); err != nil { + return err + } + cur.Annotations = apiServiceExport.Annotations + return client.Update(ctx, cur) + }); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport (conflict-retried): %w", err) + }Note: add import
k8s.io/client-go/util/retry.
154-177: Mapper key is correct; consider logging list errors.If c.List fails, log at V(1) to aid triage; returning empty silently can hide indexing misconfig.
- if err := c.List(ctx, &exports, client.MatchingFields{indexers.ServiceExportByBoundSchema: boundSchemaKey}); err != nil { - return []mcreconcile.Request{} - } + if err := c.List(ctx, &exports, client.MatchingFields{indexers.ServiceExportByBoundSchema: boundSchemaKey}); err != nil { + log.FromContext(ctx).V(1).Error(err, "list APIServiceExports by BoundSchema failed", "key", boundSchemaKey) + return nil + }kcp/deploy/examples/apiresourceschema-cowboys.yaml (1)
1-48: Add trailing newline.Fix YAMLlint error: ensure file ends with a newline.
@@ - status: {} \ No newline at end of file + status: {} +pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go (1)
83-126: Consider watching BoundSchema to refresh CRDs on spec changes.Without a BoundSchema watch, schema updates propagate only via ServiceExport/CRD events, leaving CRDs stale.
I can draft the informer/mapper wiring if you add a BoundSchemaInformer parameter.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
209-216: Avoid duplicating OwnerReferences.Append only if not already owned by the binding.
- crd.OwnerReferences = append(crd.OwnerReferences, newReference) + owned := false + for _, ref := range crd.OwnerReferences { + if ref.UID == binding.UID && ref.Kind == "APIServiceBinding" && ref.Name == binding.Name { + owned = true + break + } + } + if !owned { + crd.OwnerReferences = append(crd.OwnerReferences, newReference) + }backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (5)
78-87: Hoist schemaSource parsing out of the loop to avoid repeated work and reduce risk.
Parse once per call (or at reconciler construction) and reuse the computed GVK.- for _, res := range req.Spec.Resources { - parts := strings.SplitN(r.schemaSource, ".", 3) - if len(parts) != 3 { // We check this in validation, but just in case. - return fmt.Errorf("malformed schema source: %q", r.schemaSource) - } - - gvk := schema.GroupVersionKind{ - Kind: parts[0], - Version: parts[1], - Group: parts[2], - } + parts := strings.SplitN(r.schemaSource, ".", 3) + if len(parts) != 3 { // validated elsewhere; keep a defensive check. + return fmt.Errorf("malformed schema source: %q", r.schemaSource) + } + gvk := schema.GroupVersionKind{Kind: parts[0], Version: parts[1], Group: parts[2]} + for _, res := range req.Spec.Resources {
130-141: Set TerminalMessage on invalid schema to surface the reason to users.
Currently Phase is set to Failed but TerminalMessage is empty in this path.conditions.MarkFalse( req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady, "APIServiceExportRequestInvalid", conditionsapi.ConditionSeverityError, "APIServiceExportRequest %s is invalid: resource %s/%s has invalid schema", req.Name, group, plural, ) req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed - return fmt.Errorf("resource %s/%s is invalid", group, plural) + req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) + return fmt.Errorf("resource %s/%s is invalid", group, plural)
171-183: Make BoundSchema creation idempotent under races.
Handle AlreadyExists from create to avoid failing reconciliation during concurrent creates.- if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { - return err - } + if err := r.createBoundSchema(ctx, cl, boundSchema); err != nil { + if apierrors.IsAlreadyExists(err) { + continue + } + return err + }
221-229: Mark Succeeded when the export already exists.
Avoid leaving the request in Pending when we early-return due to an existing export.- } else { - // already exists; nothing to do - conditions.MarkTrue(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) - return nil - } + } else { + // already exists; mark ready and succeed + conditions.MarkTrue(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) + req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseSucceeded + return nil + }
262-268: Also mark Succeeded on AlreadyExists race when creating the export.
This mirrors the early-exit path and avoids a stuck Pending state.- if err := r.createServiceExport(ctx, cl, export); err != nil { - if apierrors.IsAlreadyExists(err) { - return nil - } - return err - } + if err := r.createServiceExport(ctx, cl, export); err != nil { + if apierrors.IsAlreadyExists(err) { + conditions.MarkTrue(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady) + req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseSucceeded + return nil + } + return err + }sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
225-232: Fix condition docstrings to match identifiers and semantics.
Docs currently refer to “Ready” while the constants are Valid/Invalid.- // BoundSchemaReady indicates that the API resource schema is ready. - // It is set to true when the API resource schema is accepted and there are no drifts detected. + // BoundSchemaValid indicates that the API resource schema is valid. + // It should be True when the schema is accepted and no drift is detected. BoundSchemaValid conditionsapi.ConditionType = "Valid" - // BoundSchemaDriftDetected indicates that there is a drift between the consumer's API and the expected API. - // It is set to true when the API resource schema is not accepted or there are drifts detected. + // BoundSchemaInvalid indicates that the API resource schema is invalid. + // It should be True when the schema is rejected or drift is detected. BoundSchemaInvalid conditionsapi.ConditionType = "Invalid"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/controllers/clusterbinding/clusterbinding_reconcile.go(3 hunks)backend/controllers/serviceexport/serviceexport_controller.go(5 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(1 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(6 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(2 hunks)backend/http/handler.go(2 hunks)backend/kubernetes/manager.go(0 hunks)backend/options/options.go(2 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-cowboys.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-sheriffs.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-cluster.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-namespaced.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/examples/sheriff.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)pkg/konnector/controllers/cluster/cluster_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(7 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(5 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/kubernetes/manager.go
🚧 Files skipped from review as they are similar to previous changes (5)
- kcp/deploy/examples/apiserviceexport-cluster.yaml
- cli/pkg/kubectl/bind/plugin/bind.go
- kcp/deploy/examples/apiserviceexport-namespaced.yaml
- deploy/crd/kube-bind.io_boundschemas.yaml
- backend/http/handler.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
📚 Learning: 2025-09-12T09:05:29.743Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.gobackend/controllers/clusterbinding/clusterbinding_reconcile.gopkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexport/serviceexport_controller.gokcp/deploy/resources/apiexport-kube-bind.io.yamlsdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:40:15.272Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.gokcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gobackend/controllers/clusterbinding/clusterbinding_reconcile.gopkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexport/serviceexport_controller.gokcp/deploy/resources/apiexport-kube-bind.io.yamlsdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:55:41.816Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yamlbackend/controllers/serviceexportrequest/serviceexportrequest_reconcile.gosdk/apis/kubebind/v1alpha2/helpers/boundschema.gobackend/controllers/clusterbinding/clusterbinding_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexport/serviceexport_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
🧬 Code graph analysis (9)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go (3)
sdk/apis/kubebind/v1alpha1/apiservicebinding_types.go (1)
APIServiceBinding(63-73)sdk/apis/kubebind/v1alpha1/clusterbinding_types.go (1)
ClusterBinding(50-61)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (6)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (3)
BoundSchema(36-42)ClusterScope(50-50)InformerScope(47-47)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (4)
APIServiceExport(55-66)SourceSpecHashAnnotationKey(27-27)APIServiceExportSpec(80-104)APIServiceExportResource(77-77)sdk/apis/third_party/conditions/util/conditions/setter.go (4)
SetSummary(126-128)Set(41-78)MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/third_party/conditions/util/conditions/getter.go (1)
GetMessage(94-99)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (2)
UnstructuredToBoundSchema(113-119)BoundSchemasSpecHash(121-129)
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (9)
BoundSchema(36-42)InformerScope(47-47)BoundSchemaSpec(60-70)APICRDSpec(72-104)CustomResourceConversion(108-119)ConversionStrategyType(168-168)WebhookConversion(122-135)WebhookClientConfig(138-165)APIResourceVersion(171-223)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (1)
APIServiceExport(55-66)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/register.go (2)
GroupName(32-32)Resource(42-44)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
GroupResource(127-143)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (5)
sdk/apis/kubebind/v1alpha1/apiservicebinding_types.go (2)
APIServiceBinding(63-73)APIServiceBindingConditionConnected(37-37)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
APIServiceExportRequest(46-60)APIServiceExportRequestPhaseFailed(154-154)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/third_party/conditions/util/conditions/setter.go (1)
MarkFalse(120-122)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemaToCRD(131-200)
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)pkg/indexers/serviceexportrequest.go (1)
ServiceExportRequestByGroupResource(26-26)
backend/controllers/serviceexport/serviceexport_reconcile.go (5)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (3)
APIServiceExport(55-66)APIServiceExportConditionProviderInSync(36-36)SourceSpecHashAnnotationKey(27-27)sdk/apis/third_party/conditions/util/conditions/setter.go (2)
MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemasSpecHash(121-129)
backend/controllers/serviceexport/serviceexport_controller.go (2)
pkg/indexers/serviceexport.go (2)
ServiceExportByBoundSchema(27-27)IndexServiceExportByBoundSchema(40-52)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (2)
ConditionType(48-48)Conditions(92-92)
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/apiexport.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-cowboys.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/cowboy.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/sheriff.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (19)
sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
86-107: Schemas optional in CRDs but required in BoundSchema; confirm persistence.If BoundSchema objects are persisted, versions without schema will violate validation.
Would you like a patch to set an explicit empty schema object when CRD schema is nil, or should we relax the BoundSchema CRD requirement instead?
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
19-35: Toolchain OK — maps/slices supported (Go ≥ 1.21).
go.mod: "go 1.24.0"; observed runtime: "go version go1.24.1".pkg/konnector/controllers/cluster/cluster_controller.go (1)
127-137: Wiring: added APIServiceExportRequests informer — LGTMThe extra informer parameter aligns with the updated servicebinding controller signature; providerBindInformers will start it. No further action.
backend/options/options.go (1)
115-117: KCP APIResourceSchema source constant — LGTMThe canonical identifier looks correct and is exposed via String().
kcp/deploy/resources/apiexport-kube-bind.io.yaml (1)
71-73: Rename to boundschemas — LGTMPublic resource rename and schema id update look consistent with the BoundSchema migration.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (3)
58-66: Constructor: added schemaSource — LGTMSignature change and plumbing look correct.
83-107: BoundSchema wiring — LGTMCache lookup and create closures look correct. No ownerRef to APIServiceExportRequest (short‑lived) — good.
152-161: BoundSchema mapper key — LGTMUsing . matches the index key.
backend/controllers/serviceexport/serviceexport_controller.go (3)
61-63: Indexer wiring LGTM.Index key and function align with pkg/indexers/serviceexport.go.
70-77: Correct: namespaced BoundSchema lookups via cache.Using types.NamespacedName with cache.Get avoids cross-namespace collisions.
184-186: Verify handler provider usage.Confirm mcbuilder.Watches accepts a provider func (clusterName, cluster) -> handler; if not, call getBoundSchemaMapper with mgr’s cluster where appropriate.
kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml (2)
13-19: Names/plural/shortNames look consistent.Values align with BoundSchema type usage across the codebase.
349-360: Status shape matches controller expectations.acceptedNames and conditions fields are compatible with current status updates.
backend/controllers/serviceexport/serviceexport_reconcile.go (3)
35-37: Good: getBoundSchema now includes namespace.Prevents collisions for namespaced BoundSchemas.
53-66: NotFound handling is correct and safe.Avoids nil deref and sets ProviderInSync=false with a clear reason.
77-85: Hash mismatch flow is fine; controller persists annotations.No further action; requeue handled upstream.
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (2)
282-297: getSchemasFromExport: message clarity LGTM.Error context includes the resource group name; good for debugging.
169-203: Status reference population looks correct.group/resource taken from BoundSchema.Spec; duplicates are filtered.
backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go (1)
273-276: Re-evaluate the 1-minute post-success failure flip.
This flips Succeeded to Failed solely based on age, which is surprising and may confuse users/automation.Would you confirm the intent? If this is a guard for stuck requests, consider gating it to Pending/NotReady states only, or remove after marking Succeeded.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
202-212: Don’t (re)create ClusterRoleBinding when scope != ClusterWhen r.scope != ClusterScope, you delete the binding but still reach the create path if it didn’t exist initially.
func (r *reconciler) ensureRBACClusterRoleBinding(...){ name := "kube-binder-" + clusterBinding.Namespace binding, err := r.getClusterRoleBinding(ctx, cache, name) ... if r.scope != kubebindv1alpha2.ClusterScope { if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) { return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err) } + return nil } ... if binding == nil { if err := r.createClusterRoleBinding(ctx, client, expected); err != nil { return fmt.Errorf("failed to create ClusterRoleBinding %s: %w", expected.Name, err) }Also applies to: 246-257
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (1)
184-185: Replace apiresourceschemas RBAC marker with boundschemas in serviceexportrequest controllerController creates/updates BoundSchema but the RBAC marker in backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (line ~184) references apiresourceschemas — change to boundschemas so list/watch/create won't 403. (boundschemas RBAC already exists in serviceexport_controller.go lines ~95–96.)
-//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=apiresourceschemas,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=kubebind.k8s.io,resources=boundschemas,verbs=get;list;watch;create;update;patch;delete
♻️ Duplicate comments (4)
kcp/deploy/examples/cowboy.yaml (1)
1-5: Looks good; trailing newline missing.Namespaced scope with explicit namespace is correct. The absence of a status block aligns with earlier feedback.
backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
169-171: Build break: slices.Collect used with []stringmaps.Keys already returns []string. slices.Collect expects an iterator.
- keys := slices.Collect(maps.Keys(grSet)) + keys := maps.Keys(grSet) slices.Sort(keys)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)
64-65: Constructor gained schemaSource — confirm all call sites updated.This param is new; ensure backend/server wiring passes it everywhere.
#!/bin/bash # Verify all invocations include the new schemaSource arg rg -n --type=go '\bNewAPIServiceExportRequestReconciler\s*\(' -C2
218-224: On error, only Phase change triggers status update (intent acknowledged).Prior feedback suggested comparing full Status; keeping Phase‑only is accepted “for now.”
🧹 Nitpick comments (14)
kcp/deploy/examples/sheriff.yaml (1)
1-4: LGTM; add trailing newline.Manifest is correct for a cluster-scoped GVK (no namespace). Add a newline at EOF to satisfy linters.
backend/http/handler.go (2)
119-125: Health check: return a tiny body and content-typeReturn a short "ok" body and set Content-Type to aid probes and CLIs.
func (h *handler) handleHealthz(w http.ResponseWriter, r *http.Request) { prepareNoCache(w) w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + _, _ = w.Write([]byte("ok")) }
391-396: Harden unstructured access to avoid panicsDirect type assertions on unstructured objects can panic; use Nested helpers and handle non-string scope values.
- if !strings.EqualFold(h.scope.String(), scope.(string)) && h.scope != kubebindv1alpha2.ClusterScope { + s, found, _ := unstructured.NestedString(item.Object, "spec", "scope") + if !found { + s = "-" + } + if !strings.EqualFold(h.scope.String(), s) && h.scope != kubebindv1alpha2.ClusterScope { continue }backend/controllers/clusterbinding/clusterbinding_reconcile.go (1)
162-185: Dedupe GRs across all exports to stabilize RBACYou dedupe per-export, but duplicates can still occur across exports; build one global GR set, then sort once for deterministic, minimal rules.
Would you like a patch that hoists grSet outside the loop and appends rules once per unique GR?
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (5)
82-95: Nit: message formatting args mismatchTerminalMessage is used as the format string; remove the extra binding.Name arg unless the message includes a verb.
- conditions.MarkFalse( + conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionConnected, "APIServiceExportFailed", conditionsapi.ConditionSeverityError, - request.Status.TerminalMessage, - binding.Name, + request.Status.TerminalMessage, )
134-136: Outdated commentRefers to APIResourceSchema; should be BoundSchema.
- // Get all APIResourceSchema objects referenced by the export + // Get all BoundSchema objects referenced by the export
205-208: Drop unused errs accumulatorThe function never appends to errs; return concrete errors directly.
-func (r *reconciler) ensureCRDsFromBoundSchema(... ) error { - var errs []error +func (r *reconciler) ensureCRDsFromBoundSchema(... ) error {
250-263: Propagate update errors and fix name in messageReturn the error so the controller can retry; use crd.Name in the message.
- if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { - return nil + if _, err := r.updateCRD(ctx, crd); err != nil && !errors.IsInvalid(err) { + return err } else if errors.IsInvalid(err) { conditions.MarkFalse( binding, kubebindv1alpha2.APIServiceBindingConditionConnected, "CustomResourceDefinitionUpdateFailed", conditionsapi.ConditionSeverityError, - "CustomResourceDefinition %s cannot be updated: %s", - binding.Name, err, + "CustomResourceDefinition %s cannot be updated: %s", + crd.Name, err, ) return nil }
287-291: Error text: be explicit (“BoundSchema”)Clarifies the failing lookup.
- return nil, fmt.Errorf("failed to get Schema %s: %w", res.ResourceGroupName(), err) + return nil, fmt.Errorf("failed to get BoundSchema %s: %w", res.ResourceGroupName(), err)backend/options/options.go (2)
122-122: Typo in comment“mostrly” → “mostly”.
- CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostrly for e2e tests + CustomResourceDefinitionSource.String(): CustomResourceDefinitionSource.String(), // mostly for e2e tests
147-155: Help text: dedupe values to avoid duplicatesBuilding the list from map values can repeat canonical strings; present a unique, sorted set.
- values := make([]string, 0, len(schemaSourceAliases)) - for _, v := range schemaSourceAliases { - values = append(values, v) - } + vs := sets.New[string]() + for _, v := range schemaSourceAliases { + vs.Insert(v) + } + values := sets.List(vs)backend/controllers/serviceexport/serviceexport_reconcile.go (3)
49-70: Missing BoundSchema handling — condition set is good; minor logging nit.Current NotFound sets ProviderInSync=False and returns. Optional: log at V(1)/V(2) for observability.
- return nil + logger.V(2).Info("BoundSchema missing", "name", name) + return nil
77-85: When hash mismatches, also mark ProviderInSync=False until persisted.Prevents transient “True” while an update is pending.
if export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] != hash { logger.V(1).Info("Updating APIServiceExport. Hash mismatch", "hash", hash, "expected", export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey]) if export.Annotations == nil { export.Annotations = map[string]string{} } export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] = hash + conditions.MarkFalse(export, kubebindv1alpha2.APIServiceExportConditionProviderInSync, "SpecHashDrift", conditionsapi.ConditionSeverityError, "Pending update to source spec hash") return nil }
51-70: Deterministic hashing: sort schemas before hashing.Guard against order-induced hash changes if resource order varies.
+ // Ensure deterministic order before hashing + sort.Slice(schemas, func(i, j int) bool { return schemas[i].Name < schemas[j].Name })Add import:
@@ - "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/errors" + "sort"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
backend/controllers/clusterbinding/clusterbinding_reconcile.go(3 hunks)backend/controllers/serviceexport/serviceexport_controller.go(5 hunks)backend/controllers/serviceexport/serviceexport_reconcile.go(1 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_controller.go(6 hunks)backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go(2 hunks)backend/http/handler.go(2 hunks)backend/kubernetes/manager.go(0 hunks)backend/options/options.go(2 hunks)cli/pkg/kubectl/bind/plugin/bind.go(1 hunks)deploy/crd/kube-bind.io_boundschemas.yaml(4 hunks)kcp/deploy/examples/apiexport.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-cowboys.yaml(1 hunks)kcp/deploy/examples/apiresourceschema-sheriffs.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-cluster.yaml(1 hunks)kcp/deploy/examples/apiserviceexport-namespaced.yaml(1 hunks)kcp/deploy/examples/cowboy.yaml(1 hunks)kcp/deploy/examples/sheriff.yaml(1 hunks)kcp/deploy/resources/apiexport-kube-bind.io.yaml(3 hunks)kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml(4 hunks)pkg/konnector/controllers/cluster/cluster_controller.go(1 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go(2 hunks)pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go(7 hunks)sdk/apis/kubebind/v1alpha2/boundchema_types.go(5 hunks)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/kubernetes/manager.go
🚧 Files skipped from review as they are similar to previous changes (11)
- cli/pkg/kubectl/bind/plugin/bind.go
- kcp/deploy/examples/apiserviceexport-cluster.yaml
- kcp/deploy/examples/apiserviceexport-namespaced.yaml
- pkg/konnector/controllers/cluster/cluster_controller.go
- deploy/crd/kube-bind.io_boundschemas.yaml
- kcp/deploy/resources/apiexport-kube-bind.io.yaml
- sdk/apis/kubebind/v1alpha2/helpers/boundschema.go
- backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go
- backend/controllers/serviceexport/serviceexport_controller.go
- kcp/deploy/resources/apiresourceschema-boundschemas.kube-bind.io.yaml
- pkg/konnector/controllers/cluster/servicebinding/servicebinding_controller.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
📚 Learning: 2025-09-12T09:05:29.743Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/client/listers/kubebind/v1alpha2/boundschema.go:46-48
Timestamp: 2025-09-12T09:05:29.743Z
Learning: In the kube-bind project, lister-gen is generating BoundSchema listers with singular resource names ("boundschema") instead of plural ("boundschemas"), which breaks client-go conventions and can cause cache lookup issues. This is identified as a generator issue that needs upstream investigation rather than manual code fixes.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gopkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:40:15.272Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go:140-143
Timestamp: 2025-09-12T08:40:15.272Z
Learning: APIServiceExportRequest resources in kube-bind are short-lived and automatically deleted after 10 minutes, so they should not be used as owner references for longer-lived resources like BoundSchema. The proper lifecycle management for BoundSchema resources created by APIServiceExportRequest is tracked in issue #297.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gopkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
📚 Learning: 2025-09-12T08:55:41.816Z
Learnt from: mjudeikis
PR: kube-bind/kube-bind#295
File: sdk/apis/kubebind/v1alpha2/helpers/boundschema.go:115-123
Timestamp: 2025-09-12T08:55:41.816Z
Learning: In BoundSchemasSpecHash function in sdk/apis/kubebind/v1alpha2/helpers/boundschema.go, silent error handling during JSON encoding (continuing on encoding errors) is acceptable to mjudeikis for the current implementation, even though it could potentially lead to incorrect hash values.
Applied to files:
backend/controllers/clusterbinding/clusterbinding_reconcile.gobackend/controllers/serviceexport/serviceexport_reconcile.gobackend/controllers/serviceexportrequest/serviceexportrequest_controller.gosdk/apis/kubebind/v1alpha2/boundchema_types.go
🧬 Code graph analysis (6)
backend/http/handler.go (1)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
ClusterScope(50-50)
backend/controllers/clusterbinding/clusterbinding_reconcile.go (3)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/kubebind/v1alpha2/register.go (2)
GroupName(32-32)Resource(42-44)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (1)
GroupResource(127-143)
backend/controllers/serviceexport/serviceexport_reconcile.go (4)
sdk/apis/kubebind/v1alpha2/apiserviceexport_types.go (3)
APIServiceExport(55-66)APIServiceExportConditionProviderInSync(36-36)SourceSpecHashAnnotationKey(27-27)sdk/apis/third_party/conditions/util/conditions/setter.go (2)
MarkFalse(120-122)MarkTrue(110-112)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemasSpecHash(121-129)
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (2)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)pkg/indexers/serviceexportrequest.go (1)
ServiceExportRequestByGroupResource(26-26)
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (6)
sdk/apis/kubebind/v1alpha1/apiservicebinding_types.go (2)
APIServiceBinding(63-73)APIServiceBindingConditionConnected(37-37)sdk/apis/kubebind/v1alpha2/apiserviceexportrequest_types.go (2)
APIServiceExportRequest(46-60)APIServiceExportRequestPhaseFailed(154-154)sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
BoundSchema(36-42)sdk/apis/third_party/conditions/util/conditions/setter.go (1)
MarkFalse(120-122)sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (1)
ConditionSeverityError(31-31)sdk/apis/kubebind/v1alpha2/helpers/boundschema.go (1)
BoundSchemaToCRD(131-200)
sdk/apis/kubebind/v1alpha2/boundchema_types.go (1)
sdk/apis/third_party/conditions/apis/conditions/v1alpha1/types.go (2)
ConditionType(48-48)Conditions(92-92)
🪛 YAMLlint (1.37.1)
kcp/deploy/examples/sheriff.yaml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiexport.yaml
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-cowboys.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/apiresourceschema-sheriffs.yaml
[error] 48-48: no new line character at the end of file
(new-line-at-end-of-file)
kcp/deploy/examples/cowboy.yaml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (21)
kcp/deploy/examples/apiexport.yaml (1)
1-16: Verify APIExport compatibility and referenced schemas exist.
- Ensure
apis.kcp.io/v1alpha2matches your KCP version.- Confirm
spec.resources[*].schemanames exactly match existing APIResourceSchemas in the workspace.- Validate that
storage: { crd: {} }is supported by your KCP version/config.kcp/deploy/examples/apiresourceschema-sheriffs.yaml (1)
1-1: Confirm apiVersion.Verify
apis.kcp.io/v1alpha1is the intended version for APIResourceSchema in your target KCP; align to newer version if available.kcp/deploy/examples/apiresourceschema-cowboys.yaml (1)
1-1: Confirm apiVersion.Verify
apis.kcp.io/v1alpha1is correct for APIResourceSchema with your KCP version; bump if needed.backend/controllers/clusterbinding/clusterbinding_reconcile.go (2)
195-196: Fix error message: “update” vs “create”Message says “create” on update path.
[raise_nitpick_refactor]- return fmt.Errorf("failed to create ClusterRole %s: %w", role.Name, err) + return fmt.Errorf("failed to update ClusterRole %s: %w", role.Name, err)
22-25: Toolchain OK — go.mod declares Go 1.24.0.
stdlib maps/slices require Go 1.21+; requirement satisfied; no changes needed.sdk/apis/kubebind/v1alpha2/boundchema_types.go (7)
29-42: BoundSchema type: looks goodCRD markers and status wiring are consistent.
Re-run codegen after merging these API changes.
54-57: InformerScope.String(): LGTM
59-70: Spec shape: LGTMInlining APICRDSpec and explicit InformerScope works.
106-120: Conversion XValidation: LGTMRule enforces webhook presence correctly for Webhook strategy.
225-233: Condition types: LGTMNames/semantics fit conditions framework.
249-256: Conditions accessors: LGTM
290-299: Schema getters/setters: LGTMRaw JSON handling is correct; preserves nil.
Also applies to: 301-312
pkg/konnector/controllers/cluster/servicebinding/servicebinding_reconcile.go (1)
169-203: BoundSchema reference/status wiring: LGTMGroup/resource extraction and status dedupe look correct.
backend/options/options.go (1)
119-126: Schema-source aliases: LGTMCanonical self-mapping and plural alias look good.
backend/controllers/serviceexportrequest/serviceexportrequest_controller.go (5)
85-94: Namespaced getBoundSchema closure — LGTM.Correctly uses types.NamespacedName and cache.Get.
152-161: Mapper key now matches index (.) — good fix.This ensures BoundSchema changes enqueue matching requests.
231-236: Full Status comparison on success path — LGTM.Ensures condition/message updates persist.
250-252: Watch BoundSchema with correct mapper — LGTM.Wiring aligns with the new index key.
105-107: Creating BoundSchema — avoid using APIServiceExportRequest as ownerRef.SERs are short‑lived (~10m). Verify the reconciler doesn’t set SER as controller/owner for BoundSchema.
backend/controllers/serviceexport/serviceexport_reconcile.go (2)
29-31: Helper and conditions imports — LGTM.
35-37: getBoundSchema now namespaced — LGTM.Matches CRD scope and other controllers.
Summary
In this PR we do the following:
BoundSchema. Purpose is the same - track the status of the bound schemaWhat Type of PR Is This?
/kind api-change
/kind cleanup
/kind feature
Related Issue(s)
Fixes #292
Fixes #309
Release Notes
Summary by CodeRabbit
New Features
Breaking Changes
Security/RBAC
Documentation