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

Add ability to merge users.txt file into bootstrap.yaml #265

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

andrewstucki
Copy link
Contributor

This adds a routine to merge users.txt into the bootstrap.yaml read when performing post install/update jobs. This is needed due to the bootstrap user additions added in this commit causing redpanda-data/helm-charts#1566.

What is essentially occuring is this:

  1. When creating a bootstrap user via the REDPANDA_BOOTSTRAP_USER environment variable said user isn't marked automatically as a superuser
  2. If you want to use something like admin_api_require_auth then the only way you can actually run management operations on your installation is to pass in the above user in to the nodes' bootstrap.yaml so that the user is immediately marked as a superuser and all of the config-watcher scripts that manage other specified users can leverage it to create the rest of the users.
  3. The bootstrap user changes that added the user to bootstrap.yaml uncovered that setting any sort of superusers values in the bootstrap.yaml is incompatible with the users created by a pre-existing users.txt secret. This is due to the superusers entry found in the bootstrap.yaml not containing them. When an upgrade finishes the jobs reset the configuration to only contain what is found in superusers without regard to anything managed by the pre-existing secret/config-watcher.
  4. The above manifested in all users from a pre-existing secret getting unmarked as superusers when an upgrade occurred. Restarting any StatefulSet pod clears this up.

Since we must still set the bootstrap user in the superusers section of bootstrap.yaml in order for admin_api_require_auth to function correctly on installations, this makes the config synchronization code aware of our users.txt.

It needs to be coupled with a change in the helm-charts code to add in a --users-txt flag as needed and an additional secrets mount to mount the users.txt file into our job containers.

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

It would be good to copy the explanation from PR cover letter to the commit message.

operator/cmd/syncclusterconfig/sync.go Outdated Show resolved Hide resolved
operator/cmd/syncclusterconfig/sync.go Show resolved Hide resolved
operator/cmd/syncclusterconfig/sync.go Outdated Show resolved Hide resolved
Copy link
Contributor

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

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

LGTM!

@andrewstucki andrewstucki merged commit deb4a85 into main Oct 16, 2024
5 checks passed
@andrewstucki andrewstucki deleted the sync-users-merge branch October 16, 2024 22:23
andrewstucki added a commit to redpanda-data/helm-charts that referenced this pull request Oct 18, 2024
RafalKorepta pushed a commit to RafalKorepta/redpanda-operator that referenced this pull request Nov 8, 2024
… (#1567)

This adds the `--users-directory` command line flag needed to leverage redpanda-data#265
RafalKorepta pushed a commit that referenced this pull request Dec 2, 2024
… (#1567)

This adds the `--users-directory` command line flag needed to leverage #265
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.

3 participants