Add multiple options to make a connection with ECS Cluster.

[rariyama]

What I did

I added more options to access to ECS Cluster.
There is currently only one option to make a connection with ECS Cluster which is the way to connect with AWS AccessKeyId and SecretAccessKey.
I made a change that DefaultAWSCredentialsProviderChain is called if agent.command_executor.ecs.<name>.access_key_id is not defined, and then the other types of credentials such as environment variables or Instance Profile are used.
I referred to the process of authorizing S3 Bucket and made the implementation closer to it as possible.

What I confirmed

I confirmed the items below.

• ./gradlew check is succeeded
• I can use access_key_id and secret_access_key if both are defined on properties.
• If access_key_id and secret_access_key are not written, the other credential is used and task is succeeded.

This is the first PR I create on this repository so I'm sorry in advance if there is any inconvenience.

[hiroyuki-sato]

I'm sorry, I actually commented the wrong PR. this comment was for #1762.

This PR author writes an article about this change.
https://tech.wealthnavi.com/entry/20221125/1669353895 written in Japanese.

@yoyama, Could you look at this PR when you get a chance?

I think this PR does not change current behavior by default.

[yoyama]

Basically it looks good.
But I hope adding tests for https://github.com/treasure-data/digdag/pull/1759/files#diff-4094a97e13055fa1f84ffe3297bb4c703cea9506ad50ae581ae9bd80a0f3504bR31

[rariyama]

Thanks for your reply.
I'm sorry if I misunderstood but your comment means that createClient is needed to be tested? DefaultAWSCredentialsProviderChain is a method of AWS SDK so it should have been tested by others and it doesn't need to be tested in digdag.

[szyn]

Thank you for creating this PR. Since this PR change will change the behavior when system properties are not set, it would be a good idea to add a test to see if the expected authentication method (AWSStaticCredentialsProvider/DefaultAW SCredentialsProviderChain) is used depending on the existence of the system property.

IMO, I understand that there is basically no problem since the behavior is the same as before in cases where system property (agent.command_executor.ecs.<name>.access_key_id) is set. Maybe writing an integration test as a follow-up PR would be a good idea.

I have one request. I would appreciate it if you leave a note on this page describing the new behavior, as in the PR, and please mention that it is supported in v0.10.5 and above.

digdag/digdag-docs/src/command_executor.md

Line 58 in 011d848

[rariyama]

@szyn
Thank you for your comments.

Thank you for creating this PR. Since this PR change will change the behavior when system properties are not set, it would be a good idea to add a test to see if the expected authentication method (AWSStaticCredentialsProvider/DefaultAW SCredentialsProviderChain) is used depending on the existence of the system property.

IMO, I understand that there is basically no problem since the behavior is the same as before in cases where system property (agent.command_executor.ecs..access_key_id) is set. Maybe writing an integration test as a follow-up PR would be a good idea.

I agreed with the idea to add integration test for confirming that digdag server can connect with ECS with the expected way. If possible, If possible, I'll try to add the test.

I have one request. I would appreciate it if you leave a note on this page describing the new behavior, as in the PR, and please mention that it is supported in v0.10.5 and above.

I added the explanation on this commit e47c6fa, so please review this if you are available.

By the way, We can choose credentials provided by DefaultAWSCredentialsProviderChain other than AWS access key when connecting with S3 temporal storage because similar method has already been implemented in S3StorageFactory and TemporalProjectArchiveStorage uses the function. Thus, the description I added on this PR can be applied to S3 access key and secret, so I think it'll also be better to mention about that. How do you think about my idea?

[szyn]

Sounds good, thank you.

By the way, We can choose credentials provided by DefaultAWSCredentialsProviderChain other than AWS access key when connecting with S3 temporal storage because similar method has already been implemented in S3StorageFactory and TemporalProjectArchiveStorage uses the function. Thus, the description I added on this PR can be applied to S3 access key and secret, so I think it'll also be better to mention about that. How do you think about my idea?

Thank you for sharing your opinion. That's a good point 👍 I agree with mentioning it.
May I ask you to add a note to the document on this point? Of course, this can be done as a separate PR.