OLS-546: Use cluster id as user id #882

onmete · 2024-05-07T08:24:26Z

Description

Use cluster id as user id

Type of change

New feature

openshift-ci-robot · 2024-05-07T08:24:30Z

@onmete: This pull request references OLS-546 which is a valid jira issue.

In response to this:

Description

Use cluster id as user id

Type of change

New feature

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 openshift-eng/jira-lifecycle-plugin repository.

ols/utils/auth_dependency.py

tisnik · 2024-05-07T13:51:44Z

tests/unit/utils/test_auth_dependency.py

+ mocked_call = MagicMock()
+ mocked_call.get_cluster_custom_object.return_value = cluster_id
+ mock_get_custom_objects_api.return_value = mocked_call
+ assert get_cluster_id() == "some-cluster-id"


prob. use real UUID? will it be tested in the code btw?

Do you mean ensuring what we get from the cluster as id is really uuid? That seems like an overkill to me, but I can add it if you think it is reasonable.

codecov-commenter · 2024-05-07T17:10:55Z

Codecov Report

Attention: Patch coverage is 88.63636% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 95.24%. Comparing base (de85e97) to head (a7dbbea).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
- Coverage   95.34%   95.24%   -0.10%     
==========================================
  Files          54       54              
  Lines        2211     2252      +41     
==========================================
+ Hits         2108     2145      +37     
- Misses        103      107       +4

Files	Coverage Δ
ols/constants.py	`100.00% <100.00%> (ø)`
ols/utils/auth_dependency.py	`77.55% <88.09%> (+4.65%)`	⬆️

... and 4 files with indirect coverage changes

ols/utils/auth_dependency.py

onmete · 2024-05-09T07:49:08Z

@bparees @xrajesh The user id in the e2e CI is "system:serviceaccount:openshift-lightspeed:test-user". In order to have the feature in the PR covered by e2e test, I need to hit the /authorized endpoint as the "kube:admin". Would you know an easy way how to do it besides updating the the request model of that endpoint to allow post user (which I'm not sure if we want to do)? Another option would be to remove the e2e test.

xrajesh · 2024-05-09T19:50:31Z

@onmete The tests are using the test-user's token. here . You can make a API call as kubeadmin by getting the kubeadmin's token. I am thinking we are logging in as the kubeadmin. Hence kubeadmin_token=$(oc whoami -t) should give us the kubeadmin token. We will need a function just like def get_user_token(name: str) -> str: to run the oc whoami -t . If you make use of that token and make a OLS api call, it should get you "kube:admin"

bparees · 2024-05-14T17:45:39Z

ols/utils/auth_dependency.py

@@ -206,7 +238,7 @@ async def __call__(self, request: Request) -> tuple[str, str]:
 status_code=403, detail="Forbidden: Invalid or expired token"
 )
 if user_info.user.username == "kube:admin":
- user_info.user.uid = DEFAULT_KUBEADMIN_UID
+ user_info.user.uid = get_cluster_id()


the cluster id isn't going to change between user queries, so we should just fetch it once during OLS startup and then use that value every time we get a request from kubeadmin.

this will also help avoid anther issue you're not handling here, which is what happens if getting the cluster id fails? you log the errors in the helper, but ultimately return None to the caller.

if you fetch it during startup you can retry + refuse to start until we successfully get the cluster id.

it also removes the extra/repeated api calls.

bparees · 2024-05-14T17:49:10Z

tests/e2e/test_api.py

@@ -728,6 +728,29 @@ def test_forbidden_user():
 assert response.status_code == requests.codes.forbidden


+# TODO: disabled until we figure out how to get the token that returns
+# user kube:admin in the CI


we can at least have a legitimate test of the get_cluster_id helper, right? (not really an e2e test since you won't be going through the OLS server, but since we have a cluster stood up, you can test that the get_cluster_id helper gets the right value.

Yes, I was thinking about it too as a backup when we can't do e2e.

onmete · 2024-05-15T14:06:55Z

/retest

xrajesh · 2024-05-29T20:39:53Z

@onmete minor change proposed - otherwise - this LGTM.

xrajesh · 2024-05-31T02:40:31Z

/lgtm

tisnik · 2024-05-31T05:59:28Z

/approve

openshift-ci · 2024-05-31T05:59:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tisnik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tisnik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tisnik · 2024-05-31T06:48:10Z

/retest

openshift-ci-robot · 2024-05-31T08:10:05Z

/retest-required

Remaining retests: 0 against base HEAD 03e4fd5 and 2 for PR HEAD a089664 in total

onmete · 2024-05-31T08:46:31Z

Service hiccup - {'errors': [{'status': 500, 'detail': 'Could not get the SSO public key', 'meta': {'response_by': 'gateway'}}]}

/retest

onmete · 2024-06-03T07:12:56Z

/retest

tisnik · 2024-06-03T11:44:59Z

/lgtm

openshift-ci-robot · 2024-06-03T13:50:32Z

/retest-required

Remaining retests: 0 against base HEAD abde71b and 2 for PR HEAD 573addf in total

onmete · 2024-06-03T14:03:54Z

/retest

openshift-ci-robot · 2024-06-03T14:26:33Z

/retest-required

Remaining retests: 0 against base HEAD dc5d14f and 1 for PR HEAD 573addf in total

openshift-ci-robot · 2024-06-03T15:20:42Z

/retest-required

Remaining retests: 0 against base HEAD 272d13f and 0 for PR HEAD 573addf in total

onmete · 2024-06-03T15:42:43Z

/retest

tisnik · 2024-06-03T16:51:07Z

/lgtm

openshift-ci · 2024-06-03T18:20:16Z

@onmete: all tests passed!

Full PR test history. Your PR dashboard.

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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 7, 2024

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2024

openshift-ci bot requested review from bparees and tisnik May 7, 2024 08:24

onmete force-pushed the cluster-id-as-user-id branch from cc97e3b to 29b61dd Compare May 7, 2024 08:26

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2024

tisnik reviewed May 7, 2024

View reviewed changes

ols/utils/auth_dependency.py Outdated Show resolved Hide resolved

onmete force-pushed the cluster-id-as-user-id branch from 29b61dd to a38961b Compare May 7, 2024 13:34

tisnik reviewed May 7, 2024

View reviewed changes

xrajesh reviewed May 7, 2024

View reviewed changes

ols/utils/auth_dependency.py Outdated Show resolved Hide resolved

onmete force-pushed the cluster-id-as-user-id branch from 76d88b2 to c124563 Compare May 9, 2024 06:03

onmete force-pushed the cluster-id-as-user-id branch 2 times, most recently from 1dca8fc to 21c3045 Compare May 14, 2024 09:06

bparees reviewed May 14, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024

onmete force-pushed the cluster-id-as-user-id branch from 5d5bb47 to 445f994 Compare May 15, 2024 08:22

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2024

onmete force-pushed the cluster-id-as-user-id branch from 445f994 to b383fd8 Compare May 15, 2024 12:35

onmete force-pushed the cluster-id-as-user-id branch 6 times, most recently from c0114a4 to e6b1429 Compare May 21, 2024 13:25

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2024

onmete force-pushed the cluster-id-as-user-id branch from 271d4ca to a089664 Compare May 30, 2024 06:35

openshift-ci bot assigned xrajesh May 31, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2024

Use cluster id as user id when user is kube:admin

573addf

onmete force-pushed the cluster-id-as-user-id branch from a089664 to 573addf Compare June 3, 2024 11:09

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024

openshift-ci bot assigned tisnik Jun 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024

Increase wait for collector logs

a7dbbea

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2024

openshift-merge-bot bot merged commit 9dc0ed3 into openshift:main Jun 3, 2024
7 checks passed

benswinney mentioned this pull request Jun 4, 2024

Update ols-tls yaml in example for new CR perms #1023

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OLS-546: Use cluster id as user id #882

OLS-546: Use cluster id as user id #882

onmete commented May 7, 2024

openshift-ci-robot commented May 7, 2024 •

edited by openshift-ci bot

Description

Type of change

tisnik May 7, 2024

onmete May 7, 2024

codecov-commenter commented May 7, 2024 •

edited

onmete commented May 9, 2024

xrajesh commented May 9, 2024

bparees May 14, 2024

bparees May 14, 2024

onmete May 15, 2024

onmete commented May 15, 2024

xrajesh commented May 29, 2024

xrajesh commented May 31, 2024

tisnik commented May 31, 2024

openshift-ci bot commented May 31, 2024

tisnik commented May 31, 2024

openshift-ci-robot commented May 31, 2024

onmete commented May 31, 2024

onmete commented Jun 3, 2024

tisnik commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

onmete commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

onmete commented Jun 3, 2024

tisnik commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

OLS-546: Use cluster id as user id #882

OLS-546: Use cluster id as user id #882

Conversation

onmete commented May 7, 2024

Description

Type of change

openshift-ci-robot commented May 7, 2024 • edited by openshift-ci bot

Description

Type of change

tisnik May 7, 2024

Choose a reason for hiding this comment

onmete May 7, 2024

Choose a reason for hiding this comment

codecov-commenter commented May 7, 2024 • edited

Codecov Report

onmete commented May 9, 2024

xrajesh commented May 9, 2024

bparees May 14, 2024

Choose a reason for hiding this comment

bparees May 14, 2024

Choose a reason for hiding this comment

onmete May 15, 2024

Choose a reason for hiding this comment

onmete commented May 15, 2024

xrajesh commented May 29, 2024

xrajesh commented May 31, 2024

tisnik commented May 31, 2024

openshift-ci bot commented May 31, 2024

tisnik commented May 31, 2024

openshift-ci-robot commented May 31, 2024

onmete commented May 31, 2024

onmete commented Jun 3, 2024

tisnik commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

onmete commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

openshift-ci-robot commented Jun 3, 2024

onmete commented Jun 3, 2024

tisnik commented Jun 3, 2024

openshift-ci bot commented Jun 3, 2024

openshift-ci-robot commented May 7, 2024 •

edited by openshift-ci bot

codecov-commenter commented May 7, 2024 •

edited