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 eck security migration #114633

Closed
wants to merge 3 commits into from

Conversation

jfreden
Copy link
Contributor

@jfreden jfreden commented Oct 11, 2024

This PR adds a one-time cleanup task for role mapping duplicates both in cluster state and the .security index. The cleanup task is triggered once, the first time the settings.json is reloaded after this PR has been deployed. The task compares the names of the role mappings in the operator-defined settings.json and the names of the role mappings in the .security index and removes or disables (TBD which one) the one in the .security index. It also adds the name field to the serialization of the role mapping, since the name is needed for the GET Role Mappings API.

Issue

This cleanup is done because there was a bug (fixed here) that caused ECK customers to have the same role mappings with the same names present both in cluster state and the .security index. The effective role mapping is the combination of the .security index mapping and the cluster state one. After the bug fix, if the mapping in cluster state is modified (through setting.json), the .security index will still contain the old mapping and this could lead to unintended behaviour.

Open Questions

  • Before this is merged we need to make sure file based settings (settings.json) is re-loaded on every master node restart (draft PR )
  • Should we disable or delete roles. The REST API returns disabled roles (with enabled: false) which could be confusing since they're not active during role mapping in the auth/authz flow. Deleting them means users can't recover if the cleanup does something wrong.
  • We probably want to separate the mapping serialization change to a separate PR too. Open question is if this should be a new field or just new metadata?

Risks

  1. Role mappings might be intended to be defined both in the .security index and the cluster state and will be removed by this PR (minimal since before 8.15 it wasn't possible to this)
  2. Role mappings modified in the .security index to workaround the bug fixed here will be removed and all changes might not be reflected in the settings.json role mappings (Only reported by a few customers)
  3. This might not trigger for a very long time, increasing the risk of (1) and (2) (mitigated by coupling this with forcing a reload of file settings on master restart)

Dependencies

  • link to file setting reload on master restart PR

TODO

  • Add new serialization/deserialization to include mapping name
  • Add testing
  • Figure out snapshot restore flow with new format
  • Figure out mixed cluster behaviour

@jfreden jfreden closed this Oct 15, 2024
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.

2 participants