IAM | Bucket policy Principal upgrade script. #9267
Conversation
WalkthroughAdds a debug log in S3 authorization, introduces a 5.21.0 upgrade script that converts bucket policy Principals to ARN form, and adds integration tests exercising the ARN-based upgrade using sensitive principal values and pool/account mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Upgrade as upgrade_bucket_policy_principal
participant Store as system_store
participant DB as Database
Note over Test,Upgrade: Trigger upgrade script
Test->>Upgrade: run({ dbg, system_store, system_server })
Upgrade->>DB: query buckets with s3_policy
DB-->>Upgrade: buckets[]
loop per bucket
Upgrade->>Upgrade: _create_arn(principals) => ARN(s)
Upgrade->>Upgrade: rewrite Statement.Principal to ARN form
end
alt any buckets changed
Upgrade->>Store: make_changes(changed_buckets)
Store->>DB: persist updates
DB-->>Store: ack
Store-->>Upgrade: success
else no changes
Upgrade-->>Test: complete (no-op)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 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: 6
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-277: Consider cleaning up the IAM account created during the test.The test creates an IAM account at line 248 but does not clean it up in the
mocha.afterhook. This could lead to leftover test data accumulating in the system store across test runs.Consider adding cleanup logic in the
mocha.afterhook:mocha.after(async function() { await s3.deleteBucket({ Bucket: BKT }); await s3.deleteBucket({ Bucket: BKT1 }); // Clean up IAM account if it exists try { await rpc_client.account.delete_account({ email: 'iam_username' }); } catch (err) { // Ignore if account doesn't exist } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:13:42.192Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.192Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (2)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (12)
require(9-9)require(10-10)_(5-5)dbg(18-18)system_store(13-13)account(239-239)buckets(29-32)buckets(41-44)bucket(95-95)bucket(230-230)new_policy(109-109)new_policy(264-264)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(14-14)new_policy(39-39)
⏰ 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). (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (3)
7-7: LGTM!The addition of
POOL_LISTto the destructuring is consistent with its usage in line 246 of the new test case.
12-12: LGTM!The
SensitiveStringimport is correctly used to wrap sensitive principal values in the test policy at lines 212 and 220.
15-15: LGTM!The upgrade script import is properly used at line 260 to test the ARN-based principal upgrade functionality.
| console.log("authorize_request_policy ====>>>", bucket_owner, owner_account); | ||
|
|
There was a problem hiding this comment.
Remove debug console.log or convert to proper logging.
This console.log statement should be removed before merging, or if needed for debugging, converted to use the dbg module (e.g., dbg.log0 or dbg.log1) for consistency with the rest of the codebase. Additionally, be cautious about logging bucket_owner and owner_account as they may contain sensitive information.
Apply this diff to remove the debug statement:
- console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
-Or, if logging is necessary, convert it to:
- console.log("authorize_request_policy ====>>>", bucket_owner, owner_account);
-
+ dbg.log1('authorize_request_policy:', { bucket_owner, owner_account });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("authorize_request_policy ====>>>", bucket_owner, owner_account); |
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_rest.js around lines 244 to 245, remove the stray debug
console.log statement or replace it with the project dbg logging (e.g., dbg.log0
or dbg.log1) to match existing logging conventions; if logging is required,
redact or avoid printing sensitive fields like bucket_owner and owner_account
(log only non-sensitive identifiers or an existence flag) and ensure the call
uses the dbg module rather than console.log.
| function _create_arn(dbg, principals, system_store) { | ||
| if (!principals.AWS) return; | ||
| const principal_arns = []; | ||
| for (const principal of principals.AWS) { | ||
| if (principal === '*') continue; | ||
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | ||
| if (!account) { | ||
| dbg.log0(`Could not found the account with email: ${principal}`); | ||
| continue; | ||
| } | ||
| let arn; | ||
| if (account.owner) { | ||
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | ||
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | ||
| } else { | ||
| arn = `aws:arn:${account._id.toString()}:root`; | ||
| } | ||
| principal_arns.push(arn); | ||
| } | ||
| return { AWS: principal_arns }; | ||
| } |
There was a problem hiding this comment.
Critical: Wildcard principals are lost during upgrade.
The function skips wildcard ('*') principals at line 13 but never adds them to principal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.
Apply this diff to preserve wildcard principals:
function _create_arn(dbg, principals, system_store) {
if (!principals.AWS) return;
const principal_arns = [];
for (const principal of principals.AWS) {
- if (principal === '*') continue;
+ if (principal === '*') {
+ principal_arns.push('*');
+ continue;
+ }
const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap());
if (!account) {
dbg.log0(`Could not found the account with email: ${principal}`);
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function _create_arn(dbg, principals, system_store) { | |
| if (!principals.AWS) return; | |
| const principal_arns = []; | |
| for (const principal of principals.AWS) { | |
| if (principal === '*') continue; | |
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | |
| if (!account) { | |
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| continue; | |
| } | |
| let arn; | |
| if (account.owner) { | |
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | |
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | |
| } else { | |
| arn = `aws:arn:${account._id.toString()}:root`; | |
| } | |
| principal_arns.push(arn); | |
| } | |
| return { AWS: principal_arns }; | |
| } | |
| function _create_arn(dbg, principals, system_store) { | |
| if (!principals.AWS) return; | |
| const principal_arns = []; | |
| for (const principal of principals.AWS) { | |
| if (principal === '*') { | |
| principal_arns.push('*'); | |
| continue; | |
| } | |
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | |
| if (!account) { | |
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| continue; | |
| } | |
| let arn; | |
| if (account.owner) { | |
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | |
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | |
| } else { | |
| arn = `aws:arn:${account._id.toString()}:root`; | |
| } | |
| principal_arns.push(arn); | |
| } | |
| return { AWS: principal_arns }; | |
| } |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
lines 9 to 29, the upgrade currently skips wildcard principals ('*') and thus
drops anonymous access from upgraded policies; modify the loop to preserve
wildcards by adding '*' to principal_arns instead of continuing (e.g., when
principal === '*' push '*' into principal_arns), keeping other logic for
resolving account ARNs unchanged so the returned object can include both
resolved ARNs and the wildcard.
| if (principal === '*') continue; | ||
| const account = system_store.data.accounts.find(acc => acc.email.unwrap() === principal.unwrap()); | ||
| if (!account) { | ||
| dbg.log0(`Could not found the account with email: ${principal}`); |
There was a problem hiding this comment.
Avoid logging sensitive principal information.
Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.
Apply this diff:
- dbg.log0(`Could not found the account with email: ${principal}`);
+ dbg.log0(`Could not find account for principal in bucket policy`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbg.log0(`Could not found the account with email: ${principal}`); | |
| dbg.log0(`Could not find account for principal in bucket policy`); |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
line 16, the code currently logs the full principal email; change the log to
avoid printing sensitive principal information by either redacting or
hashing/truncating the principal before logging (e.g., replace the local-part
with asterisks or log only a UUID/hash of the principal), and update the message
to something like "Could not find the account for principal: [REDACTED]" while
preserving enough context for debugging; implement a small helper or inline
masking (do not log the raw principal) and use that in the processLogger/dbg
call.
| if (!_.isUndefined(bucket.s3_policy.Statement)) { | ||
| const new_policy = bucket.s3_policy; | ||
| new_policy.Statement = bucket.s3_policy.Statement.map(statement => ({ | ||
| Sid: statement.Sid, | ||
| Effect: statement.Effect, | ||
| Action: statement.Action, | ||
| Resource: statement.Resource, | ||
| Principal: _create_arn(dbg, statement.Principal, system_store), | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Essential: Statement fields may be lost during upgrade.
The upgrade script only copies Sid, Effect, Action, Resource, and Principal fields from each statement. This will drop other important fields such as Condition, NotPrincipal, NotAction, and NotResource if they exist in the original policy, breaking the policy semantics.
Apply this diff to preserve all statement fields:
const new_policy = bucket.s3_policy;
new_policy.Statement = bucket.s3_policy.Statement.map(statement => ({
- Sid: statement.Sid,
- Effect: statement.Effect,
- Action: statement.Action,
- Resource: statement.Resource,
+ ...statement,
Principal: _create_arn(dbg, statement.Principal, system_store),
}));|
|
||
| module.exports = { | ||
| run, | ||
| description: 'Update bucket policy Principal to ARN formate' |
There was a problem hiding this comment.
Fix typo in description.
The word "formate" should be "format".
Apply this diff:
- description: 'Update bucket policy Principal to ARN formate'
+ description: 'Update bucket policy Principal to ARN format'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| description: 'Update bucket policy Principal to ARN formate' | |
| description: 'Update bucket policy Principal to ARN format' |
🤖 Prompt for AI Agents
In src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js around
line 70, fix the typo in the description by replacing "formate" with "format" so
the description reads 'Update bucket policy Principal to ARN format'.
Signed-off-by: Naveen Paul <[email protected]>
naveenpaul1
force-pushed
the
bucket_arn_upgrade
branch
from
60824f7 to
a597682
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/endpoint/s3/s3_rest.js (1)
244-245: Remove debug console.log or convert to proper logging.This console.log statement should be removed before merging, or converted to use the
dbgmodule for consistency with the rest of the codebase. Additionally, be cautious about loggingbucket_ownerandowner_accountas they may contain sensitive information.src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (5)
9-29: Critical: Wildcard principals are lost during upgrade.The function skips wildcard (
'*') principals at line 13 but never adds them toprincipal_arns, causing wildcard principals to be dropped from the upgraded policy. This breaks bucket policies that grant anonymous access.
9-10: Preserve original principals when AWS principals are absent.The function returns
undefinedwhenprincipals.AWSdoesn't exist, which causes thePrincipalfield to becomeundefinedin the reconstructed statement. This corrupts the policy if a statement contains only non-AWS principals (e.g.,Service,Federated, orCanonicalUser).
16-16: Avoid logging sensitive principal information.Logging the principal email directly may expose sensitive account information. Consider redacting or using a safer logging format.
38-46: Essential: Statement fields may be lost during upgrade.The upgrade script only copies
Sid,Effect,Action,Resource, andPrincipalfields from each statement. This will drop other important fields such asCondition,NotPrincipal,NotAction, andNotResourceif they exist in the original policy, breaking the policy semantics.
70-70: Fix typo in description.The word "formate" should be "format".
🧹 Nitpick comments (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (1)
204-276: Consider cleaning up the IAM account in the after hook.The test creates an IAM account at line 248 but doesn't clean it up in the
mocha.afterhook (lines 278-281). While this may be acceptable for integration tests that reset the environment, consider adding cleanup for consistency with other test resources.Apply this diff to add IAM account cleanup:
mocha.after(async function() { + // Clean up IAM account if it exists + const iam_username = 'iam_username'; + try { + await rpc_client.account.delete_account({ email: iam_username }); + } catch (err) { + // Account may not exist if test didn't run + dbg.log0('IAM account cleanup skipped:', err.message); + } await s3.deleteBucket({ Bucket: BKT }); await s3.deleteBucket({ Bucket: BKT1 }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_rest.js(1 hunks)src/test/integration_tests/internal/test_upgrade_scripts.js(2 hunks)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit configuration file
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧠 Learnings (7)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
📚 Learning: 2025-08-11T06:12:12.318Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:6-22
Timestamp: 2025-08-11T06:12:12.318Z
Learning: In the noobaa-core upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js, bucket migration from the internal mongo pool to a new default pool is planned to be handled in separate future PRs with comprehensive testing, rather than being included directly in the pool removal script.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:12:46.728Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:9-17
Timestamp: 2025-08-08T13:12:46.728Z
Learning: In upgrade script src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js for noobaa-core, rely on structural detection (e.g., pool.mongo_info, and resource_type === 'INTERNAL') with name-prefix fallback for removing legacy mongo/internal pools, instead of depending solely on config.INTERNAL_STORAGE_POOL_NAME or config.DEFAULT_POOL_NAME. Handle multi-system stores and remove all matching pools in one change.
Applied to files:
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.jssrc/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:08:38.361Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/bucket_server.js:1324-1327
Timestamp: 2025-08-08T13:08:38.361Z
Learning: In src/server/system_services/bucket_server.js, the update_all_buckets_default_pool(req) handler expects req.rpc_params.pool_name to be a plain string (not a SensitiveString wrapper), so calling .unwrap() is not needed there.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:13:42.192Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1304-1307
Timestamp: 2025-08-08T13:13:42.192Z
Learning: In noobaa-core, do not rely on config.INTERNAL_STORAGE_POOL_NAME to detect backingstore pools. In src/server/system_services/pool_server.js, prefer checking pool.hosts_pool_info.backingstore or pool.cloud_pool_info.backingstore; fallback to prefix-matching the pool name against config.DEFAULT_POOL_NAME ('backingstores') to handle potential suffixes.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-26T06:29:52.095Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9195
File: src/upgrade/upgrade_scripts/5.20.0/remove_mongo_pool.js:25-31
Timestamp: 2025-08-26T06:29:52.095Z
Learning: In noobaa-core upgrade scripts, there is only one internal/mongo pool per system, so removing mongo_pools[0] is sufficient and correct rather than handling multiple pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
📚 Learning: 2025-08-08T13:10:36.141Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9182
File: src/server/system_services/pool_server.js:1314-1317
Timestamp: 2025-08-08T13:10:36.141Z
Learning: In src/server/system_services/pool_server.js (and config usage), the constant config.INTERNAL_STORAGE_POOL_NAME has been removed from the system. Future logic should not depend on this constant and should instead use config.DEFAULT_POOL_NAME or structural markers (e.g., pool.resource_type === 'INTERNAL' or pool.mongo_info) to identify internal/mongo pools.
Applied to files:
src/test/integration_tests/internal/test_upgrade_scripts.js
🧬 Code graph analysis (2)
src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (1)
src/test/integration_tests/internal/test_upgrade_scripts.js (12)
require(9-9)require(10-10)_(5-5)dbg(18-18)system_store(13-13)account(239-239)buckets(29-32)buckets(41-44)bucket(95-95)bucket(230-230)new_policy(109-109)new_policy(264-264)
src/test/integration_tests/internal/test_upgrade_scripts.js (2)
src/test/utils/coretest/coretest.js (2)
POOL_LIST(78-86)src/upgrade/upgrade_scripts/5.21.0/upgrade_bucket_policy_principal.js (2)
account(14-14)new_policy(39-39)
⏰ 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). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
🔇 Additional comments (3)
src/test/integration_tests/internal/test_upgrade_scripts.js (3)
7-7: LGTM!The addition of
POOL_LISTto the imports is appropriate and used in the new test case at line 246.
12-12: LGTM!The
SensitiveStringimport is correctly added and used to wrap sensitive principal values in the test policy at lines 212 and 220.
15-15: LGTM!The upgrade script import is appropriately added and invoked at line 260 to test the ARN-based principal upgrade functionality.
liranmauda
left a comment
liranmauda
left a comment
There was a problem hiding this comment.
We should first fix the new bucket policies to follow the same ARN format when creating bucket policy, and only then merge this PR.
| public_access_block, | ||
| } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket); | ||
|
|
||
| console.log("authorize_request_policy ====>>>", bucket_owner, owner_account); |
There was a problem hiding this comment.
Why use console.log and not dbg.log0 ?
Is it a leftover, and we want to remove it completely?
| console.log("authorize_request_policy ====>>>", bucket_owner, owner_account); | |
| dbg.log1("authorize_request_policy ====>>>", bucket_owner, owner_account); |
| let arn; | ||
| if (account.owner) { | ||
| const iam_path = account.iam_path || iam_constants.IAM_DEFAULT_PATH; | ||
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; |
There was a problem hiding this comment.
All the places there are aws:arn...
We should use arn:aws: instead, and maybe it's worth setting a prefix const and not rewriting that every time. The same goes for the suffix.
| arn = `aws:arn:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; | |
| arn = `arn:aws:${account._id.toString()}:user${iam_path}${account.email.unwrap()}`; |
| for (const bucket of system_store.data.buckets) { | ||
| // Do not update if there are no bucket policy. | ||
| if (!bucket.s3_policy) continue; | ||
| if (!_.isUndefined(bucket.s3_policy.Statement)) { |
There was a problem hiding this comment.
Why do we need loadsh?
| if (!_.isUndefined(bucket.s3_policy.Statement)) { | |
| if (bucket.s3_policy.Statement !== undefined) { |
2 participants
Describe the Problem
Update the bucket policy Principal with ARN, not its a email but we will be updating it with ARN, Existing bucket policy also will be replaced by new ARN,
Explain the Changes
GAP
Will handle in next PR
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
test_upgrade_scripts.jstest and make suretest upgrade bucket policy to ARN version 5.21.0is sucess.Summary by CodeRabbit
Improvements
Testing