✨implement separate hub and spoke resources #59
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughAdds a v1beta1 Hub/Spoke public API and CRDs, implements full v1beta1 controllers and validating webhooks, refactors kubeconfig/args handling and kubernetes helpers, migrates v1alpha1 code to new helpers, updates Helm charts/CI, and extends unit and e2e tests and test utilities. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arturshadnik
force-pushed
the
refactor/v1beta1-implementation
branch
from
ccdea0e to
d19e8cb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
fleetconfig-controller/internal/controller/v1beta1/addon.go (2)
451-455: Normalize default install namespace when comparing.
Avoid thrash when desired namespace is empty (defaults to OCM’s addon ns) but installed has explicit ns.-func isHubAddOnMatching(installed v1beta1.InstalledHubAddOn, desired v1beta1.HubAddOn, bundleVersion string) bool { - return installed.Name == desired.Name && - installed.Namespace == desired.InstallNamespace && - installed.BundleVersion == bundleVersion -} +func isHubAddOnMatching(installed v1beta1.InstalledHubAddOn, desired v1beta1.HubAddOn, bundleVersion string) bool { + desiredNS := desired.InstallNamespace + if desiredNS == "" { + desiredNS = "open-cluster-management-addon" // or v1beta1.DefaultAddonNamespace if defined + } + return installed.Name == desired.Name && + installed.Namespace == desiredNS && + installed.BundleVersion == bundleVersion +}
420-445: String matching on CLI errors is brittle.
Consider parsing exit codes or standardizing messages at a helper to reduce false positives.fleetconfig-controller/internal/controller/v1beta1/hub_controller_test.go (1)
69-76: Test resilience: set PodNamespace if reconciler logic expects it.HubReconciler now carries PodNamespace; if Reconcile uses it, inject a value to avoid nil/empty-edge cases in tests.
Apply this minimal change:
- controllerReconciler := &HubReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - } + controllerReconciler := &HubReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + PodNamespace: "default", + }fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go (1)
69-76: Initialize new reconciler fields for deterministic tests.SpokeReconciler now has ConcurrentReconciles and PodNamespace. Seed them to avoid accidental zero/empty defaults affecting behavior.
- controllerReconciler := &SpokeReconciler{ - Client: k8sClient, - Scheme: k8sClient.Scheme(), - } + controllerReconciler := &SpokeReconciler{ + Client: k8sClient, + Scheme: k8sClient.Scheme(), + ConcurrentReconciles: 1, + PodNamespace: "default", + }fleetconfig-controller/cmd/main.go (2)
62-71: Bound the concurrency flag to sane values.Prevent 0/negative values to avoid surprising controller-runtime defaults.
Add after flag.Parse():
if spokeConcurrentReconciles <= 0 { setupLog.Info("invalid spoke-concurrent-reconciles; defaulting", "value", spokeConcurrentReconciles, "default", apiv1beta1.SpokeDefaultMaxConcurrentReconciles) spokeConcurrentReconciles = apiv1beta1.SpokeDefaultMaxConcurrentReconciles }
143-143: Log when POD_NAMESPACE is unset for local runs.Helps diagnose env misconfig outside Kubernetes.
- podNamespace := os.Getenv("POD_NAMESPACE") + podNamespace := os.Getenv("POD_NAMESPACE") + if podNamespace == "" { + setupLog.Info("POD_NAMESPACE not set; proceeding with empty namespace (local run?)") + }fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
838-898: Consider validating ConfigMap data structure.While the function handles missing ConfigMaps and keys gracefully, it doesn't validate that the YAML data in the ConfigMap is well-formed before attempting to unmarshal it. Consider adding validation to provide better error messages.
Add YAML validation before unmarshaling:
fromBytes := []byte(fromValues) + // Validate YAML syntax first + var testUnmarshal interface{} + if err := yaml.Unmarshal(fromBytes, &testUnmarshal); err != nil { + return nil, fmt.Errorf("invalid YAML in ConfigMap %s key %s: %w", nn, spoke.Spec.Klusterlet.ValuesFrom.Key, err) + } + var fromInterface = map[string]any{} err = yaml.Unmarshal(fromBytes, &fromInterface)fleetconfig-controller/api/v1beta1/spoke_types.go (1)
105-109: Typo in comment: "Deployent" should be "Deployment".Minor typographical error in the field documentation.
- // Deployent mode for klusterlet + // Deployment mode for klusterletfleetconfig-controller/api/v1beta1/hub_types.go (2)
274-279: Docstring mismatches behaviorBoth SetConditions docstrings say they “replace” existing conditions, but behavior depends on cover. Clarify to prevent misuse.
-// SetConditions sets the supplied conditions, adding net-new conditions and -// replacing any existing conditions of the same type. This is a no-op if all +// SetConditions sets the supplied conditions, adding net-new conditions and, +// when cover is true, replacing any existing conditions of the same type. This is a no-op if all // supplied conditions are identical (ignoring the last transition time) to // those already set. If cover is false, existing conditions are not replaced.-// SetConditions sets the supplied conditions on a Hub, replacing any existing conditions. +// SetConditions sets the supplied conditions on a Hub. When cover is true, existing conditions of the same type are replaced.Also applies to: 305-308
59-62: 'omitzero' is a CRD/JSONSchema marker — encoding/json ignores itencoding/json will not honor the "omitzero" option; Kubernetes codegen (kube-openapi / structured-merge-diff) does recognize it for CRD/JSONSchema behavior. Action:
- Keep "omitzero" if it’s intentionally used for CRD/schema generation.
- If you expect runtime encoding/json to omit zero-values, add
omitemptyor implement custom MarshalJSON instead.Occurrences: fleetconfig-controller/api/v1beta1/hub_types.go — RegistrationAuth (lines 59–62), Resources/Source (142–146), metadata/status tags (236–244).
fleetconfig-controller/api/v1beta1/constants.go (1)
97-99: Fix comment typos and mismatched identifier
- Comment for AddonConfigMapManifestURLKey incorrectly names RawKey.
- Spelling/grammar in ManagedByLabels/ManagedBySelector comments.
-// AddonConfigMapManifestRawKey is the data key containing a URL to download manifests. +// AddonConfigMapManifestURLKey is the data key containing a URL to download manifests.-// ManagedByLabels are labeles applies to resources to denote that fleetconfig-controller is managing the lifecycle. +// ManagedByLabels are labels applied to resources to denote that fleetconfig-controller manages their lifecycle. -// ManagedBySelector is a label selector for filtering add-on resources managed fleetconfig-controller. +// ManagedBySelector is a label selector for filtering add-on resources managed by fleetconfig-controller.Also applies to: 111-116
fleetconfig-controller/api/v1beta1/common.go (1)
153-165: Minor: prefer metav1.Now()Style: use metav1.Now() instead of wrapping time.Now().
- LastTransitionTime: metav1.Time{Time: time.Now()}, + LastTransitionTime: metav1.Now(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
fleetconfig-controller/.dockerignore(1 hunks)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go(5 hunks)fleetconfig-controller/api/v1beta1/common.go(1 hunks)fleetconfig-controller/api/v1beta1/constants.go(1 hunks)fleetconfig-controller/api/v1beta1/hub_types.go(2 hunks)fleetconfig-controller/api/v1beta1/spoke_types.go(2 hunks)fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go(7 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml(2 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/deployment-dev.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/deployment.yaml(1 hunks)fleetconfig-controller/cmd/main.go(5 hunks)fleetconfig-controller/devspace.yaml(1 hunks)fleetconfig-controller/go.mod(1 hunks)fleetconfig-controller/internal/controller/v1beta1/addon.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/common.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/hub_controller_test.go(2 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go(2 hunks)fleetconfig-controller/internal/kube/kube.go(1 hunks)fleetconfig-controller/pkg/common/common.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.gofleetconfig-controller/internal/controller/v1beta1/hub_controller_test.gofleetconfig-controller/cmd/main.gofleetconfig-controller/api/v1beta1/spoke_types.gofleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yamlfleetconfig-controller/api/v1beta1/hub_types.go
🧬 Code graph analysis (11)
fleetconfig-controller/internal/kube/kube.go (2)
fleetconfig-controller/api/v1beta1/common.go (2)
Kubeconfig(15-31)SecretReference(49-62)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
Kubeconfig(303-319)SecretReference(337-350)
fleetconfig-controller/internal/controller/v1beta1/addon.go (4)
fleetconfig-controller/api/v1alpha1/constants.go (6)
ManagedBySelector(98-98)LabelAddOnManagedBy(65-65)AddonConfigMapNamePrefix(80-80)AddonConfigMapManifestRawKey(83-83)AddonConfigMapManifestURLKey(86-86)AllowedAddonURLSchemes(90-90)fleetconfig-controller/api/v1beta1/hub_types.go (4)
Hub(231-245)AddOnConfig(158-181)InstalledHubAddOn(212-221)HubAddOn(184-198)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/api/v1beta1/spoke_types.go (2)
Spoke(265-279)AddOn(225-237)
fleetconfig-controller/cmd/main.go (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
SpokeReconciler(63-69)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
fleetconfig-controller/api/v1beta1/common.go (5)
Kubeconfig(15-31)RegistrationAuth(180-199)ResourceSpec(95-110)OCMSource(81-92)Condition(168-171)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (8)
Kubeconfig(303-319)Klusterlet(550-613)AddOn(463-475)RegistrationAuth(731-750)ResourceSpec(672-687)OCMSource(289-300)KlusterletChartConfig(617-619)Condition(163-166)
fleetconfig-controller/api/v1beta1/common.go (3)
fleetconfig-controller/internal/kube/kube.go (2)
SecretReference(116-119)Kubeconfig(109-113)fleetconfig-controller/pkg/common/common.go (2)
ResourceValues(146-148)ResourceSpec(139-143)fleetconfig-controller/api/v1alpha1/constants.go (1)
ReconcileSuccess(27-27)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (7)
fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(231-245)fleetconfig-controller/api/v1beta1/common.go (3)
NewCondition(154-165)Condition(168-171)Kubeconfig(15-31)fleetconfig-controller/internal/kube/kube.go (2)
KubeconfigFromSecretOrCluster(81-87)Kubeconfig(109-113)fleetconfig-controller/pkg/common/common.go (6)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)OperatorClient(37-47)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/api/v1beta1/spoke_types.go (2)
SpokeList(284-288)Spoke(265-279)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (10)
fleetconfig-controller/api/v1beta1/spoke_types.go (4)
Spoke(265-279)Klusterlet(86-155)KlusterletChartConfig(170-172)SpokeList(284-288)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (9)
Spoke(383-425)NewCondition(149-160)Condition(163-166)RegistrationAuth(731-750)Klusterlet(550-613)ClusterManager(250-286)Hub(178-210)Kubeconfig(303-319)KlusterletChartConfig(617-619)fleetconfig-controller/api/v1beta1/common.go (4)
NewCondition(154-165)Condition(168-171)RegistrationAuth(180-199)Kubeconfig(15-31)fleetconfig-controller/api/v1beta1/hub_types.go (3)
HubSpec(26-79)ClusterManager(119-155)Hub(231-245)fleetconfig-controller/pkg/common/common.go (7)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/kube/kube.go (4)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)RawFromInClusterRestConfig(72-78)RestConfigFromKubeconfig(19-29)fleetconfig-controller/api/v1alpha1/constants.go (3)
LabelManagedClusterType(62-62)ManagedClusterTypeHubAsSpoke(56-56)AWSIRSARegistrationDriver(74-74)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/pkg/common/common.go (2)
fleetconfig-controller/api/v1beta1/common.go (3)
Kubeconfig(15-31)ResourceSpec(95-110)ResourceValues(128-136)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (3)
fleetconfig-controller/api/v1beta1/common.go (4)
Kubeconfig(15-31)SecretReference(49-62)ResourceSpec(95-110)ResourceValues(128-136)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)SecretReference(116-119)fleetconfig-controller/pkg/common/common.go (2)
ResourceSpec(139-143)ResourceValues(146-148)
fleetconfig-controller/api/v1beta1/hub_types.go (4)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (12)
ClusterManager(250-286)Kubeconfig(303-319)SingletonControlPlane(213-224)RegistrationAuth(731-750)AddOnConfig(753-776)HubAddOn(851-865)Helm(227-247)ResourceSpec(672-687)OCMSource(289-300)Condition(163-166)InstalledHubAddOn(538-547)Hub(178-210)fleetconfig-controller/api/v1beta1/common.go (5)
Kubeconfig(15-31)RegistrationAuth(180-199)ResourceSpec(95-110)OCMSource(81-92)Condition(168-171)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)fleetconfig-controller/pkg/common/common.go (1)
ResourceSpec(139-143)
fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go (4)
fleetconfig-controller/api/v1beta1/spoke_types.go (4)
AddOn(225-237)ConfigMapRef(158-166)Klusterlet(86-155)Spoke(265-279)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (17)
AddOn(463-475)AddOnConfig(753-776)ClusterManager(250-286)Condition(163-166)Helm(227-247)Hub(178-210)HubAddOn(851-865)Kubeconfig(303-319)SingletonControlPlane(213-224)RegistrationAuth(731-750)InstalledHubAddOn(538-547)Klusterlet(550-613)SecretReference(337-350)OCMSource(289-300)ResourceSpec(672-687)ResourceValues(705-713)Spoke(383-425)fleetconfig-controller/api/v1beta1/hub_types.go (7)
AddOnConfig(158-181)ClusterManager(119-155)Helm(96-116)Hub(231-245)HubAddOn(184-198)SingletonControlPlane(82-93)InstalledHubAddOn(212-221)fleetconfig-controller/api/v1beta1/common.go (7)
Condition(168-171)Kubeconfig(15-31)RegistrationAuth(180-199)SecretReference(49-62)OCMSource(81-92)ResourceSpec(95-110)ResourceValues(128-136)
🔇 Additional comments (35)
fleetconfig-controller/.dockerignore (1)
4-4: LGTM; ignoring tmp artifacts is good.fleetconfig-controller/devspace.yaml (1)
90-92: Verify chart consumes fleetConfig.enabledcharts/fleetconfig-controller was not found in the repo; ensure the chart's templates or values.yaml reference fleetConfig.enabled or remove the unused knob. Run these checks locally:
rg -n -C1 'fleetConfig\.enabled' || true rg -n -C1 'fleetConfig' || true ls -la charts || truefleetconfig-controller/go.mod (1)
6-6: Keep mergo as a direct dependency — it's imported and used.
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go imports dario.cat/mergo (line 30) and calls mergo.Map (line 881); fleetconfig-controller/go.mod's require for dario.cat/mergo v1.0.2 is correct.fleetconfig-controller/internal/controller/v1beta1/common.go (1)
41-51: LGTM; NotFound returns (nil, nil) is appropriate.fleetconfig-controller/charts/fleetconfig-controller/templates/deployment-dev.yaml (1)
37-40: LGTM: POD_NAMESPACE via Downward API is correctly wired.Matches main.go’s expectation and won’t require extra RBAC. No further action.
fleetconfig-controller/charts/fleetconfig-controller/templates/deployment.yaml (1)
45-48: LGTM: POD_NAMESPACE added to non-dev deployment as well.Keeps dev and prod charts consistent with controller wiring.
fleetconfig-controller/internal/controller/v1beta1/hub_controller_test.go (1)
35-35: Resolved — Hub CRD is cluster-scoped.Confirmed: fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml contains "scope: Cluster", so the test's use of the fixed name "hub" without a namespace is valid.
fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go (1)
39-41: No action needed — Spoke is cluster-scoped; empty Namespace is correct.CRD at fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml declares
scope: Cluster(and api/v1beta1/spoke_types.go has+kubebuilder:resource ... scope=Cluster), so using a NamespacedName with only Name is valid.fleetconfig-controller/cmd/main.go (2)
154-158: LGTM: Hub reconciler wiring for Log and PodNamespace.Matches the chart/env changes and new struct fields.
164-169: Spoke concurrency is honored in SetupWithManager — verified.
SetupWithManager calls WithOptions(controller.Options{MaxConcurrentReconciles: r.ConcurrentReconciles}) in fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (lines 925–926).fleetconfig-controller/pkg/common/common.go (2)
108-110: LGTM: context flag only when provided.Avoids empty --context in downstream commands.
97-115: API generalization looks good; verify interface conformance at compile time (in producer packages).Since common now depends on interfaces, add compile-time assertions in the api package to catch breaks when structs change.
Example (place in api/v1beta1, not here, to avoid import cycles):
var _ common.ResourceSpec = (*v1beta1.ResourceSpec)(nil) var _ common.ResourceValues = (*v1beta1.ResourceValues)(nil)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
321-334: LGTM! Interface implementations are correct.The added getter methods correctly implement the
kube.Kubeconfiginterface, enabling polymorphic handling of kubeconfig across different API versions.
352-363: LGTM! SecretReference accessors are properly implemented.The getter methods correctly implement the
kube.SecretReferenceinterface with appropriate null-safety for pointer receivers.
365-366: Excellent use of compile-time interface checks.The interface compliance checks ensure that the types correctly implement the expected interfaces at compile time, preventing runtime errors.
689-702: LGTM! ResourceSpec interface implementation is correct.The getter methods properly implement the
common.ResourceSpecinterface, enabling interface-based resource management across the codebase.
727-728: Good addition of interface compliance checks.The compile-time checks ensure ResourceSpec and ResourceValues correctly implement their respective interfaces.
fleetconfig-controller/internal/kube/kube.go (3)
80-86: LGTM! Clean interface-based refactoring.The function now uses the
Kubeconfiginterface instead of concrete types, improving flexibility and enabling multi-version support.
90-106: LGTM! Proper interface usage with improved error handling.The function correctly uses the interface methods and provides more specific error messages including the secret reference details.
108-119: Well-designed interfaces for kubeconfig abstraction.The interfaces provide a clean abstraction layer that allows different API versions to implement their own kubeconfig types while maintaining a consistent contract.
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (7)
65-166: Comprehensive hub reconciliation implementation.The reconciliation loop properly handles all lifecycle phases including initialization, patching, finalization, and deletion. The condition management and phase transitions are well-structured.
180-197: Robust cleanup with proper preflight checks.The cleanup implementation correctly performs preflight validation before proceeding with hub cleanup, ensuring safe resource removal.
199-234: Good safety checks in cleanup preflight.The preflight checks properly validate that no ManifestWorks exist (other than addon-owned ones) before allowing cleanup to proceed, preventing data loss.
236-302: Well-orchestrated hub cleanup process.The cleanup correctly handles the deletion sequence: first marks spokes for deletion, waits for their cleanup, removes addons, and finally runs the clusteradm clean command. The error handling and logging are appropriate.
398-483: Comprehensive hub initialization with proper configuration handling.The initialization correctly handles both singleton and cluster manager configurations, including AWS IRSA, Helm values, and resource specifications. The temporary file cleanup with deferred functions is properly implemented.
485-532: Smart upgrade detection logic.The upgrade check correctly handles "default" (skip), "latest" (always upgrade), and specific version comparisons by examining the lowest bundle version across all components.
564-589: Proper controller setup with cross-resource watching.The controller correctly watches both Hub and Spoke resources, enabling timely reconciliation when spokes are created, updated, or deleted during hub operations.
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (4)
75-182: Well-structured spoke reconciliation with proper lifecycle management.The reconciliation loop correctly handles all phases including patching, finalization, deletion, and condition management. The hub metadata integration enables proper configuration propagation.
210-305: Comprehensive cleanup with proper sequencing.The cleanup correctly handles addon removal before unjoining, waits for ManifestWork cleanup, and removes all associated resources (CSR, ManagedCluster, Namespace) in the proper order.
437-570: Comprehensive spoke join implementation.The join process properly handles token retrieval, argument preparation, hub API server selection, CA certificates, proxy configuration, and klusterlet values. The temporary file cleanup is correctly implemented with deferred functions.
923-958: Good controller setup with selective Hub watching.The controller properly watches Hub resources but only triggers reconciliation when the Hub spec actually changes, avoiding unnecessary reconciliation cycles.
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
174-222: Comprehensive DeepCopy implementation for KlusterletChartConfig.The DeepCopy implementation correctly handles all nested structures including maps, slices, and embedded types. The IsEmpty check using reflection is appropriate for this use case.
298-332: Well-implemented condition management methods.The condition management methods correctly handle adding, updating, and retrieving conditions with proper equality checks that ignore transition times.
fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go (2)
147-154: DeepCopy includes Status: goodHub.DeepCopyInto deep-copies Status; prevents shared slices/maps. LGTM.
259-273: Condition and InstalledHubAddOns deep-copied correctlyConditions use element DeepCopy; InstalledHubAddOns uses value copy (fields are scalars). LGTM.
...ntroller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
fleetconfig-controller/api/v1beta1/groupversion_info.go (3)
27-27: Export the API group name for reuse and to avoid drift.Many packages (webhooks, CRD patches, RBAC, CLI) will need the group string. Exporting it as GroupName reduces duplication and typos.
-const group = "fleetconfig.open-cluster-management.io" +const GroupName = "fleetconfig.open-cluster-management.io"Also update references below (see next comment).
31-39: Derive GroupKinds from GroupVersion for single source of truth.Using GroupVersion.WithKind ensures Hub/Spoke kinds always match the registered group/version. Place GroupVersion first, then compute the GroupKinds.
-var ( - // HubGroupKind is the group kind for the Hub API - HubGroupKind = schema.GroupKind{Group: group, Kind: "Hub"} - - // SpokeGroupKind is the group kind for the Spoke API - SpokeGroupKind = schema.GroupKind{Group: group, Kind: "Spoke"} - - // GroupVersion is group version used to register these objects. - GroupVersion = schema.GroupVersion{Group: group, Version: "v1beta1"} +var ( + // GroupVersion is group version used to register these objects. + GroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1beta1"} + + // HubGroupKind is the group kind for the Hub API + HubGroupKind = GroupVersion.WithKind("Hub").GroupKind() + + // SpokeGroupKind is the group kind for the Spoke API + SpokeGroupKind = GroupVersion.WithKind("Spoke").GroupKind()
37-41: Optional compatibility alias: expose SchemeGroupVersion.Some tooling/importers expect SchemeGroupVersion. Providing an alias is low-cost and can reduce friction.
var ( // GroupVersion is group version used to register these objects. GroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1beta1"} + // SchemeGroupVersion is an alias for compatibility with k8s conventions. + SchemeGroupVersion = GroupVersionfleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (2)
61-66: Fix article in error text ("a Hub", not "an Hub").Minor UX polish for user-facing errors.
Apply:
- return fmt.Errorf("expected an Hub object but got %T", obj) + return fmt.Errorf("expected a Hub object but got %T", obj)
98-109: Use precise field paths in validation errors (spec.*), not "hub".Improves kubectl/server-side surfacing and consistency with Spoke webhook.
Apply:
- if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec.Kubeconfig, msg), - ) - } + if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), hub.Spec.Kubeconfig, msg), + ) + } - if hub.Spec.ClusterManager == nil && hub.Spec.SingletonControlPlane == nil { - allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec, "either hub.clusterManager or hub.singletonControlPlane must be specified"), - ) - } + if hub.Spec.ClusterManager == nil && hub.Spec.SingletonControlPlane == nil { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec"), hub.Spec, "either spec.clusterManager or spec.singletonControlPlane must be specified"), + ) + }fleetconfig-controller/internal/webhook/v1beta1/validation.go (7)
138-171: Prefix field paths with “spec” for addon uniqueness errors.Improves API error UX.
Apply:
- errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(i), key, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(i), key, fmt.Sprintf("duplicate addOnConfig %s (name-version) found at indices %d and %d", key, existingIndex, i))) ... - errs = append(errs, field.Invalid(field.NewPath("hubAddOns").Index(i), ha.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("hubAddOns").Index(i), ha.Name, fmt.Sprintf("duplicate hubAddOn name %s found at indices %d and %d", ha.Name, existingIndex, i))) ... - errs = append(errs, field.Invalid(field.NewPath("hubAddOns").Index(i), ha.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("hubAddOns").Index(i), ha.Name, fmt.Sprintf("hubAddOn name %s clashes with an existing addOnConfig name.", ha.Name)))
172-189: Return NotFound for missing addon ConfigMaps; keep InternalError for other failures.Better signals to users what to fix.
Apply:
- err := cli.Get(ctx, types.NamespacedName{Name: cmName, Namespace: newObject.Namespace}, &cm) - if err != nil { - errs = append(errs, field.InternalError(field.NewPath("addOnConfigs").Index(i), err)) + err := cli.Get(ctx, types.NamespacedName{Name: cmName, Namespace: newObject.Namespace}, &cm) + if err != nil { + fp := field.NewPath("spec").Child("addOnConfigs").Index(i) + if kerrs.IsNotFound(err) { + errs = append(errs, field.NotFound(fp, fmt.Sprintf("ConfigMap %s not found in namespace %s", cmName, newObject.Namespace))) + } else { + errs = append(errs, field.InternalError(fp, err)) + } continue }
191-214: Also prefix manifest source errors with “spec”.Consistent error paths.
Apply:
- errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("no inline manifests or URL found for addon %s version %s", addon.Name, addon.Version))) ... - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("only 1 of inline manifests or URL can be set for addon %s version %s", addon.Name, addon.Version)))
216-234: Avoid shadowing imported package name “url”.Use a clearer local var.
Apply:
- url, err := url.Parse(manifestsURL) + parsedURL, err := url.Parse(manifestsURL) if err != nil { ... - if !slices.Contains(v1beta1.AllowedAddonURLSchemes, url.Scheme) { + if !slices.Contains(v1beta1.AllowedAddonURLSchemes, parsedURL.Scheme) { errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("unsupported URL scheme %s for addon %s version %s. Must be one of %v", - url.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes))) + parsedURL.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes)))
306-329: Typo: isAddondEnabled → isAddonEnabled.Rename function and call site.
Apply:
- if isAddondEnabled(mcAddOns, removedAddon) { + if isAddonEnabled(mcAddOns, removedAddon) {And:
-func isAddondEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool { +func isAddonEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool {
398-412: Fix error message: you’re listing ClusterManagementAddOns.Message currently mentions ManagedClusterAddOns.
Apply:
- addonList, err := addonC.AddonV1alpha1().ClusterManagementAddOns().List(ctx, metav1.ListOptions{}) + addonList, err := addonC.AddonV1alpha1().ClusterManagementAddOns().List(ctx, metav1.ListOptions{}) if err != nil { - return nil, fmt.Errorf("failed to list ManagedClusterAddOns: %w", err) + return nil, fmt.Errorf("failed to list ClusterManagementAddOns: %w", err) }
381-396: Consider avoiding ctrl.GetConfig()/new clientset per request; add timeouts.Creating clients and doing cluster-wide Lists inside admission adds latency. If feasible, reuse a typed client injected into the validator or bound ctx with a short timeout.
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
61-68: Fix article in error text ("a Spoke", not "an Spoke").Apply:
- return fmt.Errorf("expected an Spoke object but got %T", obj) + return fmt.Errorf("expected a Spoke object but got %T", obj)
131-138: Fix oldObj type assertion error message: wrong variable in fmt.Errorf.Apply:
- if !ok { - return nil, fmt.Errorf("expected a Spoke object for the newObj but got %T", newObj) - } + if !ok { + return nil, fmt.Errorf("expected a Spoke object for the oldObj but got %T", oldObj) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fleetconfig-controller/api/v1beta1/groupversion_info.go(1 hunks)fleetconfig-controller/cmd/main.go(5 hunks)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/cmd/main.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (3)
fleetconfig-controller/internal/webhook/v1beta1/validation.go (1)
fleetconfig-controller/api/v1beta1/constants.go (2)
HubResourceName(68-68)HubInitialized(16-16)
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (2)
fleetconfig-controller/api/v1beta1/hub_types.go (3)
Hub(231-245)ClusterManager(119-155)SingletonControlPlane(82-93)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
HubGroupKind(32-32)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
Spoke(265-279)Klusterlet(86-155)fleetconfig-controller/api/v1beta1/groupversion_info.go (2)
HubGroupKind(32-32)SpokeGroupKind(35-35)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (4)
fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
27-39: Group string verified — consistent across repo.
All fleetconfig API files (v1alpha1 & v1beta1), CRDs and webhook/RBAC manifests use fleetconfig.open-cluster-management.io; no conflicting hard-coded group found.fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (2)
41-44: Webhook wiring LGTM.Validator gets a controller-runtime client; defaulter registered correctly for v1beta1.Hub.
74-79: Cannot verify — tests not found; confirm Hub tests updated for new validation ruleSandbox search returned "test/: No such file or directory" so I couldn't locate tests that may create Hub resources with empty Spec. Confirm tests that create Hub set either clusterManager or singletonControlPlane. From the repo root run to check locally:
rg -nP -C2 --type=go '\b(Hub{[^}]Spec:\sv1beta1.HubSpec{\s*}|Hub{[^}]*ObjectMeta:|NewHub()|\b(clusterManager|singletonControlPlane)\b' || true
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
40-46: Webhook wiring LGTM.Validator/defaulter registration mirrors Hub; client injection in validator is good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
99-118: Tighten field paths and bad-value payloads; use field.Required for missing refs.
- When secretReference is nil, prefer field.Required over field.Invalid.
- For invalid kubeconfig, point the path at managedClusterKubeconfig (not .secretReference) and pass the kubeconfig object as the bad value.
- For spec.kubeconfig invalid, pass spoke.Spec.Kubeconfig (not the whole object).
if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), - ) + allErrs = append(allErrs, field.Required( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), + "managedClusterKubeconfig.secretReference is required in hosted mode"), + ) } else { if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), + ) } } } if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), ) }
123-126: Correct GroupKind used for NewInvalid.Thanks for switching to SpokeGroupKind on create.
🧹 Nitpick comments (2)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
63-67: Fix article in error message.Grammar nit: “an Spoke” -> “a Spoke”.
- return fmt.Errorf("expected an Spoke object but got %T", obj) + return fmt.Errorf("expected a Spoke object but got %T", obj)
156-164: Delete validation stub is fine for now.No-op is acceptable; consider enforcing invariants or dependent cleanup later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
fleetconfig-controller/internal/controller/v1beta1/addon.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/common.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- fleetconfig-controller/internal/webhook/v1beta1/validation.go
- fleetconfig-controller/internal/controller/v1beta1/addon.go
- fleetconfig-controller/internal/controller/v1beta1/common.go
- fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
Spoke(265-279)Klusterlet(86-155)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
⏰ 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). (2)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (4)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (4)
23-34: Imports and type wiring look correct.Good switch to public v1beta1 types and controller-runtime client; operatorv1 import is appropriate.
42-45: Webhook registration LGTM.Targets v1beta1.Spoke and injects client into validator; defaulter and validator both registered.
90-96: Context-aware validate signature and type assertion are fine.No issues with ctx plumbing or type assertion here.
97-118: Sanity-check result — isKubeconfigValid only enforces presence/exclusivity; CRD enforces secretReference name/namespace
- isKubeconfigValid (fleetconfig-controller/internal/webhook/v1beta1/validation.go) only verifies that either SecretReference is set XOR InCluster is true; it does not validate SecretReference.Name/Namespace.
- SecretReference.Name and Namespace are marked +required in fleetconfig-controller/api/v1beta1/common.go and are present as required fields in the generated CRD (fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml), so server-side CRD validation will reject empty name/namespace.
- The hosted-mode check in spoke_webhook.go (managedClusterKubeconfig.SecretReference != nil) is safe assuming CRDs are installed; optionally add explicit non-empty name/namespace checks in the webhook for clearer error messages.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (3)
398-406: Skip empty CLI flags (--feature-gates,--image-registry).Passing empty values can break or confuse
clusteradm. Guard optional flags.Apply:
@@ - initArgs = append(initArgs, "--feature-gates", hub.Spec.ClusterManager.FeatureGates) + if fg := hub.Spec.ClusterManager.FeatureGates; fg != "" { + initArgs = append(initArgs, "--feature-gates", fg) + } @@ - initArgs = append(initArgs, "--image-registry", hub.Spec.ClusterManager.Source.Registry) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + initArgs = append(initArgs, "--image-registry", reg) + }(Upgrade path already covered in a previous diff block.)
Also applies to: 489-491
161-166: Avoid perpetual requeue when stable.Always requeuing keeps the reconciler hot even when
HubRunningand conditions match. Prefer no requeue in the steady state; rely on watches (incl. Spoke watch) to trigger.Apply:
- return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) + if hub.Status.Phase == v1beta1.HubRunning { + return ret(ctx, ctrl.Result{}, nil) + } + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil)
441-444: Avoid upgrade thrash whenbundleVersion=latest.Current logic upgrades on every reconcile when
latestis set. That will repeatedly invokeclusteradm upgrade, even if nothing changed.Consider gating “latest” upgrades on a spec-change or a recorded last-applied/last-checked version in
status, or compute the active bundle like in the fixed-version path and skip when equal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/internal/webhook/v1beta1/validation.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (7)
fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(231-245)fleetconfig-controller/api/v1beta1/constants.go (10)
HubCleanupFinalizer(7-7)Deleting(51-51)CleanupFailed(22-22)HubStarting(36-36)HubInitialized(16-16)AddonsConfigured(19-19)Unhealthy(48-48)HubRunning(39-39)AWSIRSARegistrationDriver(86-86)HubResourceName(68-68)fleetconfig-controller/internal/kube/kube.go (2)
KubeconfigFromSecretOrCluster(81-87)Kubeconfig(109-113)fleetconfig-controller/api/v1beta1/spoke_types.go (2)
SpokeList(284-288)Spoke(265-279)fleetconfig-controller/pkg/common/common.go (3)
AddOnClient(63-73)OperatorClient(37-47)PrepareKubeconfig(97-115)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (3)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (3)
187-211: Verify Spoke deletion scope.
cleanHubdeletes allSpokeobjects cluster-wide. If multiple Hubs are ever supported, this will remove unrelated Spokes.If multi-Hub is in scope, filter by an ownership label/selector (e.g.,
ownerHub=<hub.UID>), or set ownerReferences and use a foreground deletion.
87-93: Deferred patching LGTM.Patching with
patch.Helperin a defer ensures status/finalizers are persisted consistently.
25-26: Confirm Go toolchain ≥ 1.21 — ensure go.mod exists and declares itNo go.mod found at repo root; fleetconfig-controller/internal/controller/v1beta1/hub_controller.go imports stdlib "slices" (added in Go 1.21). Add a top-level go.mod with "go 1.21" (or replace the import with golang.org/x/exp/slices) to avoid build issues.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
328-331: Nil klusterletValues is intentional and handled downstreamLeave as-is. prepareKlusterletValuesFile returns early on nil, and ComputeHash treats nil as zero (ZeroNil: true).
🧹 Nitpick comments (3)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (3)
572-600: Improve error message context in acceptClusterInclude spoke name in the returned error to aid triage.
Apply this diff:
- return fmt.Errorf("failed to accept spoke cluster join request: %v, output: %s", err, string(out)) + return fmt.Errorf("failed to accept spoke cluster %s join request: %v, output: %s", spoke.Name, err, string(out))
603-614: Avoid magic string for condition typePrefer the upstream constant for the joined condition if available (e.g., clusterv1.ManagedClusterConditionJoined) to prevent string drift.
789-794: Avoid pointer-to-pointer and name shadowing in JSON unmarshalUsing &tokenMeta (a **tokenMeta) works but is needlessly indirect and shadows the type name.
Apply this diff:
- tokenMeta := &tokenMeta{} - if err := json.Unmarshal(stdout, &tokenMeta); err != nil { + tm := &tokenMeta{} + if err := json.Unmarshal(stdout, tm); err != nil { return nil, fmt.Errorf("failed to unmarshal join token: %w", err) } - return tokenMeta, nil + return tm, nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (8)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
Spoke(265-279)SpokeList(284-288)fleetconfig-controller/api/v1beta1/constants.go (11)
SpokeCleanupFinalizer(10-10)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)LabelManagedClusterType(74-74)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)HubResourceName(68-68)fleetconfig-controller/api/v1beta1/hub_types.go (3)
HubSpec(26-79)ClusterManager(119-155)Hub(231-245)fleetconfig-controller/pkg/common/common.go (8)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)OperatorClient(37-47)fleetconfig-controller/internal/kube/kube.go (4)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)RawFromInClusterRestConfig(72-78)RestConfigFromKubeconfig(19-29)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (2)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (9)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (9)
307-327: LGTM: Hub clients from hubKubeconfig (fallback to in-cluster via RestConfigFromKubeconfig(nil))Creation of cluster/addon clients looks correct and leverages in-cluster fallback when hubKubeconfig is nil.
386-398: LGTM: Labeling hub-as-spoke ManagedClusterConditionally labeling ManagedCluster to exclude hub-as-spoke from spoke set is sound.
401-404: LGTM: Hashing tolerates nil valuesUsing hash.ComputeHash with ZeroNil handles nil klusterletValues correctly.
618-680: LGTM: Upgrade decision logicHash change short-circuits upgrades; version comparison handles default/latest and semantic normalization properly.
683-725: LGTM: Upgrade flow and values file handlingUpgrade args, kubeconfig prep, and values file lifecycle are correct.
727-757: LGTM: Unjoin flow with purge flag and output handlingApproach is reasonable; error includes command output for diagnostics.
900-919: LGTM: Values file helper is nil/empty safeEarly returns on nil/empty and defers cleanup correctly.
925-956: LGTM: Watch wiringMax concurrency and Hub->Spoke mapping predicate are appropriate to limit reconciles to relevant spec changes.
968-984: LGTM: Hub events mapped to all SpokesSimple map func is fine for cluster-scoped Spokes.
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
97-118: Tighten field paths and “bad value” payloads; use Required for missing secretRef.
- Use
field.RequiredwhensecretReferenceis nil.- When kubeconfig is invalid, point to
managedClusterKubeconfigand pass its value, notspoke.Name.- For top‑level kubeconfig, pass
spoke.Spec.Kubeconfig, not the wholespokeobject.- if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { - if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), - ) - } else { - if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), - ) - } - } - } - if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), - ) - } + if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { + if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { + allErrs = append(allErrs, field.Required( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), + "managedClusterKubeconfig.secretReference is required in hosted mode"), + ) + } else if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), + ) + } + } + if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), + ) + }
🧹 Nitpick comments (3)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (3)
23-34: Alias k8s API errors to avoid confusion with stdlib errors.Prefer
apierrorsfor clarity and future‑proofing.- "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors"Also update uses:
- return warn, errors.NewInvalid(v1beta1.SpokeGroupKind, spoke.Name, allErrs) + return warn, apierrors.NewInvalid(v1beta1.SpokeGroupKind, spoke.Name, allErrs)and similarly in ValidateUpdate.
63-67: Fix article in error message.Grammar nit.
- return fmt.Errorf("expected an Spoke object but got %T", obj) + return fmt.Errorf("expected a Spoke object but got %T", obj)
148-153: Pass the actual kubeconfig as the bad value on update.Use the invalid field’s value, not the whole object.
- allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.066Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.066Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
Spoke(265-279)Klusterlet(86-155)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (3)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (3)
42-45: Wiring LGTM.Correct GVK target and client injection on the webhook.
123-125: Correct GroupKind on create — good catch.Now returns
SpokeGroupKind.
158-160: Correct GroupKind on update — LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (7)
798-857: Handle empty ValuesFrom.Name to avoid invalid client GETIf
spoke.Spec.Klusterlet.ValuesFromis non-nil butNameis empty, the client.Get will fail with an invalid request error.Apply this diff to handle empty name gracefully:
func (r *SpokeReconciler) mergeKlusterletValues(ctx context.Context, spoke *v1beta1.Spoke) (*v1beta1.KlusterletChartConfig, error) { logger := log.FromContext(ctx) + if spoke.Spec.Klusterlet.ValuesFrom == nil || spoke.Spec.Klusterlet.ValuesFrom.Name == "" { + // no external values source configured; use spec values as-is + logger.V(1).Info("No ValuesFrom configured, using spec values", "spoke", spoke.Name) + return spoke.Spec.Klusterlet.Values, nil + } + cm := &corev1.ConfigMap{} nn := types.NamespacedName{Name: spoke.Spec.Klusterlet.ValuesFrom.Name, Namespace: r.PodNamespace}
153-157: Track the hub retrieval error state to prevent NPEBased on the retrieved learnings, nil values are valid in certain contexts. However, when
getHubMetafails, continuing with a nilhubcan cause panics inhandleSpoke(Lines 317 and 393 dereference hub).While you set
spoke.Status.Phase = v1beta1.Unhealthyon error, consider either:
- Return early after the error to prevent the NPE
- Check for nil hub in
handleSpokebefore dereferencing
317-322: Add nil hub guard before joinSpoke callThe
joinSpokefunction dereferenceshub(Line 425-427), but ifgetHubMetareturned nil earlier, this will panic.Apply this diff to prevent the panic:
// attempt to join the spoke cluster if it hasn't already been joined if managedCluster == nil { + if hub == nil { + msg := fmt.Sprintf("Hub metadata unavailable; cannot join spoke %s", spoke.Name) + spoke.SetConditions(true, v1beta1.NewCondition( + msg, v1beta1.SpokeJoined, metav1.ConditionFalse, metav1.ConditionTrue, + )) + return fmt.Errorf(msg) + } if err := r.joinSpoke(ctx, spoke, hub, klusterletValues); err != nil {
751-753: Potential NPE when ClusterManager is nilThe code accesses
hub.Spec.ClusterManager.UseBootstrapTokenwithout checking if ClusterManager is nil.Apply this diff:
-if hub.Spec.ClusterManager != nil { - tokenArgs = append(tokenArgs, fmt.Sprintf("--use-bootstrap-token=%t", hub.Spec.ClusterManager.UseBootstrapToken)) -} +if hub.Spec.ClusterManager != nil { + tokenArgs = append(tokenArgs, fmt.Sprintf("--use-bootstrap-token=%t", hub.Spec.ClusterManager.UseBootstrapToken)) +}
922-926: Potential nil dereference in sharedFieldsChangedThe function accesses
oldSpec.ClusterManager.SourceandnewSpec.ClusterManager.Sourcewithout nil checks, which can panic.Apply this diff:
func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) || - oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || + oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity { + return true + } + // Check ClusterManager changes + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false }
387-390: Optimize upgrade check for "default" bundle versionWhen
hub.Spec.ClusterManager.Sourceis nil, accessingSource.BundleVersionwill panic. Consider adding a nil check.Apply this diff:
-upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source) +var source v1beta1.OCMSource +if hub != nil && hub.Spec.ClusterManager != nil { + source = hub.Spec.ClusterManager.Source +} +upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, source)
393-395: Guard against nil hub in upgradeSpoke pathThe upgrade path dereferences
hub.Spec.ClusterManager.Sourcewithout checking if hub is nil.Apply this diff:
if upgrade { + if hub == nil { + return fmt.Errorf("cannot upgrade spoke %s: hub metadata unavailable", spoke.Name) + } if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source); err != nil {fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
114-135: Pass invalid value instead of resource name in field errorsThe field error should pass the actual invalid value rather than the resource name for better error messages.
Apply this diff to improve error reporting:
if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { allErrs = append(allErrs, field.Invalid( field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference, "managedClusterKubeconfig.secretReference is required in hosted mode"), ) } else { if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), ) } } } if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), ) }
🧹 Nitpick comments (5)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (2)
195-199: TODO comment needs implementationThe TODO comment indicates missing error handling. Consider implementing proper error recovery strategy when hub metadata is unavailable during cleanup.
Would you like me to help implement error handling for when hub metadata is unavailable during cleanup? This could involve:
- Attempting cleanup with cached hub information
- Implementing a retry mechanism
- Providing a force-cleanup option
448-449: Use constants for magic stringsConsider defining constants for the bundle versions "default" and "latest" for better maintainability.
Add these constants at the package level:
const ( BundleVersionDefault = "default" BundleVersionLatest = "latest" )Then update the code:
-"--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, +"--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion,And in
spokeNeedsUpgrade:-if source.BundleVersion == "default" { +if source.BundleVersion == BundleVersionDefault { logger.V(0).Info("klusterlet bundleVersion is default, skipping upgrade") return false, nil } -if source.BundleVersion == "latest" { +if source.BundleVersion == BundleVersionLatest {fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
168-169: Fix invalid value parameter in field errorSimilar to the create validation, the invalid value should be the actual kubeconfig, not the entire spoke object.
Apply this diff:
allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), )
71-79: Consider caching Hub lookups for performanceThe defaulter performs a hub lookup for each spoke mutation. For clusters with many spokes, this could impact performance during bulk operations.
Consider implementing a simple TTL cache for Hub resources to reduce API server load:
type hubCache struct { sync.RWMutex entries map[string]*cacheEntry } type cacheEntry struct { hub *v1beta1.Hub timestamp time.Time } const hubCacheTTL = 30 * time.Secondfleetconfig-controller/api/v1beta1/spoke_types.go (1)
289-294: Consider extracting timeout constantThe default timeout (300) is used both as a kubebuilder annotation default (Line 72) and checked here. Consider defining a constant to maintain consistency.
const DefaultTimeout = 300 // Then in BaseArgs: fmt.Sprintf("--timeout=%d", s.Spec.Timeout),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
fleetconfig-controller/api/v1beta1/hub_types.go(2 hunks)fleetconfig-controller/api/v1beta1/spoke_types.go(2 hunks)fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go(7 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml(2 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fleetconfig-controller/api/v1beta1/hub_types.go
- fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/api/v1beta1/spoke_types.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (4)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (7)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(263-277)HubRef(84-88)KlusterletChartConfig(168-170)Klusterlet(91-153)SpokeList(282-286)fleetconfig-controller/api/v1beta1/constants.go (9)
SpokeCleanupFinalizer(10-10)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)fleetconfig-controller/api/v1beta1/common.go (1)
Kubeconfig(15-31)fleetconfig-controller/pkg/common/common.go (7)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
fleetconfig-controller/api/v1beta1/common.go (3)
Kubeconfig(15-31)ResourceSpec(95-110)Condition(168-171)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (4)
fleetconfig-controller/api/v1beta1/spoke_types.go (3)
Spoke(263-277)HubRef(84-88)Klusterlet(91-153)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
Spoke(383-425)Hub(178-210)Klusterlet(550-613)SecretReference(337-350)Kubeconfig(303-319)fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(230-244)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go (4)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
AddOn(223-235)ConfigMapRef(156-164)HubRef(84-88)Klusterlet(91-153)Spoke(263-277)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (17)
AddOn(463-475)AddOnConfig(753-776)ClusterManager(250-286)Condition(163-166)Helm(227-247)Hub(178-210)HubAddOn(851-865)Kubeconfig(303-319)SingletonControlPlane(213-224)RegistrationAuth(731-750)InstalledHubAddOn(538-547)Klusterlet(550-613)SecretReference(337-350)OCMSource(289-300)ResourceSpec(672-687)ResourceValues(705-713)Spoke(383-425)fleetconfig-controller/api/v1beta1/hub_types.go (8)
AddOnConfig(158-181)ClusterManager(119-155)Helm(96-116)Hub(230-244)HubAddOn(184-198)HubSpec(26-79)SingletonControlPlane(82-93)InstalledHubAddOn(212-221)fleetconfig-controller/api/v1beta1/common.go (7)
Condition(168-171)Kubeconfig(15-31)RegistrationAuth(180-199)SecretReference(49-62)OCMSource(81-92)ResourceSpec(95-110)ResourceValues(128-136)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/api/v1beta1/spoke_types.go (1)
166-220: Well-structured DeepCopy implementationGreat implementation of custom DeepCopy methods for
KlusterletChartConfig. The handling of maps, slices, and nested structures is thorough and correct.fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go (1)
27-587: Autogenerated DeepCopy methods are complete and correctThe autogenerated DeepCopy implementations properly handle all nested structures, including maps, slices, and pointer fields. The code correctly uses DeepCopyInto for nested structures and creates new instances for collections.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (2)
925-929: Handle nil ClusterManager in sharedFieldsChanged.Dereferencing ClusterManager.Source without a nil check can panic.
Apply this diff:
func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) || - oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || + oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity { + return true + } + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false }
804-821: Fix nil deref when ValuesFrom is omitted.ValuesFrom is optional; dereferencing it will panic when nil.
Apply this diff:
func (r *SpokeReconciler) mergeKlusterletValues(ctx context.Context, spoke *v1beta1.Spoke) (*v1beta1.KlusterletChartConfig, error) { logger := log.FromContext(ctx) + // If no external source configured, use spec values as-is. + if spoke.Spec.Klusterlet.ValuesFrom == nil { + return spoke.Spec.Klusterlet.Values, nil + } + cm := &corev1.ConfigMap{} nn := types.NamespacedName{Name: spoke.Spec.Klusterlet.ValuesFrom.Name, Namespace: r.PodNamespace}
🧹 Nitpick comments (7)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (5)
775-778: Avoid pointer-of-pointer and type/var shadowing in getToken.Use a distinct variable and pass it directly to Unmarshal.
Apply this diff:
- tokenMeta := &tokenMeta{} - if err := json.Unmarshal(stdout, &tokenMeta); err != nil { + tm := &tokenMeta{} + if err := json.Unmarshal(stdout, tm); err != nil { return nil, fmt.Errorf("failed to unmarshal join token: %w", err) } - return tokenMeta, nil + return tm, nil
153-163: Short‑circuit when hub metadata/kubeconfig is unavailable.Prevents unnecessary work/log noise when hub lookup fails or Hub is absent.
Apply this diff:
hub, hubKubeconfig, err := r.getHubMeta(ctx, spoke.Spec.HubRef) if err != nil { logger.Error(err, "Failed to get latest hub metadata") spoke.Status.Phase = v1beta1.Unhealthy } - // Handle Spoke cluster: join and/or upgrade + // Handle Spoke cluster: join and/or upgrade + if hub == nil || len(hubKubeconfig) == 0 { + logger.V(0).Info("Skipping handleSpoke: hub metadata/kubeconfig unavailable") + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) + } if err := r.handleSpoke(ctx, spoke, hub, hubKubeconfig); err != nil {
594-601: Avoid returning address of range variable.Return a pointer to the slice element to silence vet warnings and avoid pitfalls.
Apply this diff:
- for _, c := range managedCluster.Status.Conditions { - if c.Type == "ManagedClusterJoined" { - return &c - } - } + for i := range managedCluster.Status.Conditions { + if managedCluster.Status.Conditions[i].Type == "ManagedClusterJoined" { + return &managedCluster.Status.Conditions[i] + } + }
931-947: Enqueue only Spokes referencing the changed Hub.Reduces unnecessary reconciles across all Spokes.
Apply this diff:
-func (r *SpokeReconciler) mapHubEventToSpoke(ctx context.Context, _ client.Object) []reconcile.Request { +func (r *SpokeReconciler) mapHubEventToSpoke(ctx context.Context, obj client.Object) []reconcile.Request { + hub, ok := obj.(*v1beta1.Hub) + if !ok { + return nil + } spokeList := &v1beta1.SpokeList{} err := r.List(ctx, spokeList) if err != nil { r.Log.Error(err, "failed to List spokes") return nil } req := make([]reconcile.Request, 0) for _, s := range spokeList.Items { - req = append(req, reconcile.Request{ + if s.Spec.HubRef.Name != hub.Name { + continue + } + req = append(req, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: s.Name, }, }) } return req }
195-199: Replace TODO with concrete error handling.In cleanup, failing to load hub kubeconfig should set a condition and proceed with best‑effort local cleanup (e.g., CSR/namespace) rather than returning raw error.
I can draft a guarded cleanup path that skips hub‑API calls when hubKubeconfig is unavailable.
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
110-114: Fix typo in user‑facing comment.“Deployent” → “Deployment”.
Apply this diff:
- // Deployent mode for klusterlet + // Deployment mode for klusterlet
299-306: Avoid returning address of range variable.Use index iteration to return a stable pointer; mirrors fix in controller’s getJoinedCondition.
Apply this diff:
func (s *SpokeStatus) GetCondition(cType string) *Condition { - for _, c := range s.Conditions { - if c.Type == cType { - return &c - } - } + for i := range s.Conditions { + if s.Conditions[i].Type == cType { + return &s.Conditions[i] + } + } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/api/v1beta1/spoke_types.go(2 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/api/v1beta1/spoke_types.gofleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
🧬 Code graph analysis (2)
fleetconfig-controller/api/v1beta1/spoke_types.go (3)
fleetconfig-controller/api/v1beta1/common.go (3)
Kubeconfig(15-31)ResourceSpec(95-110)Condition(168-171)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
Kubeconfig(303-319)Klusterlet(550-613)ResourceSpec(672-687)KlusterletChartConfig(617-619)Condition(163-166)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (8)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(265-279)HubRef(84-88)KlusterletChartConfig(170-172)Klusterlet(91-153)SpokeList(284-288)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (7)
Spoke(383-425)Hub(178-210)Kubeconfig(303-319)ClusterManager(250-286)KlusterletChartConfig(617-619)Klusterlet(550-613)OCMSource(289-300)fleetconfig-controller/api/v1beta1/constants.go (11)
SpokeCleanupFinalizer(10-10)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)LabelManagedClusterType(74-74)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/pkg/common/common.go (7)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
843-846: Confirm merge semantics vs. comment.Comment says “ignore zero-values from spec,” but mergo.WithOverride will override existing keys (including empty values for maps). If the intent is to ignore zero/empty values from spec, confirm that YAML omitempty guarantees such keys are absent; otherwise, consider filtering empty map values before merging.
fleetconfig-controller/api/v1beta1/spoke_types.go (1)
53-56: Confirm JSON field name for proxy URL.Tag uses "proxyUrl" while field is ProxyURL. If external manifests expect "proxyURL", align accordingly; otherwise ignore.
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (4)
107-114: Treat “waiting for spokes to delete” as normal requeue, not failure.Use a sentinel error and avoid setting CleanupFailed when merely waiting.
@@ - if slices.Contains(hub.Finalizers, v1beta1.HubCleanupFinalizer) { - if err := r.cleanHub(ctx, hub); err != nil { + if slices.Contains(hub.Finalizers, v1beta1.HubCleanupFinalizer) { + if err := r.cleanHub(ctx, hub); err != nil { + if errors.Is(err, errWaitingForSpokes) { + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) + } hub.SetConditions(true, v1beta1.NewCondition( err.Error(), v1beta1.CleanupFailed, metav1.ConditionTrue, metav1.ConditionFalse, )) return ret(ctx, ctrl.Result{}, err) } } @@ const ( // originalHubKey is the key in the context that records the incoming original Hub originalHubKey contextKey = iota ) + +// errWaitingForSpokes signals transient wait during cleanup. +var errWaitingForSpokes = errors.New("waiting for background spoke deletion") @@ - // Return a retriable error to requeue and check again later - return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(spokes)) + // Return a typed error to trigger requeue without marking failure + return fmt.Errorf("%w: remaining %d spokes", errWaitingForSpokes, len(spokes))Also applies to: 168-173, 207-211
61-64: Add missing RBAC for Spokes used in cleanup.cleanHub lists/deletes Spoke resources but RBAC for Spokes is absent. Add it to avoid 403s during hub deletion.
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes,verbs=get;list;watch;delete
232-238: Avoid nil deref when ClusterManager is omitted (SingletonControlPlane).Guard hub.Spec.ClusterManager before reading PurgeOperator.
- cleanArgs := []string{ - "clean", - // name is omitted, as the default name, 'cluster-manager', is always used - fmt.Sprintf("--purge-operator=%t", hub.Spec.ClusterManager.PurgeOperator), - } + purgeOperator := false + if hub.Spec.ClusterManager != nil { + purgeOperator = hub.Spec.ClusterManager.PurgeOperator + } + cleanArgs := []string{ + "clean", + // name is omitted, as the default name, 'cluster-manager', is always used + fmt.Sprintf("--purge-operator=%t", purgeOperator), + }
239-246: Pass kubeconfig to clusteradm for clean/upgrade.Without --kubeconfig these commands may target the wrong cluster.
@@ - cmd := exec.Command(clusteradm, cleanArgs...) + cleanArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, cleanArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + } + cmd := exec.Command(clusteradm, cleanArgs...) @@ - cmd := exec.Command(clusteradm, upgradeArgs...) + upgradeArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, upgradeArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + } + cmd := exec.Command(clusteradm, upgradeArgs...)Also applies to: 486-497
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (4)
153-158: Return early on getHubMeta error to avoid noisy retries.After setting Unhealthy, exit instead of proceeding with a likely-failing handleSpoke.
hub, hubKubeconfig, err := r.getHubMeta(ctx, spoke.Spec.HubRef) if err != nil { logger.Error(err, "Failed to get latest hub metadata") spoke.Status.Phase = v1beta1.Unhealthy + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) }
923-929: Nil-safe comparison in sharedFieldsChanged.Avoid dereferencing ClusterManager when nil.
-func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) || - oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity -} +func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || + oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity { + return true + } + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false +}
387-396: Guard ClusterManager nil before dereferencing in upgrade path.SingletonControlPlane hubs have nil ClusterManager; current code will panic.
- upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source) + var upgrade bool + if hub != nil && hub.Spec.ClusterManager != nil { + upgrade, err = r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source) + } else { + logger.V(0).Info("skipping upgrade check: missing hub or hub.ClusterManager") + } if err != nil { return fmt.Errorf("failed to check if spoke cluster needs upgrade: %w", err) } @@ - if upgrade { - if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source); err != nil { + if upgrade { + if hub == nil || hub.Spec.ClusterManager == nil { + logger.V(0).Info("skipping upgrade: missing hub or hub.ClusterManager") + } else if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source); err != nil { return fmt.Errorf("failed to upgrade spoke cluster %s: %w", spoke.Name, err) } }Also applies to: 392-396
451-453: Avoid nil deref in join args when Hub.ClusterManager is absent.Gate source flags or fail fast with a clear error.
- // source args - "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, + }, spoke.BaseArgs()...) + // source args (ClusterManager may be nil with SingletonControlPlane) + if hub.Spec.ClusterManager == nil { + return fmt.Errorf("hub %s has no clusterManager; cannot determine bundle version/registry for join", hub.Name) + } + joinArgs = append(joinArgs, + "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, + "--image-registry", hub.Spec.ClusterManager.Source.Registry, }, spoke.BaseArgs()...)
🧹 Nitpick comments (3)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (2)
309-323: Don’t swallow non-change errors from addon handlers.Currently returns nil when err != nil && addonConfigChanged == false, hiding real failures. Propagate/log errors.
- addonConfigChanged, err := handleAddonConfig(ctx, r.Client, addonC, hub, r.PodNamespace) - if err != nil && addonConfigChanged { + addonConfigChanged, err := handleAddonConfig(ctx, r.Client, addonC, hub, r.PodNamespace) + if err != nil { + if addonConfigChanged { hub.SetConditions(true, v1beta1.NewCondition( err.Error(), v1beta1.AddonsConfigured, metav1.ConditionFalse, metav1.ConditionTrue, )) - return err + } + return err }
403-404: Only pass --image-registry when non-empty.Avoid emitting empty flags.
- initArgs = append(initArgs, "--image-registry", hub.Spec.ClusterManager.Source.Registry) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + initArgs = append(initArgs, "--image-registry", reg) + } @@ - "--image-registry", hub.Spec.ClusterManager.Source.Registry, + }, hub.BaseArgs()...) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + upgradeArgs = append(upgradeArgs, "--image-registry", reg) - "--wait=true", - }, hub.BaseArgs()...) + } + upgradeArgs = append(upgradeArgs, "--wait=true")Also applies to: 489-490
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
481-495: Prefer v1beta1 constant for AWS IRSA driver for consistency.Minor consistency nit: use v1beta1.AWSIRSARegistrationDriver instead of v1alpha1.
- if ra.Driver == v1alpha1.AWSIRSARegistrationDriver { + if ra.Driver == v1beta1.AWSIRSARegistrationDriver {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
🧬 Code graph analysis (2)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (8)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(265-279)HubRef(84-88)KlusterletChartConfig(170-172)Klusterlet(91-153)SpokeList(284-288)fleetconfig-controller/api/v1beta1/constants.go (12)
SpokeCleanupFinalizer(10-10)Deleting(51-51)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)LabelManagedClusterType(74-74)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/api/v1beta1/common.go (3)
NewCondition(154-165)Condition(168-171)Kubeconfig(15-31)fleetconfig-controller/pkg/common/common.go (7)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (7)
fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(230-244)fleetconfig-controller/api/v1beta1/constants.go (9)
HubCleanupFinalizer(7-7)Deleting(51-51)CleanupFailed(22-22)HubStarting(36-36)HubInitialized(16-16)AddonsConfigured(19-19)Unhealthy(48-48)HubRunning(39-39)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/internal/kube/kube.go (2)
KubeconfigFromSecretOrCluster(81-87)Kubeconfig(109-113)fleetconfig-controller/api/v1beta1/spoke_types.go (3)
SpokeList(284-288)Spoke(265-279)HubRef(84-88)fleetconfig-controller/pkg/common/common.go (4)
AddOnClient(63-73)OperatorClient(37-47)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
844-846: Verify mergo semantics match intent “ignore zero-values from spec”.With mergo.WithOverride, zero/empty values from spec may override CM values depending on types. Please confirm behavior.
Would you like me to add a unit test covering empty-string/zero-value merging cases?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
116-135: Use field.Required for missing secretReference and pass the offending value (not resource name).
- Missing secretReference should be field.Required, not field.Invalid.
- For invalid kubeconfig(s), the “bad value” should be the kubeconfig object being validated, not spoke.Name or the whole Spoke.
- Tighten the path for the invalid hosted kubeconfig to managedClusterKubeconfig (not its secretReference).
Apply:
- if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), - ) + if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { + allErrs = append(allErrs, field.Required( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), + "required in hosted mode"), + ) } else { if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), + ) } } } if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), ) }Run to spot any remaining “bad value = spoke” patterns:
#!/bin/bash rg -n -C2 -g '!**/vendor/**' -g '!**/dist/**' -P \ 'field\.Invalid\(\s*field\.NewPath\([^\)]*\)[^\)]*\)\s*,\s*spoke(\.Name)?\s*,' \ fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
165-170: Return the offending kubeconfig on update, not the Spoke object.Keeps invalid payloads actionable and consistent with create‑time validation.
- allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), + )fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (6)
61-64: Add RBAC for Spoke list/watch/delete to avoid 403s during cleanup and watches.This controller lists and deletes Spokes (cleanHub) and also watches Spoke events (SetupWithManager). Without RBAC the controller will fail at runtime.
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes,verbs=get;list;watch;delete
107-114: Don’t mark CleanupFailed when merely waiting for Spokes to delete; use a sentinel and normal requeue.Waiting for cascading Spoke deletions is not a failure and currently pollutes conditions/logs.
Introduce a typed sentinel and handle it in Reconcile:
@@ type contextKey int @@ const ( // originalHubKey is the key in the context that records the incoming original Hub originalHubKey contextKey = iota ) +// errWaitingForSpokes signals a normal, transient wait during cleanup. +var errWaitingForSpokes = errors.New("waiting for background spoke deletion")Return the sentinel from cleanHub:
@@ - // Return a retriable error to requeue and check again later - return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(spokes)) + // Return a typed, non-fatal error to trigger requeue without marking failure + return fmt.Errorf("%w: remaining %d spokes", errWaitingForSpokes, len(spokes))Treat it as a normal requeue in Reconcile:
@@ - if err := r.cleanHub(ctx, hub); err != nil { - hub.SetConditions(true, v1beta1.NewCondition( - err.Error(), v1beta1.CleanupFailed, metav1.ConditionTrue, metav1.ConditionFalse, - )) - return ret(ctx, ctrl.Result{}, err) - } + if err := r.cleanHub(ctx, hub); err != nil { + if errors.Is(err, errWaitingForSpokes) { + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) + } + hub.SetConditions(true, v1beta1.NewCondition( + err.Error(), v1beta1.CleanupFailed, metav1.ConditionTrue, metav1.ConditionFalse, + )) + return ret(ctx, ctrl.Result{}, err) + }Also applies to: 207-211, 168-173
188-205: Don’t delete all Spokes; filter to those managed by this Hub.Current logic deletes every Spoke in the cluster, risking data loss for other hubs.
- spokes := spokeList.Items + // filter spokes managed by this hub + spokes := make([]v1beta1.Spoke, 0, len(spokeList.Items)) + for i := range spokeList.Items { + if spokeList.Items[i].Spec.HubRef.Name == hub.Name { + spokes = append(spokes, spokeList.Items[i]) + } + }
232-237: Fix nil-pointer when ClusterManager is nil (SingletonControlPlane hubs).Accessing hub.Spec.ClusterManager.PurgeOperator will panic when ClusterManager is nil.
- cleanArgs := []string{ - "clean", - // name is omitted, as the default name, 'cluster-manager', is always used - fmt.Sprintf("--purge-operator=%t", hub.Spec.ClusterManager.PurgeOperator), - } + purgeOperator := false + if hub.Spec.ClusterManager != nil { + purgeOperator = hub.Spec.ClusterManager.PurgeOperator + } + cleanArgs := []string{ + "clean", + // name is omitted, as the default name, 'cluster-manager', is always used + fmt.Sprintf("--purge-operator=%t", purgeOperator), + }
239-246: Ensure clusteradm clean targets the intended cluster (append --kubeconfig).Without passing kubeconfig, clusteradm may operate on the wrong cluster.
- cleanArgs = append(cleanArgs, hub.BaseArgs()...) + cleanArgs = append(cleanArgs, hub.BaseArgs()...) + // ensure clusteradm targets the intended hub cluster + cleanArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, cleanArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + } @@ - cmd := exec.Command(clusteradm, cleanArgs...) + cmd := exec.Command(clusteradm, cleanArgs...)
486-497: Pass kubeconfig and make --image-registry optional for upgrade.Upgrade also lacks --kubeconfig; and passing an empty registry can break CLI parsing.
- upgradeArgs := append([]string{ + upgradeArgs := append([]string{ "upgrade", "clustermanager", "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, "--wait=true", }, hub.BaseArgs()...) + + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + upgradeArgs = append(upgradeArgs, "--image-registry", reg) + } + + // ensure clusteradm targets the intended hub cluster + upgradeArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, upgradeArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + }
🧹 Nitpick comments (3)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
71-79: Avoid Get() with empty HubRef.name; simplify error handling and drop unnecessary DeepCopy.
- Guard for empty HubRef.Name to prevent a failing Get during defaulting.
- For NotFound, return early after a low‑verbosity log instead of logging an “info: warning”.
- Reading scalars doesn’t need DeepCopy; use hub.Spec.Timeout/LogVerbosity directly.
Apply:
- hub := &v1beta1.Hub{} - nn := types.NamespacedName{Name: spoke.Spec.HubRef.Name} - err := d.client.Get(ctx, nn, hub) - if err != nil { - if kerrs.IsNotFound(err) { - spokelog.Info("warning: hub not found, skip setting defaults") - } - return client.IgnoreNotFound(err) - } + if spoke.Spec.HubRef.Name == "" { + spokelog.V(1).Info("HubRef.name is empty; skipping hub-derived defaults") + return nil + } + + hub := &v1beta1.Hub{} + err := d.client.Get(ctx, types.NamespacedName{Name: spoke.Spec.HubRef.Name}, hub) + if err != nil { + if kerrs.IsNotFound(err) { + spokelog.V(1).Info("hub not found; skipping hub-derived defaults", "hub", spoke.Spec.HubRef.Name) + return nil + } + return err + } @@ - if spoke.Spec.Timeout == 300 { // default value - spoke.Spec.Timeout = hub.Spec.DeepCopy().Timeout - } + if spoke.Spec.Timeout == 300 { // default value + spoke.Spec.Timeout = hub.Spec.Timeout + } - if spoke.Spec.LogVerbosity == 0 { - spoke.Spec.LogVerbosity = hub.Spec.DeepCopy().LogVerbosity - } + if spoke.Spec.LogVerbosity == 0 { + spoke.Spec.LogVerbosity = hub.Spec.LogVerbosity + }Also applies to: 81-87
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (2)
401-405: Only append --image-registry during init when non-empty.Avoid emitting an empty flag value.
- // source args - initArgs = append(initArgs, "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion) - initArgs = append(initArgs, "--image-registry", hub.Spec.ClusterManager.Source.Registry) + // source args + initArgs = append(initArgs, "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + initArgs = append(initArgs, "--image-registry", reg) + }
143-151: Error from handleHub is swallowed; confirm this is intentional.Reconcile sets phase Unhealthy but still proceeds; if this is by design, fine. Otherwise, consider returning the error to surface it to controllers/metrics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
- fleetconfig-controller/internal/webhook/v1beta1/validation.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (2)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (3)
fleetconfig-controller/api/v1beta1/spoke_types.go (3)
Spoke(265-279)HubRef(84-88)Klusterlet(91-153)fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(230-244)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (8)
fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(230-244)fleetconfig-controller/api/v1beta1/constants.go (8)
HubCleanupFinalizer(7-7)CleanupFailed(22-22)HubStarting(36-36)HubInitialized(16-16)AddonsConfigured(19-19)Unhealthy(48-48)HubRunning(39-39)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/api/v1beta1/common.go (3)
NewCondition(154-165)Condition(168-171)Kubeconfig(15-31)fleetconfig-controller/internal/kube/kube.go (2)
KubeconfigFromSecretOrCluster(81-87)Kubeconfig(109-113)fleetconfig-controller/api/v1beta1/spoke_types.go (3)
SpokeList(284-288)Spoke(265-279)HubRef(84-88)fleetconfig-controller/pkg/common/common.go (4)
AddOnClient(63-73)OperatorClient(37-47)PrepareResources(118-136)PrepareKubeconfig(97-115)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (2)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
23-35: LGTM: imports and error handling aliasing look correct.Using kerrs for apimachinery errors and bringing in field paths is standard here. No issues.
43-46: LGTM: webhook wiring.Manager wiring targets v1beta1.Spoke and injects client into defaulter/validator. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (14)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (7)
107-114: Don’t mark CleanupFailed when merely waiting for Spokes; use a sentinel and requeue.Waiting for background Spoke deletions is not a failure; it should trigger a normal requeue without setting CleanupFailed.
@@ - if slices.Contains(hub.Finalizers, v1beta1.HubCleanupFinalizer) { - if err := r.cleanHub(ctx, hub); err != nil { - hub.SetConditions(true, v1beta1.NewCondition( - err.Error(), v1beta1.CleanupFailed, metav1.ConditionTrue, metav1.ConditionFalse, - )) - return ret(ctx, ctrl.Result{}, err) - } - } + if slices.Contains(hub.Finalizers, v1beta1.HubCleanupFinalizer) { + if err := r.cleanHub(ctx, hub); err != nil { + if errors.Is(err, errWaitingForSpokes) { + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) + } + hub.SetConditions(true, v1beta1.NewCondition( + err.Error(), v1beta1.CleanupFailed, metav1.ConditionTrue, metav1.ConditionFalse, + )) + return ret(ctx, ctrl.Result{}, err) + } + }@@ type contextKey int @@ const ( // originalHubKey is the key in the context that records the incoming original Hub originalHubKey contextKey = iota ) + +// errWaitingForSpokes signals a normal, transient wait during cleanup. +var errWaitingForSpokes = errors.New("waiting for background spoke deletion")@@ - // Return a retriable error to requeue and check again later - return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(spokes)) + // Return typed, non-fatal error to trigger requeue + return fmt.Errorf("%w: remaining %d spokes", errWaitingForSpokes, len(spokes))Also applies to: 207-211
94-99: Add RBAC for Spokes (list/watch/delete).cleanHub lists and deletes Spoke objects; RBAC for Spokes is missing.
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs/status,verbs=get;update;patch // +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes,verbs=get;list;watch;delete
232-238: Guard nil ClusterManager before reading PurgeOperator.SingletonControlPlane hubs have nil ClusterManager; current code can panic.
- cleanArgs := []string{ + purgeOperator := false + if hub.Spec.ClusterManager != nil { + purgeOperator = hub.Spec.ClusterManager.PurgeOperator + } + cleanArgs := []string{ "clean", // name is omitted, as the default name, 'cluster-manager', is always used - fmt.Sprintf("--purge-operator=%t", hub.Spec.ClusterManager.PurgeOperator), + fmt.Sprintf("--purge-operator=%t", purgeOperator), }
237-246: Ensure clusteradm clean targets the Hub via --kubeconfig.Without PrepareKubeconfig, clean may hit the wrong cluster.
- cleanArgs = append(cleanArgs, hub.BaseArgs()...) - - cmd := exec.Command(clusteradm, cleanArgs...) + cleanArgs = append(cleanArgs, hub.BaseArgs()...) + cleanArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, cleanArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + } + cmd := exec.Command(clusteradm, cleanArgs...)
446-469: Guard cm == nil from getClusterManager.getClusterManager can return (nil, nil); dereferencing cm.Spec will panic.
cm, err := getClusterManager(ctx, operatorC) if err != nil { return false, err } + if cm == nil { + // CM not present yet; nothing to upgrade + return false, nil + }
486-497: Pass Hub kubeconfig to clusteradm upgrade; gate optional flags.Prevent targeting the wrong cluster and avoid passing empty registry.
upgradeArgs := append([]string{ "upgrade", "clustermanager", "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, "--wait=true", }, hub.BaseArgs()...) + + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + upgradeArgs = append(upgradeArgs, "--image-registry", reg) + } + + upgradeArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, upgradeArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return err + }
194-211: Filter to spokes managed by this Hub and compute the correct remaining count.Using all Spokes for the “remaining” count can block cleanup when unrelated Spokes exist.
- spokes := spokeList.Items - if len(spokes) > 0 { - // Mark all Spokes for deletion if they haven't been deleted yet - for i := range spokes { - spoke := &spokes[i] - if spoke.DeletionTimestamp.IsZero() && spoke.Spec.HubRef.Name == hub.Name { - logger.Info("Marking Spoke for deletion", "spoke", spoke.Name) - if err := r.Delete(ctx, spoke); err != nil && !kerrs.IsNotFound(err) { - return fmt.Errorf("failed to delete spoke %s: %w", spoke.Name, err) - } - } - } - - logger.V(1).Info("Waiting for all Spokes to be deleted before proceeding with Hub cleanup", - "remainingSpokes", len(spokes)) - // Return a retriable error to requeue and check again later - return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(spokes)) - } + // consider only spokes managed by this hub + managed := make([]v1beta1.Spoke, 0, len(spokeList.Items)) + for i := range spokeList.Items { + if spokeList.Items[i].Spec.HubRef.Name == hub.Name { + managed = append(managed, spokeList.Items[i]) + } + } + if len(managed) > 0 { + // Mark all managed Spokes for deletion if not already marked + for i := range managed { + spoke := &managed[i] + if spoke.DeletionTimestamp.IsZero() { + logger.Info("Marking Spoke for deletion", "spoke", spoke.Name) + if err := r.Delete(ctx, spoke); err != nil && !kerrs.IsNotFound(err) { + return fmt.Errorf("failed to delete spoke %s: %w", spoke.Name, err) + } + } + } + logger.V(1).Info("Waiting for Spokes managed by this hub to be deleted before proceeding with Hub cleanup", + "remainingSpokes", len(managed)) + return fmt.Errorf("%w: remaining %d spokes", errWaitingForSpokes, len(managed)) + }fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (7)
925-931: sharedFieldsChanged: guard ClusterManager pointers before comparing Source.Prevents nil deref when either side omits ClusterManager.
-func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) || - oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity -} +func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || + oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity { + return true + } + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false +}
71-74: Add missing RBAC (Hubs, ConfigMaps, CSRs, Namespaces).This controller reads Hubs/ConfigMaps and deletes CSRs/Namespaces during cleanup.
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,verbs=get;list;watch;delete +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;delete
290-302: Return early when Hub metadata is missing to avoid noisy failures.handleSpoke should gate on hub/hubKubeconfig before building clients.
func (r *SpokeReconciler) handleSpoke(ctx context.Context, spoke *v1beta1.Spoke, hub *v1beta1.Hub, hubKubeconfig []byte) error { logger := log.FromContext(ctx) logger.V(0).Info("handleSpoke", "spoke", spoke.Name) + if hub == nil || len(hubKubeconfig) == 0 { + msg := fmt.Sprintf("Hub metadata unavailable; cannot process spoke %s", spoke.Name) + spoke.SetConditions(true, v1beta1.NewCondition( + msg, v1beta1.SpokeJoined, metav1.ConditionFalse, metav1.ConditionTrue, + )) + return fmt.Errorf(msg) + }
324-331: Run ‘clusteradm accept’ against the Hub and pass --kubeconfig.acceptCluster currently uses spoke.BaseArgs and no kubeconfig; it may target the wrong cluster.
- // Accept the cluster join request - if err := acceptCluster(ctx, spoke, false); err != nil { + // Accept the cluster join request on the hub + if err := acceptCluster(ctx, r.Client, hub, spoke, false); err != nil { spoke.SetConditions(true, v1beta1.NewCondition( err.Error(), v1beta1.SpokeJoined, metav1.ConditionFalse, metav1.ConditionTrue, )) return err } @@ - if err := acceptCluster(ctx, spoke, true); err != nil { + if err := acceptCluster(ctx, r.Client, hub, spoke, true); err != nil { logger.Error(err, "failed to accept spoke cluster join request(s)", "spoke", spoke.Name) }-func acceptCluster(ctx context.Context, spoke *v1beta1.Spoke, skipApproveCheck bool) error { +func acceptCluster(ctx context.Context, kClient client.Client, hub *v1beta1.Hub, spoke *v1beta1.Spoke, skipApproveCheck bool) error { logger := log.FromContext(ctx) logger.V(0).Info("acceptCluster", "spoke", spoke.Name) - acceptArgs := append([]string{ + acceptArgs := append([]string{ "accept", "--cluster", spoke.Name, - }, spoke.BaseArgs()...) + }, hub.BaseArgs()...) + + acceptArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, kClient, hub.Spec.Kubeconfig, acceptArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return fmt.Errorf("failed to prepare hub kubeconfig: %w", err) + }Also applies to: 560-587
383-399: Guard ClusterManager before dereferencing Source in upgrade paths.ClusterManager is optional; deref without a nil check can panic.
- if hub != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { + if hub != nil && hub.Spec.ClusterManager != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source) if err != nil { return fmt.Errorf("failed to check if spoke cluster needs upgrade: %w", err) } if upgrade { if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source); err != nil { return fmt.Errorf("failed to upgrade spoke cluster %s: %w", spoke.Name, err) } } }Also applies to: 671-713
451-456: Guard ClusterManager for join source flags.Avoid nil deref when Hub uses SingletonControlPlane.
- // source args - "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, + // source args + // ensure ClusterManager is present on the hub before reading source + // (SingletonControlPlane hubs won't have it) + }, spoke.BaseArgs()...) + if hub.Spec.ClusterManager != nil { + joinArgs = append(joinArgs, + "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, + ) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + joinArgs = append(joinArgs, "--image-registry", reg) + } + } - }, spoke.BaseArgs()...)
806-816: Handle nil ValuesFrom to avoid panic.ValuesFrom is optional; dereferencing it without a nil check can panic.
- cm := &corev1.ConfigMap{} - nn := types.NamespacedName{Name: spoke.Spec.Klusterlet.ValuesFrom.Name, Namespace: r.PodNamespace} + cm := &corev1.ConfigMap{} + if spoke.Spec.Klusterlet.ValuesFrom == nil { + // no external source; use spec values as-is + return spoke.Spec.Klusterlet.Values, nil + } + nn := types.NamespacedName{Name: spoke.Spec.Klusterlet.ValuesFrom.Name, Namespace: r.PodNamespace}
🧹 Nitpick comments (2)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (2)
483-496: Prefer v1beta1 constant for driver for consistency.Use a single API version for constants.
- if ra.Driver == v1alpha1.AWSIRSARegistrationDriver { + if ra.Driver == v1beta1.AWSIRSARegistrationDriver {
266-273: Avoid taking address of range variable when deleting CSRs.Use index to get a stable pointer.
- for _, c := range csrList.Items { - trimmedName := csrSuffixPattern.ReplaceAllString(c.Name, "") - if trimmedName == spoke.Name { - if err := r.Delete(ctx, &c); err != nil { - return err - } - } - } + for i := range csrList.Items { + c := &csrList.Items[i] + trimmedName := csrSuffixPattern.ReplaceAllString(c.Name, "") + if trimmedName == spoke.Name { + if err := r.Delete(ctx, c); err != nil { + return err + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (9)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(265-279)HubRef(84-88)KlusterletChartConfig(170-172)Klusterlet(91-153)SpokeList(284-288)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (7)
Spoke(383-425)Hub(178-210)Kubeconfig(303-319)ClusterManager(250-286)KlusterletChartConfig(617-619)Klusterlet(550-613)RegistrationAuth(731-750)fleetconfig-controller/api/v1beta1/constants.go (12)
SpokeCleanupFinalizer(10-10)Deleting(51-51)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)LabelManagedClusterType(74-74)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/pkg/common/common.go (8)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)OperatorClient(37-47)fleetconfig-controller/api/v1beta1/hub_types.go (3)
Hub(230-244)ClusterManager(119-155)HubSpec(26-79)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (1)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (1)
538-551: Event mapping from Spoke to Hub looks good.HubRef is required (non-pointer); deref is safe.
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (12)
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (4)
107-114: Don’t mark CleanupFailed while just waiting on Spoke GC; use a sentinel and requeue.Current logic sets CleanupFailed=True for the normal waiting case.
Provide a typed sentinel (e.g., var errWaitingForSpokes) from cleanHub and treat it as a normal requeue in Reconcile without setting CleanupFailed.
Also applies to: 210-214
61-64: Missing RBAC for Spokes (get/list/watch/delete).cleanHub lists/deletes Spokes but RBAC only covers Hubs.
Apply:
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes,verbs=get;list;watch;delete
246-251: Run clusteradm against the intended cluster (pass kubeconfig).clean/upgrade omit --kubeconfig; may target the wrong cluster.
Apply:
- cmd := exec.Command(clusteradm, cleanArgs...) + cleanArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, cleanArgs) + if cleanupKcfg != nil { defer cleanupKcfg() } + if err != nil { return err } + cmd := exec.Command(clusteradm, cleanArgs...)- upgradeArgs := append([]string{ + upgradeArgs := append([]string{ "upgrade", "clustermanager", "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, "--wait=true", }, hub.BaseArgs()...) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + upgradeArgs = append(upgradeArgs, "--image-registry", reg) + } + upgradeArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, r.Client, hub.Spec.Kubeconfig, upgradeArgs) + if cleanupKcfg != nil { defer cleanupKcfg() } + if err != nil { return err }Also applies to: 493-509
194-214: Incorrect “remaining spokes” logic and busy requeue.You filter deletions by IsManagedBy, but remaining count/logging/error still use all spokes cluster‑wide, causing perpetual requeues when unrelated Spokes exist.
Apply:
- spokes := spokeList.Items - if len(spokes) > 0 { + // consider only spokes managed by this hub + managed := make([]v1beta1.Spoke, 0, len(spokeList.Items)) + for i := range spokeList.Items { + if spokeList.Items[i].IsManagedBy(hub.ObjectMeta) { + managed = append(managed, spokeList.Items[i]) + } + } + if len(managed) > 0 { // Mark all Spokes for deletion if they haven't been deleted yet - for i := range spokes { - spoke := &spokes[i] + for i := range managed { + spoke := &managed[i] if spoke.DeletionTimestamp.IsZero() { - if !spoke.IsManagedBy(hub.ObjectMeta) { - continue - } logger.Info("Marking Spoke for deletion", "spoke", spoke.Name) if err := r.Delete(ctx, spoke); err != nil && !kerrs.IsNotFound(err) { return fmt.Errorf("failed to delete spoke %s: %w", spoke.Name, err) } } } - logger.V(1).Info("Waiting for all Spokes to be deleted before proceeding with Hub cleanup", - "remainingSpokes", len(spokes)) - return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(spokes)) + logger.V(1).Info("Waiting for managed Spokes to delete before Hub cleanup", + "remainingSpokes", len(managed)) + return fmt.Errorf("waiting for background spoke deletion. Remaining: %d spokes", len(managed)) }fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (6)
71-74: Missing RBAC for Hubs, ConfigMaps, CSRs, Namespaces.Controller reads Hubs/ConfigMaps and deletes CSRs/Namespaces during cleanup.
Apply:
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,verbs=get;list;watch;delete +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;delete
324-331: Run ‘accept’ against the Hub (and pass kubeconfig).acceptCluster currently uses Spoke args; it must target the Hub.
Apply:
- if err := acceptCluster(ctx, spoke, false); err != nil { + if err := acceptCluster(ctx, r.Client, hub, spoke, false); err != nil { @@ - if err := acceptCluster(ctx, spoke, true); err != nil { + if err := acceptCluster(ctx, r.Client, hub, spoke, true); err != nil {-func acceptCluster(ctx context.Context, spoke *v1beta1.Spoke, skipApproveCheck bool) error { +func acceptCluster(ctx context.Context, kClient client.Client, hub *v1beta1.Hub, spoke *v1beta1.Spoke, skipApproveCheck bool) error { logger := log.FromContext(ctx) logger.V(0).Info("acceptCluster", "spoke", spoke.Name) - acceptArgs := append([]string{"accept", "--cluster", spoke.Name}, spoke.BaseArgs()...) + acceptArgs := append([]string{"accept", "--cluster", spoke.Name}, hub.BaseArgs()...) + acceptArgs, cleanupKcfg, err := common.PrepareKubeconfig(ctx, kClient, hub.Spec.Kubeconfig, acceptArgs) + if cleanupKcfg != nil { defer cleanupKcfg() } + if err != nil { return fmt.Errorf("failed to prepare hub kubeconfig: %w", err) }Also applies to: 347-351, 560-588
927-931: Nil deref risk in sharedFieldsChanged.Dereferences ClusterManager.Source without guarding for nil.
Apply:
-func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) || - oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity -} +func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || + oldSpec.Timeout != newSpec.Timeout || oldSpec.LogVerbosity != newSpec.LogVerbosity { + return true + } + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false +}
153-159: Return early on getHubMeta error to avoid thrash and nils.Continuing with nil hub/hubKubeconfig causes repeated errors.
Apply:
hub, hubKubeconfig, err := r.getHubMeta(ctx, spoke.Spec.HubRef) if err != nil { logger.Error(err, "Failed to get latest hub metadata") spoke.Status.Phase = v1beta1.Unhealthy -} + return ret(ctx, ctrl.Result{RequeueAfter: requeue}, nil) +}
806-813: Nil deref: ValuesFrom is optional.Accessing ValuesFrom.Name without a nil check will panic.
Apply:
func (r *SpokeReconciler) mergeKlusterletValues(ctx context.Context, spoke *v1beta1.Spoke) (*v1beta1.KlusterletChartConfig, error) { logger := log.FromContext(ctx) + if spoke.Spec.Klusterlet.ValuesFrom == nil { + return spoke.Spec.Klusterlet.Values, nil + } cm := &corev1.ConfigMap{} nn := types.NamespacedName{Name: spoke.Spec.Klusterlet.ValuesFrom.Name, Namespace: r.PodNamespace}
448-456: Nil deref: hub.Spec.ClusterManager may be nil (singleton path).joinSpoke dereferences Source unconditionally.
Apply:
- // source args - joinArgs = append(joinArgs, - "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, - ) + // source args (ClusterManager required for bundle/registry) + if hub.Spec.ClusterManager == nil { + return fmt.Errorf("hub has no clusterManager spec; cannot determine bundle/registry for join") + } + joinArgs = append(joinArgs, + "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, + ) + if reg := hub.Spec.ClusterManager.Source.Registry; reg != "" { + joinArgs = append(joinArgs, "--image-registry", reg) + }fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
119-138: Validation “bad value” payloads are wrong.Pass the invalid field values, not resource name/object.
Apply:
- allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "klusterlet", "managedClusterKubeconfig", "secretReference"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference, + "managedClusterKubeconfig.secretReference is required in hosted mode"), ) @@ - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "klusterlet", "managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), ) @@ - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "kubeconfig"), spoke.Spec.Kubeconfig, msg), )fleetconfig-controller/api/v1beta1/hub_types.go (1)
262-270: Fix address-of-range-variable bug in GetCondition.Returning &c yields a pointer to the loop variable, not the slice element; it will point to the wrong condition. Iterate by index and take the address of the slice element.
func (s *HubStatus) GetCondition(cType string) *Condition { - for _, c := range s.Conditions { - if c.Type == cType { - return &c - } - } + for i := range s.Conditions { + if s.Conditions[i].Type == cType { + return &s.Conditions[i] + } + } return nil }
🧹 Nitpick comments (9)
fleetconfig-controller/api/v1beta1/spoke_types.go (1)
60-61: Non-standard json tag option "omitzero".encoding/json ignores unknown options; these fields won’t be omitted. Use omitempty (or pointer types) if omission is intended.
Apply:
- Klusterlet Klusterlet `json:"klusterlet,omitzero"` + Klusterlet Klusterlet `json:"klusterlet,omitempty"` - ManagedClusterKubeconfig Kubeconfig `json:"managedClusterKubeconfig,omitzero"` + ManagedClusterKubeconfig Kubeconfig `json:"managedClusterKubeconfig,omitempty"` - Resources ResourceSpec `json:"resources,omitzero"` + Resources ResourceSpec `json:"resources,omitempty"` - metav1.ObjectMeta `json:"metadata,omitempty,omitzero"` + metav1.ObjectMeta `json:"metadata,omitempty"` - Status SpokeStatus `json:"status,omitempty,omitzero"` + Status SpokeStatus `json:"status,omitempty"`If “omit zero” is enforced elsewhere, please point to it; otherwise this change prevents surprising serialization.
Also applies to: 138-139, 148-149, 279-280, 287-288
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
168-173: Update validation: wrong “bad value” payload.Same issue on update; pass spec.kubeconfig.
Apply:
- allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec", "kubeconfig"), spoke.Spec.Kubeconfig, msg), )fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
483-496: API version mix: use v1beta1 constant for awsirsa.Avoid cross-version coupling.
Apply:
- if ra.Driver == v1alpha1.AWSIRSARegistrationDriver { + if ra.Driver == v1beta1.AWSIRSARegistrationDriver {fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml (1)
262-269: Prefer set semantics for pattern lists.Avoid duplicates and enable strategic merges.
Apply:
autoApprovedARNPatterns: description: |- @@ - items: - type: string - type: array + items: + type: string + type: array + x-kubernetes-list-type: setfleetconfig-controller/api/v1beta1/hub_types.go (5)
276-296: Ensure at most one Condition per Type; replace or append deterministically.Current logic can leave duplicates for the same Type and does not short‑circuit after replacement.
func (s *HubStatus) SetConditions(cover bool, c ...Condition) { - for _, new := range c { - exists := false - for i, existing := range s.Conditions { - if existing.Type != new.Type { - continue - } - if existing.Equal(new) { - exists = true - continue - } - exists = true - if cover { - s.Conditions[i] = new - } - } - if !exists { - s.Conditions = append(s.Conditions, new) - } - } + // index existing by type (first occurrence) + idx := make(map[string]int, len(s.Conditions)) + for i := range s.Conditions { + if _, ok := idx[s.Conditions[i].Type]; !ok { + idx[s.Conditions[i].Type] = i + } + } + for _, newC := range c { + if i, ok := idx[newC.Type]; ok { + // identical => no-op; different => replace only if cover + if s.Conditions[i].Equal(newC) || !cover { + continue + } + s.Conditions[i] = newC + continue + } + idx[newC.Type] = len(s.Conditions) + s.Conditions = append(s.Conditions, newC) + } }
254-260: Guard BaseArgs against zero/negative values.Defensive defaults avoid emitting --timeout=0 or negative verbosity if CR defaults aren’t applied.
func (h *Hub) BaseArgs() []string { - return []string{ - fmt.Sprintf("--timeout=%d", h.Spec.Timeout), - fmt.Sprintf("--v=%d", h.Spec.LogVerbosity), - } + timeout := h.Spec.Timeout + if timeout <= 0 { + timeout = 300 + } + v := h.Spec.LogVerbosity + if v < 0 { + v = 0 + } + return []string{ + fmt.Sprintf("--timeout=%d", timeout), + fmt.Sprintf("--v=%d", v), + } }
27-36: Add validation markers for APIServer and clarify CA format.
- APIServer should validate scheme with a pattern or uri format.
- Specify whether Ca expects PEM text or base64 and validate accordingly.
// APIServer is the API server URL for the Hub cluster. If provided, spokes clusters will // join the hub using this API server instead of the one in the bootstrap kubeconfig. // Spoke clusters with ForceInternalEndpointLookup set to true will ignore this field. // +optional + // +kubebuilder:validation:Pattern="^https?://" APIServer string `json:"apiServer,omitempty"` - // Hub cluster CA certificate, optional + // Hub cluster CA certificate (PEM-encoded), optional + // Consider: base64-encoded bundle if transported via env/Secret. // +optional Ca string `json:"ca,omitempty"`
95-116: Tighten Helm values docs to match types.Comments say “Comma-separated …” but fields are []string. Suggest “List of key=value strings” for Set/SetString and “List of key=jsonValue strings” for SetJSON.
- // Comma-separated Helm values, e.g., key1=val1,key2=val2. + // List of Helm values, e.g., ["key1=val1","key2=val2"]. // +optional Set []string `json:"set,omitempty"` - // Comma-separated Helm JSON values, e.g., key1=jsonval1,key2=jsonval2. + // List of Helm JSON values, e.g., ["key1={\"a\":1}"]. // +optional SetJSON []string `json:"setJson,omitempty"` - // Comma-separated Helm literal STRING values. + // List of Helm literal STRING values (no type coercion). // +optional SetLiteral []string `json:"setLiteral,omitempty"` - // Comma-separated Helm STRING values, e.g., key1=val1,key2=val2. + // List of Helm STRING values, e.g., ["key1=val1","key2=val2"]. // +optional SetString []string `json:"setString,omitempty"`
223-227: Optional: add NAME column or ready condition to printcolumns.Consider adding a Ready/Available condition column for kubectl UX.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
fleetconfig-controller/api/v1beta1/hub_types.go(2 hunks)fleetconfig-controller/api/v1beta1/spoke_types.go(2 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml(2 hunks)fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(4 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yamlfleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/api/v1beta1/hub_types.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.gofleetconfig-controller/api/v1beta1/spoke_types.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/controller/v1beta1/hub_controller.go
🧬 Code graph analysis (4)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (7)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(274-288)HubRef(84-92)KlusterletChartConfig(179-181)Klusterlet(100-162)SpokeList(293-297)fleetconfig-controller/api/v1beta1/constants.go (11)
SpokeCleanupFinalizer(10-10)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)Unhealthy(48-48)SpokeRunning(45-45)LabelManagedClusterType(74-74)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)AWSIRSARegistrationDriver(86-86)fleetconfig-controller/pkg/common/common.go (8)
ClusterClient(24-34)WorkClient(50-60)AddOnClient(63-73)GetManagedCluster(76-85)UpdateManagedCluster(88-93)PrepareResources(118-136)PrepareKubeconfig(97-115)OperatorClient(37-47)fleetconfig-controller/internal/kube/kube.go (2)
Kubeconfig(109-113)KubeconfigFromSecretOrCluster(81-87)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (3)
fleetconfig-controller/api/v1beta1/spoke_types.go (3)
Spoke(274-288)HubRef(84-92)Klusterlet(100-162)fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(229-243)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
fleetconfig-controller/api/v1beta1/hub_types.go (4)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (12)
ClusterManager(250-286)Kubeconfig(303-319)SingletonControlPlane(213-224)RegistrationAuth(731-750)AddOnConfig(753-776)HubAddOn(851-865)Helm(227-247)ResourceSpec(672-687)OCMSource(289-300)Condition(163-166)InstalledHubAddOn(538-547)Hub(178-210)fleetconfig-controller/api/v1beta1/common.go (5)
Kubeconfig(15-31)RegistrationAuth(180-199)ResourceSpec(95-110)OCMSource(81-92)Condition(168-171)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)fleetconfig-controller/pkg/common/common.go (1)
ResourceSpec(139-143)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
fleetconfig-controller/api/v1beta1/common.go (3)
Kubeconfig(15-31)ResourceSpec(95-110)Condition(168-171)fleetconfig-controller/internal/kube/kube.go (1)
Kubeconfig(109-113)
⏰ 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). (3)
- GitHub Check: test (fleetconfig-controller) / Ensure PR is reviewable & run tests
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (5)
fleetconfig-controller/api/v1beta1/spoke_types.go (2)
119-123: Typo: “Deployent” → “Deployment”.[raise_nitpick_issue]
Apply:- // Deployent mode for klusterlet + // Deployment mode for klusterlet
200-226: Deep-copy logic looks solid.Correct handling of maps, slices, and k8s structs.
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (1)
545-559: LGTM: event mapping from Spoke → Hub.Using HubRef to enqueue the owning Hub is clean and safe (required struct, no nil risk).
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml (1)
15-25: LGTM: Namespaced scope and printer columns.Matches controller lookups (HubRef includes namespace) and improves UX.
fleetconfig-controller/api/v1beta1/hub_types.go (1)
118-155: Confirm non-standard json tag option "omitzero".Fields like Resources/Source use
json:",omitzero", which encoding/json ignores. If you rely on a custom marshaller, fine; otherwise consideromitemptyor pointers for canonical OpenAPI/CRD behavior.
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
Outdated
Show resolved
Hide resolved
arturshadnik
force-pushed
the
refactor/v1beta1-implementation
branch
from
d9d5a97 to
5936f05
Compare
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
fleetconfig-controller/internal/webhook/v1beta1/validation.go (9)
88-90: Error text omits spec.kubeconfig, which is allowed above.Keep the message consistent with the masking.
- return errors.New("only changes to spec.apiServer, spec.clusterManager.source.*, spec.hubAddOns, spec.addOnConfigs, spec.logVerbosity, spec.timeout, and spec.registrationAuth are allowed when updating the hub") + return errors.New("only changes to spec.apiServer, spec.clusterManager.source.*, spec.hubAddOns, spec.addOnConfigs, spec.logVerbosity, spec.timeout, spec.registrationAuth, and spec.kubeconfig are allowed when updating the hub")
189-199: Return 422 for missing ConfigMaps; keep paths under spec.*A NotFound on referenced CM is a user error, not an internal server error. Also fix field paths.
- err := cli.Get(ctx, types.NamespacedName{Name: cmName, Namespace: newObject.Namespace}, &cm) - if err != nil { - errs = append(errs, field.InternalError(field.NewPath("addOnConfigs").Index(i), err)) - continue - } + err := cli.Get(ctx, types.NamespacedName{Name: cmName, Namespace: newObject.Namespace}, &cm) + if err != nil { + p := field.NewPath("spec").Child("addOnConfigs").Index(i) + if kerrs.IsNotFound(err) { + errs = append(errs, field.Invalid(p, cmName, "referenced ConfigMap not found")) + } else { + errs = append(errs, field.InternalError(p, err)) + } + continue + }
213-224: Use spec. field paths in errors from validateManifestSource.*- errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("no inline manifests or URL found for addon %s version %s", addon.Name, addon.Version))) @@ - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("only 1 of inline manifests or URL can be set for addon %s version %s", addon.Name, addon.Version)))
233-244: Avoid package shadowing and validate host in URLs.Variable
urlshadows the package; also accept only URLs with a non-empty host.- url, err := url.Parse(manifestsURL) + u, err := url.Parse(manifestsURL) if err != nil { - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("invalid URL '%s' for addon %s version %s. %v", manifestsURL, addon.Name, addon.Version, err.Error()))) return errs } - if !slices.Contains(v1beta1.AllowedAddonURLSchemes, url.Scheme) { - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + if u.Host == "" { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, + fmt.Sprintf("invalid URL '%s' for addon %s version %s: host is empty", manifestsURL, addon.Name, addon.Version))) + } + if !slices.Contains(v1beta1.AllowedAddonURLSchemes, u.Scheme) { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("unsupported URL scheme %s for addon %s version %s. Must be one of %v", - url.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes))) + u.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes))) }
335-338: Field path should include spec.*.- errs = append(errs, field.Invalid(field.NewPath(fieldPath), inUseAddons, + errs = append(errs, field.Invalid(field.NewPath("spec").Child(fieldPath), inUseAddons, fmt.Sprintf("cannot remove %s %v as they are still in use by managedclusteraddons", fieldPath, inUseAddons)))
319-334: Typo: isAddondEnabled → isAddonEnabled (and update call site).Also keeps naming consistent with comment.
- if isAddondEnabled(mcAddOns, removedAddon) { + if isAddonEnabled(mcAddOns, removedAddon) { inUseAddons = append(inUseAddons, removedAddon) } @@ -func isAddondEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool { +func isAddonEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool {Also applies to: 383-392
368-377: Membership test: use a set, not O(n·m) scans, and tighten the message.- cmaNames := make([]string, len(cmaList)) - for i, cma := range cmaList { - cmaNames[i] = cma.Name - } + cmaSet := make(map[string]struct{}, len(cmaList)) + for _, cma := range cmaList { + cmaSet[cma.Name] = struct{}{} + } @@ - if !slices.Contains(cmaNames, a.ConfigName) { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOns").Index(i), a.ConfigName, "no matching HubAddOn or AddOnConfig found for AddOn")) + if _, ok := cmaSet[a.ConfigName]; !ok { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOns").Index(i), a.ConfigName, "no matching ClusterManagementAddOn found for AddOn")) }
421-422: Wrong resource in error text.- return nil, fmt.Errorf("failed to list ManagedClusterAddOns: %w", err) + return nil, fmt.Errorf("failed to list ClusterManagementAddOns: %w", err)
27-35: Unused helper (here). Wire it or remove.
isKubeconfigValidisn’t used in this file; either call it from update/create paths or drop it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/internal/webhook/v1beta1/validation_test.go
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-09-22T18:42:03.376Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/PROJECT:28-31
Timestamp: 2025-09-22T18:42:03.376Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller, the PROJECT file defines multiple API resources with different webhook configurations: FleetConfig v1alpha1 has defaulting: true (requiring MutatingWebhookConfiguration), while Hub and Spoke v1beta1 resources have defaulting: false. MutatingWebhookConfiguration resources in the manifests serve the v1alpha1 FleetConfig, not the v1beta1 Hub/Spoke resources.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/webhook/v1beta1/validation.go (2)
fleetconfig-controller/api/v1beta1/constants.go (6)
AddonConfigMapNamePrefix(89-89)AddonConfigMapManifestRawKey(92-92)AddonConfigMapManifestURLKey(95-95)AllowedAddonURLSchemes(105-105)HubInitialized(16-16)ManagedBySelector(113-113)fleetconfig-controller/api/v1beta1/spoke_types.go (1)
HubRef(86-94)
⏰ 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). (2)
- GitHub Check: e2e (fleetconfig-controller) / e2e
- GitHub Check: test (fleetconfig-controller) / Run Helm Chart Tests
🔇 Additional comments (2)
fleetconfig-controller/internal/webhook/v1beta1/validation.go (2)
9-9: Confirmed — std 'slices' is safe to use (Go ≥ 1.21 present).
fleetconfig-controller/go.mod: go 1.24.4 (apiserver & uiserver: 1.24.1) — no change required.
256-266: Validate name-format alignment for removal check
removedAddOnConfigs entries use “name-version”, but eachcr.DesiredConfig.Nameholds the target resource’s name (e.g. the AddOnDeploymentConfig name)—confirm these strings actually match or adjust the matching logic so removal protection can’t be bypassed.
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/planner.yml (1)
8-10: Concurrency group will collide across all PRs targeting the same base branch.With pull_request_target, github.ref is the base branch (e.g., refs/heads/main), so PRs will cancel each other. Scope the group to the PR number.
Apply:
concurrency: - group: planner-${{ github.ref }} + # Avoid cross-PR cancellations on pull_request_target + group: planner-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true
🧹 Nitpick comments (10)
.github/workflows/planner.yml (2)
5-5: Confirm pull_request_target usage and downstream safety.Running on pull_request_target is fine for metadata-only work, but ensure any reusable workflows/jobs that execute PR code (e2e, build, etc.) never run untrusted code with inherited secrets. If they do, switch those to pull_request or gate code execution.
94-94: Trim trailing whitespace.- +fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (2)
45-55: Optional: build addon client from mgr.GetConfig() directly.Avoid converting rest.Config → kubeconfig bytes → rest.Config. It trims startup work and removes the dependency on internal helpers in this path.
- kubeconfig, err := kube.RawFromInClusterRestConfig() - if err != nil { - return err - } - addonC, err := common.AddOnClient(kubeconfig) - if err != nil { - return err - } + addonC, err := versioned.NewForConfig(mgr.GetConfig()) + if err != nil { + return fmt.Errorf("create ocm addon client: %w", err) + }Also good call not wiring a defaulter here; v1beta1 defaulting is disabled per PROJECT.
115-147: Fix Invalid “bad value” on update kubeconfig check.Return the actual invalid kubeconfig value rather than the whole object.
- allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), + )fleetconfig-controller/internal/webhook/v1beta1/validation.go (4)
87-89: Error message omits spec.kubeconfig (allowed change).You zero out Kubeconfig but don’t list it in the allowed-changes error, which is misleading.
Apply:
- return errors.New("only changes to spec.apiServer, spec.clusterManager.source.*, spec.hubAddOns, spec.addOnConfigs, spec.logVerbosity, spec.timeout, and spec.registrationAuth are allowed when updating the hub") + return errors.New("only changes to spec.apiServer, spec.clusterManager.source.*, spec.hubAddOns, spec.addOnConfigs, spec.logVerbosity, spec.timeout, spec.registrationAuth, and spec.kubeconfig are allowed when updating the hub")
193-197: Use spec. field paths and avoid package-shadowing in URL validation.*
- Switch paths from "addOnConfigs" to "spec.addOnConfigs" for accurate error locations.
- Variable name "url" shadows the imported package; rename for clarity.
Apply:
- errs = append(errs, field.InternalError(field.NewPath("addOnConfigs").Index(i), err)) + errs = append(errs, field.InternalError(field.NewPath("spec").Child("addOnConfigs").Index(i), err))- errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("no inline manifests or URL found for addon %s version %s", addon.Name, addon.Version))) @@ - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("only 1 of inline manifests or URL can be set for addon %s version %s", addon.Name, addon.Version)))- url, err := url.Parse(manifestsURL) + parsedURL, err := url.Parse(manifestsURL) @@ - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("invalid URL '%s' for addon %s version %s. %v", manifestsURL, addon.Name, addon.Version, err.Error()))) @@ - if !slices.Contains(v1beta1.AllowedAddonURLSchemes, url.Scheme) { - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(index), addon.Name, + if !slices.Contains(v1beta1.AllowedAddonURLSchemes, parsedURL.Scheme) { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(index), addon.Name, fmt.Sprintf("unsupported URL scheme %s for addon %s version %s. Must be one of %v", - url.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes))) + parsedURL.Scheme, addon.Name, addon.Version, v1beta1.AllowedAddonURLSchemes)))Also applies to: 213-219, 234-244
327-335: Typo in helper name: isAddondEnabled → isAddonEnabled.Minor readability polish; update call site too.
- if isAddondEnabled(mcAddOns.Items, removedAddon) { + if isAddonEnabled(mcAddOns.Items, removedAddon) { @@ -func isAddondEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool { +func isAddonEnabled(mcAddOns []addonv1alpha1.ManagedClusterAddOn, removedAddon string) bool {Also applies to: 382-391
343-379: Spoke AddOn validation only checks CMA existence; consider cross-checking Hub policy.Current check ensures AddOn.ConfigName exists as a ClusterManagementAddOn, but not that it’s configured/enabled by the referenced Hub (HubAddOns/AddOnConfigs). If policy intends spokes to select only hub-offered addons, add a membership check against hub.Spec.HubAddOns/AddOnConfigs.
Do tests/spec require restricting Spoke AddOns to hub-published ones? If yes, I can draft a follow‑up patch that builds a set from the Hub spec and validates membership.
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (2)
122-126: Use spec.kubeconfig in update error path.Aligns with create-path fix.
- allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec.Kubeconfig, msg), - ) + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), hub.Spec.Kubeconfig, msg), + )
23-23: Alias k8s API errors to avoid confusion with stdlib errors.Not functionally wrong, but “errors” usually refers to stdlib. Alias improves readability.
- "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" @@ - return nil, errors.NewInvalid(v1beta1.HubGroupKind, hub.Name, allErrs) + return nil, apierrors.NewInvalid(v1beta1.HubGroupKind, hub.Name, allErrs) @@ - return nil, errors.NewInvalid(v1beta1.HubGroupKind, hub.Name, allErrs) + return nil, apierrors.NewInvalid(v1beta1.HubGroupKind, hub.Name, allErrs)Also applies to: 98-99, 130-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/planner.yml(4 hunks)fleetconfig-controller/charts/fleetconfig-controller/templates/admission-webhooks/validating-webhook-configuration.yaml(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go(3 hunks)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go(3 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- fleetconfig-controller/charts/fleetconfig-controller/templates/admission-webhooks/validating-webhook-configuration.yaml
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-09-22T18:42:03.376Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/PROJECT:28-31
Timestamp: 2025-09-22T18:42:03.376Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller, the PROJECT file defines multiple API resources with different webhook configurations: FleetConfig v1alpha1 has defaulting: true (requiring MutatingWebhookConfiguration), while Hub and Spoke v1beta1 resources have defaulting: false. MutatingWebhookConfiguration resources in the manifests serve the v1alpha1 FleetConfig, not the v1beta1 Hub/Spoke resources.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-22T19:16:34.074Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/webhook/v1beta1/validation.go:103-121
Timestamp: 2025-09-22T19:16:34.074Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller v1beta1 API, the Klusterlet field in SpokeSpec is defined as a struct value (Klusterlet Klusterlet), not a pointer (*Klusterlet), so direct field access like Klusterlet.Annotations is safe without nil checks. The Klusterlet struct does not contain a Source field.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation.gofleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go
🧬 Code graph analysis (3)
fleetconfig-controller/internal/webhook/v1beta1/validation.go (2)
fleetconfig-controller/api/v1beta1/constants.go (6)
AddonConfigMapNamePrefix(89-89)AddonConfigMapManifestRawKey(92-92)AddonConfigMapManifestURLKey(95-95)AllowedAddonURLSchemes(105-105)ManagedBySelector(113-113)HubInitialized(16-16)fleetconfig-controller/api/v1beta1/spoke_types.go (1)
HubRef(86-94)
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (4)
fleetconfig-controller/internal/kube/kube.go (1)
RawFromInClusterRestConfig(75-81)fleetconfig-controller/pkg/common/common.go (1)
AddOnClient(59-69)fleetconfig-controller/api/v1beta1/spoke_types.go (1)
Spoke(276-290)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
SpokeGroupKind(35-35)
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (3)
fleetconfig-controller/internal/kube/kube.go (1)
RawFromInClusterRestConfig(75-81)fleetconfig-controller/pkg/common/common.go (1)
AddOnClient(59-69)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
HubGroupKind(32-32)
🔇 Additional comments (12)
.github/workflows/planner.yml (4)
95-101: Good: job outputs are exposed and step id matches.
154-156: Good dependency wiring.Adding extract-label-filter to needs ensures input availability before e2e dispatch.
166-166: Verify downstream input defaulting.Ensure .github/workflows/e2e.yml defines input ginkgoLabelFilter with a safe default (e.g., empty string) and handles empty/null gracefully. With the above else-branch fix, it will always be defined, but confirm e2e.yml logic.
102-137: Quote expansions, handle empty output, and avoid word‑splitting.Unquoted $LABELS and $GITHUB_OUTPUT trigger actionlint (SC2086) and can break on spaces. Also set an explicit empty output when no filters are present.
Apply:
- run: | - set -e + run: | + set -Eeuo pipefail # Find labels that start with 'ginkgo-filter:' - LABELS="${{ join(github.event.pull_request.labels.*.name, ',') }}" + LABELS="${{ join(github.event.pull_request.labels.*.name, ',') }}" # Array to collect all filters - FILTERS=() + FILTERS=() - # Extract all ginkgo filters - for label in $(echo $LABELS | tr ',' '\n'); do - if [[ $label == ginkgo-filter:* ]]; then - # Extract the filter part after the prefix - FILTER="${label#ginkgo-filter:}" - echo "Found Ginkgo filter in label: $FILTER" - FILTERS+=("$FILTER") - fi - done + # If no labels, expose empty output and exit + if [[ -z "$LABELS" ]]; then + echo "ginkgoLabelFilter=" >> "$GITHUB_OUTPUT" + exit 0 + fi + + # Extract all ginkgo filters (space‑safe) + IFS=',' read -r -a __labels <<< "$LABELS" + for label in "${__labels[@]}"; do + if [[ "$label" == ginkgo-filter:* ]]; then + FILTERS+=("${label#ginkgo-filter:}") + fi + done # If we have filters, combine them with OR operator - if [ ${#FILTERS[@]} -gt 0 ]; then - COMBINED_FILTER="" - - # OR each filter - for i in "${!FILTERS[@]}"; do - if [ $i -eq 0 ]; then - COMBINED_FILTER="(${FILTERS[$i]})" - else - COMBINED_FILTER="$COMBINED_FILTER||(${FILTERS[$i]})" - fi - done - - echo "Final ginkgo label filter: $COMBINED_FILTER" - echo "ginkgoLabelFilter=$COMBINED_FILTER" >> $GITHUB_OUTPUT - fi + if (( ${#FILTERS[@]} > 0 )); then + COMBINED_FILTER="" + for i in "${!FILTERS[@]}"; do + if (( i == 0 )); then + COMBINED_FILTER="(${FILTERS[$i]})" + else + COMBINED_FILTER="${COMBINED_FILTER}||(${FILTERS[$i]})" + fi + done + echo "Final ginkgo label filter: $COMBINED_FILTER" + echo "ginkgoLabelFilter=$COMBINED_FILTER" >> "$GITHUB_OUTPUT" + else + echo "ginkgoLabelFilter=" >> "$GITHUB_OUTPUT" + fiOptionally add permissions: contents: read for this job for least privilege.
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (5)
23-37: Imports look correct.All imports are used and align with the refactor to public v1beta1 types and addon client usage.
68-70: Validator fields LGTM.Carrying both controller-runtime client and addon client here is appropriate for validation.
105-111: Add-on validation wiring LGTM.Aggregating warnings and field errors from
validateAddonsis correct and consistent with create flow.
151-159: Delete validation LGTM.Type assertion and logging only; no additional delete-time constraints required here.
82-103: Fix validation error payloads and paths; use Required for missing secretReference.
- Pass the invalid value (kubeconfig objects), not
spoke/spoke.Name.- Use
field.RequiredwhensecretReferenceis missing.- Point the path to
managedClusterKubeconfigwhen the kubeconfig content is invalid.- if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { - if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, "managedClusterKubeconfig.secretReference is required in hosted mode"), - ) - } else { - if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), - spoke.Name, msg), - ) - } - } - } - if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("spec").Child("kubeconfig"), spoke, msg), - ) - } + if spoke.Spec.Klusterlet.Mode == string(operatorv1.InstallModeHosted) { + if spoke.Spec.Klusterlet.ManagedClusterKubeconfig.SecretReference == nil { + allErrs = append(allErrs, field.Required( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig").Child("secretReference"), + "managedClusterKubeconfig.secretReference is required in hosted mode"), + ) + } else if valid, msg := isKubeconfigValid(spoke.Spec.Klusterlet.ManagedClusterKubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("klusterlet").Child("managedClusterKubeconfig"), + spoke.Spec.Klusterlet.ManagedClusterKubeconfig, msg), + ) + } + } + if valid, msg := isKubeconfigValid(spoke.Spec.Kubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), spoke.Spec.Kubeconfig, msg), + ) + }Note: If
ManagedClusterKubeconfigitself is a pointer type, add a guard before dereferencingSecretReferenceto avoid a nil deref.To confirm whether a guard is needed, run:
#!/bin/bash # Inspect the SpokeSpec/Klusterlet types to determine the type of ManagedClusterKubeconfig rg -n -C3 'type\s+SpokeSpec\b|type\s+Klusterlet\b|ManagedClusterKubeconfig' api/ fleetconfig-controller/api/fleetconfig-controller/internal/webhook/v1beta1/validation.go (2)
103-121: Update-path masking looks correct now.You’re deep-copying Specs and masking only the allowed fields. No pointer aliasing; aligns with the “Klusterlet is a value, not a pointer” fact.
Consider adding a unit test that exercises unchanged fields (e.g., HubRef, Klusterlet.Mode) to ensure they are blocked on update.
150-171: Fix name-collision logic and field paths for addon uniqueness.
- Collision check uses AddOnConfig map keyed by "name-version" but compares HubAddOn.Name to that map; clashes are never detected.
- Field paths should include "spec".
Apply:
- // Validate that AddOnConfig names are unique within the AddOnConfigs list - addOnConfigNames := make(map[string]int) + // Validate that AddOnConfig names are unique within the AddOnConfigs list + addOnConfigNames := make(map[string]int) + addOnConfigNameIndex := make(map[string]int) // name -> first index for i, a := range newObject.Spec.AddOnConfigs { key := fmt.Sprintf("%s-%s", a.Name, a.Version) if existingIndex, found := addOnConfigNames[key]; found { - errs = append(errs, field.Invalid(field.NewPath("addOnConfigs").Index(i), key, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("addOnConfigs").Index(i), key, fmt.Sprintf("duplicate addOnConfig %s (name-version) found at indices %d and %d", key, existingIndex, i))) } else { addOnConfigNames[key] = i } + if _, ok := addOnConfigNameIndex[a.Name]; !ok { + addOnConfigNameIndex[a.Name] = i + } } @@ for i, ha := range newObject.Spec.HubAddOns { if existingIndex, found := hubAddOnNames[ha.Name]; found { - errs = append(errs, field.Invalid(field.NewPath("hubAddOns").Index(i), ha.Name, + errs = append(errs, field.Invalid(field.NewPath("spec").Child("hubAddOns").Index(i), ha.Name, fmt.Sprintf("duplicate hubAddOn name %s found at indices %d and %d", ha.Name, existingIndex, i))) } else { hubAddOnNames[ha.Name] = i } } @@ - for i, ha := range newObject.Spec.HubAddOns { - if _, found := addOnConfigNames[ha.Name]; found { - errs = append(errs, field.Invalid(field.NewPath("hubAddOns").Index(i), ha.Name, - fmt.Sprintf("hubAddOn name %s clashes with an existing addOnConfig name.", ha.Name))) - } - } + for i, ha := range newObject.Spec.HubAddOns { + if idx, found := addOnConfigNameIndex[ha.Name]; found { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("hubAddOns").Index(i), ha.Name, + fmt.Sprintf("hubAddOn name %s clashes with addOnConfig name at index %d", ha.Name, idx))) + } + }Also applies to: 173-179
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (1)
44-54: Good: addon client created once and injected.Reuses a long‑lived client instead of per‑request creation; avoids latency in webhooks.
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
arturshadnik
changed the title
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
fleetconfig-controller/api/v1beta1/common.go(1 hunks)fleetconfig-controller/api/v1beta1/hub_types.go(2 hunks)fleetconfig-controller/hack/support-bundle/serve.sh(1 hunks)fleetconfig-controller/internal/args/args.go(1 hunks)fleetconfig-controller/internal/args/args_test.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go(3 hunks)fleetconfig-controller/internal/webhook/v1beta1/validation_test.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- fleetconfig-controller/internal/args/args_test.go
- fleetconfig-controller/internal/args/args.go
- fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go
- fleetconfig-controller/hack/support-bundle/serve.sh
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation_test.gofleetconfig-controller/api/v1beta1/hub_types.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.gofleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-22T18:42:03.404Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/PROJECT:28-31
Timestamp: 2025-09-22T18:42:03.404Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller, the PROJECT file defines multiple API resources with different webhook configurations: FleetConfig v1alpha1 has defaulting: true (requiring MutatingWebhookConfiguration), while Hub and Spoke v1beta1 resources have defaulting: false. MutatingWebhookConfiguration resources in the manifests serve the v1alpha1 FleetConfig, not the v1beta1 Hub/Spoke resources.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation_test.gofleetconfig-controller/api/v1beta1/hub_types.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation_test.gofleetconfig-controller/internal/webhook/v1beta1/hub_webhook.gofleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-22T19:16:34.109Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/webhook/v1beta1/validation.go:103-121
Timestamp: 2025-09-22T19:16:34.109Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller v1beta1 API, the Klusterlet field in SpokeSpec is defined as a struct value (Klusterlet Klusterlet), not a pointer (*Klusterlet), so direct field access like Klusterlet.Annotations is safe without nil checks. The Klusterlet struct does not contain a Source field.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/validation_test.gofleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.gofleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
🧬 Code graph analysis (5)
fleetconfig-controller/internal/webhook/v1beta1/validation_test.go (2)
fleetconfig-controller/api/v1beta1/hub_types.go (1)
HubSpec(26-79)fleetconfig-controller/api/v1beta1/spoke_types.go (2)
SpokeSpec(30-83)HubRef(86-94)
fleetconfig-controller/api/v1beta1/hub_types.go (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (12)
ClusterManager(248-284)Kubeconfig(301-317)SingletonControlPlane(211-222)RegistrationAuth(707-726)AddOnConfig(729-752)HubAddOn(827-841)Helm(225-245)ResourceSpec(639-654)OCMSource(287-298)Condition(161-164)InstalledHubAddOn(505-514)Hub(176-208)fleetconfig-controller/api/v1beta1/common.go (4)
Kubeconfig(13-29)RegistrationAuth(138-157)ResourceSpec(44-59)Condition(126-129)
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (4)
fleetconfig-controller/internal/kube/kube.go (1)
RawFromInClusterRestConfig(75-81)fleetconfig-controller/pkg/common/common.go (1)
AddOnClient(59-69)fleetconfig-controller/api/v1beta1/hub_types.go (1)
Hub(243-257)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
HubGroupKind(32-32)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (9)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(276-290)HubRef(86-94)KlusterletChartConfig(181-183)Klusterlet(102-164)SpokeList(295-299)fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (10)
Spoke(350-392)Kubeconfig(301-317)NewCondition(147-158)Condition(161-164)ClusterManager(248-284)Hub(176-208)KlusterletChartConfig(584-586)Klusterlet(517-580)RegistrationAuth(707-726)OCMSource(287-298)fleetconfig-controller/api/v1beta1/constants.go (9)
SpokeCleanupFinalizer(10-10)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)SpokeRunning(45-45)LabelManagedClusterType(71-71)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)fleetconfig-controller/internal/kube/kube.go (1)
KubeconfigFromSecretOrCluster(113-119)fleetconfig-controller/pkg/common/common.go (6)
ClusterClient(20-30)WorkClient(46-56)AddOnClient(59-69)GetManagedCluster(72-81)UpdateManagedCluster(84-89)OperatorClient(33-43)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/args/args.go (2)
PrepareResources(32-55)PrepareKubeconfig(15-29)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
fleetconfig-controller/api/v1beta1/common.go (3)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (5)
SecretReference(320-333)ResourceValues(657-665)ResourceSpec(639-654)NewCondition(147-158)Condition(161-164)fleetconfig-controller/internal/args/args.go (2)
ResourceValues(65-67)ResourceSpec(58-62)fleetconfig-controller/api/v1beta1/constants.go (1)
ReconcileSuccess(30-30)
🔇 Additional comments (15)
fleetconfig-controller/internal/webhook/v1beta1/validation_test.go (1)
9-68: Solid coverage and clear messagesThe table tests exercise kubeconfig one‑of, hub/spoke allowed vs disallowed updates, and assert exact messages. Looks good.
If validation.go messages change, these tests will need sync. Consider running the suite once you adjust any error strings.
Also applies to: 70-298, 300-538
fleetconfig-controller/api/v1beta1/hub_types.go (2)
50-53: Heads‑up: +required on spec.kubeconfig may break existing manifests/testsMarking Kubeconfig required tightens the contract. If any Hub literals/YAML omit it, admission or CRD validation will fail.
Do you intend to enforce this now? If not, consider making it optional (pointer + omitempty) or update all callers before merging.
276-284: Fix address-of-range-variable bug in GetConditionReturning &c yields a pointer to the loop variable, not the slice element.
func (s *HubStatus) GetCondition(cType string) *Condition { - for _, c := range s.Conditions { - if c.Type == cType { - return &c - } - } + for i := range s.Conditions { + if s.Conditions[i].Type == cType { + return &s.Conditions[i] + } + } return nil }fleetconfig-controller/api/v1beta1/common.go (1)
72-82: Avoid panic on typed‑nil: make ResourceValues.String a value receiver and assert value typeCalling String via an interface that holds a typed‑nil *ResourceValues will dereference nil and panic. Use a value receiver; also assert the value types implement the interfaces.
-// String returns a string representation of the resource values. -func (r *ResourceValues) String() string { +// String returns a string representation of the resource values. +func (r ResourceValues) String() string { if r.CPU != "" && r.Memory != "" { return fmt.Sprintf("cpu=%s,memory=%s", r.CPU, r.Memory) } else if r.CPU != "" { return fmt.Sprintf("cpu=%s", r.CPU) } else if r.Memory != "" { return fmt.Sprintf("memory=%s", r.Memory) } return "" } @@ -// Ensure ResourceSpec implements args.ResourceSpec interface -var _ args.ResourceSpec = (*ResourceSpec)(nil) +// Ensure ResourceSpec implements args.ResourceSpec interface +var _ args.ResourceSpec = ResourceSpec{} -// Ensure ResourceValues implements args.ResourceValues interface -var _ args.ResourceValues = (*ResourceValues)(nil) +// Ensure ResourceValues implements args.ResourceValues interface +var _ args.ResourceValues = ResourceValues{}Also applies to: 105-110
fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (1)
84-99: Use correct field paths in Invalid errors (spec/… instead of hub)Error paths should point at spec and spec.kubeconfig to render properly in kubectl and clients. Update messages to reference spec.* consistently.
- if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec.Kubeconfig, msg), - ) - } + if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), hub.Spec.Kubeconfig, msg), + ) + } if hub.Spec.ClusterManager == nil && hub.Spec.SingletonControlPlane == nil { allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec, "either hub.clusterManager or hub.singletonControlPlane must be specified"), + field.NewPath("spec"), hub.Spec, "either spec.clusterManager or spec.singletonControlPlane must be specified"), ) } if hub.Spec.ClusterManager != nil && hub.Spec.SingletonControlPlane != nil { allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec, "only one of hub.clusterManager or hub.singletonControlPlane may be specified"), + field.NewPath("spec"), hub.Spec, "only one of spec.clusterManager or spec.singletonControlPlane may be specified"), ) } @@ - if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { - allErrs = append(allErrs, field.Invalid( - field.NewPath("hub"), hub.Spec.Kubeconfig, msg), - ) - } + if valid, msg := isKubeconfigValid(hub.Spec.Kubeconfig); !valid { + allErrs = append(allErrs, field.Invalid( + field.NewPath("spec").Child("kubeconfig"), hub.Spec.Kubeconfig, msg), + ) + }Also applies to: 127-131
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (10)
328-332: LGTM: nil klusterletValues is a valid, handled stateDownstream functions handle nil (prepareKlusterletValuesFile returns early; hash.ComputeHash uses ZeroNil).
405-416: Fix nil deref when Hub.ClusterManager is omittedGuard ClusterManager before accessing Source to avoid panic.
- if hub != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { + if hub != nil && hub.Spec.ClusterManager != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source, spokeKubeconfig) if err != nil { return fmt.Errorf("failed to check if spoke cluster needs upgrade: %w", err) } if upgrade { - if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source, spokeKubeconfig); err != nil { + if err := r.upgradeSpoke(ctx, spoke, klusterletValues, hub.Spec.ClusterManager.Source, spokeKubeconfig); err != nil { return fmt.Errorf("failed to upgrade spoke cluster %s: %w", spoke.Name, err) } } }
451-459: Join path: guard Hub.ClusterManager before using Source fieldsAvoid nil deref in joinSpoke when hub.Spec.ClusterManager is nil.
if hub == nil { return errors.New("hub not found") } // dont start join until the hub is ready hubInitCond := hubMeta.hub.GetCondition(v1beta1.HubInitialized) if hubInitCond == nil || hubInitCond.Status != metav1.ConditionTrue { return errors.New("hub does not have initialized condition") } + if hub.Spec.ClusterManager == nil { + return fmt.Errorf("hub %s has no clusterManager spec; cannot determine bundle version/registry", hub.Name) + } @@ // source args - "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, - "--image-registry", hub.Spec.ClusterManager.Source.Registry, + "--bundle-version", hub.Spec.ClusterManager.Source.BundleVersion, + "--image-registry", hub.Spec.ClusterManager.Source.Registry,Also applies to: 478-480
342-349: Run ‘clusteradm accept’ against the Hub (inject hub kubeconfig)Accept must target the Hub, not the Spoke. Update call to pass hubMeta.
- if err := acceptCluster(ctx, spoke, false); err != nil { + if err := acceptCluster(ctx, hubMeta, spoke, false); err != nil { spoke.SetConditions(true, v1beta1.NewCondition( err.Error(), v1beta1.SpokeJoined, metav1.ConditionFalse, metav1.ConditionTrue, )) return err }
365-369: Re-accept join requests on the HubSame fix here to avoid running against the Spoke.
- if err := acceptCluster(ctx, spoke, true); err != nil { + if err := acceptCluster(ctx, hubMeta, spoke, true); err != nil { logger.Error(err, "failed to accept spoke cluster join request(s)", "spoke", spoke.Name) }
585-613: Refactor acceptCluster to use Hub kubeconfigChange signature and use hubMeta’s kubeconfig/context.
-// acceptCluster accepts a Spoke cluster's join request -func acceptCluster(ctx context.Context, spoke *v1beta1.Spoke, skipApproveCheck bool) error { +// acceptCluster accepts a Spoke cluster's join request on the Hub +func acceptCluster(ctx context.Context, hubMeta hubMeta, spoke *v1beta1.Spoke, skipApproveCheck bool) error { logger := log.FromContext(ctx) logger.V(0).Info("acceptCluster", "spoke", spoke.Name) - acceptArgs := append([]string{ - "accept", "--cluster", spoke.Name, - }, spoke.BaseArgs()...) + acceptArgs := []string{"accept", "--cluster", spoke.Name} + // run against the Hub + var cleanupKcfg func() + var err error + acceptArgs, cleanupKcfg, err = args.PrepareKubeconfig(ctx, hubMeta.kubeconfig, hubMeta.hub.Spec.Kubeconfig.Context, acceptArgs) + if cleanupKcfg != nil { + defer cleanupKcfg() + } + if err != nil { + return fmt.Errorf("failed to prepare hub kubeconfig: %w", err) + } logger.V(1).Info("clusteradm accept", "args", acceptArgs)
572-573: Redact hub token from logsDo not log secrets in args.
- logger.V(1).Info("clusteradm join", "args", joinArgs) + // redact token for logs + logArgs := append([]string{}, joinArgs...) + for i := 0; i < len(logArgs)-1; i++ { + if logArgs[i] == "--hub-token" { + logArgs[i+1] = "REDACTED" + } + } + logger.V(1).Info("clusteradm join", "args", logArgs)
788-797: Remove token from logsgetToken logs the JSON output containing the token. Redact.
- logger.V(1).Info("got join token", "output", string(stdout)) + logger.V(1).Info("got join token") // redact sensitive output
621-625: Return pointer to slice element, not range variableFix stale-pointer bug.
- for _, c := range managedCluster.Status.Conditions { - if c.Type == "ManagedClusterJoined" { - return &c - } - } + for i := range managedCluster.Status.Conditions { + if managedCluster.Status.Conditions[i].Type == "ManagedClusterJoined" { + return &managedCluster.Status.Conditions[i] + } + }
71-74: Add missing RBAC for resources used by this controllerRequired for get/list/watch/delete operations invoked via controller-runtime client.
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,verbs=get;list;watch;delete +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;delete
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fleetconfig-controller/Makefile (2)
120-120: Default is fine; consider stripping whitespace.If LABEL_FILTER is set to whitespace, it will be treated as non-empty. Strip to ensure fallback works.
-LABEL_FILTER ?= v1beta1 +LABEL_FILTER ?= v1beta1(No change needed here; change applied at flag line below.)
129-129: Harden fallback when LABEL_FILTER is whitespace.Use $(strip ...) so whitespace-only values fall back to v1beta1.
- --label-filter="$(if $(LABEL_FILTER),$(LABEL_FILTER),v1beta1)" \ + --label-filter="$(if $(strip $(LABEL_FILTER)),$(LABEL_FILTER),v1beta1)" \fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
304-316: Short‑circuit handleSpoke when Hub is absent to avoid building clients with nil kubeconfigAvoid churn and unnecessary Unhealthy flips when Hub isn’t available yet.
Apply:
func (r *SpokeReconciler) handleSpoke(ctx context.Context, spoke *v1beta1.Spoke, hubMeta hubMeta, spokeKubeconfig []byte) error { logger := log.FromContext(ctx) logger.V(0).Info("handleSpoke", "spoke", spoke.Name) hub := hubMeta.hub hubKubeconfig := hubMeta.kubeconfig + + if hub == nil || len(hubKubeconfig) == 0 { + msg := "hub metadata unavailable; skipping join/upgrade" + logger.V(0).Info(msg, "spoke", spoke.Name) + spoke.SetConditions(true, v1beta1.NewCondition( + msg, v1beta1.SpokeJoined, metav1.ConditionFalse, metav1.ConditionTrue, + )) + return nil + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/e2e.yml(2 hunks)fleetconfig-controller/Makefile(2 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
📚 Learning: 2025-08-22T19:38:49.769Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#52
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go:49-56
Timestamp: 2025-08-22T19:38:49.769Z
Learning: In the fleetconfig-controller project, the SpokeSpec and HubSpec structs in v1beta1 contain only optional fields (like Foo *string with omitempty tags), so creating these resources without populating the Spec field does not cause validation failures in tests.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early with (nil, nil, nil), making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream prepareKlusterletValuesFile function properly handles nil values by returning early, making additional nil checks unnecessary and potentially harmful to the intended flow.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions handle nil properly: prepareKlusterletValuesFile returns early with (nil, nil, nil) for nil values, and hash.ComputeHash uses ZeroNil: true option to treat nil as zero values when computing hashes.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-12T22:46:57.106Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:328-331
Timestamp: 2025-09-12T22:46:57.106Z
Learning: In fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go, nil klusterletValues returned by mergeKlusterletValues is a valid state, not an error condition. The downstream functions (prepareKlusterletValuesFile and hash.ComputeHash) properly handle nil values - prepareKlusterletValuesFile returns early with (nil, nil, nil) and hash.ComputeHash uses ZeroNil: true option to handle nil as zero values.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
📚 Learning: 2025-09-22T19:16:34.109Z
Learnt from: arturshadnik
PR: open-cluster-management-io/lab#59
File: fleetconfig-controller/internal/webhook/v1beta1/validation.go:103-121
Timestamp: 2025-09-22T19:16:34.109Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller v1beta1 API, the Klusterlet field in SpokeSpec is defined as a struct value (Klusterlet Klusterlet), not a pointer (*Klusterlet), so direct field access like Klusterlet.Annotations is safe without nil checks. The Klusterlet struct does not contain a Source field.
Applied to files:
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
🧬 Code graph analysis (1)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (8)
fleetconfig-controller/api/v1beta1/spoke_types.go (5)
Spoke(276-290)HubRef(86-94)KlusterletChartConfig(181-183)Klusterlet(102-164)SpokeList(295-299)fleetconfig-controller/api/v1beta1/constants.go (12)
Unhealthy(48-48)SpokeCleanupFinalizer(10-10)Deleting(51-51)CleanupFailed(22-22)SpokeJoining(42-42)SpokeJoined(25-25)AddonsConfigured(19-19)SpokeRunning(45-45)LabelManagedClusterType(71-71)ManagedClusterTypeHubAsSpoke(65-65)HubInitialized(16-16)AWSIRSARegistrationDriver(83-83)fleetconfig-controller/internal/kube/kube.go (1)
KubeconfigFromSecretOrCluster(113-119)fleetconfig-controller/pkg/common/common.go (6)
ClusterClient(20-30)WorkClient(46-56)AddOnClient(59-69)GetManagedCluster(72-81)UpdateManagedCluster(84-89)OperatorClient(33-43)fleetconfig-controller/internal/hash/hash.go (1)
ComputeHash(11-22)fleetconfig-controller/internal/args/args.go (2)
PrepareResources(32-55)PrepareKubeconfig(15-29)fleetconfig-controller/internal/exec/exec.go (1)
CmdWithLogs(18-54)fleetconfig-controller/internal/version/version.go (2)
LowestBundleVersion(15-46)Normalize(49-55)
⏰ 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). (1)
- GitHub Check: e2e (fleetconfig-controller) / e2e
🔇 Additional comments (11)
.github/workflows/e2e.yml (2)
12-14: Input name looks consistent.
ginkgoLabelFilteris declared correctly and matches usage below.Confirm planner.yml outputs the same input name (ginkgoLabelFilter) to avoid drift.
39-40: Quote repo path and label filter to prevent shell parsing errors.Unquoted cd path can break on spaces. Unquoted label filters containing ||, &&, (), or spaces will be parsed by the shell and break the step.
Apply:
- cd ${{ inputs.repo }} - LABEL_FILTER=${{ inputs.ginkgoLabelFilter }} make test-e2e + cd "${{ inputs.repo }}" + LABEL_FILTER="${{ inputs.ginkgoLabelFilter }}" make test-e2efleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (9)
884-886: LGTM: correct yaml.Unmarshal targetPassing the pointer (not pointer-to-pointer) is correct.
894-903: LGTM: nil/empty Klusterlet values handled gracefullyEarly return avoids unnecessary temp files and aligns with downstream handling.
621-625: Bug: returning pointer to range variableReturns address of loop variable copy. Use index-based iteration.
Apply:
- for _, c := range managedCluster.Status.Conditions { - if c.Type == "ManagedClusterJoined" { - return &c - } - } + for i := range managedCluster.Status.Conditions { + if managedCluster.Status.Conditions[i].Type == "ManagedClusterJoined" { + return &managedCluster.Status.Conditions[i] + } + }
572-581: Redact token from logs and avoid logging command outputjoinArgs contains --hub-token; stdout may contain sensitive info.
Apply:
- logger.V(1).Info("clusteradm join", "args", joinArgs) + // redact token for logs + logArgs := append([]string(nil), joinArgs...) + for i := 0; i < len(logArgs)-1; i++ { + if logArgs[i] == "--hub-token" { + logArgs[i+1] = "REDACTED" + } + } + logger.V(1).Info("clusteradm join", "args", logArgs) @@ - logger.V(1).Info("successfully requested spoke cluster join", "output", string(stdout)) + logger.V(1).Info("successfully requested spoke cluster join")
71-74: Add missing RBAC for Hubs, ConfigMaps, CSRs, and NamespacesRequired for hub watch, ConfigMap reads, CSR deletions, and Namespace deletions used by this controller. Without these, reconcile paths will fail with RBAC Forbidden.
Apply:
// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=spokes/finalizers,verbs=update +// +kubebuilder:rbac:groups=fleetconfig.open-cluster-management.io,resources=hubs,verbs=get;list;watch +// +kubebuilder:rbac:groups="",resources=configmaps,verbs=get +// +kubebuilder:rbac:groups=certificates.k8s.io,resources=certificatesigningrequests,verbs=get;list;watch;delete +// +kubebuilder:rbac:groups="",resources=namespaces,verbs=get;list;watch;delete
954-959: Nil-deref risk in sharedFieldsChanged (ClusterManager optional)Handle nil before dereferencing .Source.
Apply:
-func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { - return !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) || - !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) -} +func sharedFieldsChanged(oldSpec, newSpec *v1beta1.HubSpec) bool { + if !reflect.DeepEqual(oldSpec.RegistrationAuth, newSpec.RegistrationAuth) { + return true + } + if (oldSpec.ClusterManager == nil) != (newSpec.ClusterManager == nil) { + return true + } + if oldSpec.ClusterManager != nil && newSpec.ClusterManager != nil && + !reflect.DeepEqual(oldSpec.ClusterManager.Source, newSpec.ClusterManager.Source) { + return true + } + return false +}
788-797: Do not log the join tokenThis logs the secret token. Remove it.
Apply:
- logger.V(1).Info("got join token", "output", string(stdout)) + logger.V(1).Info("got join token")
405-416: Nil-deref risk: guard Hub.ClusterManager before dereferencing Source in upgrade pathHub.ClusterManager is optional; current access may panic.
Apply:
- if hub != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { + if hub != nil && hub.Spec.ClusterManager != nil && hub.Spec.ClusterManager.Source.BundleVersion != "" { upgrade, err := r.spokeNeedsUpgrade(ctx, spoke, currKlusterletHash, hub.Spec.ClusterManager.Source, spokeKubeconfig)
476-481: Nil-deref risk: ensure Hub.ClusterManager is set before using Source in joinBlock join with a clear error when ClusterManager is not configured.
Apply:
hub := hubMeta.hub @@ tokenMeta, err := getToken(ctx, hubMeta) if err != nil { return fmt.Errorf("failed to get join token: %w", err) } + if hub.Spec.ClusterManager == nil { + return fmt.Errorf("hub %s has no clusterManager spec; cannot determine bundle version/registry", hub.Name) + } + joinArgs := append([]string{
TylerGillson
approved these changes
Sep 24, 2025
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arturshadnik, TylerGillson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 8b2e21d
into
open-cluster-management-io:main
9 checks passed
This was referenced Oct 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores