chore: move proxy and network specs to runtime config #2296

emosbaugh · 2025-06-11T19:40:56Z

What this PR does / why we need it:

Add missing properties from the UI install config to the runtime config ahead of merging the two.

Related
replicatedhq/kots#5355

Which issue(s) this PR fixes:

Does this PR require a test?

Does this PR require a release note?

Does this PR require documentation?

github-actions · 2025-06-11T20:21:42Z

This PR has been released (on staging) and is available for download with a embedded-cluster-smoke-test-staging-app license ID.

Online Installer:

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci/appver-dev-2142cd1" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Airgap Installer (may take a few minutes before the airgap bundle is built):

curl "https://staging.replicated.app/embedded/embedded-cluster-smoke-test-staging-app/ci-airgap/appver-dev-2142cd1?airgap=true" -H "Authorization: $EC_SMOKE_TEST_LICENSE_ID" -o embedded-cluster-smoke-test-staging-app-ci.tgz

Happy debugging!

emosbaugh · 2025-06-12T22:55:52Z

[sc-124940]

JGAntunes

Some questions/suggestions.

JGAntunes · 2025-06-16T13:14:43Z

api/controllers/install/controller.go

+type EnvSetter interface {
+	Setenv(key string, val string) error
+}


Wondering if it would make sense for this interface, the respective struct implementation and the mock implementation to live in api/pkg/utils instead? 🤔

moved to the runtimeconfig package

JGAntunes · 2025-06-16T13:25:13Z

api/integration/install_test.go

@@ -162,6 +162,7 @@ func TestConfigureInstallation(t *testing.T) {
 			installController, err := install.NewInstallController(
 				install.WithHostUtils(tc.mockHostUtils),
 				install.WithRuntimeConfig(rc),
+				install.WithEnvSetter(&testEnvSetter{}),


Wondering if we're missing a set of assertions here that validate:

The environment variables are set

All these runtime config fields are updated accordingly -

embedded-cluster/api/controllers/install/installation.go

Lines 65 to 69 in 4672f1c

c.rc.SetDataDir(config.DataDirectory)

c.rc.SetLocalArtifactMirrorPort(config.LocalArtifactMirrorPort)

c.rc.SetAdminConsolePort(config.AdminConsolePort)

c.rc.SetProxySpec(proxy)

c.rc.SetNetworkSpec(networkSpec)

JGAntunes · 2025-06-16T13:47:15Z

cmd/installer/cli/join_printcommand.go

+			if os.Getuid() != 0 {
+				return fmt.Errorf("update command must be run as root")
+			}


Do we need this? If so the msg should be changed s/update/join.

JGAntunes · 2025-06-16T13:49:43Z

cmd/installer/cli/join_printcommand.go

+			var err error
+			rc, err = rcutil.GetRuntimeConfigFromCluster(ctx)
+			if err != nil {
+				return fmt.Errorf("failed to init runtime config from cluster: %w", err)
+			}
+
+			os.Setenv("TMPDIR", rc.EmbeddedClusterTmpSubDir())
+
+			return nil


Curious, is this only required now? Or was it already being done before but in a different place?

I think this is a bug if we dont have this. The previous will not work for a non-default data directory.

JGAntunes · 2025-06-16T13:57:42Z

api/internal/managers/installation/config_test.go

@@ -356,8 +356,6 @@ func TestConfigureHost(t *testing.T) {
 					hum.On("ConfigureHost", mock.Anything, hostutils.InitForInstallOptions{
 						LicenseFile:  "license.yaml",
 						AirgapBundle: "bundle.tar",
-						PodCIDR:      "10.0.0.0/16",
-						ServiceCIDR:  "10.1.0.0/16",
 					}).Return(nil),


Should we validate the RC param VS keeping the mock.Anything?

sgalsaleh · 2025-06-16T13:59:27Z

.github/workflows/ci.yaml

@@ -212,6 +212,9 @@ jobs:
        with:
          fetch-depth: 0

+      - name: Free up runner disk space
+        uses: ./.github/actions/free-disk-space


i've seen this action take ~10 minutes some times, have you seen the build fail due to low disk space?

sgalsaleh · 2025-06-16T14:01:43Z

api/controllers/install/controller_test.go

@@ -857,3 +867,15 @@ func WithInfraManager(infraManager infra.InfraManager) InstallControllerOption {
 		c.infraManager = infraManager
 	}
 }
+
+type testEnvSetter struct {


wonder if this should live in a utils package like Joao mentioned with a proper mock like the other interfaces

moved to the runtimeconfig package

sgalsaleh · 2025-06-16T14:02:30Z

api/controllers/install/installation.go

-	os.Setenv("KUBECONFIG", c.rc.PathToKubeConfig())
-	os.Setenv("TMPDIR", c.rc.EmbeddedClusterTmpSubDir())
+	_ = c.envSetter.Setenv("KUBECONFIG", c.rc.PathToKubeConfig())
+	_ = c.envSetter.Setenv("TMPDIR", c.rc.EmbeddedClusterTmpSubDir())


why not handle the errors?

(i know the old code didn't ,but that's probably a mistake if os.Setenv returns an error)

sgalsaleh · 2025-06-17T03:46:42Z

cmd/installer/cli/join_printcommand.go

+				return fmt.Errorf("failed to init runtime config from cluster: %w", err)
+			}
+
+			os.Setenv("TMPDIR", rc.EmbeddedClusterTmpSubDir())


Shoukd this use rc.Setenv?

I didnt use setenv in places where we didnt set both env vars. we can refactor later

emosbaugh added the type::chore label Jun 11, 2025

emosbaugh force-pushed the emosbaugh/20250611/more-in-runtime-config branch 2 times, most recently from 3a0c26e to f7b7c01 Compare June 12, 2025 18:45

emosbaugh requested a review from sgalsaleh June 12, 2025 20:53

emosbaugh marked this pull request as ready for review June 12, 2025 22:19

emosbaugh enabled auto-merge (squash) June 12, 2025 22:19

emosbaugh force-pushed the emosbaugh/20250611/more-in-runtime-config branch from 79c94f9 to 4672f1c Compare June 13, 2025 21:18

JGAntunes reviewed Jun 16, 2025

View reviewed changes

sgalsaleh reviewed Jun 16, 2025

View reviewed changes

JGAntunes mentioned this pull request Jun 16, 2025

chore(tests): adding tests to the host preflights logic #2302

Merged

chore: move proxy and network specs to runtime config

358c583

emosbaugh force-pushed the emosbaugh/20250611/more-in-runtime-config branch from 4672f1c to 358c583 Compare June 16, 2025 18:40

feedback

1b0b71b

emosbaugh requested review from sgalsaleh and JGAntunes June 16, 2025 23:23

emosbaugh added 2 commits June 16, 2025 19:18

race

1992aab

race

2142cd1

sgalsaleh approved these changes Jun 17, 2025

View reviewed changes

emosbaugh merged commit 2a86d34 into main Jun 17, 2025
68 checks passed

emosbaugh deleted the emosbaugh/20250611/more-in-runtime-config branch June 17, 2025 03:48

	c.rc.SetDataDir(config.DataDirectory)
	c.rc.SetLocalArtifactMirrorPort(config.LocalArtifactMirrorPort)
	c.rc.SetAdminConsolePort(config.AdminConsolePort)
	c.rc.SetProxySpec(proxy)
	c.rc.SetNetworkSpec(networkSpec)

chore: move proxy and network specs to runtime config #2296

chore: move proxy and network specs to runtime config #2296

Uh oh!

Conversation

emosbaugh commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

github-actions bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emosbaugh commented Jun 12, 2025

Uh oh!

JGAntunes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emosbaugh commented Jun 11, 2025 •

edited

Loading

github-actions bot commented Jun 11, 2025 •

edited

Loading