diff --git a/CHANGELOG.md b/CHANGELOG.md index 672b2febdc..076d981268 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/backend/remote-state/http/client.go b/internal/backend/remote-state/http/client.go index 06ac219645..124b630719 100644 --- a/internal/backend/remote-state/http/client.go +++ b/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 +} diff --git a/internal/backend/remote-state/http/client_test.go b/internal/backend/remote-state/http/client_test.go index af8adc7bd5..1a81d37bcf 100644 --- a/internal/backend/remote-state/http/client_test.go +++ b/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) + } + }) + } +} diff --git a/internal/backend/remote-state/oss/client.go b/internal/backend/remote-state/oss/client.go index cf7f273bdb..7788016dbe 100644 --- a/internal/backend/remote-state/oss/client.go +++ b/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 diff --git a/internal/backend/remote-state/oss/client_test.go b/internal/backend/remote-state/oss/client_test.go index 560eb6c636..0c694663a9 100644 --- a/internal/backend/remote-state/oss/client_test.go +++ b/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) + } + }) + } +} diff --git a/internal/backend/remote-state/s3/client.go b/internal/backend/remote-state/s3/client.go index 563ee2eef1..4c95a48efa 100644 --- a/internal/backend/remote-state/s3/client.go +++ b/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 diff --git a/internal/backend/remote-state/s3/client_test.go b/internal/backend/remote-state/s3/client_test.go index 9b4be57c5a..90708a77e2 100644 --- a/internal/backend/remote-state/s3/client_test.go +++ b/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) + } + }) + } +} diff --git a/internal/command/unlock.go b/internal/command/unlock.go index 7a031aebb1..c5fb33c142 100644 --- a/internal/command/unlock.go +++ b/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. diff --git a/internal/states/remote/remote.go b/internal/states/remote/remote.go index ae06c0cb82..6fe46954e3 100644 --- a/internal/states/remote/remote.go +++ b/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 diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index 3874ed061d..9d67d4c96d 100644 --- a/internal/states/remote/state.go +++ b/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. diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index 9361028103..d77b1138e8 100644 --- a/internal/states/remote/state_test.go +++ b/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) + } + }) + } +} diff --git a/internal/states/statemgr/locker.go b/internal/states/statemgr/locker.go index fde81643ef..5dc1a7d02c 100644 --- a/internal/states/statemgr/locker.go +++ b/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()