fix: --buildkit-container-name was ignored when --no-buildkit-update …

…was set (#4132)

fixes earthly/earthly#4122

Signed-off-by: Alex Couture-Beil <[email protected]>

buildkitd/buildkitd.go

@@ -289,13 +289,19 @@ func maybeRestart(ctx context.Context, console conslogging.ConsoleLogger, image,
 		if err != nil {
 			return nil, nil, errors.Wrap(err, "could not get settings hash")
 		}
-		ok, err := settings.VerifyHash(hash)
+		hashOK, err := settings.VerifyHash(hash)
 		if err != nil {
 			return nil, nil, errors.Wrap(err, "verify hash")
 		}
-		if ok {
-			// No need to replace: images are the same and settings are the same.
+		useExistingContainer := false
+		if hashOK {
 			bkCons.VerbosePrintf("Settings hashes match (%q), no restart required\n", hash)
+			useExistingContainer = true
+		} else if settings.NoUpdate {
+			bkCons.Warnf("Settings do not match; however restart was inhibited. This may cause unexpected issues, proceed with caution.\n")
+			useExistingContainer = true
+		}
+		if useExistingContainer {
 			info, workerInfo, err := checkConnection(ctx, settings.BuildkitAddress, 5*time.Second, opts...)
 			if err != nil {
 				return nil, nil, errors.Wrap(err, "could not connect to buildkitd to shut down container")
@@ -305,7 +311,7 @@ func maybeRestart(ctx context.Context, console conslogging.ConsoleLogger, image,
 		bkCons.Printf("Settings do not match. Restarting buildkit daemon with updated settings...\n")
 	} else {
 		if settings.NoUpdate {
-			bkCons.Printf("Updated image available. But update was inhibited.\n")
+			bkCons.Printf("Updated image available; however update was inhibited.\n")
 			info, workerInfo, err := checkConnection(ctx, settings.BuildkitAddress, 5*time.Second, opts...)
 			if err != nil {
 				return nil, nil, errors.Wrap(err, "could not verify connection to buildkitd container")
@@ -556,6 +562,14 @@ func IsStarted(ctx context.Context, containerName string, fe containerutil.Conta
 
 // WaitUntilStarted waits until the buildkitd daemon has started and is healthy.
 func WaitUntilStarted(ctx context.Context, console conslogging.ConsoleLogger, containerName, volumeName, address string, opTimeout time.Duration, fe containerutil.ContainerFrontend, opts ...client.ClientOpt) (*client.Info, *client.WorkerInfo, error) {
+	// Check that containerName and address match when address connects over the docker-container:// scheme
+	if strings.HasPrefix(address, containerutil.DockerSchemePrefix) {
+		expectedAddress := containerutil.DockerSchemePrefix + containerName
+		if address != expectedAddress {
+			// This shouldn't happen unless there's a programming error
+			return nil, nil, errors.Errorf("expected address to be %s, but got %s", expectedAddress, address)
+		}
+	}
 	// First, wait for the container to be marked as started.
 	ctxTimeout, cancel := context.WithTimeout(ctx, opTimeout)
 	defer cancel()

cmd/earthly/app/before.go

@@ -130,7 +130,7 @@ func (app *EarthlyApp) parseFrontend(cliCtx *cli.Context, cfg *config.Config) er
 		BuildkitHostCLIValue:       app.BaseCLI.Flags().BuildkitHost,
 		BuildkitHostFileValue:      app.BaseCLI.Cfg().Global.BuildkitHost,
 		LocalRegistryHostFileValue: app.BaseCLI.Cfg().Global.LocalRegistryHost,
-		InstallationName:           app.BaseCLI.Flags().InstallationName,
+		LocalContainerName:         app.BaseCLI.Flags().ContainerName,
 		DefaultPort:                8372 + config.PortOffset(app.BaseCLI.Flags().InstallationName),
 		Console:                    console,
 	}

util/containerutil/frontend.go

@@ -44,8 +44,8 @@ type FrontendConfig struct {
 
 	LocalRegistryHostFileValue string
 
-	InstallationName string
-	DefaultPort      int
+	LocalContainerName string
+	DefaultPort        int
 
 	Console conslogging.ConsoleLogger
 }

util/containerutil/settings_test.go

@@ -36,7 +36,7 @@ func TestBuildArgMatrix(t *testing.T) {
 			},
 			noopArgs,
 			results{
-				buildkit:      "docker-container://test-buildkitd",
+				buildkit:      "docker-container://test",
 				localRegistry: "",
 			},
 		},
@@ -122,7 +122,7 @@ func TestBuildArgMatrix(t *testing.T) {
 		logger = logger.WithWriter(&logs)
 
 		frontend, err := NewStubFrontend(ctx, &FrontendConfig{
-			InstallationName: "test-stub",
+			LocalContainerName: "test-stub",
 		})
 		assert.NoError(t, err)
 
@@ -133,7 +133,7 @@ func TestBuildArgMatrix(t *testing.T) {
 			BuildkitHostCLIValue:       tt.args.buildkit,
 			BuildkitHostFileValue:      tt.config.BuildkitHost,
 			LocalRegistryHostFileValue: tt.config.LocalRegistryHost,
-			InstallationName:           "test",
+			LocalContainerName:         "test",
 			DefaultPort:                8372,
 			Console:                    logger,
 		})
@@ -189,7 +189,7 @@ func TestBuildArgMatrixValidationFailures(t *testing.T) {
 		logger = logger.WithWriter(&logs)
 
 		frontend, err := NewStubFrontend(ctx, &FrontendConfig{
-			InstallationName: "test-stub",
+			LocalContainerName: "test-stub",
 		})
 		assert.NoError(t, err)
 
@@ -200,7 +200,7 @@ func TestBuildArgMatrixValidationFailures(t *testing.T) {
 			BuildkitHostFileValue:      tt.config.BuildkitHost,
 			LocalRegistryHostFileValue: tt.config.LocalRegistryHost,
 			Console:                    logger,
-			InstallationName:           "test",
+			LocalContainerName:         "test",
 			DefaultPort:                8372,
 		})
 		assert.ErrorIs(t, err, tt.expected)
@@ -290,7 +290,7 @@ func TestBuildArgMatrixValidationNonIssues(t *testing.T) {
 		logger = logger.WithWriter(&logs)
 
 		frontend, err := NewStubFrontend(ctx, &FrontendConfig{
-			InstallationName: "test-stub",
+			LocalContainerName: "test-stub",
 		})
 		assert.NoError(t, err)
 
@@ -301,7 +301,7 @@ func TestBuildArgMatrixValidationNonIssues(t *testing.T) {
 			BuildkitHostFileValue:      tt.config.BuildkitHost,
 			LocalRegistryHostFileValue: tt.config.LocalRegistryHost,
 			Console:                    logger,
-			InstallationName:           "test",
+			LocalContainerName:         "test",
 			DefaultPort:                8372,
 		})
 		assert.NoError(t, err)

util/containerutil/shell_shared.go

@@ -358,7 +358,7 @@ func (sf *shellFrontend) setupAndValidateAddresses(feType string, cfg *FrontendC
 			calculatedBuildkitHost = cfg.BuildkitHostFileValue
 		} else {
 			var err error
-			calculatedBuildkitHost, err = DefaultAddressForSetting(feType, cfg.InstallationName, cfg.DefaultPort)
+			calculatedBuildkitHost, err = DefaultAddressForSetting(feType, cfg.LocalContainerName, cfg.DefaultPort)
 			if err != nil {
 				return nil, errors.Wrap(err, "could not validate default address")
 			}
@@ -394,16 +394,16 @@ func (sf *shellFrontend) setupAndValidateAddresses(feType string, cfg *FrontendC
 }
 
 // DefaultAddressForSetting returns an address (signifying the desired/default transport) for a given frontend specified by setting.
-func DefaultAddressForSetting(setting string, installationName string, defaultPort int) (string, error) {
+func DefaultAddressForSetting(setting string, localContainerName string, defaultPort int) (string, error) {
 	switch setting {
 	case FrontendDockerShell:
-		return fmt.Sprintf(DockerAddressFmt, installationName), nil
+		return DockerSchemePrefix + localContainerName, nil
 
 	case FrontendPodmanShell:
 		return fmt.Sprintf(TCPAddressFmt, defaultPort), nil // Right now, podman only works over TCP. There are weird errors when trying to use the provided helper from buildkit.
 
 	case FrontendStub:
-		return fmt.Sprintf(DockerAddressFmt, installationName), nil // Maintain old behavior
+		return DockerSchemePrefix + localContainerName, nil // Maintain old behavior
 	}
 
 	return "", fmt.Errorf("no default buildkit address for %s", setting)

util/containerutil/types.go

@@ -194,12 +194,8 @@ const (
 	// TCPAddressFmt is the address at which the daemon is available when using TCP.
 	TCPAddressFmt = "tcp://127.0.0.1:%d"
 
-	// DockerAddressFmt is the address at which the daemon is available when using a Docker Container directly
-	DockerAddressFmt = "docker-container://%s-buildkitd"
-
-	// PodmanAddressFmt is the address at which the daemon is available when using a Podman Container directly.
-	// Currently unused due to image export issues
-	PodmanAddressFmt = "podman-container://%s-buildkitd"
+	// DockerSchemePrefix is used to construct the buildkit address for local docker-based connections
+	DockerSchemePrefix = "docker-container://"
 
 	// SatelliteAddress is the remote address when using a Satellite to execute your builds remotely.
 	SatelliteAddress = "tcp://satellite.earthly.dev:8372"