ROX-27053: Fix subjects handling for user and group kind by mtodor · Pull Request #13410 · stackrox/stackrox

mtodor · 2024-11-22T15:55:54Z

Description

We have failing tests on GKE. The difference comes from non-namespaced subjects.

With GKE - Kubernetes versionv1.31.2-gke.1384000, we have two role bindings. One is normal (namespace) role binding, and the other is cluster (non-namespace) role binding.

apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: konnectivity-server-leases
  namespace: kube-system
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: leases-writer
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: system:konnectivity-server
  namespace: kube-system

and

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: system:konnectivity-server
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: system:auth-delegator
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: system:konnectivity-server

The problem that we are facing is that subjects of kind User and Group are non-namespace subjects and they should not have set namespace property. They are actually the same subject, but ACS considers it as two separate subjects.

Kubernetes documentation: https://kubernetes.io/docs/reference/kubernetes-api/authorization-resources/role-binding-v1/

subjects.namespace (string)

Namespace of the referenced object. If the object kind is non-namespace, such as "User" or "Group", and this value is not empty the Authorizer should report an error.

This PR introduces a helper function that will adjust subjects for role binding and remove the namespace for non-namespaced subjects.

The test is also adjusted because it uses the namespace property during comparison.

Additional information is provided in Jira.

User-facing documentation

CHANGELOG is updated OR update is not needed
documentation PR is created and is linked above OR is not needed

Testing and quality

the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
CI results are inspected

Automated testing

added new unit tests
modified existing tests

How I validated my change

I have added unit tests for service and worked on implementation after that.
After I had finished implementation, I moved tests from service to function unit tests because they fit better there and are easier to understand.

Use the latest GKE version

/pragma: gke_cluster_version:latest

rukletsov · 2024-11-22T16:09:42Z

It's unclear to me why we filter only unique subjects from the orchestrator response (we don't seem to do that in the RBAC service). Hard to say what use cases the test was intended to cover, but for symmetry, what'd you say about filtering RBAC service's response for unique names as well?

rhacs-bot · 2024-11-22T16:21:57Z

Images are ready for the commit at d62f2c1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.7.x-223-gd62f2c1ff0.

mtodor · 2024-11-22T16:22:56Z

@rukletsov I would rather do the opposite - remove filtering from orchestratorSubjects. But I don't know exactly how we store role binding -> subject relations. Is there some deduping, etc.

rukletsov · 2024-11-22T16:36:00Z

@rukletsov I would rather do the opposite - remove filtering from orchestratorSubjects. But I don't know exactly how we store role binding -> subject relations. Is there some deduping, etc.

Yeah, that would be even better if want to invest a bit more here rather than keeping this test shallow.

Looking at the response proto, I think we can try to dedupe on <kind, name, namespace> for both orchestratorSubjects and RBAC service response.

mtodor · 2024-11-22T16:59:18Z

/test gke-latest-qa-e2e-tests

codecov · 2024-11-22T16:59:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.53%. Comparing base (bbbfb54) to head (5bc3ef8).
Report is 8 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #13410   +/-   ##
=======================================
  Coverage   48.53%   48.53%           
=======================================
  Files        2473     2473           
  Lines      178677   178677           
=======================================
+ Hits        86712    86713    +1     
  Misses      85016    85016           
+ Partials     6949     6948    -1

Flag	Coverage Δ
go-unit-tests	`48.53% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

mtodor · 2024-11-25T08:30:58Z

/test gke-qa-e2e-tests

vikin91 · 2024-11-26T11:47:41Z

@mtodor please ping me when this is ready for review again.

mtodor · 2024-11-27T15:53:42Z

/test gke-qa-e2e-tests

mtodor · 2024-11-27T15:54:02Z

/test gke-qa-e2e-tests

mtodor · 2024-11-29T13:27:19Z

/test gke-qa-e2e-tests

mtodor · 2024-12-02T12:13:34Z

/test gke-qa-e2e-tests

mtodor · 2024-12-02T17:13:39Z

/retest

pkg/k8srbac/subjects_test.go

central/rbac/service/service_impl.go

vikin91

It is not my corner of expertise, but I read into the description, the code, and the tests and the change makes total sense for me 👍

pkg/k8srbac/subjects_test.go

openshift-ci · 2024-12-04T11:44:08Z

@mtodor: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/gke-upgrade-tests	`d62f2c1`	link	false	`/test gke-upgrade-tests`
ci/prow/ocp-4-12-operator-e2e-tests	`d62f2c1`	link	false	`/test ocp-4-12-operator-e2e-tests`
ci/prow/ocp-4-17-nongroovy-e2e-tests	`d62f2c1`	link	false	`/test ocp-4-17-nongroovy-e2e-tests`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

(cherry picked from commit 367a7ff)

mtodor requested review from lvalerom and vikin91 November 22, 2024 15:56

stackrox deleted a comment from openshift-ci bot Nov 22, 2024

mtodor marked this pull request as draft November 25, 2024 13:19

openshift-ci bot added the do-not-merge/work-in-progress label Nov 25, 2024

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from 5bc3ef8 to b431960 Compare November 27, 2024 15:43

github-actions bot added the area/central label Nov 27, 2024

mtodor changed the title ~~ROX-27053: Fix role binding filtering for test~~ ROX-27053: Fix subjects handling for user and group kind Nov 27, 2024

mtodor closed this Nov 27, 2024

mtodor reopened this Nov 27, 2024

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from b431960 to 21038d4 Compare November 29, 2024 13:16

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from 21038d4 to d906476 Compare December 2, 2024 11:52

mtodor marked this pull request as ready for review December 2, 2024 12:13

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 2, 2024

mtodor requested a review from a team December 2, 2024 12:13

rhybrillou approved these changes Dec 3, 2024

View reviewed changes

pkg/k8srbac/subjects_test.go Show resolved Hide resolved

central/rbac/service/service_impl.go Outdated Show resolved Hide resolved

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from d906476 to c051cc4 Compare December 3, 2024 15:09

Improve handling of non-namespaces subjects

d645526

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from c051cc4 to 8079b7e Compare December 3, 2024 15:10

vikin91 approved these changes Dec 3, 2024

View reviewed changes

pkg/k8srbac/subjects_test.go Show resolved Hide resolved

Add unit tests for pipeline

d62f2c1

mtodor force-pushed the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch from 8079b7e to d62f2c1 Compare December 4, 2024 09:17

mtodor merged commit 367a7ff into master Dec 4, 2024

mtodor deleted the mtodor/ROX-27053-fix-role-binding-filtering-in-test branch December 4, 2024 12:21

davdhacs added the backport release-4.6 Create a PR to backport this PR to release-4.6 label Feb 19, 2025

rhacs-bot pushed a commit that referenced this pull request Feb 19, 2025

ROX-27053: Fix subjects handling for user and group kind (#13410)

8251971

(cherry picked from commit 367a7ff)

rhacs-bot mentioned this pull request Feb 19, 2025

ROX-27053: Fix subjects handling for user and group kind [backport] #14348

Merged

janisz added this to the 4.5.7-rc.1 milestone Feb 26, 2025

janisz added the backport release-4.5 label Feb 26, 2025

rhacs-bot pushed a commit that referenced this pull request Feb 26, 2025

ROX-27053: Fix subjects handling for user and group kind (#13410)

12e4b2a

(cherry picked from commit 367a7ff)

rhacs-bot mentioned this pull request Feb 26, 2025

ROX-27053: Fix subjects handling for user and group kind [backport] #14414

Merged

janisz removed this from the 4.5.7-rc.1 milestone Feb 26, 2025

Conversation

mtodor commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

User-facing documentation

Testing and quality

Automated testing

How I validated my change

Use the latest GKE version

Uh oh!

rukletsov commented Nov 22, 2024

Uh oh!

rhacs-bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtodor commented Nov 22, 2024

Uh oh!

rukletsov commented Nov 22, 2024

Uh oh!

mtodor commented Nov 22, 2024

Uh oh!

codecov bot commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mtodor commented Nov 25, 2024

Uh oh!

vikin91 commented Nov 26, 2024

Uh oh!

mtodor commented Nov 27, 2024

Uh oh!

mtodor commented Nov 27, 2024

Uh oh!

mtodor commented Nov 29, 2024

Uh oh!

mtodor commented Dec 2, 2024

Uh oh!

mtodor commented Dec 2, 2024

Uh oh!

Uh oh!

Uh oh!

vikin91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openshift-ci bot commented Dec 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mtodor commented Nov 22, 2024 •

edited

Loading

rhacs-bot commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading