Skip to content

[Backport perf-v17] fix(integration-tests): align AWS keystore tests with the Secrets Manager backend - #15680

Open
scylladbbot wants to merge 2 commits into
scylladb:branch-perf-v17from
scylladbbot:backport/15670/to-perf-v17
Open

[Backport perf-v17] fix(integration-tests): align AWS keystore tests with the Secrets Manager backend#15680
scylladbbot wants to merge 2 commits into
scylladb:branch-perf-v17from
scylladbbot:backport/15670/to-perf-v17

Conversation

@scylladbbot

Copy link
Copy Markdown

Fixes the 9 Secrets Manager related integration test failures seen in PR-15647 build 1. They are unrelated to that PR (a renovate gemini bump) — the tests were still written for the S3 keystore backend, which KeyStore no longer defaults to.

What was failing

integration.test_aws_services::test_01_keystore
integration.test_aws_services::test_02_keystore_sync
integration.test_aws_services::test_03_provision[on_demand|spot|spot_fleet]
integration.test_aws_services::test_04_get_scylla_ami_versions
integration.test_aws_services::test_04b_get_scylla_ami_versions_full_tag
integration.test_aws_services::test_05_ec2_client_spot
integration.test_sync::test_multiple_sync_on_lots_of_files

all with ResourceNotFoundException ... calling the DescribeSecret operation.

Root causes

Three distinct bugs, stacked — CI only ever showed the first because it aborted at session-fixture time.

  1. Retired keys. fixture_get_real_keys synced scylla-qa-ec2 and scylla-test. Those are the legacy RSA keys; they exist in the scylla-qa-keystore bucket but were never mirrored into Secrets Manager and never will be. Everything in SCT (including sct.py) already uses scylla_test_id_ed25519. Verified against the real account:

    sct/scylla-qa-ec2              -> ResourceNotFoundException
    sct/scylla-test                -> ResourceNotFoundException
    sct/scylla_test_id_ed25519     -> sct/scylla_test_id_ed25519
    sct/scylla_test_id_ed25519.pub -> sct/scylla_test_id_ed25519.pub
    
  2. Fixture seeded the wrong backend. keystore_configure populated the moto S3 bucket only, so with the keys fixed the tests still failed on GetSecretValue. Now the same entries are seeded into moto's Secrets Manager under the sct/ prefix, and the fixture uses the KEYSTORE_* constants instead of repeating the bucket name and prefix.

  3. Broken MOTO_AMIS_PATH. The AMI fixtures live in unit_tests/test_data/, but the path was computed relative to the test file, which since the move to unit_tests/integration/ resolves to a directory that does not exist. Moto loaded no AMI data and returned a 500 for every DescribeImages, so the four EC2 tests could not pass even with a working keystore.

Also fixed

get_obj_if_needed chmods the downloaded key but wrote the <file>.version sidecar at the umask (0o664). The sidecar lands next to the key it describes, usually in ~/.ssh, where ssh refuses a directory holding group/world readable material. test_02_keystore_sync asserts every synced file is 0600 and caught it. Covered by a new unit test.

Testing

unit_tests/integration/test_aws_services.py unit_tests/integration/test_sync.py   9 passed
unit_tests/unit/test_keystore.py                                                120 passed
  • (cherry picked from commit 5ebb5e3)
  • (cherry picked from commit 8ab5685)
  • (cherry picked from commit b92d614)

Parent PR: #15670

@scylladbbot

Copy link
Copy Markdown
Author

@fruch - This PR has conflicts, therefore it was moved to draft
Please resolve them and mark this PR as ready for review by removing the conflicts label

@fruch fruch added the test-integration Enable running the integration tests suite label Aug 4, 2026
@fruch
fruch force-pushed the backport/15670/to-perf-v17 branch from 40b73f8 to 4c31e3e Compare August 4, 2026 19:22
@fruch
fruch marked this pull request as ready for review August 4, 2026 19:22
@fruch fruch removed the conflicts label Aug 4, 2026
@fruch

fruch commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Resolved the backport conflict and adapted it to this branch.

Path mappingbranch-perf-v17 has no unit_tests/integration/ tree, so the files land as unit_tests/test_aws_services.py and test_sync.py (repo root). Verified no stray unit_tests/integration/... copies were created.

Conflict (unit_tests/test_aws_services.py, keystore_configure): the base branch already had a hand-written Secrets Manager mirroring block appended after the S3 seeding, while the cherry-pick rewrote the S3 seeding into a shared entries dict that seeds both backends. Kept the incoming (upstream) shape — one entries dict feeding both S3 and Secrets Manager via KEYSTORE_S3_BUCKET / KEYSTORE_SM_PREFIX / KEYSTORE_SM_REGION — and dropped the now-duplicated hand-written block. The fixture body is now byte-identical to master.

Dropped commit: fix(integration-tests): point MOTO_AMIS_PATH at the real test_data dir is not applicable here. It changed Path(__file__).parent to .parents[1] because on master the test moved into unit_tests/integration/. On this branch the test still lives in unit_tests/, next to unit_tests/test_data/, so .parent is already correct and .parents[1] resolves to a non-existent repo-root test_data/. Confirmed empirically: with the backported .parents[1], test_04_get_scylla_ami_versions fails; with .parent it passes.

Verified locally (Python 3.13, real AWS creds + moto):

  • pytest unit_tests/test_aws_services.py unit_tests/unit/test_keystore.py -> 117 passed
  • includes the two new sidecar-permission tests from the first commit
  • ruff check / ruff format --check clean; full pre-commit hook set passed on commit
  • no conflict markers remain in any file touched by either commit

@fruch

fruch commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Integration test results (build #2) — this PR's own tests pass; branch-perf-v17's suite is broken independently

continuous-integration/jenkins/pr-merge = fail with "The build of this commit was aborted". This is not a defect in this backport:

  • jenkins/precommit pass, jenkins/unittests pass, jenkins/lint_test_cases pass
  • Build ran 64 min and ended ABORTED because the integration stage hit its 45-minute timeout: Cancelling nested steps due to timeoutTimeout has been exceededNo test report files were found.

The tests this PR fixes are green. There are no test_aws_services or test_sync failures in the run — including test_04b_get_scylla_ami_versions_full_tag, which confirms the MOTO_AMIS_PATH revert was correct for this branch's flat layout (the backported .parents[1] would have resolved to a nonexistent repo-root test_data/ and broken test_03/04/04b/05).

All 19 failures are in modules this PR does not touch:

unit_tests/test_cassandra_stress_thread.py  (9 cases)
unit_tests/test_cluster.py                  (2 cases)
unit_tests/test_config_get_version_based_on_conf.py  (5 cases)
unit_tests/test_config.py                   (3 cases)

Verified root cause for the test_config* failures: unit_tests/test_configs/ does not exist on branch-perf-v17 (git ls-tree upstream/branch-perf-v17 unit_tests/test_configs/ → empty), while it is present on master, branch-2026.2 and branch-2026.1. Tests referencing unit_tests/test_configs/minimal_test_case.yaml therefore fail at fixture setup. branch-2025.1 has the identical gap — see #15690, whose build aborted the same way for the same reason.

Consequence for reviewers: the test-integration stage cannot go green on branch-perf-v17 today, and neither this PR nor #15682 can make it green. Both remain correct and worth merging on their own merits — this PR's keystore work is verified by unit_tests/test_aws_services.py + unit_tests/unit/test_keystore.py117 passed locally against real Secrets Manager, plus the green precommit/unittests above.

Separate remediation this branch needs, all pre-existing and deliberately out of scope here:

  1. Backport or adapt unit_tests/test_configs/.
  2. Investigate the 45-minute integration timeout, which prevents any JUnit result from being published at all.
  3. Backport master's arch-aware docker-image-name-{arch} lookup in get_specific_tag_of_docker_image00-Build.txt now publishes only the -x86_64 suffixed key, so scylladb/scylla-nightly tag resolution is broken on this branch today (details in [Backport perf-v17] fix(integration-tests): stop pinning artifact URLs that get garbage collected #15682).

fruch added 2 commits August 11, 2026 13:17
`get_obj_if_needed` chmods the downloaded key to the requested
permissions but wrote the `<file>.version` sidecar with whatever the
umask allowed (0o664 on a default setup). The sidecar lands next to the
key it describes -- usually in `~/.ssh` -- where ssh refuses to use a
directory holding group/world readable material.

Apply the same permissions to the sidecar. The chmod sits outside the
download branch on purpose: a sidecar whose version is still current is
never rewritten, so one left at 0o664 by an earlier sync would keep
those permissions forever if the fix only ran after a download.

(cherry picked from commit 5ebb5e3)
KeyStore now reads from Secrets Manager by default, which broke these
tests in two ways:

* `scylla-qa-ec2` and `scylla-test` were never mirrored into Secrets
  Manager and never will be -- they are the legacy RSA keys, replaced
  everywhere in SCT by `scylla_test_id_ed25519`. Syncing them raised
  ResourceNotFoundException from the session-scoped `fixture_get_real_keys`,
  failing every test in `test_aws_services.py` on setup and taking
  `test_multiple_sync_on_lots_of_files` down with it. Sync only the
  ed25519 pair; keep 60 entries in the sync test so it still exercises
  the boto3 session lock.

* `keystore_configure` seeded the moto S3 bucket only, so once the keys
  synced the tests still failed on GetSecretValue. Seed the same entries
  into moto's Secrets Manager under the `sct/` prefix, and reuse the
  keystore constants instead of repeating the bucket name and prefix.

(cherry picked from commit b92d614)
@fruch
fruch force-pushed the backport/15670/to-perf-v17 branch from bda86f3 to 0f119ab Compare August 11, 2026 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-integration Enable running the integration tests suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants