Skip to content

feat(parameters): pluggable secret/param storage with 4 providers#190

Open
fedemaleh wants to merge 44 commits into
betafrom
feature/parameters-package
Open

feat(parameters): pluggable secret/param storage with 4 providers#190
fedemaleh wants to merge 44 commits into
betafrom
feature/parameters-package

Conversation

@fedemaleh

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread parameters/docs/adding_a_provider.md Outdated
mkdir -p parameters/tests/providers/<provider_name>
```

`<provider_name>` is `snake_case` and is what users will set in `SECRET_PROVIDER` / `PARAMETER_PROVIDER`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto en realidad tiene coincidir con el slug del provider de tipo parameters-storage.

Comment thread parameters/docs/adding_a_provider.md
Comment thread parameters/docs/adding_a_provider.md Outdated

---

## Step 4: Write `fetch_configuration` (optional)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Eso creo que dijimos de sacarlo

Comment thread parameters/docs/adding_a_provider.md Outdated
Comment on lines +202 to +205
1. Set the env var: `SECRET_PROVIDER=<provider_name>` and/or `PARAMETER_PROVIDER=<provider_name>`.
2. If using `fetch_configuration`, the platform team needs to ensure the fetch mechanism (np CLI, REST endpoint, etc.) returns the JSON shape your provider expects.

Done. The new provider is reachable from every workflow without any other change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto no va más, vamos a sacar esta información del campo provider que viene en el payload.

Comment thread parameters/docs/architecture.md Outdated

A monolithic scope tied to one backend forces fork-and-modify for every variation. This package inverts the relationship: the **dispatch layer is the package**, the **backends are pluggable modules** dropped into `providers/`.

The platform decides which provider handles each parameter — there is no per-environment / per-agent configuration of "which provider to use". The notification payload carries that information directly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto no es cierto. La plataforma decide pero hay configuraciones per-environment, hay que configurarlas vía el provider de tipo parameters-storage

Comment thread parameters/docs/architecture.md Outdated

The provider's configuration travels in the same payload at `provider.attributes`. `build_context` exports it as `PROVIDER_CONFIG` (a JSON string). Each provider's `setup` reads from `PROVIDER_CONFIG` via `get_config_value --provider '.field'` to extract specific fields (region, kms_key_id, etc.).

This means **there is no per-environment configuration of "which provider"** — the platform decides per-parameter. A single agent can serve parameters routed to Vault and secrets routed to Secrets Manager at the same time, without any agent-side configuration.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Lo mismo que antes, se configura vía una configuración.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Lo de que un mismo agente puede servir múltiples storage es correcto.

Comment thread parameters/docs/configuration.md Outdated
| **`provider.specification_id`** | **UUID identifying which provider handles this parameter** |
| **`provider.attributes`** | **Provider-specific configuration (region, vault address, etc.)** |
| `provider.nrn` | Provider-instance NRN (informational) |
| `provider.dimensions` | Provider-instance dimensions (informational, different from parameter dimensions) |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Dejemos muy claro que esto no hay que usarlo

Comment thread parameters/docs/configuration.md Outdated
`build_context` calls:

```bash
np provider specification read --id <provider.specification_id> --output json

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

--format json

Comment thread parameters/docs/configuration.md Outdated
Comment on lines +126 to +129
Two things that used to be design points but are obsolete now:

- **`SECRET_PROVIDER` / `PARAMETER_PROVIDER` env vars** — not needed. The platform sends `specification_id` per parameter, so there's no global "which provider to use" setting.
- **`fetch_configuration` scripts per provider** — not needed. Config comes in the payload as `provider.attributes`, no separate fetching step.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Borrar esto, esta es la primera implementación, no tiene sentido documentar decisiones intermedias que nunca vieron la luz.


| Kind | Storage location | This package |
|-------------|---------------------------------------------------|--------------|
| Clear-text | nullplatform API (its own datastore) | Not involved |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

nullplatform API no es clear-text. Creo que los kind son: nullplatform-storage, third-party-storage

| Clear-text | nullplatform API (its own datastore) | Not involved |
| Secret | External provider (this package: AWS SM) | Used |

Only **secret** parameters trigger the `store` / `retrieve` / `delete` workflows. Clear-text values never leave nullplatform and never touch AWS SM. From the operator's perspective: this provider is invisible until a parameter is marked as a secret.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto es mentira, se puede configurar secret manager para parámetros no secretos, es costoso pero es algo que se puede hacer.

Comment on lines +37 to +42
```
parameters/<external_id>
```

- **`parameters/`** — fixed prefix. This is the IAM anchor: it lets a single resource ARN pattern (`arn:...:secret:parameters/*`) cover everything this provider manages, and nothing else. Removing the prefix would force IAM to either allow account-wide access or maintain an enumerated list of secret names (impractical, since names are generated at runtime).
- **`<external_id>`** — UUIDv4 generated by the `store` script via `uuidgen` (with `openssl rand -hex 16` as a portable fallback). This becomes the canonical handle nullplatform persists; subsequent `retrieve` / `delete` actions get it back via `NP_ACTION_CONTEXT.notification.external_id`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto no sigue lo que hablamos, el path debería incluir los slugs / ids de entidades y el nombre del param además del uuid

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

También documentar en la docu global, los paths donde se guardan las cosas deben seguir el principio rector que si alguien entra al storage layer manualmente (secret manager, vault, etc.) debe poder encontrar lo que busca, siempre tiene que ser human friendly.

"parameter_id": 42,
"value": "the-actual-secret",
"stored_at": "2026-05-05T12:34:56Z",
"external_id": "f47ac10b-58cc-4372-a567-0e02b2c3d479"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agregaría un "managed_by":"nullplatform"

1. **Don't store non-secrets here.** Clear-text parameters belong in nullplatform's API. Mis-classification is the most common cost regression.
2. **Delete promptly.** `--force-delete-without-recovery` (already used) ends billing immediately. Without it, AWS keeps charging for the 7–30 day soft-delete window.
3. **Avoid replication unless you need DR.** Each replica is a full $0.40/month.
4. **Cache at the consumer side.** Each `GetSecretValue` is a billable call. For high-fanout reads (e.g., autoscaling pods all pulling the same value), have a single retrieve at deploy-time and inject as env var, rather than per-pod API calls.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto quitar, es responsabilidad de la plataforma optimizar esto, no del user.

Comment on lines +120 to +126
### Comparison with self-hosted Vault

This isn't apples-to-apples:

- **Vault**: no per-secret fee. Cost is the underlying infra (EC2/EKS, storage, ops time). Below ~250 secrets, self-hosted Vault tends to be cheaper on paper but more expensive in operator hours.
- **AWS SM**: linear in secret count, zero ops. Above ~250 secrets the costs converge; the trade-off becomes operational rather than financial.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No me interesa esta comparación. Sacar.


### Idempotency

- `delete` is idempotent: it suppresses errors with `|| true` and always returns `{success: true}`. Re-deleting a missing secret is a no-op.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto es una mala decisión, sólo tiene que hacer supress del error de not found, los otros errores (ej: falta de permiso de IAM deberían reportarse y marcarse como error)


### ARN shape

When you `CreateSecret --name parameters/<uuid>`, AWS appends a random 6-character suffix to the ARN:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Más que parameters/ creo que el path debería ser nullplatform/

Comment on lines +7 to +16
## Wildcards: which ones are OK

There are two distinct uses of `*` in IAM, frequently conflated:

| Pattern | Meaning | Allowed here? |
|------------------------------------------------------|--------------------------------------|---------------|
| `"Resource": "*"` | All resources of all types in the account | **No** |
| `"Resource": "arn:...:secret:parameters/*"` | Path glob on the `parameters/` prefix | **Yes** |

The second is not a privilege escalation — it is the only way to express "all secrets owned by this provider" given that secret names are UUIDs generated at runtime and cannot be enumerated in advance. Avoiding it would force either explicit per-secret policies (impossible for unknown UUIDs) or `Resource: "*"` (much wider).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sólo documentemos la necesidad que es acceder a secrets cuyo inicio es nullplatform/*


---

## Splitting agent vs consumer

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sacar esta sección, es irrelevante para esta implemetnación


For reference — these are commonly requested but **not** needed by the current scripts and should be denied unless a specific use case is documented:

- `secretsmanager:PutSecretValue` — would let the agent overwrite values. Not used; secrets are immutable in this design.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Hay algo que creo que se está confundiendo.

El modelo de datos es así:

Hay un parameter id (ej: name=DB_PASSWORD, type=secret, id=1) que tiene múltiples values (ej: env:prod value=abcd).

Los values son inmutables, cada vez que se modifica para una misma combinación de dimensiones / scope nrn se genera una nueva versión. El modelo de datos debería guardar dentro del mismo secret todo el versionado, dado que tenemos que soportar ver y restaurar versiones viejas.

Comment thread parameters/providers/aws-secrets-manager/docs/architecture.md
Comment thread parameters/docs/adding_a_provider.md Outdated

### `retrieve`

Read the value, return `{value}` or `{value: "value not found"}` on miss.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Por que no tiras un error si value not found?

Comment thread parameters/docs/adding_a_provider.md Outdated

### `delete`

Always returns `{success: true}`. Suppress errors with `|| true`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto esta mal, sólo supress not found errors, el resto de los errores hay que reportarlos y fallar.

El not found error lo suprimimos para ser indepotentes y porque ya está hecho el trabajo, pero no podemos dar un resultado exitoso si el trabajo no se hizo

Comment thread parameters/docs/adding_a_provider.md
Comment thread parameters/docs/architecture.md Outdated
`build_context` resolves this UUID into a slug using the np CLI:

```
np provider specification read --id <specification_id> --output json

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

En np cli no existe un flag --output es --format json.

Comment thread parameters/docs/architecture.md Outdated
parameters/
├── entrypoint # Action router (action → workflow)
├── build_context # Resolves ACTIVE_PROVIDER from spec_id, sources setup
├── store, retrieve, # Dispatch one-liners

@fedemaleh fedemaleh Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Podríamos tener un script dispatch que reciba el nombre del action y sepa hacer el dispatch. Me imagino algo así:

#!/bin/bash

set -euo pipefail

# Dispatch: delegate to the active provider's retrieve implementation.
# PROVIDER_DIR is exported by build_context.
source "$PROVIDER_DIR/$ACTION"

Y en los workflows:

- name: store
   type: script
   file: "$SERVICE_PATH/parameters/dispatch"
   configuration:
      ACTION: store

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Pondría todos los scripts compartidos en utils así no quedan sueltos.


For a parameter with:

- Entities: `organization=1255165411`, `account=95118862`, `namespace=37094320`, `application=321402625`

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Falta aclarar que las dimensiones pueden no existir y que opcionalmente también puede haber una entidad scope.

Aclarar esto en el doc global y en cada provider.

| Error condition | `store` | `retrieve` | `delete` |
|----------------------------------------|-------------------|--------------------------|--------------------|
| Resource exists (on store) | New version added | N/A | N/A |
| ResourceNotFoundException | N/A | `{value: "value not found"}` | Idempotent success |

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

En retrieve hay que fallar, no mandemos el value not found te lo aclare en el general y te pedi que lleves esa decision a todos los providers.

"region"
],
"properties": {
"region": {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Quitar este. Eso viene del role que use el agente con sus credenciales.

"title": "AWS Region",
"description": "AWS region where secrets will be stored (e.g. us-east-1)"
},
"name_prefix": {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Esto tampoco va.

fedemaleh added 29 commits July 3, 2026 13:11
…xt.notification_parse + assume_role_step.resolve_arn markers
…ials, drop NP_STS_CACHE_DIR/DISABLE env vars
…, action-aware entity fetch (single-wave prefetch)
@fedemaleh
fedemaleh force-pushed the feature/parameters-package branch from 02d8803 to 6673e0f Compare July 3, 2026 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant