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

Address s3 compatible remote state issues + logging #840

Merged
merged 11 commits into from Nov 9, 2023

Conversation

cam72cam
Copy link
Contributor

@cam72cam cam72cam commented Nov 8, 2023

This PR started out addressing #821, but ended up being an investigation of the workflow itself instead of the not-adopted validation changes.

  • Add in the relevant context hooks for aws-go-sdk-base logging. This is required to easily debug the rest of the changes.
  • Missing proto:// backwards compatibility
    • BaseEndpoint -> EndpointResolver fixed many of the issues related to non-proto endpoints.
    • Unfortunately it did not cover all cases, the main one I identified was in the aws-go-sdk-base middleware.
    • A workaround was added such that any non-proto endpoints would automatically have the https:// prefix added.
    • This is hopefully a sane default, and comes with a debug message that should help track down if it ever causes issues.
  • PutObject checksum calculation
  • Added a HeadObject check to state.get to work around a MinIO issue where the GetObject request would return the wrong error message and fail. I'm not sure that this is worth keeping in, though it might solve issues with other providers?

TODO:

  • Add tests to detecting / adding proto://
  • Decide if removing the new HeadObject call is the right call.

Resolves #821

Target Release

1.6.0

Draft CHANGELOG entry

BUG FIXES

  • S3 backend endpoints without proto:// now default to https:// instead of failing.
  • Most S3 compatible remote state backends should now work without checksum errors / 400s by default.
  • Logging has been re-added to S3 remote state calls.

Copy link

github-actions bot commented Nov 8, 2023

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

@cam72cam cam72cam requested a review from a team November 8, 2023 23:05
@Yantrio
Copy link
Member

Yantrio commented Nov 9, 2023

I've converted this to draft whilst we still have items here in TODO

@Yantrio Yantrio marked this pull request as draft November 9, 2023 11:29
Copy link
Member

@Yantrio Yantrio left a comment

Choose a reason for hiding this comment

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

LGTM so far, just a couple nitpicks 👍

internal/backend/remote-state/s3/backend.go Outdated Show resolved Hide resolved
internal/backend/remote-state/s3/client.go Outdated Show resolved Hide resolved
Copy link
Member

@Yantrio Yantrio left a comment

Choose a reason for hiding this comment

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

So far so good, I'll tentitively approve for now, please ping me again when the final TODO items are done and it's ready for review again. Nice work!

@cam72cam cam72cam marked this pull request as ready for review November 9, 2023 13:46
@kislerdm
Copy link
Contributor

kislerdm commented Nov 9, 2023

(question) Shall we also cover custom endpoint for STS and IAM as per the issue description:

The following behaviour happens for S3, IAM, DynamoDB and STS endpoints as well

For context, see the details:

cc: @RLRabinowitz @Yantrio @cam72cam

@cam72cam
Copy link
Contributor Author

cam72cam commented Nov 9, 2023

(question) Shall we also cover custom endpoint for STS and IAM as per the issue description:

The following behaviour happens for S3, IAM, DynamoDB and STS endpoints as well

For context, see the details:

* [IAM endpoint configuration](https://github.com/opentofu/opentofu/blob/aa6b6a2fec9924dbc70e15e04aa1ccdd514e7e16/internal/backend/remote-state/s3/backend.go#L706)

* [STS endpoint configuration](https://github.com/opentofu/opentofu/blob/aa6b6a2fec9924dbc70e15e04aa1ccdd514e7e16/internal/backend/remote-state/s3/backend.go#L713)

cc: @RLRabinowitz @Yantrio @cam72cam

The https:// correction happens for all of the endpoints in the StringOk function.

* S3/Dynamo hosts did not support configurations without proto:// (regression)
* Invalid value for SuppressDebugLog in s3 backend config
* No Logger passed to s3 backend config
* Context needed to be tied to specific loggers to function
* Add Head to state get, works around some compatability issues

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Works around issues in compatible services handing streaming checksums
by pre-computing the hash and hitting the simpler code path.

Hopefully this can be removed at some point, but for now it's a pretty
big win.

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
This is not a requirement from aws-go-sdk, but it is for the hashicorp
aws-go-sdk-base middleware.

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
@cam72cam cam72cam force-pushed the 821-address_s3_remote_state_issues branch from 1cbd353 to 46588eb Compare November 9, 2023 16:27
@cam72cam cam72cam merged commit e2d5a17 into main Nov 9, 2023
7 checks passed
@cam72cam cam72cam deleted the 821-address_s3_remote_state_issues branch November 9, 2023 16:36
thumperward pushed a commit to thumperward/opentf that referenced this pull request Jan 10, 2024
Signed-off-by: Christian Mesh <christianmesh1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3, IAM, DynamoDB and STS endpoints do not allow bare hostname
5 participants