Skip to content

refactor(kustomize): consolidate deployment manifests#132

Merged
bartoszmajsak merged 2 commits intoopendatahub-io:mainfrom
bartoszmajsak:deployment/chore/consolidate-maas-api-manifests
Oct 3, 2025
Merged

refactor(kustomize): consolidate deployment manifests#132
bartoszmajsak merged 2 commits intoopendatahub-io:mainfrom
bartoszmajsak:deployment/chore/consolidate-maas-api-manifests

Conversation

@bartoszmajsak
Copy link
Copy Markdown
Collaborator

@bartoszmajsak bartoszmajsak commented Oct 2, 2025

Important

We should be using ODH version of KServe - especially since certain features are not upstreamed yet (or even released there). That's why kserve folder is removed - to avoid confusion.

This change introduces a single source of truth to make deployments simpler.

Existing maas-api/deploy manifests are promoted to top-level deployment folder, reducing duplication and improving the structure.

Installation script for OpenShift has been adjusted:

  • Fixed OCP version check - using cluster info instead as it's more reliable
  • Add helpers to wait for CRDs/pods and a version comparator;
  • Auto-detect and patch AuthPolicy audience; apply gateway policies in sequence after infra becomes ready.
  • Detect existing ODH/RHOAI KServe; deploy components only when missing, then proceed with retries.

Summary by CodeRabbit

  • New Features

    • Version-aware deployment flow with CRD/pod readiness checks, clearer status messages, and in-cluster AuthPolicy audience patching.
  • Refactor

    • Streamlined deployment composition; added container resource limits, readiness/liveness probes and improved termination behavior.
  • Chores

    • Removed legacy KServe/OpenShift configs, RBAC, gateway/policy manifests, and several sample model deployments/overlays.
  • Documentation

    • Updated developer guidance for the new deployment flow and token/audience extraction.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 2, 2025

Walkthrough

Reorganizes maas-api into deployment/base/maas-api, removes the legacy maas-api/deploy tree (many models, RBAC, policies, KServe/OpenShift configs), switches AuthPolicy audience patching to in-cluster JSON patching, updates kustomizations and service/deployment specs, and significantly rewrites the OpenShift deploy script with version gating, readiness waits, and orchestration steps.

Changes

Cohort / File(s) Summary of Changes
OpenShift deploy script overhaul
deployment/scripts/deploy-openshift.sh
Large rewrite: adds prereq checks, version detection/comparison, wait helpers (CRDs/pods), feature-gate branching, Gateway/Kuadrant/mesh orchestration, operator restarts, policy application, in-cluster AuthPolicy audience JSON patching, Limitador image update, and expanded status/troubleshooting output.
MaaS API base restructure & kustomize
deployment/base/maas-api/kustomization.yaml, deployment/base/maas-api/deployment.yaml, deployment/base/maas-api/service.yaml, deployment/base/maas-api/networking/kustomization.yaml, deployment/base/maas-api/policies/kustomization.yaml
Consolidated resources into directory groups (rbac, networking, policies, resources); updated image mapping; modified Deployment (image name, probes, resources, terminationGracePeriodSeconds); adjusted Service selector/targetPort; replaced gateway.yaml with httproute.yaml; minor comment change in policies kustomization.
RBAC & AuthPolicy removals (base)
deployment/base/maas-api/clusterrole.yaml, deployment/base/maas-api/clusterrolebinding.yaml, deployment/base/maas-api/maas-auth-policy.yaml
Deleted ClusterRole, ClusterRoleBinding, and AuthPolicy manifests from the base maas-api directory.
KServe OpenShift component removal
deployment/components/kserve/kustomization.yaml, deployment/components/kserve/kserve-config-openshift.yaml, deployment/components/kserve/openshift-scc.yaml
Removed KServe OpenShift kustomization, ConfigMap (inferenceservice config), and SCC / ServiceAccount OpenShift resources.
Legacy maas-api/deploy tree removal
maas-api/deploy/...
Examples: maas-api/deploy/base/*, maas-api/deploy/infra/*, maas-api/deploy/models/*, maas-api/deploy/policies/*, maas-api/deploy/rbac/*, maas-api/deploy/overlays/*
Removed legacy kustomizations, base Deployment/Service, infra manifests (gateway, kuadrant, openshift-gateway-api), overlays (dev infra/networking), model manifests (qwen3, facebook-opt-125m, simulator), policy manifests (AuthPolicy, RateLimitPolicy, TokenRateLimitPolicy), RBAC, and related kustomize files. Dev overlay adds a debug-mode patch adding namespace.
Kustomization/path redirects & small edits
maas-api/deploy/overlays/odh/kustomization.yaml, maas-api/deploy/overlays/secret/kustomization.yaml, other overlay kustomizations, deployment/base/policies/kustomization.yaml
Updated resource path references to ../../../../deployment/base/maas-api, replaced some transformers, and made minor comment/text tweaks in policies kustomization.
Docs, samples, and developer flow updates
deployment/README.md, maas-api/DEV.md, maas-api/deploy/README.md, docs/samples/models/qwen3/kustomization.yaml
README/DEV guidance updated (new component paths, in-cluster AuthPolicy patching, token audience extraction, gateway policies instructions); removed legacy deploy overview; sample kustomize enables namePrefix and updates resource name references.
Build & Makefile tweaks
maas-api/Makefile
kustomize build now uses --load-restrictor LoadRestrictionsNone; removed the public deploy target.
Removed RBAC and model-tier manifests
maas-api/deploy/models/rbac/*, maas-api/deploy/models/*
Deleted model RBAC Role/RoleBinding, multiple model kustomizations and model manifests (simulator, qwen3 variants, facebook-opt), and related kustomize overlays.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Script as deploy-openshift.sh
  participant OC as OpenShift API (oc/kubectl)
  participant CRD as CRDs (Gateway API / Kuadrant / cert-manager)
  participant Mesh as Service Mesh
  participant Kuadrant as Kuadrant Operators
  participant KServe as KServe (optional)

  User->>Script: run deploy-openshift.sh
  Script->>Script: check prereqs (kubectl, jq, kustomize)
  Script->>OC: query cluster version
  alt version >= threshold
    Script->>Script: proceed without feature-gates
  else version < threshold or unknown
    Script->>OC: apply Gateway feature-gates
    Script->>CRD: wait for reconciliation
  end
  Script->>OC: ensure namespaces exist
  Script->>CRD: wait for Gateway/Kuadrant/cert-manager CRDs
  Script->>OC: apply Gateway & networking kustomize resources
  par install & readiness
    Script->>Mesh: wait for mesh readiness
    Script->>Kuadrant: apply/patch and restart operators
  end
  Script->>OC: apply policies
  Script->>OC: patch AuthPolicy audiences (in-cluster JSON patch)
  Script->>KServe: update ingress domain (if present)
  Script-->>User: report status, next steps, and troubleshooting hints
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Poem

I thumped my paw and swept the burrow clean,
Old manifests gone, new paths tidy and keen.
Scripts now wait and patch with careful art,
Gates and policies find their proper part.
Hop, carrots compiled — deploy with a happy heart! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly summarizes the main purpose of the pull request by indicating a kustomize refactor to consolidate deployment manifests, which aligns with the PR objectives of restructuring and reducing duplication. It is concise, specific, and accurately reflects the primary change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

❤️ Share

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

Comment on lines -6 to -9
namespace: maas-api
labels:
app: maas-api
version: v2
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injected using kustomize

app: maas-api
version: v2
annotations:
sidecar.istio.io/inject: "false"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we have no istio :) And no auto injection on the ns

containers:
- name: maas-api
image: quay.io/opendatahub/maas-api:latest
image: maas-api
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injected using kustomize - open for customizations in overlays

Comment on lines +27 to +33
resources:
requests:
memory: "64Mi"
cpu: "50m"
limits:
memory: "128Mi"
cpu: "200m"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can tweak it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is small enough where I don't think it matters unless it is causing us issues.

kustomize edit set image maas-api=$(FULL_IMAGE) && \
$(if $(PRE_DEPLOY_STEP),$(PRE_DEPLOY_STEP) &&) \
kustomize build . | kubectl apply -f -
kustomize build . --load-restrictor LoadRestrictionsNone | kubectl apply -f -
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only needed for dev overlay as we refer to top-level deployment manifests

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (2)
deployment/README.md (1)

98-197: Fix JWT audience extraction command

Switching to cut … | base64 -d breaks because RFC 7515 JWT segments are base64url-encoded (-/_, no padding), which GNU base64 -d refuses. The pipeline now returns an empty string and the JSON patch fails every time. Please restore a base64url-aware decode—for example:

-AUD="$(kubectl create token default --duration=10m \
-  | cut -d. -f2 \
-  | base64 -d 2>/dev/null \
-  | jq -r '.aud[0]')"
+AUD="$(
+  kubectl create token default --duration=10m \
+  | python - <<'PY'
+import base64, json, sys
+token = sys.stdin.read().strip()
+payload = token.split('.')[1]
+payload += "=" * (-len(payload) % 4)
+print(json.loads(base64.urlsafe_b64decode(payload))["aud"][0])
+PY
+)"

Any equivalent base64url decoder is fine; we just need something that consistently produces the audience string.

maas-api/DEV.md (1)

98-152: Restore base64url-safe audience extraction

The new pipeline uses base64 -d, which can’t decode JWT base64url segments, so $AUD comes back empty and the subsequent patch fails. Please switch to a base64url-aware decode (e.g., the Python snippet suggested in the README comment) to keep the workflow functional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 870bbff and 62363b1.

📒 Files selected for processing (50)
  • deployment/README.md (1 hunks)
  • deployment/base/maas-api/clusterrole.yaml (0 hunks)
  • deployment/base/maas-api/clusterrolebinding.yaml (0 hunks)
  • deployment/base/maas-api/deployment.yaml (2 hunks)
  • deployment/base/maas-api/kustomization.yaml (1 hunks)
  • deployment/base/maas-api/maas-auth-policy.yaml (0 hunks)
  • deployment/base/maas-api/networking/kustomization.yaml (0 hunks)
  • deployment/base/maas-api/policies/kustomization.yaml (1 hunks)
  • deployment/base/maas-api/service.yaml (1 hunks)
  • deployment/base/policies/kustomization.yaml (1 hunks)
  • deployment/components/kserve/kserve-config-openshift.yaml (0 hunks)
  • deployment/components/kserve/kustomization.yaml (0 hunks)
  • deployment/components/kserve/openshift-scc.yaml (0 hunks)
  • deployment/scripts/deploy-openshift.sh (9 hunks)
  • docs/samples/models/qwen3/kustomization.yaml (2 hunks)
  • maas-api/DEV.md (5 hunks)
  • maas-api/Makefile (1 hunks)
  • maas-api/deploy/README.md (0 hunks)
  • maas-api/deploy/base/deployment.yaml (0 hunks)
  • maas-api/deploy/base/kustomization.yaml (0 hunks)
  • maas-api/deploy/base/service.yaml (0 hunks)
  • maas-api/deploy/infra/kuadrant/kuadrant.yaml (0 hunks)
  • maas-api/deploy/infra/kuadrant/kustomization.yaml (0 hunks)
  • maas-api/deploy/infra/kustomization.yaml (0 hunks)
  • maas-api/deploy/infra/openshift-gateway-api/gateway-api.yaml (0 hunks)
  • maas-api/deploy/infra/openshift-gateway-api/kustomization.yaml (0 hunks)
  • maas-api/deploy/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/facebook-opt-125m-cpu/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/facebook-opt-125m-cpu/model.yaml (0 hunks)
  • maas-api/deploy/models/qwen3-0.6B/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/qwen3-0.6B/model.yaml (0 hunks)
  • maas-api/deploy/models/rbac/all-tiers.yaml (0 hunks)
  • maas-api/deploy/models/rbac/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/simulator/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/simulator/model.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/kustomization.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/networking/gateway.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/networking/httproute.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/kustomization.yaml (1 hunks)
  • maas-api/deploy/overlays/dev/patches/debug-mode.yaml (1 hunks)
  • maas-api/deploy/overlays/dev/resources/tier-mapping-configmap.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/transformers/namespace.yaml (0 hunks)
  • maas-api/deploy/overlays/odh/kustomization.yaml (1 hunks)
  • maas-api/deploy/overlays/secret/kustomization.yaml (1 hunks)
  • maas-api/deploy/policies/kustomization.yaml (0 hunks)
  • maas-api/deploy/policies/model/gateway-auth-policy.yaml (0 hunks)
  • maas-api/deploy/policies/model/kustomization.yaml (0 hunks)
  • maas-api/deploy/policies/model/rate-limit-policy.yaml (0 hunks)
  • maas-api/deploy/policies/model/token-limit-policy.yaml (0 hunks)
  • maas-api/deploy/rbac/serviceaccount.yaml (0 hunks)
💤 Files with no reviewable changes (36)
  • maas-api/deploy/README.md
  • maas-api/deploy/policies/model/kustomization.yaml
  • maas-api/deploy/base/service.yaml
  • maas-api/deploy/models/rbac/kustomization.yaml
  • maas-api/deploy/overlays/dev/infra/kustomization.yaml
  • maas-api/deploy/overlays/dev/infra/networking/httproute.yaml
  • maas-api/deploy/base/kustomization.yaml
  • deployment/components/kserve/openshift-scc.yaml
  • maas-api/deploy/infra/kuadrant/kuadrant.yaml
  • deployment/components/kserve/kustomization.yaml
  • maas-api/deploy/infra/openshift-gateway-api/gateway-api.yaml
  • maas-api/deploy/infra/kuadrant/kustomization.yaml
  • deployment/base/maas-api/maas-auth-policy.yaml
  • maas-api/deploy/models/qwen3-0.6B/model.yaml
  • maas-api/deploy/models/facebook-opt-125m-cpu/kustomization.yaml
  • maas-api/deploy/policies/kustomization.yaml
  • deployment/base/maas-api/networking/kustomization.yaml
  • maas-api/deploy/models/rbac/all-tiers.yaml
  • maas-api/deploy/kustomization.yaml
  • deployment/base/maas-api/clusterrolebinding.yaml
  • maas-api/deploy/policies/model/token-limit-policy.yaml
  • maas-api/deploy/infra/kustomization.yaml
  • maas-api/deploy/overlays/dev/infra/networking/gateway.yaml
  • maas-api/deploy/models/qwen3-0.6B/kustomization.yaml
  • maas-api/deploy/overlays/dev/resources/tier-mapping-configmap.yaml
  • maas-api/deploy/models/facebook-opt-125m-cpu/model.yaml
  • maas-api/deploy/rbac/serviceaccount.yaml
  • maas-api/deploy/policies/model/rate-limit-policy.yaml
  • maas-api/deploy/models/simulator/model.yaml
  • deployment/base/maas-api/clusterrole.yaml
  • maas-api/deploy/models/simulator/kustomization.yaml
  • maas-api/deploy/base/deployment.yaml
  • maas-api/deploy/overlays/dev/transformers/namespace.yaml
  • maas-api/deploy/infra/openshift-gateway-api/kustomization.yaml
  • deployment/components/kserve/kserve-config-openshift.yaml
  • maas-api/deploy/policies/model/gateway-auth-policy.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-12T22:19:36.331Z
Learnt from: bartoszmajsak
PR: opendatahub-io/maas-billing#73
File: maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml:5-12
Timestamp: 2025-09-12T22:19:36.331Z
Learning: In maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml, the patch file exists but is intentionally not referenced in the kustomization.yaml. This is by design - the patch is meant to be applied conditionally during development workflows (e.g., via "kustomize edit add patch" commands) rather than being permanently included in the dev overlay.

Applied to files:

  • maas-api/deploy/overlays/dev/kustomization.yaml
  • maas-api/deploy/overlays/secret/kustomization.yaml
  • maas-api/deploy/overlays/odh/kustomization.yaml
🪛 Shellcheck (0.11.0)
deployment/scripts/deploy-openshift.sh

[warning] 33-33: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 48-48: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 49-49: Declare and assign separately to avoid masking return values.

(SC2155)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
deployment/base/policies/kustomization.yaml (1)

8-8: Comment update looks good.

Clarifies that these policies apply to default model gateway traffic. No functional changes; thanks for tightening the wording.

Comment on lines 6 to 34
replicas: 1
selector:
matchLabels:
app: maas-api
template:
metadata:
labels:
app: maas-api
version: v2
annotations:
sidecar.istio.io/inject: "false"
spec:
serviceAccountName: maas-api
securityContext:
runAsNonRoot: true
containers:
- name: maas-api
image: quay.io/opendatahub/maas-api:latest
image: maas-api
imagePullPolicy: Always
ports:
- containerPort: 8080
name: http
protocol: TCP
env:
- name: PROVIDER
value: "sa-tokens"
- name: NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: KEY_NAMESPACE
value: llm
- name: SECRET_SELECTOR_LABEL
value: kuadrant.io/apikeys-by
- name: SECRET_SELECTOR_VALUE
value: rhcl-keys
- name: PORT
value: "8080"
- name: CREATE_DEFAULT_TEAM
value: "true"
- name: TOKEN_RATE_LIMIT_POLICY_NAME
value: "gateway-token-rate-limits"
- name: AUTH_POLICY_NAME
value: "gateway-auth-policy"
- name: GIN_MODE
value: "debug"
- name: PROVIDER
value: sa-tokens
resources:
requests:
memory: "64Mi"
cpu: "50m"
limits:
memory: "128Mi"
cpu: "200m"
livenessProbe:
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Deployment manifest missing required selector/labels

spec.selector and matching template.metadata.labels were dropped. Without them the API rejects the Deployment (spec.selector: Required value), so the MaaS API never reconciles. Please restore the selector and labels, for example:

 spec:
   replicas: 1
+  selector:
+    matchLabels:
+      app.kubernetes.io/name: maas-api
   template:
+    metadata:
+      labels:
+        app.kubernetes.io/name: maas-api
     spec:

You can add extra labels if needed, but the selector and matching pod labels must be present.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
replicas: 1
selector:
matchLabels:
app: maas-api
template:
metadata:
labels:
app: maas-api
version: v2
annotations:
sidecar.istio.io/inject: "false"
spec:
serviceAccountName: maas-api
securityContext:
runAsNonRoot: true
containers:
- name: maas-api
image: quay.io/opendatahub/maas-api:latest
image: maas-api
imagePullPolicy: Always
ports:
- containerPort: 8080
name: http
protocol: TCP
env:
- name: PROVIDER
value: "sa-tokens"
- name: NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: KEY_NAMESPACE
value: llm
- name: SECRET_SELECTOR_LABEL
value: kuadrant.io/apikeys-by
- name: SECRET_SELECTOR_VALUE
value: rhcl-keys
- name: PORT
value: "8080"
- name: CREATE_DEFAULT_TEAM
value: "true"
- name: TOKEN_RATE_LIMIT_POLICY_NAME
value: "gateway-token-rate-limits"
- name: AUTH_POLICY_NAME
value: "gateway-auth-policy"
- name: GIN_MODE
value: "debug"
- name: PROVIDER
value: sa-tokens
resources:
requests:
memory: "64Mi"
cpu: "50m"
limits:
memory: "128Mi"
cpu: "200m"
livenessProbe:
replicas: 1
selector:
matchLabels:
app.kubernetes.io/name: maas-api
template:
metadata:
labels:
app.kubernetes.io/name: maas-api
spec:
serviceAccountName: maas-api
securityContext:
runAsNonRoot: true
containers:
- name: maas-api
image: maas-api
imagePullPolicy: Always
ports:
- containerPort: 8080
name: http
protocol: TCP
env:
- name: NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: PROVIDER
value: sa-tokens
resources:
requests:
memory: "64Mi"
cpu: "50m"
limits:
memory: "128Mi"
cpu: "200m"
livenessProbe:
🤖 Prompt for AI Agents
In deployment/base/maas-api/deployment.yaml around lines 6 to 34, the Deployment
is missing spec.selector and matching template.metadata.labels which causes
Kubernetes to reject the manifest; add a spec.selector.matchLabels block under
spec that matches labels you add to template.metadata.labels (e.g., app:
maas-api) and ensure the same key/value pair exists on the pod template labels
so the selector and pod labels are identical and the Deployment can reconcile.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're injected by Kustomize when rendering - see kustomization.yaml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the kustomization.yaml file to confirm includeSelectors configuration
cat deployment/base/maas-api/kustomization.yaml

Length of output: 476


@bartoszmajsak You're absolutely right, and I apologize for the incorrect flag! 🙏

I can confirm that deployment/base/maas-api/kustomization.yaml has includeSelectors: true configured with the following labels:

  • app.kubernetes.io/part-of: model-as-a-service
  • app.kubernetes.io/component: api
  • app.kubernetes.io/name: maas-api

When Kustomize renders the manifests, it will automatically inject these labels into both spec.selector.matchLabels and spec.template.metadata.labels, making the Deployment valid. This is consistent with the Kustomize pattern you're using throughout the project (as I learned from PR #39).

Thank you for the clarification! The deployment manifest is correctly structured for Kustomize processing.


🧠 Learnings used
Learnt from: bartoszmajsak
PR: opendatahub-io/maas-billing#39
File: key-manager/deploy/base/deployment.yaml:5-8
Timestamp: 2025-09-05T18:05:13.035Z
Learning: In Kustomize, when labels are defined with includeSelectors: true in kustomization.yaml, the labels are automatically added to spec.selector.matchLabels and spec.template.metadata.labels for Deployment resources, making it unnecessary to manually specify these fields in the Deployment manifest.

Copy link
Copy Markdown
Collaborator Author

@bartoszmajsak bartoszmajsak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

We should be using ODH version of KServe - especially since certain features are not upstreamed yet (or even released there). That's why kserve folder is removed - to avoid confusion.

This change introduces a single source of truth to make deployments simpler.

Existing `maas-api/deploy` manifests are promoted to top-level
`deployment` folder, reducing duplication and improving the structure.

Installation script for OpenShift has been adjusted:

- Fixed OCP version check - using cluster info instead as it's more
  reliable
- Add helpers to wait for CRDs/pods and a version comparator;
- Auto-detect and patch AuthPolicy audience; apply gateway policies in
  sequence after infra becomes ready.
- Detect existing ODH/RHOAI KServe; deploy components only when missing,
  then proceed with retries.

Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
@bartoszmajsak bartoszmajsak force-pushed the deployment/chore/consolidate-maas-api-manifests branch from 62363b1 to 60a69e3 Compare October 3, 2025 09:43
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
deployment/scripts/deploy-openshift.sh (1)

19-21: Guard kubectl wait so wait_for_crd can return an error instead of exiting.

With set -e active, a timeout or API hiccup in kubectl wait terminates the whole script before the caller’s || handler runs, regressing the intended “warn and continue” behavior for transient CRD readiness issues. Wrap the call in an explicit check and return a non-zero status yourself.

-  echo "⏳ CRD ${crd} detected — waiting for it to become Established (timeout: ${timeout})…"
-  kubectl wait --for=condition=Established --timeout="$timeout" "crd/$crd"
+  echo "⏳ CRD ${crd} detected — waiting for it to become Established (timeout: ${timeout})…"
+  if ! kubectl wait --for=condition=Established --timeout="$timeout" "crd/$crd"; then
+    echo "❌ Timed out waiting for CRD $crd to become Established." >&2
+    return 1
+  fi
🧹 Nitpick comments (2)
maas-api/Makefile (1)

122-122: Catch pipe failures and align apply flags

Good call on LoadRestrictionsNone. Add pipefail so kustomize errors don’t get masked, and consider server-side apply for consistency with docs.

-        kustomize build . --load-restrictor LoadRestrictionsNone | kubectl apply -f -
+        set -o pipefail; \
+        kustomize build . --load-restrictor LoadRestrictionsNone | kubectl apply --server-side=true --force-conflicts -f -
deployment/README.md (1)

194-201: Remove dependency on external jwt CLI; use base64url-safe jq

To avoid requiring jwt-cli and to be consistent with DEV.md, derive AUD with jq using base64url decoding.

-AUD="$(kubectl create token default --duration=10m \
-  | jwt decode --json - \
-  | jq -r '.payload.aud[0]')"
+AUD="$(kubectl create token default --duration=10m \
+  | jq -Rr 'split(".")[1]
+            | (gsub("-";"+") | gsub("_";"/") + (["","==","="][(length%4)]))
+            | @base64d
+            | fromjson
+            | .aud[0]')"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 62363b1 and 60a69e3.

📒 Files selected for processing (50)
  • deployment/README.md (1 hunks)
  • deployment/base/maas-api/clusterrole.yaml (0 hunks)
  • deployment/base/maas-api/clusterrolebinding.yaml (0 hunks)
  • deployment/base/maas-api/deployment.yaml (2 hunks)
  • deployment/base/maas-api/kustomization.yaml (1 hunks)
  • deployment/base/maas-api/maas-auth-policy.yaml (0 hunks)
  • deployment/base/maas-api/networking/kustomization.yaml (0 hunks)
  • deployment/base/maas-api/policies/kustomization.yaml (1 hunks)
  • deployment/base/maas-api/service.yaml (1 hunks)
  • deployment/base/policies/kustomization.yaml (1 hunks)
  • deployment/components/kserve/kserve-config-openshift.yaml (0 hunks)
  • deployment/components/kserve/kustomization.yaml (0 hunks)
  • deployment/components/kserve/openshift-scc.yaml (0 hunks)
  • deployment/scripts/deploy-openshift.sh (9 hunks)
  • docs/samples/models/qwen3/kustomization.yaml (2 hunks)
  • maas-api/DEV.md (5 hunks)
  • maas-api/Makefile (1 hunks)
  • maas-api/deploy/README.md (0 hunks)
  • maas-api/deploy/base/deployment.yaml (0 hunks)
  • maas-api/deploy/base/kustomization.yaml (0 hunks)
  • maas-api/deploy/base/service.yaml (0 hunks)
  • maas-api/deploy/infra/kuadrant/kuadrant.yaml (0 hunks)
  • maas-api/deploy/infra/kuadrant/kustomization.yaml (0 hunks)
  • maas-api/deploy/infra/kustomization.yaml (0 hunks)
  • maas-api/deploy/infra/openshift-gateway-api/gateway-api.yaml (0 hunks)
  • maas-api/deploy/infra/openshift-gateway-api/kustomization.yaml (0 hunks)
  • maas-api/deploy/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/facebook-opt-125m-cpu/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/facebook-opt-125m-cpu/model.yaml (0 hunks)
  • maas-api/deploy/models/qwen3-0.6B/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/qwen3-0.6B/model.yaml (0 hunks)
  • maas-api/deploy/models/rbac/all-tiers.yaml (0 hunks)
  • maas-api/deploy/models/rbac/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/simulator/kustomization.yaml (0 hunks)
  • maas-api/deploy/models/simulator/model.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/kustomization.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/networking/gateway.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/infra/networking/httproute.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/kustomization.yaml (1 hunks)
  • maas-api/deploy/overlays/dev/patches/debug-mode.yaml (1 hunks)
  • maas-api/deploy/overlays/dev/resources/tier-mapping-configmap.yaml (0 hunks)
  • maas-api/deploy/overlays/dev/transformers/namespace.yaml (0 hunks)
  • maas-api/deploy/overlays/odh/kustomization.yaml (1 hunks)
  • maas-api/deploy/overlays/secret/kustomization.yaml (1 hunks)
  • maas-api/deploy/policies/kustomization.yaml (0 hunks)
  • maas-api/deploy/policies/model/gateway-auth-policy.yaml (0 hunks)
  • maas-api/deploy/policies/model/kustomization.yaml (0 hunks)
  • maas-api/deploy/policies/model/rate-limit-policy.yaml (0 hunks)
  • maas-api/deploy/policies/model/token-limit-policy.yaml (0 hunks)
  • maas-api/deploy/rbac/serviceaccount.yaml (0 hunks)
💤 Files with no reviewable changes (36)
  • maas-api/deploy/base/service.yaml
  • maas-api/deploy/infra/openshift-gateway-api/gateway-api.yaml
  • maas-api/deploy/policies/kustomization.yaml
  • maas-api/deploy/base/kustomization.yaml
  • deployment/base/maas-api/clusterrole.yaml
  • maas-api/deploy/models/rbac/kustomization.yaml
  • maas-api/deploy/infra/kustomization.yaml
  • maas-api/deploy/overlays/dev/transformers/namespace.yaml
  • deployment/components/kserve/openshift-scc.yaml
  • maas-api/deploy/models/qwen3-0.6B/model.yaml
  • maas-api/deploy/models/simulator/kustomization.yaml
  • maas-api/deploy/rbac/serviceaccount.yaml
  • maas-api/deploy/models/simulator/model.yaml
  • maas-api/deploy/policies/model/gateway-auth-policy.yaml
  • maas-api/deploy/README.md
  • maas-api/deploy/overlays/dev/infra/networking/gateway.yaml
  • deployment/components/kserve/kustomization.yaml
  • maas-api/deploy/policies/model/token-limit-policy.yaml
  • maas-api/deploy/models/rbac/all-tiers.yaml
  • maas-api/deploy/policies/model/rate-limit-policy.yaml
  • deployment/base/maas-api/networking/kustomization.yaml
  • maas-api/deploy/infra/kuadrant/kustomization.yaml
  • maas-api/deploy/base/deployment.yaml
  • maas-api/deploy/models/qwen3-0.6B/kustomization.yaml
  • maas-api/deploy/models/facebook-opt-125m-cpu/model.yaml
  • deployment/base/maas-api/clusterrolebinding.yaml
  • maas-api/deploy/overlays/dev/resources/tier-mapping-configmap.yaml
  • deployment/base/maas-api/maas-auth-policy.yaml
  • maas-api/deploy/kustomization.yaml
  • maas-api/deploy/infra/kuadrant/kuadrant.yaml
  • deployment/components/kserve/kserve-config-openshift.yaml
  • maas-api/deploy/overlays/dev/infra/kustomization.yaml
  • maas-api/deploy/policies/model/kustomization.yaml
  • maas-api/deploy/overlays/dev/infra/networking/httproute.yaml
  • maas-api/deploy/models/facebook-opt-125m-cpu/kustomization.yaml
  • maas-api/deploy/infra/openshift-gateway-api/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
  • deployment/base/maas-api/service.yaml
  • maas-api/deploy/overlays/odh/kustomization.yaml
  • deployment/base/maas-api/kustomization.yaml
  • maas-api/deploy/overlays/secret/kustomization.yaml
  • deployment/base/maas-api/policies/kustomization.yaml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-12T22:19:36.331Z
Learnt from: bartoszmajsak
PR: opendatahub-io/maas-billing#73
File: maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml:5-12
Timestamp: 2025-09-12T22:19:36.331Z
Learning: In maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml, the patch file exists but is intentionally not referenced in the kustomization.yaml. This is by design - the patch is meant to be applied conditionally during development workflows (e.g., via "kustomize edit add patch" commands) rather than being permanently included in the dev overlay.

Applied to files:

  • maas-api/deploy/overlays/dev/kustomization.yaml
📚 Learning: 2025-09-05T18:05:13.035Z
Learnt from: bartoszmajsak
PR: opendatahub-io/maas-billing#39
File: key-manager/deploy/base/deployment.yaml:5-8
Timestamp: 2025-09-05T18:05:13.035Z
Learning: In Kustomize, when labels are defined with includeSelectors: true in kustomization.yaml, the labels are automatically added to spec.selector.matchLabels and spec.template.metadata.labels for Deployment resources, making it unnecessary to manually specify these fields in the Deployment manifest.

Applied to files:

  • deployment/base/maas-api/deployment.yaml
🪛 Shellcheck (0.11.0)
deployment/scripts/deploy-openshift.sh

[warning] 33-33: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 48-48: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 49-49: Declare and assign separately to avoid masking return values.

(SC2155)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (2)
deployment/base/policies/kustomization.yaml (1)

8-11: Comment update improves clarity.

Nice context for future readers; no further action needed.

deployment/base/maas-api/deployment.yaml (1)

14-34: Deployment spec looks good; selectors/labels via Kustomize are fine

Image placeholder, probes, resources, and pod-level settings are sane. Selector/labels are expected to be injected by kustomize with includeSelectors.

Based on learnings

Also applies to: 57-57

Comment on lines 7 to 9
resources:
- ../../base/
- ./infra/
- ./resources/tier-mapping-configmap.yaml
- ../../policies

namespace: maas-api

transformers:
- transformers/namespace.yaml
- ../../../../deployment/base/maas-api

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Set namespace at overlay level (and drop namespace from patches)

Without a namespace here, most resources may land in “default”, and the debug-mode patch with namespace might not match. Define it once at overlay level.

 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization

 metadata:
   name: maas-api-dev
+namespace: maas-api

 resources:
 - ../../../../deployment/base/maas-api

Then remove metadata.namespace from the debug-mode patch (see related comment).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resources:
- ../../base/
- ./infra/
- ./resources/tier-mapping-configmap.yaml
- ../../policies
namespace: maas-api
transformers:
- transformers/namespace.yaml
- ../../../../deployment/base/maas-api
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
metadata:
name: maas-api-dev
namespace: maas-api
resources:
- ../../../../deployment/base/maas-api
🤖 Prompt for AI Agents
In maas-api/deploy/overlays/dev/kustomization.yaml around lines 7-9, the overlay
lacks a namespace, so resources default to the "default" namespace and the
debug-mode patch that includes metadata.namespace may not match; update this
overlay by adding a top-level namespace entry (e.g., namespace: dev) in
kustomization.yaml, and then edit the debug-mode patch to remove
metadata.namespace so the patch matches resources by name/labels within the
overlay namespace.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resources have namespaces explicitly set

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the debug-mode patch to see how namespace is set
cat maas-api/deploy/overlays/dev/patches/debug-mode.yaml

echo "---"
echo "Checking base deployment resources for namespace configuration:"
fd -e yaml . deployment/base/maas-api -x echo "File: {}" \; -x head -20 {}

Length of output: 5043


@bartoszmajsak, you're absolutely right! I can see that the base kustomization at deployment/base/maas-api/kustomization.yaml already defines namespace: maas-api on line 5, which applies to all resources referenced by the dev overlay. Additionally, resources that need explicit namespace declarations (like the HTTPRoute that references a parent Gateway in a different namespace) have them set directly in their metadata.

The current structure is correct:

  • The base sets the namespace for all resources
  • The dev overlay inherits this via the resource reference to ../../../../deployment/base/maas-api
  • Resources are properly scoped to the maas-api namespace

My earlier suggestion to add a namespace at the overlay level was unnecessary. The configuration works as designed.


🧠 Learnings used
Learnt from: bartoszmajsak
PR: opendatahub-io/maas-billing#73
File: maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml:5-12
Timestamp: 2025-09-12T22:19:36.331Z
Learning: In maas-api/deploy/overlays/dev/patches/sa-token-provider.yaml, the patch file exists but is intentionally not referenced in the kustomization.yaml. This is by design - the patch is meant to be applied conditionally during development workflows (e.g., via "kustomize edit add patch" commands) rather than being permanently included in the dev overlay.

kind: Deployment
metadata:
name: maas-api
namespace: maas-api
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid setting namespace in the patch (may break matching)

Strategic merge patches match by name/kind/namespace. If the base resources are namespace-less, this patch may not apply. Manage namespace at the overlay level instead.

 metadata:
   name: maas-api
-  namespace: maas-api
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
namespace: maas-api
metadata:
name: maas-api
🤖 Prompt for AI Agents
In maas-api/deploy/overlays/dev/patches/debug-mode.yaml around line 5, the patch
sets "namespace: maas-api", which can break strategic merge patch matching if
the base resources are namespace-less; remove the namespace field from this
patch and instead ensure the overlay's kustomization.yaml or overlay-level
configuration specifies the target namespace (or use a namespace transformer) so
the patch matches by name/kind and applies reliably.

Comment on lines 98 to 102
AUD="$(kubectl create token default --duration=10m \
| jwt decode --json - \
| jq -r '.payload.aud[0]')"
| cut -d. -f2 \
| base64 -d 2>/dev/null \
| jq -r '.aud[0]')"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix JWT base64url decoding (current pipeline may fail)

JWT payload is base64url, not standard base64. The current cut|base64 -d can produce empty/invalid output. Use a base64url-safe jq pipeline.

Apply these replacements:

- AUD="$(kubectl create token default --duration=10m \
-   | cut -d. -f2 \
-   | base64 -d 2>/dev/null \
-   | jq -r '.aud[0]')"
+ AUD="$(kubectl create token default --duration=10m \
+   | jq -Rr 'split(".")[1]
+             | (gsub("-";"+") | gsub("_";"/") + (["","==","="][(length%4)]))
+             | @base64d
+             | fromjson
+             | .aud[0]')"
- echo $TOKEN_RESPONSE | jq -r .token | cut -d. -f2 | base64 -d 2>/dev/null | jq .
+ echo "$TOKEN_RESPONSE" \
+   | jq -r .token \
+   | jq -Rr 'split(".")[1]
+             | (gsub("-";"+") | gsub("_";"/") + (["","==","="][(length%4)]))
+             | @base64d' \
+   | jq .

Also applies to: 151-152

🤖 Prompt for AI Agents
In maas-api/DEV.md around lines 98-102 (and also apply the same fix at lines
151-152), the current pipeline decodes the JWT payload using standard base64
which fails for base64url-encoded JWTs; replace the cut|base64 -d step with a
base64url-safe decode approach: split the token, take the payload, convert
base64url characters to standard base64 padding as needed, then decode with
base64 -d (or use jq's @base64d / fromjson if available) so the payload is
correctly decoded for jq to parse; update both occurrences accordingly.

Comment on lines -41 to -46
- name: KEY_NAMESPACE
value: llm
- name: SECRET_SELECTOR_LABEL
value: kuadrant.io/apikeys-by
- name: SECRET_SELECTOR_VALUE
value: rhcl-keys
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed for token-based manager.

Comment on lines -49 to -54
- name: CREATE_DEFAULT_TEAM
value: "true"
- name: TOKEN_RATE_LIMIT_POLICY_NAME
value: "gateway-token-rate-limits"
- name: AUTH_POLICY_NAME
value: "gateway-auth-policy"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed for token-based manager.

Comment on lines -55 to -56
- name: GIN_MODE
value: "debug"
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal for PROD, for dev mode it's always possible to use kustomize build maas-api/deploy/overlays/dev which now only does that + swaps image to custom one if provided.

Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
deployment/scripts/deploy-openshift.sh (1)

19-21: Fix: guard kubectl wait to avoid aborting under set -e (duplicate of prior)

As written, a failed kubectl wait will terminate the script before the caller’s || handlers can run. Wrap it and return non‑zero instead.

-  echo "⏳ CRD ${crd} detected — waiting for it to become Established (timeout: ${timeout})…"
-  kubectl wait --for=condition=Established --timeout="$timeout" "crd/$crd"
+  echo "⏳ CRD ${crd} detected — waiting for it to become Established (timeout: ${timeout})…"
+  if ! kubectl wait --for=condition=Established --timeout="$timeout" "crd/$crd"; then
+    echo "❌ Timed out waiting for CRD $crd to become Established." >&2
+    return 1
+  fi
🧹 Nitpick comments (7)
deployment/scripts/deploy-openshift.sh (7)

307-317: Make JWT payload decoding base64url‑safe; current base64 -d may fail

JWTs use base64url; base64 -d can silently fail, leading to skipped AuthPolicy patching.

- AUD="$(kubectl create token default --duration=10m 2>/dev/null | cut -d. -f2 | base64 -d 2>/dev/null | jq -r '.aud[0]' 2>/dev/null)"
+ # Helper: decode base64url safely via jq
+ b64url_decode() { jq -rn 'input|gsub("-";"+")|gsub("_";"/")|(. + "====")|.[0:((length/4|floor)*4)]|@base64d'; }
+ AUD="$(
+   kubectl create token default --duration=10m 2>/dev/null \
+     | cut -d. -f2 \
+     | b64url_decode 2>/dev/null \
+     | jq -r '.aud[0]' 2>/dev/null
+ )"

If you prefer no helper, translate url-safe chars before decoding and pad with “=”.


24-39: Pod readiness check is coarse; consider kubectl wait or Ready column parsing

Current grep on STATUS misses unready containers in Running phase and CrashLoopBackOff nuance. Prefer kubectl wait with guards, or parse the Ready column.

  • Option A (simple): kubectl wait -n "$namespace" --for=condition=Ready pods --all --timeout="${timeout}s" || true
  • Option B: parse Ready column and ensure ready==total.

41-52: Tidy version_compare: address SC2155 and improve robustness

Shellcheck SC2155: declare and assign separately. Minor refactor.

-version_compare() {
-  local version1="$1"
-  local version2="$2"
-  
-  local v1=$(echo "$version1" | awk -F. '{printf "%d%03d%03d", $1, $2, $3}')
-  local v2=$(echo "$version2" | awk -F. '{printf "%d%03d%03d", $1, $2, $3}')
-  
-  [ "$v1" -ge "$v2" ]
-}
+version_compare() {
+  local version1="$1"
+  local version2="$2"
+  local v1 v2
+  v1=$(awk -F. '{printf "%d%03d%03d", $1, $2, $3}' <<<"$version1")
+  v2=$(awk -F. '{printf "%d%03d%03d", $1, $2, $3}' <<<"$version2")
+  [ "$v1" -ge "$v2" ]
+}

Also addresses Shellcheck hints at Lines 48–49. Based on static analysis hints.


112-119: Optionally fail fast on missing prerequisites

Currently only prints versions. Consider hard‑failing if oc, jq, or kustomize are missing.

for bin in oc jq kustomize; do
  command -v "$bin" >/dev/null || { echo "❌ Missing required tool: $bin" >&2; exit 1; }
done

260-265: Make KServe CM patch conditional and adjust message

If the ConfigMap doesn’t exist, the patch fails and message “already configured” is misleading.

if kubectl -n kserve get configmap inferenceservice-config >/dev/null 2>&1; then
  kubectl -n kserve patch configmap inferenceservice-config ... || echo "   ⚠️  Failed to patch KServe config"
else
  echo "   ℹ️  KServe configmap not found; skipping ingress config update"
fi

202-206: Consider waiting for the full Gateway API CRD set

Only gateways.gateway.networking.k8s.io is checked. If you rely on Routes/Classes, add waits for: gatewayclasses.gateway.networking.k8s.io, httproutes.gateway.networking.k8s.io, etc.


338-343: Status printing resilient to empty output

Pipelines with xargs echo may print nothing if input is empty. Consider defaults.

kubectl get pods -n maas-api --no-headers 2>/dev/null | grep -c ' Running ' || echo "  MaaS API pods running: 0"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60a69e3 and 4856aea.

📒 Files selected for processing (1)
  • deployment/scripts/deploy-openshift.sh (9 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
deployment/scripts/deploy-openshift.sh

[warning] 33-33: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 48-48: Declare and assign separately to avoid masking return values.

(SC2155)


[warning] 49-49: Declare and assign separately to avoid masking return values.

(SC2155)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
deployment/scripts/deploy-openshift.sh (1)

171-175: Cluster‑scope CRD deletion: confirm safety in shared clusters

This deletes Kuadrant/Authorino/Limitador CRDs cluster‑wide, potentially impacting other tenants/workloads.

  • Confirm this script targets disposable/dev clusters only, or scope cleanup more narrowly (e.g., check owner labels/annotations).
  • If multi‑tenant use is possible, gate behind a --force-cleanup flag.

Comment on lines +251 to +258
if ! kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager -o jsonpath='{.spec.template.spec.containers[0].env[?(@.name=="ISTIO_GATEWAY_CONTROLLER_NAMES")]}' | grep -q "ISTIO_GATEWAY_CONTROLLER_NAMES"; then
kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
--type='json' \
-p='[{"op":"add","path":"/spec/template/spec/containers/0/env/-","value":{"name":"ISTIO_GATEWAY_CONTROLLER_NAMES","value":"openshift.io/gateway-controller/v1"}}]'
echo " ✅ Kuadrant operator patched"
else
echo " ✅ Kuadrant operator already configured"
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Prevent early exit when Kuadrant operator deployment is absent; verify value, not just presence

If the deployment doesn’t exist yet, the get … | grep condition will evaluate as “not configured” and the subsequent patch will fail and abort the script under set -e. Also, you only check presence of the env var, not its value.

- if ! kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager -o jsonpath='{.spec.template.spec.containers[0].env[?(@.name=="ISTIO_GATEWAY_CONTROLLER_NAMES")]}' | grep -q "ISTIO_GATEWAY_CONTROLLER_NAMES"; then
-   kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
+ if kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager >/dev/null 2>&1; then
+   # Check current value
+   if ! kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager \
+     -o jsonpath='{.spec.template.spec.containers[0].env[?(@.name=="ISTIO_GATEWAY_CONTROLLER_NAMES")].value}' \
+     | grep -q "^openshift\.io/gateway-controller/v1$"; then
+     kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
       --type='json' \
       -p='[{"op":"add","path":"/spec/template/spec/containers/0/env/-","value":{"name":"ISTIO_GATEWAY_CONTROLLER_NAMES","value":"openshift.io/gateway-controller/v1"}}]'
-   echo "   ✅ Kuadrant operator patched"
- else
-   echo "   ✅ Kuadrant operator already configured"
- fi
+     echo "   ✅ Kuadrant operator patched"
+   else
+     echo "   ✅ Kuadrant operator already configured"
+   fi
+ else
+   echo "   ⚠️  Kuadrant operator deployment not found yet; skipping patch for now"
+ fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ! kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager -o jsonpath='{.spec.template.spec.containers[0].env[?(@.name=="ISTIO_GATEWAY_CONTROLLER_NAMES")]}' | grep -q "ISTIO_GATEWAY_CONTROLLER_NAMES"; then
kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
--type='json' \
-p='[{"op":"add","path":"/spec/template/spec/containers/0/env/-","value":{"name":"ISTIO_GATEWAY_CONTROLLER_NAMES","value":"openshift.io/gateway-controller/v1"}}]'
echo " ✅ Kuadrant operator patched"
else
echo " ✅ Kuadrant operator already configured"
fi
if kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager >/dev/null 2>&1; then
# Check current value
if ! kubectl -n kuadrant-system get deployment kuadrant-operator-controller-manager \
-o jsonpath='{.spec.template.spec.containers[0].env[?(@.name=="ISTIO_GATEWAY_CONTROLLER_NAMES")].value}' \
| grep -q "^openshift\.io/gateway-controller/v1$"; then
kubectl -n kuadrant-system patch deployment kuadrant-operator-controller-manager \
--type='json' \
-p='[{"op":"add","path":"/spec/template/spec/containers/0/env/-","value":{"name":"ISTIO_GATEWAY_CONTROLLER_NAMES","value":"openshift.io/gateway-controller/v1"}}]'
echo " ✅ Kuadrant operator patched"
else
echo " ✅ Kuadrant operator already configured"
fi
else
echo " ⚠️ Kuadrant operator deployment not found yet; skipping patch for now"
fi
🤖 Prompt for AI Agents
In deployment/scripts/deploy-openshift.sh around lines 251-258, the current
check assumes the Kuadrant deployment exists and only tests for the env var's
presence, which causes a failing patch (and script exit under set -e) when the
deployment is absent and misses mismatched values; first query the deployment
existence (kubectl -n kuadrant-system get deployment
kuadrant-operator-controller-manager) and if it does not exist skip or wait
without causing script failure (use a conditional or || true), then fetch the
env var's actual value via jsonpath and compare it to the desired
"openshift.io/gateway-controller/v1"; if missing add the env entry, if present
but different use a JSON patch replace operation to update the value, and ensure
kubectl errors are handled so the script does not abort unexpectedly.

Copy link
Copy Markdown
Contributor

@jland-redhat jland-redhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so the idea is that our Deployment Pieces will live in the /deployment folder. And we include an overly in the maas-api folder for that references those pieces for development purposes if I understand everything correctly.

I don't know that we need the dev overlay in the maas-api folder. But I am fine with keeping it for now.

Approved but just want to check that we ran the script on a fresh cluster to validate deployment?

Comment on lines +27 to +33
resources:
requests:
memory: "64Mi"
cpu: "50m"
limits:
memory: "128Mi"
cpu: "200m"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is small enough where I don't think it matters unless it is causing us issues.

@bartoszmajsak
Copy link
Copy Markdown
Collaborator Author

bartoszmajsak commented Oct 3, 2025

I don't know that we need the dev overlay in the maas-api folder. But I am fine with keeping it for now.

@jland-redhat Can always be removed. It is used from Makefile for local dev - it enables debug mode and swaps the image.

@bartoszmajsak bartoszmajsak merged commit eeabb25 into opendatahub-io:main Oct 3, 2025
3 checks passed
@bartoszmajsak bartoszmajsak deleted the deployment/chore/consolidate-maas-api-manifests branch October 3, 2025 14:01
SB159 pushed a commit to SB159/maas-billing that referenced this pull request Oct 15, 2025
…#132)

* refactor(kustomize): consolidate deployment manifests

This change introduces a single source of truth to make deployments simpler.

Existing `maas-api/deploy` manifests are promoted to top-level
`deployment` folder, reducing duplication and improving the structure.

Installation script for OpenShift has been adjusted:

- Fixed OCP version check - using cluster info instead as it's more
  reliable
- Add helpers to wait for CRDs/pods and a version comparator;
- Auto-detect and patch AuthPolicy audience; apply gateway policies in
  sequence after infra becomes ready.
- Detect existing ODH/RHOAI KServe; deploy components only when missing,
  then proceed with retries.


Signed-off-by: Bartosz Majsak <bartosz.majsak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants