bug: add error handling for missing state lock config in S3, HTTP and OSS backends (#1977)

Signed-off-by: g0dfl3sh <alex1trendler@gmail.com>
Signed-off-by: Alexandru Trendler <117138249+g0dfl3sh@users.noreply.github.com>
Co-authored-by: Oleksandr Levchenkov <ollevche@gmail.com>

CHANGELOG.md

@@ -17,6 +17,9 @@ ENHANCEMENTS:
 * Added for-each support to providers. ([#300](https://github.com/opentofu/opentofu/issues/300))
 
 BUG FIXES:
+* Ensure that using a sensitive path for templatefile that it doesn't panic([#1801](https://github.com/opentofu/opentofu/issues/1801))
+* Fixed crash when module source is not present ([#1888](https://github.com/opentofu/opentofu/pull/1888))
+* Added error handling for `force-unlock` command when locking is disabled for S3, HTTP, and OSS backends. [#1977](https://github.com/opentofu/opentofu/pull/1977)
 * Ensured that using a sensitive path for templatefile that it doesn't panic([#1801](https://github.com/opentofu/opentofu/issues/1801))
 * Fixed a crash when module source is not present ([#1888](https://github.com/opentofu/opentofu/pull/1888))
 * Fixed a crash when importing an empty optional sensitive string ([#1986](https://github.com/opentofu/opentofu/pull/1986))

internal/backend/remote-state/http/client.go

@@ -261,3 +261,7 @@ func (c *httpClient) Delete() error {
 		return fmt.Errorf("HTTP error: %d", resp.StatusCode)
 	}
 }
+
+func (c *httpClient) IsLockingEnabled() bool {
+	return c.UnlockURL != nil
+}

internal/backend/remote-state/http/client_test.go

@@ -206,3 +206,48 @@ func (h *testBrokenHTTPHandler) Handle(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(500)
 	}
 }
+
+// Tests the IsLockingEnabled method for the HTTP client.
+// It checks whether locking is enabled based on the presence of the UnlockURL.
+func TestHttpClient_IsLockingEnabled(t *testing.T) {
+	tests := []struct {
+		name       string
+		unlockURL  string
+		wantResult bool
+	}{
+		{
+			name:       "Locking enabled when UnlockURL is set",
+			unlockURL:  "http://http-endpoint.com:3333",
+			wantResult: true,
+		},
+		{
+			name:       "Locking disabled when UnlockURL is nil",
+			unlockURL:  "", // Empty string will result in nil *url.URL
+			wantResult: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var unlockURL *url.URL
+			if tt.unlockURL != "" {
+				var err error
+				unlockURL, err = url.Parse(tt.unlockURL)
+				if err != nil {
+					t.Fatalf("Failed to parse unlockURL: %v", err)
+				}
+			} else {
+				unlockURL = nil
+			}
+
+			client := &httpClient{
+				UnlockURL: unlockURL,
+			}
+
+			gotResult := client.IsLockingEnabled()
+			if gotResult != tt.wantResult {
+				t.Errorf("IsLockingEnabled() = %v; want %v", gotResult, tt.wantResult)
+			}
+		})
+	}
+}

internal/backend/remote-state/oss/client.go

@@ -445,6 +445,10 @@ func (c *RemoteClient) getObj() (*remote.Payload, error) {
 	return payload, nil
 }
 
+func (c *RemoteClient) IsLockingEnabled() bool {
+	return c.otsTable != ""
+}
+
 const errBadChecksumFmt = `state data in OSS does not have the expected content.
 
 This may be caused by unusually long delays in OSS processing a previous state

internal/backend/remote-state/oss/client_test.go

@@ -381,3 +381,37 @@ func TestRemoteClient_stateChecksum(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// Tests the IsLockingEnabled method for the OSS remote client.
+// It checks if locking is enabled based on the otsTable field.
+func TestRemoteClient_IsLockingEnabled(t *testing.T) {
+	tests := []struct {
+		name       string
+		otsTable   string
+		wantResult bool
+	}{
+		{
+			name:       "Locking enabled when otsTable is set",
+			otsTable:   "my-lock-table",
+			wantResult: true,
+		},
+		{
+			name:       "Locking disabled when otsTable is empty",
+			otsTable:   "",
+			wantResult: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := &RemoteClient{
+				otsTable: tt.otsTable,
+			}
+
+			gotResult := client.IsLockingEnabled()
+			if gotResult != tt.wantResult {
+				t.Errorf("IsLockingEnabled() = %v; want %v", gotResult, tt.wantResult)
+			}
+		})
+	}
+}

internal/backend/remote-state/s3/client.go

@@ -469,6 +469,10 @@ func (c *RemoteClient) getSSECustomerKeyMD5() string {
 	return base64.StdEncoding.EncodeToString(b[:])
 }
 
+func (c *RemoteClient) IsLockingEnabled() bool {
+	return c.ddbTable != ""
+}
+
 const errBadChecksumFmt = `state data in S3 does not have the expected content.
 
 This may be caused by unusually long delays in S3 processing a previous state

internal/backend/remote-state/s3/client_test.go

@@ -327,3 +327,37 @@ func TestRemoteClient_stateChecksum(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// Tests the IsLockingEnabled method for the S3 remote client.
+// It checks if locking is enabled based on the ddbTable field.
+func TestRemoteClient_IsLockingEnabled(t *testing.T) {
+	tests := []struct {
+		name       string
+		ddbTable   string
+		wantResult bool
+	}{
+		{
+			name:       "Locking enabled when ddbTable is set",
+			ddbTable:   "my-lock-table",
+			wantResult: true,
+		},
+		{
+			name:       "Locking disabled when ddbTable is empty",
+			ddbTable:   "",
+			wantResult: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			client := &RemoteClient{
+				ddbTable: tt.ddbTable,
+			}
+
+			gotResult := client.IsLockingEnabled()
+			if gotResult != tt.wantResult {
+				t.Errorf("IsLockingEnabled() = %v; want %v", gotResult, tt.wantResult)
+			}
+		})
+	}
+}

internal/command/unlock.go

@@ -95,6 +95,15 @@ func (c *UnlockCommand) Run(args []string) int {
 
 	_, isLocal := stateMgr.(*statemgr.Filesystem)
 
+	if optionalLocker, ok := stateMgr.(statemgr.OptionalLocker); ok {
+		// Now we can safely call IsLockingEnabled() on optionalLocker
+		if !optionalLocker.IsLockingEnabled() {
+			c.Ui.Error("Locking is disabled for this backend")
+			return 1
+		}
+	}
+
+	// Proceed with unlocking logic if locking is enabled
 	if !force {
 		// Forcing this doesn't do anything, but doesn't break anything either,
 		// and allows us to run the basic command test too.

internal/states/remote/remote.go

@@ -33,6 +33,14 @@ type ClientLocker interface {
 	statemgr.Locker
 }
 
+// OptionalClientLocker is an optional interface that allows callers to
+// to determine whether or not locking is actually enabled.
+// See OptionalLocker for more details.
+type OptionalClientLocker interface {
+	ClientLocker
+	IsLockingEnabled() bool
+}
+
 // Payload is the return value from the remote state storage.
 type Payload struct {
 	MD5  []byte

internal/states/remote/state.go

@@ -280,6 +280,24 @@ func (s *State) Unlock(id string) error {
 	return nil
 }
 
+func (s *State) IsLockingEnabled() bool {
+	if s.disableLocks {
+		return false
+	}
+
+	switch c := s.Client.(type) {
+	// Client supports optional locking.
+	case OptionalClientLocker:
+		return c.IsLockingEnabled()
+	// Client supports locking by default.
+	case ClientLocker:
+		return true
+	// Client doesn't support any locking.
+	default:
+		return false
+	}
+}
+
 // DisableLocks turns the Lock and Unlock methods into no-ops. This is intended
 // to be called during initialization of a state manager and should not be
 // called after any of the statemgr.Full interface methods have been called.

internal/states/remote/state_test.go

@@ -746,3 +746,104 @@ func TestWriteStateForMigrationWithForcePushClient(t *testing.T) {
 		log.Fatalf("not all requests were read. Expected logIdx to be %d but got %d", logCnt, logIdx)
 	}
 }
+
+// mockOptionalClientLocker is a mock implementation of a client that supports optional locking.
+type mockOptionalClientLocker struct {
+	*mockClient         // Embedded mock client that simulates basic client behavior.
+	lockingEnabled bool // A flag indicating whether locking is enabled or disabled.
+}
+
+type mockClientLocker struct {
+	*mockClient // Embedded mock client that simulates basic client behavior.
+}
+
+// Implement the mock Lock method for mockOptionalClientLocker
+func (c *mockOptionalClientLocker) Lock(_ *statemgr.LockInfo) (string, error) {
+	return "", nil
+}
+
+// Implement the mock Unlock method for mockOptionalClientLocker
+func (c *mockOptionalClientLocker) Unlock(_ string) error {
+	// Provide a simple implementation
+	return nil
+}
+
+// Implement the mock IsLockingEnabled method for mockOptionalClientLocker
+func (c *mockOptionalClientLocker) IsLockingEnabled() bool {
+	return c.lockingEnabled
+}
+
+// Implement the mock Lock method for mockClientLocker
+func (c *mockClientLocker) Lock(_ *statemgr.LockInfo) (string, error) {
+	return "", nil
+}
+
+// Implement the mock Unlock method for mockClientLocker
+func (c *mockClientLocker) Unlock(_ string) error {
+	return nil
+}
+
+// Check for interface compliance
+var _ OptionalClientLocker = &mockOptionalClientLocker{}
+var _ ClientLocker = &mockClientLocker{}
+
+// Tests whether the IsLockingEnabled method returns the expected values based on the backend.
+func TestState_IsLockingEnabled(t *testing.T) {
+	tests := []struct {
+		name         string
+		disableLocks bool
+		client       Client
+		wantResult   bool
+	}{
+		{
+			name:         "disableLocks is true",
+			disableLocks: true,
+			client:       &mockClient{},
+			wantResult:   false,
+		},
+		{
+			name:         "OptionalClientLocker with IsLockingEnabled() == true",
+			disableLocks: false,
+			client: &mockOptionalClientLocker{
+				mockClient:     &mockClient{},
+				lockingEnabled: true,
+			},
+			wantResult: true,
+		},
+		{
+			name:         "OptionalClientLocker with IsLockingEnabled() == false",
+			disableLocks: false,
+			client: &mockOptionalClientLocker{
+				mockClient:     &mockClient{},
+				lockingEnabled: false,
+			},
+			wantResult: false,
+		},
+		{
+			name:         "ClientLocker without OptionalClientLocker",
+			disableLocks: false,
+			client: &mockClientLocker{
+				mockClient: &mockClient{},
+			},
+			wantResult: true,
+		},
+		{
+			name:         "Client without any locking",
+			disableLocks: false,
+			client:       &mockClient{},
+			wantResult:   false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s := NewState(tt.client, encryption.StateEncryptionDisabled())
+			s.disableLocks = tt.disableLocks
+
+			gotResult := s.IsLockingEnabled()
+			if gotResult != tt.wantResult {
+				t.Errorf("IsLockingEnabled() = %v; want %v", gotResult, tt.wantResult)
+			}
+		})
+	}
+}

internal/states/statemgr/locker.go

@@ -66,6 +66,16 @@ type Locker interface {
 	Unlock(id string) error
 }
 
+// OptionalLocker extends Locker interface to allow callers
+// to know whether or not locking is actually enabled.
+// This is useful for some of the backends, which support
+// optional locking based on the configuration (such as S3,
+// OSS and HTTP backends).
+type OptionalLocker interface {
+	Locker
+	IsLockingEnabled() bool
+}
+
 // test hook to verify that LockWithContext has attempted a lock
 var postLockHook func()