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

Handle Prometheus OAuth Issue #831

Conversation

andrewballantyne
Copy link
Member

@andrewballantyne andrewballantyne commented Dec 1, 2022

Resolves: #832

Description

When the OAuth token expires, it triggers a 500 error from a prometheus call for DS Projects when it tries to get the fill-size of the PVCs that are created.

How Has This Been Tested?

I tested this by suffixing the Authorization header with extra characters -- thus mimicking an invalid token.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Copy link
Member

@DaoDaoNoCode DaoDaoNoCode left a comment

Choose a reason for hiding this comment

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

lgtm

if (errorMessage.includes('Unexpected token < in JSON')) {
reject({ code: 422, response: 'Unprocessable prometheus response' });
return;
}
fastify.log.error(`Unparsed Prometheus data. ${rawData}`);
reject({ code: 500, response: rawData });
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, do we ever really want to do a 500 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The server ran into an error that shouldn't happen. It's a 500 by definition, no?

Copy link
Member

Choose a reason for hiding this comment

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

well, I guess I was on the fence since in this case we were trying to look up something and failed to find it, perhaps we could have justified a 4XX. I guess 500 or 502 is probably more appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the reason could be anything... but I suppose it's likely 502 in this case as the target of the call is misbehaving so it likely is having issues.

Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 2, 2022

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: cfchase, DaoDaoNoCode, lucferbux

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 27a406f into opendatahub-io:main Dec 2, 2022
strangiato pushed a commit to strangiato/odh-dashboard that referenced this pull request Oct 18, 2023
@andrewballantyne andrewballantyne deleted the handle-prom-oauth-issue branch June 26, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: OAuth Proxy Error in DS Projects Prometheus Calls
6 participants