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

Newer versions require IAM access to athena:GetWorkGroup #262

Closed
brabster opened this issue Apr 27, 2023 · 7 comments
Closed

Newer versions require IAM access to athena:GetWorkGroup #262

brabster opened this issue Apr 27, 2023 · 7 comments

Comments

@brabster
Copy link
Contributor

Recent change needs athena:GetWorkGroup permission in order to check if output location enforced, with presence of workgroup in config triggering the check. See here

If this permission is not granted, an AccessDeniedException is produced.

This means that eg. seeding a table fails due to this call in macro

Earlier versions didn't need this permission, and it seems safe to catch access denied and proceed if the permission is not granted, allowing the operation to fail if the output location is enforced?

@nicor88
Copy link
Member

nicor88 commented Apr 27, 2023

I'm not in favor of catching such permission error.
IMHO is better to maybe document a bit more the minum permissions needed to work with this adapter.
Also, athena:GetWorkGroup is just a read permission.

The point is: if we start to catch all those permissions errors for all the corner cases that we have we endup with a not sustainable code base!

@brabster
Copy link
Contributor Author

brabster commented Apr 27, 2023

Fair enough - this was just a breaking change for us when we tried to update. Can I ask, is is possible to set a seed as an iceberg table, without needing permissions for external locations or to upload the source data to S3? (Working in a very constrained env here, sorry the Q is a bit off topic!)

@nicor88
Copy link
Member

nicor88 commented Apr 27, 2023

@brabster you raised a really good point, there was a breaking change also for other reasons, we didn't foresee that unfortunately, otherwise, we will have highlighted it in the release notes.

Regarding seeds as iceberg: nice feature request, they are not supported currently.
As it is now you can just seed the table as parquet, then maybe consider to use the seeded table to create a model as iceberg.

Could you give me your use case about using iceberg with seeds? (note we could consider to implement that in the future).

@brabster
Copy link
Contributor Author

brabster commented Apr 27, 2023

Not an issue about the breaking change, these things happen! There seem be a few differences actually in behaviour from 1.3.5 so I forked from that tag instead of main to get us working for now. I'm not about until next week now but I'll check back in and raise a separate feature request for iceberg seed tables (if I can justify it) when I get back to it.

@nicor88
Copy link
Member

nicor88 commented Apr 27, 2023

@brabster ok, consider then raising the specific errors relative to seeds in this one, or consider proposing a fix .

Regarding iceberg for seeds, will be good to open another issue and the reason why would be needed.

@Jrmyy
Copy link
Member

Jrmyy commented May 15, 2023

@brabster @nicor88 Should we close this now we have the documentation ?

@nicor88
Copy link
Member

nicor88 commented May 15, 2023

Yes we should close we have this: #291 and this #302 to tackle this issue.

@Jrmyy Jrmyy closed this as completed May 15, 2023
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

No branches or pull requests

3 participants