feat(ibmcloud): add IBM COS S3-compatible backend support#811
Open
deekay2310 wants to merge 5 commits into
Open
feat(ibmcloud): add IBM COS S3-compatible backend support#811deekay2310 wants to merge 5 commits into
deekay2310 wants to merge 5 commits into
Conversation
Introduce IBM_COS_* env vars (access key, secret, endpoint, region) and remap them to AWS SDK env vars in Init() so the Pulumi S3 backend can authenticate to IBM Cloud Object Storage without conflicting with AWS credentials. Add DestroyStack with lock cleanup, CleanupState for post- destroy state removal, and --keep-state flag to ibm-power/ibm-z destroy commands. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds IBM COS (S3-compatible) remote-state support with helpers and env constants, introduces exported DestroyStack/CleanupState lifecycle functions, and wires a new --keep-state flag into IBM Power and IBM Z destroy commands; replaces hardcoded IBM Cloud env lookups with constants. ChangesIBM COS S3-Compatible Backend with Keep-State Flag
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provider/ibmcloud/ibmcloud.go`:
- Around line 125-126: The parseS3BackedURL helper can return an empty key and
CleanupState currently calls s3.Delete with that blank key; update
parseS3BackedURL and callers (notably CleanupState and the other block around
the parse use at ~159-167) to validate that the returned key is non-empty before
attempting deletion: either return a descriptive error from parseS3BackedURL
when the URL path yields an empty key, or have CleanupState check the key and
skip/delete only when key != "" (and log/return a clear error if appropriate).
Make sure references to parseS3BackedURL, CleanupState, and s3.Delete are
updated so no Delete is invoked with an empty key.
- Around line 96-113: The code currently calls os.Setenv for AWS_* vars which
mutates process-wide state and can leak/override credentials; instead stop using
os.Setenv in this block and construct the S3 client/config with explicit
credentials and endpoint wiring (e.g. use the AWS SDK config with
credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""), set Region
and a custom EndpointResolver/EndpointResolverWithOptions using
ensureHTTPS(endpoint)) so no environment mutation occurs; update the code that
previously relied on those env vars to accept or receive the aws.Config / s3
client built this way (replace the os.Setenv sequence in
pkg/provider/ibmcloud/ibmcloud.go with creation of an aws.Config/aws-sdk-v2
client using the provided accessKey, secretKey, endpoint, region).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 641f765f-0fde-48d9-b4d1-644371b207d5
📒 Files selected for processing (6)
cmd/mapt/cmd/ibmcloud/hosts/ibm-power.gocmd/mapt/cmd/ibmcloud/hosts/ibm-z.gopkg/provider/ibmcloud/action/ibm-power/ibm-power.gopkg/provider/ibmcloud/action/ibm-z/ibm-z.gopkg/provider/ibmcloud/constants/constants.gopkg/provider/ibmcloud/ibmcloud.go
adrianriobo
reviewed
May 26, 2026
| package constants | ||
|
|
||
| const ( | ||
| EnvIBMCosAccessKeyID = "IBM_COS_ACCESS_KEY_ID" |
Collaborator
There was a problem hiding this comment.
We already got IBMCLOUD_ACCOUNT and IBMCLOUD_API_KEY and IC_REGION which are used by the natively by the provider, should we keep those I mean use them instead ? Also what is the value for the endpoint?
Author
There was a problem hiding this comment.
Since these are HMAC keys (access key + secret key) and exist specifically for S3-compatible API access, they are separate from IAM API keys and I think we should keep them separate. For region, it should be okay to use IC_REGION.
Re: endpoint - value needs to be set manually right now. Should I look into auto-constructing it?
parseS3BackedURL could return an empty key for URLs like s3://bucket or s3://bucket/, which would then be passed to s3.Delete. Add validation to return a descriptive error when the object key is empty, protecting all callers (CleanupState and DestroyStack) in both ibmcloud and aws providers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- IBM_COS_REGION now falls back to IC_REGION when not set, since IC_REGION is already required for compute provisioning. - IBM_COS_ENDPOINT is no longer required; when unset it is auto-constructed as s3.<region>.cloud-object-storage.appdomain.cloud. Can still be overridden for private or cross-region endpoints. - Required env vars reduced from 4 to 2 (IBM_COS_ACCESS_KEY_ID and IBM_COS_SECRET_ACCESS_KEY). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… access Replace IBM_COS_ACCESS_KEY_ID and IBM_COS_SECRET_ACCESS_KEY with existing IBMCLOUD_ACCOUNT and IBMCLOUD_API_KEY env vars per reviewer feedback. Consolidate raw string references across power.go, piimages.go, and vpcimages.go to use constants from the ibmcloud/constants package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/provider/ibmcloud/ibmcloud.go (1)
171-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't report destroy success when state deletion fails.
CleanupStatelogss3.Deleteerrors and still returnsnil. Since the action layer returnsCleanupState(mCtx)directly, a failed delete still exits success even when--keep-stateis false, leaving stale backend state behind.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/provider/ibmcloud/ibmcloud.go` around lines 171 - 175, The CleanupState function currently swallows s3.Delete errors (calling logging.Warnf) and still returns nil, causing callers (e.g., action layer that calls CleanupState(mCtx)) to treat a failed delete as success; update CleanupState so that if s3.Delete(...) returns an error you propagate that error (or wrap it with context) instead of returning nil—refer to the s3.Delete call, the mCtx.Context() usage, and the existing logging.Warnf/Info calls in CleanupState to locate the change, and ensure the success log (logging.Info) only runs when no error is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/provider/ibmcloud/ibmcloud.go`:
- Around line 171-175: The CleanupState function currently swallows s3.Delete
errors (calling logging.Warnf) and still returns nil, causing callers (e.g.,
action layer that calls CleanupState(mCtx)) to treat a failed delete as success;
update CleanupState so that if s3.Delete(...) returns an error you propagate
that error (or wrap it with context) instead of returning nil—refer to the
s3.Delete call, the mCtx.Context() usage, and the existing logging.Warnf/Info
calls in CleanupState to locate the change, and ensure the success log
(logging.Info) only runs when no error is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5f9f2a0c-59d2-4601-9ceb-91297d542cbc
📒 Files selected for processing (5)
pkg/provider/ibmcloud/constants/constants.gopkg/provider/ibmcloud/data/piimages.gopkg/provider/ibmcloud/data/vpcimages.gopkg/provider/ibmcloud/ibmcloud.gopkg/provider/ibmcloud/services/power/power.go
…kend Revert to HMAC credentials for COS access (IAM keys confirmed incompatible with S3 SigV4 auth) and standardize all env vars under the IBMCLOUD_* prefix: IBMCLOUD_COS_ACCESS_KEY_ID, IBMCLOUD_COS_SECRET_ACCESS_KEY, IBMCLOUD_COS_ENDPOINT. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce IBM_COS_* env vars (access key, secret, endpoint, region) and remap them to AWS SDK env vars in Init() so the Pulumi S3 backend can authenticate to IBM Cloud Object Storage without conflicting with AWS credentials. Add DestroyStack with lock cleanup, CleanupState for post- destroy state removal, and --keep-state flag to ibm-power/ibm-z destroy commands.