Finally allow force-unlock for remote http backend

[haiderbari]

OpenTofu Version

OpenTofu v1.6.3

OpenTofu Configuration Files

...tofu config...

Debug Output

N/A

Expected Behavior

successful 200

Actual Behavior

tofu force-unlock

Do you really want to force-unlock? OpenTofu will remove the lock on the remote state. This will allow local OpenTofu commands to modify this state, even though it may still be in use. Only 'yes' will be accepted to confirm.

Enter a value: yes

Failed to unlock state: Unexpected HTTP response code 405

Steps to Reproduce

tofu force-unlock

Additional Context

This bug was reported in Terraform back in 2021. A PR was opened for the fix but wasn't accepted. There is another PR currently open on Terraform. The PR was opened back in January this year, PR is still not accepted. The issue still persists with the latest version of the OpenTofu CLI as well. If you look at the code within client.go on line 132, the Unlock function accepts the lock ID but never uses it. As a result, no lock ID is ever passed to the remote HTTP backend when performing force-unlock, causing a failure to unlock the state. This feature is important for organizations that have their own HTTP remote backend, allowing their users to use the Tofu CLI command line to unlock their state instead of using workarounds like the curl command. I am happy to contribute to the fix and open a PR.

https://github.com/opentofu/opentofu/blob/main/internal/backend/remote-state/http/client.go#L132

References

No response

[haiderbari]

If accepted I am happy to take it on.

[ollevche]

Hello @haiderbari and thank you for this issue! The core team regularly reviews new issues and discusses them, but this can take a little time. Please bear with us while we get to your issue. If you're interested, the contribution guide has a section about the decision-making process.

[haiderbari]

Hello @haiderbari and thank you for this issue! The core team regularly reviews new issues and discusses them, but this can take a little time. Please bear with us while we get to your issue. If you're interested, the contribution guide has a section about the decision-making process.

Hi ollevche thanks for taking a look at the issue. I am currently working on the fix.

[ollevche]

Hey @haiderbari the issue is not accepted yet, we need to triage it first with the core team. Once it is accepted we are happy to assign it to you for further implementation.

[haiderbari]

Hey core team, any update on this issue. I opened this issue over a month ago. Clearly this bug needs to be addressed.

[ghost]

@haiderbari I'm sorry for the long wait, a large part of the core team has been out in the last few weeks for various reasons so we haven't been able to triage this issue yet.

[ollevche]

I did take a quick look into the implementation and here is a bit more context.

In the implementation of HTTP Backend, we have an http.Client which relies on the existing jsonLockInfo captured in the previous Lock call. Obviously, we don't call Lock on tofu force-unlock and thus we don't have a captured jsonLockInfo there.

I think it is completely fine to extend an existing logic to additionally check whether or not lock IDs match (between what is passed into Lock call and what has been captured previously, if captured). In this case, if we don't have any captured jsonLockInfo, we could just pass lock info with the only field - ID.

I tried to find a specification for HTTP backend API, but I think we don't have one so it is okay to make some of the lock info fields optional on force-unlock (it doesn't hurt anyway since we don't pass anything right now).

It will also help us find more bugs related to ID mismatches because currently, HTTP client is ignoring any passed ID and uses previously captured lock info. For example, remote backend has similar logic, but it also checks if IDs match.

[ollevche]

Hey @haiderbari are you still up to implementing the fix?

[haiderbari]

Hey @haiderbari are you still up to implementing the fix?

Yes 👍

[ollevche]

Thank you for volunteering @haiderbari, I've assigned you. Please read the "I've been assigned an issue, now what?" section of the contribution guide before you begin working.

[haiderbari]

I did take a quick look into the implementation and here is a bit more context.

In the implementation of HTTP Backend, we have an http.Client which relies on the existing jsonLockInfo captured in the previous Lock call. Obviously, we don't call Lock on tofu force-unlock and thus we don't have a captured jsonLockInfo there.

I think it is completely fine to extend an existing logic to additionally check whether or not lock IDs match (between what is passed into Lock call and what has been captured previously, if captured). In this case, if we don't have any captured jsonLockInfo, we could just pass lock info with the only field - ID.

I tried to find a specification for HTTP backend API, but I think we don't have one so it is okay to make some of the lock info fields optional on force-unlock (it doesn't hurt anyway since we don't pass anything right now).

It will also help us find more bugs related to ID mismatches because currently, HTTP client is ignoring any passed ID and uses previously captured lock info. For example, remote backend has similar logic, but it also checks if IDs match.

I was discussing this with my team, it makes sense that anytime Unlock method is called that a HTTP GET call is made to get lock details and then compare the both lock ID's. I believe some of the backend's implement this already for example S3. The potently issue it would introduce a breaking change if teams haven't implemented the GET method for the lock_address. I believe it's currently a POST method. Flow would be call getLockInfo (new function) that calls the HTTP backend to get lock details and compare the ID's continue with the flow if they match else throw an error. Would you want that to be optional call or not.

[haiderbari]

Forgot to mention that when calling force unlock it doesn't instantiate LockInfo object so you can't compare the lock ID's that why you need to implement a GET call to get lock details or else just ignore it and pass on the lock ID. I think it would be better to validate that Lock Id's match within CLI than just allowing the user to pass any lock id using force unlock command.

[ollevche]

Currently, http.Client captures lock info internally on lock so we don't normally need to make an additional request. I think we can skip the comparison check if http.Client doesn't have any lock info captured already.

The implementation could be similar to the following:

• if we have jsonLockInfo -> check if lockID matches with the passed id
• if we don't have jsonLockInfo -> generate a new json lock info body with the only ID field set

I don't think we should extend HTTP Backend API to support more methods / resources.

[haiderbari]

I have open a PR for this issue, sorry for the delay. https://github.com/opentofu/opentofu/pull/2381