Fix AWS Region with Default region

[NickLaMuro]

Overview

When using the aws plugin, if the DEFAULT_REGION is set in the 1Password item, it will override any usage of AWS_REGION, which should take president normally with the aws cli, preventing the usage of being able to connect to other regions unless you regularly change the DEFAULT_REGION value in the 1Password each time.

Type of change

• Created a new plugin
• Improved an existing plugin
• Fixed a bug in an existing plugin
• Improved contributor utilities or experience

Related Issue(s)

• Resolves: #
• Relates: #

How To Test

• Set a DEFAULT_REGION value in your 1Password item you use for the aws plugin
• Run a aws s3 ls (or something similar)
  • Validate these are in the region of DEFAULT_REGION
• Run AWS_REGION=different-region aws s3 ls, using a different-region then the one you have configured
  • Validate the buckets from the previous command are different

In the previous version, this would return the same value for both commands, and this should fix that.

Other options is quering for something specific by ARN that exists in only in the DEFAULT_REGION, and with this patch, the second command will fail, where previously it would find it.

Changelog

Fix DEFAULT_REGION logic for aws plugin to allow for overrides using AWS_REGION.

[github-actions]

⚠️ This PR contains unsigned commits. To get your PR merged, please sign those commits (git rebase --exec 'git commit -S --amend --no-edit -n' @{upstream}) and force push them to this branch (git push --force-with-lease).

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

1. Generate a GPG key
2. Add the GPG key to your GitHub account
3. Configure git to use your GPG key for commit signing

Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

1. Generate an SSH key and add it to ssh-agent
2. Add the SSH key to your GitHub account
3. Configure git to use your SSH key for commit signing

Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.

Learn how to use 1Password to sign your commits.

[edif2008]

Thank you for your contribution.

I've left some comments concerning the backwards compatibility of these changes. Could you please take a look and adjust your changes accordingly?
Let me know if you have any questions or need additional guidance. 😄

[edif2008]

Instead of changing this test, can you add a new one in which the Region is configurerd? In this way, we ensure that this change is backwards compatible.

[edif2008]

This change looks backwards incompatible to me.

What we're trying to achieve is the following:

• If the 1Password item doesn't have any region defined (default region nor region), then look for the default region in the config
• If the 1Password item contains default region, use that one
• If the 1Password item contains region, use that one
• If the 1Password item contains default region and we set the environment variable AWS_REGION, then use the value from AWS_REGION

Out of these the following scenario seems to fail now:

A user has a default region in ~/.aws/config file and a default region defined in 1Password item. The shell plugin will now use the value in ~/.aws/config instead of the one in the 1Password item.

[edif2008]

Maybe this table explains it better in terms of the expected order of precedence:

AWS config file	1P item default region field	1P item region field	AWS_REGION env var	Expected result
✅	❌	❌	❌	AWS config file value
✅	✅	❌	❌	1P item default region value
✅	❌	✅	❌	1P item region value
✅	✅	❌	✅	AWS_REGION env var value

Out of these 4 cases, the second one doesn't work as expected with the current changes.

Are there any other scenarios that you're thinking of that should be covered by this PR in terms of order of precendence?

[NickLaMuro]

Sorry, getting back to this as I have been a bit swamped at work lately, and as you will probably notice below... it isn't a simple "one line" response to your answer...

If the 1Password item contains default region, use that one

So, this is where I have disagreement, though, recognize it has a nuanced take, and won't die on a hill over it. However, I think it is worth stating all the facts before making a decision.

To start, what is currently in place is wrong: treating "region" and "default region" as the same, when "default region" should only be used to in absence of a --region and $AWS_REGION.

In addition, up in the test file you say this:

Instead of changing this test, can you add a new one in which the Region is configurerd? In this way, we ensure that this change is backwards compatible.

about this change:

 			description: "has region both in 1Password and local config, but values differ",
 			itemFields: map[sdk.FieldName]string{
 				fieldname.OneTimePassword: "515467",
-				fieldname.DefaultRegion:   "us-east-2",
+-				fieldname.Region:          "us-east-2",

I personally take issue with it for two reasons:

1. The spec description doesn't match the arguments that are passed. It passes DefaultRegion from the fieldname, but the region is what is being described in the description:. So this test will need to change regardless to properly reflect what it is actually trying to test.
2. I disagree with the premise this should stay the same for backwards compatibility, because again, I think this functionality is broken. Either:
   a. It should be describing fieldname.region, which then there should be an error when conflicting with what is in the ~/.aws/config already (what I changed the test to be currently).
   b. It should be describing fieldname.defaultRegion, where we then need to set up a proper order of precedence for this plugin and then describe what should actually be tested here.

And to be clear, I should write another test case in addition to what was already there, but when I original was writing this up (9 months ago at this point), I was having trouble getting specs working locally, so I sorta gave up... but I will add another test once we decide on the a/b from above (or, as I get to later, a better understanding of how this plugin should behave, as I argue it is unclear).

So regarding the existing region/default_region precedence from AWS, we have a decent number documents to sort of derive what it is for the AWS CLI/SDK before introducing the logic from this plugin:

• https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html#envvars-list-AWS_DEFAULT_REGION
• https://docs.aws.amazon.com/sdkref/latest/guide/feature-region.html
• https://docs.aws.amazon.com/sdk-for-ruby/v3/developer-guide/region.html
• https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-files.html
• https://docs.aws.amazon.com/cli/latest/topic/config-vars.html#general-options
• https://docs.aws.amazon.com/sdk-for-go/v2/developer-guide/configure-gosdk.html

I think the first 3 links are the most useful for trying to understand the general patterns across the CLI and all of the SDKs AWS provides, but from what I found, the description in the Ruby (my main language, so also am most comfortable with it) was the most concise example for an SDK.

Generally speaking, region precedence is derived in this order of priority of CLI FLAGS > ENV VARS > ~/.aws/config, with a more detail list below (ordered from highest precedence to least):

• Directly with a CLI flag (--region) or client object (in Ruby... AWS::EC2::Client.new(:region => "..."))
• Directly in a SDK's config object (if it exists)
  • in Ruby: Aws.config(:region => "...")
• $AWS_REGION set in the ENV
• $AWS_DEFAULT_REGION set in the ENV
• The region = for the current ~/.aws/config profile, either provided as:
  • --profile (for the CLI specifically)
  • AWS_PROFILE="my_profile" (everything)
  • (you might be able to define a profile in a SDK programmatically as well, but didn't take the time to look up a example)
• The region for the default config in the ~/.aws/config
  • I will refer to this as [default]region going forward

Of note is the $AWS_DEFAULT_REGION env var. My default assumption (prior to researching and looking it up), would be to think that it would be of a lesser priority than then current profile's region (so if AWS_PROFILE=foo is set, and the [foo]region is set, it actually supersede that that and $AWS_DEFAULT_REGION will have the higher priority).

The following example can be used as an example explain a bunch of different scenarios (though, I don't think it is exhaustive):

~/.aws/config

[default]
region = "us-east-1"

[uk]
region = "eu-west-2"

$ aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-east-1 (from `default` profile in `~/.aws/config`)
$ AWS_REGION=us-west-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-west-1 (from `AWS_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to ca-central-1 (from `AWS_DEFAULT_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 AWS_REGION=us-west-1 aws sts get-caller-identity --debug 2>&1 | grep client_region
# sets the region to us-west-1 (from `AWS_REGION`)
$ AWS_DEFAULT_REGION=ca-central-1 aws sts get-caller-identity --profile uk --debug 2>&1 | grep client_region
# sets the region to ca-central-1 (from `AWS_DEFAULT_REGION`)
$ aws sts get-caller-identity --profile uk --debug 2>&1 | grep client_region
# sets the region to eu-west-2 (from `uk` profile in `~/.aws/config`)

Again, I would personally would assume normally that AWS_DEFAULT_REGION would be treated more like [default]region in the ~/.aws/config, just with a slightly higher priority, but instead, it completely supersedes the precedence even the [uk]region above. I guess the thought process from AWS' perspective is any ENV takes priority over anything in the ~/.aws/config, versus anything "default region" being evaluated first, then anything specifically "region" being evaluated after (how I personally would like it to be and makes more sense in my eyes).

Where I think a decision needs to be made is where 1Password, and this plugin, should inject itself in this mess, which I think is a non-trival question to answer.

For me personally , I think having defaultRegion be the lowest priority beyond anything above makes the most sense to me. Basically, set region to that if nothing else exists to set it, otherwise use what is provided through any other means (including fieldname.region from 1Password), and not error at all. This will act different than how fieldname.region currently works, where it will error if there is a conflict. To me, this is the most useful, where I don't want to be editing my defaultRegion in 1Password ever, and it is simply there to provide something when all else is absent, and probably most people will be using ~/.aws/config anyway, so a default can really be set there.

If, instead, we are treating this more similar to $AWS_DEFAULT_REGION, which, based on the current documentation is a reasonable conclusion to come to:

• https://developer.1password.com/docs/cli/shell-plugins/aws/#reference

Then I don't really see this having errors on around it, and it is simply a backup or initial setting of $AWS_DEFAULT_REGION isn't already set, but still will take more precedence than what is in the ~/.aws/config. What was here previously was then erroring if either fieldname.region or fieldname.defaultRegion is defined, which I think don't think makes any sense all, and what I am trying to change here. But I can at least understand logically making it behave more like the existing $AWS_DEFAULT_REGION ENV var it is meant to stand in for, regardless if it is confusing to me personally or not.

With region, I can more understand that field having errors, since without them, you could mask errors/unintended behaviour when used in conjunction with ~/.aws/config. However, I don't see anywhere in the docs above where fieldname.region is even mentioned to define it, so I this as kind of a hidden feature anyway and makes it hard to for me to determine what the expectations are. I may had a bit better of an idea of the intentions when I original wrote up this PR, as I did spend some time doing some git history explanation into this plugin, but any knowledge of that has escaped me at this point.

Personally, I don't see any utility in having the region in 1Password without it being able to be overwritten by a profile anyway (so, the errors are kind of an anti-feature to me), but also that isn't my biggest concern with this PR, and instead separating fieldname.defaultRegion from the fieldname.region is.

---

Main point with all of this is that without defining the order of precedence, and what each of these fields are supposed to do in the context of the greater existing precedence and expectations (set by AWS), then there isn't much else that I can do here before that and we should probably define that first (ideally in the documentation).

At this point, I could see what I am writing here more as the start of a GitHub Issue, which in hindsight, maybe is what I should have started with in the first place (miscalculation on the quagmire I was getting myself into I guess, so mostly self induced and a realization in hindsight). So if that makes more sense, we can do that and disregard this PR until the expectations of how this plugin should treat these fields is determined. I would say even having this discussion in regards to the documentation makes more sense, but I am not sure there is a opensource repo for me to do that against so that might not be an option.

Curious on your thoughts on what makes sense going forward. Thanks.