glue-alpha: feedback on spark jobs constructs introduced in "Refactored glue-alpha L2 CDK construct RFC 0497"

[humanzz]

Describe the feature

Hello CDK team,

As a user of glue-alpha, and having contributed the initial job construct in #12506, I recently came across the refactor from #32521 as part of updating my CDK applications to 2.178.0.

Having refactored my code to leverage the new constructs - mostly the ScalaSparkEtlJob - I noticed a couple of things and wanted to provide feedback on them

Job Role requirements and its auto-creation

• In the previous version, providing a role as part of JobProps was optional, and its absence led to the auto-creation of a default role
• In the new base JobProperties (sidenote: should it be called JobProps instead of JobProperties?), role is now required
• Moreover, code in subclasses seem to be wrongly doing something I don't understand e.g.
  • https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-glue-alpha/lib/jobs/scala-spark-etl-job.ts#L89
  • https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-glue-alpha/lib/jobs/scala-spark-etl-job.ts#L89

I think the previous behaviour of making role prop optional is a sensible default, and that behaviour should be restored, and code in subclasses corrected.

Enums / Constants

• Several classes were changed to become enums e.g. GlueVersion and WorkerType which removed the ability to use newer values easily as was possible before e.g. GlueVersion.of(...).
• Documentation on those new enums still reference no-longer existing methods of GlueVersion.of and WorkerType.of e.g. https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-glue-alpha/lib/constants.ts#L5

I think the previous approach, was better, and inline with other constructs e.g. Lambda, and that it should be restored to provide an easy way for adopting new values for GlueVersion and WorkerType without the need to use escape hatches or be blocked on CDK updates.

extraJars, extraFiles, extraPythonFiles, extraJarsFirst

• the following points are for spark jobs of different languages
• extraJars, extraJarsFirst and extraFiles are applicable to all spark jobs regardless of language (Scala/Python)
  • extraJars allow spark to load jvm-based libraries that can be used across both Scala and Python spark jobs
  • extraJarsFirst is about the order of jar loading for all spark jobs across both Scala and Python
• extraFiles is a way to load other files e.g. binary files or text files in spark - again regardless of the spark job's language
• extraPythonFiles is only relevant to Python spark jobs
• the new constructs are not implementing the above behaviour
  • extraJars is not implemented for python spark jobs
  • extraJarsFirst is implemented in only ScalaSparkFlexEtlJob even though it should be available in all spark jobs wherever extraJars is available
  • extraFiles seem to be completely missing from Scala spark jobs

Therefore

• all spark jobs should be updated to support extraJars, extraJarsFirst, extraFiles props (I found a related feat(glue-alpha): include extra jars parameter in pyspark jobs #33238)

Use Case

N/A

Proposed Solution

N/A

Other Information

N/A

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

2.178.0

Environment details (OS name and version, etc.)

macOS

[humanzz]

@natalie-white-aws just wanted to make sure you're seeing this (some related to the comments on #33238 (comment))

[natalie-white-aws]

Thanks for the tag, I hadn’t seen it. extrajars will be addressed in the other issue. Our thinking on making role mandatory was that a default role would be either too broad (I.e. not least-privilege) or too restrictive (since the L2 has no context for what the glue job will need access to), and the developer would not find out until run time. The better developer experience would be to have them create a role that has the right least privilege roles they know they need. Happy to discuss here.

[humanzz]

Overall, I created this issue as an uber-issue, rather than creating a set of smaller issues. I hope we can get to some sort of alignment on each of the issues, so that work/PRs can happen.

as for the Job Role requirements and its auto-creation, I think the developer experience is harmed by the lack of role auto-creation.

It's always a role that needs to be assumable by glue service e.g.

new iam.Role(this, 'Role' {
    assumedBy: new iam.ServicePrincipal('glue.amazonaws.com'),
    ...
}); 

which other L2 constructs - and I reference the main one I based my initial implementation on, which is Lambda - do provide e.g. https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-lambda/lib/function.ts#L946.

As for permissions, I 100% agree on granting the least privileges to that role. You can see in the lambda example above, managed policies are used to grant sensible defaults, and it's then up to the user to add more.

The question then becomes, does AWSGlueServiceRole managed policy strike a good balance with its defined permissions between the least privileges and sensible defaults. I think it goes slightly beyond what the Lambda managed policies grant, e.g. gives s3 bucket write permissions to buckets with certain name patterns but on balance, it does not look overly permissive.

I think a judgement needs to be made, whether this policy is good enough, or if maybe a smaller set of sensible permissions should be granted to the role by default.

[pahud]

Thank you. I do see this in the code doc string

aws-cdk/packages/@aws-cdk/aws-glue-alpha/lib/jobs/job.ts

Lines 316 to 321 in 5160796

	* IAM Role (required)
	* IAM Role to use for Glue job execution
	* Must be specified by the developer because the L2 doesn't have visibility
	* into the actions the script(s) takes during the job execution
	* The role must trust the Glue service principal (glue.amazonaws.com)
	* and be granted sufficient permissions.

And thank you @natalie-white-aws for the chime in.