forked from kubeflow/kubeflow
-
Notifications
You must be signed in to change notification settings - Fork 49
Open
Description
Author: @app/coderabbitai · Created: 2025-06-26T11:29:43Z · URL:
Problem Description
The current error handling logic for ConfigMap retrieval operations may lead to unintended behavior when r.Get() returns errors other than NotFound.
Issue Details
When r.Get(ctx, ..., foundTrustedCAConfigMap) returns an error that is not apierrs.IsNotFound(err), the foundTrustedCAConfigMap variable will contain its zero value, not the actual state from the cluster.
The problematic pattern appears to be:
err := r.Get(ctx, client.ObjectKey{...}, foundTrustedCAConfigMap)
if apierrs.IsNotFound(err) {
// Create logic
} else if !reflect.DeepEqual(foundTrustedCAConfigMap.Data, desiredTrustedCAConfigMap.Data) {
// Update logic - PROBLEMATIC: foundTrustedCAConfigMap might be zero-value
}Impact
This could result in:
- Attempting to update a non-existent or incorrectly fetched resource
- Unintended behavior when
r.Getfails for reasons other than ConfigMap not found (e.g., network issues, RBAC problems) - The
!reflect.DeepEqualcheck could evaluate to true ifdesiredTrustedCAConfigMap.Datais not empty, leading to an update attempt on a zero-value struct
Suggested Fix
The logic should ensure proper error handling:
err := r.Get(ctx, client.ObjectKey{...}, foundTrustedCAConfigMap)
if err != nil {
if apierrs.IsNotFound(err) {
// Create logic
} else {
// Handle other errors (e.g., log and return, or retry)
r.Log.Error(err, "Failed to get workbench-trusted-ca-bundle ConfigMap")
return err // Or handle appropriately
}
} else { // err == nil, foundTrustedCAConfigMap is valid
if !reflect.DeepEqual(foundTrustedCAConfigMap.Data, desiredTrustedCAConfigMap.Data) {
// Update logic
}
}References
- Related PR: NO-JIRA: delete
components/notebook-controller/config/overlays/standalone-service-mesh/cert-secret.yaml#637 - Original comment: NO-JIRA: delete
components/notebook-controller/config/overlays/standalone-service-mesh/cert-secret.yaml#637 (comment)
Reported by: @jiridanek
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
No labels