Skip to content

Conversation

FxKu
Copy link
Member

@FxKu FxKu commented Apr 14, 2021

The operator never deletes users (and databases) which where specified once in the cluster manifest or in the PostgresTeams CRD. With the latter it's very easy to add roles to a lot of clusters, but reversing such action manually is cumbersome. We could give some power to the operator to at least rename such "outdated" roles, so that logins with the old username will fail. Deleting the role and corresponding secret might be too dangerous due to depending database objects or deployments.

In this first approach of this PR the readPgUsersFromDatabase method is changed to not only retrieve given DB roles, but all roles that can login (or not but have a PW set like roles for preparedDatabases). In ProduceSyncRequests, DB roles that do not exists in the pgUser map would then trigger a rename request where a suffix is added.

Safer: Create a cache for pgUsers and add team roles to name list to pass to readPgUsersFromDatabase that do not exist in pgUsers anymore. Lowers the risk of renaming existing DB roles that are not (yet) in the manifest maybe due to cloning/migration etc. However, if the operator pod would crash the users cache is gone and we don't know if roles can be renamed.

A new field is added to pgUser struct to mark it for deprecation. The flow is as follows:

New Role

  1. New role will end up in pgUser map during initUsers phase
  2. Create user name array with pgUsers and role names with deprecation suffix (we do this every time just in case).
  3. New role is not found in DB and therefore not in dbUsers map.
  4. ProduceSyncRequests will trigger a new PGSyncUserAdd request because pgUsers != dbUsers.

Role is removed

  1. Removed role will not be added to pgUsers, but pgUserCache still knows it
  2. Create map with pgUsers and name+suffix (the removed role is still not covered here)
  3. Because there is a cached role that is not in the pgUser map it is also added to the DB query.
  4. The removed role is found and added to dbUsers. It doesn't have the suffix yet so the Deprecated field is still left to false
  5. In, ProduceSyncRequests an extra role in dbUsers map triggers a rename request
  6. The removed role is renamed and gets the suffix from the strategy struct

Role is added again

  1. New role will end up in pgUser map during initUsers phase
  2. Create map with pgUsers and name+suffix. The re-added role is not in the cache, but this is not checked.
  3. The new (old) role is not found and hence not in the dbUsers map
  4. The role with the deprecation suffix however is found. It's added to dbUsers with Deprecated field set to true.
  5. Check the counterpart (without the suffix) in the pgUsers map and set the Deprecated field to true as well. This is fine because this pgUser is not part of dbUsers. It will have no affect in the renaming later.
  6. In ProduceSyncRequests the user creation for the new (old) role is omitted because Deprecated is set to true.
  7. The extra dbUser triggers a rename request. But, because this user was found in the DB with the deprecation suffix the role will be renamed to the original role name.

@FxKu FxKu changed the title rename db roles that are removed from manifests rename db roles that are removed from PostgresTeam CRD Apr 16, 2021
@FxKu FxKu added this to the 1.6.3 milestone Apr 16, 2021
Copy link
Member

@sdudoladov sdudoladov left a comment

Choose a reason for hiding this comment

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

That is a visible change, so it has to be mentioned at least in the admin docs.

@sdudoladov
Copy link
Member

another interesting bur minor question is what happens if someone creates a role that is named exactly like RoleRenameSuffix, e.g, _delete_me

@Jan-M Jan-M changed the title rename db roles that are removed from PostgresTeam CRD Rename roles that are removed from PostgresTeam CRD Apr 22, 2021
@FxKu FxKu added the zalando label May 3, 2021
}

// No existing roles are deleted or stripped of role membership/flags
// but team roles will be renamed and denied from LOGIN
Copy link
Member

Choose a reason for hiding this comment

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

What would happen to roles coming from the Teams API if the API becomes unavailable ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I thought, there will be an error and the sync will stop, but no - it continues only with additional members. I return the errors now.

@Jan-M
Copy link
Member

Jan-M commented May 20, 2021

👍

@FxKu
Copy link
Member Author

FxKu commented May 20, 2021

I will add an option to enable/disable the feature.

@FxKu
Copy link
Member Author

FxKu commented May 21, 2021

👍

1 similar comment
@sdudoladov
Copy link
Member

👍

@sdudoladov sdudoladov merged commit eeb59c5 into master May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants