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

feat(stepfunctions-tasks): add EKS call to SFN-tasks #12779

Merged
merged 12 commits into from
Feb 22, 2021

Conversation

NovakGu
Copy link
Contributor

@NovakGu NovakGu commented Jan 29, 2021

Taking over the ownership of original PR #11738

feat(stepfunctions-tasks): support for EKS Call

Implementation

Update package @aws-cdk/aws-stepfunctions-tasks to include support for EKS Call

API as per documentation here:
https://docs.aws.amazon.com/step-functions/latest/dg/connect-eks.html

Includes support for the following Amazon EKS API calls:
eks:call

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 29, 2021

@mergify
Copy link
Contributor

mergify bot commented Jan 29, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@NovakGu NovakGu changed the title Guchangh/eks call feat(stepfunctions-tasks): add EKS call to SFN-tasks Jan 29, 2021
@NovakGu NovakGu marked this pull request as ready for review January 29, 2021 18:11
@daisuke-yoshimoto
Copy link
Contributor

@shivlaks
Would you please review this PR?

Comment on lines 11 to 14
/**
* Name of the cluster
*/
readonly clusterName: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not take in an ICluster and take properties out of it?
question: are step functions executions of EKS restricted to the account in which the state machine resides?

this is available as clusterName in an ICluster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will use EKS L2 Construct instead in next commit.

Comment on lines 21 to 24
/**
* API endpoint to communicate with your cluster
*/
readonly endpoint: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is clusterEndpoint from an ICluster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will use EKS L2 Construct instead in next commit.

/**
* Base 64 encoded certificate data required to communicate with your cluster
*/
readonly certificateAuthority: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is available as clusterCertificateAuthorityData from an ICluster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will use EKS L2 Construct instead in next commit.

/**
* Method type of a EKS call
*/
export enum MethodType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

choose a more specific name. It doesn't quite communicate the intent of the enum... maybe something like HttpMethods or some appropriate qualifier that is less general from a naming perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will Change to HttpMethods in next commit.

import * as cdk from '@aws-cdk/core';
import { EksCall, MethodType } from '../../lib/eks/call';

describe('Call an EKS endpoint', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing tests for invalid service integration pattern (i.e. an error is thrown if RUN_JOB is selected)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add tests for run a job pattern in next commit.

Comment on lines +4 to +6

const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-stepfunctions-tasks-eks-call-integ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is not self contained. Please include verification steps, but more importantly create the EKS cluster as a part of the integration test.

The test should create a state machine that can be executed successfully. Right now it has a dependency that an EKS job already exists called clusterName, but this is not self-contained.

Verification steps should look something like this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified integ test in the latest commit

@mergify mergify bot dismissed shivlaks’s stale review February 4, 2021 01:03

Pull request has been modified.

@NovakGu
Copy link
Contributor Author

NovakGu commented Feb 4, 2021

Commit: b831188

Description:

  • Refactor call.ts to use EKS L2 Construct
  • Add more coverage test for call.ts
  • Modified integ test for call.ts, created EKS cluster as part of the test
  • Addressed other comments from previous commit
  • Rebase local branch with master

Testing:

  • Deploy integ test to personal account, manually verified the state machine created can be executed and finish with SUCCEEDED
  • Run Coverage test locally with npm test
  • yarn integ eks/integ.call.js

@daisuke-yoshimoto
Copy link
Contributor

@shivlaks

It looks like the fix is complete. Would you please re-review it?

Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting closer! - would also love a review/input from @iliapolo who is the maintainer of the eks module in the cdk

];

protected readonly taskMetrics?: sfn.TaskMetricsConfig;
protected readonly taskPolicies?: iam.PolicyStatement[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably worth a comment why task policies are not being set in this integration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment in latest commit.

Comment on lines 24 to 26
requestBody: sfn.TaskInput.fromObject({
RequestBody: 'requestBody',
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the type of requestBody be sfn.TaskInput ? it's currently not configured in that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good point. Changed the requestBody to be sfn.TaskInput

readonly httpPath: string;

/**
* Query Parameters part of HTTP request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be helpful to include doc links and or purpose of query parameters. the current doc strings don't add much information from the parameter names

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query parameters would look something like this: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#-strong-write-operations-cronjob-v1beta1-batch-strong- I'm not sure if it would be helpful to include an example such as this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, I agree. maybe using it in the readme example would be helpful for users though.

* Request body part of HTTP request
* @default - No request body
*/
readonly requestBody?: { [key: string]: any };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not support paths: i.e. how could you supply $.blah
please change the type to sfn.TaskInput and also add a test for it.

the same applies to any parameters that support state input json path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to sfn.TaskInput and added more coverage tests

@mergify mergify bot dismissed shivlaks’s stale review February 9, 2021 19:45

Pull request has been modified.

@NovakGu
Copy link
Contributor Author

NovakGu commented Feb 9, 2021

Updating branch to re-trigger build

Comment on lines 78 to 79
CertificateAuthority: this.props.cluster.clusterCertificateAuthorityData,
Endpoint: this.props.cluster.clusterEndpoint,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these parameters required for successfully making the call? Note that if ICluster is imported, these properties might not be defined, and will throw an exception.

I would extract these and do:

try {
  const ca = this.props.cluster.clusterCertificateAuthorityData;
} catch (err) {
  // if its required, throw an error here with the right context
  // if its not required, just pass
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I'll add validation for ClusterName, ClusterEndpoint and clusterCertificateAuthorityData, as well as coverage tests for these cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With some testing I think the error thrown from the eks module has the valid context in our case.

for eks:call I'm extracting clusterName, clusterEndpoint, and clusterCertificateAuthorityData from ICluster, for example if clusterEndpoint is not defined, it will throw exception with message "clusterEndpoint" is not defined for this imported cluster, which is sufficient to show the context of the error.

All three are required to make the call, I'll just add coverage test to reflect this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shivlaks Do you think "clusterEndpoint" is not defined for this imported cluster is sufficient or more details needs to be added such as Cluster Endpoint is not defined for this imported cluster, this field is required for eks:call task.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NovakGu that seems sensible. let's keep it simple. I generally like the format of <validation error><remediation (if any)>

i.e.
The clusterEndpoint property must be specified when using an imported Cluster.

Copy link
Contributor Author

@NovakGu NovakGu Feb 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shivlaks @iliapolo
Added validation for clusterEndpoint and clusterCertificateAuthorityData.
clusterName is required when import a cluster or creating a new cluster so I'm not double checking it here.

After changes

  • if an given cluster does not have cluster Endpoint it will throw with error
    The "clusterEndpoint" property must be specified when using an imported Cluster.
  • If an given cluster does not have certAuth it will throw with error
    The "clusterCertificateAuthorityData" property must be specified when using an imported Cluster.

@shivlaks shivlaks added the pr/do-not-merge This PR should not be merged at this time. label Feb 18, 2021
shivlaks
shivlaks previously approved these changes Feb 18, 2021
Copy link
Contributor

@shivlaks shivlaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. added a do-not-merge so @iliapolo can also take a look in case he had any additional feedback.

@iliapolo
Copy link
Contributor

LGTM

@daisuke-yoshimoto
Copy link
Contributor

@NovakGu @shivlaks @iliapolo
Is this PR not yet merged?

This was referenced Mar 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants