Skip to content

Conversation

@jkupferer
Copy link
Contributor

  • Add named users to ocp4_workload_authentication_keycloak similar to ocp4_workload_authentication_htpasswd
  • Add ocp4_workload_authentication_keycloak_user_password_randomized feature to allow each user to have a separate password.
  • Fix idempotency issue where rerunning workload would reset the oauth client secret and postgresql password.
  • Fix minor bug in ocp4_workload_authentication_htpasswd

- Add named users to ocp4_workload_authentication_keycloak similar to
  ocp4_workload_authentication_htpasswd
- Add ocp4_workload_authentication_keycloak_user_password_randomized
  feature to allow each user to have a separate password.
- Fix idempotency issue where rerunning workload would reset the oauth
  client secret and postgresql password.
ocp4_workload_authentication_keycloak_admin_password: ""

# Length of password if generated for admin user.
ocp4_workload_authentication_keycloak_admin_password_length: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to 16 to make it consistent with the htpasswd role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous default was 10... imho, 16 is excessive and just makes it hard for users that may need to type the password out. I think to make it consistent we should update the htpasswd workload to also be 10.

ocp4_workload_authentication_keycloak_user_password: ""

# Length of password generated for user.
ocp4_workload_authentication_keycloak_user_password_length: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, this is the one that should be changed to 16 to be consistent with htpasswd role. But we may as well make both 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous default was 10... imho, 16 is excessive and just makes it hard for users that may need to type the password out. I think to make it consistent we should update the htpasswd workload to also be 10.

- name: Set common password for named users
when:
- not ocp4_workload_authentication_keycloak_named_user_password_randomized | bool
- _ocp4_workload_authentication_keycloak_named_user_password == ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I reading this wrong? We want to set the common password for named users when it is defined as an empty string? Shouldn't the second conditional be preceded by a not also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an explicit value is passed for ocp4_workload_authentication_keycloak_named_user_password then this overrides the setting in ocp4_workload_authentication_keycloak_named_user_password_randomized. It's implied that if the user passes a common password then they don't want it randomized.

ocp4_workload_authentication_keycloak_named_user_password sets _ocp4_workload_authentication_keycloak_named_user_password by vars. The reason for the extra level is that ocp4_workload_authentication_keycloak_named_user_password is set as an extra var and cannot be overridden with a set_fact.

We set _ocp4_workload_authentication_keycloak_named_user_password to a random value if and explicit value was not set in ocp4_workload_authentication_keycloak_named_user_password or an explicit empty string is passed. For example, if a parameter to specify the password was left blank and if the randomized behavior was not configured.

Hopefully this clears this up?

@stencell
Copy link
Contributor

@jkupferer I left a couple of comments, but before I continue and before this is merged...can you update this to be consistent with the htpasswd role you just updated? I don't see why any of the logic needs to be different at all. Aside from the actual Keycloak setup and command to create users, everything else should be identical to the htpasswd role. It will make it easier for people to understand and maintain. It would not be a lot to change, but for example:

new keycloak role: https://github.com/agnosticd/core_workloads/blob/jkupferer-add-named-users-ocp4_workload_authentication_keycloak/roles/ocp4_workload_authentication_keycloak/tasks/workload.yml#L17-L28

existing htpasswd role: https://github.com/agnosticd/core_workloads/blob/main/roles/ocp4_workload_authentication_htpasswd/tasks/setup_htpasswd.yml#L2-L13

And a smaller nit would be, if this is being overhauled now, maybe it is a good idea to break out the tasks into separate files so that everything isn't living in workload.yaml. Similar to how htpasswd is broken up for the setup. Not required, just nice to have.

@jkupferer
Copy link
Contributor Author

@stencell I can update the htpasswd workload to match the patterns here. This being my second time over the logic I think I improved a few things in keycloak. Should I include that in this same PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants