Add Akeyless Secret Store/SSH Plugin#147
Conversation
|
@jessicamack - PR is ready for review. |
|
The CI didn't start for some reason (or the runs got outdated and GH cleared them?). Re-opening the PR to trigger the CI. |
|
Tip I wonder if this needs to be a part of |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (92.27%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Re the tip about whether this needs to be part of |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/akeyless_test.py (2)
205-207: Minor: En-dash character in comment.Line 206 uses an en-dash (–) instead of a hyphen (-).
Suggested fix
-# akeyless_ssh_backend – SSH certificate plugin +# akeyless_ssh_backend - SSH certificate plugin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/akeyless_test.py` around lines 205 - 207, The comment header line containing "akeyless_ssh_backend – SSH certificate plugin" uses an en-dash; replace the en-dash (–) with a standard hyphen (-) so the line reads "akeyless_ssh_backend - SSH certificate plugin" to keep ASCII characters consistent in tests/akeyless_test.py.
89-91: Minor: En-dash character in comment.Line 90 uses an en-dash (–) instead of a hyphen (-). This is a minor formatting inconsistency.
Suggested fix
-# akeyless_backend – secret store plugin +# akeyless_backend - secret store plugin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/akeyless_test.py` around lines 89 - 91, The comment header contains an en-dash in the string "akeyless_backend – secret store plugin"; replace the en-dash (–) with a standard hyphen (-) so it reads "akeyless_backend - secret store plugin" to keep consistent ASCII punctuation in the comment.src/awx_plugins/credentials/akeyless.py (1)
267-280: Consider defensive handling for malformed JSON structures.The
_t.caston line 274 assumes the parsed JSON isdict[str, str]. If the secret contains a nested object or non-string value for the requested key, this could surface as a confusing runtime error downstream.The current implementation is acceptable given that Akeyless secret formats are well-defined, but consider whether additional validation would improve error messages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/awx_plugins/credentials/akeyless.py` around lines 267 - 280, The function _extract_structured_secret assumes secret_data JSON decodes to dict[str,str]; instead, after parsing secret_data into secret_dict, validate that it's a mapping and that secret_key exists and maps to a string (or coerce to string), and raise clear ValueError messages referencing secret_key and secret_path when the JSON is not an object or the value is missing/invalid; update uses of secret_dict, secret_key, and secret_path in the error messages so malformed JSON (e.g., nested objects or non-string values) produces a descriptive error rather than a confusing downstream exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/awx_plugins/credentials/akeyless.py`:
- Around line 267-280: The function _extract_structured_secret assumes
secret_data JSON decodes to dict[str,str]; instead, after parsing secret_data
into secret_dict, validate that it's a mapping and that secret_key exists and
maps to a string (or coerce to string), and raise clear ValueError messages
referencing secret_key and secret_path when the JSON is not an object or the
value is missing/invalid; update uses of secret_dict, secret_key, and
secret_path in the error messages so malformed JSON (e.g., nested objects or
non-string values) produces a descriptive error rather than a confusing
downstream exception.
In `@tests/akeyless_test.py`:
- Around line 205-207: The comment header line containing "akeyless_ssh_backend
– SSH certificate plugin" uses an en-dash; replace the en-dash (–) with a
standard hyphen (-) so the line reads "akeyless_ssh_backend - SSH certificate
plugin" to keep ASCII characters consistent in tests/akeyless_test.py.
- Around line 89-91: The comment header contains an en-dash in the string
"akeyless_backend – secret store plugin"; replace the en-dash (–) with a
standard hyphen (-) so it reads "akeyless_backend - secret store plugin" to keep
consistent ASCII punctuation in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ade9d49a-bf1d-453c-9098-e5a832469aa2
📒 Files selected for processing (3)
.mypy.inisrc/awx_plugins/credentials/akeyless.pytests/akeyless_test.py
|
Well, the maintenance resources (human-wise) are quite limited here. And so the advantages can easily end up being downsides. As for the infra — it can be copied to whatever place necessary 🤷♂️ |
| ssh_cert_data: str | None = 'ssh-rsa-SIGNED-CERT', | ||
| ) -> object: | ||
| """Create a mock Akeyless API instance with configurable responses.""" | ||
| mock_static_info = mocker.MagicMock() |
There was a problem hiding this comment.
Does it have to be magic or would a regular mock be enough?
There was a problem hiding this comment.
I don't see how this is connected.
There was a problem hiding this comment.
Also, don't forget about the specs.
|
@kgal-akl I think I've posted all the feedback I could. But the ultimate decision on this belonging here would be by the respective team. I'm just helping out with the repo infra occasionally. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/akeyless_test.py (1)
192-199: Consider adding a test forApiExceptionduring authentication.The current test covers
token=None(auth success but no token), but there's no test forApiExceptionraised byapi_instance.auth(). This would help verify the behavior flagged in the implementation review.Suggested test case
def test_backend_auth_api_exc_raises( patch_setup_client: _MockApiFactory, ) -> None: """An ApiException from auth should surface as RuntimeError.""" mock_api = patch_setup_client() mock_api.auth.side_effect = ApiException( status=401, reason='Unauthorized', ) with pytest.raises( RuntimeError, match=r'Akeyless API error: Unauthorized \(Status: 401\)', ): akeyless_mod.akeyless_backend(**_backend_kwargs())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/akeyless_test.py` around lines 192 - 199, Add a new test that verifies an ApiException raised by the API auth call surfaces as a RuntimeError from akeyless_mod.akeyless_backend: use the existing fixture patch_setup_client to get mock_api, set mock_api.auth.side_effect = ApiException(status=401, reason='Unauthorized'), and assert that calling akeyless_mod.akeyless_backend(**_backend_kwargs()) raises RuntimeError with a message matching "Akeyless API error: Unauthorized (Status: 401)"; place the test alongside test_backend_auth_failure_raises and name it test_backend_auth_api_exc_raises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/awx_plugins/credentials/akeyless.py`:
- Around line 356-363: The try/except around calling _authenticate only catches
ValueError so ApiException from api_instance.auth() escapes; update the except
to catch both ApiException and ValueError, for example: except (ApiException,
ValueError) as exc: raise RuntimeError(str(exc)) from exc, and apply the same
change in the akeyless_ssh_backend authentication block to match how
_fetch_secret_value and _fetch_ssh_certificate wrap ApiException into
RuntimeError.
---
Nitpick comments:
In `@tests/akeyless_test.py`:
- Around line 192-199: Add a new test that verifies an ApiException raised by
the API auth call surfaces as a RuntimeError from akeyless_mod.akeyless_backend:
use the existing fixture patch_setup_client to get mock_api, set
mock_api.auth.side_effect = ApiException(status=401, reason='Unauthorized'), and
assert that calling akeyless_mod.akeyless_backend(**_backend_kwargs()) raises
RuntimeError with a message matching "Akeyless API error: Unauthorized (Status:
401)"; place the test alongside test_backend_auth_failure_raises and name it
test_backend_auth_api_exc_raises.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8dbeb7d-a989-4f6f-a2aa-9f14592d21ed
📒 Files selected for processing (8)
.flake8.mypy.ini.pre-commit-config.yaml.pylintrc.toml.ruff.tomlpytest.inisrc/awx_plugins/credentials/akeyless.pytests/akeyless_test.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/awx_plugins/credentials/akeyless.py (2)
430-437:⚠️ Potential issue | 🟠 MajorSame issue:
ApiExceptionfrom authentication escapes unwrapped.Apply the same fix as suggested for
akeyless_backendto maintain consistent error handling.Proposed fix
try: token = _authenticate( api_instance, kwargs['access_id'], kwargs['access_key'], ) - except ValueError as val_err: + except _ApiException as api_exc: + raise RuntimeError( + f'Akeyless API error: {api_exc.reason}' + f' (Status: {api_exc.status})', + ) from api_exc + except ValueError as val_err: raise RuntimeError(str(val_err)) from val_err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/awx_plugins/credentials/akeyless.py` around lines 430 - 437, The authentication call to _authenticate in the akeyless plugin can raise ApiException which is currently not wrapped; add an except ApiException as api_err handler (in addition to the existing ValueError handler) around the token = _authenticate(...) call and re-raise a RuntimeError from api_err (e.g., raise RuntimeError(str(api_err)) from api_err) to mirror the akeyless_backend error handling and ensure consistent wrapped errors.
369-376:⚠️ Potential issue | 🟠 Major
ApiExceptionfromapi_instance.auth()escapes unwrapped.The try/except block catches
ValueErrorfrom_authenticate, butapi_instance.auth()(line 244) can also raiseApiException. This creates inconsistent error handling: API errors during secret retrieval are wrapped inRuntimeError, but identical API errors during authentication escape as-is.Proposed fix: catch both exception types from authentication
try: token = _authenticate( api_instance, kwargs['access_id'], kwargs['access_key'], ) - except ValueError as val_err: + except _ApiException as api_exc: + raise RuntimeError( + f'Akeyless API error: {api_exc.reason}' + f' (Status: {api_exc.status})', + ) from api_exc + except ValueError as val_err: raise RuntimeError(str(val_err)) from val_err🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/awx_plugins/credentials/akeyless.py` around lines 369 - 376, The authentication try/except around _authenticate currently only catches ValueError so ApiException thrown by api_instance.auth() (called inside _authenticate) escapes; update the except to catch both ValueError and ApiException and re-raise them as a RuntimeError (e.g., except (ValueError, ApiException) as err: raise RuntimeError(str(err)) from err) so authentication failures are handled consistently with secret retrieval; locate the logic around _authenticate(...) and api_instance.auth() and apply the dual-exception catch using the ApiException symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/awx_plugins/credentials/akeyless.py`:
- Around line 430-437: The authentication call to _authenticate in the akeyless
plugin can raise ApiException which is currently not wrapped; add an except
ApiException as api_err handler (in addition to the existing ValueError handler)
around the token = _authenticate(...) call and re-raise a RuntimeError from
api_err (e.g., raise RuntimeError(str(api_err)) from api_err) to mirror the
akeyless_backend error handling and ensure consistent wrapped errors.
- Around line 369-376: The authentication try/except around _authenticate
currently only catches ValueError so ApiException thrown by api_instance.auth()
(called inside _authenticate) escapes; update the except to catch both
ValueError and ApiException and re-raise them as a RuntimeError (e.g., except
(ValueError, ApiException) as err: raise RuntimeError(str(err)) from err) so
authentication failures are handled consistently with secret retrieval; locate
the logic around _authenticate(...) and api_instance.auth() and apply the
dual-exception catch using the ApiException symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 308cebc2-e120-4fff-a205-2a2fd9498764
📒 Files selected for processing (4)
docs/conf.pydocs/spelling_wordlist.txtsrc/awx_plugins/credentials/akeyless.pytests/akeyless_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/akeyless_test.py
| disallow_any_explicit = false | ||
|
|
||
| [mypy-tests.akeyless_test] | ||
| # Mock API objects are typed as `object`; test kwargs use generic dict |
There was a problem hiding this comment.
I'd rather have these ignores inline
| # directories to sys.path, which would cause false "module imports itself" | ||
| # warnings and circular-import errors for files with the same name as their | ||
| # third-party dependencies (e.g. akeyless.py vs the akeyless SDK): | ||
| source-roots = ["src"] |
There was a problem hiding this comment.
This is already achieved by the init-hook on line 64. I've been meaning to migrate to source-roots but didn't get to it. It's literally among the experiments in my Git checkout right now:
diff --git a/.pylintrc.toml b/.pylintrc.toml
index 3ffdc0e837..39b177a8d8 100644
--- a/.pylintrc.toml
+++ b/.pylintrc.toml
@@ -61,18 +61,19 @@ ignore-patterns = ["^\\.#"]
# This patch injects the project directory into the import path so that the
# local `pytest` plugin can be imported when `pylint-pytest` invokes it when
# exploring the fixtures available:
-init-hook = """
-import os, pathlib, sys
-repo_root_path = pathlib.Path.cwd()
-src_path = repo_root_path / 'src'
-sys.path[:0] = [str(src_path if src_path.exists() else repo_root_path)]
-os.environ['PYTHONPATH'] = sys.path[0]
-"""
+# init-hook = """
+# import os, pathlib, sys
+# repo_root_path = pathlib.Path.cwd()
+# src_path = repo_root_path / 'src'
+# sys.path[:0] = [str(src_path if src_path.exists() else repo_root_path)]
+# os.environ['PYTHONPATH'] = sys.path[0]
+# """
# Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the
# number of processors available to use, and will cap the count on Windows to
# avoid hangs.
-jobs = 0
+# jobs = 0
+jobs = 1
# Control the amount of potential inferred values when inferring a single object.
# This can help the performance when dealing with large functions or complex,
@@ -107,6 +108,9 @@ py-version = "3.11"
# directory used to determine a package namespace for modules located under the
# source root.
# source-roots =
+source-roots = [
+ "src/",
+]
# Allow loading of arbitrary C extensions. Extensions are imported into the
# active Python interpreter and may run arbitrary code.
@@ -435,7 +439,7 @@ disable = [
"disallowed-name",
"duplicate-code",
"fixme",
- "import-outside-toplevel",
+ # "import-outside-toplevel",
"invalid-name",
"line-too-long",
"missing-class-docstring",
@@ -444,11 +448,11 @@ disable = [
"missing-timeout",
"no-else-return",
"no-member",
- "no-name-in-module", # false-positive: https://github.com/pylint-dev/pylint/issues/10147#issuecomment-3946199493
+ # "no-name-in-module", # false-positive: https://github.com/pylint-dev/pylint/issues/10147#issuecomment-3946199493
"no-self-use",
"pointless-string-statement",
"raise-missing-from",
- "relative-beyond-top-level",
+ # "relative-beyond-top-level",
"singleton-comparison",
"too-few-public-methods",
"too-many-branches",But regardless, this seems like something that doesn't belong in this PR but could be submitted separately as a linting config / infra improvement rather than related to a plugin being presented. Would you like to send another PR with just this migration? (the init-hook-to-source-roots parts of the snippet)
| [mypy-akeyless] | ||
| # The akeyless SDK does not ship type stubs: | ||
| ignore_missing_imports = true | ||
|
|
||
| [mypy-akeyless.*] | ||
| # The akeyless SDK does not ship type stubs: | ||
| ignore_missing_imports = true | ||
|
|
There was a problem hiding this comment.
The preferred solution is adding own stubs into the Git tree like https://github.com/ansible/awx_plugins.interfaces/tree/1cec8f60cbb84806ce5cb08c27c301dd86dc866a/_type_stubs.
MyPy's already configured to pick them up from the same dir: https://github.com/ansible/awx-plugins/blob/c464cf276b9344056d7f47090bf361c2a8d85b45/.mypy.ini#L36C69-L36C104.
| disallow_any_expr = false | ||
|
|
||
| [mypy-awx_plugins.credentials.akeyless] | ||
| # The akeyless SDK does not ship type stubs. These suppressions are needed |
|
|
||
| # The following ignores have been researched and should be considered permanent: | ||
| # WPS202: two-plugin module (secret store + SSH) with Protocol types, TypedDicts, | ||
| # and multiple helpers per plugin; restructuring would harm readability. |
There was a problem hiding this comment.
Not necessarily. There's no reason why a single module can't be turned into a folder with some separation of common helpers and entry points, and maybe types.
| [pytest] | ||
| # Add src/ to sys.path so tests can import the package even in environments | ||
| # where `pip install -e .` has not been run (e.g. the pre-commit pylint venv): | ||
| pythonpath = src |
There was a problem hiding this comment.
This is definitely a wrong infra change. Things aren't supposed to be imported from the tree, only from installed envs. Additionally, pytest isn't supposed to be run directly.
|
|
||
| [mypy-tests.akeyless_test] | ||
| # Mock API objects are typed as `object`; test kwargs use generic dict | ||
| # rather than the specific TypedDicts the plugin functions expect: |
There was a problem hiding this comment.
Can't they be casted to whatever type necessary? You've done this in the runtime module already. No need to have multiple approaches for the same thing..
| ) | ||
|
|
||
| # The akeyless SDK does not ship type stubs; pylint cannot resolve its imports. | ||
| # When not installed, pylint misidentifies `from akeyless import` as a |
There was a problem hiding this comment.
So it should be installed, just like all the other deps, not need to have an exception for this one.
| mock_api.describe_item.return_value = mock_describe | ||
| mock_api.get_secret_value.return_value = {secret_path: secret_data} | ||
| mock_api.get_ssh_certificate.return_value.data = ssh_cert_data | ||
| return mock_api |
There was a problem hiding this comment.
By the way, wouldn't it be easier to go with stubs instead of mocks? It seems like half of these would be just fine with a dumb dataclass-based implementation instead of mocks (let alone magic ones).
2 participants
PR created as part of ansible/awx#16151 (comment).
Summary by CodeRabbit
New Features
Tests
Chores
Style
Documentation