feat(HMS-1246): Use permission checks in statuser #504

avitova · 2023-04-19T13:16:49Z

This adds permission logging in case of missing permissions. Caching should also be done for this. I feel like the source result should not be flagged as Unavailable or with Error in case of missing permission, as this is neither of these. Therefore, I thought adding the MissingPermission parameter might be suitable, but let me know, what you think.
For testing the missing permission in statuser, you could change iam_client hardcoded struct.

lzap · 2023-04-20T13:15:01Z

I feel like the source result should not be flagged as Unavailable or with Error in case of missing permission, as this is neither of these.

That is correct, I think a warning in logs is enough. We should not see many of these, only old sources would experience this.

lzap · 2023-04-20T13:15:50Z

Test failure seems relevant (timeout)?

lzap

Looks great, there is a failure can you take a look?

lzap · 2023-04-20T13:18:47Z

cmd/pbstatuser/main.go

 			if err != nil {
 				sr.Status = kafka.StatusUnavailable
 				sr.Err = err
-				logger.Warn().Err(err).Msg("Could not get aws assumed client")
-				chSend <- sr
+				logger.Warn().Err(err).Msgf("Could not get aws assumed client %s", err)


Please do not send error messages to messages because of Sentry.

#488

lzap · 2023-04-20T13:25:31Z

cmd/pbstatuser/main.go

+			}
+			sr.MissingPermissions = permissions
+			if len(permissions) != 0 {
+				logger.Warn().Err(err).Msgf("No sufficient permissions: %s", permissions)


This does not seem correct. CheckPermissions either returns nil result and error, or something in the result. You reported the error above, here I would simply issue a warning with the list. Also can you add a field so we can easily find all records in Kibana, for example:

logger.Warn().Bool("missing_permission", true).Msgf("No sufficient permissions: %s", permissions)

lzap · 2023-04-20T13:25:40Z

cmd/pbstatuser/main.go

+			permissions, err := ec2Client.CheckPermission(ctx, &s.Authentication)
+			if err != nil {
+				sr.Err = err
+				logger.Warn().Err(err).Msgf("Could not check aws permissions %s", err)


internal/kafka/source_result.go

mshriver · 2023-05-16T02:36:03Z

/retest

akhil-jha · 2023-05-17T08:00:01Z

/retest

lzap · 2023-05-17T08:03:40Z

For the record, @avitova might not get to working on this PR. This is not high-priority so let’s keep it opened.

lzap · 2023-07-11T09:38:32Z

Let me know when this is ready for re-review.

avitova · 2023-07-12T07:21:21Z

@lzap The statuser shows the error during the permission check and the array of missing permissions. Ready for review:)

lzap

Small nitpicks, looks great. Please rebase tests are failing for some reason, they were broken I think.

cmd/pbackend/statuser.go

lzap · 2023-07-12T08:34:28Z

cmd/pbackend/statuser.go

+			}
+			sr.MissingPermissions = permissions
+			if permissions != nil {
+				logger.Warn().Bool("missing_permission", true).Msgf("No sufficient permissions: %s", permissions)


Perhaps "insufficient" is better wording.

Also you can use strings.Join to join string slice by comma, but this works too.

Finally, when CheckPermission returns nil it likely returns an error too, can you squash this into just a single warning message? You can stuff everything into one log record, me as someone who is debugging this issue would be interested in:

source id (already present in the context logger)

account number (could be added in the processMessage too as it is in the identity but perhaps out of scope of this PR)

error message (via Err function)

missing permissions

So basically this line contains it all, perhaps just delete the 167 and move this into the if statement above or something like that.

This is a very good point. The CheckPermission function returned only missing permissions when something was missing, and the error value was returned only when something else failed. We should return an error for missing permissions, so I changed it in this PR. I hope this now makes more sense.

cmd/pbackend/statuser.go

ezr-ondrej · 2023-07-13T09:00:54Z

The AWS account used in the pr_check doesn't seem to have permissions to check assigned policies

1:24PM DBG Listing attached role policies client=ec2 hostname=provisioning-backend-statuser-654d6c7dd-r4blm version=60e5
1:24PM TRC request failed with unretryable error https response error StatusCode: 403, RequestID: fe4b8847-33fe-48ee-aaf7-e6f9f05056db, api error AccessDenied: User: arn:aws:sts::093942615996:assumed-role/redhat-provisioning-arn-role/name is not authorized to perform: iam:ListAttachedRolePolicies on resource: role redhat-provisioning-arn-role because no identity-based policy allows the iam:ListAttachedRolePolicies action client=ec2 hostname=provisioning-backend-statuser-654d6c7dd-r4blm version=60e5
1:24PM WRN AWS permission check failed error="cannot list attached role policies: operation error IAM: ListAttachedRolePolicies, https response error StatusCode: 403, RequestID: fe4b8847-33fe-48ee-aaf7-e6f9f05056db, api error AccessDenied: User: arn:aws:sts::093942615996:assumed-role/redhat-provisioning-arn-role/name is not authorized to perform: iam:ListAttachedRolePolicies on resource: role redhat-provisioning-arn-role because no identity-based policy allows the iam:ListAttachedRolePolicies action" hostname=provisioning-backend-statuser-654d6c7dd-r4blm missing_permissions=true source_id=5 version=60e5
1:24PM TRC Sending 1 source availability status messages (tick) hostname=provisioning-backend-statuser-654d6c7dd-r4blm messages=1 version=60e5

Update: the policy of the AWS account had been updated and necessary permissions added, THANK YOU @akhil-jha 🧡

ezr-ondrej · 2023-07-13T09:52:54Z

/retest

ezr-ondrej · 2023-07-13T11:49:27Z

/retest

ezr-ondrej · 2023-07-13T13:19:00Z

/retest

ezr-ondrej · 2023-07-13T13:21:03Z

Different failure now xD that one definitelly does not sound related, but let see :)

ezr-ondrej · 2023-07-13T13:43:56Z

🎉 ... no idea what was the failure before, but now it works 😏

cmd/pbackend/statuser.go

ezr-ondrej

I'm good here, thanks! 🧡

lzap

One detail - the error length is unlimited and in case user actually forgot to add any permission this gets rendered to screen as something very long (covering whole screen or something).

In another PR, I suggest to limit the list to some reasonable length and in these cases add and more to the error.

avitova force-pushed the StatuserCheckPermissions branch 2 times, most recently from 1f8ce2e to 33a1f72 Compare April 19, 2023 13:36

lzap reviewed Apr 20, 2023

View reviewed changes

avitova force-pushed the StatuserCheckPermissions branch from 33a1f72 to 9ee7144 Compare July 11, 2023 08:36

lzap requested changes Jul 12, 2023

View reviewed changes

avitova force-pushed the StatuserCheckPermissions branch 2 times, most recently from 57034d4 to df277f2 Compare July 12, 2023 13:09

ezr-ondrej reviewed Jul 13, 2023

View reviewed changes

cmd/pbackend/statuser.go Show resolved Hide resolved

avitova force-pushed the StatuserCheckPermissions branch 3 times, most recently from 369487c to fa863a1 Compare July 18, 2023 08:17

feat(HMS-1246): Use permission checks in statuser

2b4ac15

avitova force-pushed the StatuserCheckPermissions branch from fa863a1 to 2b4ac15 Compare July 18, 2023 08:22

ezr-ondrej approved these changes Jul 18, 2023

View reviewed changes

lzap approved these changes Jul 19, 2023

View reviewed changes

lzap merged commit 9a23611 into RHEnVision:main Jul 19, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(HMS-1246): Use permission checks in statuser #504

feat(HMS-1246): Use permission checks in statuser #504

avitova commented Apr 19, 2023

lzap commented Apr 20, 2023

lzap commented Apr 20, 2023

lzap left a comment

lzap Apr 20, 2023

lzap Apr 20, 2023

lzap Apr 20, 2023

mshriver commented May 16, 2023

akhil-jha commented May 17, 2023

lzap commented May 17, 2023

lzap commented Jul 11, 2023

avitova commented Jul 12, 2023

lzap left a comment

lzap Jul 12, 2023

avitova Jul 12, 2023

ezr-ondrej commented Jul 13, 2023 •

edited

Loading

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej left a comment

lzap left a comment •

edited

Loading

feat(HMS-1246): Use permission checks in statuser #504

feat(HMS-1246): Use permission checks in statuser #504

Conversation

avitova commented Apr 19, 2023

lzap commented Apr 20, 2023

lzap commented Apr 20, 2023

lzap left a comment

Choose a reason for hiding this comment

lzap Apr 20, 2023

Choose a reason for hiding this comment

lzap Apr 20, 2023

Choose a reason for hiding this comment

lzap Apr 20, 2023

Choose a reason for hiding this comment

mshriver commented May 16, 2023

akhil-jha commented May 17, 2023

lzap commented May 17, 2023

lzap commented Jul 11, 2023

avitova commented Jul 12, 2023

lzap left a comment

Choose a reason for hiding this comment

lzap Jul 12, 2023

Choose a reason for hiding this comment

avitova Jul 12, 2023

Choose a reason for hiding this comment

ezr-ondrej commented Jul 13, 2023 • edited Loading

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej commented Jul 13, 2023

ezr-ondrej left a comment

Choose a reason for hiding this comment

lzap left a comment • edited Loading

Choose a reason for hiding this comment

ezr-ondrej commented Jul 13, 2023 •

edited

Loading

lzap left a comment •

edited

Loading