Skip to content

Conversation

@SarahFrench
Copy link
Member

@SarahFrench SarahFrench commented Jun 9, 2025

A rebased* version of #34990 :

As part of our efforts to remove the large snapshot of the legacy SDK that we've been using, this replaces the uses of that package with our new lightweight backendbase package, which is a collection of helpers that can substitute for most of the parts of the legacy SDK that backends tend to use.

In the long run we're still planning to move all of the state storage backends out to provider plugins, at which point they'll presumably use the modern plugin framework, but in the meantime the legacy SDK is quite a liability because few people know how to maintain it while hopefully this interim backendbase package is easier to maintain (if needed) due to its relative simplicity.

I don't have any test environment for this backend, so I've only tested this through its unit tests. I'd appreciate if a backend maintainer could, along with reviewing this code, also run the acceptance test suite in case there are regressions that the unit tests aren't able to catch. Thanks!

*= I noticed that the schema had changed since the original PR so my approach was to re-do that work

NOTE: This PR changed metadata_host in the schema from Required to Optional. The legacy SDK allowed the required field to be set using a fallback value, so users never saw required behaviour. Moving off the legacy SDK made the field to begin behaving as required, which isn't correct. Therefore we swapped the schema to mark it as Optional, and this isn't a user-facing change as prior to this PR the field behaved as optional already.

Target Release

1.13.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

apparentlymart and others added 3 commits June 9, 2025 18:04
This now uses the backendbase package's "SDK-like" helpers instead, which
provide a much smaller subset of the former legacy SDK functionality but
enough for what this backend needs.
@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jun 9, 2025
@SarahFrench SarahFrench marked this pull request as ready for review June 9, 2025 17:20
@SarahFrench SarahFrench requested review from a team as code owners June 9, 2025 17:20
@SarahFrench
Copy link
Member Author

@hashicorp/terraform-azure, could someone with Azure credentials run the acceptance tests for the azure backend to verify the changes? Thanks!

@SarahFrench
Copy link
Member Author

SarahFrench commented Jun 9, 2025

Interesting - TestBackendConfig fails with The attribute "metadata_host" is required, but no definition was found.

I believe this is because PrepareConfig in the old code would use the empty string default when the field isn't set. This would get around the required behaviour. The new code doesn't seem to accept a default value for a required field like the old code did. Arguably, that's a more correct behaviour.

I think the test needs to be updated here, but I defer to the Azure team.

@SarahFrench
Copy link
Member Author

From a manual test, it seems like metadata_host isn't behaving as a required field despite being marked as required:

Screenshot 2025-06-16 at 10 21 59

It may be that metadata_host needs to be marked as Optional, as that's how it is currently behaving. Otherwise merging this PR would release a breaking change to users.

jackofallops
jackofallops previously approved these changes Jun 16, 2025
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SarahFrench - LGTM 👍

@SarahFrench SarahFrench requested a review from jackofallops June 16, 2025 09:46
@SarahFrench
Copy link
Member Author

Hi @jackofallops - I'm only able to run the TestBackendConfig test and it's currently failing because removing the legacy SDK has forced metadata_host to begin actually behaving like a Required field. We need to decide if it is indeed a Required field and the test should be updated, or whether the field should be Optional (as that's how it's behaving for users today).

@jackofallops
Copy link
Member

Hi @jackofallops - I'm only able to run the TestBackendConfig test and it's currently failing because removing the legacy SDK has forced metadata_host to begin actually behaving like a Required field. We need to decide if it is indeed a Required field and the test should be updated, or whether the field should be Optional (as that's how it's behaving for users today).

To the best of my knowledge it should be an optional field, it's an override for non-standard environments, so Optional I believe to be correct there. Not sure how/why it would have been flagged as Required previously.

@SarahFrench SarahFrench merged commit 481bef3 into main Jun 16, 2025
12 checks passed
@SarahFrench SarahFrench deleted the nolegacysdk-azure-rebase branch June 16, 2025 10:13
SarahFrench added a commit that referenced this pull request Jun 18, 2025
* backend/azure: Remove legacy helper/schema dependency

This now uses the backendbase package's "SDK-like" helpers instead, which
provide a much smaller subset of the former legacy SDK functionality but
enough for what this backend needs.

* remove unused replace directive

* Fix go.mod file after rebase

* Fix schema errors after self-review of code

* Change `metadata_host` to be an optional field in the schema, to match the current behavior of code

---------

Co-authored-by: Martin Atkins <[email protected]>
Co-authored-by: James Bardin <[email protected]>
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backend/azure no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants