keycloak / keycloak-operator

ARCHIVED Kubernetes Operator for the no longer supported WildFly distribution of Keycloak
Apache License 2.0
429 stars 283 forks source link

Reconciliation deletes default mappers #589

Closed creckord closed 1 year ago

creckord commented 1 year ago

Describe the bug

Keycloak defines a couple of default client protocol mappers based on the client's protocol. For OIDC, it automatically creates mappers for Client ID, Client IP Address and Client Host on the first token request if they don't exist.

This clashes with the operator's reconciliation run: if the corresponding KeycloakClient definition does not include these required mappers, the operator will delete them, the next token request will recreate them, etc ad nauseam.

Usually, this shouldn't be too harmful, however this is where Keycloak #10417 - Race when creating client protocol mappers enters the stage. Due to constantly triggering the recreation of these mappers, the likelihood of triggering the bug increases immensely, especially for "popular" clients that see many token requests. This in turn breaks reconciliation and renders further client updates impossible:

{"level":"info","ts":1666080011.0711734,"logger":"action_runner","msg":"(    1)     FAILED update a-namespace/a-client : failed to UPDATE client: (500) 500 Internal Server Error"}

The Keycloak bug aside, I don't think the operator should delete required default mappers, even if they are not included in the CRD.

I'm not sure what the state of the keycloak-operator project is right now, given that it has been deprecated. But seeing as the new operator is currently far from a replacement, it would be great if we could still get a fix in here.

Version

Keycloak 18.0.1 / Operator 18.0.2-legacy

Expected behavior

The operator does not delete default protocol mappers mandated by the client's protocol, even if they are not explicitly included in the client's CRD

Actual behavior

All default protocol mappers not explicitly included in the client's CRD get deleted during reconciliation.

How to Reproduce?

  1. Roll out a client like this:
    apiVersion: keycloak.org/v1alpha1
    kind: KeycloakClient
    metadata:
    name: myclient
    spec:
    client:
    clientId: myclient
    protocol: openid-connect
    clientAuthenticatorType: client-secret
    enabled: true
    publicClient: false
    serviceAccountsEnabled: true
    directAccessGrantsEnabled: true
    secret: atopsecretsecretwith123numbersandstuff
    defaultClientScopes:
      - roles
    protocolMappers:
      - config:
          access.token.claim: 'true'
          id.token.claim: 'false'
          included.client.audience: myclient
        name: audience-mapping
        protocol: openid-connect
        protocolMapper: oidc-audience-mapper
    realmSelector:
    matchLabels:
      realm: myrealm
  2. Check the client's mappers in the Keycloak admin frontend and observe that there is exactly one mapper audience-mapping defined
  3. Get a token:
    curl -L -X POST -s -S -i -H 'content-type: application/x-www-form-urlencoded' \
                    --data-urlencode "grant_type=client_credentials" \
                    --data-urlencode "client_id=myclient" \
                    --data-urlencode "client_secret=atopsecretsecretwith123numbersandstuff" \
                    "https://keycloak.example.org/auth/realms/myrealm/protocol/openid-connect/token"
  4. Check the UI again and observe that the three additional mappers Client ID, Client IP Address and Client Host have been created
  5. Wait for the next operator reconciliation run
  6. Check the UI again and observe that the three additional mappers have been deleted again

Anything else?

No response

stianst commented 1 year ago

Thanks (again) for reporting this issue. Keycloak 19 was the last version that included this legacy Operator, and with the release of Keycloak 20 the Operator reached EOL and this repository will be archived, please see our blog post on this topic. If this issue is still valid for the Realm Operator, please re-open it there. Thanks for your understanding. And be sure to check out our new Operator!