aws/credentials/ec2rolecreds: zero-value EC2RoleProvider is not safe

[rjeczalik]

The inline doc for ChainProvider says that it's safe to use zero-value of ec2rolecreds.EC2RoleProvider, however the doc for the latter says the Client field is required (bonus: the custom client example is outdated).

The above causes the following code to panic:

creds, err := credentials.NewCredentials(&ec2rolecreds.EC2RoleProvider{}).Get()

with

github.com/aws/aws-sdk-go/aws/ec2metadata.(*EC2Metadata).GetMetadata(0x0, 0x1275890, 0x19, 0x0, 0x0, 0x0, 0x0)
github.com/aws/aws-sdk-go/aws/ec2metadata/api.go:18 +0x1b5
github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds.requestCredList(0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds/ec2_role_provider.go:128 +0x85
github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds.(*EC2RoleProvider).Retrieve(0xc208130bd0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
github.com/aws/aws-sdk-go/aws/credentials/ec2rolecreds/ec2_role_provider.go:85 +0x61
github.com/aws/aws-sdk-go/aws/credentials.(*ChainProvider).Retrieve(0xc208130c30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
github.com/aws/aws-sdk-go/aws/credentials/chain_provider.go:65 +0xd8

Caused by calling NewRequest on a nil *client.Client.

The above example comes from real projects, e.g. terraform.

Now how to fix this - question: what about making zero-value of ec2rolecreds.EC2RoleProvider usable? By adding:

var defaultMetadataClient = ec2metadata.New(session.New(aws.NewConfig()))

func (m *EC2RoleProvider) client() *ec2metadata.EC2Metadata {
  if m.Client != nil {
    return m.Client
  }
  return defaultMetadataClient
}

And changing m.Client to m.client() here.

This way we change aws-sdk-go only, not requiring changes to any other projects that might got that wrong.

What do you think?

[jasdel]

Hi @rjeczalik thanks for reporting this documentation issue, and suggested change. Adding a default EC2Metadata client inside to ec2rolecreds package would create circular dependencies though:

ec2rolecreds -> session -> defaults -> ec2rolecreds

So we wouldn't be able to create a default EC2Metadata client within ec2rolecreds or ec2metadata pacakges. Since this circular dependencies, and the common needed to create EC2Metadata default clients or simple creation of the client with Ec2RoleProvider, I think the defaults package might be a good place for this.

We could add something like the following to the defaults package.

func EC2MetadataClient(...)  *ec2metadata.EC2Metadata {}

Calling defaults.EC2MetadataClient() would give you access to a default ec2 metadata client. Alternatively an additional package would need to be created to return a default ec2 metadata client under ec2metadata package.

How would this work for your use case?

[rjeczalik]

@jasdel I tried adding ec2metadata to defaults package, it failed due to one more cycle in ec2rolecreds tests:

ec2rolecreds_test -> ec2rolecreds -> defaults -> session -> ec2rolecreds

Then I tried adding ec2metadata/defaults package, and importing it within ec2rolecreds, also a cycle. It seems like there's not an easy way breaking out of it.

Do we have any other options? If not, I'm going to send PR against terraform instead.

[jasdel]

@rjeczalik thanks for trying that out. I don't think we'll be able to provide a default instance of the ec2metadata client for the ec2rolecreds package automatically, due to the circular references. I think the SDK's package layout will require users to pass a EC2Metadata client instance to EC2RoleProvider explicitly. We could add a default EC2Metadata instance to the defaults package, but I'm not sure if there is much benefit here over creating your own instance since it would need to be manually passed to EC2RoleProvider.

[rjeczalik]

@jasdel I agree, thanks for helping me out :)

[rjeczalik]

@jasdel Oh I need to be faster next time, wanted to do the same :D