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

fix: retry request for '"system:anonymous" cannot get resource' error #955

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

dkwon17
Copy link
Contributor

@dkwon17 dkwon17 commented Oct 16, 2023

What does this PR do?

Extension of #946 to cover the case where a 403 error happens for the provision request:
image

image

What issues does this PR fix or reference?

eclipse-che/che#22352

Is it tested? How?

The issue this PR aims to fix is difficult to reproduce. To test this PR, you can apply this patch:

diff --git a/packages/dashboard-backend/src/routes/api/helpers/getToken.ts b/packages/dashboard-backend/src/routes/api/helpers/getToken.ts
index 59e8d76f..1d5fcbf8 100644
--- a/packages/dashboard-backend/src/routes/api/helpers/getToken.ts
+++ b/packages/dashboard-backend/src/routes/api/helpers/getToken.ts
@@ -17,9 +17,5 @@ import { createFastifyError } from '@/services/helpers';
 const authorizationBearerPrefix = /^Bearer /;
 
 export function getToken(request: FastifyRequest): string {
-  const authorization = request.headers?.authorization;
-  if (!authorization || !authorizationBearerPrefix.test(authorization)) {
-    throw createFastifyError('FST_UNAUTHORIZED', 'Bearer Token Authorization is required', 401);
-  }
-  return authorization.replace(authorizationBearerPrefix, '').trim();
+  throw createFastifyError('FST_UNAUTHORIZED', 'users.user.openshift.io "~" is forbidden: User "system:anonymous" cannot get resource "users" in API group "user.openshift.io" at the cluster scope', 401);
 }

build the dashboard image, and verify that there are console warning messages notifying of request retries:
retryverify

Release Notes

Docs PR

@dkwon17 dkwon17 requested review from tolusha and akurinnoy and removed request for ibuziuk, olexii4 and akurinnoy October 16, 2023 22:28
@che-bot
Copy link
Contributor

che-bot commented Oct 16, 2023

Click here to review and test in web IDE: Contribute

@dkwon17 dkwon17 requested review from olexii4 and ibuziuk October 16, 2023 22:29
@dkwon17
Copy link
Contributor Author

dkwon17 commented Oct 16, 2023

Hello @olexii4 @akurinnoy I've noticed that the dashboard already sends a second provision request if the first one has failed:

https://user-images.githubusercontent.com/83611742/275662573-c4a2df69-dd18-481f-9da9-829a289776ad.png

Is this configurable somewhere?

@akurinnoy
Copy link
Contributor

akurinnoy commented Oct 17, 2023

@dkwon17 The dashboard is not retrying this particular request. The request was probably done from another component as a sanity check before requesting some data.

updated.
However, this behavior can be improved.

Signed-off-by: David Kwon <dakwon@redhat.com>
@@ -24,7 +24,9 @@ export async function getKubernetesNamespace(): Promise<che.KubernetesNamespace[
}

export async function provisionKubernetesNamespace(): Promise<che.KubernetesNamespace> {
const response = await axios.post(`${cheServerPrefix}/kubernetes/namespace/provision`);
const response = await AxiosWrapper.createToRetryCannotGetResourceErrors().post(
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can use createToRetryAnyErrors and don't care about error message which we can't guarantee will be same all time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I have updated the PR

Signed-off-by: David Kwon <dakwon@redhat.com>
@@ -24,7 +24,9 @@ export async function getKubernetesNamespace(): Promise<che.KubernetesNamespace[
}

export async function provisionKubernetesNamespace(): Promise<che.KubernetesNamespace> {
const response = await axios.post(`${cheServerPrefix}/kubernetes/namespace/provision`);
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an unused axios import at the top of the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this unsued import will fix the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I've made a new commit to remove it

Signed-off-by: David Kwon <dakwon@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm label Oct 18, 2023
@ibuziuk
Copy link
Member

ibuziuk commented Oct 19, 2023

@dkwon17 once merged, could you please make sure that the fix is backported for 3.10.x

@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dkwon17, ibuziuk, tolusha

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

Needs approval from an approver in each of these files:

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

Signed-off-by: David Kwon <dakwon@redhat.com>
@openshift-ci
Copy link

openshift-ci bot commented Oct 19, 2023

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm label Oct 19, 2023
@github-actions
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-955

@dkwon17 dkwon17 merged commit 5f0a611 into main Oct 19, 2023
11 checks passed
@dkwon17 dkwon17 deleted the cannotgetresource branch October 19, 2023 19:50
@devstudio-release
Copy link

Build 3.10 :: dashboard_3.x/368: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: get-sources-rhpkg-container-build_3.x/4825: FAILURE

dashboard : 3.x :: Failed in : quay.io/devspaces/dashboard-rhel8:3.10-39
FAILURE:; copied to quay

dkwon17 added a commit that referenced this pull request Oct 20, 2023
Signed-off-by: David Kwon <dakwon@redhat.com>
@devstudio-release
Copy link

Build 3.10 :: dashboard_3.x/369: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: get-sources-rhpkg-container-build_3.x/4851: FAILURE

dashboard : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN
FAILURE:; copied to quay

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.

6 participants