CH-284 secrets managers base support implementation - #858
Open
filippomc wants to merge 4 commits into
Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
|
|
||
| from __future__ import annotations | ||
| import pprint |
|
|
||
| from __future__ import annotations | ||
| import pprint | ||
| import re # noqa: F401 |
| from __future__ import annotations | ||
| import pprint | ||
| import re # noqa: F401 | ||
| import json |
|
|
||
|
|
||
| from cloudharness_model.base_model import CloudHarnessBaseModel | ||
| from pydantic import BaseModel, Field, field_validator, StrictStr, StrictBool, StrictInt, StrictFloat |
|
|
||
| from cloudharness_model.base_model import CloudHarnessBaseModel | ||
| from pydantic import BaseModel, Field, field_validator, StrictStr, StrictBool, StrictInt, StrictFloat | ||
| from typing import ClassVar, List, Dict, Any, Union, Optional, Annotated |
| from cloudharness_model.base_model import CloudHarnessBaseModel | ||
| from pydantic import BaseModel, Field, field_validator, StrictStr, StrictBool, StrictInt, StrictFloat | ||
| from typing import ClassVar, List, Dict, Any, Union, Optional, Annotated | ||
| import importlib |
|
|
||
|
|
||
| from __future__ import annotations | ||
| from inspect import getfullargspec |
| from inspect import getfullargspec | ||
| import json | ||
| import pprint | ||
| import re # noqa: F401 |
| import json | ||
| import pprint | ||
| import re # noqa: F401 | ||
| from pydantic import BaseModel, ConfigDict, Field, StrictStr, ValidationError, field_validator |
| from pydantic import BaseModel, ConfigDict, Field, StrictStr, ValidationError, field_validator | ||
| from typing import Optional | ||
| from cloudharness_model.models.secret_config import SecretConfig | ||
| from typing import Union, Any, List, Set, TYPE_CHECKING, Optional, Dict |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a “rich” harness.secrets definition format that can delegate secrets to named secret managers (e.g. onepassword, aws), while preserving the legacy plain-string behavior as the default cloudharness manager. It updates Helm/Compose templating, Codefresh export behavior, the OpenAPI model/schema, and adds test coverage and documentation for the new semantics.
Changes:
- Introduces a secret-manager framework in Helm templates (resource rendering + projected secret volume wiring) and a Compose fallback that resolves to local defaults.
- Adds Python helpers + validation for secret definitions, and updates Codefresh pipeline generation to only export CloudHarness-managed secrets (with correct handling for rich-form defaults).
- Extends the OpenAPI schema/models/docs to represent secrets as
SecretDefinition(plain value orSecretConfig) and documents usage.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/deployment-cli-tools/tests/test_helm.py | Adds Helm rendering tests for legacy secrets, rich-form defaults, unmanaged secrets, and built-in managers (1Password/AWS). |
| tools/deployment-cli-tools/tests/test_dockercompose.py | Adds a Compose test asserting rich-form secrets fall back to default locally. |
| tools/deployment-cli-tools/tests/test_codefresh.py | Adds tests ensuring only CloudHarness-managed secrets are exported to Codefresh variables and helpers work with parsed models. |
| tools/deployment-cli-tools/tests/resources/applications/myapp/deploy/values-secrets.yaml | Adds a fixture values file exercising rich secrets + unmanaged secret syntax. |
| tools/deployment-cli-tools/ch_cli_tools/templates/python/model.mustache | Avoids duplicated import headers for generated union models by moving imports out of this template. |
| tools/deployment-cli-tools/ch_cli_tools/templates/python/model_generic.mustache | Centralizes common Python model imports into the generic template. |
| tools/deployment-cli-tools/ch_cli_tools/secrets.py | Introduces Python helpers to inspect/normalize secret definitions (manager selection, default value extraction, validation helper). |
| tools/deployment-cli-tools/ch_cli_tools/configurationgenerator.py | Adds validate_secrets() to reject malformed secret definitions during values validation. |
| tools/deployment-cli-tools/ch_cli_tools/codefresh.py | Updates Codefresh variable export to skip externally managed secrets and to target _default for rich-form secrets. |
| libraries/models/README.md | Registers the new SecretConfig / SecretDefinition models in the model index. |
| libraries/models/docs/SecretDefinition.md | Adds generated model docs for SecretDefinition. |
| libraries/models/docs/SecretConfig.md | Adds generated model docs for SecretConfig. |
| libraries/models/docs/ApplicationHarnessConfig.md | Updates secrets field documentation to reference SecretDefinition. |
| libraries/models/cloudharness_model/models/secret_definition.py | Adds generated union model representing a secret value or config object. |
| libraries/models/cloudharness_model/models/secret_config.py | Adds generated model for rich secret config (manager, default, plus additional properties). |
| libraries/models/cloudharness_model/models/application_harness_config.py | Types secrets as Dict[str, SecretDefinition] and updates (de)serialization accordingly. |
| libraries/models/cloudharness_model/models/init.py | Exports SecretConfig and SecretDefinition from the models package. |
| libraries/models/cloudharness_model/base_model.py | Adds from_json/to_json helpers to support union-model parsing/serialization. |
| libraries/models/api/openapi.yaml | Adds SecretsMap, SecretConfig, SecretDefinition schemas and wires harness.secrets to the new type. |
| docs/model/SecretDefinition.md | Adds generated public docs copy for SecretDefinition. |
| docs/model/SecretConfig.md | Adds generated public docs copy for SecretConfig. |
| docs/model/ApplicationHarnessConfig.md | Updates public docs for secrets type to use SecretDefinition. |
| docs/applications/secrets.md | Documents secret managers, unmanaged secrets behavior, built-in managers, and Codefresh export rules for rich secrets. |
| deployment-configuration/value-template.yaml | Updates comments to describe rich-form secret manager delegation and unmanaged secrets. |
| deployment-configuration/helm/values.yaml | Adds top-level secretmanagers config section for deployment-wide manager settings. |
| deployment-configuration/helm/templates/auto-secrets.yaml | Stops writing externally managed secrets into the app Secret and renders external manager resources. |
| deployment-configuration/helm/templates/auto-deployments.yaml | Switches secret volume rendering to a unified helper that supports projected volumes across managers. |
| deployment-configuration/helm/templates/_secrets.tpl | Introduces Helm secret-manager framework, built-in onepassword/aws managers, and projected-volume rendering. |
| deployment-configuration/compose/templates/auto-secrets.yaml | Resolves secrets via secretValue (rich-form default) locally where managers aren’t available. |
| deployment-configuration/compose/templates/_secrets.yaml | Adds Compose equivalents of secretManager/secretValue helpers to keep semantics aligned with Helm. |
| applications/accounts/deploy/templates/_identity_providers.tpl | Switches identity provider templates to use secretValue so rich-form secrets work transparently. |
Comment on lines
+97
to
+98
| if definition is None or isinstance(definition, (str, int, float, bool)): | ||
| return None |
Comment on lines
+501
to
+510
| SecretDefinition: | ||
| description: >- | ||
| An application secret, defined either as a plain value or as a secret | ||
| configuration object. A `string` (or null) is the secret value itself: empty or | ||
| null generates a random value, `?` generates a new random value at every | ||
| deployment. A `SecretConfig` object instead delegates the secret to a secret | ||
| manager, and carries the settings that manager needs. | ||
| anyOf: | ||
| - type: string | ||
| - $ref: '#/components/schemas/SecretConfig' |
| """ # noqa: E501 | ||
| manager: Optional[StrictStr] = Field(default=None, description="Name of the secret manager handling the secret. Defaults to `cloudharness`, which creates the value in the application secret. Set explicitly to null to leave the secret unmanaged: nothing is created and the secret is assumed to exist already.") | ||
| default: Optional[StrictStr] = Field(default=None, description="Value used by the `cloudharness` manager and as a fallback when the secret manager is not available, as in local docker compose deployments. Follows the same conventions as a plain secret value: null or empty generates a random value once, `?` generates a new random value at every deployment.") | ||
| additional_properties: Dict[str, Any] = {} |
…ure/CH-284-secrets-managers
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tools/deployment-cli-tools/ch_cli_tools/secrets.py:100
secret_definition_error()currently treatsint,float, andboolsecret values as valid, but the OpenAPI schema forSecretDefinitionisanyOf: [string, SecretConfig](plus null via YAML), so non-string scalars will later fail model parsing with a less actionable error. Tighten validation here to accept onlystr/Nonefor plain secrets andstr/Nonefordefaultin the rich form.
definition = _plain(definition)
if definition is None or isinstance(definition, (str, int, float, bool)):
return None
if not isinstance(definition, dict):
return f"expected a secret value or a secret configuration, got {type(definition).__name__}"
deployment-configuration/helm/templates/_secrets.tpl:167
- Map iteration order in Helm is not guaranteed, but
deploy_utils.secretsVolumebuilds the projectedsourceslist by ranging directly over$app.harness.secrets. This can lead to non-deterministic manifest output and flaky tests (the unit tests in this PR assert a specificsourcesorder). Consider iterating oversortAlpha (keys ...)and indexing the map to produce a stable, predictable ordering.
{{- $external := list -}}
{{- range $name, $spec := $app.harness.secrets -}}
{{- if include "deploy_utils.secretIsExternal" (dict "spec" $spec) -}}
{{- $manager := include "deploy_utils.secretManager" (dict "spec" $spec) -}}
{{- $context := dict "root" $root "app" $app "name" $name "spec" $spec "resourceName" (include "deploy_utils.secretResourceName" (dict "app" $app "name" $name)) -}}
{{- $ref := splitList "/" (include (printf "deploy_utils.secretmanager.%s.ref" $manager) $context) -}}
{{- $external = append $external (dict "path" $name "secretName" (first $ref) "key" (last $ref)) -}}
{{- end -}}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 44 changed files in this pull request and generated no new comments.
Suppressed comments (5)
docs/applications/secrets/README.md:28
- Grammar: “Do that only if you aware…” should be “Do that only if you are aware…”. Also consider using “GitHub” consistently in docs.
* Set the secret's value (as in `unsecureSecret`). Do that only if you aware of what you are doing as the value may be pushed in the git(hub) repository.
tools/deployment-cli-tools/ch_cli_tools/secrets.py:100
secret_definition_error()currently treats int/float/bool as valid secret values. The OpenAPI schema forSecretDefinitiononly allows string (or null) or aSecretConfigobject, so numeric/boolean YAML values should be rejected to avoid silently accepting mis-typed secrets (e.g. unquoted numbers).
tools/deployment-cli-tools/ch_cli_tools/secrets.py:16- Docstring points at
deployment-configuration/helm/templates/_secrets.tpl, but the template added in this PR lives underdeployment-configuration/helm/templates/secrets/_secrets.tpl. The current path is misleading for future maintainers.
docs/applications/secrets/README.md:11 - Spelling/grammar issues in the intro (e.g. “build-in”, “kubernetes”, “it's”) reduce clarity in user-facing docs.
This issue also appears on line 28 of the same file.
**CloudHarness has build-in support for application specific kubernetes secrets.**
The CH secrets will be mounted as data volumes to be used by a container in a Pod and will be auto updated on change. This means that a pod doesn't need to be restarted to "see" the new value(s)
remark: an application has only access to it's "own" secrets
deployment-configuration/helm/templates/secrets/managers/onepassword.tpl:10
- The PR description says no specific managers were implemented, but this PR adds built-in manager implementations (e.g.
onepasswordandawstemplates + docs). This is a mismatch that can confuse reviewers/users relying on the PR text.
================================================================================
Secret manager: onepassword
================================================================================
Full setup guide: docs/applications/secrets/managers/onepassword.md
Reads secrets from 1Password through the 1Password Kubernetes Operator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes CH-284
Implemented solution
Added support an alternative rich specification like
The implementation detects the new syntax with managers and delegates the creation of the secret to the appropriate manager, if any exist with the given key.
No specific managers have been implemented as part of this changes, apart of the old logic reinterpreted as the default "cloudharness" manager.
How to test this PR
Define a secret as specified above. Secrets should be specified as in the simplified old setting by selecting the cloudharness manager.
Sanity checks:
Breaking changes (select one):
breaking-changeand the migration procedure is well described abovePossible deployment updates issues (select one):
alert:deploymentTest coverage (select one):
Documentation (select one):
Nice to have (if relevant):