Stop using kots to install the app #3257
Conversation
|
This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID. Online Installer: Airgap Installer (may take a few minutes before the airgap bundle is built): Happy debugging! |
sgalsaleh
force-pushed
the
stop-using-kots
branch
2 times, most recently
from
d130bdc to
1b48b5c
Compare
sgalsaleh
force-pushed
the
stop-using-kots
branch
from
1b48b5c to
ca9ab68
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| return fmt.Errorf("create metadata client: %w", err) | ||
| } | ||
| m.mcli = mcli | ||
| } |
There was a problem hiding this comment.
Bug: Potential nil pointer dereference in namespace reconciler
In setupClients(), mcli (metadata client) is only created when restClientGetter != nil. If kubernetesEnvSettings is nil, mcli remains nil. Later, when hostCABundlePath is non-empty, ensureCAConfigmap calls adminconsole.EnsureCAConfigmap which uses mcli.Resource(...) without a nil check. This will cause a nil pointer dereference panic. The ensureCAConfigmap function checks hostCABundlePath but not mcli.
Additional Locations (1)
| // Install the app | ||
| err := c.appInstallManager.Install(ctx, configValues) | ||
| // Install the app with installable charts | ||
| err = c.appInstallManager.Install(ctx, installableCharts, appConfigValues, opts.RegistrySettings, opts.HostCABundlePath) |
There was a problem hiding this comment.
Bug: Data race from closure capturing outer err variable
The goroutine at line 115 uses assignment err = ... instead of short declaration err := ..., causing the closure to capture and modify the err variable from the outer InstallApp function scope. This creates a data race since the outer function returns immediately after starting the goroutine (at line 131), while the goroutine continues to write to the captured err variable. This is inconsistent with the pattern used in upgrade.go (line 74) which correctly uses err := to declare a local variable within the goroutine.
| Archive: []byte("chart-archive-data"), | ||
| Values: map[string]any{"key": "value"}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Bug: Test creates InstallableHelmChart with nil CR field
The test data creates InstallableHelmChart structs without setting the CR field, leaving it nil. While this test uses mocks and doesn't exercise the real installHelmChart code path, the getChartDisplayName function calls chart.CR.GetName() without nil checking. If a chart with a nil CR were passed to the real install function, it would panic. The production ExtractInstallableHelmCharts always sets CR, but the test data doesn't reflect this contract.
| Values: map[string]kotsv1beta1.ConfigValue{}, | ||
| }, | ||
| configValues: types.AppConfigValues{ | ||
| "key1": {Value: "value1"}, |
There was a problem hiding this comment.
Bug: Test for empty config values uses non-empty values
The test case named "handles empty config values" was incorrectly refactored and now passes non-empty config values ("key1": {Value: "value1"}). The original test used an empty map to verify the code handles empty config values correctly. The comment at line 826 still says "Get and verify secret was created even with empty values", confirming the test intent was to test empty values. This test regression means empty config value handling is no longer tested, potentially masking bugs in that code path.
| func (r *NamespaceReconciler) reconcileNamespace(ctx context.Context, namespace string) error { | ||
| // Create namespace if it doesn't exist | ||
| ns := &corev1.Namespace{} | ||
| err := r.kcli.Get(ctx, client.ObjectKey{Name: namespace}, ns) |
There was a problem hiding this comment.
if you pass the context into kube client functions and cancel it could cause operations to be shutdown abruptly. it seems like the way you are using this you will need a way to gracefully shutdown the reconciler.
| if err != nil { | ||
| return fmt.Errorf("start namespace reconciler: %w", err) | ||
| } | ||
| defer nsReconciler.Stop() |
There was a problem hiding this comment.
this feels like a race condition that will cancel the reconciler abruptly without allowing it to finish its operations.
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| r.reconcile(ctx) |
There was a problem hiding this comment.
should you pass a background context here to allow for graceful shutdowns?
| // 2. Immediately ensures image pull secrets and other resources in all watched namespaces | ||
| // 3. Starts background polling to reconcile namespaces periodically | ||
| // Returns a cancellable namespace reconciler instance. | ||
| func runNamespaceReconciler( |
There was a problem hiding this comment.
if you split this up into two functions, one to instantiate it and another to run it that runs in the foreground you could easily gracefully shut it down in the caller.
| r.logger.WithError(err).Warn("failed to list namespaces") | ||
| return | ||
| } | ||
| namespaces = make([]string, 0, len(nsList.Items)) |
There was a problem hiding this comment.
what if a namespace is not yet created that is in the watchedNamespaces list? it will not get created with its image pull secret and config map. would it be better to make this a set and append listed namespaces? it seems like there is a different behavior whether or not there is a * here in the list and im not sure which is correct but i think we should make it consistent.
| secret = &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: r.registrySettings.ImagePullSecretName, | ||
| Namespace: namespace, |
There was a problem hiding this comment.
Should we be consistent in our labels so we can keep track of things we install? How about a utility function?
| r.logger.Infof(format, args...) | ||
| } | ||
|
|
||
| return adminconsole.EnsureCAConfigmap(ctx, addonstypes.LogFunc(logFn), r.kcli, r.mcli, namespace, r.hostCABundlePath) |
There was a problem hiding this comment.
couldnt you pass r.logger.Infof here directly as an argument?
| } | ||
| } | ||
|
|
||
| func TestCalculateRegistrySettings(t *testing.T) { |
There was a problem hiding this comment.
Is a test missing for GetRegistrySettings?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Does this PR require a test?
Does this PR require a release note?
Does this PR require documentation?