Skip to content

Commit 3f8f78b

Browse files
maskarbclaude
andauthored
fix(backend): eliminate race condition in feature flag override writes (#839)
## Summary - Replace read-modify-write pattern with merge patches for ConfigMap updates, eliminating a race condition that caused 500 errors during batch saves - `setFlagOverride`: now uses `MergePatchType` to atomically set a single key; falls back to `Create` + retry if the ConfigMap doesn't exist yet - `DeleteFeatureFlagOverride`: now uses `MergePatchType` with `null` value to atomically remove a key - Permission checks changed from `"update"`/`"create"` to `"patch"` to match the new write method ## Problem When workspace admins saved multiple feature flag overrides via the batch save UI, the frontend sent parallel PUT requests. The old `setFlagOverride` used a GET → modify → Update pattern. All concurrent requests read the same `resourceVersion`, and every request after the first failed with a Kubernetes 409 Conflict (surfaced as HTTP 500). ## Test plan - [x] Existing Ginkgo feature flag admin tests pass (3 previously-failing create-from-scratch tests now pass) - [ ] Manual: toggle 3+ flags in Workspace Settings and click Save — all should succeed Fixes: [RHOAIENG-52262](https://issues.redhat.com/browse/RHOAIENG-52262) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1ed7618 commit 3f8f78b

1 file changed

Lines changed: 48 additions & 54 deletions

File tree

components/backend/handlers/featureflags_admin.go

Lines changed: 48 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
corev1 "k8s.io/api/core/v1"
1919
"k8s.io/apimachinery/pkg/api/errors"
2020
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
21+
"k8s.io/apimachinery/pkg/types"
2122
"k8s.io/client-go/kubernetes"
2223
)
2324

@@ -417,21 +418,8 @@ func setFlagOverride(c *gin.Context, value string, responseMsg string) {
417418
return
418419
}
419420

420-
// Check if ConfigMap exists to determine required verb
421-
cm, err := reqK8s.CoreV1().ConfigMaps(namespace).Get(ctx, FeatureFlagOverridesConfigMap, metav1.GetOptions{})
422-
configMapExists := !errors.IsNotFound(err)
423-
if err != nil && !errors.IsNotFound(err) {
424-
log.Printf("Failed to get feature flag overrides ConfigMap in %s: %v", namespace, err)
425-
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set feature flag override"})
426-
return
427-
}
428-
429-
// Check user has permission to create/update ConfigMaps in namespace
430-
verb := "update"
431-
if !configMapExists {
432-
verb = "create"
433-
}
434-
allowed, err := checkConfigMapPermission(ctx, reqK8s, namespace, verb)
421+
// Check user has permission to patch ConfigMaps in namespace
422+
allowed, err := checkConfigMapPermission(ctx, reqK8s, namespace, "patch")
435423
if err != nil {
436424
log.Printf("Failed to check ConfigMap permissions in %s: %v", namespace, err)
437425
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check permissions"})
@@ -442,8 +430,21 @@ func setFlagOverride(c *gin.Context, value string, responseMsg string) {
442430
return
443431
}
444432

445-
// Use service account for the write (after validation)
446-
if !configMapExists {
433+
// Merge patch: atomically sets a single key in the ConfigMap data map.
434+
// Concurrent patches to different keys never conflict because each patch
435+
// only touches its own key — no read-modify-write cycle required.
436+
// If the ConfigMap doesn't exist yet, create it first, then patch.
437+
mergePatch := fmt.Sprintf(`{"data":{%q:%q}}`, flagName, value)
438+
439+
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Patch(
440+
ctx,
441+
FeatureFlagOverridesConfigMap,
442+
types.MergePatchType,
443+
[]byte(mergePatch),
444+
metav1.PatchOptions{},
445+
)
446+
if errors.IsNotFound(err) {
447+
// ConfigMap doesn't exist yet — create it with the flag value, then done.
447448
newCM := &corev1.ConfigMap{
448449
ObjectMeta: metav1.ObjectMeta{
449450
Name: FeatureFlagOverridesConfigMap,
@@ -453,28 +454,22 @@ func setFlagOverride(c *gin.Context, value string, responseMsg string) {
453454
"app.kubernetes.io/component": "feature-flags",
454455
},
455456
},
456-
Data: map[string]string{},
457+
Data: map[string]string{flagName: value},
457458
}
458-
cm, err = K8sClient.CoreV1().ConfigMaps(namespace).Create(ctx, newCM, metav1.CreateOptions{})
459+
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Create(ctx, newCM, metav1.CreateOptions{})
459460
if errors.IsAlreadyExists(err) {
460-
// Another concurrent request created the ConfigMap; fetch it and proceed to update.
461-
cm, err = K8sClient.CoreV1().ConfigMaps(namespace).Get(ctx, FeatureFlagOverridesConfigMap, metav1.GetOptions{})
462-
}
463-
if err != nil {
464-
log.Printf("Failed to create feature flag overrides ConfigMap in %s: %v", namespace, err)
465-
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set feature flag override"})
466-
return
461+
// Another request created it concurrently — retry the patch.
462+
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Patch(
463+
ctx,
464+
FeatureFlagOverridesConfigMap,
465+
types.MergePatchType,
466+
[]byte(mergePatch),
467+
metav1.PatchOptions{},
468+
)
467469
}
468470
}
469-
470-
if cm.Data == nil {
471-
cm.Data = map[string]string{}
472-
}
473-
cm.Data[flagName] = value
474-
475-
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Update(ctx, cm, metav1.UpdateOptions{})
476471
if err != nil {
477-
log.Printf("Failed to update feature flag overrides ConfigMap in %s: %v", namespace, err)
472+
log.Printf("Failed to set feature flag override ConfigMap in %s: %v", namespace, err)
478473
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set feature flag override"})
479474
return
480475
}
@@ -519,21 +514,7 @@ func DeleteFeatureFlagOverride(c *gin.Context) {
519514
return
520515
}
521516

522-
cm, err := reqK8s.CoreV1().ConfigMaps(namespace).Get(ctx, FeatureFlagOverridesConfigMap, metav1.GetOptions{})
523-
if errors.IsNotFound(err) {
524-
c.JSON(http.StatusOK, gin.H{
525-
"message": "No override to remove",
526-
"flag": flagName,
527-
})
528-
return
529-
}
530-
if err != nil {
531-
log.Printf("Failed to get feature flag overrides ConfigMap in %s: %v", namespace, err)
532-
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get overrides"})
533-
return
534-
}
535-
536-
allowed, err := checkConfigMapPermission(ctx, reqK8s, namespace, "update")
517+
allowed, err := checkConfigMapPermission(ctx, reqK8s, namespace, "patch")
537518
if err != nil {
538519
log.Printf("Failed to check ConfigMap permissions in %s: %v", namespace, err)
539520
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to check permissions"})
@@ -544,13 +525,26 @@ func DeleteFeatureFlagOverride(c *gin.Context) {
544525
return
545526
}
546527

547-
if cm.Data != nil {
548-
delete(cm.Data, flagName)
549-
}
528+
// JSON Merge Patch: setting a key to null removes it from the map.
529+
// If the ConfigMap doesn't exist, the patch returns 404 — treat as no-op.
530+
mergePatch := fmt.Sprintf(`{"data":{%q:null}}`, flagName)
550531

551-
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Update(ctx, cm, metav1.UpdateOptions{})
532+
_, err = K8sClient.CoreV1().ConfigMaps(namespace).Patch(
533+
ctx,
534+
FeatureFlagOverridesConfigMap,
535+
types.MergePatchType,
536+
[]byte(mergePatch),
537+
metav1.PatchOptions{},
538+
)
539+
if errors.IsNotFound(err) {
540+
c.JSON(http.StatusOK, gin.H{
541+
"message": "No override to remove",
542+
"flag": flagName,
543+
})
544+
return
545+
}
552546
if err != nil {
553-
log.Printf("Failed to update feature flag overrides ConfigMap in %s: %v", namespace, err)
547+
log.Printf("Failed to patch feature flag overrides ConfigMap in %s: %v", namespace, err)
554548
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to remove override"})
555549
return
556550
}

0 commit comments

Comments
 (0)