Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/backend/remote-state/azure: respect NextMarker #33396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

coderjoe
Copy link

When enumerating blob resources in Azure the service may return a result containing all of the results, or it might return a response containing only a subset of the results and a marker intended to be used to fetch the next subset in a subesquent call.

The azure backend state does not currently check for or enumerate across returned NextMarker values when using ListBlobs while fetching the list of workspaces. This results in partial results being returned when the API returns results requiring iteration via marker and NextMarker

This fixes the issue by updating the backend state to check for and respect the NextMarker continuation ensuring that the full results set is always processed regardless of whether or not the result set is limited via maxresults or is limited by the 5000 item per result set limit.

For more information see:
https://learn.microsoft.com/en-us/rest/api/storageservices/enumerating-blob-resources#Subheading2
https://learn.microsoft.com/en-us/rest/api/storageservices/list-blobs?tabs=azure-ad#uri-parameters

Fixes #31719

Target Release

1.5.x

Draft CHANGELOG entry

BUG FIXES

  • The AzureRM backend will now return all workspaces when using the azurerm state storage backend even when the blob storage exceeds the API's 5000 item limit for a single result query.

@coderjoe coderjoe requested a review from a team as a code owner June 20, 2023 05:40
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 20, 2023

CLA assistant check
All committers have signed the CLA.

@coderjoe
Copy link
Author

I'll be doing ad-hoc testing on this tomorrow at work to ensure it works as expected but I could use some advice or guidance on how to write up some automated tests covering this edge case.

This is only my second attempt at anything in go and I'm having a bit of trouble finding my way around the test suite.

@crw
Copy link
Contributor

crw commented Jun 20, 2023

Thanks, I'll notify the Azure provider team.

@coderjoe
Copy link
Author

@crw thanks for your continued help. :)

For what it is worth I've deployed this locally and it does seem to have resolved the issue I'm having at work.

@coderjoe
Copy link
Author

coderjoe commented Aug 3, 2023

We're nearing the 1 year birthday of issue #31719 , is there anything I can do to help get this a little more traction? Every time there's a new Terraform version released I need to re-integrate and re-test a personal build including my fix to ensure we don't run into problems.

If the issue is the quality of my code I'm more than happy to incorporate any suggestions or requests from the team. I'd also be happy for my branch to be scrapped in favor of code from an engineer more familiar with Go.

The only change that needs to be made is to ensure the state backend paginates through the results of the ListBlobs() fetch API when the API says pagination is necessary (via the NextMarker parameter). The current upstream code doesn't paginate even when it is required.

@crw
Copy link
Contributor

crw commented Aug 22, 2023

Hi @coderjoe, I can see this PR is on the triage backlog for the Azure Provider team. Although the backends are maintained by the provider teams, they are not as high a priority as provider work. In my experience, the provider team will irregularly set aside time to review a batch of backend PRs, as their schedule allows. That said, I will raise it again.

This updates the backend state to check for and respect the NextMarker
continuation if present rather than just omitting the results behind the
continuation.

Relates to: hashicorp#31719
@coderjoe coderjoe force-pushed the bug/azure.remotestate.handle.api.pagination branch from ab85882 to 104a9be Compare October 6, 2024 16:46
@coderjoe
Copy link
Author

coderjoe commented Oct 6, 2024

We're on to 2 years with no review.

I've gone ahead and rebased onto the latest master so someone can take a look.
But lets be honest - if there's no apetite to fix that's fine and I understand.
Just please just close this PR and the associated issue as whatever is the appropriate won't fix status.

If I don't hear back by the next time I do my next open contribution review I'll close them myself.

Heres hoping we can get it fixed before then though - I grow tired of manually patching terraform with every release just to keep using it.

@crw
Copy link
Contributor

crw commented Oct 7, 2024

Hi @coderjoe, I understand the frustration. The Azure team here hasn't looked at any of the Azure backend issues or PRs in some time, but I know that there is a desire to do so. Unfortunately the desire to look at these hasn't yet translated into action. I am hopeful this will get looked at before your next window closes.

@rdtechie
Copy link

@crw We are a significant Microsoft customer. Is there anything we can do to help prioritize this with our assigned Microsoft Cloud Solution Architect or Customer Success Manager? This issue has caused unintended resource deletions on our end, so this PR would be highly beneficial.

@crw
Copy link
Contributor

crw commented Dec 20, 2024

I am going to defer to the local expert, however my understanding is that these issues can get prioritized with Microsoft, as they have a team that works on Azure Provider issues (ancillary to the HashiCorp team that owns the AzureRM Provider).

@rdtechie
Copy link

rdtechie commented Jan 9, 2025

@crw Do you have an update for us?

@crw
Copy link
Contributor

crw commented Jan 9, 2025

Apologies for my previous comment, I mistakenly thought I was in an issue thread not a PR thread. In this case, we are waiting for the Azure Provider team at HashiCorp to prioritize this PR review. Thanks for your continued interest in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants