✨ expose addon configuration in Spoke API #87

arturshadnik · 2025-11-19T01:39:17Z

Adds deploymentConfig field to Spoke.Spec
Deletes Spoke.Spec.AddOn.InstallNamespace field, replaced by AddOnDeploymentConfigSpec.AgentInstallNamespace
For each AddOn where a deploymentConfig is provided, adds the --config-file flag to clusteradm addon enable
Implements hash based config revision tracking to ensure updates are correctly propagated
Updates e2e tests to verify updates to AddOn variables
Adds a docs page explaining how to use the AddOn and AddOnConfig APIs

Summary by CodeRabbit

New Features
- Rich DeploymentConfig for addons (customized variables, placement, proxy, image registries, resource constraints) and automatic detection of config changes to trigger redeploys.
Documentation
- Added comprehensive addon framework guide describing configuration and workflows.
Chores
- Bumped clusteradm installer version.
Tests
- Updated end-to-end tests to verify addon variable resolution and redeploy behavior.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Signed-off-by: Artur Shad Nik <arturshadnik@gmail.com>

coderabbitai · 2025-11-19T01:39:22Z

Walkthrough

Adds addon deployment configuration support: replaces AddOn.InstallNamespace with DeploymentConfig, introduces a deployment-config hash annotation, refactors addon enable/disable to use a temporary deployment-config file passed to clusteradm, updates CRD/schema, generated deepcopy, Dockerfile versions, docs, and e2e tests.

Changes

Cohort / File(s)	Summary
API constants `fleetconfig-controller/api/v1beta1/constants.go`	Adds exported constant `AnnotationAddOnDeploymentConfigHash = "fleetconfig.open-cluster-management.io/configHash"`
API types `fleetconfig-controller/api/v1beta1/spoke_types.go`	Replaces `AddOn.InstallNamespace` with `AddOn.DeploymentConfig *addonv1alpha1.AddOnDeploymentConfigSpec`; adds import `addonv1alpha1`
Generated deepcopy `fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go`	Adds import for addon v1alpha1 and deep-copy logic for `DeploymentConfig` with nil check
Build files `fleetconfig-controller/build/Dockerfile.base`, `fleetconfig-controller/build/Dockerfile.devspace`, `fleetconfig-controller/build/Dockerfile.eks`, `fleetconfig-controller/build/Dockerfile.gke`, `fleetconfig-controller/hack/.versions.env`	Bumps `CLUSTERADM_VERSION` from `1.0.2` to `1.1.0` across Dockerfiles and versions env
CRD / Schema `fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml`	Replaces `installNamespace` string with `deploymentConfig` object and adds nested fields: `agentInstallNamespace`, `customizedVariables`, `nodePlacement`, `proxyConfig`, `registries`, `resourceRequirements` and validation/metadata
Controller logic `fleetconfig-controller/internal/controller/v1beta1/addon.go`	Adds `createAddonDeploymentConfigFile()`; computes deployment-config hash annotation; refactors enable/disable flow to use config-file + hash; removes `patchFCCMca()` and related in-place MCA patching; updates function signatures/usages
Spoke handler `fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go`	Adds `configureFCCAddOn()` to merge FCC addon DeploymentConfig with hub/spoke vars, sorts customized variables, uses deepcopy of spoke for FCC config, and updates AddonsConfigured signaling
Documentation `fleetconfig-controller/docs/addon-framework.md`	Adds comprehensive addon framework documentation (custom addons, built-in hub addons, workflows, command construction, best practices, troubleshooting)
Test fixtures `fleetconfig-controller/test/data/addon-2-cm.yaml`, `fleetconfig-controller/test/data/fleetconfig-values.yaml`	Updates test addon namespace (`test-addon`), adds ConfigMap with addon variables and adds `deploymentConfig` entries to spoke addon test values
E2E helpers & tests `fleetconfig-controller/test/e2e/helper.go`, `fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go`	Adds helper constants and `ensureAddonVariablesResolved()`; updates tests to verify initial addon variable resolution and addon template/version update flows; imports `addonv1alpha1` in tests

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Areas needing extra attention:
- fleetconfig-controller/internal/controller/v1beta1/addon.go — hashing, temp-file creation, clusteradm invocation, and enable/disable semantics.
- CRD schema changes and spoke_types.go — API surface change and validation semantics for DeploymentConfig.
- E2E tests — ensure test updates align with new deploymentConfig behavior and variable resolution.

Possibly related PRs

refactor: fleetconfig-controller as an addon #69 — touches AddOn deployment/config types and controller integration (direct overlap with deployment-config handling).
✨implement separate hub and spoke resources #59 — modifies the same AddOn struct (InstallNamespace → DeploymentConfig) and related controller changes.
🐛 properly trigger upgrades on version and klusterlet change #56 — introduces or uses hashing utilities similar to the deployment-config hash mechanism.

Suggested labels

lgtm

Suggested reviewers

ahmad-ibra
TylerGillson

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title "expose addon configuration in Spoke API" accurately reflects the main objective of adding a deploymentConfig field to the Spoke API and replacing the simple InstallNamespace with comprehensive AddOnDeploymentConfigSpec, which is the core change throughout the changeset.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-11-19T01:39:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arturshadnik

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:

~~fleetconfig-controller/OWNERS~~ [arturshadnik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (2)

fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go (1)

359-363: Double‑check AddonsConfigured condition semantics across enable/cleanup paths

You now set AddonsConfigured to True:

After successfully enabling addons in doHubWork.

After disabling addons in hubCleanupPreflight (“Success – addons disabled/cleaned up”).

After deleting the FCC agent addon in waitForAgentAddonDeleted.

Later, after waitForAddonManifestWorksCleanup, you flip it to False to signal full cleanup.

If consumers interpret status.conditions[AddonsConfigured].status strictly as “addons are currently configured/enabled”, the intermediate True states during teardown may be confusing. If instead it’s meant as a more generic “addon operations succeeded” flag, this is fine, but it may be worth:

Using more specific reasons/messages for the cleanup paths, or

Documenting that False specifically means “all addon manifestWorks cleaned up; no enabled addons remain”.

Not a blocker, but clarifying this now will avoid ambiguity for future readers or external controllers.

Also applies to: 670-673, 717-720
fleetconfig-controller/internal/controller/v1beta1/addon.go (1)
398-464: Good partial success handling for addon enablement!

The function correctly tracks which addons were successfully enabled even when others fail:

Collects errors in enableErrs slice (lines 398, 438, 452)

Only adds to enabledAddons on success (line 456)

Returns both successful addons and aggregated error (lines 460-462)

This allows the caller to update status with partial progress, which is much better than an all-or-nothing approach.

Optional: Consider using errors.Join (Go 1.20+) for better error aggregation:
 	if len(enableErrs) > 0 {
-		return enabledAddons, fmt.Errorf("one or more addons were not enabled: %v", enableErrs)
+		return enabledAddons, fmt.Errorf("one or more addons were not enabled: %w", errors.Join(enableErrs...))
 	}
This provides better error unwrapping support for callers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9353cb0 and 0678274.

📒 Files selected for processing (16)

fleetconfig-controller/api/v1beta1/constants.go (1 hunks)
fleetconfig-controller/api/v1beta1/spoke_types.go (2 hunks)
fleetconfig-controller/api/v1beta1/zz_generated.deepcopy.go (2 hunks)
fleetconfig-controller/build/Dockerfile.base (1 hunks)
fleetconfig-controller/build/Dockerfile.devspace (1 hunks)
fleetconfig-controller/build/Dockerfile.eks (1 hunks)
fleetconfig-controller/build/Dockerfile.gke (1 hunks)
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml (1 hunks)
fleetconfig-controller/docs/addon-framework.md (1 hunks)
fleetconfig-controller/hack/.versions.env (1 hunks)
fleetconfig-controller/internal/controller/v1beta1/addon.go (6 hunks)
fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go (4 hunks)
fleetconfig-controller/test/data/addon-2-cm.yaml (1 hunks)
fleetconfig-controller/test/data/fleetconfig-values.yaml (2 hunks)
fleetconfig-controller/test/e2e/helper.go (4 hunks)
fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go (3 hunks)

🧰 Additional context used

🧠 Learnings (14)

📚 Learning: 2025-08-22T17:55:52.159Z

Learnt from: TylerGillson
Repo: open-cluster-management-io/lab PR: 51
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-22T17:55:52.159Z
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/hack/.versions.env
fleetconfig-controller/build/Dockerfile.base
fleetconfig-controller/build/Dockerfile.gke
fleetconfig-controller/build/Dockerfile.devspace
fleetconfig-controller/test/data/fleetconfig-values.yaml
fleetconfig-controller/build/Dockerfile.eks
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml

📚 Learning: 2025-08-27T21:58:32.141Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 58
File: fleetconfig-controller/charts/fleetconfig-controller/README.md:155-155
Timestamp: 2025-08-27T21:58:32.141Z
Learning: In the open-cluster-management-io/lab repository, the fleetconfig-controller follows a workflow where chart version bumps (in README.md and values.yaml) are included in PRs before the corresponding Docker image exists. The Docker image is built and pushed automatically via GitHub release workflows after the PR is merged and tagged, making the referenced version available.

Applied to files:

fleetconfig-controller/hack/.versions.env
fleetconfig-controller/build/Dockerfile.base
fleetconfig-controller/build/Dockerfile.gke
fleetconfig-controller/build/Dockerfile.devspace
fleetconfig-controller/build/Dockerfile.eks

📚 Learning: 2025-09-22T18:42:03.404Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 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/build/Dockerfile.base
fleetconfig-controller/docs/addon-framework.md
fleetconfig-controller/build/Dockerfile.devspace
fleetconfig-controller/api/v1beta1/spoke_types.go
fleetconfig-controller/test/data/fleetconfig-values.yaml
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml

📚 Learning: 2025-09-25T23:31:11.630Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/charts/fleetconfig-controller/templates/ocm/fcc-addon/addon-template.yaml:110-112
Timestamp: 2025-09-25T23:31:11.630Z
Learning: The fleetconfig-controller-manager spoke agent requires create/update/patch/delete permissions on CustomResourceDefinitions because `clusteradm upgrade klusterlet` operations need create/update permissions and cleanup operations require delete permissions for proper lifecycle management.

Applied to files:

fleetconfig-controller/build/Dockerfile.base
fleetconfig-controller/test/data/fleetconfig-values.yaml
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml

📚 Learning: 2025-08-22T19:38:49.769Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 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.go
fleetconfig-controller/test/e2e/helper.go
fleetconfig-controller/test/data/fleetconfig-values.yaml
fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go
fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml

📚 Learning: 2025-09-22T19:16:34.109Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 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/api/v1beta1/spoke_types.go
fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml

📚 Learning: 2025-09-26T04:15:34.475Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/internal/webhook/v1beta1/validation_test.go:699-701
Timestamp: 2025-09-26T04:15:34.475Z
Learning: In fleetconfig-controller/internal/webhook/v1beta1/validation_test.go, the TestValidateAddonUniqueness test has flawed assertion logic at lines 763-769 that uses `if len(err.Error()) >= len(expectedMsg)` for partial matching, which causes false positives when actual error messages are longer than expected messages, allowing tests to pass with completely incorrect expected error messages.

Applied to files:

fleetconfig-controller/test/e2e/helper.go
fleetconfig-controller/internal/controller/v1beta1/addon.go
fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go

📚 Learning: 2025-09-26T04:13:12.146Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/internal/webhook/v1beta1/validation_test.go:719-724
Timestamp: 2025-09-26T04:13:12.146Z
Learning: The TestValidateAddonUniqueness function in fleetconfig-controller/internal/webhook/v1beta1/validation_test.go has flawed assertion logic that uses `len(err.Error()) >= len(expectedMsg)` to check error messages, which doesn't actually validate message content and will pass for any error longer than the expected message.

Applied to files:

fleetconfig-controller/test/e2e/helper.go
fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go

📚 Learning: 2025-09-25T23:26:18.327Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/cmd/manager/manager.go:290-292
Timestamp: 2025-09-25T23:26:18.327Z
Learning: ManagedClusterAddOn hub kubeconfigs in Open Cluster Management use certificate-authority-data (base64-encoded inline CA certificate) rather than certificate-authority (file path reference). This makes the kubeconfig self-contained and eliminates the need to patch CA file paths.

Applied to files:

fleetconfig-controller/test/e2e/helper.go
fleetconfig-controller/test/data/fleetconfig-values.yaml
fleetconfig-controller/internal/controller/v1beta1/addon.go
fleetconfig-controller/charts/fleetconfig-controller/crds/fleetconfig.open-cluster-management.io_spokes.yaml
fleetconfig-controller/test/data/addon-2-cm.yaml

📚 Learning: 2025-10-01T20:56:57.301Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/cmd/manager/manager.go:274-277
Timestamp: 2025-10-01T20:56:57.301Z
Learning: In fleetconfig-controller/cmd/manager/manager.go, the hub kubeconfig read by getHubRestConfig() is auto-generated and mounted with a consistent format, not user-supplied. The blanket string replacement for tls.crt and tls.key paths is suitable for this controlled environment.

Applied to files:

fleetconfig-controller/test/e2e/helper.go
fleetconfig-controller/test/data/fleetconfig-values.yaml

📚 Learning: 2025-09-22T19:26:11.020Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 59
File: fleetconfig-controller/test/data/fleetconfig-v1alpha1.yaml:47-53
Timestamp: 2025-09-22T19:26:11.020Z
Learning: In the open-cluster-management-io/lab repository's fleetconfig-controller tests, the kubeconfigKey is intentionally set to "value" in test fixtures (fleetconfig-v1alpha1.yaml, fleetconfig-values.yaml) because that's how the test harness provisions the kubeconfig secret during test setup. This differs from the chart default of "kubeconfig" but is correct for the test environment.

Applied to files:

fleetconfig-controller/test/data/fleetconfig-values.yaml

📚 Learning: 2025-09-12T20:53:55.600Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 59
File: fleetconfig-controller/internal/controller/v1beta1/addon.go:641-661
Timestamp: 2025-09-12T20:53:55.600Z
Learning: The waitForAddonManifestWorksCleanup function is called specifically during spoke cleanup/unjoin operations, only when addons were previously enabled, and after addon disable operations. In this controlled context, checking for zero total ManifestWorks is equivalent to checking for zero addon ManifestWorks since other ManifestWorks would have been handled earlier in the cleanup flow.

Applied to files:

fleetconfig-controller/internal/controller/v1beta1/addon.go
fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go

📚 Learning: 2025-09-12T20:53:55.600Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 59
File: fleetconfig-controller/internal/controller/v1beta1/addon.go:641-661
Timestamp: 2025-09-12T20:53:55.600Z
Learning: In the fleetconfig-controller addon cleanup flow, waitForAddonManifestWorksCleanup is only called in contexts where non-addon ManifestWorks have already been handled, so checking for zero total ManifestWorks is equivalent to checking for zero addon ManifestWorks.

Applied to files:

fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go

📚 Learning: 2025-09-25T23:18:41.573Z

Learnt from: arturshadnik
Repo: open-cluster-management-io/lab PR: 69
File: fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go:125-133
Timestamp: 2025-09-25T23:18:41.573Z
Learning: In the fleetconfig-controller spoke deletion flow, SpokeCleanupFinalizer is always removed before HubCleanupFinalizer. This means that checking for the existence of HubCleanupFinalizer in the deletion logic is sufficient regardless of cluster type, as any SpokeCleanupFinalizer would have already been removed by the time the hub cleanup runs.

Applied to files:

fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go

🧬 Code graph analysis (4)

fleetconfig-controller/test/e2e/helper.go (1)

fleetconfig-controller/test/utils/utils.go (2)

WarnError (72-75)

Info (66-69)

fleetconfig-controller/internal/controller/v1beta1/addon.go (5)

fleetconfig-controller/api/v1beta1/constants.go (2)

ManagedBySelector (208-208)

AnnotationAddOnDeploymentConfigHash (169-169)

fleetconfig-controller/api/v1beta1/spoke_types.go (1)

AddOn (278-291)

fleetconfig-controller/internal/hash/hash.go (1)

ComputeHash (11-22)

fleetconfig-controller/internal/args/args.go (1)

SanitizeArgs (82-84)

fleetconfig-controller/internal/exec/exec.go (1)

CmdWithLogs (18-54)

fleetconfig-controller/internal/controller/v1beta1/spoke_handler.go (3)

fleetconfig-controller/api/v1beta1/constants.go (7)

AddonsConfigured (22-22)

InstanceTypeUnified (89-89)

FCCAddOnName (116-116)

ControllerNamespaceEnvVar (107-107)

HubNamespaceEnvVar (104-104)

SpokeNamespaceEnvVar (101-101)

PurgeAgentNamespaceEnvVar (113-113)

fleetconfig-controller/internal/controller/v1beta1/spoke_controller.go (1)

SpokeReconciler (52-58)

fleetconfig-controller/api/v1beta1/spoke_types.go (3)

Spoke (319-333)

HubRef (119-127)

CleanupConfig (92-116)

fleetconfig-controller/test/e2e/v1beta1_hub_spoke.go (1)

fleetconfig-controller/test/utils/utils.go (1)

WarnError (72-75)

🪛 LanguageTool

fleetconfig-controller/docs/addon-framework.md

[grammar] ~5-~5: Ensure spelling is correct
Context: ...g clusteradm commands, you can define addons in your Hub and Spoke resources, and FC...