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.

Sign up for GitHub

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

Jump to bottom

introduced error cases to supress case_clause errors because of bifrost delayed response. #3408

Merged
merged 1 commit into from Aug 11, 2022
Merged

introduced error cases to supress case_clause errors because of bifrost delayed response. #3408

merged 1 commit into from Aug 11, 2022

Conversation

sreepuramsudheer
Copy link
Contributor

@sreepuramsudheer sreepuramsudheer commented Jul 13, 2022 •
edited by PrajaktaPurohit

Description

Code changes to solve issue raised #1896.

Issues Resolved

(#1896)

Check List

@sreepuramsudheer sreepuramsudheer requested review from a team as code owners July 13, 2022 14:26
@netlify
Copy link

netlify bot commented Jul 13, 2022 •
edited

✅ Deploy Preview for chef-server ready!

Name Link
🔨 Latest commit f42b51b
🔍 Latest deploy log https://app.netlify.com/sites/chef-server/deploys/62f506c06dd32b00087737ab
😎 Deploy Preview https://deploy-preview-3408--chef-server.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sreepuramsudheer
Copy link
Contributor Author

sreepuramsudheer commented Jul 14, 2022 •
edited

Adhoc build: https://buildkite.com/chef/chef-chef-server-main-omnibus-adhoc/builds/4812
Umbrella build: https://buildkite.com/chef/chef-umbrella-main-chef-server/builds/1444

@jashaik
Copy link
Contributor

jashaik commented Jul 18, 2022

LGTM. Nice work @sreepuramsudheer

@PrajaktaPurohit PrajaktaPurohit changed the title introduced error cases to supress case_clause errors because of bifro… introduced error cases to supress case_clause errors because of bifrost delayed response. Jul 18, 2022
@PrajaktaPurohit
Copy link
Contributor

Thanks for getting the additional error reporting started. Could you please look at the verify test currently failing?

marcparadise
marcparadise requested changes Jul 27, 2022
View reviewed changes
src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz.erl Outdated
@@ -328,7 +329,9 @@ is_authorized_on_resource(RequestorId, ResourceType, ResourceId, ActorType, Acto
%% This api returns not found for any missing permission
{error, not_found} -> false;
%% Otherwise, we expect server_error; not forbidden
{error, server_error} -> {error, server_error}
{error, server_error} -> {error, server_error};
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this to have any other error tuple returned as-is, eg {error, Any} -> {error, Any}; the shape of the tuple is more important to capture than each individual possible error type.

src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz.erl
@@ -123,6 +123,7 @@ create_entity_if_authorized(Context, OrgId, CreatorAId, ObjectType) ->
{error, forbidden};
Copy link
Member

Choose a reason for hiding this comment

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

This will still fail with a 500 in case of a non-forbidden error return, which I think is correct - but it does mean that we'll potentially lose logging/error info here. Let's create a new issue to confirm & fix that if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I think it would be useful to add the matcher for the {error, Error} case from

chef-server/src/oc_erchef/apps/oc_chef_authz/src/oc_chef_authz.erl

Line 332 in 170469a

{error, Error} -> {error, Error}

Copy link
Contributor

Choose a reason for hiding this comment

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

That error would also need to propogate all the way up through

chef-server/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_organizations.erl

Line 253 in 6d31841

case oc_chef_authz:create_entity_if_authorized(AuthzCtx, OrgId, superuser, Type) of
,

chef-server/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_associations.erl

Line 212 in 6d31841

Result = oc_chef_authz:create_entity_if_authorized(Context#context.authz_context,
,

chef-server/src/oc_erchef/apps/oc_chef_wm/src/oc_chef_wm_base.erl

Line 367 in afd0ec4

case oc_chef_authz:create_entity_if_authorized(AuthzContext, OrgId,
into the modules calling those functions.

@marcparadise
Copy link
Member

We can merge after rebase

@sreepuramsudheer
introduced error cases to supress case_clause errors because of bifro…
f42b51b 
…st delayed response.

Signed-off-by: sreepuramsudheer <ssudheer@progress.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 11, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sreepuramsudheer sreepuramsudheer merged commit 3aa0809 into main Aug 11, 2022
@sreepuramsudheer sreepuramsudheer deleted the ssudheer/INFS233 branch August 11, 2022 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PrajaktaPurohit PrajaktaPurohit PrajaktaPurohit left review comments

@marcparadise marcparadise marcparadise approved these changes

Assignees

@sreepuramsudheer sreepuramsudheer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sreepuramsudheer @jashaik @PrajaktaPurohit @marcparadise