S3UTILS-216 Tool to detects missing permissions on CRR roles #365
Conversation
Hello nicolas2bert,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #365 +/- ##
====================================================
- Coverage 44.70% 43.76% -0.95%
====================================================
Files 83 84 +1
Lines 5756 5962 +206
Branches 1215 1256 +41
====================================================
+ Hits 2573 2609 +36
- Misses 3138 3307 +169
- Partials 45 46 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
| // =========================================================================== | ||
| // Repd Client | ||
| // =========================================================================== | ||
| class RepdClient { |
There was a problem hiding this comment.
nit: consider the folder Clients/RepdClient.js if you think another script could later use this client
There was a problem hiding this comment.
I wanted to have a a single self-contained file with no dependencies.
Some reasons behind it:
- we can copy it easily
- it runs anywhere Node.js exists
- No npm install needed on target
There was a problem hiding this comment.
Do we need functional tests for this? Also, critical code paths like RepdClient class, VaultKeys methods, checkBucketPermissions orchestration, and protocol handling are completely untested(unit tests)
Also, please do update the CLAUDE.md, the new replicationAudit/ directory introduces a significant new operational category but is not reflected in CLAUDE.md's script categories section.
| } | ||
|
|
||
| return { | ||
| data: JSON.parse(buffer.slice(PROTOCOL.HEADER_SIZE, totalLength).toString()), |
There was a problem hiding this comment.
Is it worth adding a try catch for JSON.parse?
When called from handleData() event handler, parsing errors will crash the script and leave TCP connections open
There was a problem hiding this comment.
The risk is very low. Repd is a well-tested internal service that always returns valid JSON. A parse error would means there is a serious bug in repd itself or network corruption.
I would leave it as-is for this "diagnostic" script. The try-catch would be defensive overkill for a tool that runs occasionally and targets a trusted internal service.
| const CONFIG = { | ||
| inputFile: process.argv[2] || '/root/buckets-with-replication.json', | ||
| leaderIp: process.argv[3] || '127.0.0.1', | ||
| outputFile: process.argv[4] || '/root/missing-replication-permissions.json', | ||
| repdPort: 4300, | ||
| dbName: 'vaultdb', | ||
| includePolicies: process.argv.includes('--include-policies'), | ||
| requestTimeoutMs: 10000, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Is it intentional that the script uses process.argv for configuration (inputFile, leaderIp, outputFile, flags) instead of following the established environment variable pattern used by other scripts like crrExistingObjects.js and cleanupNoncurrentVersions.js.
There was a problem hiding this comment.
It felt simpler for crictl exec workflow:
crictl exec $CONTAINER node script.js input.json $IP output.json
instead of
crictl exec -e VAR=value ...
replicationAudit/README.md
Outdated
| root@<supervisor-ip>:/root/ | ||
| ``` | ||
|
|
||
| 3. Copy the script to a store node: |
There was a problem hiding this comment.
A storage node is not necessarily a stateless S3 connector.
Instead of using saltstack in this procedure, use Ansible, and target one of the runners_s3 host (for example runners_s3[0]).
replicationAudit/README.md
Outdated
|
|
||
| ```bash | ||
| ssh -i ~/.ssh/cloud root@<supervisor-ip> \ | ||
| "salt 'store-1' cmd.run 'crictl ps | grep scality-vault'" |
There was a problem hiding this comment.
crictl ps | awk '/scality-vault/ {print $1}'
replicationAudit/README.md
Outdated
| scp -i ~/.ssh/cloud replicationAudit/list-buckets-with-replication.sh root@$SUPERVISOR:/root/ | ||
| scp -i ~/.ssh/cloud replicationAudit/check-replication-permissions.js root@$SUPERVISOR:/root/ | ||
|
|
||
| # Step 2: Copy scripts to store node |
There was a problem hiding this comment.
Instead of SSHing to the supervisor at each step, add an intermediate step here to connect to the sup's shell.
There was a problem hiding this comment.
I like when each "steps" are quite independent/isolated.
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following reviewers are expecting changes from the author, or must review again: The following options are set: create_integration_branches |
| 1. Copy the script to the supervisor: | ||
| ```bash | ||
| scp replicationAudit/list-buckets-with-replication.sh root@<supervisor-ip>:/root/ | ||
| ``` | ||
|
|
||
| 2. Copy the script to an S3 connector node: | ||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m copy \ | ||
| -a 'src=/root/list-buckets-with-replication.sh dest=/root/'" | ||
| ``` | ||
|
|
||
| 3. Run the script: | ||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'bash /root/list-buckets-with-replication.sh'" | ||
| ``` | ||
|
|
||
| 4. Retrieve the output file: | ||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'cat /root/buckets-with-replication.json'" | ||
| ``` |
There was a problem hiding this comment.
80% of our customers don't have ssh installed on their laptop.
add an intermediate step that thells the reader to connect through SSH to the supervisor, as root.
and then remove the unnecessary "ssh root@"
replicationAudit/README.md
Outdated
| 3. Copy the script to an S3 connector node: | ||
|
|
||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m copy \ | ||
| -a 'src=/root/check-replication-permissions.js dest=/root/'" | ||
| ``` | ||
|
|
||
| 4. Find the vault-metadata repd leader IP: | ||
|
|
||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'curl -s http://localhost:5300/_/raft/leader'" | ||
| ``` | ||
|
|
||
| This returns JSON like `{"ip":"10.160.116.162","port":4300}` - use the `ip` value. | ||
|
|
||
| **Note:** Vault metadata uses port 5300 for admin. | ||
|
|
||
| 5. Find the vault container ID: | ||
|
|
||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'crictl ps | awk \"/scality-vault/ {print \\\$1}\"'" | ||
| ``` | ||
|
|
||
| 6. Copy files to `/var/tmp` (mounted in vault container) and run the script: | ||
|
|
||
| ```bash | ||
| VAULT_CONTAINER=<vault-container-id> | ||
| LEADER_IP=<leader-ip-from-step-4> | ||
|
|
||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'cp /root/check-replication-permissions.js /var/tmp/ && cp /root/buckets-with-replication.json /var/tmp/ && crictl exec $VAULT_CONTAINER node /var/tmp/check-replication-permissions.js /var/tmp/buckets-with-replication.json $LEADER_IP /var/tmp/missing.json'" | ||
| ``` | ||
|
|
||
| 7. Retrieve the output: | ||
|
|
||
| ```bash | ||
| ssh root@<supervisor-ip> "cd /srv/scality/s3/s3-offline/federation && \ | ||
| ansible -i env/<env>/inventory runners_s3[0] -m shell \ | ||
| -a 'cat /var/tmp/missing.json'" | ||
| ``` |
There was a problem hiding this comment.
same here: add an intermediate step to connect to the supervisor.
replicationAudit/README.md
Outdated
| # Step 2: Copy scripts to S3 connector node | ||
| ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m copy \ | ||
| -a 'src=/root/list-buckets-with-replication.sh dest=/root/'" | ||
| ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m copy \ | ||
| -a 'src=/root/check-replication-permissions.js dest=/root/'" | ||
|
|
||
| # Step 3: Run list-buckets-with-replication.sh | ||
| ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m shell \ | ||
| -a 'bash /root/list-buckets-with-replication.sh'" | ||
|
|
||
| # Step 4: Find the vault-metadata repd leader IP (port 5300) | ||
| LEADER_IP=$(ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m shell \ | ||
| -a 'curl -s http://localhost:5300/_/raft/leader'" | sed -n 's/.*"ip":"\([^"]*\)".*/\1/p') | ||
| echo "Leader IP: $LEADER_IP" | ||
|
|
||
| # Step 5: Find the vault container ID | ||
| VAULT_CONTAINER=$(ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m shell \ | ||
| -a 'crictl ps | awk \"/scality-vault/ {print \\\$1}\"'" | tail -1) | ||
| echo "Vault container: $VAULT_CONTAINER" | ||
|
|
||
| # Step 6: Copy files to /var/tmp and run the permission check script | ||
| ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m shell \ | ||
| -a 'cp /root/check-replication-permissions.js /var/tmp/ && cp /root/buckets-with-replication.json /var/tmp/ && crictl exec $VAULT_CONTAINER node /var/tmp/check-replication-permissions.js /var/tmp/buckets-with-replication.json $LEADER_IP /var/tmp/missing.json'" | ||
|
|
||
| # Step 7: Retrieve results | ||
| ssh root@$SUPERVISOR "cd $FEDERATION_DIR && \ | ||
| ansible -i env/$ENV/inventory runners_s3[0] -m shell \ | ||
| -a 'cat /var/tmp/missing.json'" | ||
| ``` |
There was a problem hiding this comment.
same here - add an intermediate step to connect to the sup
| echo "]" >> "$TMP_DIR/results.json" | ||
|
|
||
| # Calculate final stats | ||
| local repl_count | ||
| repl_count=$(jq 'length' "$TMP_DIR/results.json") |
There was a problem hiding this comment.
It works but it's ugly and over-complicating the code.
No need to enclose the results between square brackets and use append_result to have a list of json objects. You could have a non-comma-separated list of json objects in $TMP_DIR/results.json and use jq --slurp to make it an array when needed.
There was a problem hiding this comment.
I did not know about jq --slurp , thanks
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
nicolas2bert
force-pushed
the
improvement/S3UTILS-216/tool-crr
branch
from
8780054 to
d938738
Compare
History mismatchMerge commit #ede3f330c7a1a3198bfc45f0c2088809c78e83ca on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
|
@bert-e reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
@bert-e approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-216. Goodbye nicolas2bert. The following options are set: approve, create_integration_branches |
No description provided.