✨ scaffold fleetconfig v1beta1 resources and webhooks #52
Conversation
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
WalkthroughAdds v1beta1 Hub and Spoke APIs (types, CRDs, deepcopies), controllers, and webhooks; updates v1alpha1 metadata and package layout; moves generated CRDs to charts/, removes kustomize CRD/webhook patches and sample manifests; adds tests and test utilities; updates RBAC and main wiring for multi-version support. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
fleetconfig-controller/internal/controller/v1alpha1/spoke.go (1)
423-472: spokeNeedsUpgrade returns the wrong boolean; upgrades will trigger when already up-to-date.
The function currently returns true when active bundle version equals desired, which inverts the intent and can cause unnecessary upgrades every reconcile.func spokeNeedsUpgrade(ctx context.Context, kClient client.Client, spoke v1alpha1.Spoke) (bool, error) { @@ - return activeBundleVersion == spoke.Klusterlet.Source.BundleVersion, nil + // Upgrade is needed only when the active version differs from the desired. + return activeBundleVersion != spoke.Klusterlet.Source.BundleVersion, nil }Follow-up: consider adding a focused unit test that covers default/latest/explicit version scenarios to prevent regressions.
Would you like me to draft a table-driven test for this function?
fleetconfig-controller/internal/controller/v1alpha1/addon.go (1)
252-272: Nil-pointer deref when AddOnTemplate is NotFound in handleAddonDelete.
When Get() returns NotFound, addon is nil but code dereferences addon.Spec.AddonName, leading to a panic.for _, addonName := range addons { // get the addon template, so we can extract spec.addonName - addon, err := addonC.AddonV1alpha1().AddOnTemplates().Get(ctx, addonName, metav1.GetOptions{}) - if err != nil && !kerrs.IsNotFound(err) { - errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) - continue - } - - // delete the addon template - if addon != nil { - err = addonC.AddonV1alpha1().AddOnTemplates().Delete(ctx, addonName, metav1.DeleteOptions{}) - if err != nil && !kerrs.IsNotFound(err) { - errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) - continue - } - } - - baseAddonName := addon.Spec.AddonName + addon, err := addonC.AddonV1alpha1().AddOnTemplates().Get(ctx, addonName, metav1.GetOptions{}) + var baseAddonName string + switch { + case err == nil: + // derive base name from the object then delete it + baseAddonName = addon.Spec.AddonName + if err := addonC.AddonV1alpha1().AddOnTemplates().Delete(ctx, addonName, metav1.DeleteOptions{}); err != nil && !kerrs.IsNotFound(err) { + errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) + continue + } + case kerrs.IsNotFound(err): + // best-effort: infer base name by trimming the last "-<version>" segment + if idx := strings.LastIndex(addonName, "-"); idx > 0 { + baseAddonName = addonName[:idx] + } else { + // cannot infer; skip purge for this entry + logger := log.FromContext(ctx) + logger.V(2).Info("could not infer base addon name from versioned name; skipping purge check", "addonName", addonName) + continue + } + default: + errs = append(errs, fmt.Errorf("failed to delete addon %s: %v", addonName, err)) + continue + } // get the addon name without a version suffix, add it to purge list purgeList = append(purgeList, baseAddonName) logger.V(0).Info("deleted addon", "AddOnTemplate", addonName) }fleetconfig-controller/internal/controller/v1alpha1/hub.go (2)
209-252: Fix hubNeedsUpgrade logic and add nil guards to avoid panics and unintended upgrades.Current code returns true when versions match and dereferences
cmwithout checking nil. This can:
- Trigger upgrades when not needed.
- Panic if ClusterManager is absent (e.g., singleton control plane path).
func hubNeedsUpgrade(ctx context.Context, fc *v1alpha1.FleetConfig, operatorC *operatorapi.Clientset) (bool, error) { logger := log.FromContext(ctx) logger.V(0).Info("hubNeedsUpgrade", "fleetconfig", fc.Name) + // If ClusterManager is not configured (singleton control plane), skip upgrades. + if fc.Spec.Hub.ClusterManager == nil { + logger.V(0).Info("no ClusterManager configured; skipping hub upgrade check") + return false, nil + } + if fc.Spec.Hub.ClusterManager.Source.BundleVersion == "default" { logger.V(0).Info("clustermanager bundleVersion is default, skipping upgrade") return false, nil } if fc.Spec.Hub.ClusterManager.Source.BundleVersion == "latest" { logger.V(0).Info("clustermanager bundleVersion is latest, attempting upgrade") return true, nil } cm, err := getClusterManager(ctx, operatorC) if err != nil { return false, err } + if cm == nil { + // Hub might not be initialized yet or singleton path in use; nothing to compare. + logger.V(0).Info("ClusterManager not found; skipping hub upgrade check") + return false, nil + } // identify lowest bundleVersion referenced in the clustermanager spec bundleSpecs := make([]string, 0) if cm.Spec.AddOnManagerImagePullSpec != "" { bundleSpecs = append(bundleSpecs, cm.Spec.AddOnManagerImagePullSpec) } if cm.Spec.PlacementImagePullSpec != "" { bundleSpecs = append(bundleSpecs, cm.Spec.PlacementImagePullSpec) } if cm.Spec.RegistrationImagePullSpec != "" { bundleSpecs = append(bundleSpecs, cm.Spec.RegistrationImagePullSpec) } if cm.Spec.WorkImagePullSpec != "" { bundleSpecs = append(bundleSpecs, cm.Spec.WorkImagePullSpec) } + if len(bundleSpecs) == 0 { + logger.V(0).Info("no image pull specs found on clustermanager; skipping hub upgrade check") + return false, nil + } activeBundleVersion, err := version.LowestBundleVersion(ctx, bundleSpecs) if err != nil { return false, fmt.Errorf("failed to detect bundleVersion from clustermanager spec: %w", err) } logger.V(0).Info("found clustermanager bundleVersions", "activeBundleVersion", activeBundleVersion, "desiredBundleVersion", fc.Spec.Hub.ClusterManager.Source.BundleVersion, ) - return activeBundleVersion == fc.Spec.Hub.ClusterManager.Source.BundleVersion, nil + // Upgrade is needed when active != desired. + return activeBundleVersion != fc.Spec.Hub.ClusterManager.Source.BundleVersion, nil }
375-419: Guard against deleting critical namespaces during cleanup.If a user misconfigures a Spoke name as a system namespace (e.g., “default”), this will attempt deletion. Add a denylist for well-known namespaces.
func cleanNamespaces(ctx context.Context, kClient client.Client, fc *v1alpha1.FleetConfig) error { logger := log.FromContext(ctx) logger.V(0).Info("cleanNamespaces", "fleetconfig", fc.Name) @@ for _, spoke := range fc.Spec.Spokes { - ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: spoke.Name}} + switch spoke.Name { + case "kube-system", "kube-public", "kube-node-lease", "default": + logger.Info("skipping deletion of reserved namespace", "namespace", spoke.Name) + continue + } + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: spoke.Name}} if err := kClient.Delete(ctx, ns, deleteOpts); err != nil && !kerrs.IsNotFound(err) { return err } logger.Info("deleted spoke namespace", "spokeNamespace", spoke.Name) namespaces = append(namespaces, spoke.Name) }fleetconfig-controller/config/certmanager/certificate-webhook.yaml (1)
1-21: Add cert-manager CA bundle injection to your webhook configurationsOur grep run shows no
cert-manager.io/inject-ca-fromannotation or explicitcaBundlein your webhook manifests. Without it, Kubernetes will reject TLS connections to your self-signed webhooks. Please update your MutatingWebhookConfiguration and ValidatingWebhookConfiguration in:
fleetconfig-controller/config/webhook/manifests.yamlFor each
webhookentry, undermetadata.annotations, add:metadata: annotations: cert-manager.io/inject-ca-from: "system/serving-cert"Ensure
"system/serving-cert"matches the namespace/name of the Certificate defined infleetconfig-controller/config/certmanager/certificate-webhook.yaml. This will enable cert-manager to inject the CA bundle automatically.fleetconfig-controller/config/rbac/hub_viewer_role.yaml (1)
1-30: Wrong API group in viewer role; should be fleetconfig.open-cluster-management.io.The Hub CRD group is fleetconfig.open-cluster-management.io. Using open-cluster-management.io will not grant access to Hub resources.
-# Grants read-only access to open-cluster-management.io resources. +# Grants read-only access to fleetconfig.open-cluster-management.io resources. @@ - apiGroups: - - open-cluster-management.io + - fleetconfig.open-cluster-management.io resources: - hubs @@ - apiGroups: - - open-cluster-management.io + - fleetconfig.open-cluster-management.io resources: - hubs/status verbs: - getOptionally, aggregate this role into the default “view” ClusterRole:
metadata: labels: app.kubernetes.io/name: fleetconfig-controller app.kubernetes.io/managed-by: kustomize + rbac.authorization.k8s.io/aggregate-to-view: "true"fleetconfig-controller/cmd/main.go (2)
65-69: Drop unuseduseWebhooktoggle and avoid dual gating mechanisms.You’re gating FleetConfig webhooks via a flag while Hub/Spoke webhooks use ENABLE_WEBHOOKS. This inconsistency will surprise operators and can lead to only some webhooks being registered.
- useWebhook bool certDir string webhookPort int
77-80: Unify webhook gating and set sane defaults for cert dir and port.
- Remove
--use-webhook; consistently use ENABLE_WEBHOOKS (as you already do for v1beta1).- Default cert dir and port to the controller-runtime defaults used by kubebuilder manifests (9443 + /tmp/...).
- flag.BoolVar(&useWebhook, "use-webhook", useWebhook, "Enable admission webhooks") - flag.StringVar(&certDir, "webhook-cert-dir", certDir, "Admission webhook cert/key dir") - flag.IntVar(&webhookPort, "webhook-port", webhookPort, "Admission webhook port") + flag.StringVar(&certDir, "webhook-cert-dir", "/tmp/k8s-webhook-server/serving-certs", "Admission webhook cert/key dir") + flag.IntVar(&webhookPort, "webhook-port", 9443, "Admission webhook port")
♻️ Duplicate comments (2)
fleetconfig-controller/api/v1alpha1/fleetconfig_types.go (2)
171-173: Good safeguard to prevent v1alpha1 Hub CRD generationAdding +kubebuilder:object:root=false and +kubebuilder:skipversion on the v1alpha1 Hub avoids accidental CRD emission for this package when v1beta1 introduces a root Hub with the same name. This aligns with the intent captured in the prior discussion.
Run to confirm there’s no v1alpha1 Hub CRD generated:
#!/bin/bash # Expect: No "v1alpha1" under Hub CRD bases rg -n "kind:\s*CustomResourceDefinition" -n config/crd/bases | rg hubs.yaml -n rg -n "versions:\n\s*-\s*name:\s*v1alpha1" config/crd/bases/fleetconfig.open-cluster-management.io_hubs.yaml || echo "OK: no v1alpha1 in Hub CRD"
345-347: Same rationale for Spoke — avoid duplicate v1alpha1 CRDThe non-root + skipversion markers on v1alpha1 Spoke are correct to prevent duplicate CRD/version surfacing alongside the v1beta1 Spoke root.
Verify there’s no v1alpha1 Spoke CRD generated:
#!/bin/bash # Expect: No "v1alpha1" under Spoke CRD bases rg -n "kind:\s*CustomResourceDefinition" -n config/crd/bases | rg spokes.yaml -n rg -n "versions:\n\s*-\s*name:\s*v1alpha1" config/crd/bases/fleetconfig.open-cluster-management.io_spokes.yaml || echo "OK: no v1alpha1 in Spoke CRD"
🧹 Nitpick comments (67)
fleetconfig-controller/internal/controller/v1alpha1/fleetconfig_controller_test.go (2)
75-83: Avoid index access on finalizers; assert presence instead.
Indexing assumes at least one finalizer at position 0. Assert containment to prevent flaky panics if order changes.- Expect(fc.Finalizers[0]).To(Equal(v1alpha1.FleetConfigFinalizer), - "FleetConfig %s wasn't given a finalizer", nn.Name) + Expect(fc.Finalizers).To(ContainElement(v1alpha1.FleetConfigFinalizer), + "FleetConfig %s wasn't given a finalizer", nn.Name)
104-111: Tighten Eventually timeout to speed up the suite.
Five minutes is heavy for unit/envtest. Unless you’ve observed flakes, 1–2 minutes with a small polling interval is typically sufficient.- }, 5*time.Minute).Should(Succeed()) + }, 2*time.Minute, 2*time.Second).Should(Succeed())fleetconfig-controller/internal/controller/v1alpha1/fleetconfig_controller.go (1)
171-183: Log the actual error in ret() to aid debugging.
Currently logs only a generic message; include the error with logger.Error.func ret(ctx context.Context, res ctrl.Result, err error) (ctrl.Result, error) { logger := log.FromContext(ctx) if res.RequeueAfter > 0 { logger.Info("requeueing", "after", res.RequeueAfter) } - if err != nil { - logger.Info("requeueing due to error") - } + if err != nil { + logger.Error(err, "requeueing due to error") + } if res.RequeueAfter == 0 && err == nil { logger.Info("reconciliation complete; no requeue or error") } return res, err }fleetconfig-controller/config/certmanager/kustomizeconfig.yaml (1)
1-8: Optionally support ClusterIssuer as well.
If any Certificate uses issuerRef.kind=ClusterIssuer, add a nameReference to keep those names updated too.nameReference: - kind: Issuer group: cert-manager.io fieldSpecs: - kind: Certificate group: cert-manager.io path: spec/issuerRef/name + - kind: ClusterIssuer + group: cert-manager.io + fieldSpecs: + - kind: Certificate + group: cert-manager.io + path: spec/issuerRef/namefleetconfig-controller/internal/controller/v1alpha1/addon.go (3)
145-155: Avoid shadowing the net/url import with a local variable.
Renaming the local to u improves readability and prevents accidental confusion with the package name.- url, err := url.Parse(manifestsURL) + u, err := url.Parse(manifestsURL) if err != nil { return errors.Wrap(err, fmt.Sprintf("failed to create addon %s version %s", a.Name, a.Version)) } - switch url.Scheme { + switch u.Scheme { case "http", "https":
406-415: Only pass --annotate when annotations are present.
Clusteradm may reject an empty value. Guard the flag.- var annots []string - for k, v := range a.Annotations { - annots = append(annots, fmt.Sprintf("%s=%s", k, v)) - } - annot := strings.Join(annots, ",") - args = append(args, fmt.Sprintf("--annotate=%s", annot)) + if len(a.Annotations) > 0 { + var annots []string + for k, v := range a.Annotations { + annots = append(annots, fmt.Sprintf("%s=%s", k, v)) + } + args = append(args, fmt.Sprintf("--annotate=%s", strings.Join(annots, ","))) + }
641-652: Pass hub kubeconfig args to install hub-addon for consistency.
Other clusteradm invocations include fc.BaseArgs(); adding them here ensures we always target the intended hub.args := []string{ install, hubAddon, fmt.Sprintf("--names=%s", addon.Name), fmt.Sprintf("--bundle-version=%s", bundleVersion), fmt.Sprintf("--create-namespace=%t", addon.CreateNamespace), } if addon.InstallNamespace != "" { args = append(args, fmt.Sprintf("--namespace=%s", addon.InstallNamespace)) } + // Ensure we talk to the desired hub cluster. + args = append(args, fc.BaseArgs()...)Note: uninstall path doesn’t currently accept fc; if desired, we can thread fc through handleHubAddonUninstall similarly.
Happy to wire this through and update call sites if you want it in this PR.
fleetconfig-controller/config/webhook/kustomizeconfig.yaml (1)
1-22: Kustomize nameReference/namespace mappings for webhook Service look correctMappings target Mutating/ValidatingWebhookConfiguration service name/namespace paths and are compatible with v1. Nothing blocking.
If you later add CRD conversion webhooks, extend this with CRD conversion paths:
nameReference: - kind: Service version: v1 fieldSpecs: - kind: MutatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/name - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/name + - kind: CustomResourceDefinition + group: apiextensions.k8s.io + path: spec/conversion/webhook/clientConfig/service/name namespace: - kind: MutatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/namespace create: true - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/namespace create: true + - kind: CustomResourceDefinition + group: apiextensions.k8s.io + path: spec/conversion/webhook/clientConfig/service/namespace + create: truefleetconfig-controller/config/crd/kustomization.yaml (2)
11-11: Add trailing newline to satisfy lintersYAMLlint flags a missing newline at EOF. Add one to quiet the CI noise.
Apply:
-configurations: -- kustomizeconfig.yaml +configurations: +- kustomizeconfig.yaml +
3-4: Missing Hub/Spoke webhook & CA injection patchesIt appears you’ve added the Hub/Spoke CRD bases in
fleetconfig-controller/config/crd/kustomization.yamlbut haven’t included corresponding patches for webhook conversion or CA injection. To maintain parity with the FleetConfig CRD, please add and reference these four patch files:• patches/webhook_in_hubs.yaml
• patches/cainjection_in_hubs.yaml
• patches/webhook_in_spokes.yaml
• patches/cainjection_in_spokes.yamlProposed update to
kustomization.yaml:resources: - bases/fleetconfig.open-cluster-management.io_fleetconfigs.yaml - bases/fleetconfig.open-cluster-management.io_hubs.yaml - bases/fleetconfig.open-cluster-management.io_spokes.yaml patches: - path: patches/webhook_in_fleetconfigs.yaml - path: patches/cainjection_in_fleetconfigs.yaml +- path: patches/webhook_in_hubs.yaml +- path: patches/cainjection_in_hubs.yaml +- path: patches/webhook_in_spokes.yaml +- path: patches/cainjection_in_spokes.yamlPlease add these files (or confirm if they’re intentionally omitted) so that Hub and Spoke CRDs receive the same CA injection and webhook conversion handling as FleetConfig.
fleetconfig-controller/config/samples/v1beta1_spoke.yaml (1)
1-9: Consider including a minimal, valid spec to make the sample directlykubectl apply-ableIf SpokeSpec currently has an optional
foo(per scaffolding), adding it here helps users:Apply:
spec: - # TODO(user): Add fields here + # Example field; adjust to your API + foo: "example"fleetconfig-controller/config/rbac/role.yaml (1)
7-19: Optionally include deletecollection to ease bulk cleanupSome controllers rely on delete-collection for garbage collection or test teardown. Not mandatory, but commonly granted.
Proposed change:
verbs: - create - delete + - deletecollection - get - list - patch - update - watchfleetconfig-controller/config/samples/v1beta1_hub.yaml (1)
1-9: Sample is fine; consider adding a concrete field to demonstrate the APISame note as Spoke: include a minimal
specexample so users can apply it as-is and see reconciliation.Suggested tweak:
spec: - # TODO(user): Add fields here + # Example field; adjust to your API + foo: "example"fleetconfig-controller/config/network-policy/allow-webhook-traffic.yaml (1)
1-3: Clarify the comments to avoid confusion between source and target namespacesThe comment suggests CRs “will only work when applied in namespaces labeled with ‘webhook: enabled’.” Admission calls come from kube-apiserver, not from the CR’s namespace. Rephrase to indicate the label controls which source namespaces’ pods may open TCP connections to the webhook server.
fleetconfig-controller/config/webhook/service.yaml (1)
10-13: Name the port and declare appProtocol for clarity and compatibility.K8s best practice for webhook Services is to name the HTTPS port. Some CNIs/NPs and tools key off the port name/protocol. Add a name and appProtocol to make intent explicit.
spec: ports: - - port: 443 - protocol: TCP - targetPort: 9443 + - name: https + port: 443 + protocol: TCP + targetPort: 9443 + appProtocol: httpsfleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go (2)
60-68: Make cleanup resilient to NotFound to avoid flaky tests.If a prior deletion completed or the object was never created due to a failed BeforeEach, this will fail. Ignore not-found on Get/Delete.
AfterEach(func() { // TODO(user): Cleanup logic after each test, like removing the resource instance. resource := &v1beta1.Spoke{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) - Expect(err).NotTo(HaveOccurred()) - - By("Cleanup the specific resource instance Spoke") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + err := k8sClient.Get(ctx, typeNamespacedName, resource) + if errors.IsNotFound(err) { + return + } + Expect(err).NotTo(HaveOccurred()) + By("Cleanup the specific resource instance Spoke") + err = k8sClient.Delete(ctx, resource) + if !errors.IsNotFound(err) { + Expect(err).NotTo(HaveOccurred()) + } })
37-38: Use a test-scoped context with timeout.Avoid using a background context across It/BeforeEach/AfterEach; a per-test timeout prevents hangs if Reconcile blocks.
- ctx := context.Background() + // Each test creates its own timeout-bound context. + // (Alternatively, create in BeforeEach and cancel in AfterEach.) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel()Add import:
import ( "context" + "time"Also applies to: 69-82
fleetconfig-controller/config/certmanager/issuer.yaml (1)
1-3: Modernize comments (docs URL and version note).The docs moved to cert-manager.io, and the v1.0 warning is stale given apiVersion v1 is used.
-# The following manifest contains a self-signed issuer CR. -# More information can be found at https://docs.cert-manager.io -# WARNING: Targets CertManager v1.0. Check https://cert-manager.io/docs/installation/upgrading/ for breaking changes. +# Self-signed Issuer for webhook and metrics Certificates. +# More information: https://cert-manager.io/docs/fleetconfig-controller/internal/controller/v1alpha1/hub.go (1)
110-117: Rename local var to reflect intent and avoid confusion.
hubNeedsUpgradereturns “needs upgrade”; renameupgradetoneedsUpgradeto match semantics.- upgrade, err := hubNeedsUpgrade(ctx, fc, operatorC) + needsUpgrade, err := hubNeedsUpgrade(ctx, fc, operatorC) if err != nil { return fmt.Errorf("failed to check if hub needs upgrade: %w", err) } - if upgrade { + if needsUpgrade { return upgradeHub(ctx, fc) }fleetconfig-controller/config/certmanager/certificate-webhook.yaml (1)
11-21: Specify key usages for TLS server auth and consider rotation hints.Defaults are okay, but being explicit reduces surprises across cert-manager versions. Optional rotation settings shown for consistency.
spec: # SERVICE_NAME and SERVICE_NAMESPACE will be substituted by kustomize # replacements in the config/default/kustomization.yaml file. dnsNames: - SERVICE_NAME.SERVICE_NAMESPACE.svc - SERVICE_NAME.SERVICE_NAMESPACE.svc.cluster.local + usages: + - digital signature + - key encipherment + - server auth + # Optional (tune as desired): + # duration: 365d + # renewBefore: 240h # 10 days issuerRef: kind: Issuer name: selfsigned-issuer secretName: webhook-server-certfleetconfig-controller/internal/controller/v1alpha1/suite_test.go (2)
34-34: Avoid import alias shadowing the current package nameImporting the API package with alias v1alpha1 inside package v1alpha1 is legal but confusing. Prefer a distinct alias to avoid mental collisions.
Apply this diff:
- v1alpha1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1alpha1" + apiv1alpha1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1alpha1"And update the AddToScheme call:
- err = v1alpha1.AddToScheme(scheme.Scheme) + err = apiv1alpha1.AddToScheme(scheme.Scheme)Also applies to: 92-93
101-110: Harden teardown: guard kubeconfigCleanup and unset KUBECONFIGIf BeforeSuite fails before kubeconfigCleanup is set, calling it will panic. Also, unsetting KUBECONFIG reduces cross-test leakage.
Apply this diff:
- Expect(os.Setenv("KUBECONFIG", kubeconfigPath)).To(Succeed()) + Expect(os.Setenv("KUBECONFIG", kubeconfigPath)).To(Succeed()) logf.Log.Info("Kubeconfig", "path", kubeconfigPath) }) var _ = AfterSuite(func() { By("tearing down the test environment") err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) - kubeconfigCleanup() + if kubeconfigCleanup != nil { + kubeconfigCleanup() + } + _ = os.Unsetenv("KUBECONFIG") })Also applies to: 112-117
fleetconfig-controller/internal/controller/v1beta1/hub_controller_test.go (2)
45-57: Make NotFound check idiomatic and set Spec explicitlyMinor cleanup: IsNotFound(err) already guards nil; and setting Spec explicitly avoids relying on zero-value marshaling.
Apply this diff:
- err := k8sClient.Get(ctx, typeNamespacedName, hub) - if err != nil && errors.IsNotFound(err) { + err := k8sClient.Get(ctx, typeNamespacedName, hub) + if errors.IsNotFound(err) { resource := &v1beta1.Hub{ ObjectMeta: metav1.ObjectMeta{ Name: resourceName, Namespace: "default", }, - // TODO(user): Specify other spec details if needed. + Spec: v1beta1.HubSpec{}, } Expect(k8sClient.Create(ctx, resource)).To(Succeed()) }
60-68: Make cleanup resilient to already-removed resourcesDelete should ignore NotFound to avoid spurious failures if the resource disappears between Get and Delete.
Apply this diff (add client import if not present):
- resource := &v1beta1.Hub{} - err := k8sClient.Get(ctx, typeNamespacedName, resource) - Expect(err).NotTo(HaveOccurred()) - - By("Cleanup the specific resource instance Hub") - Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) + resource := &v1beta1.Hub{} + By("Cleanup the specific resource instance Hub") + err := k8sClient.Delete(ctx, resource) + Expect(client.IgnoreNotFound(err)).To(Succeed())And add the import:
@@ "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile"fleetconfig-controller/internal/test/test.go (2)
25-31: Include the computed file path in error messages for faster diagnosis.Use a variable for the computed path and surface it in errors so test failures are self-explanatory.
- data, err := os.ReadFile(filepath.Join(root, "hack", "test-config.json")) + cfgPath := filepath.Join(root, "hack", "test-config.json") + data, err := os.ReadFile(cfgPath) if err != nil { - return nil, fmt.Errorf("failed to load test config: %w", err) + return nil, fmt.Errorf("failed to load test config %q: %w", cfgPath, err) }
74-96: Make project root discovery more robust and allow an env override.Relying on string-splitting the CWD to find “fleetconfig-controller” is brittle (e.g., when tests run from atypical dirs or via symlinks). Prefer walking up parents and honoring an explicit override env var.
-// GetProjectDir returns the fleetconfig-controller project root directory. -// It works by finding "fleetconfig-controller" in the current working directory path. +// GetProjectDir returns the fleetconfig-controller project root directory. +// Strategy: +// 1) If FLEETCONFIG_PROJECT_DIR is set, use it. +// 2) Walk up from the current working directory until the directory name is "fleetconfig-controller". func GetProjectDir() (string, error) { - wd, err := os.Getwd() - if err != nil { - return "", err - } - - // Find the fleetconfig-controller directory in the path - parts := strings.Split(wd, string(os.PathSeparator)) - for i, part := range parts { - if part == "fleetconfig-controller" { - // Reconstruct path up to and including fleetconfig-controller - projectPath := strings.Join(parts[:i+1], string(os.PathSeparator)) - if projectPath == "" { - projectPath = string(os.PathSeparator) // Handle root case - } - return projectPath, nil - } - } - - return "", fmt.Errorf("fleetconfig-controller directory not found in path: %s", wd) + if root := os.Getenv("FLEETCONFIG_PROJECT_DIR"); root != "" { + return root, nil + } + dir, err := os.Getwd() + if err != nil { + return "", err + } + for { + if filepath.Base(dir) == "fleetconfig-controller" { + return dir, nil + } + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + return "", fmt.Errorf("fleetconfig-controller directory not found; starting from: %s", dir) }fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (1)
17-18: Fix package doc to reference v1beta1 resources.The comment says v1alpha1; should be v1beta1.
-// Package v1beta1 contains the main reconciliation logic for fleetconfig-controller's v1alpha1 resources. +// Package v1beta1 contains the reconciliation logic for fleetconfig-controller's v1beta1 resources.fleetconfig-controller/config/rbac/spoke_editor_role.yaml (1)
15-27: Consider adding finalizers subresource if editors will manage finalizers.If editors need to set or clear finalizers on Spoke objects (less common, but sometimes desired to unblock deletions), include spokes/finalizers with update. Otherwise, ignore.
rules: - apiGroups: - - fleetconfig.open-cluster-management.io + - fleetconfig.open-cluster-management.io resources: - spokes verbs: - create - delete - get - list - patch - update - watch + - apiGroups: + - fleetconfig.open-cluster-management.io + resources: + - spokes/finalizers + verbs: + - updatefleetconfig-controller/api/v1beta1/spoke_types.go (4)
54-54: Drop non-standard json tag option "omitzero".encoding/json recognizes only "omitempty" and "string". "omitzero" is ignored by the standard library and most tooling. Unless you have custom marshalling that depends on it, prefer removing it for clarity and consistency.
- metav1.ObjectMeta `json:"metadata,omitempty,omitzero"` + metav1.ObjectMeta `json:"metadata,omitempty"` @@ - Status SpokeStatus `json:"status,omitempty,omitzero"` + Status SpokeStatus `json:"status,omitempty"`Also applies to: 62-62
38-42: Add Conditions to status for future-proofing and observability.A standard .status.conditions slice enables consistent readiness and reconciliation reporting across tooling (kubectl get --watch, UIs). Safe to add now; optional if you want to keep scaffolding minimal.
type SpokeStatus struct { - // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster - // Important: Run "make" to regenerate code after modifying this file + // Conditions represent the latest available observations of Spoke's state. + // +optional + Conditions []metav1.Condition `json:"conditions,omitempty"` }
44-47: Consider adding short names and printer columns.Short names simplify UX (e.g., kubectl get sp) and printer columns surface key fields like age/phase.
// +kubebuilder:object:root=true // +kubebuilder:subresource:status -// +kubebuilder:resource:path=spokes,scope=Cluster +// +kubebuilder:resource:path=spokes,scope=Cluster,shortName=sp +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
33-36: Placeholder field "foo" in a v1beta1 API.Shipping v1beta1 with a placeholder risks API surface churn. If this is just scaffolding, plan to remove or rename before release, and ensure webhooks/validation reflect the real spec.
I can help draft the intended SpokeSpec (mirroring the v1alpha1 Spoke-like fields you have elsewhere) and validation markers when ready.
fleetconfig-controller/config/rbac/hub_admin_role.yaml (2)
21-21: Avoid wildcard verbs; enumerate explicitly (policy hardening).While this is an admin role, enumerating verbs aligns with least-privilege policies and satisfies linters like CKV_K8S_49.
- - '*' + - create + - get + - list + - watch + - update + - patch + - delete + - deletecollection
11-14: Optional: aggregate into the built-in admin ClusterRole.If you want these rules automatically included in the standard admin role, add the aggregation label.
labels: app.kubernetes.io/name: fleetconfig-controller app.kubernetes.io/managed-by: kustomize + rbac.authorization.k8s.io/aggregate-to-admin: "true"fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go (3)
127-136: Set an explicit TLS minimum version in tests.Explicit MinVersion avoids surprises across Go/K8s versions. TLS 1.2 is a safer default for envtest compatibility; bump to 1.3 if your envtest assets support it.
- conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{ + InsecureSkipVerify: true, // test env only + MinVersion: tls.VersionTLS12, + })
75-83: Fail fast on missing CRD directory in tests.Setting ErrorIfCRDPathMissing to true tightens the feedback loop if CRD bases aren’t present or path changes.
testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")}, - ErrorIfCRDPathMissing: false, + ErrorIfCRDPathMissing: true,
84-87: Avoid double-calling getFirstFoundEnvTestBinaryDir().Minor cleanup: call once and reuse.
- // Retrieve the first found binary directory to allow running tests from IDEs - if getFirstFoundEnvTestBinaryDir() != "" { - testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() - } + // Retrieve the first found binary directory to allow running tests from IDEs + if dir := getFirstFoundEnvTestBinaryDir(); dir != "" { + testEnv.BinaryAssetsDirectory = dir + }fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_hubs.yaml (3)
1-57: Validate RBAC/CRD group alignment.CRD group here is fleetconfig.open-cluster-management.io. Ensure all RBAC manifests (hub/spoke roles and manager RBAC) reference this exact group; some files still use open-cluster-management.io.
I can scan and propose a patch across RBAC/Helm templates to normalize the apiGroups.
9-15: Optional UX: add categories and shortNames.Adding categories (e.g., fleetconfig) and shortNames (e.g., hb) improves kubectl ergonomics.
names: kind: Hub listKind: HubList plural: hubs singular: hub + shortNames: + - hb + categories: + - fleetconfig
17-56: Optional: printer columns for key fields.Printer columns (Age, Phase/Ready) help operators at a glance.
- name: v1beta1 schema: openAPIV3Schema: description: Hub is the Schema for the hubs API properties: @@ served: true storage: true subresources: status: {} + additionalPrinterColumns: + - name: Age + type: date + jsonPath: .metadata.creationTimestampfleetconfig-controller/config/rbac/spoke_admin_role.yaml (1)
4-6: Header comment is misleading about scope; clarify resource and apiGroup.The comment says “Grants full permissions ('*') over open-cluster-management.io,” but the rules only cover the Spoke resource and subresources, and the rest of the repo uses the apiGroup fleetconfig.open-cluster-management.io for Hub/Spoke. Please make the wording precise to avoid confusion.
Apply this tweak:
-# Grants full permissions ('*') over open-cluster-management.io. +# Grants admin-level permissions over Spoke resources in fleetconfig.open-cluster-management.io.fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (2)
27-28: Avoid aliasing the imported API package to the same identifier as the current package.This file’s package is v1beta1 and you import the API package as v1beta1, which is legal but confusing. Prefer a distinct alias for readability.
-import ( +import ( "context" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" - v1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" + apiv1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" ) @@ - .For(&v1beta1.Spoke{}). + .For(&apiv1beta1.Spoke{}).Also applies to: 60-61
49-55: Scaffold a minimal fetch-and-requeue to exercise reconciliation and aid testing.Using the request adds useful logs and error handling and makes unit/integration tests meaningful. Here’s a minimal, idiomatic pattern handling NotFound.
-func (r *SpokeReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { - _ = logf.FromContext(ctx) - // TODO(user): your logic here - return ctrl.Result{}, nil +func (r *SpokeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + log := logf.FromContext(ctx) + + var spoke apiv1beta1.Spoke + if err := r.Get(ctx, req.NamespacedName, &spoke); err != nil { + // Object deleted or not found; nothing to do. + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + // TODO(user): reconcile desired vs. actual state here + log.V(1).Info("reconciled Spoke", "name", req.NamespacedName) + return ctrl.Result{}, nil }Note: adjust the import to use the alias from the previous comment.
fleetconfig-controller/config/webhook/manifests.yaml (1)
94-134: Consider validating DELETE operations for v1beta1 as you do for v1alpha1.If deletion should be blocked or checked (e.g., active dependencies/finalizers), include DELETE in the ValidatingWebhookConfiguration for hubs and spokes for consistency with v1alpha1.
operations: - CREATE - UPDATE + - DELETEApplies to both vhub-v1beta1.kb.io and vspoke-v1beta1.kb.io entries.
fleetconfig-controller/config/certmanager/certificate-metrics.yaml (2)
1-2: Update the docs URL.The canonical cert-manager docs moved; consider updating the link for future readers.
-# More document can be found at https://docs.cert-manager.io +# More docs: https://cert-manager.io/docs/
17-20: Naming consistency: “metrics-certs” vs “metrics-server-cert”.Minor nit: consider aligning the Certificate name with the secret to ease ops (“metrics-server-cert” for both, or “metrics-web” consistently).
- name: metrics-certs # this name should match the one appeared in kustomizeconfig.yaml + name: metrics-server-cert # align with secret and kustomizeconfig.yamlNote: Update kustomizeconfig.yaml nameReference if you change this.
fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook_test.go (4)
27-45: Test scaffolding is fine; add ctx to enable real default/validate calls.You reference ctx in examples; declare and init it now so adding tests is trivial.
import ( + "context" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ var _ = Describe("Spoke Webhook", func() { var ( + ctx context.Context obj *fleetconfigopenclustermanagementiov1beta1.Spoke @@ BeforeEach(func() { + ctx = context.TODO()
51-62: Add a minimal defaulting test to catch regressions and bump coverage.Even if your defaulter is a no-op today, assert it doesn’t panic and can be safely called.
Context("When creating Spoke under Defaulting Webhook", func() { - // TODO (user): Add logic for defaulting webhooks + It("Should not panic when applying defaults to an empty object", func() { + Expect(func() { defaulter.Default(ctx, obj) }).NotTo(Panic()) + })
64-85: Add minimal validator happy/empty-path tests.If your validator currently allows empty specs (typical scaffold), assert nil errors for create/update; when you add real rules, expand cases.
Context("When creating or updating Spoke under Validating Webhook", func() { - // TODO (user): Add logic for validating webhooks + It("Should admit create for an empty object with no rules", func() { + Expect(validator.ValidateCreate(ctx, obj)).To(Succeed()) + }) + It("Should admit update when old and new objects are identical", func() { + Expect(validator.ValidateUpdate(ctx, oldObj, obj)).To(Succeed()) + }) + It("Should admit delete by default", func() { + Expect(validator.ValidateDelete(ctx, obj)).To(Succeed()) + })
23-25: Shorten the long import alias for readability.The alias fleetconfigopenclustermanagementiov1beta1 is unwieldy in tests. Consider a shorter alias (e.g., api or fcv1beta1).
- fleetconfigopenclustermanagementiov1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" + fcv1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" @@ - obj *fleetconfigopenclustermanagementiov1beta1.Spoke - oldObj *fleetconfigopenclustermanagementiov1beta1.Spoke + obj *fcv1beta1.Spoke + oldObj *fcv1beta1.Spoke @@ - obj = &fleetconfigopenclustermanagementiov1beta1.Spoke{} - oldObj = &fleetconfigopenclustermanagementiov1beta1.Spoke{} + obj = &fcv1beta1.Spoke{} + oldObj = &fcv1beta1.Spoke{}fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
17-25: Optional: add Kind/Resource helpers for ergonomics.Providing small helpers reduces call-site boilerplate when building GVKs/GroupResources.
Apply this diff:
import ( - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/scheme" ) var ( // GroupVersion is group version used to register these objects. GroupVersion = schema.GroupVersion{Group: "fleetconfig.open-cluster-management.io", Version: "v1beta1"} @@ // AddToScheme adds the types in this group-version to the given scheme. AddToScheme = SchemeBuilder.AddToScheme ) + +// Kind returns a Group qualified GroupKind. +func Kind(kind string) schema.GroupKind { + return GroupVersion.WithKind(kind).GroupKind() +} + +// Resource returns a Group qualified GroupResource. +func Resource(resource string) schema.GroupResource { + return GroupVersion.WithResource(resource).GroupResource() +}fleetconfig-controller/internal/webhook/v1beta1/hub_webhook_test.go (2)
20-26: Nit: shorten long import alias for readability.Using a concise alias (e.g., apiv1beta1) makes tests easier to read.
Apply this diff:
-import ( +import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - fleetconfigopenclustermanagementiov1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" + apiv1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" // TODO (user): Add any additional imports if needed )And update references:
- obj *fleetconfigopenclustermanagementiov1beta1.Hub - oldObj *fleetconfigopenclustermanagementiov1beta1.Hub + obj *apiv1beta1.Hub + oldObj *apiv1beta1.Hub @@ - obj = &fleetconfigopenclustermanagementiov1beta1.Hub{} - oldObj = &fleetconfigopenclustermanagementiov1beta1.Hub{} + obj = &apiv1beta1.Hub{} + oldObj = &apiv1beta1.Hub{}
52-63: Actionable placeholder: add at least one minimal defaulting test tied to a real field.Even a trivial test prevents regressions and exercises the webhook wiring.
Example to drop in (after you implement Default on HubCustomDefaulter):
Context("When creating Hub under Defaulting Webhook", func() { - // TODO (user): Add logic for defaulting webhooks + It("applies defaults to optional fields", func() { + By("simulating a missing optional field") + obj.Spec.Foo = nil + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + // Example expectation after your defaulter sets a default + // Expect(*obj.Spec.Foo).To(Equal("default")) + }) })fleetconfig-controller/config/crd/bases/fleetconfig.open-cluster-management.io_spokes.yaml (2)
41-46: Reminder: replace placeholder spec.foo before promoting beyond scaffolding.foo is a scaffold field. Before broader use, model real SpokeSpec and update samples/docs accordingly to avoid confusing users.
53-56: Optional: add printer columns to improve kubectl UX.Common fields (phase, age, etc.) help operators. Consider adding
additionalPrinterColumns.Example:
additionalPrinterColumns: - name: Age type: date jsonPath: .metadata.creationTimestampfleetconfig-controller/api/v1beta1/hub_types.go (1)
26-36: Scaffold note: replace Foo with real HubSpec as you converge on v1beta1.Before consumers rely on v1beta1, align HubSpec with intended fields (e.g., kubeconfig, apiServer, etc.). This reduces follow-up breaking changes.
fleetconfig-controller/internal/controller/v1beta1/suite_test.go (2)
76-79: Optional: cache the resolved envtest binary path to avoid double directory reads.getFirstFoundEnvTestBinaryDir() is called twice; store the value once to reduce I/O and ensure consistency.
Apply this diff:
-// Retrieve the first found binary directory to allow running tests from IDEs -if getFirstFoundEnvTestBinaryDir() != "" { - testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() -} +// Retrieve the first found binary directory to allow running tests from IDEs +if dir := getFirstFoundEnvTestBinaryDir(); dir != "" { + testEnv.BinaryAssetsDirectory = dir +}
86-89: Optional: add a discovery sanity check after client creation.A quick ServerGroups() or RESTMapper lookup helps fail fast if CRDs didn’t load.
Example (after client creation):
rm := restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(discovery.NewDiscoveryClientForConfigOrDie(cfg))) _, err = rm.RESTMapping(schema.GroupKind{Group: "fleetconfig.open-cluster-management.io", Kind: "Hub"}, "v1beta1") Expect(err).NotTo(HaveOccurred())fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (3)
19-19: Avoid blanket lint suppression; narrow to the specific rule (dupl).
//nolint:allsilences every linter in this file, which can hide real issues. Limit it to the duplication rule only.-//nolint:all // Required because of `dupl` between this file and spoke_webhook.go +//nolint:dupl // Temporarily suppress duplication with spoke_webhook.go
66-68: Fix minor grammar in error text (“a Hub” not “an Hub”).User-facing/logged errors should be clear and polished.
- return fmt.Errorf("expected an Hub object but got %T", obj) + return fmt.Errorf("expected a Hub object but got %T", obj)
107-110: Fix minor grammar in error text (“a Hub” not “an Hub”).- return nil, fmt.Errorf("expected a Hub object for the newObj but got %T", newObj) + return nil, fmt.Errorf("expected a Hub object for newObj but got %T", newObj)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (3)
19-19: Avoid blanket lint suppression; narrow to the specific rule (dupl).-//nolint:all // Required because of `dupl` between this file and spoke_webhook.go +//nolint:dupl // Temporarily suppress duplication with hub_webhook.go
66-68: Fix minor grammar in error text (“a Spoke” not “an Spoke”).- return fmt.Errorf("expected an Spoke object but got %T", obj) + return fmt.Errorf("expected a Spoke object but got %T", obj)
109-110: Fix minor grammar in error text (“a Spoke” not “an Spoke”).- return nil, fmt.Errorf("expected a Spoke object for the newObj but got %T", newObj) + return nil, fmt.Errorf("expected a Spoke object for newObj but got %T", newObj)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io-crds.yaml (2)
2764-2819: CRD shape for Hub (v1beta1) is minimal and valid; consider adding printer columns.Resource is cluster-scoped with a basic OpenAPI schema. For better UX, add printer columns (e.g., AGE and possibly a simple phase/status when available).
Example snippet to add under versions[0]:
additionalPrinterColumns: - name: AGE type: date jsonPath: .metadata.creationTimestamp
2820-2874: CRD shape for Spoke (v1beta1) is minimal and valid; consider adding printer columns.Same suggestion as Hub to improve kubectl readability (AGE, and any simple status once defined).
fleetconfig-controller/cmd/main.go (1)
167-180: De-duplicate ENABLE_WEBHOOKS checks for Spoke/Hub.Combine these into one block to avoid double env lookups and keep structure tidy. Functionally equivalent.
- // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1beta1.SetupSpokeWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "Spoke") - os.Exit(1) - } - } - // nolint:goconst - if os.Getenv("ENABLE_WEBHOOKS") != "false" { - if err := webhookv1beta1.SetupHubWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "Hub") - os.Exit(1) - } - } + // nolint:goconst + if os.Getenv("ENABLE_WEBHOOKS") != "false" { + if err := webhookv1beta1.SetupSpokeWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Spoke") + os.Exit(1) + } + if err := webhookv1beta1.SetupHubWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "Hub") + os.Exit(1) + } + }
fleetconfig-controller/config/certmanager/certificate-metrics.yaml
Outdated
Show resolved
Hide resolved
fleetconfig-controller/config/default/manager_webhook_patch.yaml
Outdated
Show resolved
Hide resolved
fleetconfig-controller/config/network-policy/allow-webhook-traffic.yaml
Outdated
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
Outdated
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/spoke_controller_test.go
Show resolved
Hide resolved
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go
Outdated
Show resolved
Hide resolved
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>
TylerGillson
left a comment
TylerGillson
left a comment
There was a problem hiding this comment.
We don't actually use anything under config/. Can you please remove that dir entirely and update the controller-gen marker to output the CRDs directly into the helm chart's crds dir?
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
fleetconfig-controller/config/rbac/spoke_admin_role.yaml (2)
4-4: Fix stale API group in header comment.Header still mentions open-cluster-management.io, but the CRD group used below is fleetconfig.open-cluster-management.io.
-# Grants full permissions ('*') over open-cluster-management.io. +# Grants full permissions ('*') over fleetconfig.open-cluster-management.io.
22-27: Grant full admin coverage for subresources (status + finalizers).For an admin role, include update/patch on spokes/status and update on spokes/finalizers. Otherwise, common admin actions (e.g., kubectl patch status, clearing finalizers) will fail.
- apiGroups: - fleetconfig.open-cluster-management.io resources: - spokes/status verbs: - - get + - get + - update + - patch +- + apiGroups: + - fleetconfig.open-cluster-management.io + resources: + - spokes/finalizers + verbs: + - updatefleetconfig-controller/config/rbac/hub_admin_role.yaml (1)
4-4: Fix stale API group in header comment.Same as Spoke: update the header to fleetconfig.open-cluster-management.io to avoid confusion.
-# Grants full permissions ('*') over open-cluster-management.io. +# Grants full permissions ('*') over fleetconfig.open-cluster-management.io.fleetconfig-controller/cmd/main.go (1)
163-177: Unify webhook gating to a single mechanismYou’re gating with both a flag (use-webhook) and an env var (ENABLE_WEBHOOKS). This makes behavior non-obvious (webhooks are on by default when env is unset, regardless of the flag). Prefer one gate, consistent with other components.
Minimal change within this block:
- // nolint:goconst - if useWebhook || os.Getenv("ENABLE_WEBHOOKS") != "false" { + // nolint:goconst + if os.Getenv("ENABLE_WEBHOOKS") != "false" { if err = apiv1alpha1.SetupFleetConfigWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "FleetConfig") os.Exit(1) } if err := webhookv1beta1.SetupSpokeWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Spoke") os.Exit(1) } if err := webhookv1beta1.SetupHubWebhookWithManager(mgr); err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "Hub") os.Exit(1) } }Optional follow-up (outside this hunk): remove the use-webhook flag entirely to avoid a no-op flag, or repurpose it to override the env gate explicitly (documented precedence).
To confirm there are no other divergent gates, run:
#!/bin/bash # Find all webhook gates in the repo rg -nP -C2 'ENABLE_WEBHOOKS|use-webhook|Setup.*WebhookWithManager'I can send a follow-up patch to remove the flag and update the chart/README if you want.
🧹 Nitpick comments (5)
fleetconfig-controller/config/rbac/spoke_admin_role.yaml (1)
20-21: Optional: minimize wildcard verbs to satisfy policy scanners (CKV_K8S_49).If you want to avoid “*” for this admin role, enumerate verbs explicitly.
resources: - spokes verbs: - - '*' + - get + - list + - watch + - create + - update + - patch + - deletefleetconfig-controller/config/rbac/hub_admin_role.yaml (1)
20-21: Optional: replace wildcard verbs for “hubs” with explicit list.Mirrors the Spoke suggestion; helps pass conservative scanners.
resources: - hubs verbs: - - '*' + - get + - list + - watch + - create + - update + - patch + - deletefleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (3)
27-28: Disambiguate API import alias to avoid confusion with local packageImporting the API package as v1beta1 from within package v1beta1 is easy to misread. Use a distinct alias to make call sites obvious.
Apply:
-import ( +import ( @@ - v1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" + fleetconfigv1beta1 "github.com/open-cluster-management-io/lab/fleetconfig-controller/api/v1beta1" ) @@ - For(&v1beta1.Spoke{}). + For(&fleetconfigv1beta1.Spoke{}).Also applies to: 60-61
49-55: Remove no-op logger assignment or use it
_ = logf.FromContext(ctx)is dead code. Either log something at trace level or drop the line.Minimal cleanup:
func (r *SpokeReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) { - _ = logf.FromContext(ctx) + // log := logf.FromContext(ctx)
47-48: Avoid version-pinning in docs link to prevent driftUse the unversioned pkg.go.dev path so the link stays valid across controller-runtime bumps.
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.21.0/pkg/reconcile +// - https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/reconcile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
fleetconfig-controller/cmd/main.go(3 hunks)fleetconfig-controller/config/rbac/hub_admin_role.yaml(1 hunks)fleetconfig-controller/config/rbac/hub_editor_role.yaml(1 hunks)fleetconfig-controller/config/rbac/hub_viewer_role.yaml(1 hunks)fleetconfig-controller/config/rbac/role.yaml(1 hunks)fleetconfig-controller/config/rbac/spoke_admin_role.yaml(1 hunks)fleetconfig-controller/config/rbac/spoke_editor_role.yaml(1 hunks)fleetconfig-controller/config/rbac/spoke_viewer_role.yaml(1 hunks)fleetconfig-controller/internal/controller/v1beta1/hub_controller.go(1 hunks)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- fleetconfig-controller/config/rbac/spoke_viewer_role.yaml
- fleetconfig-controller/config/rbac/hub_viewer_role.yaml
- fleetconfig-controller/internal/controller/v1beta1/hub_controller.go
- fleetconfig-controller/config/rbac/role.yaml
- fleetconfig-controller/config/rbac/hub_editor_role.yaml
- fleetconfig-controller/config/rbac/spoke_editor_role.yaml
🧰 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.732Z
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.732Z
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.732Z
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-08-22T17:55:52.134Z
Learnt from: TylerGillson
PR: open-cluster-management-io/lab#51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.134Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.
Applied to files:
fleetconfig-controller/cmd/main.go
🧬 Code graph analysis (2)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
fleetconfig-controller/api/v1beta1/spoke_types.go (1)
Spoke(49-63)
fleetconfig-controller/cmd/main.go (7)
fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
AddToScheme(35-35)fleetconfig-controller/internal/controller/v1alpha1/fleetconfig_controller.go (1)
FleetConfigReconciler(58-62)fleetconfig-controller/internal/controller/v1beta1/hub_controller.go (1)
HubReconciler(32-35)fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)
SpokeReconciler(31-34)fleetconfig-controller/api/v1alpha1/fleetconfig_webhook.go (1)
SetupFleetConfigWebhookWithManager(38-43)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
SetupSpokeWebhookWithManager(40-45)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (1)
SetupHubWebhookWithManager(40-45)
🪛 Checkov (3.2.334)
fleetconfig-controller/config/rbac/hub_admin_role.yaml
[MEDIUM] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
fleetconfig-controller/config/rbac/spoke_admin_role.yaml
[MEDIUM] 8-27: Minimize wildcard use in Roles and ClusterRoles
(CKV_K8S_49)
⏰ 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 (6)
fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (2)
36-38: RBAC apiGroup corrected — LGTMRBAC markers now target fleetconfig.open-cluster-management.io for spokes, status, and finalizers. This unblocks manager RBAC generation for the new API group.
57-63: Controller registration looks correctWatching Spoke and naming the controller "spoke" is consistent with the Hub scaffold and controller-runtime conventions.
fleetconfig-controller/cmd/main.go (4)
38-43: Multi-version wiring imports — LGTMSeparate aliases for API, controllers, and webhooks keep responsibilities clear. Good separation by version.
54-55: Scheme registration for v1alpha1 and v1beta1 — LGTMBoth API versions are added to the scheme; this is required for decoding and client usage.
139-147: FleetConfig controller setup — LGTMController registration for v1alpha1 is intact and namespaced logging context is preserved.
148-161: Hub/Spoke controller setup — LGTMControllers are registered with the manager and use the shared scheme. Matches the scaffolded patterns.
@TylerGillson we do have a |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fleetconfig-controller/Makefile (1)
85-89: manifests target now only generates CRDs; update target description and drop unused kustomize prereqThe rule description still mentions WebhookConfiguration, and
kustomizeis listed as a prerequisite but not used in the recipe.Apply this minimal fix:
-.PHONY: manifests -manifests: kustomize controller-gen ## Generate CustomResourceDefinition and WebhookConfiguration objects. - $(CONTROLLER_GEN) crd paths="./..." output:crd:artifacts:config=charts/fleetconfig-controller/crds +.PHONY: manifests +manifests: controller-gen ## Generate CustomResourceDefinitions. + $(CONTROLLER_GEN) crd paths="./..." output:crd:artifacts:config=charts/fleetconfig-controller/crds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
fleetconfig-controller/Makefile(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml(1 hunks)fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml(1 hunks)fleetconfig-controller/config/crd/kustomization.yaml(0 hunks)fleetconfig-controller/config/crd/kustomizeconfig.yaml(0 hunks)fleetconfig-controller/config/crd/patches/cainjection_in_fleetconfigs.yaml(0 hunks)fleetconfig-controller/config/crd/patches/webhook_in_fleetconfigs.yaml(0 hunks)fleetconfig-controller/config/samples/fleetconfig_invalid_v1alpha1.yaml(0 hunks)fleetconfig-controller/config/samples/fleetconfig_singleton_v1alpha1.yaml(0 hunks)fleetconfig-controller/config/samples/fleetconfig_v1alpha1.yaml(0 hunks)fleetconfig-controller/config/webhook/manifests.yaml(0 hunks)
💤 Files with no reviewable changes (8)
- fleetconfig-controller/config/samples/fleetconfig_singleton_v1alpha1.yaml
- fleetconfig-controller/config/samples/fleetconfig_invalid_v1alpha1.yaml
- fleetconfig-controller/config/crd/patches/webhook_in_fleetconfigs.yaml
- fleetconfig-controller/config/crd/kustomizeconfig.yaml
- fleetconfig-controller/config/crd/patches/cainjection_in_fleetconfigs.yaml
- fleetconfig-controller/config/samples/fleetconfig_v1alpha1.yaml
- fleetconfig-controller/config/crd/kustomization.yaml
- fleetconfig-controller/config/webhook/manifests.yaml
✅ Files skipped from review due to trivial changes (2)
- fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml
- fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_hubs.yaml
🧰 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.732Z
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-22T17:55:52.134Z
Learnt from: TylerGillson
PR: open-cluster-management-io/lab#51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.134Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.
Applied to files:
fleetconfig-controller/Makefile
⏰ 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
I's also like to leave in the |
Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
fleetconfig-controller/config/webhook/manifests.yaml (2)
27-66: Avoid hard-coding webhook Service name/namespace (will break outside “system”)The new v1beta1 mutating webhooks reference
service: { name: webhook-service, namespace: system }. This is fragile for integration tests and any non-system namespace.Action options:
- Use kustomize overlays to set
metadata.namespacefor the Service and rewriteclientConfig.service.{name,namespace}.- Or template these via Helm (if consumed there) and keep test-time overlays in kustomize.
Note: A similar concern was raised earlier; marking as a follow-up on the new entries.
94-134: Same namespace/name templating concern for ValidatingWebhookConfigurationMirror the fix here for the validating webhooks (v1beta1 hub/spoke entries). Consider also setting
matchPolicy: Equivalentand atimeoutSeconds(e.g., 10) to be explicit, though that’s optional.
🧹 Nitpick comments (8)
fleetconfig-controller/api/v1alpha1/webhook_suite_test.go (1)
77-83: Ensure webhook manifest path is rooted for all run contextsThe suite already correctly references CRDs under
charts/fleetconfig-controller/crdsand those include the v1alpha1 FleetConfig CRD (fleetconfig.open-cluster-management.io_fleetconfigs.yamlwithversion: v1alpha1).
To make webhook bootstrap resilient regardless of working directory (IDE,go test ./…, etc.), root-join the webhook manifests path the same way:• In fleetconfig-controller/api/v1alpha1/webhook_suite_test.go (around line 82), update WebhookInstallOptions.Paths:
testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{ - filepath.Join(root, "charts", "fleetconfig-controller", "crds"), + filepath.Join(root, "charts", "fleetconfig-controller", "crds"), }, ErrorIfCRDPathMissing: false, WebhookInstallOptions: envtest.WebhookInstallOptions{ - Paths: []string{filepath.Join("..", "..", "config", "webhook")}, + Paths: []string{filepath.Join(root, "config", "webhook")}, }, }This change ensures the test always finds the webhook manifests under
<project-root>/config/webhookinstead of relying on the process’s current directory.fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go (4)
75-86: Root join for webhook manifests to avoid brittle relative pathsYou already resolve CRDs via project root. Do the same for the webhook manifests to keep IDE/
go test ./...runs stable.testEnv = &envtest.Environment{ CRDDirectoryPaths: []string{filepath.Join(root, "charts", "fleetconfig-controller", "crds")}, ErrorIfCRDPathMissing: false, WebhookInstallOptions: envtest.WebhookInstallOptions{ - Paths: []string{filepath.Join("..", "..", "..", "config", "webhook")}, + Paths: []string{filepath.Join(root, "config", "webhook")}, }, }
88-91: Deduplicate envtest-binaries discovery (use the shared helper)This suite introduces
getFirstFoundEnvTestBinaryDir()while other suites useinternal/test.FindEnvTestBinaryDir. Prefer the shared helper for consistency and single point of maintenance.- if getFirstFoundEnvTestBinaryDir() != "" { - testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() - } + if dir := test.FindEnvTestBinaryDir(nil); dir != "" { + testEnv.BinaryAssetsDirectory = dir + }If you need a variant that doesn’t require a config arg, consider adding a small wrapper in
internal/testand using it across suites; then drop the local helper (see lines 150–171).
132-140: Harden TLS client config in readiness probe (test-only)The readiness probe uses
InsecureSkipVerify: true, which is fine for tests, but you can nudge it a bit safer by pinning a minimum TLS version (1.3) to satisfy linters without impacting test behavior.- conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true}) + conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{ + InsecureSkipVerify: true, // test-only + MinVersion: tls.VersionTLS13, + })
150-171: MovegetFirstFoundEnvTestBinaryDirinto internal/test and remove local copyThis helper duplicates logic that belongs in
internal/test. Centralize it (or reuseFindEnvTestBinaryDir) and drop this local implementation. This also lets you remove theosimport.fleetconfig-controller/Makefile (3)
143-149: Simplify CRD install/uninstall and avoid duplicate per-file applyApplying the entire crds/ directory is simpler and avoids double-applying both aggregated and split CRD files if present.
-install-crds: manifests ## Install CRDs. - find charts/fleetconfig-controller/crds/ -name "fleetconfig.open-cluster-management.*.yaml" -exec $(KUBECTL) apply -f {} \; +install-crds: manifests kubectl ## Install CRDs. + $(KUBECTL) apply -f charts/fleetconfig-controller/crds -uninstall-crds: manifests ## Uninstall CRDs. Call with ignore-not-found=true to ignore resource not found errors during deletion. - find charts/fleetconfig-controller/crds/ -name "fleetconfig.open-cluster-management.*.yaml" -exec $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f {} \; +uninstall-crds: manifests kubectl ## Uninstall CRDs. Call with ignore-not-found=true to ignore resource not found errors during deletion. + $(KUBECTL) delete --ignore-not-found=$(ignore-not-found) -f charts/fleetconfig-controller/crds
1-75: Optional: add conventional phony targetsStatic checks flagged missing
allandclean. If you want to appease tooling:+.PHONY: all +all: images + +.PHONY: clean +clean: + rm -rf bin/ _build/ cover.out
86-88: Manifests target remains pure; install_crds.sh only syncs CRDsI’ve verified that
hack/install_crds.shdoes not callkubectl,kustomize,apply, ordelete. It simply downloads CRD YAMLs and copies them intocharts/fleetconfig-controller/crds, so invoking it from themanifeststarget has no side effects on any live cluster.
- No changes to the
manifeststarget are required.- Optional: for clarity, you may rename
hack/install_crds.shto something likesync_crds.sh, since it doesn’t actually install resources on a cluster.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
fleetconfig-controller/Makefile(2 hunks)fleetconfig-controller/api/v1alpha1/webhook_suite_test.go(1 hunks)fleetconfig-controller/config/webhook/manifests.yaml(2 hunks)fleetconfig-controller/internal/controller/v1alpha1/suite_test.go(2 hunks)fleetconfig-controller/internal/controller/v1beta1/suite_test.go(1 hunks)fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- fleetconfig-controller/internal/controller/v1alpha1/suite_test.go
- fleetconfig-controller/internal/controller/v1beta1/suite_test.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.732Z
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-22T17:55:52.134Z
Learnt from: TylerGillson
PR: open-cluster-management-io/lab#51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.134Z
Learning: In the open-cluster-management-io/lab repository, chart versioning for fleetconfig-controller is handled automatically via GitHub release workflows, not through manual version bumps in Chart.yaml during regular PRs.
Applied to files:
fleetconfig-controller/Makefile
🧬 Code graph analysis (1)
fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go (4)
fleetconfig-controller/internal/test/test.go (1)
GetProjectDir(76-96)fleetconfig-controller/api/v1beta1/groupversion_info.go (1)
AddToScheme(35-35)fleetconfig-controller/internal/webhook/v1beta1/spoke_webhook.go (1)
SetupSpokeWebhookWithManager(40-45)fleetconfig-controller/internal/webhook/v1beta1/hub_webhook.go (1)
SetupHubWebhookWithManager(40-45)
🪛 checkmake (0.2.2)
fleetconfig-controller/Makefile
[warning] 147-147: Missing required phony target "all"
(minphony)
[warning] 147-147: Missing required phony target "clean"
(minphony)
🪛 ast-grep (0.38.6)
fleetconfig-controller/internal/webhook/v1beta1/webhook_suite_test.go
[warning] 133-133: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{InsecureSkipVerify: true}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
⏰ 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/webhook_suite_test.go (1)
104-121: LGTM: webhook manager wiring for Spoke and HubManager setup, webhook server options, and the registration of both Spoke and Hub webhook handlers look correct and align with the new v1beta1 APIs.
fleetconfig-controller/Makefile (1)
86-87: LGTM: controller-gen outputs CRDs directly to charts/This aligns generation with where the chart consumes CRDs and with updated tests loading CRDs from charts/.
|
[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 0cfc896
into
open-cluster-management-io:main
2 participants
Description
PROJECTfile to the fleetconfig-controller project to track created resources and allowkubebuilderto functionv1alpha1module to organize the controllersv1beta1resources,HubandSpokein the samefleetconfig.open-cluster-management.iogroup.config/resources.manager-rbactemplate to provide the same permissions for the new resources as for FleetConfigNo implementation on this PR, so that it can be review in a followup with no noise.
Summary by CodeRabbit