Skip to content

Commit 6290e08

Browse files
committed
fix: address PR review feedback for OpenShift overlay
- Move SetClusterDefaultsNamespace before buildConfigMapCacheNamespaces so the manager cache watches the correct operator namespace instead of the hardcoded default - Add strategic merge patches for OpenShift overlay to replace local-dev keycloak.localtest.me URLs with in-cluster keycloak-service.keycloak.svc endpoints - Patch authbridge-runtime-config issuer, keycloak_url, and keycloak_realm for both inbound and outbound pipelines - Patch spiffe-helper-config jwt_audience to use in-cluster URL Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> fix: address PR review feedback for OpenShift overlay - Protect ClusterDefaultsNamespace with sync.Once to prevent potential data races from concurrent reads/writes across reconciler goroutines - Requeue on ensureNamespaceSCCBinding failure when ClusterRole exists (OpenShift) to prevent agent pods from failing with SCC violations due to transient API errors - Fix identity.type nesting in OpenShift authbridge patch to match the base template structure (under outbound.plugins[0].config) - Document root CA dependency chain and recovery procedure in certificates.yaml - Clarify getOperatorNamespace fallback exists for local dev/test runs Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
1 parent 4ccf702 commit 6290e08

7 files changed

Lines changed: 89 additions & 13 deletions

File tree

kagenti-operator/cmd/main.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,9 @@ func init() {
7676
}
7777

7878
// getOperatorNamespace returns the namespace the operator is running in.
79-
// Reads from POD_NAMESPACE environment variable (set via downward API in deployment),
80-
// falling back to kagenti-system if not set.
79+
// In production, the manager_webhook_patch.yaml injects POD_NAMESPACE via
80+
// the downward API, so the fallback is effectively dead code. It exists for
81+
// local development and test runs where the webhook patch is not applied.
8182
func getOperatorNamespace() string {
8283
if ns := os.Getenv("POD_NAMESPACE"); ns != "" {
8384
return ns
@@ -309,6 +310,11 @@ func main() {
309310
})
310311
}
311312

313+
// ========================================
314+
// Operator namespace resolution
315+
// ========================================
316+
controller.SetClusterDefaultsNamespace(getOperatorNamespace())
317+
312318
cmCacheNamespaces := buildConfigMapCacheNamespaces(
313319
requireA2ASignature, spireTrustBundleConfigMapName, spireTrustBundleConfigMapNS,
314320
)
@@ -365,11 +371,6 @@ func main() {
365371
}
366372
}
367373

368-
// ========================================
369-
// Operator namespace resolution
370-
// ========================================
371-
controller.SetClusterDefaultsNamespace(getOperatorNamespace())
372-
373374
if !requireA2ASignature && !enableVerifiedFetch {
374375
setupLog.Info("WARNING: Neither --require-a2a-signature nor --enable-verified-fetch is set. " +
375376
"Identity binding requires at least one trust mechanism to function. " +

kagenti-operator/config/istio-trust/certificates.yaml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,12 @@
11
# cert-manager Certificates for multi-mesh Istio shared trust.
22
# Root CA in cert-manager namespace, intermediate CAs per Istio control plane.
3+
#
4+
# Dependency chain: Root CA (istio-mesh-root-ca, 10-year duration) issues all
5+
# intermediate CAs via the istio-mesh-ca ClusterIssuer. If the root CA secret
6+
# (istio-mesh-root-ca-secret) is accidentally deleted, all intermediates become
7+
# orphaned and workload mTLS will break.
8+
# Recovery: re-create this Certificate to generate a new root CA, then restart
9+
# cert-manager to re-issue the intermediate CA certificates.
310
---
411
apiVersion: cert-manager.io/v1
512
kind: Certificate

kagenti-operator/config/openshift/kustomization.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,3 +9,7 @@ resources:
99
- ../istio-trust
1010
- ../authbridge
1111
- ../security
12+
13+
patches:
14+
- path: patches/authbridge-keycloak.yaml
15+
- path: patches/spiffe-helper-keycloak.yaml
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Strategic merge patch: override authbridge-runtime-config with
2+
# in-cluster Keycloak URLs for OpenShift deployments.
3+
apiVersion: v1
4+
kind: ConfigMap
5+
metadata:
6+
name: authbridge-runtime-config
7+
namespace: kagenti-operator-system
8+
data:
9+
config.yaml: |
10+
spiffe: {}
11+
pipeline:
12+
inbound:
13+
plugins:
14+
- name: jwt-validation
15+
config:
16+
issuer: "http://keycloak-service.keycloak.svc:8080/realms/kagenti"
17+
keycloak_url: "http://keycloak-service.keycloak.svc:8080"
18+
keycloak_realm: "kagenti"
19+
outbound:
20+
plugins:
21+
- name: token-exchange
22+
config:
23+
keycloak_url: "http://keycloak-service.keycloak.svc:8080"
24+
keycloak_realm: "kagenti"
25+
default_policy: "passthrough"
26+
identity:
27+
type: "client-secret"
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Strategic merge patch: override spiffe-helper-config with
2+
# in-cluster Keycloak JWT audience for OpenShift deployments.
3+
apiVersion: v1
4+
kind: ConfigMap
5+
metadata:
6+
name: spiffe-helper-config
7+
namespace: kagenti-operator-system
8+
data:
9+
helper.conf: |
10+
agent_address = "/spiffe-workload-api/spire-agent.sock"
11+
cmd = ""
12+
cmd_args = ""
13+
svid_file_name = "/opt/svid.pem"
14+
svid_key_file_name = "/opt/svid_key.pem"
15+
svid_bundle_file_name = "/opt/svid_bundle.pem"
16+
cert_file_mode = 0644
17+
key_file_mode = 0640
18+
jwt_svids = [{jwt_audience="http://keycloak-service.keycloak.svc:8080/realms/kagenti", jwt_svid_file_name="/opt/jwt_svid.token"}]
19+
jwt_svid_file_mode = 0644
20+
include_federated_domains = true

kagenti-operator/internal/controller/agentruntime_config.go

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"encoding/hex"
2323
"encoding/json"
2424
"fmt"
25+
"sync"
2526

2627
corev1 "k8s.io/api/core/v1"
2728
"k8s.io/apimachinery/pkg/types"
@@ -50,14 +51,22 @@ const (
5051
)
5152

5253
// ClusterDefaultsNamespace is the namespace where cluster-level ConfigMaps
53-
// and template ConfigMaps live. Defaults to "kagenti-system"; set to the
54-
// operator's own namespace by main() via SetClusterDefaultsNamespace
55-
var ClusterDefaultsNamespace = clusterDefaultsNamespaceDefault
54+
// and template ConfigMaps live. Defaults to "kagenti-system"; set once to the
55+
// operator's own namespace by main() via SetClusterDefaultsNamespace.
56+
//
57+
// Write-once semantics: SetClusterDefaultsNamespace must be called exactly once
58+
// from main() before the manager starts. Subsequent calls are no-ops.
59+
var (
60+
ClusterDefaultsNamespace = clusterDefaultsNamespaceDefault
61+
clusterDefaultsNamespaceOnce sync.Once
62+
)
5663

5764
func SetClusterDefaultsNamespace(ns string) {
58-
if ns != "" {
59-
ClusterDefaultsNamespace = ns
60-
}
65+
clusterDefaultsNamespaceOnce.Do(func() {
66+
if ns != "" {
67+
ClusterDefaultsNamespace = ns
68+
}
69+
})
6170
}
6271

6372
// resolvedConfig is the canonical representation used for hash computation.

kagenti-operator/internal/controller/agentruntime_controller.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,11 +210,19 @@ func (r *AgentRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Request
210210
// 4.7. Ensure SCC RoleBinding exists in the namespace.
211211
// Creates a RoleBinding granting all ServiceAccounts in the namespace
212212
// access to the kagenti-authbridge SCC. No-op on non-OpenShift clusters.
213+
// On OpenShift, a transient failure is retried via requeue to prevent
214+
// agent pods from failing with SCC violations at runtime.
213215
if err := r.ensureNamespaceSCCBinding(ctx, rt.Namespace); err != nil {
214216
logger.Error(err, "Failed to ensure SCC RoleBinding")
215217
if r.Recorder != nil {
216218
r.Recorder.Event(rt, corev1.EventTypeWarning, "SCCBindingError", err.Error())
217219
}
220+
r.setPhase(rt, agentv1alpha1.RuntimePhaseError)
221+
r.setCondition(rt, ConditionTypeReady, metav1.ConditionFalse, "SCCBindingError", err.Error())
222+
if statusErr := r.Status().Update(ctx, rt); statusErr != nil {
223+
logger.Error(statusErr, "Failed to update status")
224+
}
225+
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
218226
}
219227

220228
// 5. Compute config hash from merged configuration (cluster → namespace)

0 commit comments

Comments
 (0)