ESO-396: Refactor Proxy NetworkPolicy and user CA bundle implementation for improved handling

Signed-off-by: Bharath B <bhb@redhat.com>

Author: bharath-b-rh

bindata/external-secrets/networkpolicy_allow-dns.yaml

@@ -33,4 +33,4 @@ spec:
         - protocol: UDP
           port: 53
   policyTypes:
-      - Egress
+      - Egress

pkg/controller/common/errors.go

@@ -27,11 +27,6 @@ const (
 	// The operator sets Degraded. Recovery is driven by watches on the affected resource;
 	// NotFound errors still use periodic requeue until the referenced object exists.
 	UserConfigurationError ErrorReason = "UserConfigurationError"
-
-	// UserTrustedCABundleError indicates user configured trustedCABundle is invalid or incomplete.
-	// The operator sets Degraded. Recovery for existing ConfigMaps is driven by ConfigMap watches;
-	// NotFound errors still use periodic requeue until the ConfigMap is created.
-	UserTrustedCABundleError ErrorReason = "UserTrustedCABundleError"
 )
 
 // ReconcileError represents an error that occurred during reconciliation.
@@ -88,18 +83,6 @@ func NewUserConfigurationError(err error, message string, args ...any) *Reconcil
 	}
 }
 
-// NewUserTrustedCABundleError creates a ReconcileError for user trustedCABundle misconfiguration.
-func NewUserTrustedCABundleError(err error, message string, args ...any) *ReconcileError {
-	if err == nil {
-		return nil
-	}
-	return &ReconcileError{
-		Reason:  UserTrustedCABundleError,
-		Message: fmt.Sprintf(message, args...),
-		Err:     err,
-	}
-}
-
 // IsIrrecoverableError checks if the given error is a ReconcileError
 // with IrrecoverableError reason. Returns false if err is nil or
 // not a ReconcileError.
@@ -138,24 +121,6 @@ func IsUserConfigurationNotFound(err error) bool {
 	return apierrors.IsNotFound(rerr.Err)
 }
 
-// IsUserTrustedCABundleError checks if the given error is a ReconcileError with UserTrustedCABundleError reason.
-func IsUserTrustedCABundleError(err error) bool {
-	rerr := &ReconcileError{}
-	if errors.As(err, &rerr) {
-		return rerr.Reason == UserTrustedCABundleError
-	}
-	return false
-}
-
-// IsUserTrustedCABundleNotFound reports whether err is a UserTrustedCABundleError caused by a missing ConfigMap.
-func IsUserTrustedCABundleNotFound(err error) bool {
-	rerr := &ReconcileError{}
-	if !errors.As(err, &rerr) || rerr.Reason != UserTrustedCABundleError {
-		return false
-	}
-	return apierrors.IsNotFound(rerr.Err)
-}
-
 // Error implements the error interface, returning a formatted string
 // containing both the message and the underlying error.
 func (e *ReconcileError) Error() string {

pkg/controller/common/errors_test.go

@@ -22,70 +22,51 @@ func TestIsUserConfigurationNotFound(t *testing.T) {
 	}{
 		{
 			name: "user configuration error wrapping NotFound",
-			err:  NewUserConfigurationError(notFound, "issuer %q not found", "external-secrets/missing"),
+			err:  NewUserConfigurationError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
 			want: true,
 		},
 		{
 			name: "wrapped user configuration NotFound",
-			err:  fmt.Errorf("reconcile deployment: %w", NewUserConfigurationError(notFound, "issuer %q not found", "external-secrets/missing")),
+			err:  fmt.Errorf("reconcile deployment: %w", NewUserConfigurationError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing")),
 			want: true,
 		},
+		{
+			name: "user configuration error missing key",
+			err: NewUserConfigurationError(
+				fmt.Errorf("key %q not found", "ca.crt"),
+				"trustedCABundle ConfigMap %q does not contain key %q",
+				"external-secrets/cm", "ca.crt",
+			),
+			want: false,
+		},
 		{
 			name: "user configuration error invalid PEM",
-			err:  NewUserConfigurationError(errors.New("invalid pem"), "invalid issuer configuration"),
+			err:  NewUserConfigurationError(errors.New("trusted CA bundle contains no valid PEM-encoded CA certificates"), "trustedCABundle ConfigMap %q key %q has invalid PEM", "external-secrets/cm", "ca.crt"),
 			want: false,
 		},
 		{
-			name: "bare NotFound",
-			err:  notFound,
+			name: "user configuration error invalid proxy URL",
+			err:  NewUserConfigurationError(errors.New("invalid proxy URL configured"), "proxy configuration validation failed"),
 			want: false,
 		},
-		{name: "nil error", err: nil, want: false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			if got := IsUserConfigurationNotFound(tt.err); got != tt.want {
-				t.Fatalf("IsUserConfigurationNotFound() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-func TestIsUserTrustedCABundleNotFound(t *testing.T) {
-	t.Parallel()
-
-	cmGR := schema.GroupResource{Resource: "configmaps"}
-	notFound := apierrors.NewNotFound(cmGR, "missing")
-
-	tests := []struct {
-		name string
-		err  error
-		want bool
-	}{
 		{
-			name: "trusted CA bundle error wrapping NotFound",
-			err:  NewUserTrustedCABundleError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
-			want: true,
+			name: "bare NotFound",
+			err:  notFound,
+			want: false,
 		},
 		{
-			name: "wrapped trusted CA bundle NotFound",
-			err:  fmt.Errorf("reconcile deployment: %w", NewUserTrustedCABundleError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing")),
-			want: true,
+			name: "retry required reconcile error",
+			err:  NewRetryRequiredError(notFound, "fetch configmap %q", "user-ca"),
+			want: false,
 		},
 		{
-			name: "trusted CA bundle error missing key",
-			err: NewUserTrustedCABundleError(
-				fmt.Errorf("key %q not found", "ca.crt"),
-				"trustedCABundle ConfigMap %q does not contain key %q",
-				"external-secrets/cm", "ca.crt",
-			),
+			name: "irrecoverable reconcile error",
+			err:  NewIrrecoverableError(apierrors.NewForbidden(cmGR, "user-ca", errors.New("denied")), "forbidden"),
 			want: false,
 		},
 		{
-			name: "user configuration NotFound is not trusted CA NotFound",
-			err:  NewUserConfigurationError(notFound, "issuer %q not found", "external-secrets/missing"),
+			name: "generic error",
+			err:  errors.New("something failed"),
 			want: false,
 		},
 		{name: "nil error", err: nil, want: false},
@@ -94,8 +75,8 @@ func TestIsUserTrustedCABundleNotFound(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := IsUserTrustedCABundleNotFound(tt.err); got != tt.want {
-				t.Fatalf("IsUserTrustedCABundleNotFound() = %v, want %v", got, tt.want)
+			if got := IsUserConfigurationNotFound(tt.err); got != tt.want {
+				t.Fatalf("IsUserConfigurationNotFound() = %v, want %v", got, tt.want)
 			}
 		})
 	}
@@ -112,56 +93,44 @@ func TestIsUserConfigurationError(t *testing.T) {
 		want bool
 	}{
 		{
-			name: "user configuration error",
-			err:  NewUserConfigurationError(notFound, "issuer %q not found", "external-secrets/missing"),
+			name: "trusted CA bundle user configuration error",
+			err:  NewUserConfigurationError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
 			want: true,
 		},
 		{
-			name: "trusted CA bundle error is not user configuration",
-			err:  NewUserTrustedCABundleError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
-			want: false,
+			name: "proxy user configuration error",
+			err:  NewUserConfigurationError(errors.New("invalid proxy URL"), "proxy configuration validation failed"),
+			want: true,
 		},
-		{name: "nil error", err: nil, want: false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			if got := IsUserConfigurationError(tt.err); got != tt.want {
-				t.Fatalf("IsUserConfigurationError() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-func TestIsUserTrustedCABundleError(t *testing.T) {
-	t.Parallel()
-
-	notFound := apierrors.NewNotFound(schema.GroupResource{Resource: "configmaps"}, "missing")
-
-	tests := []struct {
-		name string
-		err  error
-		want bool
-	}{
 		{
-			name: "trusted CA bundle error",
-			err:  NewUserTrustedCABundleError(notFound, "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
+			name: "wrapped user configuration error",
+			err:  fmt.Errorf("apply user CA: %w", NewUserConfigurationError(errors.New("invalid pem"), "invalid PEM")),
 			want: true,
 		},
 		{
-			name: "user configuration error is not trusted CA bundle",
-			err:  NewUserConfigurationError(notFound, "issuer %q not found", "external-secrets/missing"),
+			name: "client NotFound maps to retry required",
+			err:  FromClientError(notFound, "failed to fetch trustedCABundle ConfigMap %q", "external-secrets/missing"),
 			want: false,
 		},
+		{
+			name: "irrecoverable reconcile error",
+			err:  NewIrrecoverableError(apierrors.NewForbidden(schema.GroupResource{Resource: "configmaps"}, "user-ca", errors.New("denied")), "forbidden"),
+			want: false,
+		},
+		{
+			name: "retry required reconcile error",
+			err:  NewRetryRequiredError(errors.New("timeout"), "temporary failure"),
+			want: false,
+		},
+		{name: "bare NotFound", err: notFound, want: false},
 		{name: "nil error", err: nil, want: false},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := IsUserTrustedCABundleError(tt.err); got != tt.want {
-				t.Fatalf("IsUserTrustedCABundleError() = %v, want %v", got, tt.want)
+			if got := IsUserConfigurationError(tt.err); got != tt.want {
+				t.Fatalf("IsUserConfigurationError() = %v, want %v", got, tt.want)
 			}
 		})
 	}
@@ -182,14 +151,34 @@ func TestIsIrrecoverableError(t *testing.T) {
 			err:  NewIrrecoverableError(errors.New("bad config"), "invalid configuration"),
 			want: true,
 		},
+		{
+			name: "forbidden client error",
+			err:  FromClientError(apierrors.NewForbidden(cmGR, "user-ca", errors.New("denied")), "patch configmap %q", "user-ca"),
+			want: true,
+		},
+		{
+			name: "unauthorized client error",
+			err:  FromClientError(apierrors.NewUnauthorized("unauthorized"), "get configmap"),
+			want: true,
+		},
+		{
+			name: "invalid client error",
+			err:  FromClientError(apierrors.NewInvalid(schema.GroupKind{Kind: "ConfigMap"}, "user-ca", nil), "invalid configmap"),
+			want: true,
+		},
+		{
+			name: "NotFound client error is retry required",
+			err:  FromClientError(apierrors.NewNotFound(cmGR, "user-ca"), "get configmap %q", "user-ca"),
+			want: false,
+		},
 		{
 			name: "user configuration error",
-			err:  NewUserConfigurationError(apierrors.NewNotFound(cmGR, "missing"), "issuer %q not found", "external-secrets/missing"),
+			err:  NewUserConfigurationError(apierrors.NewNotFound(cmGR, "missing"), "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
 			want: false,
 		},
 		{
-			name: "trusted CA bundle error",
-			err:  NewUserTrustedCABundleError(apierrors.NewNotFound(cmGR, "missing"), "trustedCABundle ConfigMap %q not found", "external-secrets/missing"),
+			name: "retry required reconcile error",
+			err:  NewRetryRequiredError(errors.New("conflict"), "update conflict"),
 			want: false,
 		},
 		{name: "nil error", err: nil, want: false},
@@ -227,11 +216,31 @@ func TestFromClientError(t *testing.T) {
 			err:        apierrors.NewForbidden(cmGR, "user-ca", errors.New("denied")),
 			wantReason: IrrecoverableError,
 		},
+		{
+			name:       "Unauthorized",
+			err:        apierrors.NewUnauthorized("unauthorized"),
+			wantReason: IrrecoverableError,
+		},
+		{
+			name:       "Invalid",
+			err:        apierrors.NewInvalid(schema.GroupKind{Kind: "ConfigMap"}, "user-ca", nil),
+			wantReason: IrrecoverableError,
+		},
+		{
+			name:       "BadRequest",
+			err:        apierrors.NewBadRequest("bad request"),
+			wantReason: IrrecoverableError,
+		},
 		{
 			name:       "ServiceUnavailable",
 			err:        apierrors.NewServiceUnavailable("service unavailable"),
 			wantReason: RetryRequiredError,
 		},
+		{
+			name:       "Conflict",
+			err:        apierrors.NewConflict(cmGR, "user-ca", errors.New("conflict")),
+			wantReason: RetryRequiredError,
+		},
 	}
 
 	for _, tt := range tests {
@@ -250,6 +259,9 @@ func TestFromClientError(t *testing.T) {
 			if got.Reason != tt.wantReason {
 				t.Fatalf("Reason = %q, want %q", got.Reason, tt.wantReason)
 			}
+			if !errors.Is(got, tt.err) {
+				t.Fatalf("expected wrapped error %v, got %v", tt.err, got.Err)
+			}
 		})
 	}
 }
@@ -261,11 +273,22 @@ func TestReconcileErrorConstructorsNil(t *testing.T) {
 		name string
 		got  *ReconcileError
 	}{
-		{name: "NewIrrecoverableError", got: NewIrrecoverableError(nil, "msg")},
-		{name: "NewRetryRequiredError", got: NewRetryRequiredError(nil, "msg")},
-		{name: "NewUserConfigurationError", got: NewUserConfigurationError(nil, "msg")},
-		{name: "NewUserTrustedCABundleError", got: NewUserTrustedCABundleError(nil, "msg")},
-		{name: "FromClientError", got: FromClientError(nil, "msg")},
+		{
+			name: "NewIrrecoverableError",
+			got:  NewIrrecoverableError(nil, "msg"),
+		},
+		{
+			name: "NewRetryRequiredError",
+			got:  NewRetryRequiredError(nil, "msg"),
+		},
+		{
+			name: "NewUserConfigurationError",
+			got:  NewUserConfigurationError(nil, "msg"),
+		},
+		{
+			name: "FromClientError",
+			got:  FromClientError(nil, "msg"),
+		},
 	}
 
 	for _, tt := range tests {

pkg/controller/external_secrets/configmap_test.go

@@ -232,7 +232,11 @@ func TestEnsureTrustedCABundleConfigMap(t *testing.T) {
 			} else {
 				esc = testESCWithProxy()
 			}
-			r.setProxyStatus(esc)
+			if proxyConfig, err := r.resolveProxyConfiguration(esc); err != nil {
+				t.Fatalf("resolveProxyConfiguration() error = %v", err)
+			} else {
+				r.proxyConfig = proxyConfig
+			}
 
 			err := r.ensureTrustedCABundleConfigMap(esc, tt.resourceMetadata)