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

Investigate why some shares did not go to failed state, but remained stuck or in-progress #932

Closed
anushka-singh opened this issue Dec 18, 2023 · 5 comments · Fixed by #933
Labels
effort: medium priority: high status: in-progress This issue has been picked and is being implemented
Milestone

Comments

@anushka-singh
Copy link
Contributor

anushka-singh commented Dec 18, 2023

Describe the bug

Investigate why the shares stayed in progress even if they actually must have failed because of missing permissions or any other reason.

How to Reproduce

I am able to replicate this in local dev env. 
I have created an error in grant_role_bucket_policy by doing this: y = 1/0 print("yyyyyyy", y)
Then, I caused another error in handle_share_failure alarm by doing this: x = 1/0 print(x)
At this point, the share remains stuck or SHARE_IN_PROGRESS.

Expected behavior

No response

Your project

No response

Screenshots

No response

OS

Mac

Python version

3.8

AWS data.all version

2.2

Additional context

There's a bug when you share 2 things.. but 1 fails and 1 succeeds.. then you end up with 1 being in_progress forever
Ideally it should mark the success one as share_succeeded and failed one as share_failed and overall status as Processed

But instead its doing :
mark the success one as share_succeeded and failed one as share_in_progress or share_approved and overall status as in progress

@anushka-singh
Copy link
Contributor Author

The "in-progress" problem exists in 2 places. Share approval and revoke approval too.

I have found a resolution for the Revoke in Progress Bug.
Looking into share in progress bug now.

@anushka-singh
Copy link
Contributor Author

Error in actual prod:

Sharing task failed due to: An error occurred (InvalidParameter) when calling the Publish operation: Invalid parameter: Subject

@anushka-singh
Copy link
Contributor Author

Figured out the error and solution for Share In Progress.

The error is occurring in the publish method of the sns client, and it is related to the "Subject" parameter being considered invalid. The maximum length for the subject is 100 characters for AWS SNS.

I checked a few failed shares and their Subject exceeded length of 100 chars.
We were constructing Subject like this:

subject = ( f'ALARM: DATAALL S3 Bucket {bucket.S3BucketName} {alarm_type} Failure Notification' )

Example share:

  • The Subject became: ALARM: DATAALL S3 Bucket xx-xxxx-xxxxx-xxxxxxxx-xxxxxxxxxxxx-xxxxx Sharing Failure Notification which is 106 chars.

When it exceeded the 100 char limit, the alarm creation failed share remained in STUCK state.

@anushka-singh
Copy link
Contributor Author

Resolution:

I have changed the Subject to be constructed like this:
subject = f'ALARM: DATAALL S3 Bucket {bucket.S3BucketName} Sharing Failure'[:100]

This will ensure any unnecessary words from Subject are removed and that we always guarantee a 100 character limit. The message body is further created properly and with a lot of details in case the Subject line gets truncated.
I will repeat this pattern for revoke shares too and for folder and table shares

@dlpzx
Copy link
Contributor

dlpzx commented Jan 3, 2024

The discussion continues in the mentioned PR. Thanks @anushka-singh for the investigation and fix!!

@dlpzx dlpzx added status: in-progress This issue has been picked and is being implemented priority: high effort: medium labels Jan 3, 2024
noah-paige added a commit that referenced this issue Jan 4, 2024
…ut remained stuck or in-progress (#933)

### Feature or Bugfix
- Bugfix

### Detail
- Investigate why the shares stayed in progress even if they actually
must have failed because of missing permissions or any other reason.
- Please look at the ISSUE linked to read more about the cause. 
 - Also includes some linting fixes. 

### Resolution:

I have changed the Subject to be constructed like this:
subject = f'ALARM: DATAALL S3 Bucket {bucket.S3BucketName} Sharing
Failure'[:100]

This will ensure any unnecessary words from Subject are removed and that
we always guarantee a 100 character limit. The message body is further
created properly and with a lot of details in case the Subject line gets
truncated.
I will repeat this pattern for revoke shares too and for folder and
table shares

### Relates
- #932

### Security
Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: dlpzx <71252798+dlpzx@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: jaidisido <jaidisido@gmail.com>
Co-authored-by: dlpzx <dlpzx@amazon.com>
Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com>
Co-authored-by: MK <manjula_kasturi@hotmail.com>
Co-authored-by: Manjula <manjula.kasturi@gmail.com>
Co-authored-by: Zilvinas Saltys <zilvinas.saltys@gmail.com>
Co-authored-by: Zilvinas Saltys <zilvinas.saltys@yahooinc.com>
Co-authored-by: Daniel Lorch <98748454+lorchda@users.noreply.github.com>
Co-authored-by: Anushka Singh <anushka.singh@yahooinc.com>
Co-authored-by: Tejas Rajopadhye <71188245+TejasRGitHub@users.noreply.github.com>
Co-authored-by: trajopadhye <tejas.rajopadhye@yahooinc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: medium priority: high status: in-progress This issue has been picked and is being implemented
Projects
None yet
3 participants