feat(provider): add Kubernetes - #387
Conversation
| }), | ||
| Err(_) => runtime().block_on(future), | ||
| } | ||
| } |
There was a problem hiding this comment.
Just to be upfront, this snippet is essentially a copy/paste from vault_common.rs. I don't know if it should be extracted to some common function or if just mentioning in a comment that it's essentially a duplicate is fine. Initially I just used super::block_on but that caused panics when trying to set a secret value.
domenkozar
left a comment
There was a problem hiding this comment.
Six actionable inline findings on the Kubernetes provider implementation.
| resource_attributes: Some(ResourceAttributes { | ||
| namespace: Some(namespace.into()), | ||
| verb: Some("patch".to_string()), | ||
| resource: Some(self.config.kind.to_string()), |
There was a problem hiding this comment.
[P1] Use plural resource names in access reviews
Kubernetes RBAC checks use plural API resource names. Display yields secret or configmap here, so ordinary roles granting resources: ["secrets"] or resources: ["configmaps"] will not match this SelfSubjectAccessReview, and authorized writes will be rejected. Map each kind to its plural API resource name for this field.
| config: KubernetesConfig, | ||
| name: "kubernetes", | ||
| description: "Kubernetes", | ||
| schemes: ["k8s+configmap", "k8s+secret"], |
There was a problem hiding this comment.
[P1] Keep the bare name out of provider selection
The global provider picker persists info.name, so this registration offers kubernetes even though the registry only accepts the two configured URI schemes. Selecting it produces a provider value that later construction—and config global init --provider kubernetes itself—rejects. Please either exclude this provider from bare-name selection or make the flow collect a concrete Kubernetes URI.
| description: "Kubernetes", | ||
| schemes: ["k8s+configmap", "k8s+secret"], | ||
| examples: ["k8s+secret://db-config", "k8s+configmap://db-config@default"], | ||
| deletes: true, |
There was a problem hiding this comment.
[P2] Stop advertising deletion until it is implemented
This advertises static delete support, but KubernetesProvider implements neither delete nor supports_delete, so it inherits the unsupported-operation behavior. That admits cache invalidation and import --delete-source planning only for instance deletion to fail. Remove this flag until deletion is implemented, or implement the capability consistently.
| fn check_writable(&self, _addr: Address<'_>) -> Result<()> { | ||
| let can_i_patch = block_on(self.can_i_patch())?; |
There was a problem hiding this comment.
[P2] Resolve the address during write preflight
_addr is ignored, so check_writable can succeed for a native address containing an unsupported coordinate such as field; set then reaches resolve_coords and rejects it only after the CLI may have prompted for a value. Resolve the address here before the permission review so preflight rejects everything set will reject.
| } | ||
|
|
||
| fn format_secret_name(project: &str, profile: &str, key: &str) -> Result<String> { | ||
| let secret_name = format!("secretspec-{}-{}-{}", project, profile, key); |
There was a problem hiding this comment.
[P2] Encode convention components without collisions
Hyphen concatenation is not injective: (project="a-b", profile="c", key="KEY") and (project="a", profile="b-c", key="KEY") both become secretspec-a-b-c-KEY. Projects sharing the Kubernetes object can therefore read or overwrite one another. Use an injective component encoding or a reserved delimiter so the profile-aware path remains isolated.
| struct: KubernetesProvider, | ||
| config: KubernetesConfig, | ||
| name: "kubernetes", | ||
| description: "Kubernetes", |
There was a problem hiding this comment.
[P2] Add the release version to the CLI description
This description feeds the config init selector, so it currently renders kubernetes: Kubernetes without the release marker. Because this provider targets the unreleased 0.20 release, label the registration description Kubernetes (0.20+), matching the version required at every selector entry.
86566bf to
75d67f6
Compare
| **Prerequisites**: A Kubernetes configuration in `$KUBECONFIG` or | ||
| `$HOME/.kube/config`; build with `--features kubernetes` (0.20+) | ||
| **Authentication**: Configured in Kubernetes configuration | ||
| **Storage**: `secretspec-{project}-{profile}-{key}` key under `.data` in the |
There was a problem hiding this comment.
Kubernetes Secret and ConfigMap .data fields are flat key/value maps rather than folder hierarchies, and / is not valid in a data key. The implementation reserves -- to separate the SecretSpec coordinates, so this should match it: secretspec--{project}--{profile}--{key}.
Summary
Scenario coverage
Validation
cargo test --alldevenv shell -- prek run -anpm --prefix docs run buildFixes #316