Address code review feedback

Fixed issues from code review:
- MEDIUM: Reject explicit primaryUpstreamProvider when no embedded auth
  server is configured. The early-return direct-IdP branch in
  validateAuthzUpstreamAvailable now checks for a non-empty explicit
  name first and returns SpecValidationError with
  ConditionReasonAuthzUpstreamUnknown when set — closing the silent
  misconfiguration where the converter would forward an unresolvable
  name into Cedar config at runtime.
- MEDIUM: Update the converter block comment so it accurately describes
  both rejection paths (mismatch with declared upstreams AND explicit
  name without an embedded AS), keeping the comment synchronized with
  the validator's behavior per the go-style.md rule.
- MEDIUM: Replace the misleading "is normalized via ResolveUpstreamName"
  converter test with a case that actually exercises normalization
  (upstream Name:"" resolving to "default", user pinning to "default").
  Removes redundancy with the single-upstream-honored case and matches
  the test's claimed assertion.
- MEDIUM: Add validator test case for explicit primaryUpstreamProvider
  with no embedded auth server, locking the new rejection in.

Author: tgrunnagle

cmd/thv-operator/controllers/virtualmcpserver_controller.go

@@ -583,7 +583,52 @@ func (*VirtualMCPServerReconciler) validateAuthzUpstreamAvailable(
 	// Direct-IdP flow: no embedded AS. Cedar evaluates against identity.Claims
 	// populated by incoming OIDC middleware from the IdP token. No upstream
 	// needed; nothing to warn about. Remove any stale condition.
+	//
+	// However, an explicit primaryUpstreamProvider is meaningless in this mode
+	// — there is no upstream-token table for Cedar to look it up in — so the
+	// converter would forward a name that cannot resolve at runtime. Reject at
+	// admission for the same "fail loudly instead of denying every request"
+	// reason as the configured-AS mismatch path below.
 	if vmcp.Spec.AuthServerConfig == nil {
+		explicitProvider := ""
+		if vmcp.Spec.IncomingAuth.AuthzConfig.Inline != nil {
+			explicitProvider = vmcp.Spec.IncomingAuth.AuthzConfig.Inline.PrimaryUpstreamProvider
+		}
+		if explicitProvider != "" {
+			statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{})
+
+			message := fmt.Sprintf(
+				"spec.incomingAuth.authzConfig.inline.primaryUpstreamProvider=%q is set but "+
+					"spec.authServerConfig is not configured. The field names an upstream IDP "+
+					"on the embedded auth server, which is required for it to take effect. "+
+					"Remove primaryUpstreamProvider, or configure spec.authServerConfig with "+
+					"an upstream of that name.",
+				explicitProvider,
+			)
+
+			ctxLogger := log.FromContext(ctx)
+			ctxLogger.Info("authz primaryUpstreamProvider set without an embedded auth server; rejecting VirtualMCPServer",
+				"name", vmcp.Name,
+				"namespace", vmcp.Namespace,
+				"primaryUpstreamProvider", explicitProvider,
+				"reason", mcpv1beta1.ConditionReasonAuthzUpstreamUnknown,
+			)
+
+			statusManager.SetPhase(mcpv1beta1.VirtualMCPServerPhaseFailed)
+			statusManager.SetMessage(message)
+			statusManager.SetAuthServerConfigValidatedCondition(
+				mcpv1beta1.ConditionReasonAuthzUpstreamUnknown,
+				message,
+				metav1.ConditionFalse,
+			)
+			statusManager.SetObservedGeneration(vmcp.Generation)
+			return &SpecValidationError{
+				Message: fmt.Sprintf(
+					"authz primaryUpstreamProvider %q set without an embedded auth server",
+					explicitProvider,
+				),
+			}
+		}
 		statusManager.RemoveConditionsWithPrefix(mcpv1beta1.ConditionTypeAuthzUpstreamSelectionWarning, []string{})
 		return nil
 	}

cmd/thv-operator/controllers/virtualmcpserver_controller_test.go

@@ -3700,6 +3700,22 @@ func TestVirtualMCPServerValidateAuthzUpstreamAvailable(t *testing.T) {
 			expectedReason:  mcpv1beta1.ConditionReasonAuthzUpstreamUnknown,
 			expectedWarning: warningExpectation{expectPresent: false},
 		},
+		{
+			// Explicit PrimaryUpstreamProvider with no embedded auth server at
+			// all is rejected at admission. The field names an upstream IDP on
+			// the embedded AS — without an AS there is nothing for it to refer
+			// to, and the converter would otherwise forward an unresolvable
+			// name. Same condition reason as the upstream-mismatch case.
+			name: "explicit primary provider without embedded auth server is invalid",
+			incomingAuth: &mcpv1beta1.IncomingAuthConfig{
+				Type:        "oidc",
+				AuthzConfig: authzRefWithPrimary("okta"),
+			},
+			authServerConfig: nil,
+			expectError:      true,
+			expectedReason:   mcpv1beta1.ConditionReasonAuthzUpstreamUnknown,
+			expectedWarning:  warningExpectation{expectPresent: false},
+		},
 	}
 
 	for _, tt := range tests {

cmd/thv-operator/pkg/vmcpconfig/converter.go

@@ -203,9 +203,11 @@ func (c *Converter) convertIncomingAuth(
 		// explicitly, honor it (after normalization). Otherwise fall back to the first
 		// configured upstream — matching the SubjectProviderName precedent on the
 		// token-exchange and AWS-STS strategies. The validator at
-		// validateAuthzUpstreamAvailable rejects names that don't match any declared
-		// upstream, so by the time we reach this branch the explicit value is known
-		// to resolve to a real upstream.
+		// validateAuthzUpstreamAvailable rejects an explicit name that does not match
+		// any declared upstream, and also rejects an explicit name when no embedded
+		// auth server is configured at all, so by the time we reach this branch the
+		// value resolves to a real upstream on the embedded AS.
+		// TODO: load primaryUpstreamProvider from configMap
 		switch {
 		case vmcp.Spec.IncomingAuth.AuthzConfig.Inline != nil &&
 			vmcp.Spec.IncomingAuth.AuthzConfig.Inline.PrimaryUpstreamProvider != "":

cmd/thv-operator/pkg/vmcpconfig/converter_test.go

@@ -2021,23 +2021,29 @@ func TestConvertIncomingAuth_PrimaryUpstreamProvider(t *testing.T) {
 			expectedProvider: "github",
 		},
 		{
-			// Explicit value flows through ResolveUpstreamName for normalization
-			// parity with the auto-select branch — though for non-empty input the
-			// resolver is the identity function. Keeps both paths normalized.
-			name: "explicit primary provider is normalized via ResolveUpstreamName",
+			// Exercises the actual normalization step inside ResolveUpstreamName:
+			// the upstream is declared with Name:"" (which resolves to "default")
+			// and the user pins primaryUpstreamProvider to "default". The explicit
+			// branch must forward "default" — exercising both the explicit path
+			// and the resolver's empty-input handling. The previous "okta -> okta"
+			// case did not exercise normalization because ResolveUpstreamName is
+			// the identity function for non-empty input.
+			name: "explicit primary provider 'default' resolves to default upstream",
 			authServerConfig: &mcpv1beta1.EmbeddedAuthServerConfig{
 				Issuer: "https://authserver.example.com",
 				UpstreamProviders: []mcpv1beta1.UpstreamProviderConfig{
-					{Name: "okta", Type: mcpv1beta1.UpstreamProviderTypeOIDC},
+					{Name: "", Type: mcpv1beta1.UpstreamProviderTypeOIDC},
 				},
 			},
-			authzConfig:      authzWith("okta"),
-			expectedProvider: "okta",
+			authzConfig:      authzWith("default"),
+			expectedProvider: "default",
 		},
 		{
 			// Explicit primary provider is honored even without an embedded AS
-			// configured on the spec. The validator catches the mismatch in this
-			// case; the converter's job is only to forward the explicit choice.
+			// configured on the spec. The validator rejects this combination
+			// (covered by TestVirtualMCPServerValidateAuthzUpstreamAvailable),
+			// but the converter still forwards the explicit value as a defined
+			// contract — this case locks that contract in.
 			name:             "explicit primary provider without auth server is forwarded",
 			authServerConfig: nil,
 			authzConfig:      authzWith("okta"),