✨ Keycloak dependency removed.#562
Open
jortel wants to merge 15 commits into
Open
Conversation
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
📝 WalkthroughWalkthroughThis PR replaces operator-managed Keycloak with Hub-centric OIDC and federation: it adds IdentityProvider/IdpClient/LdapProvider CRDs and CSV RBAC, removes Keycloak templates, updates Ansible to detect or accept federated IdPs and create CRs, injects OIDC env vars and APIKEY_SECRET into Hub/UI, and adds AUTH.md documentation. ChangesHub OIDC and Federated Auth Migration
Estimated code review effort: Possibly related issues:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
AUTH.md (3)
242-242: ⚡ Quick winClarify behavior when multiple IdentityProvider CRs exist.
The statement "Multiple providers can be configured (Hub will use the first one found)" could cause confusion since Kubernetes list order is not guaranteed to be stable. Consider documenting:
- Whether this scenario is supported or discouraged
- How "first" is determined (alphabetical by name, creation timestamp, etc.)
- Whether Hub will error/warn if multiple CRs exist
This would help operators avoid unexpected behavior if they accidentally create multiple IdentityProvider resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AUTH.md` at line 242, Update the AUTH.md section about multiple IdentityProvider CRs to explicitly state whether multiple IdentityProvider custom resources are supported or discouraged, describe exactly how the Hub selects one when multiple exist (e.g., selection by alphabetical metadata.name or by metadata.creationTimestamp or by unspecified/undefined ordering), and document whether Hub emits a warning or error if more than one IdentityProvider CR is present; if current behavior is to pick the "first" in an unpredictable list, mark multiple CRs as discouraged and recommend operators ensure a single IdentityProvider or use a clear deterministic selection mechanism.
12-14: ⚡ Quick winAdd language specifier to fenced code blocks.
The ASCII architecture diagrams should include a language identifier (use
textor leave empty) to satisfy markdown linters while preserving formatting.📝 Proposed fix
-``` +```text Browser → UI Route (/oidc proxy) → Hub OIDC → Hub API```diff -``` +```text Browser → UI Route (/oidc proxy) → Hub OIDC → [Federated IDP] → Hub API ↓ IdentityProvider CR</details> Also applies to: 17-21 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@AUTH.mdaround lines 12 - 14, Add a language specifier to the fenced code
blocks containing the ASCII architecture diagrams so markdown linters accept
them; specifically update the blocks containing "Browser → UI Route (/oidc
proxy) → Hub OIDC → Hub API" and the multi-line block that includes "[Federated
IDP]" and "IdentityProvider CR" to usetext (or an empty language) instead of plainso formatting is preserved and linting passes.</details> --- `296-301`: _💤 Low value_ **Consider adding Ingress troubleshooting examples.** The troubleshooting section uses OpenShift `route` commands. Since the document mentions "Route/Ingress" throughout, consider adding equivalent commands for Kubernetes Ingress environments: ```bash kubectl get ingress tackle -n konveyor-tackle -o jsonpath='{.spec.rules[0].host}' ``` This would help users on vanilla Kubernetes clusters. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AUTH.md` around lines 296 - 301, Add an equivalent Kubernetes Ingress example alongside the OpenShift Route commands in AUTH.md: after the kubectl get route tackle ... line, add the ingress command kubectl get ingress tackle -n konveyor-tackle -o jsonpath='{.spec.rules[0].host}' (and similarly note the OIDC_ISSUER retrieval remains the same for the tackle-hub and tackle-ui deployments using the env name OIDC_ISSUER) so users on vanilla Kubernetes can extract the host exactly like the Route example. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@bundle/manifests/konveyor-operator.clusterserviceversion.yaml:
- Around line 204-212: The operator ClusterRole/Role for "tackle-operator" is
missing permissions for the newly-owned CRDs (IdentityProvider, IdpClient,
LdapProvider); update the RBAC rule that grants access to the tackle.konveyor.io
API group (the "tackle-operator" Role/ClusterRole resource) to include the
resources identityproviders, idpclients, and ldapproviders with appropriate
verbs (get, list, watch, create, update, patch, delete as needed) so
reconciliation can create and manage these CRs.In
@helm/templates/crds/tackle.konveyor.io_identityproviders.yaml:
- Around line 41-119: The IdentityProvider CRD allows empty spec objects; update
the schema for the IdentityProvider.spec to mark required fields so invalid CRs
are rejected: add a required array listing "issuer", "clientId", "redirectURI"
(and optionally "name") under the spec object definition, ensure existing
properties (issuer, clientId, redirectURI, name) remain defined, and keep
x-kubernetes-map-type or object types intact; locate the IdentityProvider.spec
block (the spec properties section containing clientId, issuer, redirectURI,
name, tls, etc.) and add the required: [...] entry there to enforce validation.In
@helm/templates/crds/tackle.konveyor.io_idpclients.yaml:
- Around line 189-190: The IdpClient CRD schema currently allows resources
without a top-level spec; update the OpenAPI v3 schema for the IdpClient (the
IdpClient CRD object under the schema served: true) to declare spec as required
by adding a required: ["spec"] entry at the same level as type: object (and
ensure the existing spec properties remain under properties.spec). Locate the
IdpClient CRD schema block in
helm/templates/crds/tackle.konveyor.io_idpclients.yaml and insert the required
key so top-level spec is enforced by admission validation.In
@roles/tackle/defaults/main.yml:
- Around line 86-94: The removal of keycloak_database_image_fqin leaves
kai_database_image_fqin referencing a missing variable; update the defaults so
kai_database_image_fqin no longer depends on keycloak_database_image_fqin by
either adding an explicit default for kai_database_image_fqin or changing its
expression to use a safe fallback (e.g., a literal default or empty string)
instead of keycloak_database_image_fqin; locate and modify the
kai_database_image_fqin variable definition in defaults/main.yml and remove or
replace any occurrence of keycloak_database_image_fqin there.In
@roles/tackle/tasks/main.yml:
- Around line 640-647: The IdentityProvider CR task ("Create IdentityProvider CR
for federated authentication") runs before hub_oidc_issuer is available,
producing an invalid redirect URI; update the task's when clause to also require
hub_oidc_issuer to be defined and non-empty (e.g. add "hub_oidc_issuer is
defined" and "hub_oidc_issuer | length > 0") or move this task to a point after
the code that derives/sets hub_oidc_issuer so the customresource-idp.yml.j2
template can render the correct callback URI.- Around line 839-845: The "Create IdpClient CRs" task currently runs when
feature_auth_required is true even if app_base_url is empty, producing invalid
redirect URIs; update this by adding a readiness guard and/or a fail-fast
assert: require app_base_url to be set/non-empty (e.g.
app_base_url|default('')|trim != '') in the task's when clause and/or add a
preceding assert/fail task that checks app_base_url and aborts with a clear
message before the "Create IdpClient CRs" task (reference the task name "Create
IdpClient CRs", the template "customresource-idpclient.yml.j2", and the variable
app_base_url and feature_auth_required).In
@roles/tackle/templates/customresource-idp.yml.j2:
- Around line 21-22: The template currently hardcodes tls.insecure: true in
customresource-idp.yml.j2; change this to make TLS verification the default and
make disabling it opt-in by replacing the hardcoded value with a
templated/variable-driven default (e.g., use a variable like tls_insecure or
idp.tls.insecure with a default of false) and update any values/values.yaml or
role defaults to expose that variable so callers can opt in to insecure TLS when
needed; ensure references to tls.insecure in related rendering
logic/documentation are updated accordingly.In
@roles/tackle/templates/deployment-ui.yml.j2:
- Around line 101-106: The UI is currently always given OIDC env vars
(AUTH_REQUIRED, OIDC_ISSUER, OIDC_CLIENT_ID) causing a conflict when the
oauth-proxy sidecar is deployed for feature_auth_type == "oauth"; change the
template so those OIDC-related env entries are only emitted when
feature_auth_type is not "oauth" (or specifically when using OIDC auth), e.g.
wrap the AUTH_REQUIRED / OIDC_ISSUER / OIDC_CLIENT_ID entries behind a
conditional that checks feature_auth_type != "oauth" (or feature_auth_type ==
"oidc") so the oauth-proxy path and the UI OIDC path are not both advertised.
Nitpick comments:
In@AUTH.md:
- Line 242: Update the AUTH.md section about multiple IdentityProvider CRs to
explicitly state whether multiple IdentityProvider custom resources are
supported or discouraged, describe exactly how the Hub selects one when multiple
exist (e.g., selection by alphabetical metadata.name or by
metadata.creationTimestamp or by unspecified/undefined ordering), and document
whether Hub emits a warning or error if more than one IdentityProvider CR is
present; if current behavior is to pick the "first" in an unpredictable list,
mark multiple CRs as discouraged and recommend operators ensure a single
IdentityProvider or use a clear deterministic selection mechanism.- Around line 12-14: Add a language specifier to the fenced code blocks
containing the ASCII architecture diagrams so markdown linters accept them;
specifically update the blocks containing "Browser → UI Route (/oidc proxy) →
Hub OIDC → Hub API" and the multi-line block that includes "[Federated IDP]" and
"IdentityProvider CR" to usetext (or an empty language) instead of plain
so formatting is preserved and linting passes.- Around line 296-301: Add an equivalent Kubernetes Ingress example alongside
the OpenShift Route commands in AUTH.md: after the kubectl get route tackle ...
line, add the ingress command kubectl get ingress tackle -n konveyor-tackle -o
jsonpath='{.spec.rules[0].host}' (and similarly note the OIDC_ISSUER retrieval
remains the same for the tackle-hub and tackle-ui deployments using the env name
OIDC_ISSUER) so users on vanilla Kubernetes can extract the host exactly like
the Route example.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `7042fd35-dbc0-4461-8b1a-85953af7efd7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8e88d273279bdd2047c2bdfae8ba3e11a39ef631 and 56aef6857af50e953552ea9240b5522511243d70. </details> <details> <summary>📒 Files selected for processing (24)</summary> * `AUTH.md` * `bundle/manifests/konveyor-operator.clusterserviceversion.yaml` * `helm/templates/crds/tackle.konveyor.io_identityproviders.yaml` * `helm/templates/crds/tackle.konveyor.io_idpclients.yaml` * `helm/templates/crds/tackle.konveyor.io_ldapproviders.yaml` * `roles/tackle/defaults/main.yml` * `roles/tackle/tasks/main.yml` * `roles/tackle/templates/customresource-idp.yml.j2` * `roles/tackle/templates/customresource-idpclient.yml.j2` * `roles/tackle/templates/customresource-rhbk-keycloak.yml.j2` * `roles/tackle/templates/customresource-rhsso-keycloak.yml.j2` * `roles/tackle/templates/deployment-hub.yml.j2` * `roles/tackle/templates/deployment-keycloak-postgresql.yml.j2` * `roles/tackle/templates/deployment-keycloak-sso.yml.j2` * `roles/tackle/templates/deployment-ui.yml.j2` * `roles/tackle/templates/persistentvolumeclaim-keycloak-postgresql.yml.j2` * `roles/tackle/templates/secret-hub.yml.j2` * `roles/tackle/templates/secret-keycloak-db.yml.j2` * `roles/tackle/templates/secret-keycloak-postgresql.yml.j2` * `roles/tackle/templates/secret-keycloak-sso.yml.j2` * `roles/tackle/templates/service-keycloak-postgresql-migration.yml.j2` * `roles/tackle/templates/service-keycloak-postgresql.yml.j2` * `roles/tackle/templates/service-keycloak-rhbk.yml.j2` * `roles/tackle/templates/service-keycloak-sso.yml.j2` </details> <details> <summary>💤 Files with no reviewable changes (12)</summary> * roles/tackle/templates/deployment-keycloak-postgresql.yml.j2 * roles/tackle/templates/persistentvolumeclaim-keycloak-postgresql.yml.j2 * roles/tackle/templates/service-keycloak-postgresql-migration.yml.j2 * roles/tackle/templates/secret-keycloak-db.yml.j2 * roles/tackle/templates/secret-keycloak-postgresql.yml.j2 * roles/tackle/templates/secret-keycloak-sso.yml.j2 * roles/tackle/templates/service-keycloak-rhbk.yml.j2 * roles/tackle/templates/customresource-rhbk-keycloak.yml.j2 * roles/tackle/templates/service-keycloak-sso.yml.j2 * roles/tackle/templates/service-keycloak-postgresql.yml.j2 * roles/tackle/templates/deployment-keycloak-sso.yml.j2 * roles/tackle/templates/customresource-rhsso-keycloak.yml.j2 </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+86
to
+94
| # Deprecated Keycloak reference variables (for backwards compatibility only) | ||
| # Operator no longer deploys Keycloak - users must deploy and configure their own | ||
| # These variables exist only for explicit external Keycloak configuration | ||
|
|
||
| keycloak_sso_realm: "{{ app_name }}" | ||
| keycloak_sso_req_passwd_update: true | ||
| keycloak_sso_client_id: "{{ app_name }}-ui" | ||
| keycloak_api_audience: "konveyor-api" | ||
| keycloak_sso_tls_enabled: "{{ true if openshift_cluster | bool else false }}" | ||
| keycloak_sso_tls_secret_name: "{{ keycloak_sso_service_name }}-serving-cert" | ||
| keycloak_sso_port: "{{ '8443' if keycloak_sso_tls_enabled | bool else '8080' }}" | ||
| keycloak_sso_proto: "{{ 'https' if keycloak_sso_tls_enabled | bool else 'http' }}" | ||
| keycloak_sso_url: "{{ keycloak_sso_proto }}://{{ keycloak_sso_service_name }}.{{ app_namespace }}.svc:{{ keycloak_sso_port }}" | ||
| keycloak_sso_hostname: "" | ||
| keycloak_sso_hostname_backchannel_dynamic: false | ||
| keycloak_sso_url: "" # Set explicitly for external Keycloak | ||
| rhbk_url: "" # Set explicitly for external RHBK | ||
|
|
There was a problem hiding this comment.
Removing Keycloak DB defaults leaves a dangling reference used by Kai.
This change removes keycloak_database_image_fqin, but kai_database_image_fqin still depends on it later in the same file. That will break variable resolution when Kai paths are evaluated.
Suggested direction
+# Keep an explicit Kai DB image default independent of removed Keycloak vars.
+kai_database_image_fqin: "{{ lookup('env', 'RELATED_IMAGE_TACKLE_POSTGRES') }}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@roles/tackle/defaults/main.yml` around lines 86 - 94, The removal of
keycloak_database_image_fqin leaves kai_database_image_fqin referencing a
missing variable; update the defaults so kai_database_image_fqin no longer
depends on keycloak_database_image_fqin by either adding an explicit default for
kai_database_image_fqin or changing its expression to use a safe fallback (e.g.,
a literal default or empty string) instead of keycloak_database_image_fqin;
locate and modify the kai_database_image_fqin variable definition in
defaults/main.yml and remove or replace any occurrence of
keycloak_database_image_fqin there.
Contributor
Author
There was a problem hiding this comment.
Is kai actually accessing the keycloak db?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how kai_database is deployed and whether it references keycloak DB
rg -n "kai_database" roles/tackle/
echo "---"
# Check for any task that deploys kai-db
rg -n "kai.db\|kai-db\|kai_db" roles/tackle/tasks/
echo "---"
# Confirm keycloak_database_image_fqin usage across the repo
rg -n "keycloak_database_image_fqin" roles/Length of output: 2753
@jortel — No, Kai does not access the Keycloak database. Kai runs its own dedicated PostgreSQL instance (kai-db service), and kai_database_image_fqin was only reusing keycloak_database_image_fqin as a convenient way to pull the same Postgres image — not to share the database.
That said, the broken variable reference is still real: roles/tackle/defaults/main.yml line 288 still reads:
kai_database_image_fqin: "{{ keycloak_database_image_fqin }}"…and keycloak_database_image_fqin was removed by this PR. The fix is simply to give Kai its own direct image lookup:
kai_database_image_fqin: "{{ lookup('env', 'RELATED_IMAGE_TACKLE_POSTGRES') }}"
Comment on lines
+21
to
+22
| tls: | ||
| insecure: true |
There was a problem hiding this comment.
Do not hardcode tls.insecure: true for federated IDP.
This makes insecure TLS the default for all deployments. It should be opt-in and default to certificate verification enabled.
Suggested change
tls:
- insecure: true
+ insecure: {{ federated_idp_tls_insecure | default(false) | bool }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@roles/tackle/templates/customresource-idp.yml.j2` around lines 21 - 22, The
template currently hardcodes tls.insecure: true in customresource-idp.yml.j2;
change this to make TLS verification the default and make disabling it opt-in by
replacing the hardcoded value with a templated/variable-driven default (e.g.,
use a variable like tls_insecure or idp.tls.insecure with a default of false)
and update any values/values.yaml or role defaults to expose that variable so
callers can opt in to insecure TLS when needed; ensure references to
tls.insecure in related rendering logic/documentation are updated accordingly.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AUTH.md`:
- Around line 28-32: Add a language identifier to the fenced code block
containing the ASCII diagram in AUTH.md so it satisfies markdownlint MD040;
locate the triple-backtick block that contains "Browser → UI Route (/oidc proxy)
→ Hub OIDC → [LDAP Server] → Hub API" and change the opening fence from ``` to
```text (i.e., add "text" as the language specifier) so the diagram is a
lint-compliant fenced code block.
- Line 325: The example in AUTH.md shows a contradictory custom client id value
("id: 100") vs the rule that custom clients must be >= 1000; update the sample
"id" field in the example to a value >= 1000 (e.g., 1000) so the example matches
the stated rule and ensures consistency for readers referencing the "id" field
in the auth client example.
In `@bundle/manifests/tackle.konveyor.io_identityproviders.yaml`:
- Around line 41-118: The CRD currently allows an empty spec; add schema-level
required constraints so core IdentityProvider fields cannot be omitted: update
the spec schema for the IdentityProvider resource to include a "required" array
containing "name", "issuer", and "clientId" (and include "redirectURI" if the
controller treats it as mandatory), and ensure those properties (name, issuer,
clientId, redirectURI) remain defined under spec with correct types; this will
enforce validation at creation/update rather than failing later in reconcile
(refer to the spec object and property names clientId, clientSecret, issuer,
name, redirectURI in the manifest).
In `@bundle/manifests/tackle.konveyor.io_idpclients.yaml`:
- Around line 104-110: The IdpClient CRD schema currently forbids custom client
IDs by enforcing "maximum: 999" on the id property and also leaves "spec"
optional; update the IdpClient schema so custom clients are allowed and spec is
required: remove (or adjust) the "maximum: 999" constraint on the id property in
the IdpClient schema (the "id" property block) so IDs >=1000 are permitted, and
add "spec" to the top-level required array of the IdpClient schema so the "spec"
object is mandatory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cd7a304-eac9-4944-90c3-eb1d7f59559a
📒 Files selected for processing (7)
AUTH.mdbundle/manifests/konveyor-operator.clusterserviceversion.yamlbundle/manifests/tackle.konveyor.io_identityproviders.yamlbundle/manifests/tackle.konveyor.io_idpclients.yamlbundle/manifests/tackle.konveyor.io_ldapproviders.yamlhelm/templates/rbac/role.yamlroles/tackle/templates/customresource-idpclient.yml.j2
✅ Files skipped from review due to trivial changes (1)
- bundle/manifests/tackle.konveyor.io_ldapproviders.yaml
Comment on lines
+28
to
+32
| ``` | ||
| Browser → UI Route (/oidc proxy) → Hub OIDC → [LDAP Server] → Hub API | ||
| ↓ | ||
| LdapProvider CR | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to fenced block (markdownlint MD040).
The fenced block starting at Line 28 should specify a language (for this diagram, text is appropriate).
Suggested lint-compliant fence
-```
+```text
Browser → UI Route (/oidc proxy) → Hub OIDC → [LDAP Server] → Hub API
↓
LdapProvider CR</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AUTH.md` around lines 28 - 32, Add a language identifier to the fenced code
block containing the ASCII diagram in AUTH.md so it satisfies markdownlint
MD040; locate the triple-backtick block that contains "Browser → UI Route (/oidc
proxy) → Hub OIDC → [LDAP Server] → Hub API" and change the opening fence from
``` to ```text (i.e., add "text" as the language specifier) so the diagram is a
lint-compliant fenced code block.
| name: my-custom-app | ||
| namespace: konveyor-tackle | ||
| spec: | ||
| id: 100 # Must be >= 1000 for custom clients (< 1000 reserved for seeded clients) |
There was a problem hiding this comment.
Fix contradictory custom client ID example.
Line 325 uses id: 100 but the same line says custom clients must be >= 1000. Update the sample value to match the stated rule.
Suggested doc fix
- id: 100 # Must be >= 1000 for custom clients (< 1000 reserved for seeded clients)
+ id: 1000 # Must be >= 1000 for custom clients (< 1000 reserved for seeded clients)📝 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
| id: 100 # Must be >= 1000 for custom clients (< 1000 reserved for seeded clients) | |
| id: 1000 # Must be >= 1000 for custom clients (< 1000 reserved for seeded clients) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@AUTH.md` at line 325, The example in AUTH.md shows a contradictory custom
client id value ("id: 100") vs the rule that custom clients must be >= 1000;
update the sample "id" field in the example to a value >= 1000 (e.g., 1000) so
the example matches the stated rule and ensures consistency for readers
referencing the "id" field in the auth client example.
Comment on lines
+104
to
+110
| id: | ||
| description: |- | ||
| ID is the database ID for the seeded client. | ||
| Must be less than 1000 (reserved range for seeded clients). | ||
| maximum: 999 | ||
| minimum: 1 | ||
| type: integer |
There was a problem hiding this comment.
IdpClient schema currently blocks custom clients and permits missing spec.
Line 108 caps id at 999, which conflicts with the documented custom-client range (>= 1000), and Lines 189-190 do not require spec at the top level. Together, this creates a broken/weak API contract.
Suggested schema corrections
id:
description: |-
ID is the database ID for the seeded client.
Must be less than 1000 (reserved range for seeded clients).
- maximum: 999
minimum: 1
type: integer
@@
- type: object
+ required:
+ - spec
+ type: objectAlso applies to: 189-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundle/manifests/tackle.konveyor.io_idpclients.yaml` around lines 104 - 110,
The IdpClient CRD schema currently forbids custom client IDs by enforcing
"maximum: 999" on the id property and also leaves "spec" optional; update the
IdpClient schema so custom clients are allowed and spec is required: remove (or
adjust) the "maximum: 999" constraint on the id property in the IdpClient schema
(the "id" property block) so IDs >=1000 are permitted, and add "spec" to the
top-level required array of the IdpClient schema so the "spec" object is
mandatory.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
fabianvf
added a commit
to fabianvf/tackle2-operator
that referenced
this pull request
May 27, 2026
llm-proxy is now reached only through tackle2-hub's /services/llm-proxy reverse-proxy endpoint. The hub validates the bearer token, enforces the llm-proxy RBAC scope, and forwards user identity in the X-Hub-User / X-Hub-User-Attributes headers it injects in ServiceHandler.Forward. Operator changes: - Hub Deployment gains LLM_PROXY_URL env (only when kai_llm_proxy_enabled), pointing at the cluster-internal Service URL. Hub reads this at startup to register the llm-proxy route in its serviceRoutes map. - UI Deployment drops KAI_LLM_PROXY_URL; the new UI image's server-side proxy targets TACKLE_HUB_URL with pathRewrite /llm-proxy -> /services/llm-proxy. - llm-proxy ConfigMap drops the oauth2_token Keycloak JWKS auth block. llm-proxy itself does no token validation; the namespace NetworkPolicy is the only barrier against in-namespace callers bypassing the hub. Same trust posture as kai-solution-server today. - Renamed kai_llm_proxy_url -> kai_llm_proxy_internal_url to signal "only hub should call this directly". - test-llm-proxy.sh rewritten for the new auth flow: uses HTTP Basic Auth against hub's seeded admin user and hits the LLM through /hub/services/llm-proxy/* so the test path goes through the existing /hub UI proxy and does not depend on the new tackle2-ui image. Companion changes required in: - tackle2-hub: register llm-proxy in serviceRoutes + inject X-Hub-User identity headers in ServiceHandler.Forward. - tackle2-ui: server/src/proxies.js retargets /llm-proxy at TACKLE_HUB_URL with pathRewrite to /services/llm-proxy. (Browser use only; not needed for the e2e test in this PR.) Based on PR konveyor#562 (Keycloak dependency removal / hub OIDC provider).
llm-proxy is now reached only through tackle2-hub's /services/llm-proxy reverse-proxy endpoint. The hub validates the bearer token, enforces the llm-proxy RBAC scope, and forwards user identity in the X-Hub-User / X-Hub-User-Attributes headers it injects in ServiceHandler.Forward. Operator changes: - Hub Deployment gains LLM_PROXY_URL env (only when kai_llm_proxy_enabled), pointing at the cluster-internal Service URL. Hub reads this at startup to register the llm-proxy route in its serviceRoutes map. - UI Deployment drops KAI_LLM_PROXY_URL; the new UI image's server-side proxy targets TACKLE_HUB_URL with pathRewrite /llm-proxy -> /services/llm-proxy. - llm-proxy ConfigMap drops the oauth2_token Keycloak JWKS auth block. llm-proxy itself does no token validation; the namespace NetworkPolicy is the only barrier against in-namespace callers bypassing the hub. Same trust posture as kai-solution-server today. - Renamed kai_llm_proxy_url -> kai_llm_proxy_internal_url to signal "only hub should call this directly". - test-llm-proxy.sh rewritten for the new auth flow: uses HTTP Basic Auth against hub's seeded admin user and hits the LLM through /hub/services/llm-proxy/* so the test path goes through the existing /hub UI proxy and does not depend on the new tackle2-ui image. Companion changes required in: - tackle2-hub: register llm-proxy in serviceRoutes + inject X-Hub-User identity headers in ServiceHandler.Forward. - tackle2-ui: server/src/proxies.js retargets /llm-proxy at TACKLE_HUB_URL with pathRewrite to /services/llm-proxy. (Browser use only; not needed for the e2e test in this PR.) Based on PR konveyor#562 (Keycloak dependency removal / hub OIDC provider).
✨ Route llm-proxy through hub /services/llm-proxy
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bundle/manifests/konveyor-operator.clusterserviceversion.yaml (2)
240-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the CSV auth description.
The install description still says auth mode deploys Keycloak, but this PR moves auth to Hub OIDC with optional federation. OLM surfaces this text directly to users, so it should match the new behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bundle/manifests/konveyor-operator.clusterserviceversion.yaml` around lines 240 - 242, The CSV's install description string that mentions "Authentication capabilities may be enabled via the `feature_auth_required` parameter..." and specifically calls out Keycloak must be updated to reflect the new behavior: replace the Keycloak-specific text with a description stating that auth is provided via Hub OIDC (with optional federation to external providers) when `feature_auth_required` is enabled; ensure the CSV entry that contains the bullet list and the sentence mentioning Keycloak is edited (the install/description field containing "Authentication capabilities..." and the bullet "* Keycloak, to manage authentication...") so OLM shows the new Hub OIDC/federation behavior instead of Keycloak.
282-296:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop write verbs for legacy Keycloak CRs.
This PR removes operator-managed Keycloak, so this rule should not still allow cluster-wide
create/update/patch/deleteon Keycloak resources. Keeping those verbs is broader than the new migration/detection flow needs.Suggested fix
- apiGroups: - keycloak.org - k8s.keycloak.org resources: - keycloaks - keycloakrealms - keycloakclients - keycloakusers verbs: - get - list - watch - - create - - update - - patch - - delete🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bundle/manifests/konveyor-operator.clusterserviceversion.yaml` around lines 282 - 296, The ClusterServiceVersion RBAC rule for API groups "keycloak.org" and "k8s.keycloak.org" currently grants write verbs (create, update, patch, delete) for resources "keycloaks", "keycloakrealms", "keycloakclients", and "keycloakusers"; remove the write verbs so only read/detect verbs remain (e.g., keep get, list, watch) to match the new migration/detection-only behavior and avoid cluster-wide operator-managed Keycloak writes.helm/templates/crds/tackle.konveyor.io_idpclients.yaml (1)
104-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow custom
IdpClientIDs.The CRD caps
spec.idat999, butAUTH.mdnow documents user-createdIdpClientresources with IDs>= 1000. As written, admission will reject the custom-client path this PR introduces.Suggested fix
id: description: |- - ID is the database ID for the seeded client. - Must be less than 1000 (reserved range for seeded clients). - maximum: 999 + ID is the database ID for the client. + IDs 1-999 are reserved for operator-seeded clients. minimum: 1 type: integer🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/templates/crds/tackle.konveyor.io_idpclients.yaml` around lines 104 - 109, The CRD currently enforces a maximum of 999 on the IdpClient "id" field which blocks user-created IdpClient resources with spec.id >= 1000; remove the hard cap (delete the "maximum: 999" entry) or raise it to an intentionally large upper bound so the "id" property under the IdpClient CRD (the "id" schema used for spec.id) no longer rejects custom-client IDs >=1000, then run a CRD validation check to ensure the schema still requires an integer >= 1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/llm-proxy/test-llm-proxy.sh`:
- Around line 42-46: The script sets HUB_URL to "http://$INGRESS_HOST" which
breaks when ingress enforces SSL redirects for auth-enabled runs; update the
INGRESS handling (the block that sets INGRESS_HOST and HUB_URL) to set
HUB_URL="https://$INGRESS_HOST" when AUTH_MODE is not "none" (or when an
AUTH_MODE env var indicates auth is enabled), otherwise keep http; ensure this
uses the existing INGRESS_HOST variable and preserves the empty-string fallback
behavior.
---
Outside diff comments:
In `@bundle/manifests/konveyor-operator.clusterserviceversion.yaml`:
- Around line 240-242: The CSV's install description string that mentions
"Authentication capabilities may be enabled via the `feature_auth_required`
parameter..." and specifically calls out Keycloak must be updated to reflect the
new behavior: replace the Keycloak-specific text with a description stating that
auth is provided via Hub OIDC (with optional federation to external providers)
when `feature_auth_required` is enabled; ensure the CSV entry that contains the
bullet list and the sentence mentioning Keycloak is edited (the
install/description field containing "Authentication capabilities..." and the
bullet "* Keycloak, to manage authentication...") so OLM shows the new Hub
OIDC/federation behavior instead of Keycloak.
- Around line 282-296: The ClusterServiceVersion RBAC rule for API groups
"keycloak.org" and "k8s.keycloak.org" currently grants write verbs (create,
update, patch, delete) for resources "keycloaks", "keycloakrealms",
"keycloakclients", and "keycloakusers"; remove the write verbs so only
read/detect verbs remain (e.g., keep get, list, watch) to match the new
migration/detection-only behavior and avoid cluster-wide operator-managed
Keycloak writes.
In `@helm/templates/crds/tackle.konveyor.io_idpclients.yaml`:
- Around line 104-109: The CRD currently enforces a maximum of 999 on the
IdpClient "id" field which blocks user-created IdpClient resources with spec.id
>= 1000; remove the hard cap (delete the "maximum: 999" entry) or raise it to an
intentionally large upper bound so the "id" property under the IdpClient CRD
(the "id" schema used for spec.id) no longer rejects custom-client IDs >=1000,
then run a CRD validation check to ensure the schema still requires an integer
>= 1.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 565889ed-011f-4f7d-b629-715a4828886b
📒 Files selected for processing (20)
bundle/manifests/konveyor-operator.clusterserviceversion.yamlbundle/manifests/tackle.konveyor.io_identityproviders.yamlbundle/manifests/tackle.konveyor.io_idpclients.yamlbundle/manifests/tackle.konveyor.io_ldapproviders.yamlhelm/templates/crds/tackle.konveyor.io_identityproviders.yamlhelm/templates/crds/tackle.konveyor.io_idpclients.yamlhelm/templates/crds/tackle.konveyor.io_ldapproviders.yamlhelm/templates/deployment.yamlhelm/values.yamlroles/tackle/defaults/main.ymlroles/tackle/tasks/main.ymlroles/tackle/templates/deployment-hub.yml.j2roles/tackle/templates/deployment-ui.yml.j2roles/tackle/templates/ingress-ui.yml.j2roles/tackle/templates/kai/llm-proxy-configmap.yaml.j2roles/tackle/templates/route-ui.yml.j2roles/tackle/templates/secret-cookie-secret.yml.j2roles/tackle/templates/service-ui.yml.j2roles/tackle/templates/serviceaccount-ui.yml.j2test/e2e/llm-proxy/test-llm-proxy.sh
💤 Files with no reviewable changes (5)
- roles/tackle/templates/route-ui.yml.j2
- roles/tackle/templates/secret-cookie-secret.yml.j2
- helm/templates/deployment.yaml
- roles/tackle/templates/serviceaccount-ui.yml.j2
- helm/values.yaml
✅ Files skipped from review due to trivial changes (3)
- helm/templates/crds/tackle.konveyor.io_identityproviders.yaml
- helm/templates/crds/tackle.konveyor.io_ldapproviders.yaml
- bundle/manifests/tackle.konveyor.io_identityproviders.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- bundle/manifests/tackle.konveyor.io_ldapproviders.yaml
- bundle/manifests/tackle.konveyor.io_idpclients.yaml
- roles/tackle/tasks/main.yml
Comment on lines
+42
to
46
| if kubectl get ingress tackle -n "$NAMESPACE" &>/dev/null; then | ||
| INGRESS_HOST=$(kubectl get ingress tackle -n "$NAMESPACE" -o jsonpath='{.spec.rules[0].host}' 2>/dev/null || echo "") | ||
| if [ -n "$INGRESS_HOST" ]; then | ||
| HUB_URL="http://$INGRESS_HOST" | ||
| fi |
There was a problem hiding this comment.
Use HTTPS for ingress-derived HUB_URL in auth-enabled runs.
Line 45 hardcodes http://, but ingress auth mode now enforces SSL redirect; that can make the Line 76 and Line 110 requests return redirects/non-JSON instead of API responses.
Suggested fix
HUB_URL=""
+CURL_TLS_ARGS=()
if kubectl get ingress tackle -n "$NAMESPACE" &>/dev/null; then
INGRESS_HOST=$(kubectl get ingress tackle -n "$NAMESPACE" -o jsonpath='{.spec.rules[0].host}' 2>/dev/null || echo "")
if [ -n "$INGRESS_HOST" ]; then
- HUB_URL="http://$INGRESS_HOST"
+ HUB_URL="https://$INGRESS_HOST"
+ CURL_TLS_ARGS=(-k)
fi
fi
@@
- PROXY_RESPONSE=$(curl -s -X POST "${HUB_URL}/hub/services/llm-proxy/v1/chat/completions" \
+ PROXY_RESPONSE=$(curl -s "${CURL_TLS_ARGS[@]}" -X POST "${HUB_URL}/hub/services/llm-proxy/v1/chat/completions" \
@@
-INVALID_STATUS=$(curl -s -o /dev/null -w "%{http_code}" -X POST "${HUB_URL}/hub/services/llm-proxy/v1/chat/completions" \
+INVALID_STATUS=$(curl -s "${CURL_TLS_ARGS[@]}" -o /dev/null -w "%{http_code}" -X POST "${HUB_URL}/hub/services/llm-proxy/v1/chat/completions" \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/llm-proxy/test-llm-proxy.sh` around lines 42 - 46, The script sets
HUB_URL to "http://$INGRESS_HOST" which breaks when ingress enforces SSL
redirects for auth-enabled runs; update the INGRESS handling (the block that
sets INGRESS_HOST and HUB_URL) to set HUB_URL="https://$INGRESS_HOST" when
AUTH_MODE is not "none" (or when an AUTH_MODE env var indicates auth is
enabled), otherwise keep http; ensure this uses the existing INGRESS_HOST
variable and preserves the empty-string fallback behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces a new OIDC-based authentication architecture where the Hub acts as the primary OpenID Connect provider, with optional federation to external identity
providers (Keycloak, RHSSO, RHBK, LDAP).
Key Changes
New Authentication Architecture
Hub as OIDC Provider: Hub now serves as a built-in OAuth 2.0 / OIDC server with endpoints at
/oidc/oidc/.well-known/openid-configuration/oidc/authorize/oidc/token/oidc/callbackFlexible Authentication Modes:
New Custom Resource Definitions
Added three new CRDs under
tackle.konveyor.io/v1alpha1:Documentation
Backward Compatibility
Security Features
APIKEY_SECRETMigration Impact
For existing deployments:
https://<ui-route-host>/oidc/callbackcloses #560
Summary by CodeRabbit
Documentation
New Features
Refactor
Tests