Skip to content

Conversation

@lsierant
Copy link
Contributor

@lsierant lsierant commented Oct 25, 2025

Summary

This PR fixes a problem which was preventing creating cluster-wide mongodb custom roles.

Context
As described in MongoDB docs the resource object is either:

{ db: <database>, collection: <collection> }

or

{ cluster : true }

So that means we cannot provide empty strings for db and collection fields as empty string means "any db/collection".

At the same time our previous serialization rules were always sending empty strings even if not set. This was the source of the problem - it wasn't possible to specify cluster: true because the operator was also sending empty db and collections.

Backwards compatibility
Making db and collection fields just as omitempty *string is not sufficient, because that would change the semantics of the resource and would be potentially a breaking change. In order to preserve backwards compatibility we need additional logic (see normalizePrivilegeResource that will maintain the same behavior for non-cluster-wide resources (sending empty strings even if the field is not set in yaml).

Proof of Work

Tests passing.

Based on PR #551

Chain of upstream PRs as of 2025-10-25

@lsierant lsierant self-assigned this Oct 25, 2025
@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch from 43c0502 to 2d7b9a0 Compare October 27, 2025 14:46
@lsierant lsierant force-pushed the lsierant/custom-roles branch 2 times, most recently from cce9040 to 91233d8 Compare October 27, 2025 14:51
@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch from 2d7b9a0 to 3040210 Compare October 27, 2025 14:53
@lsierant lsierant force-pushed the lsierant/custom-roles branch from 91233d8 to d9c39b3 Compare October 27, 2025 15:45
@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch 2 times, most recently from 70c14e8 to c150032 Compare October 27, 2025 15:49
@lsierant lsierant force-pushed the lsierant/custom-roles branch from d9c39b3 to 0492502 Compare October 27, 2025 15:52
@lsierant lsierant marked this pull request as ready for review October 27, 2025 16:48
@lsierant lsierant requested review from a team and vinilage as code owners October 27, 2025 16:48
@lsierant lsierant requested review from anandsyncs, mircea-cosbuc and nammn and removed request for a team October 27, 2025 16:48
@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch from c150032 to 545dc80 Compare October 29, 2025 12:40
@lsierant lsierant force-pushed the lsierant/custom-roles branch from d3381a7 to 8fa7a1a Compare October 29, 2025 12:41
date: 2025-10-27
---

* Fixed a problem that prevented specifying cluster-wide privileges in database custom roles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I propose to change to: "Fixed inability to specify cluster-wide privileges in custom roles."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks!

@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch from 545dc80 to 71cd46b Compare October 31, 2025 09:28
@lsierant lsierant force-pushed the lsierant/custom-roles branch from 4c3939d to f515880 Compare October 31, 2025 09:28
@lsierant lsierant requested a review from vinilage October 31, 2025 09:30
Copy link
Collaborator

@vinilage vinilage left a comment

Choose a reason for hiding this comment

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

LGMT!

Comment on lines +1 to +6
---
kind: fix
date: 2025-10-27
---

* Fixed inability to specify cluster-wide privileges in custom roles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@mircea-cosbuc mircea-cosbuc left a comment

Choose a reason for hiding this comment

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

LGTM!

@lsierant lsierant force-pushed the lsierant/custom-roles-regression-tests branch from 71cd46b to 724fa83 Compare November 3, 2025 08:39
@lsierant lsierant force-pushed the lsierant/custom-roles branch from f515880 to 3392469 Compare November 3, 2025 08:39
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.

5 participants