IAM | Save ARN for accounts in secret #1739
Conversation
WalkthroughAdds ARN propagation: new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
pkg/noobaaaccount/reconciler.go (2)
383-383: Remove redundant type conversion and add defensive check for ARN.The type conversion
string(accountInfo.ARN)is unnecessary sinceARNis already astringin theCreateAccountReplystruct. Additionally, there's no validation that the ARN field is populated before storing it.Apply this diff:
- r.Secret.StringData["ARN"] = string(accountInfo.ARN) + if accountInfo.ARN != "" { + r.Secret.StringData["ARN"] = accountInfo.ARN + }
385-402: Handle backward compatibility for ARN like AccessKeys.The code has fallback logic for
AccessKeyswhen talking to older NooBaa versions (lines 388-398), but no similar handling forARN. If the NooBaa API doesn't return ARN (older versions or API changes), an empty string will be silently stored in the secret.Consider applying consistent handling:
var accessKeys nb.S3AccessKeys // if we didn't get the access keys in the create_account reply we might be talking to an older noobaa version (prior to 5.1) // in that case try to get it using read account if len(accountInfo.AccessKeys) == 0 { log.Info("CreateAccountAPI did not return access keys. calling ReadAccountAPI to get keys..") readAccountReply, err := r.NBClient.ReadAccountAPI(nb.ReadAccountParams{Email: r.NooBaaAccount.Name}) if err != nil { return err } accessKeys = readAccountReply.AccessKeys[0] + if readAccountReply.ARN != "" { + accountInfo.ARN = readAccountReply.ARN + } } else { accessKeys = accountInfo.AccessKeys[0] } r.Secret.StringData = map[string]string{} r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey) r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey) - r.Secret.StringData["ARN"] = string(accountInfo.ARN) + if accountInfo.ARN != "" { + r.Secret.StringData["ARN"] = accountInfo.ARN + }Note: Verify that
AccountInfostruct (fromReadAccountAPI) also has theARNfield, or adjust accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/nb/types.go(1 hunks)pkg/noobaaaccount/noobaaaccount.go(1 hunks)pkg/noobaaaccount/reconciler.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Neon-White
Repo: noobaa/noobaa-operator PR: 1587
File: pkg/system/reconciler.go:287-291
Timestamp: 2025-07-14T10:59:21.959Z
Learning: NooBaa operator design constraint: Multiple NooBaa instances are not supported in the same namespace, only across different namespaces. This means hard-coded resource names like ConfigMaps are acceptable within a namespace scope.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-operator-tests
🔇 Additional comments (3)
pkg/nb/types.go (1)
414-415: LGTM! Clean API response structure extension.The new
IdandARNfields follow Go naming conventions and are properly tagged for JSON serialization.pkg/noobaaaccount/noobaaaccount.go (1)
486-486: LGTM! Properly includes ARN in status output.The condition correctly extends the filtered keys to include ARN alongside AWS credentials for display.
pkg/noobaaaccount/reconciler.go (1)
385-402: Verify ARN availability in ReadAccountAPI response.The concern raised about whether the
AccountInfostruct (used byReadAccountAPI) includes anARNfield requires verification against the actual API contract. The code at lines 385-402 accessesaccountInfo.ARNunconditionally after bothCreateAccountAPIandReadAccountAPIpaths, so clarification is needed on whether:
- The NooBaa API returns ARN in both create and read account responses
- The
AccountInfostruct definition includes the ARN field- Error handling is appropriate if ARN is unavailable from
ReadAccountAPIPlease confirm the struct definition and API behavior to ensure consistent handling of the ARN field across both code paths.
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
98a35e6 to
88ce48f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/nb/types.go (1)
414-415: LGTM! New fields support ARN propagation.The addition of
IdandARNfields correctly extendsCreateAccountReplyto include account identifiers returned by the API.Minor: Consider renaming
IdtoIDto follow Go naming conventions for initialisms, unless the lowercase form is required to match the exact API response format.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/nb/types.go(1 hunks)pkg/noobaaaccount/noobaaaccount.go(1 hunks)pkg/noobaaaccount/reconciler.go(2 hunks)pkg/obc/provisioner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/noobaaaccount/noobaaaccount.go
- pkg/noobaaaccount/reconciler.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-sa-test
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
88ce48f to
2672bb4
Compare
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
2672bb4 to
a9b3486
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/nb/types.go (1)
412-418: CreateAccountReply additions look correct; consider idiomaticIDnamingThe added
IdandARNfields withjson:"id"/json:"arn"tags look consistent with the intended API and are backward compatible with older responses. From a Go style perspective, acronyms are usually all caps (e.g.,IDrather thanId), and this file already follows that pattern elsewhere (AzureTenantID,AzureClientID, etc.). If you’re okay touching this now, renaming toIDwould make the API more idiomatic, while keeping the JSON tag asjson:"id"to avoid breaking the wire format:-type CreateAccountReply struct { - Id string `json:"id"` - ARN string `json:"arn"` - Token string `json:"token"` - AccessKeys []S3AccessKeys `json:"access_keys"` -} +type CreateAccountReply struct { + ID string `json:"id"` + ARN string `json:"arn"` + Token string `json:"token"` + AccessKeys []S3AccessKeys `json:"access_keys"` +}If there are many existing references expecting
Id, treating this as a follow‑up cleanup is also acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/nb/types.go(1 hunks)pkg/noobaaaccount/noobaaaccount.go(1 hunks)pkg/noobaaaccount/reconciler.go(2 hunks)pkg/obc/provisioner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/noobaaaccount/noobaaaccount.go
- pkg/noobaaaccount/reconciler.go
- pkg/obc/provisioner.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-operator-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-hac-test
- GitHub Check: run-cli-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-tls-token-test
naveenpaul1
changed the title
| r.OB.Spec.AdditionalState["arn"] = accountInfo.ARN | ||
|
|
||
| log.Infof("✅ Successfully created account %q with access to bucket %q", r.AccountName, r.BucketName) | ||
| return nil |
There was a problem hiding this comment.
Will we see in the CLI the printed ARN of the OBC?
If not, perhaps you can add one.
There was a problem hiding this comment.
We are not listing any OBC accounts and couldnt get details with status command
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
a9b3486 to
ffd5651
Compare
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
ffd5651 to
8bd9501
Compare
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
8bd9501 to
f1863a7
Compare
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_account_arn
branch
from
f1863a7 to
d7ad849
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/nb/types.go(1 hunks)pkg/noobaaaccount/noobaaaccount.go(1 hunks)pkg/noobaaaccount/reconciler.go(2 hunks)pkg/obc/provisioner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/noobaaaccount/noobaaaccount.go
- pkg/obc/provisioner.go
- pkg/noobaaaccount/reconciler.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: golangci-lint
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-operator-tests
- GitHub Check: run-admission-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-dev-test
3 participants
Explain the changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
Note: OBC accounts can not create IAM users and OBC account is not part of this change
Summary by CodeRabbit
New Features
Status/Display
✏️ Tip: You can customize this high-level summary in your review settings.