Skip to content

feat: sensitive secret configs#219

Merged
yanksyoon merged 13 commits intomainfrom
feat/sensitive-secret-configs
Apr 21, 2026
Merged

feat: sensitive secret configs#219
yanksyoon merged 13 commits intomainfrom
feat/sensitive-secret-configs

Conversation

@yanksyoon
Copy link
Copy Markdown
Member

@yanksyoon yanksyoon commented Apr 17, 2026

Applicable spec:

Overview

  • Use Juju secrets for sensitive configuration values

Rationale

  • Juju secret is a secure way to manage sensitive values

Juju Events Changes

Module Changes

Library Changes

Checklist

No app changes

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates sensitive OpenStack password configuration from plaintext charm config to Juju secrets, updating runtime parsing and test scaffolding accordingly.

Changes:

  • Switch openstack-password config to type: secret and update documentation in charmcraft.yaml.
  • Update OpenStack clouds config parsing to read the password from a Juju secret via model.get_secret(...).get_content().
  • Update unit/integration test fixtures to create and grant the OpenStack password secret.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/state.py Fetch OpenStack password from a Juju secret and raise config errors on secret lookup/content failures.
charmcraft.yaml Change openstack-password config option to type: secret and document required secret content format.
docs/changelog.md Add changelog entry noting Juju secrets usage for sensitive config.
tests/unit/conftest.py Update harness fixture to create/grant a user secret and set config to the secret id.
tests/unit/factories.py Update mock charm config to use a secret id and mock model.get_secret() content.
tests/unit/test_state.py Add unit tests for secret lookup/access errors and missing secret content key.
tests/integration/conftest.py Add integration fixtures for OpenStack password secret, pass secret id via config, and grant secret to the app.
Comments suppressed due to low confidence (1)

tests/integration/conftest.py:436

  • The declared return type says the third element is a set, but charmhub_info[...].keys() returns a dict_keys view. Either convert it (e.g., set(...)) or adjust the return annotation to match what’s actually returned, otherwise mypy/type-checking can fail.
async def _prepare_charmhub_app_config(ops_test, app_config: dict) -> tuple[str, dict, set]:
    """Prepare the application config for charmhub deployment.

    Args:
        ops_test: The pytest operator test instance.
        app_config: The base application configuration.

    Returns:
        A tuple of (channel, prepared_config, config_options).

    """
    charmhub_channel = "edge"
    ret_code, stdout, stderr = await ops_test.juju(
        "info", "--format", "json", "--channel", charmhub_channel, "github-runner-image-builder"
    )
    assert ret_code == 0, f"Failed to get charm info: {stderr}"
    charmhub_info = json.loads(stdout.strip())
    charmhub_config_options = charmhub_info["charm"]["config"]["Options"].keys()


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/state.py Outdated
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
Comment thread tests/unit/factories.py Outdated
@yanksyoon yanksyoon marked this pull request as draft April 17, 2026 05:15
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces support for providing the OpenStack password via Juju secrets, reducing the need to store sensitive credentials in plain-text charm config while keeping legacy configuration supported for backward compatibility.

Changes:

  • Add a new openstack-password-secret charm config option (type secret) and document deprecation of openstack-password.
  • Update OpenStack clouds config parsing to prefer fetching the password from a Juju secret when configured.
  • Update unit and integration tests/factories to exercise secret-based and legacy password paths.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/state.py Adds new config constant and updates _parse_openstack_clouds_config to fetch password from a Juju secret when provided.
charmcraft.yaml Adds openstack-password-secret config option and marks openstack-password as deprecated in its description.
tests/unit/factories.py Updates mock charm factory defaults to use the secret-based password path and mocks model.get_secret().
tests/unit/test_state.py Adds unit coverage for secret-fetch error cases, missing password key in secret, legacy password fallback, and missing-password scenarios.
tests/integration/conftest.py Adds integration secret creation/granting for OpenStack password and wires secret-based config into app deployment fixtures.
docs/changelog.md Adds changelog entry describing the new secret-based password configuration and deprecation of the legacy option.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py Outdated
Comment thread src/state.py Outdated
Comment thread src/state.py
Comment thread charmcraft.yaml Outdated
Comment thread docs/changelog.md Outdated
yanksyoon and others added 2 commits April 17, 2026 06:28
- charmcraft.yaml: remove trailing backslash in YAML literal block for
  openstack-password deprecation notice to avoid literal backslash in
  rendered description
- docs/changelog.md: add PR link and date to changelog entry for
  consistency with other entries
- src/state.py: remove unnecessary nosec comment on
  OPENSTACK_PASSWORD_SECRET_CONFIG_NAME (not a password literal);
  add Juju version check and secret ID format validation before calling
  model.get_secret() in _parse_openstack_clouds_config so users get
  clear error messages (upgrade guidance or expected format)
- tests/integration/conftest.py: fix dict_keys vs set type annotation in
  _prepare_charmhub_app_config; add legacy openstack-password fallback
  when charmhub revision does not expose openstack-password-secret; fix
  grant order in app_on_charmhub_fixture (deploy without secret config,
  grant, then set_config to trigger config-changed after permissions are
  in place); import OPENSTACK_PASSWORD_CONFIG_NAME
- tests/unit/factories.py: remove misleading nosec marker from secret ID
  value (it is a reference, not a credential)
- tests/unit/test_state.py: add patch_juju_version_33 to tests using the
  secret path; add tests for unsupported Juju version and invalid secret
  ID format

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread src/state.py Outdated
Comment thread src/state.py
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
Comment thread tests/integration/conftest.py
Copy link
Copy Markdown
Member Author

@yanksyoon yanksyoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review

yanksyoon and others added 6 commits April 20, 2026 15:02
Bandit flags 'openstack-password-secret' as B105 (hardcoded_password_string)
because the string contains 'password'. Restore the nosec comment on
OPENSTACK_PASSWORD_SECRET_CONFIG_NAME in state.py.

Also restore the nosec comment in factories.py for the mock secret content
dict, which bandit flags as B105 in full-repo scan mode.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Collapse dict comprehension to single line to satisfy black formatting
- Collapse set_config call to single line to satisfy black formatting
- Add pylint disable for too-many-arguments and
  too-many-positional-arguments on app_on_charmhub_fixture which now
  takes 6 fixture parameters

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mypy rejects encoding= when handlers= is also passed to basicConfig,
since only the filename-based overload accepts encoding. The encoding
is already set on the WatchedFileHandler constructor, so removing it
from basicConfig is correct and harmless.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yanksyoon yanksyoon marked this pull request as ready for review April 20, 2026 18:08
@yanksyoon yanksyoon merged commit 61a61d6 into main Apr 21, 2026
51 of 59 checks passed
@yanksyoon yanksyoon deleted the feat/sensitive-secret-configs branch April 21, 2026 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants