Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bitnami/keycloak] reverts(#30368) #31227

Merged
merged 12 commits into from
Jan 31, 2025
Merged

[bitnami/keycloak] reverts(#30368) #31227

merged 12 commits into from
Jan 31, 2025

Conversation

fmulero
Copy link
Collaborator

@fmulero fmulero commented Jan 7, 2025

Description of the change

Reverts #30368

Benefits

According to the documentation it's useful to have a fixed hostname and still use proxy-headers (proxyHeaders in this Helm chart):

The proxy-headers is still relevant even when the hostname is set to a full URL as the headers are used to determine the origin of the request. For example:

bin/kc.[sh|bat] start --hostname https://my.keycloak.org --proxy-headers xforwarded

In this case, while nothing is dynamically resolved from the X-Forwarded-* headers, the X-Forwarded-* headers are used to determine the correct origin of the request.

To summarize: this is useful for when you'd like to use a fixed hostname, but you're still using a reverse-proxy and would rather X-Forwarded-For be respected for determining the end user's IP address. In this case, X-Forwarded-Host and other values would be ignored, but X-Forwarded-For would be used.

Possible drawbacks

TBD

Applicable issues

Additional information

Please see comments in #30368

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@fmulero fmulero changed the title [bitnami/keyclak] reverts(#30368) [bitnami/keycloak] reverts(#30368) Jan 7, 2025
@github-actions github-actions bot added verify Execute verification workflow for these changes bitnami labels Jan 7, 2025
@github-actions github-actions bot requested a review from jotamartos January 7, 2025 11:47
bitnami-bot and others added 5 commits January 7, 2025 11:51
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
@RDemarneffe
Copy link
Contributor

RDemarneffe commented Jan 15, 2025

Hello,

I would like to deploy the "Fully dynamic URLs." case of the documentation https://www.keycloak.org/server/hostname#_fully_dynamic_urls but, this configuration requires no hostname configuration parameter set.
The current chart configuration does not support this case.

I use the same Keycloak instance as the identity provider for different products with different DNS. Each product uses the keycloak instance through a different URL (keycloak url: https://identity.<product dns>). To configure Keycloak to support this deployment configuration, I must be able to:

  • Configure Ingress with multiple hostnames. The part of my helm chart configuration is the following:
## Keycloak ingress parameters
## ref: https://kubernetes.io/docs/user-guide/ingress/
##
ingress:
  ## @param ingress.enabled Enable ingress record generation for Keycloak
  ##
  enabled: true
  ## @param ingress.ingressClassName IngressClass that will be be used to implement the Ingress (Kubernetes 1.18+)
  ## This is supported in Kubernetes 1.18+ and required if you have more than one IngressClass marked as the default for your cluster .
  ## ref: https://kubernetes.io/blog/2020/04/02/improvements-to-the-ingress-api-in-kubernetes-1.18/
  ##
  ingressClassName: "nginx"
  ## @param ingress.hostname Default host for the ingress record (evaluated as template)
  ##
  hostname: identity.<first-product-dns>
  ## @param ingress.hostnameStrict Disables dynamically resolving the hostname from request headers.
  ## Should always be set to true in production, unless your reverse proxy overwrites the Host header.
  ## If enabled, the hostname option needs to be specified.
  ##
  #
  # Let it as false because we use multiple hostname for Keycloak (with different DNS suffixes)
  hostnameStrict: false
  ## @param ingress.annotations [object] Additional annotations for the Ingress resource. To enable certificate autogeneration, place here your cert-manager annotations.
  ## Use this parameter to set the required annotations for cert-manager, see
  ## ref: https://cert-manager.io/docs/usage/ingress/#supported-annotations
  ## e.g:
  ## annotations:
  ##   kubernetes.io/ingress.class: nginx
  ##   cert-manager.io/cluster-issuer: cluster-issuer-name
  ##
  annotations:
    # A common issue with Keycloak and nginx is that the proxy buffer may be too
    # small for what Keycloak is trying to send. This will result in a Bad Gateway
    # (502) error. There are many issues around the internet about this. The
    # solution is to increase the buffer size of nginx. This can be done by
    # creating an annotation in the ingress specification
    # See: https://stackoverflow.com/questions/57503590/upstream-sent-too-big-header-while-reading-response-header-from-upstream-in-keyc
    nginx.ingress.kubernetes.io/proxy-buffer-size: "256k"
  ## @param ingress.tls Enable TLS configuration for the host defined at `ingress.hostname` parameter
  ## TLS certificates will be retrieved from a TLS secret with name: `{{- printf "%s-tls" (tpl .Values.ingress.hostname .) }}`
  ## You can:
  ##   - Use the `ingress.secrets` parameter to create this TLS secret
  ##   - Rely on cert-manager to create it by setting the corresponding annotations
  ##   - Rely on Helm to create self-signed certificates by setting `ingress.selfSigned=true`
  ##
  tls: true
  ## @param ingress.extraHosts An array with additional hostname(s) to be covered with the ingress record
  ## e.g:
  ## extraHosts:
  ##   - name: keycloak.local
  ##     path: /
  ##
  extraHosts:
    - name: identity.<second-product-dns>
      path: /
  ## @param ingress.extraTls The tls configuration for additional hostnames to be covered with this ingress record.
  ## see: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls
  ## extraTls:
  ## - hosts:
  ##     - keycloak.local
  ##   secretName: keycloak.local-tls
  ##
  extraTls:
    - hosts:
        - identity<first-product-dns>
      secretName: cluster-wildcard-tls
    - hosts:
        - identity.<second-product-dns>
      secretName: ${replace(dnsName, ".", "-")}-cluster-wildcard-tls
  • Avoid setting the KEYCLOAK_HOSTNAME environment variable.

If the KEYCLOAK_HOSTNAME environment variable is set, Keycloak always reply with the identity.<first-product-dns> url (this url is also referenced into the tokens emitted) and the second product is not able to validate / trust the tokens.

By reading the documentation, I think we have 2 options:

  • Adding a separate configuration parameter for setting or not the KEYCLOAK_HOSTNAME environment variable;
  • Using the value of ´ingress.hostnameStrictto prevent setting theKEYCLOAK_HOSTNAME` environment variable.

I think the second one is the best.

Regards,
Renaud

Carlos Rodríguez Hernández and others added 2 commits January 17, 2025 12:45
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
@carrodher carrodher added bitnami and removed bitnami labels Jan 22, 2025
carrodher and others added 2 commits January 22, 2025 18:28
Signed-off-by: Carlos Rodríguez Hernández <[email protected]>
Signed-off-by: Bitnami Containers <[email protected]>
@carrodher carrodher removed the request for review from jotamartos January 23, 2025 09:16
@carrodher carrodher assigned fmulero and unassigned jotamartos Jan 23, 2025
juan131
juan131 previously approved these changes Jan 23, 2025
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

LGTM! However, this doesn't address the use case reported by @RDemarneffe

@fmulero
Copy link
Collaborator Author

fmulero commented Jan 27, 2025

Hi @RDemarneffe, I think these changes are compatible with your needs. Have you tried unsetting the ingress.hostname value? For instance using this config:

## Keycloak ingress parameters
## ref: https://kubernetes.io/docs/user-guide/ingress/
##
ingress:
  ## @param ingress.enabled Enable ingress record generation for Keycloak
  ##
  enabled: true
  ## @param ingress.ingressClassName IngressClass that will be be used to implement the Ingress (Kubernetes 1.18+)
  ## This is supported in Kubernetes 1.18+ and required if you have more than one IngressClass marked as the default for your cluster .
  ## ref: https://kubernetes.io/blog/2020/04/02/improvements-to-the-ingress-api-in-kubernetes-1.18/
  ##
  ingressClassName: "nginx"
  ## @param ingress.hostname Default host for the ingress record (evaluated as template)
  ##
  hostname: ""
  ## @param ingress.hostnameStrict Disables dynamically resolving the hostname from request headers.
  ## Should always be set to true in production, unless your reverse proxy overwrites the Host header.
  ## If enabled, the hostname option needs to be specified.
  ##
  #
  # Let it as false because we use multiple hostname for Keycloak (with different DNS suffixes)
  hostnameStrict: false
  ## @param ingress.annotations [object] Additional annotations for the Ingress resource. To enable certificate autogeneration, place here your cert-manager annotations.
  ## Use this parameter to set the required annotations for cert-manager, see
  ## ref: https://cert-manager.io/docs/usage/ingress/#supported-annotations
  ## e.g:
  ## annotations:
  ##   kubernetes.io/ingress.class: nginx
  ##   cert-manager.io/cluster-issuer: cluster-issuer-name
  ##
  annotations:
    # A common issue with Keycloak and nginx is that the proxy buffer may be too
    # small for what Keycloak is trying to send. This will result in a Bad Gateway
    # (502) error. There are many issues around the internet about this. The
    # solution is to increase the buffer size of nginx. This can be done by
    # creating an annotation in the ingress specification
    # See: https://stackoverflow.com/questions/57503590/upstream-sent-too-big-header-while-reading-response-header-from-upstream-in-keyc
    nginx.ingress.kubernetes.io/proxy-buffer-size: "256k"
  ## @param ingress.tls Enable TLS configuration for the host defined at `ingress.hostname` parameter
  ## TLS certificates will be retrieved from a TLS secret with name: `{{- printf "%s-tls" (tpl .Values.ingress.hostname .) }}`
  ## You can:
  ##   - Use the `ingress.secrets` parameter to create this TLS secret
  ##   - Rely on cert-manager to create it by setting the corresponding annotations
  ##   - Rely on Helm to create self-signed certificates by setting `ingress.selfSigned=true`
  ##
  tls: true
  ## @param ingress.extraHosts An array with additional hostname(s) to be covered with the ingress record
  ## e.g:
  ## extraHosts:
  ##   - name: keycloak.local
  ##     path: /
  ##
  extraHosts:
    - name: identity.<first-product-dns>
      path: /
    - name: identity.<second-product-dns>
      path: /
  ## @param ingress.extraTls The tls configuration for additional hostnames to be covered with this ingress record.
  ## see: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls
  ## extraTls:
  ## - hosts:
  ##     - keycloak.local
  ##   secretName: keycloak.local-tls
  ##
  extraTls:
    - hosts:
        - identity<first-product-dns>
      secretName: cluster-wildcard-tls
    - hosts:
        - identity.<second-product-dns>
      secretName: ${replace(dnsName, ".", "-")}-cluster-wildcard-tls

Ingress file is created for both entries ( identity.<first-product-dns> and identity.<second-product-dns>) and the hostname is not set in Keycloak

Signed-off-by: Bitnami Containers <[email protected]>
Copy link
Contributor

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

LGTM

@fmulero fmulero merged commit 328ffc4 into bitnami:main Jan 31, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitnami keycloak solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants