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

keycloak_quarkus: use absolute path for certificate files #39

Conversation

cattz
Copy link

@cattz cattz commented Jun 30, 2022

If you want to use a certificate for both Keycloak and Nginx, the cert files are quite unlikely to be under keycloak.home
Even in case you want to keep the under Keycloak path, you can always define the variable:

   keycloak_quarkus_cert_file: "{{ keycloak.home }}/conf/my_keycloak_cert.pem"
   keycloak_quarkus_key_file: "{{ keycloak.home }}/conf/my_keycloak_key.pem"

This change will break your previous configuration (unless you use the default values).
To fix your config for the new changes, you will have use full paths for the 2 variables mentioned above. For example:

   keycloak_quarkus_key_file: "/etc/ssl/certs/my_keycloak_key.pem"
   keycloak_quarkus_cert_file: "/etc/ssl/certs/my_keycloak_cert.pem"

@guidograzioli
Copy link
Member

I am ok with the change, but you'll need to update meta/argument_specs, defaults/main.yml and README as well

@guidograzioli guidograzioli added the minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix label Jun 30, 2022
@cattz cattz force-pushed the use_absolute_path_for_certs branch from 063eb5b to 41caa49 Compare July 1, 2022 08:31
Copy link
Member

@guidograzioli guidograzioli left a comment

Choose a reason for hiding this comment

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

LGTM

@cattz
Copy link
Author

cattz commented Jul 1, 2022

This change will break existing configurations, unless they are using the default value. Not sure how to warn users about this

@guidograzioli
Copy link
Member

guidograzioli commented Jul 1, 2022

This change will break existing configurations, unless they are using the default value. Not sure how to warn users about this

I was thinking at that; if it is breaking, I'll switch labels on the PR from minor_change to breaking_change; the generated changelog will contain a link to this page: so please edit your first comment with a few lines of description and porting guide
(see here: https://ansiblemiddleware.com/keycloak/main/CHANGELOG.html#v1-0-7-devel ); you are not required to do it now, but it is needed before we release next version to galaxy (I'll edit the title that shows in the changelog text with the keycloak_quarkus: prefix)

@guidograzioli guidograzioli added breaking_changes MUST include changes that break existing playbooks and removed minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix labels Jul 1, 2022
@guidograzioli guidograzioli changed the title Use absolute path for certificate files keycloak_quarkus: use absolute path for certificate files Jul 1, 2022
@cattz
Copy link
Author

cattz commented Jul 1, 2022

I'm not sure on why CI is failing now

@guidograzioli
Copy link
Member

Could be docker running out of ports; lets try again and if it fails I'll debug molecule later

@guidograzioli
Copy link
Member

Found the issue here:
https://github.com/ansible-middleware/keycloak/blob/main/molecule/quarkus/converge.yml#L12
(the molecule test playbook was not using default and broke).
The certs are created in the prepare step here:
https://github.com/ansible-middleware/keycloak/blob/main/molecule/quarkus/prepare.yml#L9
Please amend the test config so it passes

@guidograzioli
Copy link
Member

LGTM (again) thanks for contributing

@guidograzioli guidograzioli merged commit 9252433 into ansible-middleware:main Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_changes MUST include changes that break existing playbooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants