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

proxy-header enhancement #227

Merged
merged 6 commits into from
May 16, 2024

Conversation

hwo-wd
Copy link
Collaborator

@hwo-wd hwo-wd commented May 14, 2024

Deprecated proxy setting will only be emitted in keycloak.conf if keycloak_quarkus_proxy_headers is not set.

Fix #226

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

@guidograzioli guidograzioli added the minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix label May 15, 2024
@guidograzioli guidograzioli changed the title Fix #226 - minor proxy-header enhancement proxy-header enhancement May 15, 2024
@guidograzioli
Copy link
Member

There is another earlier problem: keycloak_quarkus_proxy_headers (https://github.com/ansible-middleware/keycloak/blob/main/roles/keycloak_quarkus/meta/argument_specs.yml#L329) is defined as default '', but the default is missing from defaults/main.yml, making it essentially a variable.

Can you take care of that here? (adding the "" default (preferred), or otherwise dropping it from arg_specs, and handling the undefined case)

@guidograzioli
Copy link
Member

guidograzioli commented May 16, 2024

I do not really understand why the idempotency issue happens here (and not in previous builds); I think we have to drop the changed_when: true from here (which i added about a week ago to have orange logs instead of red, and to silence the linter complaining about ignore_errors...)

- keycloak_quarkus_proxy_mode is defined
- keycloak_quarkus_proxy_headers is defined and keycloak_quarkus_proxy_headers | length == 0
- keycloak_quarkus_version.split('.') | first | int >= 24
changed_when: true
Copy link
Member

Choose a reason for hiding this comment

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

I propose, for the sake of idempotency in tests, to change this to:
changed_when: keycloak_quarkus_deprecation_warnings
keycloak_quarkus_deprecation_warnings is default true, but we set it to false in all molecule tests

wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so now I know that I know nothing; no fuck*** clue why it complains here https://github.com/world-direct/ansible-keycloak/blob/62cbaa35966f7edfeeeb4f76497a4b87de840f7e/roles/keycloak_quarkus/tasks/deprecations.yml#L46 but here it is fine: https://github.com/world-direct/ansible-keycloak/blob/62cbaa35966f7edfeeeb4f76497a4b87de840f7e/roles/keycloak_quarkus/tasks/deprecations.yml#L13

and I've tried plenty...

image

anyway, I ended up what I should have done from the beginning: following your advice, the new param is called keycloak_quarkus_show_deprecation_warnings, so let's be done with this PR

@hwo-wd hwo-wd force-pushed the feature/226 branch 3 times, most recently from e4c7996 to dd7f85f Compare May 16, 2024 12:30
@guidograzioli guidograzioli merged commit 0de0b65 into ansible-middleware:main May 16, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KC>22: Don't set (deprecated) proxy setting when proxy-headers are set
2 participants