josmo / drone-ecs

Drone plugin for triggering Amazon EC2 Container Service (ECS) deployments
Apache License 2.0
30 stars 41 forks source link

Fixing non-awsvpc networking issue #39 and add volumes and mount points #48

Closed joescharf closed 4 years ago

joescharf commented 4 years ago

This pull request fixes issue #39 where non-awsvpc network modes will not deploy

The problem

    func (p *Plugin) setupServiceNetworkConfiguration() *ecs.NetworkConfiguration {
    netConfig := ecs.NetworkConfiguration{AwsvpcConfiguration: &ecs.AwsVpcConfiguration{}}

    if p.NetworkMode != ecs.NetworkModeAwsvpc {
        return &netConfig
    }

When we aren't using awsvpc mode, we return a ecs.NetworkConfiguration{AwsvpcConfiguration: &ecs.AwsVpcConfiguration{}} which looks like essentially a bunch of nil structs, but we have a non nil AwsvpcConfiguration pointer field pointing to an empty AwsVpcConfiguration struct.

When NewtworkConfiguration calls its Validate() method:

    if s.AwsvpcConfiguration != nil {
        if err := s.AwsvpcConfiguration.Validate(); err != nil {
            invalidParams.AddNested("AwsvpcConfiguration", err.(request.ErrInvalidParams))
        }
    }

AwsvpcConfiguration is not nil so AwsvpcConfiguration.Validate() is called.

and AwsvpcConfiguration.Validate() checks for required field Subnets

func (s *AwsVpcConfiguration) Validate() error {
    invalidParams := request.ErrInvalidParams{Context: "AwsVpcConfiguration"}
    if s.Subnets == nil {
        invalidParams.Add(request.NewErrParamRequired("Subnets"))
    }

and therefore we get the error:

2020/01/11 05:48:15 InvalidParameter: 1 validation error(s) found.
- missing required field, UpdateServiceInput.NetworkConfiguration.AwsvpcConfiguration.Subnets.

Therefore instead of:

    func (p *Plugin) setupServiceNetworkConfiguration() *ecs.NetworkConfiguration {
    netConfig := ecs.NetworkConfiguration{AwsvpcConfiguration: &ecs.AwsVpcConfiguration{}}

    if p.NetworkMode != ecs.NetworkModeAwsvpc {
        return &netConfig
    }

We should return a nil if p.NetworkMode != ecs.NetworkModeAwsvpc like so:

    func (p *Plugin) setupServiceNetworkConfiguration() *ecs.NetworkConfiguration {
    netConfig := ecs.NetworkConfiguration{AwsvpcConfiguration: &ecs.AwsVpcConfiguration{}}

    if p.NetworkMode != ecs.NetworkModeAwsvpc {
        return nil
    }

So we should only pass a NetworkConfiguration if the networktype is awsvpc

Per this ECS API page networkConfiguration is not a required parameter to pass for UpdateServiceRequest()

joescharf commented 4 years ago

I was going to separate out the fix for #39 and the addition of mount_points and volumes for #21 but I guess they're all coming through in one pull request now.

josmo commented 4 years ago

Thanks @joescharf I'll merge and tag for a new release :)