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

[Alexandru Trendler (g0dfl3sh)]

Resolves #1852

Resolves #

Target Release

1.9.0

Checklist

• I have read the contribution guide.
• I have not used an AI coding assistant to create this PR.
• I have written all code in this PR myself OR I have marked all code I have not written myself (including modified code, e.g. copied from other places and then modified) with a comment indicating where it came from.
• I (and other contributors to this PR) have not looked at the Terraform source code while implementing this PR.

Go checklist

• I have run golangci-lint on my change and receive no errors relevant to my code.
• I have run existing tests to ensure my code doesn't break anything.
• I have added tests for all relevant use cases of my code, and those tests are passing.
• I have only exported functions, variables and structs that should be used from other packages.
• I have added meaningful comments to all exported functions, variables, and structs.

Website/documentation checklist

• I have locally started the website as described here and checked my changes.

[github-actions]

Reminder for the PR assignee: If this is a user-visible change, please update the changelog as part of the PR.

[Lucas Badico (LucasBadico)]

Good man, great work! I won't approve because I don't have all the context in mind.

Look like you nailed it!

[Deleted user (ghost)]

Hey Alexandru Trendler (@g0dfl3sh) thank you for this PR. Can you please address the linter issues?

[Alexandru Trendler (g0dfl3sh)]

Thanks for pointing it out @abstractionfactory! I had the wrong linter selected in my VSCode extension. Should be good to go now.

[Alexandru Trendler (g0dfl3sh)]

@abstractionfactory Not sure why the unit tests for darwin fail now, but it seems unrelated to my changes. Would you be able to re-trigger?

[Alexandru Trendler (g0dfl3sh)]

Thanks! It worked this time; probably just a flaky test.

[Deleted user (ghost)]

We have a few of those. :) Let me get a review in.

[Deleted user (ghost)]

LGTM, @opentofu/core-engineers can someone with more experience around locking please provide a second review?

[Oleksandr Levchenkov (ollevche)]

Hello and thank you for the contribution!

The fix will help when force-unlocking the state, but it will break the configuration for existing users on tofu apply, when there is no locking enabled.

HTTP, OSS and S3 remote state backends support optional locking (based on configuration). When the locking is disabled, Lock and Unlock methods are going to be called anyway so it should not error in such cases.

I suggest looking into the force-unlock command implementation to provide a more isolated fix to not break anything outside the scope of the issue. We can check if the locking is disabled before trying to force-unlock and then provide a meaningful error.

I am happy to help if you have any further questions!

[Alexandru Trendler (g0dfl3sh)]

Thanks for the hints Oleksandr Levchenkov (@ollevche)! Going to start looking into that.

[Alexandru Trendler (g0dfl3sh)]

Hey Oleksandr Levchenkov (@ollevche)! Just pushed a new commit attempting to do what you asked for the S3 backend. Please let me know if i am going the right path (if so, I will push the changes for OSS and HTTP as well).
I feel a little uneasy about declaring all those (what should be in theory) no-ops, but that was the only way I could get rid of the type errors I was getting (I'm sure there must be better/smarter ways to do that).

[Oleksandr Levchenkov (ollevche)]

Thank you for the update! You are certainly going in the right direction.

I would introduce a separate interface similar to the following:

type OptionalLocker interface {
    Locker
    IsLockingEnabled() bool
}

Then we could extend only OSS, HTTP and S3 backends to implement IsLockingEnabled function.

In the force unlock command we can assert if the implementation conforms OptionalLocker interface and if so check the output of IsLockingEnabled.

This is not an ideal solution, but that way we follow general code style and extend only what is needed to extend.

What do you think?

[Alexandru Trendler (g0dfl3sh)]

Thank you for the suggestion! That makes a whole lot more sense and will probably be cleaner. Will test this tomorrow and let you know.

[Alexandru Trendler (g0dfl3sh)]

Sorry for the delay; just pushed my changes.
I had to add the if backendConfig.Type == "s3" || backendConfig.Type == "oss" || backendConfig.Type == "http" statement because otherwise it would break state locking for other backends such as GCS.
Let me know what you think when you have time!

[Alexandru Trendler (g0dfl3sh)]

Oleksandr Levchenkov (@ollevche) The unit tests errored out with null pointer deference. I have added an additional check for backendConfig to not be nil. Should hopefully pass now.

[Oleksandr Levchenkov (ollevche)]

Thank you! I left a few small comments, but overall the PR looks good 👍

[Alexandru Trendler (g0dfl3sh)]

Thanks for the help! Should be good to go now.

[Alexandru Trendler (g0dfl3sh)]

The Compare check failed, but given the errors it seems unrelated to changes from this PR. Should we try a re-run?

Edit: Succeeded on the 2nd run. Thanks for re-running!

[Oleksandr Levchenkov (ollevche)]

Looks good, left a couple of suggestions to describe public interfaces, other than that - we are good to go!

[Alexandru Trendler (g0dfl3sh)]

Thanks for adding those! I need to make a habit out of adding comments early on as I always end up forgetting.

[Deleted user (ghost)]

Hey Alexandru Trendler (@g0dfl3sh) thank you very much for this fix! I have two comments to make:

1. There are no tests included. Combined with the changes in locking behavior, it would be nice to see at least rudimentary test coverage that proves the old code wrong and the new code working so it can't be broken in the future.
2. Downcasting is usually code smell, I wonder if it would be possible to achieve this without doing it? (Liskov Substitution Principle and all.) I'm not super worried if it goes into the codebase like this, but it definitely makes the code harder to read.

Again, thank you so much for your work!

[Oleksandr Levchenkov (ollevche)]

2. Downcasting is usually code smell, I wonder if it would be possible to achieve this without doing it? (Liskov Substitution Principle and all.) I'm not super worried if it goes into the codebase like this, but it definitely makes the code harder to read.

This is not ideal and we should consider a larger refactoring of state manager interfaces, because currently, it makes little sense. However, I understand historical reasons behind this approach.

IMO it should not be part of this PR / bug fix.

[Alexandru Trendler (g0dfl3sh)]

Thanks for the feedback folks; I've learned a lot! Going to start working on adding tests.

[Alexandru Trendler (g0dfl3sh)]

I managed to write tests for most of the the use cases of the added code, but I wasn't able to write any meaningful tests for the actual behavior of the force-unlock command. It seems like whatever I tried it would default to using the local backend and bypassing the whole logic, which is the current behavior ofTestUnlock.
Maybe that could be better integrated as acceptance tests? I'm really not sure, any pointer would be much appreciated!

[Deleted user (ghost)]

Christian Mesh (@cam72cam) did you want to add e2e tests to this? Otherwise I'm happy with it, thanks a lot Alexandru Trendler (@g0dfl3sh) !

[Alexandru Trendler (g0dfl3sh)]

Thanks a lot for the review @abstractionfactory! And if a decision is made that we need e2e tests for this I can definitely try to look into that as well.

[Alexandru Trendler (g0dfl3sh)]

Thanks for running the checks Oleksandr Levchenkov (@ollevche)! Just pushed a fix for the linting errors.

[Oleksandr Levchenkov (ollevche)]

Alexandru Trendler (@g0dfl3sh) thanks for the contribution!

I tested changes manually with an S3 backend and it worked just fine.

I agree we would want to put an e2e test here as well, but that will be hard to achieve due to backend dependencies.

@abstractionfactory your S3 mocking test tools will be super useful here once they are merged!

[Alexandru Trendler (g0dfl3sh)]

Thanks for all the support folks! Do I need to backport this to any specific branch? I was just reading the Backporting section in CONTRIBUTING.md and I'm not sure if it applies here.

[Deleted user (ghost)]

Alexandru Trendler (@g0dfl3sh) I don't know if we want to backport these changes, I think that should be a separate issue.