[Robustesse] unwrap()/panics sur les chemins de reconcile (namespace, désérialisation JSON) #16

New Issue

xmortelette · 2026-06-12T09:29:51+02:00

xmortelette commented

2026-06-12 09:29:51 +02:00

Constat

Plusieurs unwrap() / expect() sont présents sur des chemins chauds du reconcile, où une donnée inattendue fait paniquer la task du contrôleur au lieu de remonter une erreur gérée par l'error_policy :

operator/src/instance_common.rs : inst.namespace().unwrap() (plusieurs occurrences dans do_reconcile/do_cleanup), context.as_object_mut().unwrap() répété, serde_json::from_value(...).unwrap() sur les patchs JSON.
common/src/k8sgeneric.rs : ~80 unwrap().
operator/src/jukebox.rs : serde_json::from_value(...).unwrap() pour les patchs.

Pour un opérateur, une Resource namespacée a normalement toujours un namespace, et les json!(...) littéraux ne devraient pas échouer — donc beaucoup de ces unwrap sont « théoriquement sûrs ». Mais :

Un panic dans une task de reconcile est plus difficile à diagnostiquer qu'une Error loggée par error_policy (qui requeue proprement).
Les unwrap() sur des données issues du cluster ou de désérialisation de status/options (champs Option, JSON arbitraire fourni par l'utilisateur dans spec.options) peuvent réellement paniquer sur entrée malformée.

Pistes

Remplacer les inst.namespace().unwrap() par une erreur explicite (Error::Other("instance without namespace")) ou un unwrap_or_default() documenté.
Auditer k8sgeneric.rs : distinguer les unwrap sur invariants internes (acceptables, à commenter) de ceux sur données externes (à convertir en Result).
Envisager un catch_unwind / supervision au niveau du contrôleur pour qu'un panic isolé ne tue pas le watcher.

Note

Pas de bug observé en production à ce jour ; ticket de durcissement préventif. Priorité basse, mais utile à traiter en même temps que les correctifs de instance_common.rs (#12, #15) puisque le fichier sera déjà ouvert.

## Constat Plusieurs `unwrap()` / `expect()` sont présents sur des chemins chauds du reconcile, où une donnée inattendue fait **paniquer la task du contrôleur** au lieu de remonter une erreur gérée par l'`error_policy` : - [`operator/src/instance_common.rs`](operator/src/instance_common.rs) : `inst.namespace().unwrap()` (plusieurs occurrences dans `do_reconcile`/`do_cleanup`), `context.as_object_mut().unwrap()` répété, `serde_json::from_value(...).unwrap()` sur les patchs JSON. - [`common/src/k8sgeneric.rs`](common/src/k8sgeneric.rs) : ~80 `unwrap()`. - [`operator/src/jukebox.rs`](operator/src/jukebox.rs) : `serde_json::from_value(...).unwrap()` pour les patchs. Pour un opérateur, une `Resource` namespacée a normalement toujours un namespace, et les `json!(...)` littéraux ne devraient pas échouer — donc beaucoup de ces `unwrap` sont « théoriquement sûrs ». Mais : 1. Un panic dans une task de reconcile est plus difficile à diagnostiquer qu'une `Error` loggée par `error_policy` (qui requeue proprement). 2. Les `unwrap()` sur des données issues du cluster ou de désérialisation de status/options (champs `Option`, JSON arbitraire fourni par l'utilisateur dans `spec.options`) peuvent réellement paniquer sur entrée malformée. ## Pistes 1. Remplacer les `inst.namespace().unwrap()` par une erreur explicite (`Error::Other("instance without namespace")`) ou un `unwrap_or_default()` documenté. 2. Auditer `k8sgeneric.rs` : distinguer les `unwrap` sur invariants internes (acceptables, à commenter) de ceux sur données externes (à convertir en `Result`). 3. Envisager un `catch_unwind` / supervision au niveau du contrôleur pour qu'un panic isolé ne tue pas le watcher. ## Note Pas de bug observé en production à ce jour ; ticket de durcissement préventif. Priorité basse, mais utile à traiter en même temps que les correctifs de `instance_common.rs` (#12, #15) puisque le fichier sera déjà ouvert.

xmortelette added the

Kind/Enhancement labels 2026-06-12 09:29:51 +02:00

shuss removed the

Reviewed

Confirmed

4

label 2026-06-12 10:27:42 +02:00

shuss commented

2026-06-12 11:15:35 +02:00

Intéressant, et oui, dans un code Rust propre il ne faut pas de unwrap.

Pour autant, je serais intéressé à une analyse plus en profondeur : un unwrap pour un champ qui est garanti mécaniquement d'être là n'est pas un risque.

Ici, je ne vois que "j'ai vu des unwrap, c'est pas bien". C'est pas faux, mais très incomplet. Typiquement l'exemple donné :
inst.namespace().unwrap() sur une instance d'un objets k8s qui est namespacé, le champ namespace est garanti par kubernetes. Ce n'est une option dans l'API que parce que l'api gère Namespaced et clusterwide de la même manière, et en effet, pour les instance d'objet non-namespacés, le champ est a None. Ici, le retour est invalide.

Intéressant, et oui, dans un code Rust propre il ne faut pas de unwrap. Pour autant, je serais intéressé à une analyse plus en profondeur : un unwrap pour un champ qui est garanti mécaniquement d'être là n'est pas un risque. Ici, je ne vois que "j'ai vu des unwrap, c'est pas bien". C'est pas faux, mais très incomplet. Typiquement l'exemple donné : `inst.namespace().unwrap()` sur une instance d'un objets k8s qui est namespacé, le champ namespace est garanti par kubernetes. Ce n'est une option dans l'API que parce que l'api gère Namespaced et clusterwide de la même manière, et en effet, pour les instance d'objet non-namespacés, le champ est a None. Ici, le retour est invalide.

shuss added

and removed

labels 2026-06-12 11:16:45 +02:00

shuss added the

Reviewed

need-more-info

5

label 2026-06-12 11:23:49 +02:00

xmortelette commented

2026-06-12 11:54:49 +02:00

Retour accepté — l'issue initiale comptait les unwrap sans les lire un par un. Voici l'audit en profondeur, occurrence par occurrence, avec classification.

Classe A — invariant garanti mécaniquement : aucun risque

Tous les unwrap des chemins de reconcile de l'opérateur (instance_common.rs, jukebox.rs) tombent dans cette classe :

Occurrence	Garantie
`inst.namespace().unwrap()` (do_reconcile/do_cleanup, ×5)	Les trois CRD d'instance sont namespaced ; l'API server garantit `metadata.namespace` sur tout objet servi par un watch namespaced. Le `Option` n'existe que parce que l'API kube-rs unifie namespaced/cluster-wide — exactement ton point.
`context.as_object_mut().unwrap()` (×13)	`base_context` est construit par l'opérateur lui-même comme objet JSON ; invariant interne au programme.
`serde_json::from_value(json!([...])).unwrap()` (patchs littéraux)	La forme est connue à l'écriture du code — un échec serait une erreur de compilation « morale », détectée au premier test.
`job.is_ok() && job.unwrap().is_some()` (×5)	Gardé par le `is_ok()` sur la même valeur, même expression. Non idiomatique (`matches!(job, Ok(Some(_)))` serait plus propre), mais pas un risque.

Conclusion sur l'opérateur : aucun unwrap réellement dangereux trouvé. L'issue surestimait le risque.

Classe B — garanti par convention d'usage, pas par le type

Dans common/src/k8sgeneric.rs (code exposé à Rhai, donc exécuté par l'agent — une panique y fait échouer le job, retenté ensuite ; elle ne tue pas le contrôleur) :

Les ~80 unwrap des chaînes de conditions sont en réalité gardés : chaque as_object().unwrap()["x"] est précédé d'un is_object() + test de présence de clé dans la même expression. Sûrs aujourd'hui. La fragilité est ailleurs : la garde et l'unwrap sont dupliqués et textuellement éloignés — un refactoring qui déplace l'un sans l'autre casse l'invariant silencieusement.
self.obj.uid().unwrap() : sûr si l'objet vient de l'API (uid toujours posé par le serveur), paniquerait sur un objet construit localement avant création. La garantie tient à la convention d'appel des scripts, pas au type.

Classe C — réellement faillible

Rien trouvé sur les chemins chauds après lecture complète. Les désérialisations de données externes (spec.options, status) passent par ?/map_err, pas par unwrap.

Requalification proposée

Pas de chasse aux unwrap. Au plus, du ménage opportuniste à coût quasi nul quand les fichiers seront ouverts pour #12/#15 :

un helper du type fn ns<T: Resource>(inst: &T) -> String portant un commentaire qui documente l'invariant namespaced — un seul endroit à lire au lieu de cinq ;
dans k8sgeneric.rs, remplacer les chaînes garde+unwrap dupliquées par un accesseur (json_path_str(obj, "status.conditions") → Option) — motivation : lisibilité et robustesse au refactoring, pas correction de bug ;
matches!(job, Ok(Some(_))) au passage.

Si ce niveau de ménage ne vaut pas une issue ouverte, celle-ci peut être fermée — l'audit ci-dessus en garde la trace.

Analyse et rédaction : Claude (assistant IA), publié via le compte de Xavier.

Retour accepté — l'issue initiale comptait les `unwrap` sans les lire un par un. Voici l'audit en profondeur, occurrence par occurrence, avec classification. ## Classe A — invariant garanti mécaniquement : aucun risque Tous les `unwrap` des chemins de reconcile de l'opérateur (`instance_common.rs`, `jukebox.rs`) tombent dans cette classe : | Occurrence | Garantie | |---|---| | `inst.namespace().unwrap()` (do_reconcile/do_cleanup, ×5) | Les trois CRD d'instance sont namespaced ; l'API server garantit `metadata.namespace` sur tout objet servi par un watch namespaced. Le `Option` n'existe que parce que l'API kube-rs unifie namespaced/cluster-wide — exactement ton point. | | `context.as_object_mut().unwrap()` (×13) | `base_context` est construit par l'opérateur lui-même comme objet JSON ; invariant interne au programme. | | `serde_json::from_value(json!([...])).unwrap()` (patchs littéraux) | La forme est connue à l'écriture du code — un échec serait une erreur de compilation « morale », détectée au premier test. | | `job.is_ok() && job.unwrap().is_some()` (×5) | Gardé par le `is_ok()` sur la même valeur, même expression. Non idiomatique (`matches!(job, Ok(Some(_)))` serait plus propre), mais pas un risque. | **Conclusion sur l'opérateur : aucun `unwrap` réellement dangereux trouvé.** L'issue surestimait le risque. ## Classe B — garanti par convention d'usage, pas par le type Dans `common/src/k8sgeneric.rs` (code exposé à Rhai, donc exécuté par l'**agent** — une panique y fait échouer le job, retenté ensuite ; elle ne tue pas le contrôleur) : - Les ~80 `unwrap` des chaînes de conditions sont en réalité **gardés** : chaque `as_object().unwrap()["x"]` est précédé d'un `is_object()` + test de présence de clé dans la même expression. Sûrs aujourd'hui. La fragilité est ailleurs : la garde et l'`unwrap` sont dupliqués et textuellement éloignés — un refactoring qui déplace l'un sans l'autre casse l'invariant silencieusement. - `self.obj.uid().unwrap()` : sûr si l'objet vient de l'API (uid toujours posé par le serveur), paniquerait sur un objet construit localement avant création. La garantie tient à la convention d'appel des scripts, pas au type. ## Classe C — réellement faillible Rien trouvé sur les chemins chauds après lecture complète. Les désérialisations de données externes (`spec.options`, status) passent par `?`/`map_err`, pas par `unwrap`. ## Requalification proposée Pas de chasse aux `unwrap`. Au plus, du ménage opportuniste à coût quasi nul quand les fichiers seront ouverts pour #12/#15 : 1. un helper du type `fn ns<T: Resource>(inst: &T) -> String` portant un commentaire qui documente l'invariant namespaced — un seul endroit à lire au lieu de cinq ; 2. dans `k8sgeneric.rs`, remplacer les chaînes garde+unwrap dupliquées par un accesseur (`json_path_str(obj, "status.conditions")` → `Option`) — motivation : lisibilité et robustesse au refactoring, pas correction de bug ; 3. `matches!(job, Ok(Some(_)))` au passage. Si ce niveau de ménage ne vaut pas une issue ouverte, celle-ci peut être fermée — l'audit ci-dessus en garde la trace. --- *Analyse et rédaction : Claude (assistant IA), publié via le compte de Xavier.*

shuss commented

2026-06-12 17:10:18 +02:00

La chasse aux unwrap en soit est pas le problème, de plus l'analyse en profondeur fourni est intéressante.
Au final tous ce qui rentre dans la classe B doit changer. Il n'y a pas d'urgence, mais oui, un code qui met mieux en évidence qu'il est robuste est clairement désirable. Un code plus idiomatique l'est tout autant. Les améliorations opportunistes sont les bienvenues

La chasse aux unwrap en soit est pas le problème, de plus l'analyse en profondeur fourni est intéressante. Au final tous ce qui rentre dans la classe B doit changer. Il n'y a pas d'urgence, mais oui, un code qui met mieux en évidence qu'il est robuste est clairement désirable. Un code plus idiomatique l'est tout autant. Les améliorations opportunistes sont les bienvenues

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: shuss/vynil#16