feat: add lake formation support in catalog #810
Conversation
vgkowski
force-pushed
the
feat/lake-formation-catalog
branch
from
3f69301 to
ce60908
Compare
| * Only needed when permissionModel is set to Lake Formation or Hybrid | ||
| * @default - A new role is created | ||
| */ | ||
| readonly lakeFormationDataAccessRole?: IRole; |
There was a problem hiding this comment.
Can you be more explicit here? Who will assume this role? What is its principal? Lakeformation, Glue, Cloudformation?
| * Only needed when permissionModel is set to Lake Formation or Hybrid | ||
| * @default - A new role is created | ||
| */ | ||
| readonly lakeFormationConfigurationRole?: IRole; |
There was a problem hiding this comment.
Can you be more explicit here? Who will assume this role? What is its principal? Lakeformation, Glue, Cloudformation?
| import { Construct } from 'constructs'; | ||
| import { DataCatalogDatabaseProps } from './data-catalog-database-props'; | ||
| import { Context, TrackedConstruct, TrackedConstructProps, Utils } from '../../utils'; | ||
| import { /*grantDataLakeLocation,*/ grantCrawler, grantDataLakeLocation, putDataLakeSettings, registerS3Location, revokeIamAllowedPrincipal } from './lake-formation-helpers'; |
There was a problem hiding this comment.
can you clean this grantDataLakeLocation its commented out.
| /** | ||
| * The IAM Role used by Lake Formation to access data. | ||
| */ | ||
| readonly lfDataAccessRole?: IRole; |
There was a problem hiding this comment.
Please be more explicit in the variable naming, I understand lf means lakeformation, but we need to reflect it in the variable name.
| /** | ||
| * The Lake Formation grant on the data location for the Crawler when Lake Formation or Hybrid is used | ||
| */ | ||
| readonly crawlerLfLocationGrant?: CfnPrincipalPermissions; |
| /** | ||
| * The IAM Role used to revoke LakeFormation IAMAllowedPrincipals | ||
| */ | ||
| readonly lfRevokeRole?: IRole; |
| /** | ||
| * The Lake Formation grant on the data location for the CDK role | ||
| */ | ||
| readonly cdkLfLocationGrant?: CfnPrincipalPermissions; |
There was a problem hiding this comment.
Can you clarify this is only available when using lakeformation, is it also the case for hybrid?
| @@ -59,6 +114,8 @@ export class DataCatalogDatabase extends TrackedConstruct { | |||
|
|
|||
| super(scope, id, trackedConstructProps); | |||
| const catalogType = this.determineCatalogType(props); | |||
| this.permissionModel = props.permissionModel || DataCatalogDatabase.DEFAULT_PERMISSION_MODEL; | |||
| const useLakeFormation = props.permissionModel === PermissionModel.LAKE_FORMATION || props.permissionModel === PermissionModel.HYBRID; | |||
There was a problem hiding this comment.
can you first if the value exist? instead of this being implicit. Meaning use props?.
|
|
||
| if (catalogType === CatalogType.S3 && useLakeFormation) { | ||
|
|
||
| if (props.permissionModel === PermissionModel.LAKE_FORMATION) { |
There was a problem hiding this comment.
Why a nested condition? please lets remove the nested condition and check this on line 180 too.
| * Only needed when permissionModel is set to Lake Formation or Hybrid | ||
| * @default - A new role is created for the entire Data Lake | ||
| */ | ||
| readonly lakeFormationDataAccessRole?: IRole; |
There was a problem hiding this comment.
same as the comment in the datalog datatabse props.
| @@ -10,6 +10,17 @@ | |||
| "@jridgewell/gen-mapping" "^0.3.5" | |||
| "@jridgewell/trace-mapping" "^0.3.24" | |||
|
|
|||
| "@asamuzakjp/css-color@^2.8.2": | |||
There was a problem hiding this comment.
Is there a chance we avoid changing the lockfile in the PR? Ideally we would this to be in a separate PR.
Issue #, if available:
Description of changes:
Add the support for Lake Formation permission model in the
DataCatalogDatabaseand theDataLakeCatalogconstructs. Supports both full and hybrid mode.Checklist
fix:,feat:,docs:, ...)Breaking change checklist
RFC issue #:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.