fix error removing policy from policy engine if no policies for existing deployment.

[dushaniw]

Fixing #1041

Summary by CodeRabbit

• Bug Fixes

  • Improved handling when removing derived policy configurations: missing policies are treated as non-errors (logged as debug) while other failures are surfaced and logged as errors.

• Chores

  • Enhanced success/error logging for policy operations with additional correlation context for better traceability.

• Tests

  • Added unit tests and mocks to validate policy-removal error classification and handling.

[coderabbitai]

Walkthrough

Refactors policy removal on API update to derive a dedicated policy ID, call RemovePolicy(policyID), and classify missing policies via a new storage sentinel error; updates logging to include api_id, policy_id, and correlation_id; adds tests and a mock policy manager.

Changes

Cohort / File(s)	Summary
Control plane policy removal & logging gateway/gateway-controller/pkg/controlplane/client.go	On API update compute derived policyID and call RemovePolicy(policyID); treat storage.IsPolicyNotFoundError as non-error (debug log with api_id,policy_id,correlation_id); include policy_id and correlation_id in success/error logs.
API handler error handling gateway/gateway-controller/pkg/api/handlers/handlers.go	Refined error handling when removing derived policy: only ErrPolicyNotFound is non-error (debug); other errors are logged as errors with api_id,policy_id,correlation_id.
Storage sentinel error added gateway/gateway-controller/pkg/storage/errors.go	Adds exported ErrPolicyNotFound = errors.New("policy configuration not found") and helper IsPolicyNotFoundError(err error) bool.
Policy store error wrapping gateway/gateway-controller/pkg/storage/policy_store.go	PolicyStore.Delete now wraps not-found cases with ErrPolicyNotFound using %w.
Tests & mocks gateway/gateway-controller/pkg/api/handlers/handlers_test.go, gateway/gateway-controller/pkg/controlplane/controlplane_test.go	Adds mock policy manager types and tests (TestPolicyRemovalErrorHandling, TestPolicyRemovalInControlPlane) to verify classification of remove errors (not-found vs other errors) and behavior on success.

Sequence Diagram(s)

sequenceDiagram
  participant PlatformAPI as Platform API
  participant ControlPlane as Gateway ControlPlane
  participant PolicyMgr as Policy Manager
  participant Storage as Storage

  PlatformAPI->>ControlPlane: Deploy API (update)
  ControlPlane->>ControlPlane: derive policyID = storedConfig.ID + "-policies"
  ControlPlane->>PolicyMgr: RemovePolicy(policyID)
  alt Remove returns ErrPolicyNotFound
    PolicyMgr->>ControlPlane: error (policy not found)
    ControlPlane->>ControlPlane: log debug (api_id, policy_id, correlation_id)
  else Remove returns other error
    PolicyMgr->>ControlPlane: error (storage error)
    ControlPlane->>ControlPlane: log error (api_id, policy_id, correlation_id)
  else Remove succeeds
    PolicyMgr->>ControlPlane: success
    ControlPlane->>ControlPlane: log success (api_id, policy_id, correlation_id)
  end
  ControlPlane->>Storage: update stored policy snapshot (if applicable)
  Storage->>ControlPlane: ack

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hopped through code to fix a fright,
Found missing policies hiding out of sight,
Gave them a name and gently logged the trail,
Now updates skip the noisy wail,
Thump, deploy, and onward I alight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is minimal—only a link to the issue. It lacks required template sections like Purpose, Goals, Approach, documentation, and test coverage information.	Expand the description to follow the repository template, including Purpose, Goals, Approach, documentation, and test coverage details.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: fixing error handling when removing policies from the policy engine for deployments without policies.
Linked Issues check	✅ Passed	The PR successfully addresses issue #1041 by implementing proper error handling for missing policy configurations during redeployments.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing policy removal error handling during API redeployments. No out-of-scope modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

gateway/gateway-controller/pkg/api/handlers/handlers_test.go (1)

2420-2464: Duplicate mock and test across packages.

MockPolicyManager and TestPolicyRemovalErrorHandling are structurally identical to mockPolicyManagerForCP and TestPolicyRemovalInControlPlane in controlplane_test.go. Both tests only exercise IsPolicyNotFoundError against a mock — they don't verify the actual handler/control-plane code paths that call RemovePolicy.

Consider:

1. Keeping a single test for IsPolicyNotFoundError in the storage package (closest to the code under test).
2. In each consumer package, testing the real code path (e.g., the handler or control-plane method that calls RemovePolicy and branches on IsPolicyNotFoundError) rather than re-testing the sentinel helper in isolation.

This way the mock is still useful but the test validates integration, not just errors.Is.

gateway/gateway-controller/pkg/controlplane/controlplane_test.go (1)

679-725: Same observation as handlers_test.go — this mock + test duplicates the pattern without testing Client integration.

The mock mockPolicyManagerForCP isn't wired into the Client (e.g., via createTestClient), so the test validates storage.IsPolicyNotFoundError in isolation rather than verifying the control plane's branching logic when RemovePolicy returns a not-found error. A more valuable test would inject this mock into the Client and exercise the actual update/delete flow that calls RemovePolicy, confirming the not-found case is silently handled while other errors are surfaced.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Fixes noisy errors during API redeployments when the initial deployment had no attached policies, by avoiding (or reducing) attempts to remove non-existent derived policy configurations from the policy engine (issue #1041).

Changes:

• Adds correlation_id (and policy_id) to policy-engine-related logs for better traceability.
• On update events with no derived policies, checks for policy existence before attempting removal and logs successful removals.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

gateway/gateway-controller/pkg/api/handlers/handlers.go (1)

803-812: ⚠️ Potential issue | 🟡 Minor

Inconsistent error handling: UpdateAPI swallows all RemovePolicy errors at debug level.

In CreateAPI (Lines 282-290), you now differentiate ErrPolicyNotFound (debug) from other errors (error level). However, UpdateAPI still logs all RemovePolicy errors at debug level, silently swallowing real storage/snapshot failures. The same applies to UpdateLLMProvider (Line 1377), UpdateLLMProxy (Line 1628), and UpdateMCPProxy (Line 2001).

Apply the same differentiated handling from CreateAPI to these update paths.

♻️ Suggested fix for UpdateAPI (apply similar pattern to other update methods)

 		} else {
 			// API no longer has policies, remove the existing policy configuration
 			policyID := existing.ID + "-policies"
 			if err := s.policyManager.RemovePolicy(policyID); err != nil {
-				// Log at debug level since policy may not exist if API never had policies
-				log.Debug("No policy configuration to remove", slog.String("policy_id", policyID))
+				if errors.Is(err, storage.ErrPolicyNotFound) {
+					log.Debug("No policy configuration to remove", slog.String("policy_id", policyID))
+				} else {
+					log.Error("Failed to remove policy configuration",
+						slog.Any("error", err),
+						slog.String("policy_id", policyID))
+				}
 			} else {

🧹 Nitpick comments (1)

gateway/gateway-controller/pkg/storage/errors.go (1)

28-29: Consider adding an IsPolicyNotFoundError helper for consistency.

Other sentinel errors in this file (ErrNotFound, ErrConflict, ErrDatabaseUnavailable, ErrOperationNotAllowed) all have corresponding Is*Error() helper functions. ErrPolicyNotFound lacks one, while callers use errors.Is() directly. This is a minor inconsistency.

♻️ Suggested addition

+// IsPolicyNotFoundError checks if an error is a policy not found error
+func IsPolicyNotFoundError(err error) bool {
+	return errors.Is(err, ErrPolicyNotFound)
+}