feat(teamrolebindings): teamrbac supports single users #927

gciezkowski-acc · 2025-02-21T15:50:41Z

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Related Issue # (issue)
Closes [FEAT] - TeamRBAC to support single users #920
Fixes # (issue)

Added tests?

👍 yes
🙅 no, because they aren't needed
🙋 no, because I need help
Separate ticket for tests # (issue/pr)

Added to documentation?

📜 README.md
🤝 Documentation pages updated
🙅 no documentation needed
(if applicable) generated OpenAPI docs for CRD changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
New and existing unit tests pass locally with my changes

IvoGoman · 2025-02-24T14:47:13Z

pkg/apis/greenhouse/v1alpha1/teamrolebinding_types.go

@@ -17,13 +17,18 @@ type TeamRoleBindingSpec struct {
 	TeamRoleRef string `json:"teamRoleRef,omitempty"`
 	// TeamRef references a Greenhouse Team by name
 	TeamRef string `json:"teamRef,omitempty"`
+	// Usernames define list of users with team role


Suggested change

// Usernames define list of users with team role

// Usernames defines list of users to add to the (Cluster-)RoleBindings

pkg/apis/greenhouse/v1alpha1/teamrolebinding_types.go

pkg/controllers/teamrbac/teamrolebinding_controller.go

IvoGoman · 2025-02-24T15:04:38Z

pkg/controllers/teamrbac/teamrolebinding_controller.go

+	for _, nampespaceName := range teamRoleBinding.Spec.Namespaces {
+		namespace := new(corev1.Namespace)
+		err := r.Client.Get(ctx, types.NamespacedName{Name: nampespaceName}, namespace)
+		if err == nil {
+			continue
+		}
+		if apierrors.IsNotFound(err) {
+			err := r.Client.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: nampespaceName}})
+			if err != nil {
+				return err
+			}
+		} else {
+			return err
+		}
+	}
+
+	return nil


This creates the namespaces on the Greenhouse cluster. The namespaces should be created on the remote cluster.
It should return a not found error here, when trying to create a RoleBinding in a Namespace that does not exist. It should be fine to create the namespace if there is a NotFound error and then return the original error.
This way the controller will try again and now the namespace exists. This way there is no need for an additional retry logic.

IvoGoman · 2025-02-24T15:05:16Z

pkg/controllers/teamrbac/teamrolebinding_controller_test.go

+			By("checking that the Namespace is created")
+			namespace := &corev1.Namespace{}
+			Eventually(func(g Gomega) {
+				err := test.K8sClient.Get(test.Ctx, types.NamespacedName{Name: "non-existing-namespace"}, namespace)


The used client is for the greenhouse cluster, this test should verify that the namespace was created on the remote cluster.

IvoGoman · 2025-02-24T15:12:17Z

pkg/controllers/teamrbac/teamrolebinding_controller.go

 	}
 }

+func joinUsernamesWithDefault(usernames []string, mappedIDPGroup string) []rbacv1.Subject {


Suggested change

func joinUsernamesWithDefault(usernames []string, mappedIDPGroup string) []rbacv1.Subject {

// generateSubjects returns a list of subjects with mappedIDPGroup as a rbacv1.GroupKind, and any usernames as rbacv1.UserKind

func generateSubjects(usernames []string, mappedIDPGroup string) []rbacv1.Subject {

IvoGoman · 2025-02-26T15:44:49Z

pkg/controllers/teamrbac/teamrolebinding_controller.go


+	err = createNamespaces(ctx, trb, cl)


The reconcileClusterRoleBinding is called only when there are no namespaces set on the ClusterRoleBindings. The ClusterRoleBindings that are created by this method are cluster scoped resources. It will never be the case that the namespace of the TeamRoleBinding will be filled.

This logic will also not work, as the namespace needs to be created before the rolebinding can be created.
It would require something like

if err != nil{ if apierrors.IsNotFound(err){ // create the namespace for the RoleBinding that is currently reconciled } return err }

IvoGoman · 2025-02-28T10:52:59Z

pkg/controllers/teamrbac/teamrolebinding_controller.go

+			err = createNamespaces(ctx, trb, remoteRestClient)
+			if err != nil {
+				trb.SetPropagationStatus(cluster.GetName(), metav1.ConditionFalse, greenhousev1alpha1.CreateNamespacesFailed, "Failed to create namespaces: "+err.Error())
+				continue
+			}


This will try to create the namespaces on every reconciliation of the TeamRoleBinding. This will result in at least one get request per namespace.
Please move the creation of the individual namespaces into this part of the creation of the rolebinding on the remote cluster

greenhouse/pkg/controllers/teamrbac/teamrolebinding_controller.go

Lines 568 to 570 in 2827d24

if err != nil {

return err

}

The create of a RoleBinding will fail with an NotFound error if the namespace does not exists.
That error can be caught as previously suggested with apierrors.IsNotFound(..).

In case it is such an error the namespace can be created. It should still return an error so that the TeamRoleBinding is reconciled once more. In the second reconciliation the create should succeed since the namespace was created.

something like

if err != nil{ if apierrors.IsNotFound(err){ // create the namespace for the RoleBinding that is currently reconciled } return err }

…920)

github-actions bot added feature core-apis helm-charts documentation Improvements or additions to documentation labels Feb 21, 2025

gciezkowski-acc changed the title ~~Feat/920 teamrbac supports single users~~ feat(teamrolebindings): teamrbac supports single users Feb 21, 2025

gciezkowski-acc force-pushed the feat/920_teamrbac_supports_single_users branch from 00cc0ac to d24741b Compare February 21, 2025 16:06

gciezkowski-acc marked this pull request as ready for review February 24, 2025 11:55

gciezkowski-acc requested a review from a team as a code owner February 24, 2025 11:55

gciezkowski-acc requested review from IvoGoman, uwe-mayer and abhijith-darshan February 24, 2025 11:56

IvoGoman requested changes Feb 24, 2025

View reviewed changes

gciezkowski-acc force-pushed the feat/920_teamrbac_supports_single_users branch from c0b8307 to 5018f56 Compare February 25, 2025 09:08

gciezkowski-acc requested a review from IvoGoman February 25, 2025 13:09

gciezkowski-acc force-pushed the feat/920_teamrbac_supports_single_users branch 3 times, most recently from 34ddcfb to 80657c3 Compare February 26, 2025 12:17

IvoGoman requested changes Feb 26, 2025

View reviewed changes

gciezkowski-acc force-pushed the feat/920_teamrbac_supports_single_users branch 2 times, most recently from 1d9011b to 16e5449 Compare February 27, 2025 11:15

gciezkowski-acc requested a review from IvoGoman February 27, 2025 11:47

IvoGoman requested changes Feb 28, 2025

View reviewed changes

gciezkowski-acc and others added 8 commits February 28, 2025 12:27

feat(teamrbac) Create namespace flag (#920)

cc162af

feat(teamrolebindings) Add team role bindings subjects (#920)

fd07e13

Automatic generation of CRD API Docs

c3e8a48

feat(teamrbac) Usernames instead of subjects (#920)

d05ba67

Automatic generation of CRD API Docs

308b310

feat(teamrbac) Make namespaces on remote cluster (#920)

54f8169

feat(teamrbac) Adjust the comments (#920)

44de052

Automatic generation of CRD API Docs

039ab5d

feat(teamrbac) Create namespace with rolebinging reconcile (#920)

5a25362

gciezkowski-acc force-pushed the feat/920_teamrbac_supports_single_users branch from 16e5449 to 5a25362 Compare February 28, 2025 11:30

feat(teamrbac) Move creating namespace to reconcileRoleBinding func (#…

e292f44

…920)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(teamrolebindings): teamrbac supports single users #927

feat(teamrolebindings): teamrbac supports single users #927

gciezkowski-acc commented Feb 21, 2025 •

edited

Loading

IvoGoman Feb 24, 2025

gciezkowski-acc Feb 25, 2025

IvoGoman Feb 24, 2025

gciezkowski-acc Feb 25, 2025

IvoGoman Feb 24, 2025

gciezkowski-acc Feb 25, 2025

IvoGoman Feb 24, 2025

gciezkowski-acc Feb 25, 2025

IvoGoman Feb 26, 2025

IvoGoman Feb 26, 2025

IvoGoman Feb 28, 2025

	// Usernames define list of users with team role
	// Usernames defines list of users to add to the (Cluster-)RoleBindings

	func joinUsernamesWithDefault(usernames []string, mappedIDPGroup string) []rbacv1.Subject {
	// generateSubjects returns a list of subjects with mappedIDPGroup as a rbacv1.GroupKind, and any usernames as rbacv1.UserKind
	func generateSubjects(usernames []string, mappedIDPGroup string) []rbacv1.Subject {

feat(teamrolebindings): teamrbac supports single users #927

Are you sure you want to change the base?

feat(teamrolebindings): teamrbac supports single users #927

Conversation

gciezkowski-acc commented Feb 21, 2025 • edited Loading

Description

What type of PR is this? (check all applicable)

Related Tickets & Documents

Added tests?

Added to documentation?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gciezkowski-acc commented Feb 21, 2025 •

edited

Loading