add swagger-ui integration with proper base path support #40
Conversation
Add standalone swagger-ui deployment alongside existing redocly setup: - Configure swagger-ui to handle /docs/ base path via URLRewrite filter - Include spec merger init container for OpenAPI aggregation - Add OIDC authentication support through authelia client - Create dedicated HTTPRoute with path prefix matching and rewriting - Add heimdall ruleset for public access to documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Jordan Evans <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds Swagger UI integration to the LFX platform alongside the existing Redocly documentation setup. The implementation includes proper base path support, OIDC authentication through Authelia, and OpenAPI spec aggregation capabilities.
Key changes:
- Configure Swagger UI deployment with spec merger init container for OpenAPI aggregation
- Add OIDC authentication support via Authelia client configuration
- Create HTTPRoute with URLRewrite filter to handle
/docs/base path routing
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/lfx-platform/values.yaml | Adds Swagger UI configuration and Authelia OIDC client setup |
| charts/lfx-platform/templates/swagger_ui/deployment.yaml | Creates Swagger UI deployment with spec merger init container |
| charts/lfx-platform/templates/swagger_ui/service.yaml | Defines Kubernetes service for Swagger UI |
| charts/lfx-platform/templates/swagger_ui/httproute.yaml | Configures Gateway API HTTPRoute with path rewriting |
| charts/lfx-platform/templates/swagger_ui/ruleset.yaml | Creates Heimdall ruleset for public documentation access |
| charts/lfx-platform/templates/swagger_ui/configmap.yaml | Provides shell script for OpenAPI spec merging and OIDC configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Currently the docs are built by an init container, but long-term we'll want to rebuild them periodically (or when certain deployments are updated). That can come in via another change later. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Swagger UI feature to the Helm chart (templates for Deployment, Service, HTTPRoute, RuleSet, ConfigMap with a merge script), extends values with Swagger UI and Authelia OIDC client settings, bumps chart version, and adds "swaggerapi" to the cspell dictionary. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Gateway as Gateway (HTTPRoute)
participant Heimdall as Heimdall (optional)
participant Service as Service: swagger-ui
participant Pod as Pod: swagger-ui
rect rgba(200,220,255,0.18)
note over Pod: Init phase (spec-merger)
participant Init as InitContainer
Init->>Init: run merge-specs.sh\n- fetch & validate specs\n- openapi-merge-cli merge\n- inject server & optional OIDC\n- write /shared/openapi-merged.yaml
Init-->>Pod: merged spec available (/shared/openapi-merged.yaml)
end
note over Pod: Main container serves Swagger UI (port 8080)
User->>Gateway: GET https://lfx-api.<domain>/docs
alt Heimdall enabled & auth enabled
Gateway->>Heimdall: authorize /docs/*
Heimdall-->>Gateway: allow_all (RuleSet)
end
Gateway->>Service: route /docs → swagger-ui (rewrite /docs → /)
Service->>Pod: forward to port 8080
Pod-->>User: Swagger UI served (uses merged spec)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed Checks (1 warning, 1 inconclusive)
✅ Passed Checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches🧪 Generate unit tests
Comment |
Signed-off-by: Jordan Evans <[email protected]>
Signed-off-by: Jordan Evans <[email protected]>
Signed-off-by: Jordan Evans <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
charts/lfx-platform/values.yaml (2)
444-446: Redirect URI naming now consistent with Swagger UI’s default.The use of oauth2-redirect.html matches Swagger UI’s expected path and resolves the prior mismatch noted in earlier reviews.
22-29: Pin container image tags; avoid 'latest' and broad majors.
- Using 'latest' for swagger-ui and a broad major tag for node undermines reproducibility and supply-chain safety. Pin explicit versions (ideally to digests) to make builds deterministic.
Apply a concrete pin (adjust versions as desired):
lfx: swagger_ui: enabled: true image: swaggerapi/swagger-ui - tag: latest + tag: "v5.17.14" # pin to a known-good release merger: image: node - tag: 24-alpine + tag: "24.6-alpine" # or pin to a digestAdditionally consider switching both to immutable digests for production.
charts/lfx-platform/templates/swagger_ui/configmap.yaml (2)
48-59: Harden curl validation: follow redirects, timeouts, and retries.Specs might be served behind redirects; add -L, timeouts, and limited redirects/retries.
- HTTP_CODE=$(curl -s -o /dev/null -w "%{http_code}" "{{ $fullURL }}") + HTTP_CODE=$(curl -sSL --retry 3 --retry-delay 1 --connect-timeout 5 --max-time 30 --max-redirs 3 -o /dev/null -w "%{http_code}" "{{ $fullURL }}")If you want to fail the script on non-2xx without manual code checks, use curl -fS and rely on set -e.
75-94: Use authorizationCode flow (with tokenUrl) and preserve endpoint security arrays.
- You’re adding OAuth2 implicit flow; switch to authorizationCode and include tokenUrl from values.
- Appending to .paths[].*.security may fail when security is absent; preserve existing arrays and create when missing.
- # Add OAuth2 security scheme to the merged spec + # Add OAuth2 (authorizationCode) security scheme to the merged spec echo "Adding OIDC configuration to merged spec..." @@ - # Add components.securitySchemes to the spec with client ID - # https://auth.k8s.orb.local/api/oidc/authorization - yq eval '.components.securitySchemes.oidc = {"type": "oauth2", "flows": {"implicit": {"authorizationUrl": "{{ .Values.lfx.swagger_ui.auth.authorizationUrl }}", "scopes": {"api:access": "Access to the API"}}}}' -i /shared/openapi-merged.yaml + # Add components.securitySchemes to the spec + yq eval ' + .components.securitySchemes.oidc = { + "type": "oauth2", + "flows": { + "authorizationCode": { + "authorizationUrl": "{{ .Values.lfx.swagger_ui.auth.authorizationUrl }}", + "tokenUrl": "{{ .Values.lfx.swagger_ui.auth.tokenUrl }}", + "scopes": {"api:access": "Access to the API"} + } + } + } + ' -i /shared/openapi-merged.yaml @@ - # Add oidc as an alternative security option to all endpoints - yq eval '.paths[].*.security += [{"oidc": ["api:access"]}]' -i /shared/openapi-merged.yaml + # Add oidc as an additional security option, preserving existing entries + yq eval ' + with(.paths[]; with(.[]; + if has("security") then + .security += [{"oidc": ["api:access"]}] + else + .security = [{"oidc": ["api:access"]}] + end)) + ' -i /shared/openapi-merged.yamlcharts/lfx-platform/templates/swagger_ui/deployment.yaml (1)
23-33: Past concern still applies: avoid installing deps or unpinned network fetches inside init script.Previous review flagged the init script doing installs and external HTTP calls. Prefer a purpose-built merger image with pinned tools, plus an egress-restricted NetworkPolicy.
If you share the merge-specs.sh contents or image, I can propose a hardened Dockerfile and a minimal egress policy.
🧹 Nitpick comments (11)
charts/lfx-platform/values.yaml (1)
34-38: Server base URL is fine for local dev; consider supporting multiple servers.You’re overwriting servers in the merged spec later. If consumers rely on multiple server entries, you might instead append a default without removing others.
I propose an alternative in the ConfigMap comment to preserve existing servers.
charts/lfx-platform/templates/swagger_ui/ruleset.yaml (1)
14-21: Allow HEAD and OPTIONS for browser compatibility and health checks.CORS preflight and some proxies use OPTIONS/HEAD. Restricting to GET can cause unnecessary 405s.
match: methods: - - GET + - GET + - HEAD + - OPTIONScharts/lfx-platform/templates/swagger_ui/configmap.yaml (3)
14-16: Tighten shell flags for robustness.Use nounset and safe IFS; pipefail isn’t available in /bin/sh on BusyBox, but -eu is.
- #!/bin/sh - set -e + #!/bin/sh + set -eu + IFS=$(printf '\n\t')
70-74: Don’t unconditionally drop existing servers; prefer appending default.Some specs declare multiple servers or server variables. Overwriting can break clients.
- yq eval 'del(.servers)' -i /shared/openapi-merged.yaml - - yq eval '.servers = [{"url": "{{ .Values.lfx.swagger_ui.server }}", "description": "Default api server"}]' -i /shared/openapi-merged.yaml + yq eval ' + .servers = ( (.servers // []) + [{"url": "{{ .Values.lfx.swagger_ui.server }}", "description": "Default api server"}] ) + ' -i /shared/openapi-merged.yaml
10-12: Label value consistency.You use app: swagger_ui here while other templates commonly use dash-case. Consider app: swagger-ui for consistency across selectors.
- labels: - app: swagger_ui + labels: + app: swagger-uiEnsure corresponding selectors match in the Deployment/Service.
charts/lfx-platform/templates/swagger_ui/service.yaml (2)
9-9: Namespace can be omitted (Helm will set it); keep consistent across templates.Helm injects metadata.namespace automatically. Keeping it is harmless but can cause drift if someone overrides. Pick one approach (explicit vs. implicit) and apply consistently across Deployment/Service/HTTPRoute.
- namespace: {{ .Release.Namespace }} + # namespace set by Helm release
10-18: Add standard Kubernetes app labels for better ops/selection.Using only app: swagger-ui works, but standard labels aid observability and selectors.
metadata: name: swagger-ui namespace: {{ .Release.Namespace }} labels: - app: swagger-ui + app: swagger-ui + app.kubernetes.io/name: swagger-ui + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/part-of: lfx-platform + helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version }} spec:charts/lfx-platform/templates/swagger_ui/deployment.yaml (3)
18-21: Add a checksum annotation to roll pods when the merge script changes.Without a checksum of the ConfigMap in pod template annotations, changing the merge script won’t trigger a rollout, leaving stale behavior.
template: metadata: labels: app: swagger-ui + annotations: + checksum/merge-script: {{ include (print $.Template.BasePath "/swagger_ui/configmap.yaml") . | sha256sum }}
23-33: Harden initContainer command/args invocation.Running the script via sh is fine, but using args clarifies intent and avoids shell resolution quirks if the script evolves.
- command: - - /bin/sh - - /scripts/merge-specs.sh + command: ["/bin/sh"] + args: ["/scripts/merge-specs.sh"]
21-40: Add pod/container security context (non-root, no writeable FS, no privilege escalation).Public-facing docs often live in shared clusters. Minimal hardening is low-cost.
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault initContainers: - name: spec-merger image: {{ .Values.lfx.swagger_ui.merger.image }}:{{ .Values.lfx.swagger_ui.merger.tag }} command: ["/bin/sh"] args: ["/scripts/merge-specs.sh"] volumeMounts: - name: shared-volume mountPath: /shared - name: merge-script mountPath: /scripts + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 65532 + runAsGroup: 65532 resources: requests: memory: "64Mi" cpu: "100m" limits: memory: "128Mi" cpu: "200m" containers: - name: swagger-ui image: {{ .Values.lfx.swagger_ui.image }}:{{ .Values.lfx.swagger_ui.tag }} ports: - name: http containerPort: 8080 volumeMounts: - name: shared-volume mountPath: /usr/share/nginx/html/spec env: - name: SWAGGER_JSON value: "/spec/openapi-merged.yaml" - name: OAUTH_CLIENT_ID value: {{ .Values.lfx.swagger_ui.auth.clientId | quote }} + securityContext: + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + runAsUser: 101 # nginx user in many images; adjust to match your base image + runAsGroup: 101If your base image uses different UIDs, I can align these to the actual image.
Also applies to: 40-61
charts/lfx-platform/templates/swagger_ui/httproute.yaml (1)
21-23: Consider host configurability and multi-host support.Hard-coding a single host to lfx-api. may be too restrictive. If you need docs on multiple hostnames or an alternate subdomain, consider allowing a list.
- hostnames: - - "lfx-api.{{ .Values.lfx.domain }}" + hostnames: +{{- range $h := (default (list (printf "lfx-api.%s" .Values.lfx.domain)) .Values.lfx.swagger_ui.hostnames) }} + - "{{ $h }}" +{{- end }}…and introduce lfx.swagger_ui.hostnames (optional) in values.yaml.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.cspell.json(1 hunks)charts/lfx-platform/Chart.yaml(1 hunks)charts/lfx-platform/templates/swagger_ui/configmap.yaml(1 hunks)charts/lfx-platform/templates/swagger_ui/deployment.yaml(1 hunks)charts/lfx-platform/templates/swagger_ui/httproute.yaml(1 hunks)charts/lfx-platform/templates/swagger_ui/ruleset.yaml(1 hunks)charts/lfx-platform/templates/swagger_ui/service.yaml(1 hunks)charts/lfx-platform/values.yaml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-platform/templates/swagger_ui/httproute.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-platform/templates/swagger_ui/service.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-platform/templates/swagger_ui/deployment.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
charts/lfx-platform/templates/swagger_ui/configmap.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (8)
charts/lfx-platform/Chart.yaml (1)
8-8: Chart version bump looks good.Version updated to 0.2.3. Consider updating any changelog/release notes in the repo to reflect the new Swagger UI feature.
.cspell.json (1)
30-30: Dictionary update is appropriate.Adding "swaggerapi" prevents noisy spellcheck failures given the new image reference.
charts/lfx-platform/templates/swagger_ui/ruleset.yaml (1)
4-6: YAML linter error is a false positive due to Helm templating.The leading control structure is valid for Helm templates. No change required.
charts/lfx-platform/templates/swagger_ui/service.yaml (1)
4-4: Yamllint is tripping on Helm templating; adjust linting to run on rendered manifests or ignore templates.The error “syntax error: expected the node content, but found '-'” at Line 4 is a yaml parser issue with Go templates (the “{{- … }}” token), not with your YAML. Prefer updating CI to lint rendered output (helm template | yamllint) or excluding templates from yamllint. Avoid code workarounds here.
Would you like a short CI snippet that renders this chart then runs yamllint so we lint actual YAML?
charts/lfx-platform/templates/swagger_ui/deployment.yaml (1)
3-4: Same yamllint/template parsing issue as in Service.Lint rendered manifests or exclude templates to avoid false “found '-'” errors at template blocks.
charts/lfx-platform/templates/swagger_ui/httproute.yaml (3)
24-41: Base-path rewrite looks correct; verify static assets and OAuth redirect flow under /docs.URLRewrite ReplacePrefixMatch: "/" will map “/docs/” to “/”, which should serve assets fine. Please test:
- /docs (UI index)
- /docs/swagger-ui.css (asset)
- /docs/oauth2-redirect.html (OAuth redirect helper)
If assets/redirect fail, we may need an additional asset path rule or explicit oauth2RedirectUrl in the UI config.I can provide a quick k6/hey script to probe these endpoints after a test deploy.
29-35: Auth middleware gating vs. “public docs” ruleset—confirm intended behavior.You add Heimdall middleware only when both heimdall.enabled and auth.enabled are true, but there’s also a ruleset to expose docs publicly. Ensure values combinations produce the intended result:
- Public docs: auth.enabled=false (no middleware, ruleset allows)
- Protected docs: auth.enabled=true (middleware present, ruleset should not open)
Would you like me to draft a values matrix test (helm template) to verify all combinations render correctly?
4-4: Yamllint false positive on template block; align CI approach.Same parse error as other templates. Lint rendered YAML or exclude templates to eliminate noise.
| enabled: true | ||
| domain: "https://auth.k8s.orb.local" | ||
| authorizationUrl: "https://auth.k8s.orb.local/api/oidc/authorization" | ||
| clientId: "docs" | ||
| server: "http://lfx-api.k8s.orb.local" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add tokenUrl to auth block and derive URLs from a single domain.
The config currently sets only authorizationUrl; for authorization code flow you’ll also need tokenUrl. Also, you have both domain and authorizationUrl — prefer deriving endpoints from domain to reduce drift.
auth:
enabled: true
- domain: "https://auth.k8s.orb.local"
- authorizationUrl: "https://auth.k8s.orb.local/api/oidc/authorization"
+ domain: "https://auth.k8s.orb.local"
+ authorizationUrl: "https://auth.k8s.orb.local/api/oidc/authorization"
+ tokenUrl: "https://auth.k8s.orb.local/api/oidc/token"
clientId: "docs"Follow up by updating the ConfigMap script to use both URLs (see separate 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.
| enabled: true | |
| domain: "https://auth.k8s.orb.local" | |
| authorizationUrl: "https://auth.k8s.orb.local/api/oidc/authorization" | |
| clientId: "docs" | |
| server: "http://lfx-api.k8s.orb.local" | |
| auth: | |
| enabled: true | |
| domain: "https://auth.k8s.orb.local" | |
| authorizationUrl: "https://auth.k8s.orb.local/api/oidc/authorization" | |
| tokenUrl: "https://auth.k8s.orb.local/api/oidc/token" | |
| clientId: "docs" | |
| server: "http://lfx-api.k8s.orb.local" |
🤖 Prompt for AI Agents
In charts/lfx-platform/values.yaml around lines 30 to 34, the auth block only
defines domain and authorizationUrl but lacks tokenUrl and duplicates endpoints;
change the block to derive both authorizationUrl and tokenUrl from the single
domain (e.g. authorizationUrl: "<domain>/api/oidc/authorization" and tokenUrl:
"<domain>/api/oidc/token") or remove explicit authorizationUrl and compute both
from domain, and add client-facing tokenUrl entry; then update the ConfigMap
script (per follow-up comment) to read and use both authorizationUrl and
tokenUrl from values rather than hardcoded endpoints.
| - client_id: docs | ||
| client_name: LFX Swagger UI Docs | ||
| public: true | ||
| redirect_uris: | ||
| - "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | ||
| - "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | ||
| scopes: | ||
| - openid | ||
| - profile | ||
| - api:access | ||
| audience: | ||
| - "http://lfx-api.k8s.orb.local" | ||
| grant_types: | ||
| - implicit | ||
| authorization_policy: one_factor | ||
| response_types: | ||
| - code | ||
| - id_token | ||
| - token |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch Swagger UI OIDC client from implicit to authorization code + PKCE.
Implicit flow is deprecated and not recommended. Since the Swagger UI frontend is a public client, use authorization_code with PKCE; restrict response types to code only.
Apply:
- client_id: docs
client_name: LFX Swagger UI Docs
public: true
redirect_uris:
- "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html"
- "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html"
scopes:
- openid
- email
- profile
- api:access
audience:
- "http://lfx-api.k8s.orb.local"
grant_types:
- - implicit
+ - authorization_code
authorization_policy: one_factor
response_types:
- - code
- - id_token
- - token
+ - code
+ token_endpoint_auth_method: noneEnsure the Swagger UI deployment enables PKCE (e.g., set usePkceWithAuthorizationCodeGrant=true via env/config).
Would you like me to scan the repo for the Swagger UI deployment to add PKCE-related settings?
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Searching for Swagger UI deployment references in charts directory..."
rg -n "swagger" -C3 -g "charts/**" || true
echo
echo "🔍 Searching for oauth2-redirect HTML references..."
rg -n "oauth2-redirect" -C3 -g "charts/**" || true
echo
echo "🔍 Searching for PKCE-related config flags (usePkce, pkce)..."
rg -n "usePkce" -C3 -g "charts/**" || true
rg -n "pkce" -C3 -g "charts/**" || trueLength of output: 15436
Switch Swagger UI OIDC client from Implicit to Authorization Code + PKCE
To eliminate usage of the deprecated Implicit flow and harden our public Swagger UI client, apply the following changes:
• charts/lfx-platform/values.yaml
– Change grant_types from implicit to authorization_code
– Restrict response_types to code only
– Add token_endpoint_auth_method: none
• charts/lfx-platform/templates/swagger_ui/configmap.yaml
– Replace the implicit flow in .components.securitySchemes.oidc with an authorizationCode flow
– Include both authorizationUrl and tokenUrl
• charts/lfx-platform/templates/swagger_ui/deployment.yaml
– Enable PKCE in the Swagger UI container (e.g. set usePkceWithAuthorizationCodeGrant=true via env var)
Example diffs:
--- charts/lfx-platform/values.yaml
@@ client_id: docs
- grant_types:
- - implicit
+ grant_types:
+ - authorization_code
authorization_policy: one_factor
- response_types:
- - code
- - id_token
- - token
+ response_types:
+ - code
+ token_endpoint_auth_method: none--- charts/lfx-platform/templates/swagger_ui/configmap.yaml
@@ # Add OAuth2 security scheme to the merged spec
- yq eval '.components.securitySchemes.oidc = {"type": "oauth2", "flows": {"implicit": {"authorizationUrl": "{{ .Values.lfx.swagger_ui.auth.authorizationUrl }}", "scopes": {"api:access": "Access to the API"}}}}' -i /shared/openapi-merged.yaml
+ yq eval '.components.securitySchemes.oidc = {
+ "type": "oauth2",
+ "flows": {
+ "authorizationCode": {
+ "authorizationUrl": "{{ .Values.lfx.swagger_ui.auth.authorizationUrl }}",
+ "tokenUrl": "{{ .Values.lfx.swagger_ui.auth.tokenUrl }}",
+ "scopes": {"api:access": "Access to the API"}
+ }
+ }
+ }' -i /shared/openapi-merged.yaml--- charts/lfx-platform/templates/swagger_ui/deployment.yaml
@@ spec:
env:
- name: SWAGGER_JSON_URL
value: "spec/openapi-merged.yaml"
- name: OAUTH_CLIENT_ID
value: {{ .Values.lfx.swagger_ui.auth.clientId | quote }}
+ - name: USE_PKCE_WITH_AUTHORIZATION_CODE_GRANT
+ value: "true"Please implement these updates to fully migrate Swagger UI to the more secure authorization code + PKCE flow.
📝 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.
| - client_id: docs | |
| client_name: LFX Swagger UI Docs | |
| public: true | |
| redirect_uris: | |
| - "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| - "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| scopes: | |
| - openid | |
| - profile | |
| - api:access | |
| audience: | |
| - "http://lfx-api.k8s.orb.local" | |
| grant_types: | |
| - implicit | |
| authorization_policy: one_factor | |
| response_types: | |
| - code | |
| - id_token | |
| - token | |
| # charts/lfx-platform/values.yaml | |
| - client_id: docs | |
| client_name: LFX Swagger UI Docs | |
| public: true | |
| redirect_uris: | |
| - "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| - "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| scopes: | |
| - openid | |
| - profile | |
| - api:access | |
| audience: | |
| - "http://lfx-api.k8s.orb.local" | |
| grant_types: | |
| - authorization_code | |
| authorization_policy: one_factor | |
| response_types: | |
| - code | |
| token_endpoint_auth_method: none |
| - client_id: docs | |
| client_name: LFX Swagger UI Docs | |
| public: true | |
| redirect_uris: | |
| - "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| - "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| scopes: | |
| - openid | |
| - profile | |
| - api:access | |
| audience: | |
| - "http://lfx-api.k8s.orb.local" | |
| grant_types: | |
| - implicit | |
| authorization_policy: one_factor | |
| response_types: | |
| - code | |
| - id_token | |
| - token | |
| # charts/lfx-platform/templates/swagger_ui/configmap.yaml | |
| # Add OAuth2 security scheme to the merged spec | |
| yq eval '.components.securitySchemes.oidc = { | |
| "type": "oauth2", | |
| "flows": { | |
| "authorizationCode": { | |
| "authorizationUrl": "{{ .Values.lfx.swagger_ui.auth.authorizationUrl }}", | |
| "tokenUrl": "{{ .Values.lfx.swagger_ui.auth.tokenUrl }}", | |
| "scopes": {"api:access": "Access to the API"} | |
| } | |
| } | |
| }' -i /shared/openapi-merged.yaml |
| - client_id: docs | |
| client_name: LFX Swagger UI Docs | |
| public: true | |
| redirect_uris: | |
| - "http://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| - "https://lfx-api.k8s.orb.local/docs/oauth2-redirect.html" | |
| scopes: | |
| - openid | |
| - profile | |
| - api:access | |
| audience: | |
| - "http://lfx-api.k8s.orb.local" | |
| grant_types: | |
| - implicit | |
| authorization_policy: one_factor | |
| response_types: | |
| - code | |
| - id_token | |
| - token | |
| # charts/lfx-platform/templates/swagger_ui/deployment.yaml | |
| env: | |
| - name: SWAGGER_JSON_URL | |
| value: "spec/openapi-merged.yaml" | |
| - name: OAUTH_CLIENT_ID | |
| value: {{ .Values.lfx.swagger_ui.auth.clientId | quote }} | |
| - name: USE_PKCE_WITH_AUTHORIZATION_CODE_GRANT | |
| value: "true" |
🤖 Prompt for AI Agents
In charts/lfx-platform/values.yaml around lines 440-459, update the public
Swagger UI OIDC client to use Authorization Code + PKCE: change grant_types from
implicit to authorization_code, set response_types to only code, and add
token_endpoint_auth_method: none; then update
charts/lfx-platform/templates/swagger_ui/configmap.yaml to replace the implicit
flow under .components.securitySchemes.oidc with an authorizationCode flow that
includes both authorizationUrl and tokenUrl; finally modify
charts/lfx-platform/templates/swagger_ui/deployment.yaml to enable PKCE in the
Swagger UI container (for example by adding an environment variable
usePkceWithAuthorizationCodeGrant=true or the equivalent config flag).
Signed-off-by: Jordan Evans <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml (1)
9-9: Resolved: namespace now uses release namespace (matches Helm best practices).Previous feedback about avoiding a hardcoded namespace has been addressed. Using
.Release.Namespacehere is correct and consistent with the rest of the chart.
🧹 Nitpick comments (3)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml (3)
3-3: Avoid emitting an empty YAML document when the condition is false.The top-level
---causes Helm to render an empty document when.Values.lfx.swagger_ui.enabledor.Values.heimdall.enabledis false. Not harmful for kubectl, but it trips some linters/pipelines. Remove the document separator or move it inside the conditional.Apply this diff:
---- {{- if and .Values.lfx.swagger_ui.enabled .Values.heimdall.enabled }} +--- apiVersion: heimdall.dadrus.github.com/v1alpha4
12-19: Broaden method and path matches to avoid edge-case denials (HEAD/OPTIONS and /docs without trailing slash).Some clients and proxies issue HEAD requests by default; CORS preflights use OPTIONS; and users may hit
/docs(no trailing slash). To reduce surprises, include HEAD and OPTIONS and add/docsto routes.Apply this diff:
match: methods: - - GET + - GET + - HEAD + - OPTIONS routes: + - path: /docs - path: /docs/ - path: /docs/**
7-9: Add standard Helm/Kubernetes labels for traceability.Including the common labels improves operability (kubectl selectors, ownership, debugging).
Apply this diff:
metadata: name: lfx-swagger-ui namespace: {{ .Release.Namespace }} + labels: + app.kubernetes.io/name: swagger-ui + app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/managed-by: Helm + app.kubernetes.io/part-of: lfx-platform
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml
[error] 4-4: syntax error: expected the node content, but found '-'
(syntax)
⏰ 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: MegaLinter
🔇 Additional comments (3)
charts/lfx-platform/templates/swagger_ui/ruleset.yaml (3)
4-4: Yamllint error is a known false positive for Helm templating.The "expected the node content, but found '-'" error comes from linting the raw template token
{{- ... }}. Safe to ignore at the template level; rely onhelm templateoutput for YAML validation.For sanity:
#!/bin/bash # Render this template with default values and validate YAML structure helm template . --show-only charts/lfx-platform/templates/swagger_ui/ruleset.yaml | yq -oy '.' >/dev/null
5-6: Ensure Heimdall RuleSet apiVersion matches installed CRD versionBoth of your RuleSet templates are using
apiVersion: heimdall.dadrus.github.com/v1alpha4, but we couldn’t confirm the cluster-side CRD version in CI:
- charts/lfx-platform/templates/whoami/ruleset.yaml (line 5)
- charts/lfx-platform/templates/swagger_ui/ruleset.yaml (line 5)
Please manually verify that the target cluster has the Heimdall RuleSet CRD at v1alpha4 (or that it at least supports it) before applying, for example:
kubectl get crd rulesets.heimdall.dadrus.github.com -o=jsonpath='{.spec.versions[*].name}'If the output does not include
v1alpha4, update your templates (or the CRD) to avoid apply-time errors.
20-20: Ensureauthorizer: allow_allis valid in your Heimdall setupI’ve found two occurrences of
authorizer: allow_allin your templates, while the default invalues.yamlis set todeny_all. Ifallow_allisn’t a recognized built-in or a pre-registered authorizer in your deployment, these routes may silently fail at admission time. Please confirm or adjust as needed:• charts/lfx-platform/templates/swagger_ui/ruleset.yaml (line 20)
• charts/lfx-platform/templates/whoami/ruleset.yaml (line 24)If your environment uses a different authorizer name (or you need to reference a custom authorizer), update these entries for consistency with your existing RuleSets.
Signed-off-by: Jordan Evans <[email protected]>
Add standalone swagger-ui deployment alongside existing redocly setup:
🤖 Generated with Claude Code