diff --git a/deployment/base/maas-controller/overlays/xks/cert-manager/kustomization.yaml b/deployment/base/maas-controller/overlays/xks/cert-manager/kustomization.yaml new file mode 100644 index 000000000..c9d0de3f3 --- /dev/null +++ b/deployment/base/maas-controller/overlays/xks/cert-manager/kustomization.yaml @@ -0,0 +1,5 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +resources: + - maas-webhook-certificate.yaml diff --git a/deployment/base/maas-controller/overlays/xks/cert-manager/maas-webhook-certificate.yaml b/deployment/base/maas-controller/overlays/xks/cert-manager/maas-webhook-certificate.yaml new file mode 100644 index 000000000..5dceb7e68 --- /dev/null +++ b/deployment/base/maas-controller/overlays/xks/cert-manager/maas-webhook-certificate.yaml @@ -0,0 +1,16 @@ +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: maas-controller-webhook-server +spec: + commonName: maas-controller-webhook-service.$(NAMESPACE).svc + dnsNames: + - maas-controller-webhook-service.$(NAMESPACE).svc + - maas-controller-webhook-service.$(NAMESPACE).svc.cluster.local + duration: 8760h # 1 year + renewBefore: 720h # 30 days + issuerRef: + name: $(ISSUER_REF_NAME) + kind: $(ISSUER_REF_KIND) + group: $(ISSUER_REF_GROUP) + secretName: maas-controller-webhook-cert diff --git a/deployment/base/maas-controller/overlays/xks/kustomization.yaml b/deployment/base/maas-controller/overlays/xks/kustomization.yaml new file mode 100644 index 000000000..95fe83a85 --- /dev/null +++ b/deployment/base/maas-controller/overlays/xks/kustomization.yaml @@ -0,0 +1,122 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +resources: + - ../../crd + - ../../rbac + - ../../manager + - ../../webhook + - cert-manager/ + # monitoring is excluded on xKS (OpenShift-specific ServiceMonitor/PodMonitor) + +labels: + - pairs: + app.kubernetes.io/name: maas-controller + app.kubernetes.io/component: models-as-a-service + app.kubernetes.io/part-of: models-as-a-service + +configMapGenerator: + - name: maas-parameters + envs: + - params.env + +generatorOptions: + disableNameSuffixHash: true + +replacements: + - source: + kind: ConfigMap + name: maas-parameters + fieldPath: data.maas-controller-image + targets: + - select: + kind: Deployment + name: maas-controller + fieldPaths: + - spec.template.spec.containers.[name=manager].image + - source: + kind: ConfigMap + name: maas-parameters + fieldPath: data.infrastructure-namespace + targets: + - select: + kind: Deployment + name: maas-controller + fieldPaths: + - spec.template.spec.containers.[name=manager].env.[name=INFRA_NAMESPACE].value + - source: + kind: ConfigMap + name: maas-parameters + fieldPath: data.gateway-namespace + targets: + - select: + kind: Deployment + name: maas-controller + fieldPaths: + - spec.template.spec.containers.[name=manager].env.[name=GATEWAY_NAMESPACE].value + +patches: + # Remove OCP service-serving cert annotation (cert-manager handles TLS on xKS) + - target: + kind: Service + name: maas-controller-webhook-service + patch: | + - op: remove + path: /metadata/annotations/service.beta.openshift.io~1serving-cert-secret-name + - op: remove + path: /metadata/annotations/service.beta.openshift.io~1inject-cabundle + + # Replace OCP CA injection with cert-manager CA injection on webhook + - target: + kind: ValidatingWebhookConfiguration + name: maas-validating-webhook-configuration + patch: | + - op: remove + path: /metadata/annotations/service.beta.openshift.io~1inject-cabundle + - op: add + path: /metadata/annotations/cert-manager.io~1inject-ca-from + value: "$(NAMESPACE)/maas-controller-webhook-server" + + # Set platform manifests path so maas-controller uses xKS tenant overlay + - target: + kind: Deployment + name: maas-controller + patch: | + - op: add + path: /spec/template/spec/containers/0/env/- + value: + name: MAAS_PLATFORM_MANIFESTS + value: /maas-api/deploy/overlays/xks + +vars: + - name: NAMESPACE + objref: + apiVersion: v1 + kind: ConfigMap + name: maas-parameters + fieldref: + fieldpath: data.namespace + - name: ISSUER_REF_NAME + objref: + apiVersion: v1 + kind: ConfigMap + name: maas-parameters + fieldref: + fieldpath: data.ISSUER_REF_NAME + - name: ISSUER_REF_KIND + objref: + apiVersion: v1 + kind: ConfigMap + name: maas-parameters + fieldref: + fieldpath: data.ISSUER_REF_KIND + - name: ISSUER_REF_GROUP + objref: + apiVersion: v1 + kind: ConfigMap + name: maas-parameters + fieldref: + fieldpath: data.ISSUER_REF_GROUP + +configurations: + - params.yaml diff --git a/deployment/base/maas-controller/overlays/xks/params.env b/deployment/base/maas-controller/overlays/xks/params.env new file mode 100644 index 000000000..fc241df48 --- /dev/null +++ b/deployment/base/maas-controller/overlays/xks/params.env @@ -0,0 +1,11 @@ +maas-api-image=quay.io/opendatahub/maas-api:odh-stable +maas-controller-image=quay.io/opendatahub/maas-controller:odh-stable +payload-processing-image=quay.io/opendatahub/odh-ai-gateway-payload-processing:odh-stable +maas-api-key-cleanup-image=registry.redhat.io/ubi9/ubi-minimal:9.7 +monitoring-namespace= +infrastructure-namespace=AUTO +gateway-namespace=redhat-ods-applications +namespace=redhat-ods-applications +ISSUER_REF_NAME=rhai-ca-issuer +ISSUER_REF_KIND=ClusterIssuer +ISSUER_REF_GROUP=cert-manager.io diff --git a/deployment/base/maas-controller/overlays/xks/params.yaml b/deployment/base/maas-controller/overlays/xks/params.yaml new file mode 100644 index 000000000..25a35b25a --- /dev/null +++ b/deployment/base/maas-controller/overlays/xks/params.yaml @@ -0,0 +1,13 @@ +varReference: + - path: spec/commonName + kind: Certificate + - path: spec/dnsNames + kind: Certificate + - path: spec/issuerRef/name + kind: Certificate + - path: spec/issuerRef/kind + kind: Certificate + - path: spec/issuerRef/group + kind: Certificate + - path: metadata/annotations + kind: ValidatingWebhookConfiguration diff --git a/deployment/base/maas-controller/rbac/clusterrole.yaml b/deployment/base/maas-controller/rbac/clusterrole.yaml index e9dd09db5..6ec07a8b3 100644 --- a/deployment/base/maas-controller/rbac/clusterrole.yaml +++ b/deployment/base/maas-controller/rbac/clusterrole.yaml @@ -118,6 +118,17 @@ rules: - create - delete - get +- apiGroups: + - cert-manager.io + resources: + - certificates + verbs: + - create + - delete + - get + - list + - patch + - watch - apiGroups: - config.openshift.io resources: diff --git a/deployment/base/networking/maas/maas-gateway-api.yaml b/deployment/base/networking/maas/maas-gateway-api.yaml index d67a63298..bb4ac8227 100644 --- a/deployment/base/networking/maas/maas-gateway-api.yaml +++ b/deployment/base/networking/maas/maas-gateway-api.yaml @@ -21,7 +21,7 @@ spec: protocol: HTTPS allowedRoutes: namespaces: - from: All + from: Same tls: certificateRefs: - group: '' diff --git a/deployment/base/payload-processing/manager/envoy-filter.yaml b/deployment/base/payload-processing/manager/envoy-filter.yaml index 37c384a82..bf560220d 100644 --- a/deployment/base/payload-processing/manager/envoy-filter.yaml +++ b/deployment/base/payload-processing/manager/envoy-filter.yaml @@ -4,10 +4,19 @@ metadata: name: payload-processing namespace: openshift-ingress spec: - targetRefs: - - group: gateway.networking.k8s.io - kind: Gateway - name: maas-default-gateway + # Must run AFTER Kuadrant's EnvoyFilter (default priority 0), which INSERT_BEFOREs + # envoy.filters.http.wasm relative to the router. Without a positive priority, istiod + # applies this resource first (same priority 0, older creationTimestamp) and the RHCL + # wasm anchors miss — ext_proc never enters the gateway filter chain (404 NR on + # body-routed /v1/*). Multi-tenant gateways are especially affected: their + # payload-processing-* EFs are often created before Kuadrant's per-gateway EF. + # ODH WasmPlugin anchors are unaffected: WasmPlugin is in the base chain before + # any EnvoyFilter patches. + # See: https://istio.io/latest/docs/reference/config/networking/envoy-filter/#EnvoyFilter + priority: 10 + workloadSelector: + labels: + gateway.networking.k8s.io/gateway-name: maas-default-gateway configPatches: # Stage 1: must run BEFORE the WasmPlugin so auth sees X-Gateway-Model-Name. - applyTo: HTTP_FILTER @@ -70,6 +79,7 @@ spec: message_timeout: 300s # RHCL 1.4 injects auth via envoy.filters.http.wasm (no WasmPlugin CR). Only one anchor # pair matches per cluster: WasmPlugin on ODH/community Kuadrant, wasm filter on RHCL 1.4. + # These patches require priority > Kuadrant's EF (see spec.priority above). - applyTo: HTTP_FILTER match: context: GATEWAY diff --git a/deployment/base/payload-processing/manager/plugins-configmap.yaml b/deployment/base/payload-processing/manager/plugins-configmap.yaml index 09e5680d7..0672159d0 100644 --- a/deployment/base/payload-processing/manager/plugins-configmap.yaml +++ b/deployment/base/payload-processing/manager/plugins-configmap.yaml @@ -3,6 +3,11 @@ kind: ConfigMap metadata: name: payload-processing-plugins namespace: openshift-ingress + # Do NOT set opendatahub.io/managed=false here: PostRender drops any resource that + # already carries that annotation, which would prevent first-time creation. + # The tenant reconciler stamps managed=false after bootstrap/migrate so later + # SSA leaves this ConfigMap alone (see ApplyRendered). Set managed=true on the + # live ConfigMap to opt back into continuous reconciler management. data: custom-pre-processing-ipp-config.yaml: | apiVersion: llm-d.ai/v1alpha1 @@ -39,5 +44,10 @@ data: - pluginRef: model-provider-resolver - pluginRef: api-translation - pluginRef: apikey-injection - response: - - pluginRef: api-translation + # Response api-translation is off by default so SSE streaming for + # internal/OpenAI-compatible models is not held by a response processor. + # Re-enable for providers that need response rewrite (e.g. Anthropic): + # response: + # - pluginRef: api-translation + # See docs/content/install/external-model-setup.md (IPP response translation). + response: [] diff --git a/deployment/components/observability/observability/dashboards/usage-dashboard.yaml b/deployment/components/observability/observability/dashboards/usage-dashboard.yaml index 364defb80..8424713c8 100644 --- a/deployment/components/observability/observability/dashboards/usage-dashboard.yaml +++ b/deployment/components/observability/observability/dashboards/usage-dashboard.yaml @@ -174,6 +174,7 @@ spec: spec: display: name: Token consumption chart + # [2h] matches the Loki dashboards' step for consistent cross-dashboard granularity. description: >- Tokens over time by model or subscription. View by drives chart sum by (${view_by:raw}). @@ -209,7 +210,7 @@ spec: query: >- round(sum by (${view_by:raw}) (increase(authorized_hits_total{user!="", user=~"$user", subscription=~"$subscription", - model=~"$model"}[30m]))) + model=~"$model"}[2h]))) tokenConsumptionByUser: kind: Panel spec: diff --git a/deployment/components/observability/usage-logs/envoy-otel-access-log.yaml b/deployment/components/observability/usage-logs/envoy-otel-access-log.yaml index 35726badd..95c7f00d4 100644 --- a/deployment/components/observability/usage-logs/envoy-otel-access-log.yaml +++ b/deployment/components/observability/usage-logs/envoy-otel-access-log.yaml @@ -37,10 +37,9 @@ metadata: app.kubernetes.io/part-of: maas-observability app.kubernetes.io/managed-by: maas-controller spec: - targetRefs: - - group: gateway.networking.k8s.io - kind: Gateway - name: maas-default-gateway + workloadSelector: + labels: + gateway.networking.k8s.io/gateway-name: maas-default-gateway configPatches: # ── Cluster: gRPC endpoint to OTel Collector ────────────────────────── diff --git a/deployment/components/observability/usage-logs/usage-logs-admin-dashboard.yaml b/deployment/components/observability/usage-logs/usage-logs-admin-dashboard.yaml index ecbaf412c..470697f16 100644 --- a/deployment/components/observability/usage-logs/usage-logs-admin-dashboard.yaml +++ b/deployment/components/observability/usage-logs/usage-logs-admin-dashboard.yaml @@ -1,8 +1,9 @@ # Admin usage dashboard — queries Loki structured logs via LogQL. # Uses $__range to bind LogQL windows to the native time picker. -# LokiLabelValuesVariable for model/subscription dropdowns (COO 1.5+). -# User filter uses TextVariable (regex input) to avoid Loki cardinality issues -# from indexing high-cardinality user_id as a label. Admins enter regex patterns. +# All three dropdowns (user, subscription, model) follow the dashboard time +# picker. User filter uses LokiLogQLVariable with [$__range] — auto-populates +# from structured metadata via count by (user_id), bypassing /label/values +# (which requires stream labels). Subscription/model use LokiLabelValuesVariable. # customAllValue: ".*" on ListVariables ensures "All" matches entries with # absent labels (e.g. subscription when capturing is disabled). Do not remove. apiVersion: perses.dev/v1alpha1 @@ -21,13 +22,24 @@ spec: rate limiting, and per-user breakdown across all users and subscriptions. duration: 1h variables: - - kind: TextVariable + - kind: ListVariable spec: name: user display: - name: "User (regex)" - description: "Filter by user ID. Use regex: 'user-1' (single), 'user-1|user-2' (multiple), '.*' (all)" - defaultValue: ".*" + name: "User" + description: "Filter by user. Populated from the selected time range." + allowMultiple: true + allowAllValue: true + customAllValue: ".*" + defaultValue: "$__all" + plugin: + kind: LokiLogQLVariable + spec: + datasource: + kind: LokiDatasource + name: usage-logs-all + expr: 'count by (user_id) (count_over_time({service_name="models-as-a-service"} | user_id!="" | user_id!="-" | keep user_id [$__range]))' + labelName: user_id - kind: ListVariable spec: name: subscription @@ -240,11 +252,11 @@ spec: spec: display: name: "Token consumption chart" + # [2h] is the minimal stable step. Loki split_queries_by_interval is 30m; 1h still produces + # gaps at chunk boundaries. 2h is the lowest value that returns consistent results. description: >- Tokens from successful requests (2xx) over time by model or subscription. - **View by** drives chart ``sum by (${view_by:raw})``. Table uses hardcoded ``sum by (model, subscription)``. - LogQL ``[30m]`` is the range window for ``sum_over_time`` (Perses time-series step). - Visual: OpenShift Perses pattern — ``line`` + ``areaOpacity: 1`` + ``stack: all``. + **View by** drives the chart grouping. plugin: kind: TimeSeriesChart spec: @@ -277,7 +289,7 @@ spec: sum by (${view_by:raw}) (sum_over_time({service_name="models-as-a-service", subscription=~"$subscription", model=~"$model", response_type="hit"} | user_id=~"$user" - | unwrap tokens_total [30m])) + | unwrap tokens_total [2h])) tokenConsumptionTable: kind: Panel diff --git a/deployment/components/observability/usage-logs/usage-logs-dashboard.yaml b/deployment/components/observability/usage-logs/usage-logs-dashboard.yaml index 13061efa6..62bf56ae5 100644 --- a/deployment/components/observability/usage-logs/usage-logs-dashboard.yaml +++ b/deployment/components/observability/usage-logs/usage-logs-dashboard.yaml @@ -204,6 +204,8 @@ spec: spec: display: name: "Token consumption chart" + # [2h] is the minimal stable step. Loki split_queries_by_interval is 30m; 1h still produces + # gaps at chunk boundaries. 2h is the lowest value that returns consistent results. description: >- Tokens from successful requests (2xx) over time by model or subscription. plugin: @@ -237,7 +239,7 @@ spec: query: >- sum by (${view_by:raw}) (sum_over_time({service_name="models-as-a-service", subscription=~"$subscription", model=~"$model", response_type="hit"} - | unwrap tokens_total [30m])) + | unwrap tokens_total [2h])) tokenConsumptionTable: kind: Panel diff --git a/docs/content/advanced-administration/controller-performance-tuning.md b/docs/content/advanced-administration/controller-performance-tuning.md new file mode 100644 index 000000000..d18a801eb --- /dev/null +++ b/docs/content/advanced-administration/controller-performance-tuning.md @@ -0,0 +1,51 @@ +# Controller Performance Tuning + +The maas-controller processes subscription and auth policy reconciliation within a single leader pod. By default, reconciliation is parallelized across 5 concurrent workers. This page describes how to tune concurrency for large-scale deployments. + +## `--max-concurrent-reconciles` + +Controls the number of concurrent reconciliation goroutines for the MaaSSubscription and MaaSAuthPolicy controllers. Other controllers (AITenant, MaaSModelRef, Tenant, Lifecycle) always use 1 to avoid conflicts on shared resources. + +| Parameter | Value | +|---|---| +| Flag | `--max-concurrent-reconciles` | +| Default | 5 | +| Range | 1–10 | +| Applies to | MaaSSubscription, MaaSAuthPolicy controllers | + +## Benchmark Results + +Tested on RHOAI 3.5 cluster with 300 MaaSSubscriptions created simultaneously: + +| MaxConcurrentReconciles | Time to all Active | Speedup | +|---|---|---| +| 1 | 236s | baseline | +| 5 (default) | 60s | 3.9x | +| 10 | 67s | 3.5x | + +At default pod resource limits, values above 5 show diminishing returns due to API server contention. + +## Scaling Guidance + +| Deployment Size | Recommended Value | Resource Changes | +|---|---|---| +| Small (< 100 subscriptions) | 5 (default) | None | +| Medium (100–500 subscriptions) | 5 | None | +| Large (500+ subscriptions) | 5–10 | Increase controller CPU and memory | + +To increase the value beyond 5, update the controller deployment args and scale resources: + +```bash +# Increase concurrency +oc patch deploy maas-controller -n --type=json \ + -p '[{"op":"add","path":"/spec/template/spec/containers/0/args/-","value":"--max-concurrent-reconciles=10"}]' + +# Scale resources to support higher concurrency +oc patch deploy maas-controller -n --type=json \ + -p '[{"op":"replace","path":"/spec/template/spec/containers/0/resources/limits/memory","value":"512Mi"}, + {"op":"replace","path":"/spec/template/spec/containers/0/resources/limits/cpu","value":"1"}]' +``` + +## Leader Election + +The maas-controller uses Kubernetes leader election (`--leader-elect`). Only the leader pod runs reconcilers — additional replicas are standby for high-availability failover, not parallel processing. `MaxConcurrentReconciles` is the mechanism for parallelizing reconciliation within the single leader. diff --git a/docs/content/concepts/auth-modes.md b/docs/content/concepts/auth-modes.md index d5e44c442..976063fe3 100644 --- a/docs/content/concepts/auth-modes.md +++ b/docs/content/concepts/auth-modes.md @@ -58,7 +58,7 @@ spec: | `clientId` | OAuth2 client ID. Tokens must have `azp` claim matching this value. | | `ttl` | JWKS cache duration in seconds (default: 300, minimum: 30). | -For unmanaged tenants (not backed by an AITenant), configure OIDC on the Tenant CR directly: +For unmanaged tenants (legacy `Tenant` CR, not backed by an AITenant -- deprecated, will be removed in a future release): ```yaml apiVersion: maas.opendatahub.io/v1alpha1 @@ -108,9 +108,9 @@ For Mode 2 (standalone OIDC), the IdP must: MaaS does **not** perform OAuth2 client authentication (no `client_secret`). It validates bearer tokens only. Client authentication and secret management are the IdP's responsibility. -## Field Alignment (Tenant vs AITenant) +## Field Alignment (Legacy Tenant vs AITenant) -Configuration fields align between Tenant CR and AITenant CR: +Configuration fields align between the legacy Tenant CR and the current AITenant CR: | Field | Tenant CR path | AITenant CR path | Semantics | |-------|---------------|-------------------|-----------| @@ -126,5 +126,5 @@ Configuration fields align between Tenant CR and AITenant CR: - [External OIDC Configuration](../advanced-administration/external-oidc.md) — JWKS cache TTL, monitoring, security controls - [API Key Authentication](api-key-authentication.md) — API key creation flow and architecture - [Authentication Internals](../architecture-internals/authentication-internals.md) — Gateway identity pipeline details -- [Tenant CRD Reference](../reference/crds/tenant.md) — Full Tenant spec including OIDC fields +- [MaasTenantConfig CRD Reference](../reference/crds/tenant.md) — Runtime tenant configuration (API keys, telemetry) - [AITenant CRD Reference](../reference/crds/ai-tenant.md) — AITenant spec including OIDC fields diff --git a/docs/content/configuration-and-management/model-setup.md b/docs/content/configuration-and-management/model-setup.md index e8e574998..560aaafa7 100644 --- a/docs/content/configuration-and-management/model-setup.md +++ b/docs/content/configuration-and-management/model-setup.md @@ -59,6 +59,9 @@ To enable MaaS policies for an LLMInferenceService: Without the gateway reference, the model uses the standard gateway and MaaS policies do not apply. +!!! tip "Multi-tenant deployments" + For models that serve a non-default tenant, set `spec.tenantRef` on the MaaSModelRef to the AITenant name. This tells the controller to resolve the gateway from the named AITenant instead of using namespace-based inference. See [Multi-Tenant Setup — Configure Models](../install/multi-tenant-setup.md#5-configure-models). + --- ## External Models diff --git a/docs/content/configuration-and-management/tenant-rbac.md b/docs/content/configuration-and-management/tenant-rbac.md index 5e5f1ec6a..dc91b6b60 100644 --- a/docs/content/configuration-and-management/tenant-rbac.md +++ b/docs/content/configuration-and-management/tenant-rbac.md @@ -25,7 +25,7 @@ The tenant-admin Role in the tenant namespace grants: - `get`, `list`, `watch`, `create`, `update`, `patch`, and `delete` on `MaaSAuthPolicy` - `get`, `list`, `watch`, `create`, `update`, `patch`, and `delete` on `MaaSSubscription` -- `get`, `update`, and `patch` on `Tenant/default-tenant` +- `get`, `update`, and `patch` on `MaasTenantConfig/default-tenant` - `get`, `list`, and `watch` on `MaaSModelRef` The object-admin Role grants `get` on the specific `AITenant` object in the AITenant infrastructure namespace. Bind it when tenant administrators or dashboards need to read tenant bootstrap status. diff --git a/docs/content/install/external-model-setup.md b/docs/content/install/external-model-setup.md index 0d406da5e..dbf72aac3 100644 --- a/docs/content/install/external-model-setup.md +++ b/docs/content/install/external-model-setup.md @@ -7,12 +7,10 @@ This guide walks through deploying an external AI/ML model (e.g., OpenAI, Anthro ## Multi-Tenant Limitation -!!! danger "External models are not supported in multi-tenant deployments" - When multiple AITenants are deployed, each tenant gets a dedicated Gateway and Inference Payload Processor (IPP) stack. The ExternalModel reconciler is not tenant-aware — it always creates HTTPRoutes pointing to the default tenant's gateway (`--gateway-name` / `--gateway-namespace` controller flags). Each tenant's IPP EnvoyFilter targets its own gateway, which conflicts with the ExternalModel HTTPRoute's static gateway reference. +!!! warning "External models are only supported for the default tenant" + In multi-tenant deployments, external models work for the **default tenant only**. Non-default tenant IPP (Inference Payload Processor) instances have the ExternalModel controller disabled to prevent HTTPRoute conflicts between tenants. - **Result:** External models do not work for any tenant — including the default tenant — when multiple IPP instances are running. - - External models are only supported in single-tenant deployments. This limitation is tracked for a future fix. + External model support for non-default tenants is planned for a future release. ## Prerequisites @@ -45,6 +43,33 @@ IPP is required for external models — it injects the provider API key and tran MaaS deploys the payload-processing component from the [`ai-gateway-payload-processing`](https://github.com/opendatahub-io/ai-gateway-payload-processing) repository. For detailed configuration and usage, see that project's documentation. +### IPP response translation (opt-in) + +By default, the `api-translation` plugin runs on the **request** path only. Response-side `api-translation` is off so SSE streaming for internal and OpenAI-compatible models is not held by a response processor. + +Providers that rewrite responses (notably Anthropic Messages ↔ OpenAI) need response translation enabled manually on the plugins ConfigMap. After MaaS creates `payload-processing-plugins`, the controller stamps `opendatahub.io/managed: "false"` and then leaves the ConfigMap alone so your edits stick. + +```bash +GATEWAY_NAMESPACE="${GATEWAY_NAMESPACE:-openshift-ingress}" + +# 1. Edit the live ConfigMap — under profiles[0].plugins.response add: +# - pluginRef: api-translation +kubectl edit configmap payload-processing-plugins -n "${GATEWAY_NAMESPACE}" + +# 2. Reload IPP +kubectl rollout restart deployment/payload-processing -n "${GATEWAY_NAMESPACE}" +``` + +To reset the ConfigMap to product defaults once, remove the opt-out annotation (or set `opendatahub.io/managed=true` for continuous reconciler management), wait for reconcile, then optionally set `opendatahub.io/managed=false` again after editing: + +```bash +# One-shot reset to defaults (controller re-applies, then stamps managed=false again) +kubectl annotate configmap payload-processing-plugins -n "${GATEWAY_NAMESPACE}" opendatahub.io/managed- + +# Or keep the reconciler owning the ConfigMap continuously: +kubectl annotate configmap payload-processing-plugins -n "${GATEWAY_NAMESPACE}" opendatahub.io/managed=true --overwrite +``` + !!! note If MaaS was deployed via the MaasTenantConfig CR (standard RHOAI path), IPP is already deployed as a subcomponent. Verify with: diff --git a/docs/content/install/multi-tenant-setup.md b/docs/content/install/multi-tenant-setup.md index 5cb714547..9e2dcef1b 100644 --- a/docs/content/install/multi-tenant-setup.md +++ b/docs/content/install/multi-tenant-setup.md @@ -15,7 +15,12 @@ Before creating additional tenants: Each AITenant requires a dedicated Gateway. Gateways cannot be shared between AITenants. -Get the cluster domain and create the Gateway: +Get the cluster domain and create the Gateway. + +The Gateway uses a per-tenant label selector for `allowedRoutes` so only explicitly +labelled namespaces can attach HTTPRoutes — more secure than `from: All`. Label each +namespace that needs access (infra namespace, model namespaces) before or after Gateway +creation: ```bash TENANT_NAME="red-team" @@ -23,6 +28,14 @@ CLUSTER_DOMAIN=$(oc get ingresses.config.openshift.io cluster -o jsonpath='{.spe GATEWAY_HOSTNAME="${TENANT_NAME}-maas.${CLUSTER_DOMAIN}" GATEWAY_NAMESPACE="openshift-ingress" CERT_NAME="router-certs-default" +GATEWAY_ACCESS_LABEL="maas.opendatahub.io/gateway-access-${TENANT_NAME}" + +# Label the namespaces that need to attach HTTPRoutes to this tenant Gateway. +# At minimum: the infrastructure namespace where maas-api is deployed. +# Also label any model namespaces (e.g. llm) where LLMInferenceServices run. +INFRA_NS="odh-ai-gateway-infra" # adjust if using RHOAI (redhat-ai-gateway-infra) +oc label namespace "${INFRA_NS}" "${GATEWAY_ACCESS_LABEL}=true" --overwrite +# oc label namespace llm "${GATEWAY_ACCESS_LABEL}=true" --overwrite # repeat for model namespaces cat <` - `maas.opendatahub.io/managed-by-aitenant=true` -Verify the Tenant CR exists: +Verify the MaasTenantConfig CR exists: ```bash -oc get tenant default-tenant -n ai-tenant-${TENANT_NAME} +oc get maastenantconfig default-tenant -n ai-tenant-${TENANT_NAME} ``` Verify the maas-api deployment is running in the infrastructure namespace: @@ -199,25 +221,34 @@ See [Tenant RBAC](../configuration-and-management/tenant-rbac.md) for examples w ## 5. Configure Models -Create MaaS resources in the tenant namespace to expose models: +Create the MaaSModelRef in the **model namespace** (co-located with the backend resource) and use `tenantRef` to associate it with the tenant's gateway. MaaSAuthPolicy and MaaSSubscription must be created in the **tenant namespace** (where the MaasTenantConfig CR lives). ```bash TENANT_NS="ai-tenant-${TENANT_NAME}" +MODEL_NS="llm" # namespace where the LLMInferenceService runs -# Create a MaaSModelRef +# The model namespace must carry the tenant Gateway's access label +# so the controller-generated HTTPRoute can attach. +oc label namespace "${MODEL_NS}" "maas.opendatahub.io/gateway-access-${TENANT_NAME}=true" --overwrite + +# Create a MaaSModelRef in the model namespace. +# tenantRef tells the controller to resolve the gateway from this AITenant +# instead of using namespace-based inference (which defaults to the default tenant). cat <` - `maas.opendatahub.io/managed-by-aitenant: "true"` -Verify the Tenant CR: +Verify the MaasTenantConfig CR: ```bash -oc get tenant default-tenant -n ${TENANT_NS} -o yaml +oc get maastenantconfig default-tenant -n ${TENANT_NS} -o yaml ``` Expected: `status.phase` is `Active`. @@ -122,8 +122,8 @@ Each tenant's maas-api instance serves only its own tenant's data. API keys are echo "=== AITenant ===" oc get aitenant ${TENANT_NAME} -n ai-tenants -echo "=== Tenant CR ===" -oc get tenant default-tenant -n ${TENANT_NS} +echo "=== MaasTenantConfig CR ===" +oc get maastenantconfig default-tenant -n ${TENANT_NS} echo "=== maas-api ===" oc get deployment maas-api-${TENANT_NAME} -n ${INFRA_NS} @@ -175,7 +175,7 @@ gateway openshift-ingress/red-team is already in use by AITenant ai-tenants/othe ### MaaSSubscription rejected -MaaSSubscription and MaaSAuthPolicy must be created in a namespace that contains a `Tenant` CR. Wait for the AITenant controller to create the Tenant CR before creating these resources. +MaaSSubscription and MaaSAuthPolicy must be created in a namespace that contains a `MaasTenantConfig` CR. Wait for the AITenant controller to create the MaasTenantConfig before creating these resources. ## See Also diff --git a/docs/content/reference/crds/maas-model-ref.md b/docs/content/reference/crds/maas-model-ref.md index 042526550..0be9e777d 100644 --- a/docs/content/reference/crds/maas-model-ref.md +++ b/docs/content/reference/crds/maas-model-ref.md @@ -12,6 +12,7 @@ Identifies an AI/ML model for the MaaS platform. The backend may be on-cluster ( |-------|------|----------|-------------| | modelRef | ModelReference | Yes | Reference to the model backend (kind and name) | | endpointOverride | string | No | Optional override for the endpoint URL. See [Endpoint Override](#endpoint-override) below. | +| tenantRef | string | No | Name of the AITenant this model belongs to. When omitted, the model is assigned to the default tenant. See [Multi-Tenant Models](#multi-tenant-models) below. | ### ModelReference @@ -102,6 +103,34 @@ The override does not bypass backend validation. The controller still checks tha --- +## Multi-Tenant Models + +By default, the controller resolves the gateway for a MaaSModelRef using namespace-based inference — it looks for a MaasTenantConfig in the model's namespace and uses the associated default-tenant gateway. + +In multi-tenant deployments, models often live in a shared namespace (e.g. `llm`) but need to route through a specific tenant's gateway. Set `spec.tenantRef` to the name of the AITenant. The controller looks up the AITenant in the AITenant namespace and uses its gateway directly. + +A validating webhook rejects `tenantRef` values that do not match an existing AITenant. The value must use lowercase alphanumeric characters and hyphens (matching the CRD pattern `^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) and be no longer than 253 characters. + +**Example:** +```yaml +apiVersion: maas.opendatahub.io/v1alpha1 +kind: MaaSModelRef +metadata: + name: granite-7b + namespace: llm +spec: + modelRef: + kind: LLMInferenceService + name: granite-7b-instruct + tenantRef: red-team +``` + +The resolved tenant appears in `status.resolvedTenantRef`. When `tenantRef` is removed or left empty, the field is cleared and the controller reverts to namespace-based resolution. + +For step-by-step instructions, see [Multi-Tenant Setup — Configure Models](../../install/multi-tenant-setup.md#5-configure-models). + +--- + ## Status ### MaaSModelRefStatus @@ -115,6 +144,7 @@ The override does not bypass backend validation. The controller still checks tha | httpRouteGatewayName | string | Name of the Gateway that the HTTPRoute references | | httpRouteGatewayNamespace | string | Namespace of the Gateway that the HTTPRoute references | | httpRouteHostnames | []string | Hostnames configured on the HTTPRoute | +| resolvedTenantRef | string | The explicitly selected AITenant from `spec.tenantRef`; empty when `tenantRef` is omitted and namespace-based resolution is used. | | conditions | []Condition | Latest observations of the model's state | --- diff --git a/docs/content/release-notes/index.md b/docs/content/release-notes/index.md index 134f9b5dc..78c2c6e0d 100644 --- a/docs/content/release-notes/index.md +++ b/docs/content/release-notes/index.md @@ -8,6 +8,7 @@ This table maps each supported Red Hat OpenShift AI (RHOAI) release to the corre | RHOAI Version | MaaS Version | RHOAI Image Tag | Status | Notes | |---------------|--------------|-----------------|--------|-------| +| 3.5 | v0.2.1 | `v3.5` | GA | Multi-tenancy; body-based routing; xKS support; see [Upgrade Guide](../migration/upgrade-to-3.5.md) | | 3.4 | v0.1.1 | `v3.4` | GA | Subscription-based access; `Tenant` CR; see [Upgrade Guide](../migration/upgrade-to-3.4.md) | | 3.3 | v0.0.2 | `v3.3` | Tech Preview | `ModelsAsService` CR added to DSC; operator-managed deployment | | 3.2 | v0.0.2 | `v3.2` | Tech Preview | Tier-based access; standalone deploy (`modelsAsService` not in DSC schema) | @@ -21,7 +22,108 @@ For dependency version requirements (OCP, Kuadrant/RHCL, Gateway API), see [Vers --- -## v0.1.2 +## v0.2.1 + +**Release Date:** TBD + +### Breaking Changes + +**`X-MaaS-Tenant` header eliminated (RHOAIENG-70517)** +- The `X-MaaS-Tenant` header is no longer sent or expected. Tenant identity is now derived from the `AITenant` CR and namespace context. Clients and middleware that depend on this header must be updated. + +**AITenant automatic RBAC bindings removed** +- `AITenant.spec.rbac` is deprecated and ignored. Existing manifests that still include the field remain schema-valid, but the controller no longer creates RoleBindings from it. The controller still creates tenant-admin Roles; platform administrators must create standard Kubernetes RoleBindings to grant access. See [Tenant RBAC](../configuration-and-management/tenant-rbac.md). + +**Tenant configuration migrated to MaasTenantConfig** +- MaaS runtime settings (`apiKeys`, `telemetry`) previously in the legacy `Tenant` CR are now managed via the namespace-scoped `MaasTenantConfig` CR. OIDC and gateway context moved to the owning `AITenant`. Existing `Tenant.spec` fields are copied to `MaasTenantConfig/default-tenant` automatically during the migration grace window. + +**Infrastructure namespace separation** +- MaaS infrastructure resources (secrets, config) now reside in a dedicated infrastructure namespace rather than the controller namespace. Existing deployments are migrated automatically. + +### New Features + +**Body-based routing (BBR)** + +- Models can now be selected via the `model` field in the request body (OpenAI-compatible format) in addition to URL path routing. Enables standard OpenAI SDK compatibility without path manipulation. +- Canonical BBR model ID surfaced in `GET /v1/models` and `MaaSModelRef.status`. +- Model-provider-resolver and maas-headers-guard added to the IPP pipeline for BBR support. + +**Multi-tenancy enhancements** + +- Default `AITenant` bootstrapped automatically for single-tenant deployments. +- Any MaaS tenant can now be removed (not just non-default tenants). +- Validating webhook prevents multiple `AITenant` CRs from claiming the same namespace. +- Per-tenant IPP (Inference Payload Processing) stacks deployed for each `AITenant` gateway. +- Configurable `AITenant` deletion timeout with force-remove finalizer. +- `GET /v1/tenants` endpoint for gateway discovery. + +**Observability** + +- OTel Collector deployment for usage log collection. +- EnvoyFilter for OTel structured usage logging with model and tenant context. +- Logs-based usage dashboards and tenant-specific dashboard panels. +- Tenant-level metrics, tracing, and logging. +- Perses dashboards deployed via `maas-controller`. + +**Controller self-teardown** + +- `maas-controller` supports clean uninstallation — removes managed resources while preserving tenant namespaces. + +**Non-OpenShift Kubernetes (xKS) support** + +- New `xKS` overlay enables MaaS deployment on vanilla Kubernetes clusters. +- OpenShift-only watchers are skipped on xKS to prevent cache-sync timeouts. + +**Security hardening** + +- Container hardening: `readOnlyRootFilesystem`, `seccompProfile` (FIND-007). +- Request body size limit to prevent OOM (FIND-011). +- Startup fails on missing gateway host; non-HTTPS probe URLs rejected (FIND-010). +- Debug CORS restricted to `http://localhost` only (FIND-Debug-CORS). +- Database credentials removed from error messages (FIND-006). +- `X-MaaS-Username` and `X-MaaS-Group` added to sensitive headers (FIND-014). +- SA token automount disabled on cleanup CronJob (FIND-015). +- `X-MaaS-Subscription` header ignored for non-API-key requests (FIND-009). +- Username hashed/redacted in logs. +- GitHub Actions pinned to immutable commit SHAs. +- `govulncheck` added for maas-api and maas-controller. + +**Additional features** + +- OpenShift cluster TLS profiles honored for gateway and controller TLS configuration. +- OIDC JWKS cache TTL configurable via `Tenant` CR and wired to Authorino. +- `maas-api` and `payload-processing` replica count configurable via `Tenant` annotation. +- API key display name exposed in auth identity. +- Unauthorized models filtered from `GET /subscriptions` response. +- `stream_options.include_usage` enforced in IPP pipeline. +- KServe upgraded to v0.19.0 with model-based routing support. +- New AI Gateway base manifest entry point for modularized deployment. +- Configurable Limitador scrape interval. +- API key update debouncing. + +### Key Fixes + +- **CVE-2026-33815 / CVE-2026-33816:** pgx memory-safety and SQL injection fixes. +- Prevent crash-loop when Kuadrant or KServe CRDs are not installed. +- Preserve MaaS traffic during RHOAI 3.5 upgrades. +- Require both `Accepted` and `Enforced` conditions for gateway `AuthPolicy` readiness. +- Scope gateway `deny-all` auth to model inference paths only; exempt `/v1/subscriptions` and `/v1/api-keys`. +- Resolve body-routed model names in gateway `AuthPolicy` and subscription validation. +- Gracefully handle empty `monitoring-namespace` configuration. +- Scope Secret informer cache to infrastructure namespace. +- Parse Authorino bracket-wrapped groups header format. +- Return empty list (not error) from management endpoints when no auth context present. + +### Known Limitations + +- **Token rate limits for non-OpenAI API formats:** Token-based rate limiting counts tokens only for OpenAI-compatible request/response formats. Models using other API formats (e.g., Anthropic Messages API) are not metered. See [Token Rate Limiting](../configuration-and-management/quota-and-access-configuration.md). +- **External models in multi-tenant deployments:** External models are not yet fully supported in multi-tenant configurations. See [External Model Setup](../install/external-model-setup.md). + +[Full Changelog](https://github.com/opendatahub-io/models-as-a-service/compare/v0.2.0...v0.2.1) + +--- + +## v0.2.0 **Release Date:** TBD @@ -33,7 +135,6 @@ For dependency version requirements (OCP, Kuadrant/RHCL, Gateway API), see [Vers - `status.authPolicies` now references `maas-gateway-auth / openshift-ingress` instead of per-model policy names. - New admission webhooks (`failurePolicy=Ignore`) validate that `MaaSAuthPolicy` and `MaaSSubscription` are created in namespaces that contain a `MaasTenantConfig` CR. - `AITenant` created outside the configured `--aitenant-namespace` are now rejected at admission instead of being accepted and later marked `Failed/InvalidPlacement` by the controller. -- `AITenant.spec.rbac` is deprecated and ignored. Existing manifests that still include it remain schema-valid, but the controller no longer creates RoleBindings from it. The controller still creates tenant-admin Roles, and platform administrators must create standard Kubernetes RoleBindings to grant access. See [Tenant RBAC](../configuration-and-management/tenant-rbac.md). - **Minimum Kuadrant version:** v1.4.2 or later required for `spec.defaults.rules` support. - **End-user auth behavior is unchanged** — valid API key + active subscription + allowed group still returns `200`. @@ -52,6 +153,8 @@ For dependency version requirements (OCP, Kuadrant/RHCL, Gateway API), see [Vers - **`tenant-gateway-isolation` rule is a stub.** The gateway policy includes an always-allow placeholder for multi-gateway tenant isolation. This will be replaced with a real hostname check in a future release. +[Full Changelog](https://github.com/opendatahub-io/models-as-a-service/compare/v0.1.1...v0.2.0) + --- ## v0.1.1 diff --git a/docs/mkdocs.yml b/docs/mkdocs.yml index fb6ca5a65..c08620a82 100644 --- a/docs/mkdocs.yml +++ b/docs/mkdocs.yml @@ -99,6 +99,7 @@ nav: - Limitador Persistence: advanced-administration/limitador-persistence.md - Authorino Caching: configuration-and-management/authorino-caching.md - External OIDC: advanced-administration/external-oidc.md + - Controller Performance Tuning: advanced-administration/controller-performance-tuning.md - Observability: - Overview: observability/index.md - Setup: observability/setup.md diff --git a/maas-api/deploy/overlays/xks/certificate.yaml b/maas-api/deploy/overlays/xks/certificate.yaml new file mode 100644 index 000000000..04125a265 --- /dev/null +++ b/maas-api/deploy/overlays/xks/certificate.yaml @@ -0,0 +1,15 @@ +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: maas-api-serving-cert +spec: + secretName: maas-api-serving-cert + issuerRef: + name: rhai-ca-issuer + kind: ClusterIssuer + group: cert-manager.io + duration: 2160h # 90 days + renewBefore: 360h # 15 days + dnsNames: + - "maas-api.opendatahub.svc" + - "maas-api.opendatahub.svc.cluster.local" diff --git a/maas-api/deploy/overlays/xks/kustomization.yaml b/maas-api/deploy/overlays/xks/kustomization.yaml new file mode 100644 index 000000000..9246ac073 --- /dev/null +++ b/maas-api/deploy/overlays/xks/kustomization.yaml @@ -0,0 +1,72 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization + +metadata: + name: maas-api-xks + +# xKS overlay: uses cert-manager for TLS instead of OCP service-serving-certs. +# On xKS, cert-manager creates the serving cert and the cloud-manager injects the CA. +resources: + - ../../../../deployment/base/maas-api/overlays/tls + - ../../../../deployment/base/maas-controller/policies + - ../../../../deployment/base/payload-processing/default + - certificate.yaml + +namespace: opendatahub + +patches: + # Replace OCP service-ca ConfigMap with an emptyDir (CA is injected by cert-manager + # via the trust-manager or mounted from the cluster CA bundle) + - target: + kind: Deployment + name: maas-api + patch: | + - op: replace + path: /spec/template/spec/volumes/1 + value: + name: openshift-service-ca + secret: + secretName: opendatahub-ca + items: + - key: tls.crt + path: service-ca.crt + optional: true + + # Remove OCP service-ca annotation (cert-manager handles cert creation) + - target: + kind: Service + name: maas-api + patch: | + - op: remove + path: /metadata/annotations/service.beta.openshift.io~1serving-cert-secret-name + + # Remove OCP CA bundle injection annotation from ConfigMap + - target: + kind: ConfigMap + name: openshift-service-ca.crt + patch: | + - op: remove + path: /metadata/annotations/service.beta.openshift.io~1inject-cabundle + + # Exclude monitoring resources on xKS (PodMonitor CRD requires Prometheus Operator, + # NetworkPolicy references OCP-specific redhat-ods-monitoring namespace) + - target: + group: monitoring.coreos.com + version: v1 + kind: PodMonitor + name: maas-api-metrics + patch: | + $patch: delete + apiVersion: monitoring.coreos.com/v1 + kind: PodMonitor + metadata: + name: maas-api-metrics + - target: + kind: NetworkPolicy + name: maas-api-allow-monitoring + patch: | + $patch: delete + apiVersion: networking.k8s.io/v1 + kind: NetworkPolicy + metadata: + name: maas-api-allow-monitoring diff --git a/maas-api/internal/handlers/models.go b/maas-api/internal/handlers/models.go index f174106eb..bd491a580 100644 --- a/maas-api/internal/handlers/models.go +++ b/maas-api/internal/handlers/models.go @@ -391,6 +391,10 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) { h.logger.Debug("User token request - returning all accessible models") } + // Prevent clients and proxies from caching authorization-checked model listings. + // Set early so every return path (including early 403s) includes the header. + c.Header("Cache-Control", "no-store") + // Determine which subscriptions to use for model filtering subscriptionsToUse, shouldReturn := h.selectSubscriptionsForListing(c, userContext, requestedSubscription, returnAllModels) if shouldReturn { @@ -422,7 +426,6 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) { } else { // User has zero accessible subscriptions - return empty list h.logger.Debug("User has zero accessible subscriptions, returning empty model list") - // modelList is already initialized to empty slice above } } else { // Filter models by subscription(s) and aggregate subscriptions @@ -435,10 +438,7 @@ func (h *ModelsHandler) ListLLMs(c *gin.Context) { h.logger.Debug("MaaSModelRef lister not configured, returning empty model list") } - // Prevent clients and proxies from caching authorization-checked model listings. - // The access check is a point-in-time snapshot; auth policies may change at any moment. // X-Access-Checked-At lets clients assess the freshness of the authorization decision. - c.Header("Cache-Control", "no-store") c.Header("X-Access-Checked-At", accessCheckedAt.Format(time.RFC3339)) h.logger.Debug("GET /v1/models returning models", "count", len(modelList)) diff --git a/maas-api/internal/models/maasmodelref.go b/maas-api/internal/models/maasmodelref.go index 39fcc96db..86b2dde2f 100644 --- a/maas-api/internal/models/maasmodelref.go +++ b/maas-api/internal/models/maasmodelref.go @@ -57,9 +57,10 @@ func GVR() schema.GroupVersionResource { // maasModelRefToModel converts a MaaSModelRef unstructured to a Model for the API. // // For LLMInferenceService-backed models (BBR clusters), the model ID is read from -// status.resolvedModelAlias (the canonical publishers/{ns}/models/{name} form), and -// the URL is derived from status.httpRouteHostnames[0] (the shared gateway base URL). +// status.resolvedModelAlias (the canonical publishers/{ns}/models/{name} form). // For ExternalModel refs, the ExternalModel CR name is used as the ID. +// Both kinds advertise the shared gateway base URL from status.httpRouteHostnames[0] +// when present (falling back to status.endpoint). func maasModelRefToModel(u *unstructured.Unstructured) *Model { if u == nil { return nil @@ -115,28 +116,17 @@ func maasModelRefToModel(u *unstructured.Unstructured) *Model { } } + // Both LLMInferenceService and ExternalModel share the gateway base URL on BBR + // clusters. Prefer status.httpRouteHostnames[0]; fall back to status.endpoint + // (already a base URL after controller reconcile) when hostnames are not set yet. var urlPtr *apis.URL - switch kind { - case kindExternalModel: - // ExternalModel models keep using status.endpoint as their URL. - if endpoint != "" { - if parsed, err := url.Parse(endpoint); err == nil { - urlPtr = (*apis.URL)(parsed) - } + if hostnames, _, _ := unstructured.NestedStringSlice(u.Object, "status", "httpRouteHostnames"); len(hostnames) > 0 { + if parsed, err := url.Parse("https://" + hostnames[0]); err == nil { + urlPtr = (*apis.URL)(parsed) } - default: - // LLMInferenceService-backed models on BBR clusters share the gateway base URL. - // Derive it from status.httpRouteHostnames[0] so all models point at the same - // gateway entry-point instead of per-model path URLs. - if hostnames, _, _ := unstructured.NestedStringSlice(u.Object, "status", "httpRouteHostnames"); len(hostnames) > 0 { - if parsed, err := url.Parse("https://" + hostnames[0]); err == nil { - urlPtr = (*apis.URL)(parsed) - } - } else if endpoint != "" { - // Fall back to endpoint when httpRouteHostnames is not yet populated. - if parsed, err := url.Parse(endpoint); err == nil { - urlPtr = (*apis.URL)(parsed) - } + } else if endpoint != "" { + if parsed, err := url.Parse(endpoint); err == nil { + urlPtr = (*apis.URL)(parsed) } } diff --git a/maas-controller/README.md b/maas-controller/README.md index e07198bfc..0f7190d82 100644 --- a/maas-controller/README.md +++ b/maas-controller/README.md @@ -377,6 +377,10 @@ kubectl annotate authpolicy -n opendatahub.io/managed- kubectl annotate tokenratelimitpolicy -n opendatahub.io/managed- ``` +### IPP plugins ConfigMap + +The `payload-processing-plugins` ConfigMap (gateway namespace) is stamped with `opendatahub.io/managed=false` after the controller creates or migrates it, so operators can edit the IPP plugin profile (for example re-enable response `api-translation`) without reconcile overwriting the change. Set `opendatahub.io/managed=true` to opt back into continuous reconciler management, or remove the annotation for a one-shot reset to product defaults. See [External Model Setup — IPP response translation](../docs/content/install/external-model-setup.md#ipp-response-translation-opt-in). + > **Warning: orphaned resources.** An opted-out policy can become permanently orphaned (no longer reconciled and not deleted) in the following situations: > > - **Last owner deleted.** When the last `MaaSAuthPolicy` or `MaaSSubscription` that references a model is deleted, the controller skips deletion of any opted-out generated policy for that model. The policy will persist until it is manually deleted. diff --git a/maas-controller/cmd/manager/main.go b/maas-controller/cmd/manager/main.go index 2e12fa2af..4e826da9b 100644 --- a/maas-controller/cmd/manager/main.go +++ b/maas-controller/cmd/manager/main.go @@ -69,8 +69,6 @@ var ( setupLog = ctrl.Log.WithName("setup") ) -const defaultAITenantBootstrappedAnnotation = "maas.opendatahub.io/default-aitenant-bootstrapped" - const ( tlsProfileFetchMaxRetries = 3 tlsProfileFetchTimeout = 10 * time.Second @@ -553,13 +551,23 @@ func ensureDefaultAITenantBootstrap(ctx context.Context, c client.Client, tenant return false, fmt.Errorf("get default AITenant: %w", err) } } else { + // Only mark bootstrap complete when the default AITenant is not being + // deleted and has a Ready condition set to True. If the AITenant is + // Terminating (e.g. stuck on a finalizer) or not yet ready, we must not + // set the annotation so that bootstrap can create a healthy replacement + // once the stuck resource is cleaned up. + isTerminating := !existing.DeletionTimestamp.IsZero() || + existing.Status.Phase == "Terminating" + if isTerminating || !apimeta.IsStatusConditionTrue(existing.Status.Conditions, maasv1alpha1.AITenantConditionReady) { + return false, nil + } if err := markDefaultAITenantBootstrapped(ctx, c, &ct); err != nil { return false, err } return false, nil } - if ct.Annotations[defaultAITenantBootstrappedAnnotation] == "true" { + if ct.Annotations[maas.DefaultAITenantBootstrappedAnnotation] == "true" { return false, nil } @@ -610,14 +618,11 @@ func ensureDefaultAITenantBootstrap(ctx context.Context, c client.Client, tenant } return false, fmt.Errorf("create default AITenant: %w", err) } - if err := markDefaultAITenantBootstrapped(ctx, c, &ct); err != nil { - return true, err - } return true, nil } func markDefaultAITenantBootstrapped(ctx context.Context, c client.Client, ct *maasv1alpha1.Config) error { - if ct == nil || ct.Annotations[defaultAITenantBootstrappedAnnotation] == "true" { + if ct == nil || ct.Annotations[maas.DefaultAITenantBootstrappedAnnotation] == "true" { return nil } base := ct.DeepCopy() @@ -625,7 +630,7 @@ func markDefaultAITenantBootstrapped(ctx context.Context, c client.Client, ct *m if annotations == nil { annotations = map[string]string{} } - annotations[defaultAITenantBootstrappedAnnotation] = "true" + annotations[maas.DefaultAITenantBootstrappedAnnotation] = "true" ct.SetAnnotations(annotations) if err := c.Patch(ctx, ct, client.MergeFrom(base)); err != nil { return fmt.Errorf("mark default AITenant bootstrap on Config/default: %w", err) @@ -841,6 +846,19 @@ func resolveInfraNamespace(infraNs, controllerNs string) string { // deriveInfraNamespace maps controller namespace to infrastructure namespace. // This implements namespace separation: controller runs in one namespace, infrastructure services in another. +func clampConcurrentReconciles(v int) int { + const minConcurrent, maxConcurrent = 1, 10 + if v < minConcurrent { + setupLog.Info("clamping --max-concurrent-reconciles to minimum", "requested", v, "using", minConcurrent) + return minConcurrent + } + if v > maxConcurrent { + setupLog.Info("clamping --max-concurrent-reconciles to maximum", "requested", v, "using", maxConcurrent) + return maxConcurrent + } + return v +} + func deriveInfraNamespace(controllerNs string) string { switch controllerNs { case "redhat-ods-applications": @@ -922,6 +940,7 @@ func main() { var authzCacheTTL int64 var subscriptionNamespaceMaintainInterval time.Duration var enableTenantNamespaceDiscovery bool + var maxConcurrentReconciles int var observabilityManifestsPath string var monitoringNamespace string var usageLogsManifestPath string @@ -946,6 +965,8 @@ func main() { flag.DurationVar(&subscriptionNamespaceMaintainInterval, "subscription-namespace-maintain-interval", 30*time.Second, "How often to re-check controller-managed namespaces while the manager is running (recreate if deleted). "+ "Larger values reduce apiserver load; smaller values detect external deletions sooner.") + flag.IntVar(&maxConcurrentReconciles, "max-concurrent-reconciles", 5, + "Maximum number of concurrent reconciles for subscription and auth policy controllers (1-10). Values above 5 may require increased CPU/memory on the controller pod.") flag.BoolVar(&enableTenantNamespaceDiscovery, "enable-tenant-namespace-discovery", false, "Discover AITenant-managed tenant namespaces labeled ai-gateway.opendatahub.io/tenant or maas.opendatahub.io/managed-by-aitenant=true and reconcile MaaS tenant CRs from them.") @@ -953,6 +974,8 @@ func main() { opts.BindFlags(flag.CommandLine) flag.Parse() + maxConcurrentReconciles = clampConcurrentReconciles(maxConcurrentReconciles) + // Allow empty monitoring-namespace to disable observability features (e.g. on xKS // where the monitoring namespace may not exist). Non-empty values must be valid. if monitoringNamespace != "" { @@ -1131,6 +1154,7 @@ func main() { MetadataCacheTTL: metadataCacheTTL, AuthzCacheTTL: authzCacheTTL, TenantNamespaceDiscoveryEnabled: enableTenantNamespaceDiscovery, + MaxConcurrentReconciles: maxConcurrentReconciles, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MaaSAuthPolicy") os.Exit(1) @@ -1142,6 +1166,7 @@ func main() { TenantNamespaceDiscoveryEnabled: enableTenantNamespaceDiscovery, GatewayName: gatewayName, GatewayNamespace: gatewayNamespace, + MaxConcurrentReconciles: maxConcurrentReconciles, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MaaSSubscription") os.Exit(1) @@ -1198,7 +1223,10 @@ func main() { manifestPath := os.Getenv("MAAS_PLATFORM_MANIFESTS") if manifestPath == "" { - manifestPath = tenantreconcile.DefaultManifestPath() + // tlsConfig.available reflects whether config.openshift.io API exists, + // which is the authoritative signal for OCP vs vanilla Kubernetes. + isOCP := tlsConfig.available + manifestPath = tenantreconcile.ManifestPathForPlatform(isOCP) } if abs, err := filepath.Abs(manifestPath); err == nil { manifestPath = abs diff --git a/maas-controller/cmd/manager/main_test.go b/maas-controller/cmd/manager/main_test.go index 731473ef4..f271ce275 100644 --- a/maas-controller/cmd/manager/main_test.go +++ b/maas-controller/cmd/manager/main_test.go @@ -241,8 +241,8 @@ func TestEnsureDefaultAITenantBootstrapCreatesAITenantFromExistingTenant(t *test if err := cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &cfg); err != nil { t.Fatalf("get Config: %v", err) } - if got := cfg.Annotations[defaultAITenantBootstrappedAnnotation]; got != "true" { - t.Fatalf("Config bootstrap annotation = %q, want true", got) + if got := cfg.Annotations[maas.DefaultAITenantBootstrappedAnnotation]; got != "" { + t.Fatalf("Config bootstrap annotation = %q, want empty until AITenant is Ready", got) } } @@ -347,6 +347,16 @@ func TestEnsureDefaultAITenantBootstrapNoopsWhenAITenantExistsAndMarksConfig(t * Spec: maasv1alpha1.AITenantSpec{ Gateway: &maasv1alpha1.AITenantGatewayRef{Name: "already-owned"}, }, + Status: maasv1alpha1.AITenantStatus{ + Conditions: []metav1.Condition{ + { + Type: maasv1alpha1.AITenantConditionReady, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Reason: "Ready", + }, + }, + }, }, ). Build() @@ -382,11 +392,213 @@ func TestEnsureDefaultAITenantBootstrapNoopsWhenAITenantExistsAndMarksConfig(t * if err := cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &cfg); err != nil { t.Fatalf("get Config: %v", err) } - if got := cfg.Annotations[defaultAITenantBootstrappedAnnotation]; got != "true" { + if got := cfg.Annotations[maas.DefaultAITenantBootstrappedAnnotation]; got != "true" { t.Fatalf("Config bootstrap annotation = %q, want true", got) } } +func TestEnsureDefaultAITenantBootstrapSkipsTerminatingAITenant(t *testing.T) { + ctx := context.Background() + s := managerTestScheme(t) + now := metav1.Now() + cl := controllerfake.NewClientBuilder(). + WithScheme(s). + WithObjects( + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.MaaSControllerDeploymentName, + Namespace: "opendatahub", + }, + }, + &maasv1alpha1.Config{ + ObjectMeta: metav1.ObjectMeta{ + Name: maasv1alpha1.ConfigInstanceName, + UID: types.UID("cfg-default"), + }, + }, + &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.DefaultAITenantName, + Namespace: tenantreconcile.DefaultAITenantNamespace, + DeletionTimestamp: &now, + Finalizers: []string{"test-finalizer"}, + }, + Spec: maasv1alpha1.AITenantSpec{ + Gateway: &maasv1alpha1.AITenantGatewayRef{Name: "gw"}, + }, + Status: maasv1alpha1.AITenantStatus{ + Conditions: []metav1.Condition{ + { + Type: maasv1alpha1.AITenantConditionReady, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Reason: "Ready", + }, + }, + }, + }, + ). + Build() + + created, err := ensureDefaultAITenantBootstrap( + ctx, + cl, + "models-as-a-service", + tenantreconcile.DefaultAITenantNamespace, + "opendatahub", + tenantreconcile.MaaSControllerDeploymentName, + "maas-default-gateway", + "openshift-ingress", + ) + if err != nil { + t.Fatalf("ensure default AITenant: %v", err) + } + if created { + t.Fatalf("created = true, want false when AITenant is Terminating") + } + + var cfg maasv1alpha1.Config + if err := cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &cfg); err != nil { + t.Fatalf("get Config: %v", err) + } + if got := cfg.Annotations[maas.DefaultAITenantBootstrappedAnnotation]; got == "true" { + t.Fatalf("Config bootstrap annotation = %q, want empty when AITenant is Terminating", got) + } +} + +func TestEnsureDefaultAITenantBootstrapSkipsTerminatingPhaseAITenant(t *testing.T) { + ctx := context.Background() + s := managerTestScheme(t) + cl := controllerfake.NewClientBuilder(). + WithScheme(s). + WithObjects( + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.MaaSControllerDeploymentName, + Namespace: "opendatahub", + }, + }, + &maasv1alpha1.Config{ + ObjectMeta: metav1.ObjectMeta{ + Name: maasv1alpha1.ConfigInstanceName, + UID: types.UID("cfg-default"), + }, + }, + &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.DefaultAITenantName, + Namespace: tenantreconcile.DefaultAITenantNamespace, + }, + Spec: maasv1alpha1.AITenantSpec{ + Gateway: &maasv1alpha1.AITenantGatewayRef{Name: "gw"}, + }, + Status: maasv1alpha1.AITenantStatus{ + Phase: "Terminating", + Conditions: []metav1.Condition{ + { + Type: maasv1alpha1.AITenantConditionReady, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.Now(), + Reason: "Ready", + }, + }, + }, + }, + ). + Build() + + created, err := ensureDefaultAITenantBootstrap( + ctx, + cl, + "models-as-a-service", + tenantreconcile.DefaultAITenantNamespace, + "opendatahub", + tenantreconcile.MaaSControllerDeploymentName, + "maas-default-gateway", + "openshift-ingress", + ) + if err != nil { + t.Fatalf("ensure default AITenant: %v", err) + } + if created { + t.Fatalf("created = true, want false when AITenant phase is Terminating") + } + + var cfg maasv1alpha1.Config + if err := cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &cfg); err != nil { + t.Fatalf("get Config: %v", err) + } + if got := cfg.Annotations[maas.DefaultAITenantBootstrappedAnnotation]; got == "true" { + t.Fatalf("Config bootstrap annotation = %q, want empty when AITenant phase is Terminating", got) + } +} + +func TestEnsureDefaultAITenantBootstrapSkipsNotReadyAITenant(t *testing.T) { + ctx := context.Background() + s := managerTestScheme(t) + cl := controllerfake.NewClientBuilder(). + WithScheme(s). + WithObjects( + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.MaaSControllerDeploymentName, + Namespace: "opendatahub", + }, + }, + &maasv1alpha1.Config{ + ObjectMeta: metav1.ObjectMeta{ + Name: maasv1alpha1.ConfigInstanceName, + UID: types.UID("cfg-default"), + }, + }, + &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: tenantreconcile.DefaultAITenantName, + Namespace: tenantreconcile.DefaultAITenantNamespace, + }, + Spec: maasv1alpha1.AITenantSpec{ + Gateway: &maasv1alpha1.AITenantGatewayRef{Name: "gw"}, + }, + Status: maasv1alpha1.AITenantStatus{ + Conditions: []metav1.Condition{ + { + Type: maasv1alpha1.AITenantConditionReady, + Status: metav1.ConditionFalse, + LastTransitionTime: metav1.Now(), + Reason: "NotReady", + }, + }, + }, + }, + ). + Build() + + created, err := ensureDefaultAITenantBootstrap( + ctx, + cl, + "models-as-a-service", + tenantreconcile.DefaultAITenantNamespace, + "opendatahub", + tenantreconcile.MaaSControllerDeploymentName, + "maas-default-gateway", + "openshift-ingress", + ) + if err != nil { + t.Fatalf("ensure default AITenant: %v", err) + } + if created { + t.Fatalf("created = true, want false when AITenant is not Ready") + } + + var cfg maasv1alpha1.Config + if err := cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &cfg); err != nil { + t.Fatalf("get Config: %v", err) + } + if got := cfg.Annotations[maas.DefaultAITenantBootstrappedAnnotation]; got == "true" { + t.Fatalf("Config bootstrap annotation = %q, want empty when AITenant is not Ready", got) + } +} + func TestEnsureDefaultAITenantBootstrapWaitsForConfigUID(t *testing.T) { ctx := context.Background() s := managerTestScheme(t) @@ -562,7 +774,7 @@ func TestEnsureDefaultAITenantBootstrapDoesNotRecreateAfterBootstrapMarker(t *te Name: maasv1alpha1.ConfigInstanceName, UID: types.UID("cfg-default"), Annotations: map[string]string{ - defaultAITenantBootstrappedAnnotation: "true", + maas.DefaultAITenantBootstrappedAnnotation: "true", }, }, }, diff --git a/maas-controller/pkg/controller/maas/aitenant_controller.go b/maas-controller/pkg/controller/maas/aitenant_controller.go index 9f0d74206..393510568 100644 --- a/maas-controller/pkg/controller/maas/aitenant_controller.go +++ b/maas-controller/pkg/controller/maas/aitenant_controller.go @@ -37,13 +37,16 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1" maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" @@ -151,44 +154,84 @@ func (r *AITenantReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{RequeueAfter: time.Second}, nil } - gatewayRef, err := r.validateTenantGateway(ctx, &aitenant) + gatewayRef := r.gatewayRefFor(&aitenant) aitenant.Status.GatewayRef = gatewayRef - if err != nil { - setAITenantPhase(&aitenant, "Failed", "GatewayCheckFailed", err.Error()) - if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { - return ctrl.Result{}, err2 - } - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil - } if err := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err != nil { return ctrl.Result{}, err } statusSnapshot = aitenant.Status.DeepCopy() - if err := r.ensureGatewayClaim(ctx, &aitenant, gatewayRef); err != nil { - setAITenantPhase(&aitenant, "Failed", "GatewayClaimFailed", err.Error()) - if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { - return ctrl.Result{}, err2 + var tenantConfigReady bool + ensureTenantResources := func() (ctrl.Result, bool, error) { + namespaceCreated, err := r.ensureTenantNamespace(ctx, &aitenant) + if err != nil { + setAITenantPhase(&aitenant, "Failed", "TenantNamespaceFailed", err.Error()) + if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { + return ctrl.Result{}, true, err2 + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, true, nil } - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + if namespaceCreated { + setAITenantPhase(&aitenant, "Pending", "TenantNamespacePending", "waiting for tenant namespace to become available") + if err := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err != nil { + return ctrl.Result{}, true, err + } + return ctrl.Result{RequeueAfter: time.Second}, true, nil + } + + var namespacePending bool + tenantConfigReady, namespacePending, err = r.ensureTenantConfig(ctx, &aitenant) + if err != nil { + setAITenantPhase(&aitenant, "Failed", "TenantConfigReconcileFailed", err.Error()) + if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { + return ctrl.Result{}, true, err2 + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, true, nil + } + if namespacePending { + setAITenantPhase(&aitenant, "Pending", "TenantNamespacePending", "waiting for tenant namespace to accept tenant resources") + if err := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err != nil { + return ctrl.Result{}, true, err + } + return ctrl.Result{RequeueAfter: time.Second}, true, nil + } + + return ctrl.Result{}, false, nil } - if err := r.ensureTenantNamespace(ctx, &aitenant); err != nil { - setAITenantPhase(&aitenant, "Failed", "TenantNamespaceFailed", err.Error()) + // The default namespace must be enabled before the Gateway becomes Ready so + // the UI is not blocked by the tenant-namespace admission check during normal + // bootstrap. Other AITenants keep the existing gateway-first provisioning + // order. + defaultTenantBootstrap := aitenant.Name == tenantreconcile.DefaultAITenantName + if defaultTenantBootstrap { + if res, done, err := ensureTenantResources(); err != nil || done { + return res, err + } + } + + if err := r.validateTenantGateway(ctx, gatewayRef); err != nil { + setAITenantPhase(&aitenant, "Failed", "GatewayCheckFailed", err.Error()) if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { return ctrl.Result{}, err2 } return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - if err := r.ensureTenantConfig(ctx, &aitenant); err != nil { - setAITenantPhase(&aitenant, "Failed", "TenantConfigReconcileFailed", err.Error()) + if err := r.ensureGatewayClaim(ctx, &aitenant, gatewayRef); err != nil { + setAITenantPhase(&aitenant, "Failed", "GatewayClaimFailed", err.Error()) if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { return ctrl.Result{}, err2 } return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } + if !defaultTenantBootstrap { + if res, done, err := ensureTenantResources(); err != nil || done { + return res, err + } + } + if err := r.ensureTenantAdminRBAC(ctx, &aitenant); err != nil { setAITenantPhase(&aitenant, "Failed", "RBACReconcileFailed", err.Error()) if err2 := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err2 != nil { @@ -197,6 +240,14 @@ func (r *AITenantReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } + if !tenantConfigReady { + setAITenantPhase(&aitenant, "Pending", "TenantConfigNotReady", "waiting for MaasTenantConfig to report Ready") + if err := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } + setAITenantPhase(&aitenant, "Active", "Reconciled", "AITenant bootstrap resources are reconciled") if err := r.updateAITenantStatus(ctx, &aitenant, statusSnapshot); err != nil { return ctrl.Result{}, err @@ -213,9 +264,32 @@ func (r *AITenantReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&maasv1alpha1.AITenant{}, builder.WithPredicates( predicate.Or(predicate.GenerationChangedPredicate{}, predicate.Funcs{UpdateFunc: deletionTimestampSet}), )). + Watches( + &maasv1alpha1.MaasTenantConfig{}, + handler.EnqueueRequestsFromMapFunc(r.enqueueAITenantForTenantConfig), + ). Complete(r) } +// enqueueAITenantForTenantConfig maps MaasTenantConfig events back to the +// owning AITenant. This ensures the AITenant reconciler re-creates the +// MaasTenantConfig when a ghost from a previous install cycle finishes deleting. +func (r *AITenantReconciler) enqueueAITenantForTenantConfig(_ context.Context, obj client.Object) []reconcile.Request { + annotations := obj.GetAnnotations() + if annotations == nil { + return nil + } + name := annotations[aitenantNameAnnotation] + ns := annotations[aitenantNamespaceAnnotation] + if name == "" || ns == "" { + return nil + } + return []reconcile.Request{{NamespacedName: types.NamespacedName{ + Name: name, + Namespace: ns, + }}} +} + func (r *AITenantReconciler) validateAITenantPlacement(aitenant *maasv1alpha1.AITenant) error { if aitenant.Namespace == "" { return fmt.Errorf("AITenant %q must be namespaced", aitenant.Name) @@ -254,7 +328,7 @@ func (r *AITenantReconciler) tenantNamespaceName(aitenant *maasv1alpha1.AITenant return tenantreconcile.TenantNamespaceForAITenant(aitenant.Name, r.TenantNamespace) } -func (r *AITenantReconciler) ensureTenantNamespace(ctx context.Context, aitenant *maasv1alpha1.AITenant) error { +func (r *AITenantReconciler) ensureTenantNamespace(ctx context.Context, aitenant *maasv1alpha1.AITenant) (bool, error) { name := r.tenantNamespaceName(aitenant) var ns corev1.Namespace err := r.get(ctx, client.ObjectKey{Name: name}, &ns) @@ -269,54 +343,53 @@ func (r *AITenantReconciler) ensureTenantNamespace(ctx context.Context, aitenant setMapValue(&toCreate.Annotations, aitenantCreatedAnnotation, "true") if createErr := r.Create(ctx, toCreate); createErr != nil { if !isAlreadyExistsError(createErr) { - return fmt.Errorf("create tenant namespace %q: %w", name, createErr) + return false, fmt.Errorf("create tenant namespace %q: %w", name, createErr) } if err := r.get(ctx, client.ObjectKey{Name: name}, &ns); err != nil { - return fmt.Errorf("get tenant namespace %q after create conflict: %w", name, err) + return false, fmt.Errorf("get tenant namespace %q after create conflict: %w", name, err) } err = nil } else { - return nil + return true, nil } } if err != nil { - return fmt.Errorf("get tenant namespace %q: %w", name, err) + return false, fmt.Errorf("get tenant namespace %q: %w", name, err) } if ns.Status.Phase == corev1.NamespaceTerminating { - return fmt.Errorf("tenant namespace %q is terminating", name) + return false, fmt.Errorf("tenant namespace %q is terminating", name) } if hasAITenantOwnerAnnotations(&ns) && !ownedByAITenant(&ns, aitenant) { - return fmt.Errorf("tenant namespace %q is managed by another AITenant", name) + return false, fmt.Errorf("tenant namespace %q is managed by another AITenant", name) } base := ns.DeepCopy() applyAITenantMetadata(&ns, aitenant, name) if equality.Semantic.DeepEqual(base, &ns) { - return nil + return false, nil } if err := r.Patch(ctx, &ns, client.MergeFrom(base)); err != nil { - return fmt.Errorf("patch tenant namespace %q: %w", name, err) + return false, fmt.Errorf("patch tenant namespace %q: %w", name, err) } - return nil + return false, nil } -func (r *AITenantReconciler) validateTenantGateway(ctx context.Context, aitenant *maasv1alpha1.AITenant) (maasv1alpha1.TenantGatewayRef, error) { - ref := r.gatewayRefFor(aitenant) +func (r *AITenantReconciler) validateTenantGateway(ctx context.Context, ref maasv1alpha1.TenantGatewayRef) error { if ref.Namespace == "" { - return ref, errors.New("gateway namespace is required; set --gateway-namespace") + return errors.New("gateway namespace is required; set --gateway-namespace") } if ref.Name == "" { - return ref, errors.New("spec.gateway.name is required when AITenant name is empty") + return errors.New("spec.gateway.name is required when AITenant name is empty") } var gateway gatewayapiv1.Gateway key := client.ObjectKey{Namespace: ref.Namespace, Name: ref.Name} if err := r.get(ctx, key, &gateway); err != nil { if isNotFoundError(err) { - return ref, fmt.Errorf("gateway %s/%s not found: the Gateway must be created by a network or cluster administrator before AITenant can be provisioned", key.Namespace, key.Name) + return fmt.Errorf("gateway %s/%s not found: the Gateway must be created by a network or cluster administrator before AITenant can be provisioned", key.Namespace, key.Name) } - return ref, fmt.Errorf("get Gateway %s/%s: %w", key.Namespace, key.Name, err) + return fmt.Errorf("get Gateway %s/%s: %w", key.Namespace, key.Name, err) } - return ref, nil + return nil } func (r *AITenantReconciler) gatewayRefFor(aitenant *maasv1alpha1.AITenant) maasv1alpha1.TenantGatewayRef { @@ -375,8 +448,9 @@ func (r *AITenantReconciler) legacyGatewayNameIsSharedDefault(aitenant *maasv1al return aitenant.Name != tenantreconcile.DefaultAITenantName && gatewayName == defaultGatewayName } -func (r *AITenantReconciler) ensureTenantConfig(ctx context.Context, aitenant *maasv1alpha1.AITenant) error { +func (r *AITenantReconciler) ensureTenantConfig(ctx context.Context, aitenant *maasv1alpha1.AITenant) (bool, bool, error) { tenantNamespace := r.tenantNamespaceName(aitenant) + config := &maasv1alpha1.MaasTenantConfig{ TypeMeta: metav1.TypeMeta{ APIVersion: maasv1alpha1.GroupVersion.String(), @@ -392,15 +466,30 @@ func (r *AITenantReconciler) ensureTenantConfig(ctx context.Context, aitenant *m if !ok { return fmt.Errorf("expected MaasTenantConfig, got %T", obj) } + if !t.DeletionTimestamp.IsZero() { + return fmt.Errorf("MaasTenantConfig %s/%s is being deleted; waiting for cleanup to finish before recreating", t.Namespace, t.Name) + } applyAITenantMetadata(t, aitenant, tenantNamespace) if err := r.copyLegacyTenantConfig(ctx, t); err != nil { return err } return nil }); err != nil { - return err + if isNamespaceMissingError(err) { + return false, true, nil + } + return false, false, err + } + if err := r.markLegacyTenantDeprecated(ctx, tenantNamespace); err != nil { + return false, false, err } - return r.markLegacyTenantDeprecated(ctx, tenantNamespace) + if err := r.get(ctx, client.ObjectKeyFromObject(config), config); err != nil { + return false, false, fmt.Errorf("get MaasTenantConfig %s/%s readiness: %w", config.Namespace, config.Name, err) + } + ready := apimeta.FindStatusCondition(config.Status.Conditions, tenantreconcile.ReadyConditionType) + return ready != nil && + ready.Status == metav1.ConditionTrue && + ready.ObservedGeneration == config.Generation, false, nil } func (r *AITenantReconciler) copyLegacyTenantConfig(ctx context.Context, config *maasv1alpha1.MaasTenantConfig) error { @@ -1360,6 +1449,25 @@ func isAlreadyExistsError(err error) bool { return hasAPIStatusReason(err, metav1.StatusReasonAlreadyExists) } +func isNamespaceMissingError(err error) bool { + var statusErr *apierrors.StatusError + if !errors.As(err, &statusErr) { + return false + } + if statusErr.Status().Reason != metav1.StatusReasonNotFound { + return false + } + details := statusErr.Status().Details + if details != nil { + kind := strings.ToLower(details.Kind) + if kind == "namespace" || kind == "namespaces" { + return true + } + } + msg := strings.ToLower(statusErr.Status().Message) + return strings.HasPrefix(msg, "namespaces ") && strings.Contains(msg, "not found") +} + func hasAPIStatusReason(err error, reason metav1.StatusReason) bool { for err != nil { status, ok := err.(apierrors.APIStatus) diff --git a/maas-controller/pkg/controller/maas/aitenant_controller_test.go b/maas-controller/pkg/controller/maas/aitenant_controller_test.go index ac53493f4..3970a4f49 100644 --- a/maas-controller/pkg/controller/maas/aitenant_controller_test.go +++ b/maas-controller/pkg/controller/maas/aitenant_controller_test.go @@ -84,12 +84,52 @@ func (r *firstNotFoundReader) Get(ctx context.Context, key client.ObjectKey, obj func reconcileAITenantTwice(t *testing.T, r *AITenantReconciler, key types.NamespacedName) { t.Helper() g := NewWithT(t) + ctx := context.Background() - res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) g.Expect(err).NotTo(HaveOccurred()) g.Expect(res.RequeueAfter).To(Equal(time.Second)) - res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + // Finalizer convergence and tenant namespace creation can each requeue for + // one second before MaasTenantConfig is created. + converged := false + for i := 0; i < 3; i++ { + res, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + if res.RequeueAfter != time.Second { + converged = true + break + } + } + g.Expect(converged).To(BeTrue(), "AITenant bootstrap did not converge after expected one-second requeues") +} + +func reconcileAITenantToActive(t *testing.T, r *AITenantReconciler, key types.NamespacedName) { + t.Helper() + g := NewWithT(t) + ctx := context.Background() + + reconcileAITenantTwice(t, r, key) + + var aitenant maasv1alpha1.AITenant + g.Expect(r.Get(ctx, key, &aitenant)).To(Succeed()) + var tenantConfig maasv1alpha1.MaasTenantConfig + tenantConfigKey := client.ObjectKey{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: aitenant.Status.TenantNamespace, + } + g.Expect(r.Get(ctx, tenantConfigKey, &tenantConfig)).To(Succeed()) + tenantConfig.Status.Phase = "Active" + apimeta.SetStatusCondition(&tenantConfig.Status.Conditions, metav1.Condition{ + Type: tenantreconcile.ReadyConditionType, + Status: metav1.ConditionTrue, + Reason: "Reconciled", + ObservedGeneration: tenantConfig.Generation, + LastTransitionTime: metav1.Now(), + }) + g.Expect(r.Update(ctx, &tenantConfig)).To(Succeed()) + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) g.Expect(err).NotTo(HaveOccurred()) g.Expect(res).To(Equal(ctrl.Result{})) } @@ -132,7 +172,7 @@ func TestAITenantReconcile_ValidatesExistingGatewayAndCreatesBootstrapResources( } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var ns corev1.Namespace g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: "ai-tenant-team-a"}, &ns)).To(Succeed()) @@ -236,16 +276,16 @@ func TestAITenantReconcile_PersistsGatewayStatusBeforeTenantCreate(t *testing.T) GatewayNamespace: "openshift-ingress", } - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) } -func TestAITenantReconcile_MissingGatewaySetsFailedStatus(t *testing.T) { +func TestAITenantReconcile_DefaultTenantCreatesConfigBeforeGatewayReady(t *testing.T) { g := NewWithT(t) s := aitenantTestScheme(t) aitenant := &maasv1alpha1.AITenant{ ObjectMeta: metav1.ObjectMeta{ - Name: "team-missing-gw", + Name: tenantreconcile.DefaultAITenantName, Namespace: tenantreconcile.DefaultAITenantNamespace, }, Spec: maasv1alpha1.AITenantSpec{}, @@ -269,6 +309,10 @@ func TestAITenantReconcile_MissingGatewaySetsFailedStatus(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(res.RequeueAfter).To(Equal(time.Second)) + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(time.Second)) + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) g.Expect(err).NotTo(HaveOccurred()) g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) @@ -278,7 +322,7 @@ func TestAITenantReconcile_MissingGatewaySetsFailedStatus(t *testing.T) { g.Expect(updated.Status.Phase).To(Equal("Failed")) g.Expect(updated.Status.GatewayRef).To(Equal(maasv1alpha1.TenantGatewayRef{ Namespace: "openshift-ingress", - Name: "team-missing-gw", + Name: tenantreconcile.DefaultAITenantName, })) ready := apimeta.FindStatusCondition(updated.Status.Conditions, maasv1alpha1.AITenantConditionReady) g.Expect(ready).NotTo(BeNil()) @@ -286,12 +330,121 @@ func TestAITenantReconcile_MissingGatewaySetsFailedStatus(t *testing.T) { g.Expect(ready.Message).To(ContainSubstring("must be created by a network or cluster administrator")) var tenant maasv1alpha1.MaasTenantConfig - err = cl.Get(context.Background(), client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-missing-gw"}, &tenant) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + g.Expect(cl.Get(context.Background(), client.ObjectKey{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: "models-as-a-service", + }, &tenant)).To(Succeed()) + g.Expect(tenant.Labels).To(HaveKeyWithValue(aitenantManagedLabel, "true")) + g.Expect(tenant.Annotations).To(HaveKeyWithValue(aitenantNameAnnotation, tenantreconcile.DefaultAITenantName)) var ns corev1.Namespace - err = cl.Get(context.Background(), client.ObjectKey{Name: "ai-tenant-team-missing-gw"}, &ns) + g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: "models-as-a-service"}, &ns)).To(Succeed()) +} + +func TestAITenantReconcile_CustomTenantWaitsForGatewayBeforeCreatingResources(t *testing.T) { + g := NewWithT(t) + s := aitenantTestScheme(t) + + aitenant := &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "team-missing-gw", + Namespace: tenantreconcile.DefaultAITenantNamespace, + }, + } + cl := fake.NewClientBuilder(). + WithScheme(s). + WithStatusSubresource(&maasv1alpha1.AITenant{}). + WithObjects(aitenant). + Build() + r := &AITenantReconciler{ + Client: cl, + Scheme: s, + APIReader: cl, + AppNamespace: "opendatahub", + TenantNamespace: "models-as-a-service", + GatewayNamespace: "openshift-ingress", + } + key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(time.Second)) + + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) + + err = cl.Get(context.Background(), client.ObjectKey{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: "ai-tenant-team-missing-gw", + }, &maasv1alpha1.MaasTenantConfig{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + err = cl.Get(context.Background(), client.ObjectKey{Name: "ai-tenant-team-missing-gw"}, &corev1.Namespace{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) +} + +func TestAITenantReconcile_RetriesTenantConfigAfterNamespaceNotFound(t *testing.T) { + g := NewWithT(t) + s := aitenantTestScheme(t) + + aitenant := &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "team-retry", + Namespace: tenantreconcile.DefaultAITenantNamespace, + Finalizers: []string{aitenantFinalizer}, + }, + } + tenantNamespace := "ai-tenant-team-retry" + namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: tenantNamespace}} + gateway := existingAITenantGateway(aitenant.Name) + + namespaceNotFound := true + cl := fake.NewClientBuilder(). + WithScheme(s). + WithStatusSubresource(&maasv1alpha1.AITenant{}). + WithObjects(aitenant, namespace, gateway). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*maasv1alpha1.MaasTenantConfig); ok && namespaceNotFound { + namespaceNotFound = false + return apierrors.NewNotFound(schema.GroupResource{Resource: "namespaces"}, obj.GetNamespace()) + } + return c.Create(ctx, obj, opts...) + }, + }). + Build() + r := &AITenantReconciler{ + Client: cl, + Scheme: s, + APIReader: cl, + AppNamespace: "opendatahub", + TenantNamespace: "models-as-a-service", + GatewayNamespace: "openshift-ingress", + } + key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} + + res, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(time.Second)) + err = cl.Get(context.Background(), client.ObjectKey{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: tenantNamespace, + }, &maasv1alpha1.MaasTenantConfig{}) g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + + var pending maasv1alpha1.AITenant + g.Expect(cl.Get(context.Background(), key, &pending)).To(Succeed()) + ready := apimeta.FindStatusCondition(pending.Status.Conditions, maasv1alpha1.AITenantConditionReady) + g.Expect(ready).NotTo(BeNil()) + g.Expect(ready.Reason).To(Equal("TenantNamespacePending")) + + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) + g.Expect(cl.Get(context.Background(), client.ObjectKey{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: tenantNamespace, + }, &maasv1alpha1.MaasTenantConfig{})).To(Succeed()) } func TestAITenantReconcile_ExplicitGatewayNameResolvesExistingGateway(t *testing.T) { @@ -322,7 +475,7 @@ func TestAITenantReconcile_ExplicitGatewayNameResolvesExistingGateway(t *testing } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -391,6 +544,22 @@ func TestAITenantReconcile_UpdatesPreExistingTenant(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(res.RequeueAfter).To(Equal(time.Second)) + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) + + var readyConfig maasv1alpha1.MaasTenantConfig + readyConfigKey := client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-adoptcfg"} + g.Expect(cl.Get(context.Background(), readyConfigKey, &readyConfig)).To(Succeed()) + apimeta.SetStatusCondition(&readyConfig.Status.Conditions, metav1.Condition{ + Type: tenantreconcile.ReadyConditionType, + Status: metav1.ConditionTrue, + Reason: "Reconciled", + ObservedGeneration: readyConfig.Generation, + LastTransitionTime: metav1.Now(), + }) + g.Expect(cl.Update(context.Background(), &readyConfig)).To(Succeed()) + res, err = r.Reconcile(context.Background(), ctrl.Request{NamespacedName: key}) g.Expect(err).NotTo(HaveOccurred()) g.Expect(res).To(Equal(ctrl.Result{})) @@ -547,7 +716,7 @@ func TestAITenantReconcile_IgnoresLegacyDefaultGatewayForNonDefaultTenant(t *tes } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -649,7 +818,7 @@ func TestAITenantReconcile_LabelsPreExistingDerivedNamespace(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updatedNS corev1.Namespace g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: "ai-tenant-team-b"}, &updatedNS)).To(Succeed()) @@ -799,7 +968,7 @@ func TestAITenantReconcile_AllowsDefaultTenantNamespaceFromInfraNamespace(t *tes } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -843,7 +1012,7 @@ func TestAITenantReconcile_DefaultAITenantUsesConfiguredTenantNamespace(t *testi } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -883,7 +1052,7 @@ func TestAITenantReconcile_IdempotentWhenActive(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var afterActive maasv1alpha1.AITenant g.Expect(cl.Get(ctx, key, &afterActive)).To(Succeed()) @@ -899,6 +1068,112 @@ func TestAITenantReconcile_IdempotentWhenActive(t *testing.T) { g.Expect(afterRepeat.Status).To(Equal(afterActive.Status)) } +func TestAITenantReconcile_DeletingTenantConfigBlocksActive(t *testing.T) { + g := NewWithT(t) + s := aitenantTestScheme(t) + ctx := context.Background() + + aitenant := &maasv1alpha1.AITenant{ + ObjectMeta: metav1.ObjectMeta{ + Name: "team-ghost", + Namespace: tenantreconcile.DefaultAITenantNamespace, + }, + Spec: maasv1alpha1.AITenantSpec{}, + } + // Simulate a ghost MaasTenantConfig that is mid-deletion (has DeletionTimestamp + // and a cleanup finalizer). This happens during reinstall when the old tenant + // config is still cleaning up while a new AITenant is created. + now := metav1.Now() + ghostTenantConfig := &maasv1alpha1.MaasTenantConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: maasv1alpha1.MaasTenantConfigInstanceName, + Namespace: "ai-tenant-team-ghost", + DeletionTimestamp: &now, + Finalizers: []string{tenantFinalizer}, + Annotations: map[string]string{ + aitenantNameAnnotation: "team-ghost", + aitenantNamespaceAnnotation: tenantreconcile.DefaultAITenantNamespace, + }, + }, + } + ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ai-tenant-team-ghost"}} + cl := fake.NewClientBuilder(). + WithScheme(s). + WithStatusSubresource(&maasv1alpha1.AITenant{}). + WithObjects(aitenant, ghostTenantConfig, ns, existingAITenantGateway("team-ghost")). + Build() + r := &AITenantReconciler{ + Client: cl, + Scheme: s, + APIReader: cl, + AppNamespace: "opendatahub", + TenantNamespace: "models-as-a-service", + GatewayNamespace: "openshift-ingress", + } + + key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} + + // First reconcile adds the finalizer. + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(time.Second)) + + // Second reconcile should detect the deleting MaasTenantConfig and NOT go Active. + res, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) + + var updated maasv1alpha1.AITenant + g.Expect(cl.Get(ctx, key, &updated)).To(Succeed()) + g.Expect(updated.Status.Phase).To(Equal("Failed")) + ready := apimeta.FindStatusCondition(updated.Status.Conditions, maasv1alpha1.AITenantConditionReady) + g.Expect(ready).NotTo(BeNil()) + g.Expect(ready.Reason).To(Equal("TenantConfigReconcileFailed")) + g.Expect(ready.Message).To(ContainSubstring("being deleted")) + + // Simulate the ghost finalizer completing: remove finalizer so the object can be deleted. + var ghost maasv1alpha1.MaasTenantConfig + g.Expect(cl.Get(ctx, client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-ghost"}, &ghost)).To(Succeed()) + controllerutil.RemoveFinalizer(&ghost, tenantFinalizer) + g.Expect(cl.Update(ctx, &ghost)).To(Succeed()) + + // After the ghost is gone, reconciliation should create a new MaasTenantConfig + // but remain Pending until the tenant controller reports the runtime Ready. + res, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(30 * time.Second)) + + g.Expect(cl.Get(ctx, key, &updated)).To(Succeed()) + g.Expect(updated.Status.Phase).To(Equal("Pending")) + readyAfter := apimeta.FindStatusCondition(updated.Status.Conditions, maasv1alpha1.AITenantConditionReady) + g.Expect(readyAfter).NotTo(BeNil()) + g.Expect(readyAfter.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(readyAfter.Reason).To(Equal("TenantConfigNotReady")) + + var replacement maasv1alpha1.MaasTenantConfig + replacementKey := client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-ghost"} + g.Expect(cl.Get(ctx, replacementKey, &replacement)).To(Succeed()) + apimeta.SetStatusCondition(&replacement.Status.Conditions, metav1.Condition{ + Type: tenantreconcile.ReadyConditionType, + Status: metav1.ConditionTrue, + Reason: "Reconciled", + ObservedGeneration: replacement.Generation, + LastTransitionTime: metav1.Now(), + }) + g.Expect(cl.Update(ctx, &replacement)).To(Succeed()) + + res, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: key}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res).To(Equal(ctrl.Result{})) + + g.Expect(cl.Get(ctx, key, &updated)).To(Succeed()) + g.Expect(updated.Status.Phase).To(Equal("Active")) + readyAfter = apimeta.FindStatusCondition(updated.Status.Conditions, maasv1alpha1.AITenantConditionReady) + g.Expect(readyAfter).NotTo(BeNil()) + g.Expect(readyAfter.Status).To(Equal(metav1.ConditionTrue)) + g.Expect(readyAfter.Reason).To(Equal("Reconciled")) +} + func TestAITenantReconcile_RejectsNamespaceOwnedByAnotherAITenant(t *testing.T) { g := NewWithT(t) s := aitenantTestScheme(t) @@ -1215,7 +1490,7 @@ func TestAITenantReconcile_OIDCStaysInAITenantSpec(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var tenant maasv1alpha1.MaasTenantConfig g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-oidc"}, &tenant)).To(Succeed()) @@ -1248,7 +1523,7 @@ func TestAITenantReconcile_NoOIDCSetsTenantOIDCNil(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var tenant maasv1alpha1.MaasTenantConfig g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: maasv1alpha1.MaasTenantConfigInstanceName, Namespace: "ai-tenant-team-nooidc"}, &tenant)).To(Succeed()) @@ -1325,7 +1600,7 @@ func TestAITenantReconcile_GatewayClaimCreated(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -1370,7 +1645,7 @@ func TestAITenantReconcile_GatewayClaimBlocksDuplicateGateway(t *testing.T) { } key1 := types.NamespacedName{Name: aitenant1.Name, Namespace: aitenant1.Namespace} - reconcileAITenantTwice(t, r, key1) + reconcileAITenantToActive(t, r, key1) var updated1 maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key1, &updated1)).To(Succeed()) @@ -1439,7 +1714,7 @@ func TestAITenantReconcile_GatewayClaimCleanedOnDeletion(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) // Verify claim exists. gatewayRef := maasv1alpha1.TenantGatewayRef{Namespace: "openshift-ingress", Name: "cleanup-gw"} @@ -1494,7 +1769,7 @@ func TestAITenantReconcile_GatewayClaimIdempotent(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var afterFirst maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &afterFirst)).To(Succeed()) @@ -1538,7 +1813,7 @@ func TestAITenantReconcile_GatewayClaimHasOwnerReference(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(context.Background(), key, &updated)).To(Succeed()) @@ -1620,7 +1895,7 @@ func TestAITenantReconcile_GatewayClaimRetroactiveOwnerReference(t *testing.T) { // Reconcile the AITenant -- the controller should retroactively add the // OwnerReference to the existing claim. - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) var updated maasv1alpha1.AITenant g.Expect(cl.Get(ctx, key, &updated)).To(Succeed()) @@ -1666,7 +1941,7 @@ func TestAITenantReconcile_StaleClaimCleanedOnGatewayRetarget(t *testing.T) { } key := types.NamespacedName{Name: aitenant.Name, Namespace: aitenant.Namespace} - reconcileAITenantTwice(t, r, key) + reconcileAITenantToActive(t, r, key) // Verify old claim exists. oldRef := maasv1alpha1.TenantGatewayRef{Namespace: "openshift-ingress", Name: "gateway-old"} diff --git a/maas-controller/pkg/controller/maas/conflict_detection_test.go b/maas-controller/pkg/controller/maas/conflict_detection_test.go index df1f7457b..4ad882757 100644 --- a/maas-controller/pkg/controller/maas/conflict_detection_test.go +++ b/maas-controller/pkg/controller/maas/conflict_detection_test.go @@ -120,6 +120,7 @@ func TestDetectConflictingAuthPolicies_RogueDetected(t *testing.T) { httpRouteName = "maas-" + modelName maasPolicyName = "policy-a" rogueName = "kserve-route-authn" + gatewayNS = "openshift-ingress" ) model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) @@ -130,11 +131,11 @@ func TestDetectConflictingAuthPolicies_RogueDetected(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, rogueAP). + WithObjects(model, route, maasPolicy, rogueAP, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -168,6 +169,7 @@ func TestDetectConflictingAuthPolicies_MultipleRogues(t *testing.T) { namespace = "default" httpRouteName = "maas-" + modelName maasPolicyName = "policy-a" + gatewayNS = "openshift-ingress" ) model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) @@ -179,11 +181,11 @@ func TestDetectConflictingAuthPolicies_MultipleRogues(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, rogue1, rogue2). + WithObjects(model, route, maasPolicy, rogue1, rogue2, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -220,6 +222,7 @@ func TestDetectConflictingAuthPolicies_DifferentRoute(t *testing.T) { namespace = "default" httpRouteName = "maas-" + modelName maasPolicyName = "policy-a" + gatewayNS = "openshift-ingress" ) model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) @@ -230,11 +233,11 @@ func TestDetectConflictingAuthPolicies_DifferentRoute(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, unrelatedAP). + WithObjects(model, route, maasPolicy, unrelatedAP, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -264,6 +267,7 @@ func TestDetectConflictingAuthPolicies_CrossNamespaceIsolation(t *testing.T) { otherNS = "other-ns" httpRouteName = "maas-" + modelName maasPolicyName = "policy-a" + gatewayNS = "openshift-ingress" ) model := newMaaSModelRef(modelName, modelNamespace, "ExternalModel", modelName) @@ -274,11 +278,11 @@ func TestDetectConflictingAuthPolicies_CrossNamespaceIsolation(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, rogueInOtherNS). + WithObjects(model, route, maasPolicy, rogueInOtherNS, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: policyNS}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: unexpected error: %v", err) @@ -307,6 +311,7 @@ func TestDetectConflictingAuthPolicies_ConflictResolved(t *testing.T) { httpRouteName = "maas-" + modelName maasPolicyName = "policy-a" rogueName = "kserve-route-authn" + gatewayNS = "openshift-ingress" ) model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) @@ -317,11 +322,11 @@ func TestDetectConflictingAuthPolicies_ConflictResolved(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, rogueAP). + WithObjects(model, route, maasPolicy, rogueAP, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} ctx := context.Background() req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} @@ -367,6 +372,7 @@ func TestDetectConflictingAuthPolicies_MissingModel(t *testing.T) { const ( namespace = "default" maasPolicyName = "policy-a" + gatewayNS = "openshift-ingress" ) maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", @@ -375,11 +381,11 @@ func TestDetectConflictingAuthPolicies_MissingModel(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(maasPolicy). + WithObjects(maasPolicy, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: %v", err) @@ -414,6 +420,8 @@ func TestDetectConflictingAuthPolicies_GatewayTarget(t *testing.T) { route := newHTTPRoute(httpRouteName, namespace) maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", maasv1alpha1.ModelRef{Name: modelName, Namespace: namespace}) + gatewayNS := "openshift-ingress" + gatewayAP := &unstructured.Unstructured{} gatewayAP.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) gatewayAP.SetName("gateway-default-auth") @@ -429,11 +437,11 @@ func TestDetectConflictingAuthPolicies_GatewayTarget(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasPolicy, gatewayAP). + WithObjects(model, route, maasPolicy, gatewayAP, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() - r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system"} + r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, InfraNamespace: "maas-system", GatewayNamespace: gatewayNS, GatewayName: "maas-default-gateway"} req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { t.Fatalf("Reconcile: %v", err) diff --git a/maas-controller/pkg/controller/maas/constants.go b/maas-controller/pkg/controller/maas/constants.go index b53c7ebba..6cca143ea 100644 --- a/maas-controller/pkg/controller/maas/constants.go +++ b/maas-controller/pkg/controller/maas/constants.go @@ -4,6 +4,10 @@ const ( // DefaultUsageLogsTenancyProxyImage is the default image for the usage-logs tenancy proxy container. // Can be overridden via RELATED_IMAGE_ODH_PYTHON_312_IMAGE for disconnected environments. DefaultUsageLogsTenancyProxyImage = "registry.redhat.io/ubi9/python-312@sha256:f6713d327d37e654a443752e6654b5aab88f31690e1161eed9c34dd837870172" + + // DefaultAITenantBootstrappedAnnotation records that the default AITenant was + // bootstrapped successfully and must not be recreated after an intentional deletion. + DefaultAITenantBootstrappedAnnotation = "maas.opendatahub.io/default-aitenant-bootstrapped" ) // OptionalAPIGroups lists API groups whose CRDs are installed by optional platform diff --git a/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go b/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go index 0d1f4d29d..e2f3eb0f0 100644 --- a/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go +++ b/maas-controller/pkg/controller/maas/maasauthpolicy_controller.go @@ -41,6 +41,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -87,6 +88,9 @@ type MaaSAuthPolicyReconciler struct { // Recorder emits Kubernetes events for conflict detection warnings. Recorder record.EventRecorder + // MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. + // Defaults to 1 if not set. + MaxConcurrentReconciles int } // oidcConfig holds resolved OIDC configuration from AITenant or a legacy Tenant CR. @@ -600,12 +604,13 @@ func (r *MaaSAuthPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque } } - if err := r.reconcileGatewayAuthPolicy(ctx, log, string(modelAllowlistsJSON), oidc, xAPIKeyEnabled, tenantID, gatewayNs, gatewayName); err != nil { - log.Error(err, "failed to reconcile gateway AuthPolicy") - r.updateStatus(ctx, policy, maasv1alpha1.PhaseFailed, fmt.Sprintf("Failed to reconcile gateway AuthPolicy: %v", err), statusSnapshot) - return ctrl.Result{}, err + gwChanged, reconcileErr := r.reconcileGatewayAuthPolicy(ctx, log, string(modelAllowlistsJSON), oidc, xAPIKeyEnabled, tenantID, gatewayNs, gatewayName) + if reconcileErr != nil { + log.Error(reconcileErr, "failed to reconcile gateway AuthPolicy") + r.updateStatus(ctx, policy, maasv1alpha1.PhaseFailed, fmt.Sprintf("Failed to reconcile gateway AuthPolicy: %v", reconcileErr), statusSnapshot) + return ctrl.Result{}, reconcileErr } - if legacyPolicyExists { + if gwChanged || legacyPolicyExists || policy.Status.Phase != maasv1alpha1.PhaseActive { gatewayPolicyReady, readinessMessage, readinessErr := r.gatewayAuthPolicyReady(ctx, gatewayNs, gatewayName) if readinessErr != nil { log.Error(readinessErr, "failed to check gateway AuthPolicy readiness") @@ -614,7 +619,7 @@ func (r *MaaSAuthPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque } if !gatewayPolicyReady { message := fmt.Sprintf( - "Waiting for gateway AuthPolicy %s/%s to be accepted and enforced before removing the working legacy AuthPolicy: %s", + "Waiting for gateway AuthPolicy %s/%s to be accepted and enforced: %s", gatewayNs, r.gatewayAuthPolicyName(gatewayNs, gatewayName), readinessMessage, @@ -803,7 +808,7 @@ func (r *MaaSAuthPolicyReconciler) buildGatewayAuthPolicySpec(modelAccessJSON st if xAPIKeyEnabled { authenticationRules["api-keys-x-api-key"] = map[string]any{ "plain": map[string]any{ - "expression": `"Bearer " + request.headers["x-api-key"]`, + "selector": "request.headers.x-api-key", }, "when": []any{ map[string]any{ @@ -1210,10 +1215,9 @@ allow { return map[string]any{ "targetRef": map[string]any{ - "group": "gateway.networking.k8s.io", - "kind": "Gateway", - "name": gatewayName, - "namespace": gatewayNamespace, + "group": "gateway.networking.k8s.io", + "kind": "Gateway", + "name": gatewayName, }, // "when" must live inside "defaults" (not at spec level) because Kuadrant treats // top-level "when" as implicit defaults, which conflicts with explicit "defaults". @@ -1281,9 +1285,75 @@ func (r *MaaSAuthPolicyReconciler) gatewayAuthPolicyReady(ctx context.Context, g return ready, message, nil } +// specMatchesDesired reports whether the current spec (from the API server) +// contains all fields present in the desired spec with equal values. Fields +// added by the API server or its controllers (e.g. Kuadrant defaults like +// "allValues", "strategy") are ignored — only the fields we explicitly set +// are compared. Both sides are JSON-round-tripped first so Go type +// differences (int64 vs float64) are normalised. +func specMatchesDesired(desired, current map[string]any) bool { + desiredJSON, err := json.Marshal(desired) + if err != nil { + return false + } + currentJSON, err := json.Marshal(current) + if err != nil { + return false + } + var desiredNorm, currentNorm map[string]any + if err := json.Unmarshal(desiredJSON, &desiredNorm); err != nil { + return false + } + if err := json.Unmarshal(currentJSON, ¤tNorm); err != nil { + return false + } + stripExtraFields(currentNorm, desiredNorm) + return reflect.DeepEqual(desiredNorm, currentNorm) +} + +// stripExtraFields recursively removes keys from current that do not exist +// in desired, so that server-added defaults do not cause false mismatches. +func stripExtraFields(current, desired map[string]any) { + for k, cv := range current { + dv, exists := desired[k] + if !exists { + delete(current, k) + continue + } + if dMap, ok := dv.(map[string]any); ok { + if cMap, ok := cv.(map[string]any); ok { + stripExtraFields(cMap, dMap) + } + } + if dSlice, ok := dv.([]any); ok { + if cSlice, ok := cv.([]any); ok { + stripExtraFieldsSlice(cSlice, dSlice) + } + } + } +} + +func stripExtraFieldsSlice(current, desired []any) { + for i := 0; i < len(current) && i < len(desired); i++ { + if dMap, ok := desired[i].(map[string]any); ok { + if cMap, ok := current[i].(map[string]any); ok { + stripExtraFields(cMap, dMap) + } + } + if dSlice, ok := desired[i].([]any); ok { + if cSlice, ok := current[i].([]any); ok { + stripExtraFieldsSlice(cSlice, dSlice) + } + } + } +} + // reconcileGatewayAuthPolicy creates or updates the singleton Gateway-level AuthPolicy in // the gateway namespace. All MaaSAuthPolicy reconciliations converge on this one resource. -func (r *MaaSAuthPolicyReconciler) reconcileGatewayAuthPolicy(ctx context.Context, log logr.Logger, modelAccessJSON string, oidc *oidcConfig, xAPIKeyEnabled bool, tenantID, gatewayNamespace, gatewayName string) error { +func (r *MaaSAuthPolicyReconciler) reconcileGatewayAuthPolicy( + ctx context.Context, log logr.Logger, modelAccessJSON string, + oidc *oidcConfig, xAPIKeyEnabled bool, tenantID, gatewayNamespace, gatewayName string, +) (bool, error) { log.Info("reconcileGatewayAuthPolicy entered", "gatewayNamespace", gatewayNamespace, "gatewayName", gatewayName, "tenantID", tenantID, "xAPIKeyEnabled", xAPIKeyEnabled) // Calculate tenantName from tenantID @@ -1316,7 +1386,7 @@ func (r *MaaSAuthPolicyReconciler) reconcileGatewayAuthPolicy(ctx context.Contex existing.SetGroupVersionKind(gwPolicy.GroupVersionKind()) err := r.Get(ctx, client.ObjectKeyFromObject(gwPolicy), existing) if err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to get gateway AuthPolicy: %w", err) + return false, fmt.Errorf("failed to get gateway AuthPolicy: %w", err) } existingFound := err == nil @@ -1334,14 +1404,14 @@ func (r *MaaSAuthPolicyReconciler) reconcileGatewayAuthPolicy(ctx context.Contex // delete it to prevent orphaned resources. if existingFound && isManaged(existing) { if delErr := r.Delete(ctx, existing); delErr != nil { - return fmt.Errorf("failed to delete stale tenant gateway AuthPolicy %s/%s: %w", gatewayNamespace, authPolicyName, delErr) + return false, fmt.Errorf("failed to delete stale tenant gateway AuthPolicy %s/%s: %w", gatewayNamespace, authPolicyName, delErr) } log.Info("deleted stale tenant gateway AuthPolicy (Gateway no longer exists)", "name", authPolicyName, "namespace", gatewayNamespace) } // Nothing to create or update without a Gateway. - return nil + return false, nil } - return fmt.Errorf("failed to get Gateway %s/%s for OwnerReference: %w", gatewayNamespace, gatewayName, gwErr) + return false, fmt.Errorf("failed to get Gateway %s/%s for OwnerReference: %w", gatewayNamespace, gatewayName, gwErr) } } @@ -1351,41 +1421,41 @@ func (r *MaaSAuthPolicyReconciler) reconcileGatewayAuthPolicy(ctx context.Contex setGatewayOwnerReference(gateway, gwPolicy) } if err := unstructured.SetNestedMap(gwPolicy.Object, spec, "spec"); err != nil { - return fmt.Errorf("failed to set gateway AuthPolicy spec: %w", err) + return false, fmt.Errorf("failed to set gateway AuthPolicy spec: %w", err) } if err := r.Create(ctx, gwPolicy); err != nil { - return fmt.Errorf("failed to create gateway AuthPolicy: %w", err) + return false, fmt.Errorf("failed to create gateway AuthPolicy: %w", err) } log.Info("gateway AuthPolicy created", "name", authPolicyName, "namespace", gatewayNamespace) r.deleteGatewayDefaultAuthPolicy(ctx, log) - return nil + return true, nil } if !isManaged(existing) { log.Info("gateway AuthPolicy opted out of management, skipping", "name", authPolicyName) - return nil + return false, nil } - snapshot := existing.DeepCopy() + currentSpec, _, _ := unstructured.NestedMap(existing.Object, "spec") if err := unstructured.SetNestedMap(existing.Object, spec, "spec"); err != nil { - return fmt.Errorf("failed to set gateway AuthPolicy spec for update: %w", err) + return false, fmt.Errorf("failed to set gateway AuthPolicy spec for update: %w", err) } // Ensure OwnerReferences are set on existing tenant gateway AuthPolicies // (handles upgrade from pre-ownerref versions). if isTenantGateway { setGatewayOwnerReference(gateway, existing) } - if equality.Semantic.DeepEqual(snapshot.Object, existing.Object) { + if specMatchesDesired(spec, currentSpec) { log.Info("gateway AuthPolicy unchanged, skipping update", "name", authPolicyName) r.deleteGatewayDefaultAuthPolicy(ctx, log) - return nil + return false, nil } if err := r.Update(ctx, existing); err != nil { - return fmt.Errorf("failed to update gateway AuthPolicy: %w", err) + return false, fmt.Errorf("failed to update gateway AuthPolicy: %w", err) } log.Info("gateway AuthPolicy updated", "name", authPolicyName, "namespace", gatewayNamespace) r.deleteGatewayDefaultAuthPolicy(ctx, log) - return nil + return true, nil } // reconcileModelAuthPolicies creates or updates the per-model group-membership AuthPolicy for @@ -2012,6 +2082,7 @@ func (r *MaaSAuthPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { }) b := ctrl.NewControllerManagedBy(mgr). + WithOptions(controller.Options{MaxConcurrentReconciles: max(1, r.MaxConcurrentReconciles)}). For(&maasv1alpha1.MaaSAuthPolicy{}, builder.WithPredicates(predicate.Or( predicate.GenerationChangedPredicate{}, predicate.Funcs{UpdateFunc: deletionTimestampSet}, diff --git a/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go b/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go index 23430b1ef..5928f17ea 100644 --- a/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go +++ b/maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go @@ -1887,9 +1887,9 @@ func TestBuildGatewayAuthPolicySpec_XAPIKeyEnabled(t *testing.T) { if !ok { t.Fatalf("api-keys-x-api-key is not a map: %T", xAPIKey) } - expr, _, _ := unstructured.NestedString(xAPIKeyMap, "plain", "expression") - if !contains(expr, "x-api-key") { - t.Errorf("api-keys-x-api-key plain.expression should reference x-api-key header, got: %s", expr) + sel, _, _ := unstructured.NestedString(xAPIKeyMap, "plain", "selector") + if sel != "request.headers.x-api-key" { + t.Errorf("api-keys-x-api-key plain.selector should be request.headers.x-api-key, got: %s", sel) } priority, ok := xAPIKeyMap["priority"].(int64) @@ -2119,6 +2119,7 @@ func TestMaaSAuthPolicyReconciler_MissingModelRef_FailedPhase(t *testing.T) { namespace = "default" maasAuthName = "auth-missing" missingModel = "non-existent-model" + gatewayNS = "openshift-ingress" ) // Create auth policy referencing a non-existent model @@ -2128,15 +2129,16 @@ func TestMaaSAuthPolicyReconciler_MissingModelRef_FailedPhase(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(maasAuth). + WithObjects(maasAuth, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() r := &MaaSAuthPolicyReconciler{ - Client: c, - Scheme: scheme, - InfraNamespace: namespace, - GatewayName: "openshift-ingress/maas-default-gateway", + Client: c, + Scheme: scheme, + InfraNamespace: namespace, + GatewayNamespace: gatewayNS, + GatewayName: "maas-default-gateway", } req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasAuthName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { @@ -2173,6 +2175,7 @@ func TestMaaSAuthPolicyReconciler_PartialModelRefs_DegradedPhase(t *testing.T) { validModel = "valid-model" missingModel = "missing-model" httpRouteName = "maas-" + validModel + gatewayNS = "openshift-ingress" ) // Create valid model and route @@ -2187,15 +2190,16 @@ func TestMaaSAuthPolicyReconciler_PartialModelRefs_DegradedPhase(t *testing.T) { c := fake.NewClientBuilder(). WithScheme(scheme). WithRESTMapper(testRESTMapper()). - WithObjects(model, route, maasAuth). + WithObjects(model, route, maasAuth, newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName)). WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). Build() r := &MaaSAuthPolicyReconciler{ - Client: c, - Scheme: scheme, - InfraNamespace: namespace, - GatewayName: "openshift-ingress/maas-default-gateway", + Client: c, + Scheme: scheme, + InfraNamespace: namespace, + GatewayNamespace: gatewayNS, + GatewayName: "maas-default-gateway", } req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasAuthName, Namespace: namespace}} if _, err := r.Reconcile(context.Background(), req); err != nil { @@ -3030,3 +3034,144 @@ func TestMaaSAuthPolicyReconciler_TenantGateway_StaleCleanup_UnmanagedPreserved( t.Fatalf("expected unmanaged stale tenant gateway AuthPolicy %q to be preserved, but Get returned error: %v", staleAuthPolicyName, getErr) } } + +// TestMaaSAuthPolicyReconciler_RequeuesUntilEnforcedAfterUpdate verifies that +// the controller requeues until the gateway AuthPolicy is both Accepted and +// Enforced after creating or updating it — not just during legacy upgrades. +func TestMaaSAuthPolicyReconciler_RequeuesUntilEnforcedAfterUpdate(t *testing.T) { + const ( + modelName = "llm" + namespace = "default" + gatewayNS = "openshift-ingress" + maasPolicyName = "policy-a" + ) + + model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) + route := newHTTPRoute("maas-"+modelName, namespace) + maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", + maasv1alpha1.ModelRef{Name: modelName, Namespace: namespace}) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(testRESTMapper()). + WithObjects(model, route, maasPolicy). + WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). + Build() + + r := &MaaSAuthPolicyReconciler{ + Client: c, + Scheme: scheme, + InfraNamespace: "maas-system", + GatewayNamespace: gatewayNS, + GatewayName: "maas-default-gateway", + } + ctx := context.Background() + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} + + // Step 1: First reconcile creates the gateway AuthPolicy. No status yet → requeue. + result, err := r.Reconcile(ctx, req) + if err != nil { + t.Fatalf("step 1: Reconcile error: %v", err) + } + if result.RequeueAfter != 10*time.Second { + t.Fatalf("step 1: RequeueAfter = %s, want 10s (gateway AuthPolicy not yet enforced)", result.RequeueAfter) + } + + // Verify the gateway AuthPolicy was created. + gwAP := &unstructured.Unstructured{} + gwAP.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"}) + if err := c.Get(ctx, types.NamespacedName{Name: maasGatewayAuthPolicyName, Namespace: gatewayNS}, gwAP); err != nil { + t.Fatalf("step 1: gateway AuthPolicy not found: %v", err) + } + + // Step 2: Set Accepted=True but Enforced=False → still requeues. + _ = unstructured.SetNestedSlice(gwAP.Object, []any{ + map[string]any{"type": "Accepted", "status": "True"}, + map[string]any{"type": "Enforced", "status": "False"}, + }, "status", "conditions") + _ = unstructured.SetNestedField(gwAP.Object, gwAP.GetGeneration(), "status", "observedGeneration") + if err := c.Update(ctx, gwAP); err != nil { + t.Fatalf("step 2: update gateway AuthPolicy status: %v", err) + } + + result, err = r.Reconcile(ctx, req) + if err != nil { + t.Fatalf("step 2: Reconcile error: %v", err) + } + if result.RequeueAfter != 10*time.Second { + t.Fatalf("step 2: RequeueAfter = %s, want 10s (Enforced still False)", result.RequeueAfter) + } + + // Step 3: Set both Accepted=True and Enforced=True → reconcile completes. + if err := c.Get(ctx, types.NamespacedName{Name: maasGatewayAuthPolicyName, Namespace: gatewayNS}, gwAP); err != nil { + t.Fatalf("step 3: get gateway AuthPolicy: %v", err) + } + _ = unstructured.SetNestedSlice(gwAP.Object, []any{ + map[string]any{"type": "Accepted", "status": "True"}, + map[string]any{"type": "Enforced", "status": "True"}, + }, "status", "conditions") + _ = unstructured.SetNestedField(gwAP.Object, gwAP.GetGeneration(), "status", "observedGeneration") + if err := c.Update(ctx, gwAP); err != nil { + t.Fatalf("step 3: update gateway AuthPolicy status: %v", err) + } + + result, err = r.Reconcile(ctx, req) + if err != nil { + t.Fatalf("step 3: Reconcile error: %v", err) + } + if result.RequeueAfter != 0 { + t.Fatalf("step 3: RequeueAfter = %s, want 0 (gateway AuthPolicy is enforced)", result.RequeueAfter) + } +} + +// TestMaaSAuthPolicyReconciler_NoRequeueWhenUnchanged verifies that when the +// gateway AuthPolicy spec is already up to date, the controller does not +// requeue for enforcement — the policy is already enforced from a prior reconcile. +func TestMaaSAuthPolicyReconciler_NoRequeueWhenUnchanged(t *testing.T) { + const ( + modelName = "llm" + namespace = "default" + gatewayNS = "openshift-ingress" + maasPolicyName = "policy-a" + ) + + model := newMaaSModelRef(modelName, namespace, "ExternalModel", modelName) + route := newHTTPRoute("maas-"+modelName, namespace) + maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a", + maasv1alpha1.ModelRef{Name: modelName, Namespace: namespace}) + readyGatewayPolicy := newReadyGatewayAuthPolicy(gatewayNS, maasGatewayAuthPolicyName) + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithRESTMapper(testRESTMapper()). + WithObjects(model, route, maasPolicy, readyGatewayPolicy). + WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}). + Build() + + r := &MaaSAuthPolicyReconciler{ + Client: c, + Scheme: scheme, + InfraNamespace: "maas-system", + GatewayNamespace: gatewayNS, + GatewayName: "maas-default-gateway", + } + ctx := context.Background() + req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}} + + // First reconcile: updates the gateway AuthPolicy with real spec. + // Since the pre-populated policy has no real spec, this IS a change. + // But the policy is pre-populated with ready conditions, so enforcement + // check should pass. + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("first Reconcile: %v", err) + } + + // Second reconcile: same content, no change → should not requeue. + result, err := r.Reconcile(ctx, req) + if err != nil { + t.Fatalf("second Reconcile: %v", err) + } + if result.RequeueAfter != 0 { + t.Errorf("second Reconcile: RequeueAfter = %s, want 0 (no spec change, no enforcement wait needed)", result.RequeueAfter) + } +} diff --git a/maas-controller/pkg/controller/maas/maassubscription_controller.go b/maas-controller/pkg/controller/maas/maassubscription_controller.go index 1ea8db611..c2537baad 100644 --- a/maas-controller/pkg/controller/maas/maassubscription_controller.go +++ b/maas-controller/pkg/controller/maas/maassubscription_controller.go @@ -40,6 +40,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -67,6 +68,9 @@ type MaaSSubscriptionReconciler struct { // Tenant does not yet carry spec.gatewayRef. GatewayName string GatewayNamespace string + // MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. + // Defaults to 1 if not set. + MaxConcurrentReconciles int } //+kubebuilder:rbac:groups=maas.opendatahub.io,resources=maassubscriptions,verbs=get;list;watch;create;update;patch;delete @@ -1068,6 +1072,7 @@ func (r *MaaSSubscriptionReconciler) SetupWithManager(mgr ctrl.Manager) error { } b := ctrl.NewControllerManagedBy(mgr). + WithOptions(controller.Options{MaxConcurrentReconciles: max(1, r.MaxConcurrentReconciles)}). For(&maasv1alpha1.MaaSSubscription{}, builder.WithPredicates(predicate.Or( predicate.GenerationChangedPredicate{}, predicate.Funcs{UpdateFunc: deletionTimestampSet}, diff --git a/maas-controller/pkg/controller/maas/providers_external.go b/maas-controller/pkg/controller/maas/providers_external.go index c136297c5..10a43a610 100644 --- a/maas-controller/pkg/controller/maas/providers_external.go +++ b/maas-controller/pkg/controller/maas/providers_external.go @@ -227,15 +227,14 @@ func (h *externalModelHandler) Status(ctx context.Context, log logr.Logger, mode return endpoint, true, nil } -// GetModelEndpoint returns the endpoint URL for the ExternalModel. -// Uses ExternalModel name (spec.modelRef.name) in the path to match IPP's -// model-provider-resolver store key. The HTTPRoute object name itself is -// MaaS-prefixed to avoid colliding with the upstream inference ExternalModel controller. +// GetModelEndpoint returns the shared gateway base URL for the ExternalModel. +// Matches LLMInferenceService BBR catalog URLs (https://{gatewayHost}) so clients +// use one base_url and select the model via body.model / X-Gateway-Model-Name. +// Path-based HTTPRoute rules remain for backward-compatible clients. func (h *externalModelHandler) GetModelEndpoint(ctx context.Context, log logr.Logger, model *maasv1alpha1.MaaSModelRef) (string, error) { extModelName := model.Spec.ModelRef.Name if len(model.Status.HTTPRouteHostnames) > 0 { - hostname := model.Status.HTTPRouteHostnames[0] - return fmt.Sprintf("https://%s/%s/%s", hostname, model.Namespace, extModelName), nil + return fmt.Sprintf("https://%s", model.Status.HTTPRouteHostnames[0]), nil } gatewayName := h.r.gatewayName() @@ -248,19 +247,19 @@ func (h *externalModelHandler) GetModelEndpoint(ctx context.Context, log logr.Lo for _, listener := range gateway.Spec.Listeners { if listener.Hostname != nil { - return fmt.Sprintf("https://%s/%s/%s", string(*listener.Hostname), model.Namespace, extModelName), nil + return fmt.Sprintf("https://%s", string(*listener.Hostname)), nil } } for _, addr := range gateway.Status.Addresses { if addr.Type != nil && *addr.Type == gatewayapiv1.HostnameAddressType { - return fmt.Sprintf("https://%s/%s/%s", addr.Value, model.Namespace, extModelName), nil + return fmt.Sprintf("https://%s", addr.Value), nil } } if len(gateway.Status.Addresses) > 0 { log.Info("Using IP-based gateway address; TLS hostname verification may fail", "address", gateway.Status.Addresses[0].Value, "model", extModelName) - return fmt.Sprintf("https://%s/%s/%s", gateway.Status.Addresses[0].Value, model.Namespace, extModelName), nil + return fmt.Sprintf("https://%s", gateway.Status.Addresses[0].Value), nil } return "", fmt.Errorf("unable to determine endpoint: gateway %s/%s has no hostname or addresses", gatewayNS, gatewayName) diff --git a/maas-controller/pkg/controller/maas/providers_external_test.go b/maas-controller/pkg/controller/maas/providers_external_test.go index 7e99c2444..a98d0d95f 100644 --- a/maas-controller/pkg/controller/maas/providers_external_test.go +++ b/maas-controller/pkg/controller/maas/providers_external_test.go @@ -203,8 +203,8 @@ func TestExternalModel_Status_Ready(t *testing.T) { if !ready { t.Error("Status: ready = false, want true") } - if endpoint != "https://maas.example.com/default/gpt-4o" { - t.Errorf("Status: endpoint = %q, want %q", endpoint, "https://maas.example.com/default/gpt-4o") + if endpoint != "https://maas.example.com" { + t.Errorf("Status: endpoint = %q, want %q", endpoint, "https://maas.example.com") } } @@ -254,8 +254,8 @@ func TestExternalModel_GetModelEndpoint_FromHostnames(t *testing.T) { if err != nil { t.Fatalf("GetModelEndpoint: unexpected error: %v", err) } - if endpoint != "https://maas.example.com/default/claude-sonnet" { - t.Errorf("GetModelEndpoint = %q, want %q", endpoint, "https://maas.example.com/default/claude-sonnet") + if endpoint != "https://maas.example.com" { + t.Errorf("GetModelEndpoint = %q, want %q", endpoint, "https://maas.example.com") } } @@ -273,8 +273,8 @@ func TestExternalModel_GetModelEndpoint_FromGateway(t *testing.T) { if err != nil { t.Fatalf("GetModelEndpoint: unexpected error: %v", err) } - if endpoint != "https://maas.cluster.example.com/default/gpt-4o" { - t.Errorf("GetModelEndpoint = %q, want %q", endpoint, "https://maas.cluster.example.com/default/gpt-4o") + if endpoint != "https://maas.cluster.example.com" { + t.Errorf("GetModelEndpoint = %q, want %q", endpoint, "https://maas.cluster.example.com") } } diff --git a/maas-controller/pkg/controller/maas/self_deployment_controller_test.go b/maas-controller/pkg/controller/maas/self_deployment_controller_test.go index 05904e12a..26e6302ff 100644 --- a/maas-controller/pkg/controller/maas/self_deployment_controller_test.go +++ b/maas-controller/pkg/controller/maas/self_deployment_controller_test.go @@ -304,6 +304,39 @@ func TestLifecycleReconciler_TeardownRequestedWithoutConfigRequestsOrphanCleanup g.Expect(updatedDep.Annotations[TeardownCompletedAnnotation]).To(BeEmpty()) } +func TestLifecycleReconciler_TeardownClearsBootstrapMarkerBeforeAITenantCleanupCompletes(t *testing.T) { + g := NewWithT(t) + s := lifecycleTestScheme(t) + + cfg := &maasv1alpha1.Config{ + ObjectMeta: metav1.ObjectMeta{ + Name: maasv1alpha1.ConfigInstanceName, + Annotations: map[string]string{ + DefaultAITenantBootstrappedAnnotation: "true", + "example.com/preserved": "true", + }, + }, + } + aitenant := lifecycleTestUnstructured( + schema.GroupVersionKind{Group: "maas.opendatahub.io", Version: "v1alpha1", Kind: "AITenant"}, + tenantreconcile.DefaultAITenantNamespace, + tenantreconcile.DefaultAITenantName, + aitenantFinalizer, + ) + + cl := fake.NewClientBuilder().WithScheme(s).WithRuntimeObjects(cfg, aitenant).Build() + r := &LifecycleReconciler{Client: cl, Scheme: s} + + res, err := r.handleRequestedTeardown(context.Background(), nil, cfg) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(teardownRequeueAfter)) + + var updatedCfg maasv1alpha1.Config + g.Expect(cl.Get(context.Background(), client.ObjectKey{Name: maasv1alpha1.ConfigInstanceName}, &updatedCfg)).To(Succeed()) + g.Expect(updatedCfg.Annotations).NotTo(HaveKey(DefaultAITenantBootstrappedAnnotation)) + g.Expect(updatedCfg.Annotations["example.com/preserved"]).To(Equal("true")) +} + func TestLifecycleReconciler_NormalReconcileDoesNotSetDeploymentOwnerReference(t *testing.T) { g := NewWithT(t) s := lifecycleTestScheme(t) diff --git a/maas-controller/pkg/controller/maas/self_teardown.go b/maas-controller/pkg/controller/maas/self_teardown.go index e714db2db..4aa78b5bb 100644 --- a/maas-controller/pkg/controller/maas/self_teardown.go +++ b/maas-controller/pkg/controller/maas/self_teardown.go @@ -60,6 +60,10 @@ var resourceTypesToRemove = []schema.GroupVersionKind{ // If the process crashes between the two steps, the next reconcile finds nothing pending // and no Config, and simply (idempotently) marks completion. func (r *LifecycleReconciler) handleRequestedTeardown(ctx context.Context, dep *appsv1.Deployment, cfg *maasv1alpha1.Config) (ctrl.Result, error) { + if err := r.clearDefaultAITenantBootstrapMarker(ctx, cfg); err != nil { + return ctrl.Result{}, err + } + pending, err := r.cleanupTeardownResources(ctx) if err != nil { return ctrl.Result{}, err @@ -81,6 +85,22 @@ func (r *LifecycleReconciler) handleRequestedTeardown(ctx context.Context, dep * return ctrl.Result{}, nil } +// clearDefaultAITenantBootstrapMarker allows a later install to recreate the default +// AITenant if teardown is interrupted after deleting AITenants but before deleting +// Config/default. Bootstrap remains disabled while teardown is requested. +func (r *LifecycleReconciler) clearDefaultAITenantBootstrapMarker(ctx context.Context, cfg *maasv1alpha1.Config) error { + if cfg == nil || cfg.GetAnnotations()[DefaultAITenantBootstrappedAnnotation] == "" { + return nil + } + + base := cfg.DeepCopy() + delete(cfg.Annotations, DefaultAITenantBootstrappedAnnotation) + if err := r.Patch(ctx, cfg, client.MergeFrom(base)); err != nil { + return fmt.Errorf("clear default AITenant bootstrap marker during teardown: %w", err) + } + return nil +} + // markTeardownCompleted sets TeardownCompletedAnnotation on the Deployment so external // operators have a single durable signal for "self-teardown is done" that survives // Config (and anything cascade-deleted through it) disappearing. Idempotent so patching diff --git a/maas-controller/pkg/controller/maas/tenant_controller.go b/maas-controller/pkg/controller/maas/tenant_controller.go index 2ca52f24d..e26d065f1 100644 --- a/maas-controller/pkg/controller/maas/tenant_controller.go +++ b/maas-controller/pkg/controller/maas/tenant_controller.go @@ -103,6 +103,7 @@ type TenantReconciler struct { // +kubebuilder:rbac:groups=telemetry.istio.io,resources=telemetries,verbs=get;list;watch;create;patch;delete // +kubebuilder:rbac:groups=batch,resources=cronjobs,verbs=get;list;watch;create;patch;delete // +kubebuilder:rbac:groups=monitoring.coreos.com,resources=podmonitors;servicemonitors,verbs=get;list;watch;create;patch;delete +// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;patch;delete // clusterroles/clusterrolebindings: TenantReconciler SSA-applies the maas-api and payload-processing-reader // ClusterRoles. The API-server escalation check requires the applying SA to already hold every permission those diff --git a/maas-controller/pkg/controller/maas/tenant_reconcile.go b/maas-controller/pkg/controller/maas/tenant_reconcile.go index 15fedee89..8f6b4705a 100644 --- a/maas-controller/pkg/controller/maas/tenant_reconcile.go +++ b/maas-controller/pkg/controller/maas/tenant_reconcile.go @@ -386,7 +386,7 @@ func (r *TenantReconciler) setFinalStatus(ctx context.Context, tenant *maasv1alp if apimeta.IsStatusConditionTrue(tenant.Status.Conditions, tenantreconcile.ConditionTypeDegraded) { tenant.Status.Phase = "Degraded" } - setDeploymentsAvailableCondition(tenant, true, "DeploymentsReady", "maas-api deployment is available") + setDeploymentsAvailableCondition(tenant, true, "DeploymentsReady", "maas-api deployment and payload-processing EnvoyFilter are available") apimeta.SetStatusCondition(&tenant.Status.Conditions, metav1.Condition{ Type: tenantreconcile.ReadyConditionType, Status: metav1.ConditionTrue, diff --git a/maas-controller/pkg/platform/tenantreconcile/apply.go b/maas-controller/pkg/platform/tenantreconcile/apply.go index 2fe454d1e..5c5244e64 100644 --- a/maas-controller/pkg/platform/tenantreconcile/apply.go +++ b/maas-controller/pkg/platform/tenantreconcile/apply.go @@ -37,6 +37,10 @@ func ApplyRendered(ctx context.Context, c client.Client, scheme *runtime.Scheme, // Skip resources whose live cluster copy has opendatahub.io/managed=false, // allowing operators to opt specific resources out of reconciliation. + // The payload-processing-plugins ConfigMap is stamped with managed=false on + // bootstrap/migrate (see preparePayloadProcessingPluginsConfigMapApply) so + // subsequent reconciles leave user plugin edits alone unless they set + // opendatahub.io/managed=true to opt back into continuous management. if isLiveResourceUnmanaged(ctx, c, u) { ctrl.LoggerFrom(ctx).V(1).Info("Skipping SSA for resource with opendatahub.io/managed=false on cluster", "kind", u.GetKind(), "name", u.GetName(), "namespace", u.GetNamespace()) @@ -68,6 +72,7 @@ func ApplyRendered(ctx context.Context, c client.Client, scheme *runtime.Scheme, } setTenantTrackingLabels(u, tenant) } + preparePayloadProcessingPluginsConfigMapApply(ctx, c, u) unstructured.RemoveNestedField(u.Object, "metadata", "managedFields") unstructured.RemoveNestedField(u.Object, "metadata", "resourceVersion") unstructured.RemoveNestedField(u.Object, "status") @@ -119,6 +124,52 @@ func isLiveResourceUnmanaged(ctx context.Context, c client.Client, rendered *uns return ann != nil && ann[AnnotationManaged] == "false" } +// isPayloadProcessingPluginsConfigMap reports whether u is the IPP plugins ConfigMap +// (default name or per-tenant suffix). +func isPayloadProcessingPluginsConfigMap(u *unstructured.Unstructured) bool { + if u == nil || u.GetKind() != "ConfigMap" { + return false + } + name := u.GetName() + if name == PayloadProcessingPluginsConfigMapName { + return true + } + return strings.HasPrefix(name, PayloadProcessingPluginsConfigMapName+"-") +} + +// preparePayloadProcessingPluginsConfigMapApply stamps opendatahub.io/managed on the +// plugins ConfigMap about to be SSA'd: +// - managed=false (default): next reconcile skips via isLiveResourceUnmanaged so +// operators can edit response plugins (e.g. re-enable api-translation) without +// the controller overwriting the ConfigMap. +// - managed=true preserved when the live object already opted into continuous +// reconciler management. +// +// Do not put managed=false in the source YAML: PostRender drops resources that +// already carry that annotation, which would prevent first-time creation. +func preparePayloadProcessingPluginsConfigMapApply(ctx context.Context, c client.Client, u *unstructured.Unstructured) { + if !isPayloadProcessingPluginsConfigMap(u) { + return + } + managedValue := "false" + live := &unstructured.Unstructured{} + live.SetGroupVersionKind(u.GroupVersionKind()) + key := client.ObjectKeyFromObject(u) + if key.Name != "" { + if err := c.Get(ctx, key, live); err == nil { + if ann := live.GetAnnotations(); ann != nil && ann[AnnotationManaged] == "true" { + managedValue = "true" + } + } + } + ann := u.GetAnnotations() + if ann == nil { + ann = make(map[string]string) + } + ann[AnnotationManaged] = managedValue + u.SetAnnotations(ann) +} + // isOwnedByExternalController returns true when the live cluster copy of the // rendered resource has a controller:true ownerReference whose UID differs from // the given Config UID. This prevents the tenant config reconciler from SSA-applying diff --git a/maas-controller/pkg/platform/tenantreconcile/apply_plugins_configmap_test.go b/maas-controller/pkg/platform/tenantreconcile/apply_plugins_configmap_test.go new file mode 100644 index 000000000..9c1bdb8a5 --- /dev/null +++ b/maas-controller/pkg/platform/tenantreconcile/apply_plugins_configmap_test.go @@ -0,0 +1,98 @@ +package tenantreconcile + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestIsPayloadProcessingPluginsConfigMap(t *testing.T) { + t.Parallel() + + cm := func(name string) *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(GVKConfigMap) + u.SetName(name) + return u + } + + assert.True(t, isPayloadProcessingPluginsConfigMap(cm(PayloadProcessingPluginsConfigMapName))) + assert.True(t, isPayloadProcessingPluginsConfigMap(cm(PayloadProcessingPluginsConfigMapForTenant("redteam")))) + assert.False(t, isPayloadProcessingPluginsConfigMap(cm("other-config"))) + assert.False(t, isPayloadProcessingPluginsConfigMap(cm(PayloadProcessingPluginsConfigMapName+"extra"))) + dep := &unstructured.Unstructured{} + dep.SetKind("Deployment") + dep.SetName(PayloadProcessingPluginsConfigMapName) + assert.False(t, isPayloadProcessingPluginsConfigMap(dep)) +} + +func TestPreparePayloadProcessingPluginsConfigMapApply(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + newRendered := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(GVKConfigMap) + u.SetNamespace("openshift-ingress") + u.SetName(PayloadProcessingPluginsConfigMapName) + return u + } + + t.Run("stamps managed=false when ConfigMap does not exist", func(t *testing.T) { + t.Parallel() + c := fake.NewClientBuilder().WithScheme(scheme).Build() + u := newRendered() + preparePayloadProcessingPluginsConfigMapApply(context.Background(), c, u) + assert.Equal(t, "false", u.GetAnnotations()[AnnotationManaged]) + }) + + t.Run("stamps managed=false when live has no managed annotation", func(t *testing.T) { + t.Parallel() + live := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: PayloadProcessingPluginsConfigMapName, + Namespace: "openshift-ingress", + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(live).Build() + u := newRendered() + preparePayloadProcessingPluginsConfigMapApply(context.Background(), c, u) + assert.Equal(t, "false", u.GetAnnotations()[AnnotationManaged]) + }) + + t.Run("preserves managed=true when live opted into management", func(t *testing.T) { + t.Parallel() + live := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: PayloadProcessingPluginsConfigMapName, + Namespace: "openshift-ingress", + Annotations: map[string]string{ + AnnotationManaged: "true", + }, + }, + } + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(live).Build() + u := newRendered() + preparePayloadProcessingPluginsConfigMapApply(context.Background(), c, u) + assert.Equal(t, "true", u.GetAnnotations()[AnnotationManaged]) + }) + + t.Run("ignores non-plugins ConfigMaps", func(t *testing.T) { + t.Parallel() + c := fake.NewClientBuilder().WithScheme(scheme).Build() + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(GVKConfigMap) + u.SetName("unrelated") + preparePayloadProcessingPluginsConfigMapApply(context.Background(), c, u) + assert.Nil(t, u.GetAnnotations()) + }) +} diff --git a/maas-controller/pkg/platform/tenantreconcile/constants.go b/maas-controller/pkg/platform/tenantreconcile/constants.go index 4cc88d922..6f3a5e8a4 100644 --- a/maas-controller/pkg/platform/tenantreconcile/constants.go +++ b/maas-controller/pkg/platform/tenantreconcile/constants.go @@ -78,6 +78,7 @@ const ( baseMaaSAPIServiceName = "maas-api" baseMaaSAPIKeyCleanupScriptConfigMapName = "maas-api-key-cleanup-script" //nolint:gosec // Kubernetes resource name, not a credential baseMaaSAPIDeploymentNSNetworkPolicyName = "maas-api-allow-deployment-ns" + baseMaaSAPIServingCertName = "maas-api-serving-cert" // Base IPP resource names in kustomize manifests. Per-tenant deployments suffix // these with "-{tenantID}" (default tenant keeps unsuffixed names). @@ -85,6 +86,9 @@ const ( PayloadPreProcessingName = "payload-pre-processing" PayloadProcessingPluginsConfigMapName = "payload-processing-plugins" PayloadProcessingReaderClusterRoleBindingName = "payload-processing-reader" + // PayloadProcessingEnvoyFilterPriority runs after Kuadrant's default-priority (0) + // EnvoyFilter so RHCL's envoy.filters.http.wasm anchor exists when we INSERT_*. + PayloadProcessingEnvoyFilterPriority int64 = 10 // LabelTenantInstance distinguishes pods when multiple IPP stacks share a gateway namespace. LabelTenantInstance = "maas.opendatahub.io/tenant-instance" @@ -121,6 +125,7 @@ var ( GVKNetworkPolicy = schema.GroupVersionKind{Group: "networking.k8s.io", Version: "v1", Kind: "NetworkPolicy"} GVKPersesDashboard = schema.GroupVersionKind{Group: "perses.dev", Version: "v1alpha1", Kind: "PersesDashboard"} GVKPersesDatasource = schema.GroupVersionKind{Group: "perses.dev", Version: "v1alpha1", Kind: "PersesDatasource"} + GVKCertificate = schema.GroupVersionKind{Group: "cert-manager.io", Version: "v1", Kind: "Certificate"} ) // Resource naming functions for multi-tenant deployment. @@ -225,6 +230,10 @@ func PayloadProcessingNetworkPolicyName(tenantID string) string { return resourceNameForTenant(PayloadProcessingName, tenantID) } +func MaaSAPIServingCertName(tenantID string) string { + return resourceNameForTenant(baseMaaSAPIServingCertName, tenantID) +} + func PayloadProcessingReaderClusterRoleBindingNameForTenant(tenantID string) string { return resourceNameForTenant(PayloadProcessingReaderClusterRoleBindingName, tenantID) } diff --git a/maas-controller/pkg/platform/tenantreconcile/kustomize.go b/maas-controller/pkg/platform/tenantreconcile/kustomize.go index 009c5b9f7..f107e594b 100644 --- a/maas-controller/pkg/platform/tenantreconcile/kustomize.go +++ b/maas-controller/pkg/platform/tenantreconcile/kustomize.go @@ -182,3 +182,17 @@ func DefaultManifestPath() string { } return "../maas-api/deploy/overlays/odh" } + +// ManifestPathForPlatform returns the appropriate kustomize overlay path based on +// whether the cluster is OpenShift (isOCP=true) or vanilla Kubernetes (isOCP=false). +// The xKS overlay avoids OCP-specific resources like service-serving-certs and +// service-ca ConfigMap injection that don't exist on non-OpenShift clusters. +func ManifestPathForPlatform(isOCP bool) string { + if v := os.Getenv("MAAS_PLATFORM_MANIFESTS"); v != "" { + return v + } + if isOCP { + return "/maas-api/deploy/overlays/odh" + } + return "/maas-api/deploy/overlays/xks" +} diff --git a/maas-controller/pkg/platform/tenantreconcile/kustomize_test.go b/maas-controller/pkg/platform/tenantreconcile/kustomize_test.go new file mode 100644 index 000000000..b9716461a --- /dev/null +++ b/maas-controller/pkg/platform/tenantreconcile/kustomize_test.go @@ -0,0 +1,27 @@ +package tenantreconcile + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestManifestPathForPlatform(t *testing.T) { + t.Run("returns OCP overlay when isOCP is true", func(t *testing.T) { + t.Setenv("MAAS_PLATFORM_MANIFESTS", "") + path := ManifestPathForPlatform(true) + assert.Equal(t, "/maas-api/deploy/overlays/odh", path) + }) + + t.Run("returns xKS overlay when isOCP is false", func(t *testing.T) { + t.Setenv("MAAS_PLATFORM_MANIFESTS", "") + path := ManifestPathForPlatform(false) + assert.Equal(t, "/maas-api/deploy/overlays/xks", path) + }) + + t.Run("respects MAAS_PLATFORM_MANIFESTS override", func(t *testing.T) { + t.Setenv("MAAS_PLATFORM_MANIFESTS", "/custom/path") + path := ManifestPathForPlatform(true) + assert.Equal(t, "/custom/path", path) + }) +} diff --git a/maas-controller/pkg/platform/tenantreconcile/params.go b/maas-controller/pkg/platform/tenantreconcile/params.go index 22f620b5c..20c7aa1db 100644 --- a/maas-controller/pkg/platform/tenantreconcile/params.go +++ b/maas-controller/pkg/platform/tenantreconcile/params.go @@ -239,6 +239,8 @@ func patchResource(log logr.Logger, r *unstructured.Unstructured, params Platfor case gvk == GVKClusterRoleBinding && name == PayloadProcessingReaderClusterRoleBindingName: r.SetName(PayloadProcessingReaderClusterRoleBindingNameForTenant(tenantID)) return patchPayloadProcessingClusterRoleBinding(r, params) + case gvk == GVKCertificate && name == baseMaaSAPIServingCertName: + return patchMaaSAPIServingCert(log, r, params) } return nil } @@ -275,6 +277,35 @@ func patchDeploymentNSNetworkPolicy(r *unstructured.Unstructured, controllerName return unstructured.SetNestedSlice(r.Object, ingress, "spec", "ingress") } +// patchMaaSAPIServingCert remaps the Certificate's secretName and dnsNames to use +// the actual infra namespace (replacing the kustomize overlay's hardcoded "opendatahub" +// placeholder). This makes the controller self-sufficient for TLS — no dependency on +// the Helm chart hook to create the cert in the correct namespace. +func patchMaaSAPIServingCert(log logr.Logger, r *unstructured.Unstructured, params PlatformParams) error { + tenantID := params.TenantIdentifier + certName := MaaSAPIServingCertName(tenantID) + r.SetName(certName) + + secretName := certName + if err := unstructured.SetNestedField(r.Object, secretName, "spec", "secretName"); err != nil { + return fmt.Errorf("patch Certificate secretName: %w", err) + } + + serviceName := MaaSAPIServiceName(tenantID) + newDNSNames := []any{ + fmt.Sprintf("%s.%s.svc", serviceName, params.AppNamespace), + fmt.Sprintf("%s.%s.svc.cluster.local", serviceName, params.AppNamespace), + } + if err := unstructured.SetNestedSlice(r.Object, newDNSNames, "spec", "dnsNames"); err != nil { + return fmt.Errorf("patch Certificate dnsNames: %w", err) + } + + log.V(4).Info("Patched maas-api serving Certificate", + "name", certName, "secretName", secretName, + "dnsNames", newDNSNames, "namespace", params.AppNamespace) + return nil +} + func patchMaaSAPIDeployment(log logr.Logger, r *unstructured.Unstructured, params PlatformParams) error { if params.MaaSAPIReplicas != nil { if err := unstructured.SetNestedField(r.Object, int64(*params.MaaSAPIReplicas), "spec", "replicas"); err != nil { @@ -361,6 +392,11 @@ func patchPayloadProcessingDeployment(log logr.Logger, r *unstructured.Unstructu if err := setOrAddEnvVar(r, "payload-processing", "TENANT_NAMESPACE", params.SubscriptionNamespace); err != nil { return fmt.Errorf("patch TENANT_NAMESPACE: %w", err) } + if params.TenantIdentifier != "" { + if err := setOrAddEnvVar(r, "payload-processing", "DISABLE_EXTERNAL_MODEL_CONTROLLER", "true"); err != nil { + return fmt.Errorf("patch DISABLE_EXTERNAL_MODEL_CONTROLLER: %w", err) + } + } if err := addPodTemplateLabel(r, LabelTenantInstance, deploymentName); err != nil { return fmt.Errorf("patch tenant-instance label: %w", err) } @@ -610,22 +646,23 @@ func grpcClusterName(service, namespace string, port int) string { func patchPayloadProcessingEnvoyFilter(log logr.Logger, r *unstructured.Unstructured, params PlatformParams) error { r.SetNamespace(params.GatewayNamespace) - targetRefs, found, err := unstructured.NestedSlice(r.Object, "spec", "targetRefs") - if err != nil { - return fmt.Errorf("read EnvoyFilter targetRefs: %w", err) - } - if !found || len(targetRefs) == 0 { - return errors.New("EnvoyFilter targetRefs not found") + // Ensure we patch after Kuadrant's wasm INSERT (priority 0). Without this, + // RHCL subFilter matches on envoy.filters.http.wasm never fire — especially + // on secondary tenant gateways whose payload-processing EF is often created + // before Kuadrant's per-gateway EF (same priority 0 → creationTimestamp order). + if err := unstructured.SetNestedField(r.Object, PayloadProcessingEnvoyFilterPriority, "spec", "priority"); err != nil { + return fmt.Errorf("write EnvoyFilter priority: %w", err) } - ref, ok := targetRefs[0].(map[string]any) - if !ok { - return errors.New("EnvoyFilter targetRefs[0] is not an object") - } - ref["name"] = params.GatewayName - targetRefs[0] = ref - if err := unstructured.SetNestedSlice(r.Object, targetRefs, "spec", "targetRefs"); err != nil { - return fmt.Errorf("write EnvoyFilter targetRefs: %w", err) + + if err := unstructured.SetNestedStringMap(r.Object, + map[string]string{"gateway.networking.k8s.io/gateway-name": params.GatewayName}, + "spec", "workloadSelector", "labels"); err != nil { + return fmt.Errorf("write EnvoyFilter workloadSelector: %w", err) } + // targetRefs and workloadSelector are mutually exclusive (Istio 1.26+). Drop any + // leftover targetRefs from older manifests so SSA/admission never sees both. + unstructured.RemoveNestedField(r.Object, "spec", "targetRefs") + unstructured.RemoveNestedField(r.Object, "spec", "targetRef") anchorName := wasmpluginAnchorName(params.GatewayNamespace, params.GatewayName) beforeCluster := grpcClusterName(PayloadPreProcessingDeploymentName(params.TenantIdentifier), params.GatewayNamespace, 9004) diff --git a/maas-controller/pkg/platform/tenantreconcile/params_test.go b/maas-controller/pkg/platform/tenantreconcile/params_test.go index 66a3e4d59..70e6f3b19 100644 --- a/maas-controller/pkg/platform/tenantreconcile/params_test.go +++ b/maas-controller/pkg/platform/tenantreconcile/params_test.go @@ -260,6 +260,8 @@ func TestApplyPlatformParamsWithRenderedOverlay(t *testing.T) { assert.Equal(t, params.SubscriptionNamespace, requireEnvVarValue(t, payloadDeployment, "payload-processing", "TENANT_NAMESPACE")) assertDeploymentSelectorLabelAbsent(t, payloadDeployment, LabelTenantInstance) assert.Equal(t, PayloadProcessingDeploymentName(tenantID), requirePodTemplateLabel(t, payloadDeployment, LabelTenantInstance)) + // Default tenant (empty TenantIdentifier) must NOT have DISABLE_EXTERNAL_MODEL_CONTROLLER + assertEnvVarAbsent(t, payloadDeployment, "payload-processing", "DISABLE_EXTERNAL_MODEL_CONTROLLER") if cleanupCronJob := findResource(resources, GVKCronJob, MaaSAPIKeyCleanupCronJobName(tenantID)); cleanupCronJob != nil { assert.Equal(t, params.MaaSAPIKeyCleanupImage, requireContainerImage(t, cleanupCronJob, "spec", "jobTemplate", "spec", "template", "spec", "containers")) @@ -311,13 +313,17 @@ func TestApplyPlatformParamsWithRenderedOverlay(t *testing.T) { payloadEnvoyFilter := requireResource(t, resources, GVKEnvoyFilter, PayloadProcessingEnvoyFilterName(tenantID)) assert.Equal(t, params.GatewayNamespace, payloadEnvoyFilter.GetNamespace()) - targetRefs, found, err := unstructured.NestedSlice(payloadEnvoyFilter.Object, "spec", "targetRefs") + priority, found, err := unstructured.NestedInt64(payloadEnvoyFilter.Object, "spec", "priority") + require.NoError(t, err) + require.True(t, found, "EnvoyFilter spec.priority must be set so RHCL wasm anchors apply after Kuadrant") + assert.Equal(t, PayloadProcessingEnvoyFilterPriority, priority) + wsLabels, found, err := unstructured.NestedStringMap(payloadEnvoyFilter.Object, "spec", "workloadSelector", "labels") require.NoError(t, err) require.True(t, found) - require.NotEmpty(t, targetRefs) - firstTargetRef, ok := targetRefs[0].(map[string]any) - require.True(t, ok) - assert.Equal(t, params.GatewayName, firstTargetRef["name"]) + assert.Equal(t, params.GatewayName, wsLabels["gateway.networking.k8s.io/gateway-name"]) + _, targetRefsFound, err := unstructured.NestedSlice(payloadEnvoyFilter.Object, "spec", "targetRefs") + require.NoError(t, err) + assert.False(t, targetRefsFound, "targetRefs must be cleared; mutually exclusive with workloadSelector") // Verify dual-stage filter chain with dual anchors: // [0..1] WasmPlugin (ODH/community Kuadrant), [2..3] wasm filter (RHCL 1.4), @@ -504,6 +510,8 @@ func TestApplyPlatformParamsWithRenderedOverlay_AITenant(t *testing.T) { payloadDeployment := requireResource(t, resources, GVKDeployment, "payload-processing-redteam") assert.Equal(t, "redteam-gateway", requireEnvVarValue(t, payloadDeployment, "payload-processing", "GATEWAY_NAME")) assert.Equal(t, "ai-tenant-redteam", requireEnvVarValue(t, payloadDeployment, "payload-processing", "TENANT_NAMESPACE")) + // Non-default tenant must have DISABLE_EXTERNAL_MODEL_CONTROLLER=true + assert.Equal(t, "true", requireEnvVarValue(t, payloadDeployment, "payload-processing", "DISABLE_EXTERNAL_MODEL_CONTROLLER")) assert.Equal(t, "payload-processing-redteam", requireDeploymentSelectorLabel(t, payloadDeployment, LabelTenantInstance)) payloadBeforeDeployment := requireResource(t, resources, GVKDeployment, "payload-pre-processing-redteam") @@ -594,6 +602,33 @@ func requireEnvVarValue(t *testing.T, r *unstructured.Unstructured, containerNam return "" } +func assertEnvVarAbsent(t *testing.T, r *unstructured.Unstructured, containerName, envName string) { + t.Helper() + + containers, found, err := unstructured.NestedSlice(r.Object, "spec", "template", "spec", "containers") + require.NoError(t, err) + require.True(t, found) + + containerFound := false + for _, c := range containers { + containerMap, ok := c.(map[string]any) + require.True(t, ok) + if containerMap["name"] != containerName { + continue + } + containerFound = true + + envSlice, _ := containerMap["env"].([]any) + for _, e := range envSlice { + envMap, ok := e.(map[string]any) + require.True(t, ok) + assert.NotEqual(t, envName, envMap["name"], "env var %q should not be present in container %q", envName, containerName) + } + break + } + require.True(t, containerFound, "container %q not found", containerName) +} + func requirePodTemplateLabel(t *testing.T, r *unstructured.Unstructured, key string) string { t.Helper() @@ -638,3 +673,87 @@ func requireServiceSelectorLabel(t *testing.T, r *unstructured.Unstructured, key require.True(t, ok, "selector label %q not found", key) return value } + +func TestPatchMaaSAPIServingCert_DefaultTenant(t *testing.T) { + cert := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "metadata": map[string]any{ + "name": "maas-api-serving-cert", + "namespace": "redhat-ai-gateway-infra", + }, + "spec": map[string]any{ + "secretName": "maas-api-serving-cert", + "issuerRef": map[string]any{ + "name": "rhai-ca-issuer", + "kind": "ClusterIssuer", + "group": "cert-manager.io", + }, + "dnsNames": []any{ + "maas-api.opendatahub.svc", + "maas-api.opendatahub.svc.cluster.local", + }, + }, + }} + + params := PlatformParams{ + AppNamespace: "redhat-ai-gateway-infra", + TenantIdentifier: "", + } + + err := patchMaaSAPIServingCert(logr.Discard(), cert, params) + require.NoError(t, err) + + assert.Equal(t, "maas-api-serving-cert", cert.GetName()) + + secretName, _, _ := unstructured.NestedString(cert.Object, "spec", "secretName") + assert.Equal(t, "maas-api-serving-cert", secretName) + + dnsNames, _, _ := unstructured.NestedStringSlice(cert.Object, "spec", "dnsNames") + assert.Equal(t, []string{ + "maas-api.redhat-ai-gateway-infra.svc", + "maas-api.redhat-ai-gateway-infra.svc.cluster.local", + }, dnsNames) +} + +func TestPatchMaaSAPIServingCert_MultiTenant(t *testing.T) { + cert := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "cert-manager.io/v1", + "kind": "Certificate", + "metadata": map[string]any{ + "name": "maas-api-serving-cert", + "namespace": "redhat-ai-gateway-infra", + }, + "spec": map[string]any{ + "secretName": "maas-api-serving-cert", + "issuerRef": map[string]any{ + "name": "rhai-ca-issuer", + "kind": "ClusterIssuer", + "group": "cert-manager.io", + }, + "dnsNames": []any{ + "maas-api.opendatahub.svc", + "maas-api.opendatahub.svc.cluster.local", + }, + }, + }} + + params := PlatformParams{ + AppNamespace: "redhat-ai-gateway-infra", + TenantIdentifier: "redteam", + } + + err := patchMaaSAPIServingCert(logr.Discard(), cert, params) + require.NoError(t, err) + + assert.Equal(t, "maas-api-serving-cert-redteam", cert.GetName()) + + secretName, _, _ := unstructured.NestedString(cert.Object, "spec", "secretName") + assert.Equal(t, "maas-api-serving-cert-redteam", secretName) + + dnsNames, _, _ := unstructured.NestedStringSlice(cert.Object, "spec", "dnsNames") + assert.Equal(t, []string{ + "maas-api-redteam.redhat-ai-gateway-infra.svc", + "maas-api-redteam.redhat-ai-gateway-infra.svc.cluster.local", + }, dnsNames) +} diff --git a/maas-controller/pkg/platform/tenantreconcile/pipeline.go b/maas-controller/pkg/platform/tenantreconcile/pipeline.go index 028bda9b7..07cfeed26 100644 --- a/maas-controller/pkg/platform/tenantreconcile/pipeline.go +++ b/maas-controller/pkg/platform/tenantreconcile/pipeline.go @@ -5,11 +5,13 @@ import ( "errors" "fmt" "path/filepath" + "strconv" "github.com/go-logr/logr" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" @@ -108,6 +110,13 @@ func RunPlatform( if !ready { return &RunResult{DeploymentPending: true, Detail: detail, Warnings: params.Warnings}, nil } + ready, detail, err = PayloadProcessingEnvoyFilterReady(ctx, c, params.GatewayNamespace, params.GatewayName, tenantID) + if err != nil { + return nil, fmt.Errorf("payload-processing EnvoyFilter status: %w", err) + } + if !ready { + return &RunResult{DeploymentPending: true, Detail: detail, Warnings: params.Warnings}, nil + } return &RunResult{Warnings: params.Warnings}, nil } @@ -213,3 +222,57 @@ func MaasAPIDeploymentReady(ctx context.Context, c client.Client, appNamespace, } return true, "", nil } + +// PayloadProcessingEnvoyFilterReady verifies the per-tenant gateway EnvoyFilter that +// wires ext_proc is present with a priority high enough to run after Kuadrant's wasm +// insert. Without that, RHCL body-routed inference returns 404 NR on that gateway. +// +// This is a config-shape check (not a live config_dump). Use +// scripts/check-payload-ext-proc-filters.sh to confirm filters are in the proxy. +func PayloadProcessingEnvoyFilterReady(ctx context.Context, c client.Client, gatewayNamespace, gatewayName, tenantID string) (ready bool, detail string, err error) { + efName := PayloadProcessingEnvoyFilterName(tenantID) + ef := &unstructured.Unstructured{} + ef.SetGroupVersionKind(GVKEnvoyFilter) + key := types.NamespacedName{Namespace: gatewayNamespace, Name: efName} + if err := c.Get(ctx, key, ef); err != nil { + if apierrors.IsNotFound(err) { + return false, fmt.Sprintf( + "EnvoyFilter %s/%s not found — ext_proc will not run; body-routed /v1/* returns 404 NR", + gatewayNamespace, efName), nil + } + return false, "", err + } + + priority, found, err := unstructured.NestedInt64(ef.Object, "spec", "priority") + if err != nil { + return false, "", fmt.Errorf("read EnvoyFilter priority: %w", err) + } + if !found || priority < PayloadProcessingEnvoyFilterPriority { + shown := "missing" + if found { + shown = strconv.FormatInt(priority, 10) + } + return false, fmt.Sprintf( + "EnvoyFilter %s/%s spec.priority=%s; need >= %d so HTTP_FILTER inserts apply after Kuadrant wasm (otherwise body-routed /v1/* returns 404 NR)", + gatewayNamespace, efName, shown, PayloadProcessingEnvoyFilterPriority), nil + } + + // Istio 1.26+: targetRefs and workloadSelector are mutually exclusive. MaaS + // EnvoyFilters use workloadSelector keyed by gateway-name (see params patch). + wsLabels, found, err := unstructured.NestedStringMap(ef.Object, "spec", "workloadSelector", "labels") + if err != nil { + return false, "", fmt.Errorf("read EnvoyFilter workloadSelector: %w", err) + } + const gatewayNameLabel = "gateway.networking.k8s.io/gateway-name" + if !found || wsLabels[gatewayNameLabel] == "" { + return false, fmt.Sprintf( + "EnvoyFilter %s/%s has no workloadSelector.labels[%q]", + gatewayNamespace, efName, gatewayNameLabel), nil + } + if got := wsLabels[gatewayNameLabel]; got != gatewayName { + return false, fmt.Sprintf( + "EnvoyFilter %s/%s workloadSelector.labels[%q]=%q; expected gateway %q", + gatewayNamespace, efName, gatewayNameLabel, got, gatewayName), nil + } + return true, "", nil +} diff --git a/maas-controller/pkg/platform/tenantreconcile/pipeline_test.go b/maas-controller/pkg/platform/tenantreconcile/pipeline_test.go index 311098391..f13436825 100644 --- a/maas-controller/pkg/platform/tenantreconcile/pipeline_test.go +++ b/maas-controller/pkg/platform/tenantreconcile/pipeline_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -65,3 +66,102 @@ func TestSyncMaaSParametersConfigMap_UpdatesValue(t *testing.T) { require.NoError(t, c.Get(context.Background(), types.NamespacedName{Name: maasParametersConfigMapName, Namespace: "test-ns"}, &updated)) assert.Equal(t, "365", updated.Data["api-key-max-expiration-days"]) } + +func payloadProcessingEnvoyFilter(ns, efName, gatewayName string, priority *int64) *unstructured.Unstructured { + ef := &unstructured.Unstructured{} + ef.SetGroupVersionKind(GVKEnvoyFilter) + ef.SetNamespace(ns) + ef.SetName(efName) + spec := map[string]any{ + "workloadSelector": map[string]any{ + "labels": map[string]any{ + "gateway.networking.k8s.io/gateway-name": gatewayName, + }, + }, + } + if priority != nil { + spec["priority"] = *priority + } + ef.Object["spec"] = spec + return ef +} + +func TestPayloadProcessingEnvoyFilterReady(t *testing.T) { + const ( + gwNS = "openshift-ingress" + gwName = "partner" + tenantID = "partner" + ) + efName := PayloadProcessingEnvoyFilterName(tenantID) + prioOK := PayloadProcessingEnvoyFilterPriority + prioLow := int64(0) + + t.Run("missing EnvoyFilter", func(t *testing.T) { + c := fake.NewClientBuilder().Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, "not found") + assert.Contains(t, detail, efName) + assert.Contains(t, detail, "404 NR") + }) + + t.Run("missing priority", func(t *testing.T) { + c := fake.NewClientBuilder().WithObjects(payloadProcessingEnvoyFilter(gwNS, efName, gwName, nil)).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, "priority=missing") + assert.Contains(t, detail, "404 NR") + }) + + t.Run("priority too low", func(t *testing.T) { + c := fake.NewClientBuilder().WithObjects(payloadProcessingEnvoyFilter(gwNS, efName, gwName, &prioLow)).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, "priority=0") + }) + + t.Run("wrong gateway workloadSelector", func(t *testing.T) { + c := fake.NewClientBuilder().WithObjects(payloadProcessingEnvoyFilter(gwNS, efName, "other-gw", &prioOK)).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, "workloadSelector.labels") + assert.Contains(t, detail, "other-gw") + }) + + t.Run("missing workloadSelector", func(t *testing.T) { + ef := &unstructured.Unstructured{} + ef.SetGroupVersionKind(GVKEnvoyFilter) + ef.SetNamespace(gwNS) + ef.SetName(efName) + ef.Object["spec"] = map[string]any{"priority": prioOK} + c := fake.NewClientBuilder().WithObjects(ef).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, "has no workloadSelector") + }) + + t.Run("ignores default-tenant EnvoyFilter name for secondary tenants", func(t *testing.T) { + // Secondary tenants must look up payload-processing-, not payload-processing. + c := fake.NewClientBuilder().WithObjects( + payloadProcessingEnvoyFilter(gwNS, PayloadProcessingName, gwName, &prioOK), + ).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.False(t, ready) + assert.Contains(t, detail, efName) + assert.Contains(t, detail, "not found") + }) + + t.Run("ready", func(t *testing.T) { + c := fake.NewClientBuilder().WithObjects(payloadProcessingEnvoyFilter(gwNS, efName, gwName, &prioOK)).Build() + ready, detail, err := PayloadProcessingEnvoyFilterReady(context.Background(), c, gwNS, gwName, tenantID) + require.NoError(t, err) + assert.True(t, ready) + assert.Empty(t, detail) + }) +} diff --git a/maas-controller/pkg/platform/tenantreconcile/postrender.go b/maas-controller/pkg/platform/tenantreconcile/postrender.go index 48525e65c..4bfd99dc4 100644 --- a/maas-controller/pkg/platform/tenantreconcile/postrender.go +++ b/maas-controller/pkg/platform/tenantreconcile/postrender.go @@ -364,17 +364,17 @@ func buildTelemetryLabels(log logr.Logger, config *maasv1alpha1.TenantTelemetryC } labels := map[string]any{ "subscription": "auth.identity.selected_subscription", - "cost_center": "auth.identity.subscription_info.costCenter", + "cost_center": `has(auth.identity.subscription_info.costCenter) ? auth.identity.subscription_info.costCenter : ""`, } if captureOrganization { - labels["organization_id"] = "auth.identity.subscription_info.organizationId" + labels["organization_id"] = `has(auth.identity.subscription_info.organizationId) ? auth.identity.subscription_info.organizationId : ""` } if captureUser { log.Info("WARNING: User identity metrics enabled - ensure GDPR/privacy compliance", "field", "captureUser", "value", true) labels["user"] = "auth.identity.userid" } if captureGroup { - labels["group"] = "auth.identity.group" + labels["group"] = "auth.identity.groups_str" } if captureModelUsage { labels["model"] = "responseBodyJSON(\"/model\")" diff --git a/maas-controller/pkg/platform/tenantreconcile/postrender_test.go b/maas-controller/pkg/platform/tenantreconcile/postrender_test.go new file mode 100644 index 000000000..1f53200ca --- /dev/null +++ b/maas-controller/pkg/platform/tenantreconcile/postrender_test.go @@ -0,0 +1,114 @@ +package tenantreconcile + +import ( + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + + maasv1alpha1 "github.com/opendatahub-io/models-as-a-service/maas-controller/api/maas/v1alpha1" +) + +func boolPtr(b bool) *bool { return &b } + +func TestBuildTelemetryLabels(t *testing.T) { + tests := []struct { + name string + config *maasv1alpha1.TenantTelemetryConfig + expectedLabels map[string]any + absentKeys []string + }{ + { + name: "nil config uses defaults", + config: nil, + expectedLabels: map[string]any{ + "subscription": "auth.identity.selected_subscription", + "cost_center": `has(auth.identity.subscription_info.costCenter) ? auth.identity.subscription_info.costCenter : ""`, + "organization_id": `has(auth.identity.subscription_info.organizationId) ? auth.identity.subscription_info.organizationId : ""`, + "model": "responseBodyJSON(\"/model\")", + }, + absentKeys: []string{"user", "group"}, + }, + { + name: "captureGroup true emits groups_str path", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureGroup: boolPtr(true), + }, + }, + expectedLabels: map[string]any{ + "group": "auth.identity.groups_str", + }, + }, + { + name: "captureGroup false omits group label", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureGroup: boolPtr(false), + }, + }, + absentKeys: []string{"group"}, + }, + { + name: "captureUser true emits userid path", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureUser: boolPtr(true), + }, + }, + expectedLabels: map[string]any{ + "user": "auth.identity.userid", + }, + }, + { + name: "captureOrganization false omits organization_id", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureOrganization: boolPtr(false), + }, + }, + absentKeys: []string{"organization_id"}, + }, + { + name: "captureModelUsage false omits model", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureModelUsage: boolPtr(false), + }, + }, + absentKeys: []string{"model"}, + }, + { + name: "all flags enabled", + config: &maasv1alpha1.TenantTelemetryConfig{ + Metrics: &maasv1alpha1.TenantMetricsConfig{ + CaptureGroup: boolPtr(true), + CaptureUser: boolPtr(true), + CaptureOrganization: boolPtr(true), + CaptureModelUsage: boolPtr(true), + }, + }, + expectedLabels: map[string]any{ + "subscription": "auth.identity.selected_subscription", + "cost_center": `has(auth.identity.subscription_info.costCenter) ? auth.identity.subscription_info.costCenter : ""`, + "organization_id": `has(auth.identity.subscription_info.organizationId) ? auth.identity.subscription_info.organizationId : ""`, + "user": "auth.identity.userid", + "group": "auth.identity.groups_str", + "model": "responseBodyJSON(\"/model\")", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + labels := buildTelemetryLabels(logr.Discard(), tt.config) + + for k, v := range tt.expectedLabels { + assert.Equal(t, v, labels[k], "label %q", k) + } + for _, k := range tt.absentKeys { + assert.NotContains(t, labels, k, "label %q should be absent", k) + } + }) + } +} diff --git a/scripts/README.md b/scripts/README.md index 5623a1fb8..3b6897235 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -275,11 +275,55 @@ DRY_RUN=true ./scripts/setup-gateway.sh - `DISCONNECTED` - Disable GitHub manifest fallback (`true`/`false`, default: false) - `DRY_RUN` - Preview changes without applying (`true`/`false`, default: false) - `MAAS_MANIFEST_REF` - Git tag or commit SHA for remote kustomize fallback (defaults to current repo `HEAD` when run from a clone; required when fetching without a local tree) +- `ALLOWED_ROUTE_NAMESPACES` - Comma-separated list of namespaces allowed to attach HTTPRoutes to the Gateway (e.g. `"opendatahub,odh-ai-gateway-infra,llm"`). Uses `from: Selector` with `matchExpressions` on `kubernetes.io/metadata.name`. Takes precedence over `NAMESPACE_SELECTOR_LABELS`. +- `NAMESPACE_SELECTOR_LABELS` - Comma-separated `key=value` label pairs for namespace selection (e.g. `"gateway-access=true"`). Uses `from: Selector` with `matchLabels`. Ignored when `ALLOWED_ROUTE_NAMESPACES` is set. + +When neither is set, the Gateway defaults to `allowedRoutes: namespaces: from: Same`, which restricts HTTPRoute attachment to the Gateway's own namespace (`openshift-ingress`). + +> **Important for MaaS:** The infra namespace (`odh-ai-gateway-infra` / `redhat-ai-gateway-infra`) hosts `maas-api-route` — omitting it causes 404 on all MaaS API calls. `deploy.sh` sets `ALLOWED_ROUTE_NAMESPACES` automatically to `,[,]`; set `MODEL_NAMESPACE` to also include the model namespace. **Note:** Route mode auto-detects cluster TLS certificates. Override with `CERT_NAME` if needed. --- +### `create-ai-tenant.sh` +Creates a new AITenant with an isolated Gateway and infrastructure for multi-tenant deployments. + +**Usage:** +```bash +# Auto-detect cluster domain (creates -maas. hostname) +./scripts/create-ai-tenant.sh + +# Specify a custom gateway hostname +./scripts/create-ai-tenant.sh + +# MaaS on ODH — allow app, infra, and model namespaces to attach HTTPRoutes +ALLOWED_ROUTE_NAMESPACES="opendatahub,odh-ai-gateway-infra,llm" \ + ./scripts/create-ai-tenant.sh myteam + +# MaaS on RHOAI +ALLOWED_ROUTE_NAMESPACES="redhat-ods-applications,redhat-ai-gateway-infra,llm" \ + ./scripts/create-ai-tenant.sh myteam + +# Restrict by label selector +NAMESPACE_SELECTOR_LABELS="gateway-access=true" \ + ./scripts/create-ai-tenant.sh myteam +``` + +**What it does:** +- Creates a Gateway in `openshift-ingress` with LoadBalancer service and auto-detected TLS certificate +- Creates an AITenant CR (triggers controller to create MaasTenantConfig, maas-api, etc.) + +**Environment Variables:** +- `ALLOWED_ROUTE_NAMESPACES` - Comma-separated list of namespaces allowed to attach HTTPRoutes. Uses `from: Selector` with `matchExpressions` on `kubernetes.io/metadata.name`. +- `NAMESPACE_SELECTOR_LABELS` - Comma-separated `key=value` label pairs for namespace selection (e.g. `"gateway-access=true"`). Uses `from: Selector` with `matchLabels`. Ignored when `ALLOWED_ROUTE_NAMESPACES` is set. + +When neither is set, the Gateway defaults to `allowedRoutes: namespaces: from: Same`. + +> **Important for MaaS deployments:** HTTPRoutes are created in the application namespace (`opendatahub` / `redhat-ods-applications`) and often in model namespaces (e.g. `llm`), not in `openshift-ingress`. Set `ALLOWED_ROUTE_NAMESPACES` (or a label selector) accordingly, otherwise the tenant Gateway will block HTTPRoute attachment. + +--- + ### `install-dependencies.sh` Installs individual dependencies (Kuadrant, ODH, etc.). diff --git a/scripts/check-payload-ext-proc-filters.sh b/scripts/check-payload-ext-proc-filters.sh new file mode 100755 index 000000000..b3740ba3a --- /dev/null +++ b/scripts/check-payload-ext-proc-filters.sh @@ -0,0 +1,166 @@ +#!/usr/bin/env bash +# Check that payload-processing EnvoyFilter is shaped correctly AND that +# ext_proc filters are present in the live gateway Envoy config. +# +# Catches the RHCL failure mode where EnvoyFilter YAML exists but HTTP_FILTER +# inserts never match (e.g. missing positive priority) → 404 NR on body-routed /v1/*. +# +# Usage: +# ./scripts/check-payload-ext-proc-filters.sh +# GATEWAY_NAMESPACE=openshift-ingress GATEWAY_NAME=maas-default-gateway ./scripts/check-payload-ext-proc-filters.sh +# GATEWAY_NAME=partner EF_NAME=payload-processing-partner ./scripts/check-payload-ext-proc-filters.sh +# +# Requires: oc/kubectl, python3, curl (for local port-forward to Envoy admin) + +set -euo pipefail + +GATEWAY_NAMESPACE="${GATEWAY_NAMESPACE:-openshift-ingress}" +GATEWAY_NAME="${GATEWAY_NAME:-maas-default-gateway}" +EF_NAME="${EF_NAME:-payload-processing}" +MIN_PRIORITY="${MIN_PRIORITY:-10}" +REQUIRED_FILTERS=( + "envoy.filters.http.ext_proc.ipp-pre" + "envoy.filters.http.ext_proc.ipp" +) + +KUBECTL="${KUBECTL:-}" +if [[ -z "$KUBECTL" ]]; then + if command -v oc >/dev/null 2>&1; then + KUBECTL=oc + else + KUBECTL=kubectl + fi +fi + +fail() { echo "FAIL: $*" >&2; exit 1; } +ok() { echo "OK: $*"; } + +echo "== EnvoyFilter ${GATEWAY_NAMESPACE}/${EF_NAME} ==" +if ! "$KUBECTL" get envoyfilter "$EF_NAME" -n "$GATEWAY_NAMESPACE" >/dev/null 2>&1; then + fail "EnvoyFilter not found — ext_proc cannot run (body-routed /v1/* → 404 NR)" +fi + +priority="$("$KUBECTL" get envoyfilter "$EF_NAME" -n "$GATEWAY_NAMESPACE" -o jsonpath='{.spec.priority}' 2>/dev/null || true)" +if [[ -z "$priority" ]]; then + fail "spec.priority is missing; need >= ${MIN_PRIORITY} so inserts apply after Kuadrant wasm" +fi +if (( priority < MIN_PRIORITY )); then + fail "spec.priority=${priority}; need >= ${MIN_PRIORITY}" +fi +ok "spec.priority=${priority}" + +target="$("$KUBECTL" get envoyfilter "$EF_NAME" -n "$GATEWAY_NAMESPACE" -o jsonpath='{.spec.targetRefs[0].name}' 2>/dev/null || true)" +[[ "$target" == "$GATEWAY_NAME" ]] || fail "targetRefs[0].name=${target:-empty}; expected ${GATEWAY_NAME}" +ok "targetRefs → Gateway/${GATEWAY_NAME}" + +echo "== Live gateway http_filters ==" +pod="$("$KUBECTL" get pods -n "$GATEWAY_NAMESPACE" \ + -l "gateway.networking.k8s.io/gateway-name=${GATEWAY_NAME}" \ + -o jsonpath='{.items[0].metadata.name}' 2>/dev/null || true)" +[[ -n "$pod" ]] || fail "no Running pod for Gateway/${GATEWAY_NAME} in ${GATEWAY_NAMESPACE}" + +local_port=15000 +pf_log="$(mktemp)" +dump="$(mktemp)" +cleanup() { + kill "$pf_pid" 2>/dev/null || true + wait "$pf_pid" 2>/dev/null || true + rm -f "$pf_log" "$dump" +} +trap cleanup EXIT + +"$KUBECTL" port-forward -n "$GATEWAY_NAMESPACE" "pod/${pod}" "${local_port}:15000" >"$pf_log" 2>&1 & +pf_pid=$! + +# Wait for admin port +for _ in $(seq 1 30); do + if curl -fsS "http://127.0.0.1:${local_port}/ready" >/dev/null 2>&1; then + break + fi + sleep 0.2 +done +curl -fsS "http://127.0.0.1:${local_port}/config_dump?resource=dynamic_listeners" -o "$dump" \ + || fail "could not fetch Envoy config_dump from ${pod} (see ${pf_log})" + +python3 - "$dump" "${REQUIRED_FILTERS[@]}" <<'PY' +import json, sys +path = sys.argv[1] +required = sys.argv[2:] +with open(path) as f: + data = json.load(f) + +chains = [] + +def walk(o): + if isinstance(o, dict): + if "http_filters" in o: + names = [f.get("name", "") for f in o["http_filters"]] + chains.append(names) + for v in o.values(): + walk(v) + elif isinstance(o, list): + for v in o: + walk(v) + +walk(data) +if not chains: + print("FAIL: no http_filters found in config_dump", file=sys.stderr) + sys.exit(1) + +# Prefer a chain that already has kuadrant wasm / wasmplugin (auth-bearing listener) +def score(names): + s = 0 + joined = " ".join(names) + if "envoy.filters.http.wasm" in joined or "wasmplugin" in joined: + s += 10 + if "envoy.filters.http.router" in names: + s += 1 + return s + +chains.sort(key=score, reverse=True) +names = chains[0] +print("filter chain:") +for n in names: + print(f" - {n}") + +missing = [r for r in required if r not in names] +if missing: + print("FAIL: missing required ext_proc filters:", ", ".join(missing), file=sys.stderr) + print("hint: EnvoyFilter inserts may not be matching (check priority / auth anchor).", file=sys.stderr) + sys.exit(1) + +# Ordering: ipp-pre before auth (wasm|wasmplugin), ipp after auth, before router +def idx(exact): + try: + return names.index(exact) + except ValueError: + return -1 + +def idx_substr(substr): + for i, n in enumerate(names): + if substr in n: + return i + return -1 + +pre = idx("envoy.filters.http.ext_proc.ipp-pre") +ipp = idx("envoy.filters.http.ext_proc.ipp") +auth = idx("envoy.filters.http.wasm") +if auth < 0: + auth = idx_substr("wasmplugin") +router = idx("envoy.filters.http.router") + +if pre < 0 or ipp < 0 or router < 0: + print("FAIL: unexpected filter names", file=sys.stderr) + sys.exit(1) +if auth >= 0 and not (pre < auth < ipp < router): + print(f"FAIL: bad order pre={pre} auth={auth} ipp={ipp} router={router}", file=sys.stderr) + print("expected: ipp-pre → auth → ipp → router", file=sys.stderr) + sys.exit(1) +if auth < 0 and not (pre < ipp < router): + print(f"FAIL: bad order pre={pre} ipp={ipp} router={router}", file=sys.stderr) + sys.exit(1) + +print("OK: ext_proc filters present with correct relative order") +PY + +echo "All checks passed." diff --git a/scripts/create-ai-tenant.sh b/scripts/create-ai-tenant.sh index 6c874d08f..8b14faae4 100755 --- a/scripts/create-ai-tenant.sh +++ b/scripts/create-ai-tenant.sh @@ -13,14 +13,52 @@ # - Gateway with LoadBalancer service and TLS certificate # - AITenant CR (triggers controller to create MaasTenantConfig, maas-api, etc.) # +# AllowedRoutes configuration (controls which namespaces can attach HTTPRoutes): +# ALLOWED_ROUTE_NAMESPACES - Comma-separated namespace names allowed to attach HTTPRoutes. +# e.g. "opendatahub" or "redhat-ods-applications,llm" +# Uses 'from: Selector' with matchExpressions on +# kubernetes.io/metadata.name. +# NAMESPACE_SELECTOR_LABELS - Comma-separated key=value label pairs for namespace selection. +# Uses 'from: Selector' with matchLabels. +# Ignored if ALLOWED_ROUTE_NAMESPACES is set. +# (neither set) - Defaults to 'from: Same' (secure default; only +# openshift-ingress can attach HTTPRoutes). +# +# Multi-tenant deployments: +# Per-tenant Gateways receive HTTPRoutes from the infrastructure namespace (where +# maas-api is deployed) and from any model namespaces (where LLMInferenceServices +# run). Since these namespaces vary per cluster, the recommended approach is a +# per-gateway label selector. After running this script, label each namespace that +# needs to attach HTTPRoutes: +# +# NAMESPACE_SELECTOR_LABELS="maas.opendatahub.io/gateway-access-myteam=true" \ +# ./scripts/create-ai-tenant.sh myteam +# +# # Then label the infra namespace and any model namespaces: +# oc label namespace odh-ai-gateway-infra \ +# maas.opendatahub.io/gateway-access-myteam=true --overwrite +# oc label namespace llm \ +# maas.opendatahub.io/gateway-access-myteam=true --overwrite +# +# Simple / single-namespace examples: +# ALLOWED_ROUTE_NAMESPACES="opendatahub,llm" ./scripts/create-ai-tenant.sh myteam +# NAMESPACE_SELECTOR_LABELS="maas.opendatahub.io/gateway-access-myteam=true" \ +# ./scripts/create-ai-tenant.sh myteam +# set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=deployment-helpers.sh +source "${SCRIPT_DIR}/deployment-helpers.sh" + TENANT_NAME=${1:-} GATEWAY_HOSTNAME=${2:-} GATEWAY_NAMESPACE="openshift-ingress" AITENANT_NAMESPACE="ai-tenants" HOSTNAME_AUTO_DETECTED=false +ALLOWED_ROUTE_NAMESPACES="${ALLOWED_ROUTE_NAMESPACES:-}" +NAMESPACE_SELECTOR_LABELS="${NAMESPACE_SELECTOR_LABELS:-}" validate_dns1123_subdomain() { local value="$1" @@ -97,8 +135,15 @@ fi echo "Using TLS certificate: $TLS_SECRET_NAME" +if [[ -z "$ALLOWED_ROUTE_NAMESPACES" && -z "$NAMESPACE_SELECTOR_LABELS" ]]; then + log_warn "No ALLOWED_ROUTE_NAMESPACES or NAMESPACE_SELECTOR_LABELS set; using from: Same." + log_warn "MaaS HTTPRoutes attach from the app/model namespaces — set ALLOWED_ROUTE_NAMESPACES (e.g. opendatahub,llm) or NAMESPACE_SELECTOR_LABELS." +fi + # Create Gateway with LoadBalancer service (default Gateway API pattern) # Note: Gateway name must match tenant name (AITenant controller defaults gatewayRef.name to tenant name) +# Indent 4: listener content level in this heredoc +allowed_routes_yaml="$(build_allowed_routes_yaml 4)" oc apply -f - < +# Override rate-limiting policy engine # --enable-tls-backend Enable TLS for Authorino/MaaS API (default: on) # --enable-keycloak Deploy Keycloak for external OIDC (optional) # --namespace Target namespace @@ -133,7 +135,9 @@ esac DEPLOYMENT_MODE="${DEPLOYMENT_MODE:-operator}" OPERATOR_TYPE="${OPERATOR_TYPE:-odh}" -POLICY_ENGINE="" # Auto-determined: odh→kuadrant, rhoai→rhcl +POLICY_ENGINE="${POLICY_ENGINE:-}" # Auto-determined unless set via env or --policy-engine +RHCL_STARTING_CSV="${RHCL_STARTING_CSV:-}" +RHCL_NAMESPACE="${RHCL_NAMESPACE:-kuadrant-system}" NAMESPACE="${DEPLOYMENT_NAMESPACE:-}" # Auto-determined based on operator type ENABLE_TLS_BACKEND="${ENABLE_TLS_BACKEND:-true}" ENABLE_KEYCLOAK="${ENABLE_KEYCLOAK:-false}" @@ -175,6 +179,12 @@ OPTIONS: - odh → kuadrant (community v1.4.2 with AuthPolicy v1) Only applies when --deployment-mode=operator + --policy-engine + Rate-limiting policy engine (default: auto-selected) + - rhcl: Red Hat Connectivity Link from redhat-operators (stable channel head) + - kuadrant: upstream community catalog (v1.4.2) + Overrides auto-selection for both operator and kustomize modes. + --enable-tls-backend Enable TLS backend for Authorino and MaaS API (default: enabled) Configures HTTPS for Authorino to maas-api communication @@ -251,6 +261,9 @@ ENVIRONMENT VARIABLES: OPERATOR_STARTING_CSV ODH Subscription startingCSV (optional; when unset, follows the channel head) OPERATOR_INSTALL_PLAN_APPROVAL ODH Subscription OLM approval (default: Manual — no auto-upgrades; first InstallPlan is auto-approved by the script) OPERATOR_TYPE Operator type (rhoai/odh) + POLICY_ENGINE Policy engine override (rhcl|kuadrant) + RHCL_STARTING_CSV Pin RHCL operator CSV (default: channel head on redhat-operators) + RHCL_NAMESPACE RHCL operator/Kuadrant workload namespace (default: kuadrant-system) EXTERNAL_OIDC Enable external OIDC on maas-api (true/false) OIDC_ISSUER_URL External OIDC issuer URL for maas-api AuthPolicy patching LOG_LEVEL Logging verbosity (DEBUG, INFO, WARN, ERROR) @@ -326,6 +339,11 @@ parse_arguments() { OPERATOR_TYPE="$2" shift 2 ;; + --policy-engine) + require_flag_value "$1" "${2:-}" + POLICY_ENGINE="$2" + shift 2 + ;; --enable-tls-backend) ENABLE_TLS_BACKEND="true" shift @@ -473,10 +491,17 @@ validate_configuration() { fi fi - # Auto-determine policy engine based on operator type + # Auto-determine policy engine based on operator type unless explicitly set. # - ODH uses community Kuadrant (v1.4.2 from upstream catalog has AuthPolicy v1) # - RHOAI uses RHCL (Red Hat Connectivity Link - downstream) - if [[ "$DEPLOYMENT_MODE" == "operator" ]]; then + if [[ -n "$POLICY_ENGINE" ]]; then + if [[ ! "$POLICY_ENGINE" =~ ^(rhcl|kuadrant)$ ]]; then + log_error "Invalid policy engine: $POLICY_ENGINE" + log_error "Must be 'rhcl' or 'kuadrant'" + exit 1 + fi + log_debug "Using explicitly configured policy engine: $POLICY_ENGINE" + elif [[ "$DEPLOYMENT_MODE" == "operator" ]]; then case "$OPERATOR_TYPE" in odh) POLICY_ENGINE="kuadrant" @@ -723,10 +748,12 @@ EOF # Infrastructure namespace is configurable via deployment overlays (params.env). log_info "" log_info "Waiting for Tenant reconciler to deploy maas-api..." - local infra_namespace_raw="${INFRA_NAMESPACE:-AUTO}" + local infra_namespace_raw="${INFRA_NAMESPACE-AUTO}" local infra_namespace if [ "$infra_namespace_raw" = "AUTO" ]; then infra_namespace=$(derive_infra_namespace "$NAMESPACE") + elif [ -z "$infra_namespace_raw" ]; then + infra_namespace="$NAMESPACE" else infra_namespace="$infra_namespace_raw" fi @@ -912,10 +939,12 @@ validate_postgres_connection() { # wait_for_operator_maas_api waits for maas-api to be deployed by the Tenant # reconciler (maas-controller) in the infrastructure namespace. wait_for_operator_maas_api() { - local infra_namespace_raw="${INFRA_NAMESPACE:-AUTO}" + local infra_namespace_raw="${INFRA_NAMESPACE-AUTO}" local infra_namespace if [ "$infra_namespace_raw" = "AUTO" ]; then infra_namespace=$(derive_infra_namespace "$NAMESPACE") + elif [ -z "$infra_namespace_raw" ]; then + infra_namespace="$NAMESPACE" else infra_namespace="$infra_namespace_raw" fi @@ -943,10 +972,13 @@ wait_for_operator_maas_api() { deploy_postgresql() { # Infrastructure namespace where maas-api runs (AUTO = derive from controller namespace) - local infra_ns_raw="${INFRA_NAMESPACE:-AUTO}" + local controller_ns="${NAMESPACE:-opendatahub}" + local infra_ns_raw="${INFRA_NAMESPACE-AUTO}" local infra_ns if [ "$infra_ns_raw" = "AUTO" ]; then - infra_ns=$(derive_infra_namespace "$NAMESPACE") + infra_ns=$(derive_infra_namespace "$controller_ns") + elif [ -z "$infra_ns_raw" ]; then + infra_ns="$controller_ns" else infra_ns="$infra_ns_raw" fi @@ -965,7 +997,7 @@ deploy_postgresql() { log_warn " (AWS RDS, Crunchy Operator, Azure Database, etc.)" log_warn "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" # setup-database.sh handles upgrade detection and namespace selection - "${SCRIPT_DIR}/setup-database.sh" + NAMESPACE="$controller_ns" "${SCRIPT_DIR}/setup-database.sh" fi } @@ -1053,12 +1085,20 @@ install_policy_engine() { case "$POLICY_ENGINE" in rhcl) log_info "Installing RHCL (Red Hat Connectivity Link - downstream)" + local rhcl_ns="${RHCL_NAMESPACE:-kuadrant-system}" + local rhcl_starting_csv="${RHCL_STARTING_CSV:-}" + if [[ -n "$rhcl_starting_csv" ]]; then + log_info "Pinning RHCL operator to startingCSV: $rhcl_starting_csv" + else + log_info "Using RHCL channel head from redhat-operators (stable)" + fi + log_info "Installing RHCL into namespace: $rhcl_ns" if ! install_olm_operator \ "rhcl-operator" \ - "rh-connectivity-link" \ + "$rhcl_ns" \ "redhat-operators" \ "stable" \ - "" \ + "$rhcl_starting_csv" \ "AllNamespaces" \ "" \ ""; then @@ -1067,10 +1107,10 @@ install_policy_engine() { fi # Patch RHCL CSV to recognize OpenShift Gateway controller - patch_kuadrant_csv "rh-connectivity-link" "rhcl-operator" + patch_kuadrant_csv "$rhcl_ns" "rhcl-operator" # Apply RHCL/Kuadrant custom resource - apply_kuadrant_cr "rh-connectivity-link" + apply_kuadrant_cr "$rhcl_ns" ;; kuadrant) @@ -1457,12 +1497,29 @@ apply_kuadrant_cr() { # Setup Gateway using standalone script (replaces inline setup_gateway_api + setup_maas_gateway) # The script handles GatewayClass creation, Gateway creation with TLS cert detection, # and waits for Gateway to be Programmed before returning. + # Default allowedRoutes to the app namespace so maas-api HTTPRoutes can attach. + # Include MODEL_NAMESPACE when set (e2e/demos deploy models outside the app ns). + # Override with ALLOWED_ROUTE_NAMESPACES or NAMESPACE_SELECTOR_LABELS as needed. + local gateway_allowed_namespaces="${ALLOWED_ROUTE_NAMESPACES:-}" + if [[ -z "$gateway_allowed_namespaces" && -z "${NAMESPACE_SELECTOR_LABELS:-}" ]]; then + # Always include the infra namespace: maas-api-route lives there and must attach + # to the gateway for API key/subscription calls to reach maas-api. + local infra_ns + infra_ns=$(derive_infra_namespace "$NAMESPACE") + gateway_allowed_namespaces="$NAMESPACE,$infra_ns" + if [[ -n "${MODEL_NAMESPACE:-}" && "${MODEL_NAMESPACE}" != "$NAMESPACE" ]]; then + gateway_allowed_namespaces="${gateway_allowed_namespaces},${MODEL_NAMESPACE}" + fi + fi + INGRESS_MODE="${INGRESS_MODE:-route}" \ DISCONNECTED="${DISCONNECTED:-false}" \ CLUSTER_DOMAIN="${CLUSTER_DOMAIN:-}" \ CERT_NAME="${CERT_NAME:-}" \ DRY_RUN="${DRY_RUN:-false}" \ MAAS_MANIFEST_REF="${MAAS_MANIFEST_REF:-}" \ + ALLOWED_ROUTE_NAMESPACES="${gateway_allowed_namespaces}" \ + NAMESPACE_SELECTOR_LABELS="${NAMESPACE_SELECTOR_LABELS:-}" \ "${SCRIPT_DIR}/setup-gateway.sh" || { log_error "Gateway setup failed" return 1 @@ -1692,14 +1749,10 @@ configure_tenant_external_oidc() { configure_tls_backend() { log_info "Configuring TLS backend for Authorino and MaaS API..." - # Determine Authorino namespace based on rate limiter - local authorino_namespace + # Authorino and Kuadrant workloads run in kuadrant-system for both RHCL and community Kuadrant. + local authorino_namespace="${RHCL_NAMESPACE:-kuadrant-system}" case "$POLICY_ENGINE" in - rhcl) - authorino_namespace="rh-connectivity-link" - ;; - kuadrant) - authorino_namespace="kuadrant-system" + rhcl|kuadrant) ;; *) log_warn "Unknown policy engine: $POLICY_ENGINE, defaulting to kuadrant-system" @@ -1740,10 +1793,12 @@ configure_tls_backend() { log_info "Restarting deployments to pick up TLS configuration..." # maas-api deploys to infrastructure namespace - local infra_namespace_raw="${INFRA_NAMESPACE:-AUTO}" + local infra_namespace_raw="${INFRA_NAMESPACE-AUTO}" local infra_namespace if [ "$infra_namespace_raw" = "AUTO" ]; then infra_namespace=$(derive_infra_namespace "$NAMESPACE") + elif [ -z "$infra_namespace_raw" ]; then + infra_namespace="$NAMESPACE" else infra_namespace="$infra_namespace_raw" fi diff --git a/scripts/deployment-helpers.sh b/scripts/deployment-helpers.sh index 7e09c889d..f1b7e4839 100755 --- a/scripts/deployment-helpers.sh +++ b/scripts/deployment-helpers.sh @@ -195,6 +195,39 @@ export AUTHORINO_MIN_VERSION="0.22.0" export LIMITADOR_MIN_VERSION="0.16.0" export DNS_OPERATOR_MIN_VERSION="0.15.0" +# resolve_policy_engine_namespace returns the namespace where RHCL/Kuadrant workloads run. +# Optional argument: policy engine name (rhcl|kuadrant). Defaults to POLICY_ENGINE env or auto-detect. +resolve_policy_engine_namespace() { + local policy_engine="${1:-${POLICY_ENGINE:-}}" + local default_ns="${RHCL_NAMESPACE:-kuadrant-system}" + if [[ -z "$policy_engine" ]]; then + if kubectl get ns kuadrant-system &>/dev/null \ + && kubectl get deploy authorino -n kuadrant-system &>/dev/null 2>&1; then + echo "kuadrant-system" + return + fi + if kubectl get ns rh-connectivity-link &>/dev/null \ + && kubectl get deploy authorino -n rh-connectivity-link &>/dev/null 2>&1; then + echo "rh-connectivity-link" + return + fi + echo "$default_ns" + return + fi + case "$policy_engine" in + rhcl|kuadrant) echo "$default_ns" ;; + *) + log_warn "Unknown policy engine '$policy_engine', defaulting to $default_ns" + echo "$default_ns" + ;; + esac +} + +# resolve_authorino_namespace returns the namespace where Authorino runs. +resolve_authorino_namespace() { + resolve_policy_engine_namespace "$@" +} + # ========================================== # Logging Functions # ========================================== @@ -1783,3 +1816,250 @@ dump_llmis_diagnostics() { echo "End of diagnostics for: $llmis_name" echo "==========================================" } + +# ========================================== +# Gateway AllowedRoutes Helpers +# ========================================== + +# _allowed_routes_same_yaml +# Emits the secure-default allowedRoutes YAML block (from: Same). +_allowed_routes_same_yaml() { + local I="$1" + printf '%s' \ +"${I}allowedRoutes: +${I} namespaces: +${I} from: Same" +} + +# _is_valid_dns1123_label +# Returns 0 if value is a DNS-1123 label (safe to embed in YAML/JSON quotes). +_is_valid_dns1123_label() { + local v="$1" + [[ ${#v} -le 63 ]] && [[ "$v" =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ ]] +} + +# _is_valid_k8s_label_key +# Returns 0 if key is a valid Kubernetes label key (name or prefix/name). +_is_valid_k8s_label_key() { + local key="$1" name prefix + [[ -z "$key" || ${#key} -gt 253 ]] && return 1 + if [[ "$key" == */* ]]; then + prefix="${key%/*}" + name="${key##*/}" + [[ -z "$prefix" || -z "$name" ]] && return 1 + # Prefix: DNS subdomain; name: DNS-1123 label with optional dots/underscores mid-segment + [[ "$prefix" =~ ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ ]] || return 1 + [[ ${#name} -le 63 && "$name" =~ ^[a-zA-Z0-9]([-_.a-zA-Z0-9]*[a-zA-Z0-9])?$ ]] || return 1 + return 0 + fi + [[ ${#key} -le 63 && "$key" =~ ^[a-zA-Z0-9]([-_.a-zA-Z0-9]*[a-zA-Z0-9])?$ ]] +} + +# _is_valid_k8s_label_value +# Returns 0 if value is a valid Kubernetes label value (or empty). +_is_valid_k8s_label_value() { + local v="$1" + [[ -z "$v" ]] && return 0 + [[ ${#v} -le 63 && "$v" =~ ^[a-zA-Z0-9]([-_.a-zA-Z0-9]*[a-zA-Z0-9])?$ ]] +} + +# build_allowed_routes_yaml +# Generates the allowedRoutes YAML block for a Gateway listener at the given +# indentation level. Designed for embedding directly into heredoc manifests. +# +# Uses env vars (see below) to pick a mode: +# ALLOWED_ROUTE_NAMESPACES → from: Selector with matchExpressions on +# kubernetes.io/metadata.name (namespace name list) +# NAMESPACE_SELECTOR_LABELS → from: Selector with matchLabels (label filter) +# (neither / invalid) → from: Same (secure default; only the Gateway's +# own namespace can attach HTTPRoutes) +# +# Arguments: +# indent - Number of spaces for the 'allowedRoutes:' key (default: 6) +# +# Environment: +# ALLOWED_ROUTE_NAMESPACES - Comma-separated namespace names, e.g. "opendatahub,llm" +# NAMESPACE_SELECTOR_LABELS - Comma-separated key=value pairs, e.g. "gateway-access=true" +build_allowed_routes_yaml() { + local indent="${1:-6}" + local I + I="$(printf '%*s' "$indent" '')" + + if [[ -n "${ALLOWED_ROUTE_NAMESPACES:-}" ]]; then + local values_lines="" _ns + IFS=',' read -ra _ns_arr <<< "$ALLOWED_ROUTE_NAMESPACES" + for _ns in "${_ns_arr[@]}"; do + _ns="${_ns//[[:space:]]/}" + [[ -z "$_ns" ]] && continue + if ! _is_valid_dns1123_label "$_ns"; then + log_warn "Ignoring invalid namespace name in ALLOWED_ROUTE_NAMESPACES: ${_ns}" + continue + fi + values_lines+="${I} - \"${_ns}\""$'\n' + done + if [[ -z "$values_lines" ]]; then + # Empty/invalid list would emit values:[] which matches nothing and is not + # a useful config — fall back to the secure default instead. + log_warn "ALLOWED_ROUTE_NAMESPACES has no valid namespace names; falling back to from: Same" + _allowed_routes_same_yaml "$I" + return 0 + fi + printf '%s' \ +"${I}allowedRoutes: +${I} namespaces: +${I} from: Selector +${I} selector: +${I} matchExpressions: +${I} - key: kubernetes.io/metadata.name +${I} operator: In +${I} values: +${values_lines}" + elif [[ -n "${NAMESPACE_SELECTOR_LABELS:-}" ]]; then + local labels_lines="" _pair _key _val + IFS=',' read -ra _pairs <<< "$NAMESPACE_SELECTOR_LABELS" + for _pair in "${_pairs[@]}"; do + _pair="${_pair//[[:space:]]/}" + [[ -z "$_pair" || "$_pair" != *=* ]] && continue + _key="${_pair%%=*}" + _val="${_pair#*=}" + if [[ -z "$_key" ]] || ! _is_valid_k8s_label_key "$_key" || ! _is_valid_k8s_label_value "$_val"; then + log_warn "Ignoring invalid label selector pair in NAMESPACE_SELECTOR_LABELS: ${_pair}" + continue + fi + labels_lines+="${I} ${_key}: \"${_val}\""$'\n' + done + if [[ -z "$labels_lines" ]]; then + # No valid key=value pairs — fall back to the secure default instead of + # emitting an empty matchLabels selector, which would match all namespaces. + log_warn "NAMESPACE_SELECTOR_LABELS has no valid key=value pairs; falling back to from: Same" + _allowed_routes_same_yaml "$I" + return 0 + fi + printf '%s' \ +"${I}allowedRoutes: +${I} namespaces: +${I} from: Selector +${I} selector: +${I} matchLabels: +${labels_lines}" + else + _allowed_routes_same_yaml "$I" + fi +} + +# build_allowed_routes_json +# Outputs the allowedRoutes value as JSON for use with kubectl patch --type=json. +# Uses the same ALLOWED_ROUTE_NAMESPACES / NAMESPACE_SELECTOR_LABELS env vars +# as build_allowed_routes_yaml; defaults to {"namespaces":{"from":"Same"}}. +build_allowed_routes_json() { + if [[ -n "${ALLOWED_ROUTE_NAMESPACES:-}" ]]; then + local values="" _ns + IFS=',' read -ra _ns_arr <<< "$ALLOWED_ROUTE_NAMESPACES" + for _ns in "${_ns_arr[@]}"; do + _ns="${_ns//[[:space:]]/}" + [[ -z "$_ns" ]] && continue + if ! _is_valid_dns1123_label "$_ns"; then + log_warn "Ignoring invalid namespace name in ALLOWED_ROUTE_NAMESPACES: ${_ns}" >&2 + continue + fi + [[ -n "$values" ]] && values+="," + values+="\"${_ns}\"" + done + if [[ -z "$values" ]]; then + log_warn "ALLOWED_ROUTE_NAMESPACES has no valid namespace names; falling back to from: Same" >&2 + printf '{"namespaces":{"from":"Same"}}' + return 0 + fi + printf '{"namespaces":{"from":"Selector","selector":{"matchExpressions":[{"key":"kubernetes.io/metadata.name","operator":"In","values":[%s]}]}}}' "$values" + elif [[ -n "${NAMESPACE_SELECTOR_LABELS:-}" ]]; then + local labels_json="{" first=true _pair _key _val + IFS=',' read -ra _pairs <<< "$NAMESPACE_SELECTOR_LABELS" + for _pair in "${_pairs[@]}"; do + _pair="${_pair//[[:space:]]/}" + [[ -z "$_pair" || "$_pair" != *=* ]] && continue + _key="${_pair%%=*}" + _val="${_pair#*=}" + if [[ -z "$_key" ]] || ! _is_valid_k8s_label_key "$_key" || ! _is_valid_k8s_label_value "$_val"; then + log_warn "Ignoring invalid label selector pair in NAMESPACE_SELECTOR_LABELS: ${_pair}" >&2 + continue + fi + [[ "$first" == "true" ]] && first=false || labels_json+="," + labels_json+="\"${_key}\":\"${_val}\"" + done + labels_json+="}" + if [[ "$labels_json" == "{}" ]]; then + # No valid key=value pairs — fall back to the secure default instead of + # emitting matchLabels:{} which matches all namespaces (equivalent to from: All). + log_warn "NAMESPACE_SELECTOR_LABELS has no valid key=value pairs; falling back to from: Same" >&2 + printf '{"namespaces":{"from":"Same"}}' + return 0 + fi + printf '{"namespaces":{"from":"Selector","selector":{"matchLabels":%s}}}' "$labels_json" + else + printf '{"namespaces":{"from":"Same"}}' + fi +} + +# patch_gateway_allowed_routes +# Ensures ALL listeners' allowedRoutes on an existing Gateway match the desired +# configuration. Patches when: +# - Any listener has from: All (upgrades insecure default), OR +# - ALLOWED_ROUTE_NAMESPACES or NAMESPACE_SELECTOR_LABELS is set (applies user config) +# Skips when all listeners are already at a secure non-All state and no custom +# config is requested. Applies the same allowedRoutes to every listener so that +# multi-listener Gateways (e.g. HTTP + HTTPS) are patched consistently. +# +# Arguments: +# gateway_name - Name of the Gateway resource +# gateway_namespace - Namespace of the Gateway +patch_gateway_allowed_routes() { + local gateway_name="$1" + local gateway_namespace="$2" + + # Count listeners without requiring jq: emit one 'x' per listener then count chars. + local listener_count + if ! listener_count=$(kubectl get gateway "$gateway_name" -n "$gateway_namespace" \ + -o jsonpath='{range .spec.listeners[*]}x{end}' 2>/dev/null | wc -c | tr -d ' '); then + log_error "Unable to read Gateway ${gateway_namespace}/${gateway_name} for allowedRoutes update" + return 1 + fi + if [[ "$listener_count" -eq 0 ]]; then + log_debug " Gateway ${gateway_namespace}/${gateway_name} has no listeners — skipping allowedRoutes patch" + return 0 + fi + + local has_custom_config=false + [[ -n "${ALLOWED_ROUTE_NAMESPACES:-}" || -n "${NAMESPACE_SELECTOR_LABELS:-}" ]] && has_custom_config=true + + # Check whether any listener still carries the insecure from: All default. + local any_all=false + local i current_from + for ((i=0; i/dev/null || echo "") + [[ "$current_from" == "All" ]] && any_all=true && break + done + + # Skip if all listeners are already secure and no custom config is requested. + if [[ "$any_all" == "false" && "$has_custom_config" == "false" ]]; then + log_debug " Gateway allowedRoutes already secure on all listeners — skipping" + return 0 + fi + + if [[ "${DRY_RUN:-false}" == "true" ]]; then + log_info " [DRY RUN] Would update allowedRoutes on ${listener_count} listener(s)" + return 0 + fi + + log_info " Updating Gateway allowedRoutes on ${listener_count} listener(s)..." + local json patch_ops="" sep="" + json="$(build_allowed_routes_json)" + # Build a single JSON patch array covering every listener. + # op:add is safe for both present and absent allowedRoutes fields (RFC 6902 §4.1). + for ((i=0; i/dev/null; then - KUADRANT_PODS=$(kubectl get pods -n kuadrant-system --no-headers 2>/dev/null | grep -c "Running" || echo "0") - if [ "$KUADRANT_PODS" -gt 0 ]; then - print_success "Kuadrant has $KUADRANT_PODS running pod(s)" + POLICY_ENGINE_NAMESPACE="kuadrant-system" +elif kubectl get namespace rh-connectivity-link &>/dev/null; then + POLICY_ENGINE_NAMESPACE="rh-connectivity-link" +fi +if [[ -n "$POLICY_ENGINE_NAMESPACE" ]]; then + POLICY_ENGINE_PODS=$(kubectl get pods -n "$POLICY_ENGINE_NAMESPACE" --no-headers 2>/dev/null | grep -c "Running" || true) + [[ "$POLICY_ENGINE_PODS" =~ ^[0-9]+$ ]] || POLICY_ENGINE_PODS=0 + if [ "$POLICY_ENGINE_PODS" -gt 0 ]; then + print_success "Policy engine has $POLICY_ENGINE_PODS running pod(s) in $POLICY_ENGINE_NAMESPACE" else - print_fail "No Kuadrant pods running" "Kuadrant operators may not be installed" "Check: kubectl get pods -n kuadrant-system" + print_fail "No policy engine pods running" "RHCL/Kuadrant operators may not be installed" "Check: kubectl get pods -n $POLICY_ENGINE_NAMESPACE" fi else - print_fail "Kuadrant namespace not found" "Kuadrant may not be installed" "Run: ./scripts/install-dependencies.sh --kuadrant" + print_fail "Policy engine namespace not found" "RHCL or Kuadrant may not be installed" "Run deploy.sh with --policy-engine rhcl or --policy-engine kuadrant" fi # Check OpenDataHub/KServe pods diff --git a/scripts/verify-models-and-limits.sh b/scripts/verify-models-and-limits.sh index de4fbdef4..626391de4 100755 --- a/scripts/verify-models-and-limits.sh +++ b/scripts/verify-models-and-limits.sh @@ -127,7 +127,7 @@ echo -e "${GREEN}✓ API key created successfully (name: $KEY_NAME)${NC}" echo -e "${BLUE}Discovering available models...${NC}" MODELS_RESPONSE=$(curl -sSk \ - -H "Authorization: Bearer $TOKEN" \ + -H "Authorization: Bearer $OC_TOKEN" \ -H "Content-Type: application/json" \ -w "\nHTTP_STATUS:%{http_code}\n" \ "${API_BASE}/maas-api/v1/models" 2>&1) @@ -202,7 +202,7 @@ EOF ) response=$(curl -sSk \ - -H "Authorization: Bearer $TOKEN" \ + -H "Authorization: Bearer $OC_TOKEN" \ -H "Content-Type: application/json" \ -X POST \ -d "$REQUEST_BODY" \ @@ -273,7 +273,7 @@ EOF echo -n "Request status: " for i in {1..25}; do response=$(curl -sSk \ - -H "Authorization: Bearer $TOKEN" \ + -H "Authorization: Bearer $OC_TOKEN" \ -H "Content-Type: application/json" \ -X POST \ -d "$REQUEST_BODY_SIMPLE" \ diff --git a/test/e2e/README.md b/test/e2e/README.md index bb67df80e..b9426e77a 100644 --- a/test/e2e/README.md +++ b/test/e2e/README.md @@ -60,7 +60,7 @@ Modules outside the explicit smoke list (for example `test_subscription_list_end ## CI -CI runs `./test/e2e/scripts/prow_run_smoke_test.sh`: pytest on the default smoke modules listed above (including `test_aitenant_lifecycle.py`, `test_tenant_namespace_discovery.py`, `test_gateway_scoped_authpolicy.py`, `test_multi_tenant_integration.py`, and the gated S24/S4 modules), then deployment validation; reports under `ARTIFACT_DIR` when set. +CI runs `./test/e2e/scripts/prow_run_smoke_test.sh`: deploys MaaS with **Red Hat Connectivity Link (RHCL)** from the cluster `redhat-operators` catalog (`POLICY_ENGINE=rhcl` by default, channel head unless `RHCL_STARTING_CSV` is set) into **`kuadrant-system`**, then pytest on the default smoke modules listed above (including `test_aitenant_lifecycle.py`, `test_tenant_namespace_discovery.py`, `test_gateway_scoped_authpolicy.py`, `test_multi_tenant_integration.py`, and the gated S24/S4 modules), then deployment validation; reports under `ARTIFACT_DIR` when set. Multi-tenancy discovery tests run by default in `prow_run_smoke_test.sh`, which sets `ENABLE_TENANT_NAMESPACE_DISCOVERY=true` unless explicitly overridden and patches maas-controller before pytest. If set to `false`, `test_tenant_namespace_discovery.py` and `test_multi_tenant_integration.py` skip. When discovery is enabled, `test_namespace_scoping.py` skips (dormant-mode assumptions). diff --git a/test/e2e/scripts/prow_run_smoke_test.sh b/test/e2e/scripts/prow_run_smoke_test.sh index 446afe5c3..8d903ab4b 100755 --- a/test/e2e/scripts/prow_run_smoke_test.sh +++ b/test/e2e/scripts/prow_run_smoke_test.sh @@ -43,6 +43,10 @@ # DEPLOY_MODE - deploy.sh --deployment-mode to use: kustomize (default, matches default CI) # or operator (exercises ODH's ModelsAsService/AIGateway component # reconcilers directly; required for AI_GATEWAY_OPERATOR_IMAGE) +# POLICY_ENGINE - Rate-limiting policy engine (default: rhcl). Prow uses Red Hat Connectivity Link +# from the cluster redhat-operators catalog (stable channel head) in kuadrant-system. +# RHCL_STARTING_CSV - Optional RHCL operator startingCSV pin (default: unset = channel head) +# RHCL_NAMESPACE - RHCL/Kuadrant workload namespace (default: kuadrant-system) # INSECURE_HTTP - Deploy without TLS and use HTTP for tests (default: false) # Affects deploy.sh (via --disable-tls-backend) and test env # EXTERNAL_OIDC - Enable external OIDC e2e coverage (default: false). When true, deploy.sh runs with @@ -110,7 +114,17 @@ export OPERATOR_IMAGE=${OPERATOR_IMAGE:-} # instead of installing maas-controller/maas-api directly via kustomize. Required when # AI_GATEWAY_OPERATOR_IMAGE is set, since ai-gateway-operator is only deployed by the operator. DEPLOY_MODE=${DEPLOY_MODE:-kustomize} -AUTHORINO_NAMESPACE="kuadrant-system" +# Use RHCL channel head (no startingCSV pin) so CI validates the latest released RHCL. +export POLICY_ENGINE="${POLICY_ENGINE:-rhcl}" +export RHCL_NAMESPACE="${RHCL_NAMESPACE:-kuadrant-system}" +# Optional pin for debugging only; leave unset to follow redhat-operators stable head. +export RHCL_STARTING_CSV="${RHCL_STARTING_CSV:-}" +if [[ "${SKIP_DEPLOYMENT:-false}" == "true" ]]; then + AUTHORINO_NAMESPACE="$(resolve_authorino_namespace)" +else + AUTHORINO_NAMESPACE="$(resolve_authorino_namespace "$POLICY_ENGINE")" +fi +export AUTHORINO_NAMESPACE DEPLOYMENT_NAMESPACE="${DEPLOYMENT_NAMESPACE:-opendatahub}" MAAS_SUBSCRIPTION_NAMESPACE="${MAAS_SUBSCRIPTION_NAMESPACE:-models-as-a-service}" MODEL_NAMESPACE="${MODEL_NAMESPACE:-llm}" @@ -319,13 +333,17 @@ deploy_maas_platform() { echo "Using OIDC issuer: ${OIDC_ISSUER_URL}" fi - # 3. Deploy MaaS via operator (Kuadrant, gateway, maas-api, maas-controller, policies) + # 3. Deploy MaaS via operator (RHCL/Kuadrant, gateway, maas-api, maas-controller, policies) # Note: ODH/catalog already installed by install-odh.sh; deploy.sh will skip duplicate installs # CI Postgres pods do not have TLS; override sslmode to avoid connection failures. export DB_SSLMODE="${DB_SSLMODE:-disable}" + echo "Using policy engine: ${POLICY_ENGINE} (Authorino namespace: ${AUTHORINO_NAMESPACE})" + # deploy.sh includes MODEL_NAMESPACE in Gateway allowedRoutes when exported + export MODEL_NAMESPACE local deploy_cmd=( "$PROJECT_ROOT/scripts/deploy.sh" --deployment-mode "${DEPLOY_MODE}" + --policy-engine "${POLICY_ENGINE}" ) if [[ -n "${OPERATOR_CATALOG:-}" ]]; then deploy_cmd+=(--operator-catalog "${OPERATOR_CATALOG}") @@ -768,7 +786,7 @@ run_e2e_tests() { echo "❌ ERROR: Authenticated gateway access not working after ${auth_timeout}s" echo " The gateway is not forwarding authenticated requests to maas-api." echo " Check AuthPolicy status: kubectl get authpolicy -A -o wide" - echo " Check Authorino logs: kubectl logs -n kuadrant-system -l app=authorino --tail=50" + echo " Check Authorino logs: kubectl logs -n ${AUTHORINO_NAMESPACE} -l app=authorino --tail=50" exit 1 fi @@ -808,7 +826,7 @@ run_e2e_tests() { done if [[ $SECONDS -ge $oidc_deadline ]]; then echo "⚠️ WARNING: OIDC gateway readiness failed after ${oidc_timeout}s (still HTTP 401)." - echo " Issuer check already passed; suspect JWKS/network from kuadrant-system to Keycloak or token signature." + echo " Issuer check already passed; suspect JWKS/network from ${AUTHORINO_NAMESPACE} to Keycloak or token signature." echo " kubectl get authpolicy maas-gateway-auth -n ${GATEWAY_NAMESPACE:-openshift-ingress} -o yaml | grep -A30 oidc" echo " kubectl logs -n ${AUTHORINO_NAMESPACE} -l app=authorino --tail=80" if [[ "${OIDC_READINESS_STRICT}" == "true" ]]; then diff --git a/test/e2e/tests/multitenancy_helpers.py b/test/e2e/tests/multitenancy_helpers.py index 2c61ea802..aa88cb713 100644 --- a/test/e2e/tests/multitenancy_helpers.py +++ b/test/e2e/tests/multitenancy_helpers.py @@ -16,6 +16,8 @@ from test_helper import ( DEPLOYMENT_NAMESPACE, + GATEWAY_PROPAGATION_DELAY, + GATEWAY_PROPAGATION_RETRIES, MAAS_API_DEPLOYMENT_NAMESPACE, MODEL_NAMESPACE, MODEL_REF, @@ -24,6 +26,7 @@ _apply_cr, _delete_cr, _ns, + _request_with_gateway_retry, _wait_reconcile, ) @@ -679,10 +682,11 @@ def _predicate(obj: dict) -> bool: parent_namespace = parent_ref.get("namespace") or gateway_namespace if parent_ref.get("name") != gateway_name or parent_namespace != gateway_namespace: continue - return any( + if any( condition.get("type") == "Accepted" and condition.get("status") == "True" for condition in parent.get("conditions") or [] - ) + ): + return True return False return wait_for_json("httproute", route_name, namespace, predicate=_predicate, timeout=timeout, interval=interval) @@ -1070,14 +1074,12 @@ def get_ipp_deployment_env(deployment_name: str, namespace: str = GATEWAY_NAMESP def envoyfilter_target_gateway(name: str, namespace: str = GATEWAY_NAMESPACE) -> str: + """Return the gateway this IPP EnvoyFilter selects via workloadSelector.""" envoyfilter = get_json_or_none("envoyfilter", name, namespace) if not envoyfilter: return "" - spec = envoyfilter.get("spec") or {} - target_refs = spec.get("targetRefs") or [] - if target_refs: - return target_refs[0].get("name") or "" - return (spec.get("targetRef") or {}).get("name") or "" + ws_labels = ((envoyfilter.get("spec") or {}).get("workloadSelector") or {}).get("labels") or {} + return ws_labels.get("gateway.networking.k8s.io/gateway-name") or "" def envoyfilter_grpc_cluster_names(envoyfilter: dict) -> list[str]: @@ -1281,13 +1283,21 @@ def bearer_headers(token: str) -> dict[str, str]: def create_api_key_at(base_url: str, oc_token: str, name: str, *, subscription: Optional[str] = None) -> requests.Response: + """Create an API key against a tenant-scoped maas-api URL. + + Retries empty 403 / Authorino AUTH_FAILURE while per-tenant gateway + AuthPolicy enforcement catches up (same flake mode as single-tenant helpers). + """ body: dict[str, str] = {"name": name} if subscription: body["subscription"] = subscription - return requests.post( + return _request_with_gateway_retry( + requests.post, f"{base_url}/v1/api-keys", headers=bearer_headers(oc_token), json=body, + retries=GATEWAY_PROPAGATION_RETRIES, + delay=GATEWAY_PROPAGATION_DELAY, timeout=TIMEOUT, verify=TLS_VERIFY, ) diff --git a/test/e2e/tests/test_api_keys.py b/test/e2e/tests/test_api_keys.py index 88e1ac4b3..af63a8b31 100644 --- a/test/e2e/tests/test_api_keys.py +++ b/test/e2e/tests/test_api_keys.py @@ -57,57 +57,27 @@ _get_cr, _maas_api_url, _ns, + _request_with_gateway_retry, _sa_to_user, _scale_controller_down, _scale_controller_up, + _wait_for_gateway_auth_enforced, _wait_for_maas_subscription_phase, _wait_reconcile, ) log = logging.getLogger(__name__) -# --------------------------------------------------------------------------- -# Gateway propagation retry helper -# --------------------------------------------------------------------------- -# Kuadrant gateway propagation can lag behind MaaS CR readiness. -# MaaSAuthPolicy "Active" means the controller created the Kuadrant AuthPolicy, -# but Envoy may not have loaded it yet. Retry on empty 403 (gateway rejection). -GATEWAY_PROPAGATION_RETRIES = 6 -GATEWAY_PROPAGATION_DELAY = 5 # seconds - - -def _request_with_gateway_retry(method, url, retries=GATEWAY_PROPAGATION_RETRIES, **kwargs): - """Make an HTTP request, retrying on transient gateway/auth errors. - - Retries on: - - Empty 403: Envoy hasn't loaded the AuthPolicy yet. - - 500 with AUTH_FAILURE: Authorino race condition during AuthConfig updates - (metadata evaluators not yet linked after policy reconciliation). - - Returns the last response — the caller's assertion will surface the - failure clearly if the gateway never becomes ready. - """ - for attempt in range(1, retries + 1): - r = method(url, timeout=kwargs.pop("timeout", 30), verify=kwargs.pop("verify", TLS_VERIFY), **kwargs) - retryable = (r.status_code == 403 and not r.text.strip()) or ( - r.status_code == 500 and "AUTH_FAILURE" in r.text - ) - if retryable and attempt < retries: - log.info("Gateway returned %d (attempt %d/%d), retrying in %ds...", - r.status_code, attempt, retries, GATEWAY_PROPAGATION_DELAY) - time.sleep(GATEWAY_PROPAGATION_DELAY) - continue - return r - return r # last attempt's response — assertion will catch the failure - @pytest.fixture(scope="session", autouse=True) def _warm_gateway(api_keys_base_url: str, headers: dict): - """Wait for the gateway to start forwarding requests before running tests. + """Wait for gateway AuthPolicy enforcement before running API key tests. - Makes a single request with gateway retry to absorb the propagation delay. - All subsequent tests can then make requests directly without retry logic. + MaaSAuthPolicy churn in earlier modules (or this session) can leave + maas-gateway-auth reconciling; empty 403s follow until Enforced=True and + Envoy loads the config. """ + _wait_for_gateway_auth_enforced() r = _request_with_gateway_retry( requests.post, api_keys_base_url, @@ -1558,26 +1528,36 @@ def test_search_filters_by_subscription(self, api_keys_base_url: str, headers: d _delete_cr("maassubscription", sub_a, namespace=ns) _delete_cr("maasauthpolicy", f"{sub_a}-auth", namespace=ns) _delete_sa(sa_name, namespace=MODEL_NAMESPACE) - _wait_reconcile() + # Deleting MaaSAuthPolicies rewrites maas-gateway-auth; wait until + # Kuadrant reports Enforced again before the next test hits maas-api. + _wait_for_gateway_auth_enforced() def test_search_without_subscription_returns_all(self, api_keys_base_url: str, headers: dict): """Search without subscription filter returns keys across all subscriptions.""" + # Prior tests may have just mutated MaaSAuthPolicies; ensure gateway auth + # is Enforced before minting keys (avoids empty 403 flakes). + _wait_for_gateway_auth_enforced() key_ids = [] try: # Create keys with explicit subscription binding for i in range(2): - r = requests.post( + r = _request_with_gateway_retry( + requests.post, api_keys_base_url, headers=headers, json={"name": f"e2e-nofilter-{i}", "subscription": SIMULATOR_SUBSCRIPTION}, timeout=TIMEOUT, verify=TLS_VERIFY, ) - assert r.status_code in (200, 201), f"Failed to create key: {r.text}" + assert r.status_code in (200, 201), ( + f"Failed to create key: {r.status_code} " + f"{r.text.strip() or '(empty body — gateway AuthPolicy not ready)'}" + ) key_ids.append(r.json()["id"]) # Search without subscription filter - r_search = requests.post( + r_search = _request_with_gateway_retry( + requests.post, f"{api_keys_base_url}/search", headers=headers, json={ @@ -1587,7 +1567,10 @@ def test_search_without_subscription_returns_all(self, api_keys_base_url: str, h timeout=TIMEOUT, verify=TLS_VERIFY, ) - assert r_search.status_code == 200, f"Search failed: {r_search.status_code} {r_search.text}" + assert r_search.status_code == 200, ( + f"Search failed: {r_search.status_code} " + f"{r_search.text.strip() or '(empty body — gateway AuthPolicy not ready)'}" + ) items = r_search.json().get("items") or r_search.json().get("data") or [] result_ids = [item["id"] for item in items] diff --git a/test/e2e/tests/test_gateway_scoped_authpolicy.py b/test/e2e/tests/test_gateway_scoped_authpolicy.py index 6a56d9468..89c52dacc 100644 --- a/test/e2e/tests/test_gateway_scoped_authpolicy.py +++ b/test/e2e/tests/test_gateway_scoped_authpolicy.py @@ -8,6 +8,7 @@ """ import json +import logging import uuid import pytest @@ -24,12 +25,24 @@ from test_helper import ( MODEL_NAMESPACE, MODEL_REF, + _create_api_key, + _create_sa_token, _create_test_auth_policy, + _create_test_subscription, _delete_cr, + _delete_sa, + _get_cr, + _ns, + _sa_to_user, + _scale_kuadrant_controller_down, + _scale_kuadrant_controller_up, + _wait_for_gateway_auth_enforced, _wait_for_maas_auth_policy_phase, _wait_reconcile, ) +log = logging.getLogger(__name__) + def _gateway_auth_rego() -> str: ap = get_gateway_authpolicy() @@ -192,3 +205,122 @@ def test_gateway_default_auth_scoped_if_present(self): "gateway-default-auth predicate must include header-based model identity check, " f"got: {predicate}" ) + + +class TestEnforcementGapAfterAuthPolicyChange: + """RHOAIENG-79568: Auth enforcement gap when a spec change updates the gateway AuthPolicy. + + Reproduces the scenario where a new MaaSAuthPolicy (or any change that alters the + aggregated model allowlist) updates the gateway AuthPolicy spec. Kuadrant must + re-process the update; until it does, observedGeneration lags behind generation + and enforcement is stale. + + The test scales Kuadrant down to freeze enforcement, then creates a second + MaaSAuthPolicy to trigger a gateway AuthPolicy spec change. This widens the + normally sub-second enforcement gap to a permanent, observable state. + + Verifies: + - With the controller fix: MaaSAuthPolicy stays Pending while gateway is unenforced + - Without the fix: MaaSAuthPolicy falsely reports Active + """ + + def test_controller_holds_pending_while_unenforced(self): + """With Kuadrant down, MaaSAuthPolicy must NOT reach Active after a + gateway AuthPolicy spec change. + + Steps: + 1. Establish baseline: auth policy Active, gateway Enforced + 2. Scale Kuadrant down (freeze enforcement) + 3. Create a second MaaSAuthPolicy (changes aggregated allowlist → spec update) + 4. Wait for maas-controller to reconcile + 5. Check MaaSAuthPolicy phase — should be Pending (not Active) + 6. Scale Kuadrant back up, wait for enforcement + 7. Verify MaaSAuthPolicy reaches Active and API key creation succeeds + """ + ns = _ns() + suffix = uuid.uuid4().hex[:8] + auth_name_1 = f"e2e-enforce-gap-auth1-{suffix}" + auth_name_2 = f"e2e-enforce-gap-auth2-{suffix}" + sub_name = f"e2e-enforce-gap-sub-{suffix}" + sa_name = f"e2e-enforce-gap-sa-{suffix}" + + try: + # Step 1: Establish baseline with first auth policy. + oc_token = _create_sa_token(sa_name, namespace=MODEL_NAMESPACE) + sa_user = _sa_to_user(sa_name, namespace=MODEL_NAMESPACE) + + _create_test_auth_policy(auth_name_1, MODEL_REF, users=[sa_user]) + _create_test_subscription(sub_name, MODEL_REF, users=[sa_user]) + _wait_for_maas_auth_policy_phase(auth_name_1, timeout=120, require_enforced=True) + _wait_for_gateway_auth_enforced() + log.info("Step 1: Baseline established — auth Active, gateway Enforced") + + # Step 2: Scale Kuadrant down to freeze enforcement. + log.info("Step 2: Scaling Kuadrant down to freeze enforcement...") + _scale_kuadrant_controller_down() + + # Step 3: Create a second MaaSAuthPolicy with a different group. + # This changes the aggregated model allowlist, which changes the gateway + # AuthPolicy spec. The maas-controller will update the spec, but Kuadrant + # (now down) cannot re-process it → observedGeneration lags → not enforced. + log.info("Step 3: Creating second MaaSAuthPolicy to trigger spec change...") + unique_group = f"e2e-trigger-group-{suffix}" + _create_test_auth_policy(auth_name_2, MODEL_REF, groups=[unique_group]) + + # Step 4: Wait for maas-controller to reconcile. + # The controller updates the gateway AuthPolicy, then checks enforcement. + # With Kuadrant down, observedGeneration won't catch up → Enforced stale. + _wait_reconcile(seconds=15) + + # Step 5: Check MaaSAuthPolicy phase. + # Check both policies — both should be affected by the enforcement gate. + cr1 = _get_cr("maasauthpolicy", auth_name_1, namespace=ns) + cr2 = _get_cr("maasauthpolicy", auth_name_2, namespace=ns) + phase1 = (cr1 or {}).get("status", {}).get("phase", "unknown") + phase2 = (cr2 or {}).get("status", {}).get("phase", "unknown") + log.info("Step 5: MaaSAuthPolicy phases: %s=%s, %s=%s", + auth_name_1, phase1, auth_name_2, phase2) + + if phase2 == "Pending": + log.info("Controller correctly holding MaaSAuthPolicy in Pending " + "while gateway AuthPolicy is not enforced") + elif phase2 == "Active": + log.warning("MaaSAuthPolicy is Active despite gateway not being enforced " + "— controller is NOT checking enforcement (pre-fix behavior)") + + # Step 6: Scale Kuadrant back up and wait for enforcement. + log.info("Step 6: Scaling Kuadrant back up...") + _scale_kuadrant_controller_up() + _wait_for_gateway_auth_enforced(timeout=180) + _wait_for_maas_auth_policy_phase( + auth_name_1, "Active", timeout=120, require_enforced=True + ) + log.info("Step 6: Enforcement restored") + + # Step 7: Verify API key creation succeeds. + api_key = _create_api_key(oc_token, name=f"post-gap-{suffix}", subscription=sub_name) + assert api_key and api_key.startswith("sk-"), ( + f"Expected valid API key after enforcement restored, got: " + f"{api_key[:20] if api_key else None}" + ) + log.info("Step 7: API key creation succeeded after enforcement restored") + + # Final assertion: the second auth policy (the one that triggered the + # spec change) should have been held in Pending while Kuadrant was down. + assert phase2 == "Pending", ( + f"RHOAIENG-79568: MaaSAuthPolicy '{auth_name_2}' was '{phase2}' while " + f"gateway AuthPolicy was NOT enforced (Kuadrant was down). " + f"With the enforcement-check fix, the controller should hold Pending " + f"until Enforced=True." + ) + + finally: + try: + _scale_kuadrant_controller_up() + except Exception as e: + log.warning("Failed to scale Kuadrant up during cleanup: %s", e) + _delete_cr("maassubscription", sub_name, namespace=ns) + _delete_cr("maasauthpolicy", auth_name_2, namespace=ns) + _delete_cr("maasauthpolicy", auth_name_1, namespace=ns) + _delete_sa(sa_name, namespace=MODEL_NAMESPACE) + _wait_reconcile() diff --git a/test/e2e/tests/test_helper.py b/test/e2e/tests/test_helper.py index 7ed91e51e..34181f2b8 100644 --- a/test/e2e/tests/test_helper.py +++ b/test/e2e/tests/test_helper.py @@ -40,7 +40,12 @@ - E2E_DISTINCT_MODEL_2_ID: Canonical BBR model ID for second distinct model (default: publishers/{MODEL_NAMESPACE}/models/test/e2e-distinct-model-2) - E2E_TRLP_TEST_MODEL_REF: TRLP test model ref (default: e2e-trlp-test-simulated) - E2E_TRLP_TEST_MODEL_PATH: Path to TRLP test model (default: /llm/e2e-trlp-test-simulated) - - E2E_TRLP_TEST_MODEL_ID: Model ID for TRLP test model (default: test/e2e-trlp-test-model) + - E2E_TRLP_TEST_MODEL_ID: Model ID for TRLP test model (default: test/e2e-trlp-test-model) + - E2E_GATEWAY_AUTH_POLICY_NAME: Gateway Kuadrant AuthPolicy name (default: maas-gateway-auth) + - E2E_GATEWAY_PROPAGATION_RETRIES: Retries for empty 401/403 / AUTH_FAILURE (default: 6) + - E2E_GATEWAY_PROPAGATION_DELAY: Delay between gateway retries in seconds (default: 5) + - E2E_GATEWAY_ENFORCED_TIMEOUT: Wait for AuthPolicy Accepted+Enforced (default: 180) + - E2E_GATEWAY_ENFORCED_MISSING_GRACE: Fail early if AuthPolicy CR absent this long (default: 30) """ import base64 @@ -71,6 +76,17 @@ # Clients must use this form in the "model" field when targeting the BBR gateway endpoint. MODEL_CANONICAL_ID = os.environ.get("E2E_MODEL_CANONICAL_ID", f"publishers/{MODEL_NAMESPACE}/models/{MODEL_NAME}") DEPLOYMENT_NAMESPACE = os.environ.get("DEPLOYMENT_NAMESPACE", "opendatahub") +# Kuadrant gateway AuthPolicy that Authorino enforces for maas-api + model routes. +GATEWAY_AUTH_POLICY_NAME = os.environ.get("E2E_GATEWAY_AUTH_POLICY_NAME", "maas-gateway-auth") +# Empty 403 / Authorino AUTH_FAILURE while Envoy catches up after AuthPolicy updates. +GATEWAY_PROPAGATION_RETRIES = int(os.environ.get("E2E_GATEWAY_PROPAGATION_RETRIES", "6")) +GATEWAY_PROPAGATION_DELAY = int(os.environ.get("E2E_GATEWAY_PROPAGATION_DELAY", "5")) +# Wait for maas-gateway-auth Accepted+Enforced before minting keys / calling maas-api. +# 180s covers AuthConfig backlog after heavy MaaSAuthPolicy churn in Konflux e2e. +GATEWAY_ENFORCED_TIMEOUT = int(os.environ.get("E2E_GATEWAY_ENFORCED_TIMEOUT", "180")) +# If the AuthPolicy CR is missing this long, fail fast (misconfigured name/namespace) +# instead of burning the full GATEWAY_ENFORCED_TIMEOUT. +GATEWAY_ENFORCED_MISSING_GRACE = int(os.environ.get("E2E_GATEWAY_ENFORCED_MISSING_GRACE", "30")) def _derive_infra_namespace(controller_namespace: str) -> str: @@ -244,9 +260,47 @@ def _get_cluster_token(): # API Key Management # --------------------------------------------------------------------------- +def _request_with_gateway_retry(method, url, retries=None, delay=None, **kwargs): + """Make an HTTP request, retrying transient gateway/auth propagation errors. + + Retries on: + - Empty 403 / empty 401: Envoy has not loaded the AuthPolicy yet (common after + MaaSAuthPolicy churn; some gateways return 401 instead of 403). + - 500 with AUTH_FAILURE: Authorino race while AuthConfig is updating. + + Returns the last response — callers' assertions surface a permanent failure. + """ + retries = GATEWAY_PROPAGATION_RETRIES if retries is None else retries + delay = GATEWAY_PROPAGATION_DELAY if delay is None else delay + timeout = kwargs.pop("timeout", TIMEOUT) + verify = kwargs.pop("verify", TLS_VERIFY) + r = None + for attempt in range(1, retries + 1): + r = method(url, timeout=timeout, verify=verify, **kwargs) + empty_auth_reject = r.status_code in (401, 403) and not r.text.strip() + retryable = empty_auth_reject or ( + r.status_code == 500 and "AUTH_FAILURE" in r.text + ) + if retryable and attempt < retries: + log.info( + "Gateway returned %d (attempt %d/%d), retrying in %ds...", + r.status_code, + attempt, + retries, + delay, + ) + time.sleep(delay) + continue + return r + return r + + def _create_api_key_raw(oc_token: str, name: str = None, subscription: str = None): """Create an API key and return the raw response (for testing error cases). + Retries empty 403 / Authorino AUTH_FAILURE so callers see the real API + response after gateway AuthPolicy propagation, not a transient reject. + Args: oc_token: OC token for authentication with maas-api name: Optional name for the key (auto-generated if not provided) @@ -262,7 +316,8 @@ def _create_api_key_raw(oc_token: str, name: str = None, subscription: str = Non if subscription: body["subscription"] = subscription - return requests.post( + return _request_with_gateway_retry( + requests.post, url, headers={ "Authorization": f"Bearer {oc_token}", @@ -287,7 +342,11 @@ def _create_api_key(oc_token: str, name: str = None, subscription: str = None) - """ r = _create_api_key_raw(oc_token, name, subscription) if r.status_code not in (200, 201): - raise RuntimeError(f"Failed to create API key: {r.status_code} {r.text}") + detail = r.text.strip() or ( + "empty body (likely gateway AuthPolicy not yet Enforced / Envoy not loaded; " + f"check: oc get authpolicy {GATEWAY_AUTH_POLICY_NAME} -n {GATEWAY_NAMESPACE})" + ) + raise RuntimeError(f"Failed to create API key: {r.status_code} {detail}") data = r.json() api_key = data.get("key") @@ -743,6 +802,117 @@ def _wait_reconcile(seconds=None): time.sleep(seconds or RECONCILE_WAIT) +def _authpolicy_conditions(cr, *types): + """Return {type: (status, reason, message)} for requested condition types (single pass).""" + wanted = set(types) + result = {t: (None, None, None) for t in wanted} + for condition in (cr or {}).get("status", {}).get("conditions", []): + ctype = condition.get("type") + if ctype in wanted: + result[ctype] = ( + condition.get("status"), + condition.get("reason"), + condition.get("message"), + ) + return result + + +def _authpolicy_condition(cr, condition_type: str): + """Return (status, reason, message) for an AuthPolicy condition type, or (None, None, None).""" + return _authpolicy_conditions(cr, condition_type)[condition_type] + + +def _truncate_auth_message(message: Optional[str], limit: int = 240) -> str: + """Shrink Kuadrant Enforced messages that list dozens of pending AuthConfigs.""" + if not message: + return "" + if len(message) <= limit: + return message + # Prefer a compact count when Kuadrant dumps AuthConfig hashes. + authconfig_count = message.count("AuthConfig (") + if authconfig_count: + return f"{message[:limit]}… ({authconfig_count} AuthConfigs pending sync)" + return message[:limit] + "…" + + +def _wait_for_gateway_auth_enforced( + name: Optional[str] = None, + namespace: Optional[str] = None, + timeout: Optional[int] = None, +): + """Wait until the gateway Kuadrant AuthPolicy is Accepted and Enforced. + + MaaSAuthPolicy phase Active only means the controller reconciled CRs. + HTTP calls through the gateway still need Kuadrant's AuthPolicy + (typically maas-gateway-auth) to report Enforced=True; otherwise Envoy + often returns an empty 403. + + Default timeout is GATEWAY_ENFORCED_TIMEOUT (env E2E_GATEWAY_ENFORCED_TIMEOUT). + If the AuthPolicy CR is absent for GATEWAY_ENFORCED_MISSING_GRACE seconds, + fail early — that usually means a wrong name/namespace, not slow enforcement. + + Raises: + TimeoutError: with Accepted/Enforced snapshot so failures are actionable + """ + name = name or GATEWAY_AUTH_POLICY_NAME + namespace = namespace or GATEWAY_NAMESPACE + timeout = GATEWAY_ENFORCED_TIMEOUT if timeout is None else timeout + deadline = time.time() + timeout + last_snapshot = "AuthPolicy not found" + missing_since = None + warned_missing = False + log.info( + "Waiting for gateway AuthPolicy %s/%s Accepted+Enforced (timeout: %ds)...", + namespace, + name, + timeout, + ) + + while time.time() < deadline: + cr = _get_cr("authpolicy", name, namespace) + if cr is None: + last_snapshot = "AuthPolicy not found" + now = time.time() + if missing_since is None: + missing_since = now + if not warned_missing: + log.warning( + "Gateway AuthPolicy %s/%s not found yet; will fail after %ds if it never appears " + "(check E2E_GATEWAY_AUTH_POLICY_NAME / GATEWAY_NAMESPACE)", + namespace, + name, + GATEWAY_ENFORCED_MISSING_GRACE, + ) + warned_missing = True + elif now - missing_since >= GATEWAY_ENFORCED_MISSING_GRACE: + raise TimeoutError( + f"Gateway AuthPolicy {namespace}/{name} was not found within " + f"{GATEWAY_ENFORCED_MISSING_GRACE}s (misconfigured name/namespace?). " + f"Empty HTTP 403 from maas-api usually means Kuadrant has not finished " + f"enforcing auth on the gateway after MaaSAuthPolicy changes." + ) + else: + missing_since = None + conds = _authpolicy_conditions(cr, "Accepted", "Enforced") + accepted, a_reason, a_msg = conds["Accepted"] + enforced, e_reason, e_msg = conds["Enforced"] + last_snapshot = ( + f"Accepted={accepted} reason={a_reason!r} message={_truncate_auth_message(a_msg)!r}; " + f"Enforced={enforced} reason={e_reason!r} message={_truncate_auth_message(e_msg)!r}" + ) + if accepted == "True" and enforced == "True": + log.info("Gateway AuthPolicy %s/%s is Accepted and Enforced", namespace, name) + return cr + log.debug("Gateway AuthPolicy %s/%s not ready: %s", namespace, name, last_snapshot) + time.sleep(2) + + raise TimeoutError( + f"Gateway AuthPolicy {namespace}/{name} was not Accepted+Enforced within {timeout}s " + f"(last status: {last_snapshot}). Empty HTTP 403 from maas-api usually means Kuadrant " + f"has not finished enforcing auth on the gateway after MaaSAuthPolicy changes." + ) + + def _wait_for_token_rate_limit_policy(model_ref, model_namespace=MODEL_NAMESPACE, timeout=60): """Wait for TokenRateLimitPolicy to be created and enforced for a model. @@ -905,8 +1075,10 @@ def _wait_for_maas_auth_policy_phase(name, expected_phase="Active", namespace=No timeout: Maximum wait time in seconds (default: 60) require_auth_policies: If True, requires authPolicies to be populated (default: False). Keep False for gateway-only AuthPolicy reconciliation. - require_enforced: If True, requires all authPolicies to have ready=True - (default: True). Only applies when require_auth_policies is True. + require_enforced: If True (default): + - with require_auth_policies=True: all status.authPolicies entries must be ready + - with require_auth_policies=False and expected_phase Active: also wait for the + gateway Kuadrant AuthPolicy (maas-gateway-auth) to be Accepted+Enforced Returns: The auth policy CR dict when the expected phase is reached @@ -926,8 +1098,16 @@ def _wait_for_maas_auth_policy_phase(name, expected_phase="Active", namespace=No auth_policies = status.get("authPolicies", []) if phase == expected_phase: - # No auth policies required — phase match is sufficient + # No per-model auth policies required — phase match is sufficient for the CR, + # but gateway-only mode still needs Kuadrant Enforced before HTTP calls. + # Callers that intentionally stop Kuadrant (e.g. TRLP degraded tests) must + # pass require_enforced=False — Enforced cannot become True while Kuadrant is down. if not require_auth_policies: + if require_enforced and expected_phase == "Active": + # Keep a floor so a slow phase wait does not starve Kuadrant Enforced. + # AuthConfig backlog after policy churn often needs the full enforced timeout. + remaining = max(GATEWAY_ENFORCED_TIMEOUT, int(deadline - time.time())) + _wait_for_gateway_auth_enforced(timeout=remaining) log.info(f"MaaSAuthPolicy {name} reached phase '{expected_phase}'") return cr diff --git a/test/e2e/tests/test_multi_tenant_integration.py b/test/e2e/tests/test_multi_tenant_integration.py index eda21b710..e3cc120d7 100644 --- a/test/e2e/tests/test_multi_tenant_integration.py +++ b/test/e2e/tests/test_multi_tenant_integration.py @@ -183,7 +183,6 @@ def test_same_named_resources_across_tenants(self): apply_maas_subscription(shared_sub, case["tenant_ns"]) wait_for_finalizer("maasauthpolicy", shared_policy, case["tenant_ns"], FINALIZER_AUTHPOLICY) wait_for_finalizer("maassubscription", shared_sub, case["tenant_ns"], FINALIZER_SUBSCRIPTION) - wait_for_status_phase("maasauthpolicy", shared_policy, case["tenant_ns"], expected_phase="Active") expected_subs = [f"{case_a['tenant_ns']}/{shared_sub}", f"{case_b['tenant_ns']}/{shared_sub}"] wait_for_annotation_contains( @@ -213,7 +212,6 @@ def test_tenant_namespace_label_change_triggers_reconciliation(self): apply_discovery_labels(case["tenant_ns"], case["tenant_label_name"]) wait_for_finalizer("maasauthpolicy", first_policy, case["tenant_ns"], FINALIZER_AUTHPOLICY) - wait_for_status_phase("maasauthpolicy", first_policy, case["tenant_ns"], expected_phase="Active") remove_discovery_labels(case["tenant_ns"]) _wait_reconcile(10) diff --git a/test/e2e/tests/test_negative_security.py b/test/e2e/tests/test_negative_security.py index 5276c2f71..9e8dd0a8a 100644 --- a/test/e2e/tests/test_negative_security.py +++ b/test/e2e/tests/test_negative_security.py @@ -52,6 +52,7 @@ _inference, _maas_api_url, _poll_status, + _wait_for_gateway_auth_enforced, _wait_for_maas_auth_policy_phase, _wait_for_maas_subscription_phase, ) @@ -80,6 +81,9 @@ def test_injected_identity_headers_ignored(self): The request should succeed (200) using the real key-derived identity, proving the spoofed headers had no effect on authorization. """ + # Earlier tests may have rewritten maas-gateway-auth; minting a key + # against an unenforced gateway yields empty 403 flakes. + _wait_for_gateway_auth_enforced() api_key = _create_api_key(_get_cluster_token(), subscription=SIMULATOR_SUBSCRIPTION) spoofed_headers = { @@ -107,6 +111,7 @@ def test_duplicate_subscription_headers_ignored(self): Duplicate or conflicting X-MaaS-Subscription headers must not override the key-derived subscription. """ + _wait_for_gateway_auth_enforced() api_key = _create_api_key(_get_cluster_token(), subscription=SIMULATOR_SUBSCRIPTION) # Use http.client to send genuinely duplicate X-MaaS-Subscription headers. @@ -231,6 +236,7 @@ def test_key_cannot_access_model_outside_subscription(self): Uses the pre-deployed unconfigured model (a model with no subscription granting access to it) to test cross-model access denial. """ + _wait_for_gateway_auth_enforced() api_key = _create_api_key(_get_cluster_token(), subscription=SIMULATOR_SUBSCRIPTION) # The unconfigured model exists but has no subscription granting access. @@ -420,6 +426,7 @@ def test_special_characters_in_subscription_header(self): Ensures the platform returns a clean 403 (subscription not found) without leaking errors, stack traces, or SQL/NoSQL injection. """ + _wait_for_gateway_auth_enforced() api_key = _create_api_key(_get_cluster_token(), subscription=SIMULATOR_SUBSCRIPTION) injection_payloads = [ diff --git a/test/e2e/tests/test_per_tenant_ipp_isolation.py b/test/e2e/tests/test_per_tenant_ipp_isolation.py index bf5ae4c4d..df532b9ad 100644 --- a/test/e2e/tests/test_per_tenant_ipp_isolation.py +++ b/test/e2e/tests/test_per_tenant_ipp_isolation.py @@ -223,18 +223,19 @@ def test_per_tenant_ipp_env_vars(self, ipp_tenant_cases): f"{names['processing_deployment']} TENANT_NAMESPACE mismatch: {env!r}" ) - def test_per_tenant_envoyfilter_target_ref_isolated(self, ipp_tenant_cases): + def test_per_tenant_envoyfilter_workload_selector_isolated(self, ipp_tenant_cases): for case in ipp_tenant_cases: names = per_tenant_ipp_names(case["tenant_label_name"]) target = envoyfilter_target_gateway(names["envoyfilter"], GATEWAY_NAMESPACE) assert target == case["gateway_name"], ( - f"{names['envoyfilter']} must target gateway {case['gateway_name']}, got {target!r}" + f"{names['envoyfilter']} workloadSelector must select gateway " + f"{case['gateway_name']}, got {target!r}" ) default_target = envoyfilter_target_gateway("payload-processing", GATEWAY_NAMESPACE) assert default_target == DEFAULT_GATEWAY_NAME, ( - f"default payload-processing EnvoyFilter must target {DEFAULT_GATEWAY_NAME}, " - f"got {default_target!r}" + f"default payload-processing EnvoyFilter workloadSelector must select " + f"{DEFAULT_GATEWAY_NAME}, got {default_target!r}" ) def test_per_tenant_envoyfilter_grpc_clusters(self, ipp_tenant_cases): diff --git a/test/e2e/tests/test_subscription.py b/test/e2e/tests/test_subscription.py index 881e20bde..e0e1591b4 100644 --- a/test/e2e/tests/test_subscription.py +++ b/test/e2e/tests/test_subscription.py @@ -87,6 +87,7 @@ _revoke_api_key, _sa_to_user, _snapshot_cr, + _wait_for_gateway_auth_enforced, _wait_for_maas_auth_policy_phase, _wait_for_maas_subscription_phase, _wait_for_token_rate_limit_policy, @@ -1953,13 +1954,14 @@ def test_subscription_degraded_trlp_blocks_inference(self): Uses pre-deployed e2e-trlp-test-simulated model to avoid TRLP sharing with concurrent tests. Test flow: - 1. Scale down Kuadrant controller - 2. Create subscription with valid model - TRLP created but not accepted - 3. Wait for subscription to enter Degraded phase (TRLP ready=false) - 4. Create API key and verify inference is blocked (403 Forbidden) - 5. Scale Kuadrant controller back up - 6. Wait for subscription to reach Active phase (TRLP ready=true) - 7. Verify inference works (200 OK) + 1. Create auth policy with Kuadrant up — wait for Active + Enforced + 2. Scale down Kuadrant controller + 3. Create subscription — TRLP created but not accepted + 4. Wait for subscription to enter Degraded phase (TRLP ready=false) + 5. Create API key and verify inference is blocked (403 Forbidden) + 6. Scale Kuadrant controller back up + 7. Wait for subscription to reach Active phase (TRLP ready=true) + 8. Verify inference works (200 OK) """ ns = _ns() subscription_name = "e2e-trlp-degraded-sub" @@ -1967,25 +1969,36 @@ def test_subscription_degraded_trlp_blocks_inference(self): sa_name = "e2e-trlp-degraded-sa" try: - # Step 1: Scale down Kuadrant controller BEFORE creating subscription - log.info("Step 1: Scaling down Kuadrant controller...") - _scale_kuadrant_controller_down() - - # Step 2: Create auth policy and subscription - log.info("Step 2: Creating subscription with Kuadrant controller down...") + # Step 1: Create auth policy with Kuadrant UP so it can reach Active + Enforced. + # The controller gates MaaSAuthPolicy on gateway AuthPolicy enforcement + # (RHOAIENG-79568), so Kuadrant must be running for the phase to reach Active. + log.info("Step 1: Creating auth policy with Kuadrant up...") sa_token = _create_sa_token(sa_name, namespace=MODEL_NAMESPACE) sa_user = _sa_to_user(sa_name, namespace=MODEL_NAMESPACE) _create_test_auth_policy(auth_name, TRLP_TEST_MODEL_REF, users=[sa_user]) - _create_test_subscription(subscription_name, TRLP_TEST_MODEL_REF, users=[sa_user]) + _wait_for_maas_auth_policy_phase( + auth_name, + "Active", + timeout=120, + require_auth_policies=False, + ) + log.info("Step 1: Auth policy Active + Enforced") + + # Step 2: Scale down Kuadrant controller BEFORE creating subscription. + # The auth policy is already Active and the gateway AuthPolicy is Enforced. + # Creating a subscription does NOT change the gateway AuthPolicy spec + # (only MaaSAuthPolicy subjects feed the allowlist), so no enforcement + # re-check is triggered. + log.info("Step 2: Scaling down Kuadrant controller...") + _scale_kuadrant_controller_down() - # Wait for auth policy to reconcile. In gateway-only mode, it remains Active even when - # Kuadrant TRLP reconciliation is degraded. - log.info("Waiting for MaaSAuthPolicy to reconcile...") - _wait_for_maas_auth_policy_phase(auth_name, "Active", timeout=60, require_auth_policies=False) + # Step 3: Create subscription — TRLP cannot be reconciled with Kuadrant down. + log.info("Step 3: Creating subscription with Kuadrant controller down...") + _create_test_subscription(subscription_name, TRLP_TEST_MODEL_REF, users=[sa_user]) - # Step 3: Wait for subscription to reach Degraded phase with TRLP not ready - log.info("Step 3: Waiting for subscription to enter Degraded phase (TRLP not ready)...") + # Step 4: Wait for subscription to reach Degraded phase with TRLP not ready + log.info("Step 4: Waiting for subscription to enter Degraded phase (TRLP not ready)...") cr = _wait_for_maas_subscription_phase(subscription_name, "Degraded", timeout=120) _wait_for_subscription_trlp_status(subscription_name, expected_ready=False, timeout=120) @@ -1998,22 +2011,24 @@ def test_subscription_degraded_trlp_blocks_inference(self): assert any(not trlp.get("ready") for trlp in trlp_statuses), "Expected at least one TRLP to be not ready" log.info("✅ Subscription in Degraded phase with TRLP not ready") - # Step 4: Create API key and verify inference is blocked - log.info("Step 4: Creating API key and verifying inference is blocked...") + # Step 5: Create API key and verify inference is blocked + log.info("Step 5: Creating API key and verifying inference is blocked...") api_key = _create_api_key(sa_token, name="e2e-trlp-test-key", subscription=subscription_name) resp = _poll_status(api_key, 403, path=TRLP_TEST_MODEL_PATH, model_name=TRLP_TEST_MODEL_ID, timeout=60) assert resp.status_code == 403, f"Expected 403 Forbidden for Degraded subscription with TRLP not ready, got {resp.status_code}: {resp.text}" log.info("✅ Inference blocked for Degraded subscription with TRLP not ready") - # Step 5: Scale Kuadrant controller back up - log.info("Step 5: Scaling Kuadrant controller back up...") + # Step 6: Scale Kuadrant controller back up + log.info("Step 6: Scaling Kuadrant controller back up...") _scale_kuadrant_controller_up() - # Step 6: Wait for subscription to reach Active phase with TRLP ready - log.info("Step 6: Waiting for subscription to reach Active phase (TRLP ready)...") + # Step 7: Wait for subscription to reach Active phase with TRLP ready + log.info("Step 7: Waiting for subscription to reach Active phase (TRLP ready)...") _wait_for_maas_subscription_phase(subscription_name, "Active", timeout=120) _wait_for_subscription_trlp_status(subscription_name, expected_ready=True, timeout=120) + # Drain AuthConfig backlog from the Kuadrant-down window before HTTP checks. + _wait_for_gateway_auth_enforced() cr = _get_cr("maassubscription", subscription_name, namespace=ns) status = cr.get("status", {}) @@ -2024,8 +2039,8 @@ def test_subscription_degraded_trlp_blocks_inference(self): assert all(trlp.get("ready") for trlp in trlp_statuses), "Expected all TRLPs to be ready" log.info("✅ Subscription returned to Active phase with all TRLPs ready") - # Step 7: Verify inference works (poll to allow Envoy config propagation) - log.info("Step 7: Verifying inference works with Active subscription...") + # Step 8: Verify inference works (poll to allow Envoy config propagation) + log.info("Step 8: Verifying inference works with Active subscription...") resp = _poll_status(api_key, 200, path=TRLP_TEST_MODEL_PATH, model_name=TRLP_TEST_MODEL_ID, timeout=60) assert resp.status_code == 200, f"Expected 200 OK for Active subscription, got {resp.status_code}: {resp.text}" log.info("✅ Inference works with Active subscription after Kuadrant recovery") diff --git a/test/e2e/tests/test_tenant_namespace_discovery.py b/test/e2e/tests/test_tenant_namespace_discovery.py index 1a2c28160..7e66c3183 100644 --- a/test/e2e/tests/test_tenant_namespace_discovery.py +++ b/test/e2e/tests/test_tenant_namespace_discovery.py @@ -223,7 +223,6 @@ def test_namespace_qualified_collision_prevention(self): apply_maas_subscription(shared_sub_name, case["tenant_ns"]) wait_for_finalizer("maasauthpolicy", shared_policy_name, case["tenant_ns"], FINALIZER_AUTHPOLICY) wait_for_finalizer("maassubscription", shared_sub_name, case["tenant_ns"], FINALIZER_SUBSCRIPTION) - _wait_for_maas_auth_policy_phase(shared_policy_name, namespace=case["tenant_ns"], timeout=120) assert_no_per_model_authpolicy(MODEL_REF, MODEL_NAMESPACE) assert get_gateway_authpolicy() is not None