Skip to content

[Backport 2025.1] fix(integration-tests): align AWS keystore tests with the Secrets Manager backend - #15690

Open
scylladbbot wants to merge 2 commits into
scylladb:branch-2025.1from
scylladbbot:backport/15670/to-2025.1
Open

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

Conversation

@scylladbbot

@scylladbbot scylladbbot commented Aug 4, 2026

Copy link
Copy Markdown

Backport of #15670 to branch-2025.1, reduced to the parts that apply to this branch.

What this backport delivers on 2025.1

  1. sdcm/keystore.py — chmod the Secrets Manager version sidecar. get_obj_if_needed chmods the downloaded key to the requested permissions 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. 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. Also adds the KEYSTORE_SM_PREFIX constant. Covered by two new tests in unit_tests/unit/test_keystore.py.

  2. test_sync.py — drop the retired keys. KeyStore on this branch already defaults to the Secrets Manager backend, and scylla-qa-ec2 / 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 raises ResourceNotFoundException, so test_multiple_sync_on_lots_of_files could not pass. Now syncs only the ed25519 pair, keeping 60 entries so the test still exercises the boto3 session lock.

What was intentionally dropped

The parent PR's third change — the keystore_configure fixture rework that seeds moto's Secrets Manager, plus the follow-up MOTO_AMIS_PATH fix — lives entirely in unit_tests/integration/test_aws_services.py. That file does not exist on branch-2025.1, and master's copy imports parse_scylla_version_tag from sdcm.utils.version_utils, which does not exist anywhere on this branch. Materializing the file here would fail at import time with an ImportError during collection, so it is not carried over. The MOTO_AMIS_PATH commit touched only that file and is therefore empty on this branch and dropped as well.

Consequence for reviewers: this backport does not fix the nine test_aws_services.py failures described in the parent PR — those tests do not exist on 2025.1. It fixes the keystore sidecar permissions and test_multiple_sync_on_lots_of_files.

Note on the test-integration label

pytest.mark.integration is not registered on branch-2025.1, so the root test_sync.py only emits PytestUnknownMarkWarning: Unknown pytest.mark.integration rather than being selected by the marker. The labelled integration stage may therefore not collect anything from this diff — worth keeping in mind when reading a green integration run as evidence that the sync fix was exercised.

Testing

unit_tests/unit/test_keystore.py                    108 passed
  -k sidecar                                          3 passed
test_sync.py --collect-only                         1 test collected
pre-commit (ruff, ruff-format, hygiene, commitlint)  all passed
  • (cherry picked from commit 5ebb5e3)
  • (cherry picked from commit b92d614)
  • (not backported: 8ab5685MOTO_AMIS_PATH, empty on this branch)

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

@scylladbbot scylladbbot added this to the 2025.1.15 milestone Aug 4, 2026
@fruch fruch added the test-integration Enable running the integration tests suite label Aug 4, 2026
@fruch
fruch force-pushed the backport/15670/to-2025.1 branch from 3a96415 to 2f6da3a Compare August 4, 2026 19:22
@fruch fruch removed the conflicts label Aug 4, 2026
@fruch
fruch marked this pull request as ready for review August 4, 2026 19:23
@fruch

fruch commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Integration test results (build #2) — the branch-2025.1 integration suite is broadly broken, independent of this PR

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
  • The build ran 54 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.
  • 64 test failures in the integration stage, spanning modules this PR does not touch (test_cql_stress_cassandra_stress_thread, test_gemini_thread, test_run_cqlsh, and every parametrization of test_config_get_version_based_on_conf).

Root cause for the whole test_config_get_version_based_on_conf module, verified against the branch tree: the test sets

monkeypatch.setenv("SCT_CONFIG_FILES", "unit_tests/test_configs/minimal_test_case.yaml")

but unit_tests/test_configs/ does not exist on branch-2025.1 (git ls-tree upstream/branch-2025.1 unit_tests/test_configs/ → empty; master moved it there in c16e41a55c, never backported). So the autouse fixture fails and every test in the module errors — which is why test_images[*], test_baremetal, test_unified_package and friends all fail here.

Consequence for reviewers: the test-integration stage cannot go green on branch-2025.1 today, and neither this PR nor #15686 can make it green. Both backports are still correct and worth merging on their own merits — the keystore sidecar chmod and the retired-key sync fix in this PR are verified by unit_tests/unit/test_keystore.py (108 passed) and its precommit/unittests runs above — but the branch needs separate remediation work before its integration suite means anything:

  1. Backport or adapt unit_tests/test_configs/ (or point the tests at the branch's own layout).
  2. Remove the stale duplicate unit_tests/test_config_get_version_based_on_conf.py (205 lines), which still pins the dead November-2023 relocatable and is collected by pytest -m integration unit_tests/ alongside the current 352-line copy.
  3. Investigate the 45-minute integration timeout, which prevents any result from being reported at all.

Those are pre-existing branch problems, deliberately left out of scope here.

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 reads from Secrets Manager by default, and `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 raises ResourceNotFoundException,
so `test_multiple_sync_on_lots_of_files` could not pass. Sync only the
ed25519 pair; keep 60 entries so the test still exercises the boto3
session lock.

Backport note: the upstream commit also reworked the `keystore_configure`
fixture of `unit_tests/integration/test_aws_services.py` to seed moto's
Secrets Manager, and a follow-up commit fixed that file's
`MOTO_AMIS_PATH`. Neither applies here -- this branch has no
`test_aws_services.py`, and master's copy imports
`parse_scylla_version_tag`, which does not exist on this branch. Only
the sync-test hunk is backported; on this branch the test lives at the
repository root rather than under `unit_tests/integration/`.

(cherry picked from commit b92d614)
@fruch
fruch force-pushed the backport/15670/to-2025.1 branch from c53daba to cd929ef 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