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(codebuild): Add support for triggering and monitoring AWS CodeBu… #595

Merged
merged 5 commits into from
Feb 4, 2020

Conversation

Kaixiang-AWS
Copy link
Contributor

@Kaixiang-AWS Kaixiang-AWS commented Jan 22, 2020

…ild builds

Add functionalities to start a build in AWS CodeBuild and monitor the build status. AWS CodeBuild is different from the existing build services (Jenkins, Travis) therefore cannot use the existing architecture. It's more like Google Cloud Build which implements its own authentication strategy.

For CodeBuild account in igor, it uses AWS access key id and secret to authenticate. I don't know if there's way to talk to clouddriver to fetch aws cloud provider credentials. If yes, I can update my implementation later to fetch credentials from clouddriver.

Related Issue: spinnaker/spinnaker#5292

this.client =
(AWSCodeBuildClient)
AWSCodeBuildClientBuilder.standard()
.withCredentials(new AWSStaticCredentialsProvider(awsCreds))
Copy link
Member

Choose a reason for hiding this comment

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

Ideally you don't use static credentials, you use a role so that you don't have to store static creds in your hal config. With clouddriver, the 'aws' provider account is configured with a role to assume, and the environment where clouddriver runs is configured with credentials that the SDK default creds provider can pick up to assume that role (like instance creds, ECS creds, env variable creds, etc) using the STSAssumeRoleSessionCredentialsProvider.

So my hal config looks something like this:

      accounts:
      - name: my-aws-devel-acct
        requiredGroupMembership: []
        providerVersion: V1
        accountId: '111111111111'
        regions:
        - name: eu-central-1
        assumeRole: role/SpinnakerManaged

And then clouddriver just uses the role session provider:
https://github.com/spinnaker/clouddriver/tree/master/clouddriver-aws/src/main/groovy/com/netflix/spinnaker/clouddriver/aws/security

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ideal scenario would be specifying aws account name in codebuild account configuration and then all codebuild operations use the credentials from that aws account associated. However, I couldn't find a way to let igor talk to clouddriver to fetch the credentials. Another way is re-implementing the assume role logic in igor. That will introduce duplicate code in two packages. I can do that if that's really necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah agree that would be ideal, but I do think you will need to duplicate the logic unfortunately, though it can probably be simpler in Igor than the way it's implemented in clouddriver


public static StartBuildRequest toStartBuildRequest(Map<String, Object> requestBody) {
String projectName = (String) requestBody.get(PROJECT_NAME);
return new StartBuildRequest().withProjectName(projectName);
Copy link
Member

Choose a reason for hiding this comment

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

When this is a stage in a pipeline, is it common to get any other attributes like the commit ID that needs to be built? I'm surprised not to see sourceVersion as an input

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add more advanced use case such as source version and environment variables in later commits. This commit is more focusing on successfully trigger a build from Spinnaker and monitor build status.

public class AwsCodeBuildRequestHandler extends RequestHandler2 {
@Override
public void afterError(Request<?> request, Response<?> response, Exception e) {
if (e instanceof AWSCodeBuildException) {
Copy link
Member

Choose a reason for hiding this comment

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

Would this include ThrottlingException? Should API rate throttling be a build job error or a runtime exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ThrottlingException does not inherit from AWSCodeBuildException so it will be a RuntimeException

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the implications of picking build job error vs runtime exception are for how Spinnaker behaves when these are returned; I'm not super familiar with Igor. Why should ThrottlingException be runtime error but AccountLimitExceededException be a build error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BuildJobError is returned as 400 while RuntimeException will become an InternalError and return 500. I think throttling exception should probably return 400 instead of 500. I will categorize it as BuildJobError as well.

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 32 to 33
private String accessKeyId;
private String secretAccessKey;
Copy link
Member

@clareliguori clareliguori Jan 29, 2020

Choose a reason for hiding this comment

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

Ideally this is account ID + role, instead of static credentials that have to be rotated

@clareliguori clareliguori added the ready to merge Approved and ready for merge label Feb 4, 2020
@mergify mergify bot merged commit 8422a0a into spinnaker:master Feb 4, 2020
@mergify mergify bot added the auto merged label Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants