chore: scaffold operator module with controller-runtime and helm v3#416
chore: scaffold operator module with controller-runtime and helm v3#416bhuvan-somisetty wants to merge 3 commits into
Conversation
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new operator module: kubebuilder-style Satellite CRD and deep-copy methods, a SatelliteReconciler that installs/upgrades a Helm chart, an operator main entrypoint with controller-runtime wiring, module dependency declarations, and build task entries. ChangesKubernetes Fleet Operator Module
Sequence Diagram(s)sequenceDiagram
participant Manager
participant Reconciler
participant HelmConfig
participant HelmAction
Manager->>Reconciler: Reconcile(ctx, Request)
Reconciler->>Reconciler: Get Satellite object
Reconciler->>HelmConfig: Init ActionConfig (with logger)
Reconciler->>HelmAction: History(Max=1)
alt release exists
Reconciler->>HelmAction: Upgrade release (deploy/helm/satellite)
else no release
Reconciler->>HelmAction: Install release (chart or remote when GroundControlURL set)
end
HelmAction-->>Reconciler: result/error
Reconciler-->>Manager: return ctrl.Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 41 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@operator/api/v1alpha1/groupversion_info.go`:
- Around line 9-20: Replace the package-level globals and the init function with
explicit registration functions: remove GroupVersion, SchemeBuilder, Scheme and
init, add an exported function (e.g., RegisterSatelliteTypes(s *runtime.Scheme)
error or NewSatelliteScheme() (*runtime.Scheme, error)) that constructs
schema.GroupVersion, creates a scheme.Builder for that GroupVersion, registers
&Satellite and &SatelliteList, and calls AddToScheme on the provided or newly
created *runtime.Scheme; return any error instead of panicking so callers (main)
can handle wiring and logging. Ensure the function name references are easy to
find (e.g., RegisterSatelliteTypes, NewSatelliteScheme) and update main to call
that function to perform registration at startup.
In `@operator/go.mod`:
- Line 3: The go directive currently pins the module to an affected runtime ("go
1.24"); update the go directive in go.mod to at least "go 1.26.2" to pick up
stdlib fixes for CVE-2025-68121 and CVE-2026-32282, and optionally add a
"toolchain go 1.26.2" directive to pin the toolchain used by builders; ensure
you replace the existing "go 1.24" entry and commit the updated go.mod.
- Around line 26-27: Bump the vulnerable transitive modules in the operator
module: update github.com/containerd/containerd to v1.7.29 and
golang.org/x/oauth2 to v0.27.0 (these are the exact module paths referenced in
go.mod) and then refresh the module graph so the new versions are applied (e.g.,
run the module upgrade/resolve flow and tidy the module cache). Ensure the
go.mod entries are updated to those patched versions and that go.sum is
regenerated so builds use the patched releases.
In `@operator/internal/controller/satellite_controller.go`:
- Around line 57-63: The code leaves chartRef empty when
sat.Spec.GroundControlURL is blank which causes loader.Load(chartRef) to fail;
change the logic so chartRef is always set to a deterministic default (e.g.
"deploy/helm/satellite") and only override it if a different path is required by
sat.Spec.GroundControlURL; update the handling around chartRef (the variable and
the call to loader.Load) to use this default value unconditionally or derive it
via a helper constant so loader.Load never receives an empty string.
- Around line 36-38: cfg.Init is being called with a nil RESTClientGetter which
breaks Helm runtime; replace the nil with a real
genericclioptions.RESTClientGetter injected into the reconciler (created from
in-cluster config or via genericclioptions.NewConfigFlags(false) wired from
operator/cmd/main.go). Modify the reconciler to accept and store a
RESTClientGetter field, populate that field when building the controller in
main, and pass that field into cfg.Init instead of nil so Helm can create a
working kube client during install/upgrade operations.
- Around line 59-60: The hardcoded chartRef = "deploy/helm/satellite" points to
a missing Helm chart and will break loader.Load() calls in installRelease() /
upgradeRelease(); fix by either adding a valid Helm chart under
deploy/helm/satellite (Chart.yaml, templates/, values.yaml, etc.) so
loader.Load(chartRef) succeeds, or change the controller to use an embedded
chart or external source (OCI/remote Helm repo) and update chartRef and the code
paths that call loader.Load() to use the appropriate loader (e.g., load from
embedded FS or use chart pulled from OCI/HTTP) so installRelease() and
upgradeRelease() no longer rely on a nonexistent local path.
- Around line 45-47: The current logic treats any error from
histClient.Run(releaseName) as "release not found" and calls
r.installRelease(cfg, sat); change it to inspect the returned error and only
call r.installRelease when the error equals driver.ErrReleaseNotFound (or
errors.Is(err, driver.ErrReleaseNotFound)); for any other error (e.g.,
auth/API/storage failures) return that error up the stack instead of installing.
Keep the call sites histClient.Run(releaseName) and r.installRelease(cfg, sat)
unchanged except for the conditional branching and use errors.Is to handle
wrapped errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00a24ad6-540b-4a35-9d16-64f28d40ae45
⛔ Files ignored due to path filters (1)
operator/go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Taskfile.ymloperator/api/v1alpha1/groupversion_info.gooperator/api/v1alpha1/satellite_types.gooperator/api/v1alpha1/zz_generated_deepcopy.gooperator/cmd/main.gooperator/go.modoperator/internal/controller/satellite_controller.gotaskfiles/build.yml
| var ( | ||
| GroupVersion = schema.GroupVersion{Group: "fleet.harbor-satellite.io", Version: "v1alpha1"} | ||
| SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion} | ||
| Scheme = runtime.NewScheme() | ||
| ) | ||
|
|
||
| func init() { //nolint:gochecknoinits | ||
| SchemeBuilder.Register(&Satellite{}, &SatelliteList{}) | ||
| if err := SchemeBuilder.AddToScheme(Scheme); err != nil { | ||
| panic(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactor away package globals + init() to satisfy enforced lint policy.
GroupVersion, SchemeBuilder, Scheme, and init() violate the repository’s strict lint contract and are currently bypassed with nolint. Please expose explicit constructor/registration functions and wire them from main instead of relying on init-time side effects.
As per coding guidelines, “Enforce strict linting standards via golangci-lint with 50+ linters including no global variables (gochecknoglobals), no init functions (gochecknoinits)”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/api/v1alpha1/groupversion_info.go` around lines 9 - 20, Replace the
package-level globals and the init function with explicit registration
functions: remove GroupVersion, SchemeBuilder, Scheme and init, add an exported
function (e.g., RegisterSatelliteTypes(s *runtime.Scheme) error or
NewSatelliteScheme() (*runtime.Scheme, error)) that constructs
schema.GroupVersion, creates a scheme.Builder for that GroupVersion, registers
&Satellite and &SatelliteList, and calls AddToScheme on the provided or newly
created *runtime.Scheme; return any error instead of panicking so callers (main)
can handle wiring and logging. Ensure the function name references are easy to
find (e.g., RegisterSatelliteTypes, NewSatelliteScheme) and update main to call
that function to perform registration at startup.
| if err := cfg.Init(nil, sat.Namespace, r.HelmDriver, func(format string, v ...any) { | ||
| ctrl.Log.Info(fmt.Sprintf(format, v...)) | ||
| }); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Helm v3 action.Configuration.Init, is passing a nil RESTClientGetter supported for in-cluster install/upgrade operations?
💡 Result:
No—passing a nil RESTClientGetter to helm v3 action.Configuration.Init is not supported for in-cluster install/upgrade operations. Evidence and reasoning: 1) helm’s Configuration.Init expects a non-nil RESTClientGetter interface implementation. The Init signature takes a RESTClientGetter value and the Helm CLI passes a concrete settings.RESTClientGetter implementation into Init (not nil) during normal operations [1]. 2) Helm’s action code uses i.cfg.RESTClientGetter methods (e.g., ToDiscoveryClient/ToRESTMapper) during install/upgrade flows. Helm had a recent change explicitly to “check nil for restClientGetter” before calling those methods, implying nil previously caused failures and is only conditionally handled in specific areas rather than being a supported contract [2]. 3) Community attempts that build in-cluster configs still provide a concrete RESTClientGetter (e.g., via a ConfigFlags instance populated from rest.InClusterConfig) rather than nil, which aligns with the above expectation [3]. 4) A concrete workaround for in-cluster usage is to create a RESTClientGetter from in-cluster rest.Config (or kubeconfig bytes) and pass that into actionConfig.Init; e.g., using an implementation like genericclioptions.ConfigFlags populated from in-cluster config, then calling actionConfig.Init(kubeConfigGetter, namespace,...) [3]. Practical takeaway: - For in-cluster install/upgrade, do not pass nil. - Create a RESTClientGetter implementation backed by the in-cluster auth (commonly by populating genericclioptions.ConfigFlags from rest.InClusterConfig) and pass that to action.Configuration.Init. Confidence: high (grounded in Helm code usage patterns and an explicit Helm fix for nil handling).
Citations:
- 1: https://github.com/helm/helm/blob/340b06d8/pkg/action/action.go
- 2: helm/helm@c15e711
- 3: Helm 3.0.2 - generating an action.Configuration for in-cluster runtime helm/helm#7430
cfg.Init is called with a nil Kubernetes REST client getter—this will fail at runtime.
Helm's action.Configuration.Init requires a non-nil RESTClientGetter interface implementation. Passing nil prevents Helm from creating a working kube client; it will call methods on this getter during install/upgrade operations. Inject a real genericclioptions.RESTClientGetter into the reconciler (populated from in-cluster config) and pass it to cfg.Init.
Suggested direction
type SatelliteReconciler struct {
client.Client
HelmDriver string
+ RESTClientGetter genericclioptions.RESTClientGetter
}
- if err := cfg.Init(nil, sat.Namespace, r.HelmDriver, func(format string, v ...any) {
+ if err := cfg.Init(r.RESTClientGetter, sat.Namespace, r.HelmDriver, func(format string, v ...any) {Also wire RESTClientGetter from operator/cmd/main.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/internal/controller/satellite_controller.go` around lines 36 - 38,
cfg.Init is being called with a nil RESTClientGetter which breaks Helm runtime;
replace the nil with a real genericclioptions.RESTClientGetter injected into the
reconciler (created from in-cluster config or via
genericclioptions.NewConfigFlags(false) wired from operator/cmd/main.go). Modify
the reconciler to accept and store a RESTClientGetter field, populate that field
when building the controller in main, and pass that field into cfg.Init instead
of nil so Helm can create a working kube client during install/upgrade
operations.
| if _, err := histClient.Run(releaseName); err != nil { | ||
| return r.installRelease(cfg, sat) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What error does Helm v3 history return when a release does not exist, and is driver.ErrReleaseNotFound the recommended check?
💡 Result:
Helm v3 helm history <release> when the release does not exist ultimately corresponds to Helm’s storage sentinel error driver.ErrReleaseNotFound, whose message is release: not found [1]. Is driver.ErrReleaseNotFound the recommended check? Yes for code that uses the Helm storage layer: driver.ErrReleaseNotFound is explicitly defined as errors.New("release: not found") and is intended to indicate that a release is not found [1]. Helm’s storage History method is documented to return driver.ErrReleaseNotFound when no such release name exists [2], and the Storage.History action implementation returns h.cfg.Releases.History(name) (so callers can rely on that error) [3]. Note: at the CLI level, Helm commands may surface slightly different wrapper messages depending on which action you ran (e.g., other commands can print Error: release: not found in practice, but that’s still the same underlying sentinel) [4].
Citations:
- 1: https://github.com/helm/helm/blob/master/pkg/storage/driver/driver.go
- 2: https://github.com/helm/helm/blob/340b06d8/pkg/storage/storage.go
- 3: https://github.com/helm/helm/blob/master/pkg/action/history.go
- 4: https://stackoverflow.com/questions/78405082/error-release-not-found-when-trying-to-rollback-existing-helm-release
🏁 Script executed:
# First, find the file
find . -name "satellite_controller.go" -type fRepository: container-registry/harbor-satellite
Length of output: 134
🏁 Script executed:
# Read the file to see the current code and context
cat -n operator/internal/controller/satellite_controller.go | head -60Repository: container-registry/harbor-satellite
Length of output: 2204
🏁 Script executed:
# Check the imports in the file
grep -n "^import" operator/internal/controller/satellite_controller.go -A 20Repository: container-registry/harbor-satellite
Length of output: 747
🏁 Script executed:
# Check if go.mod exists and contains helm.sh/helm/v3
cat go.mod | grep helm.shRepository: container-registry/harbor-satellite
Length of output: 116
🏁 Script executed:
# Check for other error handling patterns in the file
grep -n "errors.Is\|driver\." operator/internal/controller/satellite_controller.goRepository: container-registry/harbor-satellite
Length of output: 61
🏁 Script executed:
# Look at the rest of the file to see the installRelease and upgradeRelease methods
tail -50 operator/internal/controller/satellite_controller.goRepository: container-registry/harbor-satellite
Length of output: 1400
Check for specific driver.ErrReleaseNotFound instead of treating all history errors as "release not found."
Line 45 currently routes all histClient.Run() errors to install. Only ErrReleaseNotFound should trigger install; other errors (auth failures, API issues, storage errors) must be returned to avoid masking real problems.
Targeted fix
+import "errors"
+import "helm.sh/helm/v3/pkg/storage/driver"
if _, err := histClient.Run(releaseName); err != nil {
- return r.installRelease(cfg, sat)
+ if errors.Is(err, driver.ErrReleaseNotFound) {
+ return r.installRelease(cfg, sat)
+ }
+ return fmt.Errorf("helm history: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err := histClient.Run(releaseName); err != nil { | |
| return r.installRelease(cfg, sat) | |
| } | |
| if _, err := histClient.Run(releaseName); err != nil { | |
| if errors.Is(err, driver.ErrReleaseNotFound) { | |
| return r.installRelease(cfg, sat) | |
| } | |
| return fmt.Errorf("helm history: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/internal/controller/satellite_controller.go` around lines 45 - 47,
The current logic treats any error from histClient.Run(releaseName) as "release
not found" and calls r.installRelease(cfg, sat); change it to inspect the
returned error and only call r.installRelease when the error equals
driver.ErrReleaseNotFound (or errors.Is(err, driver.ErrReleaseNotFound)); for
any other error (e.g., auth/API/storage failures) return that error up the stack
instead of installing. Keep the call sites histClient.Run(releaseName) and
r.installRelease(cfg, sat) unchanged except for the conditional branching and
use errors.Is to handle wrapped errors.
| chartRef := "" | ||
| if sat.Spec.GroundControlURL != "" { | ||
| chartRef = "deploy/helm/satellite" | ||
| } | ||
|
|
||
| ch, err := loader.Load(chartRef) | ||
| if err != nil { |
There was a problem hiding this comment.
chartRef can remain empty and guarantees install failure.
If sat.Spec.GroundControlURL is empty, chartRef stays "" and loader.Load(chartRef) fails. Set a deterministic default chart reference regardless of optional spec fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/internal/controller/satellite_controller.go` around lines 57 - 63,
The code leaves chartRef empty when sat.Spec.GroundControlURL is blank which
causes loader.Load(chartRef) to fail; change the logic so chartRef is always set
to a deterministic default (e.g. "deploy/helm/satellite") and only override it
if a different path is required by sat.Spec.GroundControlURL; update the
handling around chartRef (the variable and the call to loader.Load) to use this
default value unconditionally or derive it via a helper constant so loader.Load
never receives an empty string.
| chartRef = "deploy/helm/satellite" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether deployment/build assets package the chart path used by the controller.
fd -i 'dockerfile|containerfile|helm|chart' -t f
rg -n -C2 'deploy/helm/satellite|COPY .*deploy/helm/satellite|ADD .*deploy/helm/satellite|operator' $(fd -i 'dockerfile|containerfile' -t f || true)
rg -n -C2 'deploy/helm/satellite'Repository: container-registry/harbor-satellite
Length of output: 154
🏁 Script executed:
# Check if deploy/helm/satellite directory exists
ls -la deploy/helm/ 2>/dev/null || echo "deploy/helm directory not found"
# Check the structure around the satellite directory
find . -name "satellite" -type d 2>/dev/null | head -20
# Check if there's any helm packaging or tarballs
find . -name "*.tar.gz" -o -name "*.tgz" 2>/dev/null | grep -i satellite || echo "No satellite tarballs found"Repository: container-registry/harbor-satellite
Length of output: 300
🏁 Script executed:
# Examine the satellite_controller.go file to see the exact context
head -100 operator/internal/controller/satellite_controller.go | tail -50Repository: container-registry/harbor-satellite
Length of output: 1400
🏁 Script executed:
# Check the Dockerfile to see what gets copied/bundled
cat DockerfileRepository: container-registry/harbor-satellite
Length of output: 801
🏁 Script executed:
# Search for any helm deployment configurations or references to satellite chart
rg -n "satellite" Dockerfile* || echo "No satellite references in Dockerfiles"Repository: container-registry/harbor-satellite
Length of output: 230
🏁 Script executed:
# Check for git submodules
cat .gitmodules 2>/dev/null || echo "No .gitmodules file"
# Check for any helm repository configurations or OCI references
rg -n "oci://|helm repo|satellite" operator/internal/controller/satellite_controller.go
# Look for any chart embedding or build-time generation
rg -n "embed|//go:embed|chart|satellite" operator/internal/controller/ -A 2 -B 2
# Check if there's any setup/build documentation
find . -name "*.md" -o -name "Makefile" -o -name "*.sh" | xargs grep -l "deploy/helm/satellite" 2>/dev/null || echo "No build docs found"Repository: container-registry/harbor-satellite
Length of output: 2486
🏁 Script executed:
# Check the internal/satellite directory to see what it contains
ls -la internal/satellite/ 2>/dev/null || echo "Directory not found"
find internal/satellite -type f | head -20Repository: container-registry/harbor-satellite
Length of output: 287
Chart directory deploy/helm/satellite does not exist in the repository and will cause runtime failures.
The controller attempts to load a Helm chart from the hardcoded path deploy/helm/satellite (lines 59-60, 77), but this directory is not present in the repository. The Dockerfile copies the entire codebase with COPY . ., so the missing chart will not be available in the container. Any call to installRelease() or upgradeRelease() will fail when loader.Load() attempts to open the nonexistent chart directory.
Create the Helm chart at deploy/helm/satellite/ or refactor to use an embedded chart, OCI registry source, or remote Helm repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@operator/internal/controller/satellite_controller.go` around lines 59 - 60,
The hardcoded chartRef = "deploy/helm/satellite" points to a missing Helm chart
and will break loader.Load() calls in installRelease() / upgradeRelease(); fix
by either adding a valid Helm chart under deploy/helm/satellite (Chart.yaml,
templates/, values.yaml, etc.) so loader.Load(chartRef) succeeds, or change the
controller to use an embedded chart or external source (OCI/remote Helm repo)
and update chartRef and the code paths that call loader.Load() to use the
appropriate loader (e.g., load from embedded FS or use chart pulled from
OCI/HTTP) so installRelease() and upgradeRelease() no longer rely on a
nonexistent local path.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="operator/internal/controller/satellite_controller.go">
<violation number="1" location="operator/internal/controller/satellite_controller.go:36">
P0: `cfg.Init(nil, ...)` will panic at runtime. Helm's `action.Configuration.Init` requires a non-nil `RESTClientGetter` — it immediately passes the getter to `kube.New(getter)` which dereferences it. Inject a real `genericclioptions.RESTClientGetter` (e.g. built from `rest.InClusterConfig()`) into the reconciler and pass it here.</violation>
<violation number="2" location="operator/internal/controller/satellite_controller.go:45">
P2: Treating every `History.Run` error as "release missing" can wrongly install on RBAC/network/storage failures instead of only on `ErrReleaseNotFound`.</violation>
<violation number="3" location="operator/internal/controller/satellite_controller.go:57">
P1: Install can pass an empty chart reference when `groundControlURL` is unset, causing chart loading to fail for that valid spec state.</violation>
<violation number="4" location="operator/internal/controller/satellite_controller.go:77">
P1: Hardcoding a repo-relative Helm chart path that is neither present in the repo nor copied into the runtime image will make reconcile fail in deployment.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| func (r *SatelliteReconciler) reconcileHelmRelease(_ context.Context, sat *fleetv1alpha1.Satellite) error { | ||
| cfg := new(action.Configuration) | ||
| if err := cfg.Init(nil, sat.Namespace, r.HelmDriver, func(format string, v ...any) { |
There was a problem hiding this comment.
P0: cfg.Init(nil, ...) will panic at runtime. Helm's action.Configuration.Init requires a non-nil RESTClientGetter — it immediately passes the getter to kube.New(getter) which dereferences it. Inject a real genericclioptions.RESTClientGetter (e.g. built from rest.InClusterConfig()) into the reconciler and pass it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At operator/internal/controller/satellite_controller.go, line 36:
<comment>`cfg.Init(nil, ...)` will panic at runtime. Helm's `action.Configuration.Init` requires a non-nil `RESTClientGetter` — it immediately passes the getter to `kube.New(getter)` which dereferences it. Inject a real `genericclioptions.RESTClientGetter` (e.g. built from `rest.InClusterConfig()`) into the reconciler and pass it here.</comment>
<file context>
@@ -0,0 +1,92 @@
+
+func (r *SatelliteReconciler) reconcileHelmRelease(_ context.Context, sat *fleetv1alpha1.Satellite) error {
+ cfg := new(action.Configuration)
+ if err := cfg.Init(nil, sat.Namespace, r.HelmDriver, func(format string, v ...any) {
+ ctrl.Log.Info(fmt.Sprintf(format, v...))
+ }); err != nil {
</file context>
| install.Namespace = sat.Namespace | ||
| install.CreateNamespace = true | ||
|
|
||
| chartRef := "" |
There was a problem hiding this comment.
P1: Install can pass an empty chart reference when groundControlURL is unset, causing chart loading to fail for that valid spec state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At operator/internal/controller/satellite_controller.go, line 57:
<comment>Install can pass an empty chart reference when `groundControlURL` is unset, causing chart loading to fail for that valid spec state.</comment>
<file context>
@@ -0,0 +1,92 @@
+ install.Namespace = sat.Namespace
+ install.CreateNamespace = true
+
+ chartRef := ""
+ if sat.Spec.GroundControlURL != "" {
+ chartRef = "deploy/helm/satellite"
</file context>
| @@ -0,0 +1,92 @@ | |||
| package controller | |||
There was a problem hiding this comment.
P1: Hardcoding a repo-relative Helm chart path that is neither present in the repo nor copied into the runtime image will make reconcile fail in deployment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At operator/internal/controller/satellite_controller.go, line 77:
<comment>Hardcoding a repo-relative Helm chart path that is neither present in the repo nor copied into the runtime image will make reconcile fail in deployment.</comment>
<file context>
@@ -0,0 +1,92 @@
+ upgrade := action.NewUpgrade(cfg)
+ upgrade.Namespace = sat.Namespace
+
+ ch, err := loader.Load("deploy/helm/satellite")
+ if err != nil {
+ return fmt.Errorf("load chart: %w", err)
</file context>
| releaseName := sat.Name | ||
| histClient := action.NewHistory(cfg) | ||
| histClient.Max = 1 | ||
| if _, err := histClient.Run(releaseName); err != nil { |
There was a problem hiding this comment.
P2: Treating every History.Run error as "release missing" can wrongly install on RBAC/network/storage failures instead of only on ErrReleaseNotFound.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At operator/internal/controller/satellite_controller.go, line 45:
<comment>Treating every `History.Run` error as "release missing" can wrongly install on RBAC/network/storage failures instead of only on `ErrReleaseNotFound`.</comment>
<file context>
@@ -0,0 +1,92 @@
+ releaseName := sat.Name
+ histClient := action.NewHistory(cfg)
+ histClient.Max = 1
+ if _, err := histClient.Run(releaseName); err != nil {
+ return r.installRelease(cfg, sat)
+ }
</file context>
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
|
@bupd — following the scaffold in this PR, I went through the operator code carefully and found several issues that would cause runtime failures. I've pushed fixes on this branch: Critical fixes:
Other fixes:
The binary now builds cleanly with |
1 participant
Closes #412.
The LFX 2026 Term 2 operator deliverable (#375) needs a foundation to build on. Without a dedicated Go module for the operator, contributors have no defined starting point and will inevitably pick incompatible versions of controller-runtime and the Helm SDK.
This PR creates that foundation:
operator/is a new Go module (github.com/container-registry/harbor-satellite/operator) withsigs.k8s.io/controller-runtime v0.20.4andhelm.sh/helm/v3 v3.17.3as direct dependencies.operator/api/v1alpha1/definesSatelliteandSatelliteListCRD types (SatelliteSpec,SatelliteStatus) with the standardDeepCopy*methods required by the runtime scheme.operator/internal/controller/satellite_controller.gowires aSatelliteReconcilerthat uses the Helm action SDK (action.NewInstall/action.NewUpgrade) to install or upgrade the existing satellite Helm chart on each reconcile.operator/cmd/main.gobootstraps a controller-runtime manager with the fleet scheme registered.build-operatortask is added toTaskfile.yml(backed bytaskfiles/build.yml) so the binary can be built withtask build-operator. It is intentionally kept separate from the defaultbuildtask to avoid pulling operator dependencies into the existing CI matrix until the module is further along.All three binaries (satellite, ground-control, operator) compile cleanly. Existing CI jobs are unaffected.
Signed-off-by: bhuvan-somisetty somisettybhuvan5@gmail.com
Summary by CodeRabbit
New Features
Chores