Updated commit hash in ceph tests #9152
Conversation
achouhan09
requested review from
nadavMiz,
romayalon,
a team and
alphaprinz
and removed request for
a team
|
""" WalkthroughThe changes update the Ceph S3 test configuration to include new IAM-related sections and keys, adjust the setup scripts to use placeholder substitution for access/secret keys via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Suggested labels
Suggested reviewers
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. 🔧 ESLint
npm error Exit handler never called! Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf(2 hunks)src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js(1 hunks)src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh(1 hunks)
📓 Path-based instructions (1)
src/test/**/*.*
⚙️ CodeRabbit Configuration File
src/test/**/*.*: Ensure that the PR includes tests for the changes.
Files:
src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.shsrc/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.jssrc/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
🧰 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/external_tests/ceph_s3_tests/test_ceph_s3_deploy.shsrc/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.jssrc/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
⏰ 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). (2)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (4)
src/test/external_tests/ceph_s3_tests/test_ceph_s3_deploy.sh (1)
20-20: Validated new CEPH_TESTS_VERSION hash
- The commit aafacab2017bec1dd1ce3159105455bb7e9447b1 exists in the ceph/s3-tests repository on master.
- It is only 4 days old, well within the 600-day limit.
No further changes required.
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js (1)
95-96: Good improvement in credential injection consistency.The change from appending keys to replacing placeholders using
sedimproves consistency and aligns well with the updated config file structure. The OS-specific handling for macOS is correctly implemented.Also applies to: 101-102
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf (2)
22-28: LGTM: IAM naming configuration added.The new IAM naming prefixes support the enhanced IAM functionality mentioned in the PR objectives.
62-63: LGTM: Tenant name configuration added.The tenant field addition supports enhanced tenant testing capabilities.
| ## replace these with your access keys | ||
| access_key = s3_access_key | ||
| secret_key = s3_secret_key |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify placeholder substitution doesn't conflict with main section.
The s3_access_key and s3_secret_key placeholders are also used in the [s3 main] section. Ensure the setup script correctly handles these duplicate placeholders and assigns appropriate values to each section.
🏁 Script executed:
#!/bin/bash
# Description: Check how s3_access_key and s3_secret_key placeholders are used across sections
# Expected: Should find usage in both [s3 main] and [s3 alt] sections
echo "Checking s3_access_key usage across config sections:"
rg -A 2 -B 2 "s3_access_key" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
echo -e "\nChecking s3_secret_key usage across config sections:"
rg -A 2 -B 2 "s3_secret_key" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
echo -e "\nChecking setup script for handling of these placeholders:"
rg -A 2 -B 2 "s3_access_key\|s3_secret_key" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.jsLength of output: 1130
🏁 Script executed:
#!/bin/bash
# List any setup-related files in the ceph_s3_tests directory
echo "Setup files under src/test/external_tests/ceph_s3_tests/:"
fd setup src/test/external_tests/ceph_s3_tests/
# Display the beginning of the setup script to inspect placeholder handling
echo -e "\n--- test_ceph_s3_config_setup.js (first 200 lines) ---"
sed -n '1,200p' src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js || echo "File not found or cannot be read"Length of output: 5133
Ensure distinct placeholders for tenant credentials in the alt section
The setup script only replaces s3_access_key/s3_secret_key once, so both your main and alt sections end up using the same S3 account keys. To correctly assign tenant credentials, use separate placeholders in the [s3 alt] section or update the script:
• File: src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
- Around lines 71–73 in the
[s3 alt]section, change:## replace these with your access keys
-access_key = s3_access_key
-secret_key = s3_secret_key
+access_key = tenant_access_key
+secret_key = tenant_secret_key
```
• File: src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js
- Verify it already replaces
tenant_access_key/tenant_secret_keywith the tenant’s keys (as it does) and that it still replacess3_access_key/s3_secret_keyfor the primary account.
This change will ensure the alt tests receive the tenant’s credentials rather than the main S3 account’s.
📝 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.
| ## replace these with your access keys | |
| access_key = s3_access_key | |
| secret_key = s3_secret_key | |
| ## replace these with your access keys | |
| access_key = tenant_access_key | |
| secret_key = tenant_secret_key |
🤖 Prompt for AI Agents
In src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf around lines
71 to 73 in the [s3 alt] section, replace the placeholders s3_access_key and
s3_secret_key with distinct placeholders like tenant_access_key and
tenant_secret_key. This ensures the alt section uses separate tenant
credentials. Also, verify in
src/test/external_tests/ceph_s3_tests/test_ceph_s3_config_setup.js that
tenant_access_key and tenant_secret_key are replaced with tenant keys while
s3_access_key and s3_secret_key remain for the primary account.
| #following section needs to be added for all sts-tests | ||
| [iam] | ||
| #used for iam operations in sts-tests | ||
| #email from vstart.sh | ||
| email = ceph.alt@noobaa.com | ||
|
|
||
| #user_id from vstart.sh | ||
| user_id = 56789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234 | ||
|
|
||
| #access_key from vstart.sh | ||
| access_key = tenant_access_key | ||
|
|
||
| #secret_key vstart.sh | ||
| secret_key = tenant_secret_key | ||
|
|
||
| #display_name from vstart.sh | ||
| display_name = cephalt | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify IAM section credentials don't conflict with other sections.
The [iam] section uses tenant_access_key and tenant_secret_key placeholders, which are also used in other sections. Additionally, the email ceph.alt@noobaa.com and user_id are shared with the [s3 alt] section, which might cause conflicts.
🏁 Script executed:
#!/bin/bash
# Description: Analyze credential and identifier reuse across config sections
# Expected: Should identify potential conflicts in email, user_id, and key placeholders
echo "Checking tenant_access_key usage across sections:"
rg -A 1 -B 1 "tenant_access_key" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
echo -e "\nChecking email address reuse:"
rg -A 1 -B 1 "ceph\.alt@noobaa\.com" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf
echo -e "\nChecking user_id reuse:"
rg -A 1 -B 1 "56789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234" src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.confLength of output: 1542
Avoid Shared Credentials Across Sections
Multiple sections in src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf are reusing the same placeholders and identifiers, which will cause your IAM- and S3-related tests to run under the same identity:
• Access keys:
– tenant_access_key appears in [default], [iam], [iam root], [iam alt root]
– tenant_secret_key likewise
• Email:
– ceph.alt@noobaa.com used in both the [s3 alt] and [iam] sections
• User ID:
– 56789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234 repeated across multiple sections
Please assign unique placeholder values (or parameterize these via your test harness) so that each test section operates with its own credentials and avoids collisions.
🤖 Prompt for AI Agents
In src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf between lines
75 and 92, the configuration reuses the same placeholder credentials like
access_key, secret_key, email, and user_id across multiple sections, causing
identity collisions in tests. To fix this, assign unique placeholder values for
each section's access_key, secret_key, email, and user_id, or parameterize these
values through the test harness to ensure each test section uses distinct
credentials and avoids conflicts.
| # iam account root user for iam_account tests | ||
| [iam root] | ||
| access_key = tenant_access_key | ||
| secret_key = tenant_secret_key | ||
| user_id = 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef | ||
| email = main.alt@noobaa.com | ||
|
|
||
| # iam account root user in a different account than [iam root] | ||
| [iam alt root] | ||
| access_key = tenant_access_key | ||
| secret_key = tenant_secret_key | ||
| user_id = 56789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234 | ||
| email = ceph.alt@noobaa.com |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review root IAM account configuration for consistency.
Both [iam root] and [iam alt root] sections use tenant_access_key and tenant_secret_key placeholders. This means they will receive the same credentials from the setup script, which may not be the intended behavior for testing different root accounts.
Consider using distinct placeholder names for different root accounts to ensure they receive different credentials:
# iam account root user for iam_account tests
[iam root]
-access_key = tenant_access_key
-secret_key = tenant_secret_key
+access_key = iam_root_access_key
+secret_key = iam_root_secret_key
user_id = 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
email = main.alt@noobaa.com
# iam account root user in a different account than [iam root]
[iam alt root]
-access_key = tenant_access_key
-secret_key = tenant_secret_key
+access_key = iam_alt_root_access_key
+secret_key = iam_alt_root_secret_key
user_id = 56789abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234
email = ceph.alt@noobaa.comThis would require updating the setup script to handle these new placeholders appropriately.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/external_tests/ceph_s3_tests/test_ceph_s3_config.conf around lines
93 to 105, the [iam root] and [iam alt root] sections both use the same
placeholders tenant_access_key and tenant_secret_key, causing them to share
identical credentials. To fix this, rename the placeholders in the [iam alt
root] section to distinct names like alt_tenant_access_key and
alt_tenant_secret_key, and update the setup script accordingly to provide
different credentials for these placeholders, ensuring the two root accounts
have separate access keys and secret keys for proper testing.
29e5498 to
ca05916
Compare
Signed-off-by: Aayush <aayush@li-2392f9cc-287a-11b2-a85c-9fcc04b05da6.ibm.com>
Describe the Problem
After updating the ceph commit hash, s3 tests were failing for both NSFS and non-NSFS configurations. The failures were caused by missing required configuration fields that the updated ceph test suite expects.
Explain the Changes
ceph-s3 output:
nsfs-ceph-s3 output:
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
make test-cephs3andmake test-nsfs-cephs3Summary by CodeRabbit
Summary by CodeRabbit
New Features
Chores
Documentation