Skip to content

Feat/aws setup role arn - #2280

Open
EduVencovsky wants to merge 2 commits into
mainfrom
feat/aws-setup-role-arn
Open

Feat/aws setup role arn#2280
EduVencovsky wants to merge 2 commits into
mainfrom
feat/aws-setup-role-arn

Conversation

@EduVencovsky

Copy link
Copy Markdown
Contributor

Description of the issue

The onboarding setup scripts (scripts/) had two usability gaps in the Azure-to-AWS flow:

  1. Wrong-account failure mode. The Azure-side setup takes the full IAM role ARN, but aws/setup.sh only took a role name and trusted whatever account the shell was authenticated against. Running it in the wrong account's credential context created the role and trust there, and the failure only surfaced later — silently — on the assume-role side.

  2. Buried tenant ID. In the Azure VM flow, the tenant ID (the value the AWS trust step needs) was logged mid-output, after the minutes-long identity assignment, making it easy to miss and hard to copy. A mistyped VM name was also only discovered mid-run, via a raw CLI error from the identity step.

Description of changes

aws/setup.sh now also accepts CWAGENT_AWS_ROLE_ARN. When set, it validates the ARN format, fails fast when the ARN's partition or account does not match the shell's credentials (sts get-caller-identity), and derives the role name from the ARN. A CWAGENT_AWS_ROLE_NAME that disagrees with the ARN is rejected; an agreeing or default-valued one is allowed (the dispatcher always exports the default). The interactive prompt now asks for an ARN, still accepting a plain role name or empty input for the default. A warning is printed if the resulting role differs from the provided ARN (e.g. an existing EC2 instance-profile role takes precedence). Validation runs before any IAM mutation.

azure/setup.sh (azure_vm flow): the az vm show probe now fails fast with a clear message when the VM cannot be found (az's own stderr is preserved so permission or throttling errors are not misread as not-found); the tenant ID is emitted before the slow identity and install steps, so the user can start the AWS side in parallel; and it is repeated at the very end for easy copying — in the install-success message ("Agent installed on in tenant ") and above the next-steps text of the identity-only flow.

README.md: CWAGENT_AWS_ROLE_ARN documented in the common environment variable table.

No breaking changes: all existing inputs and flows behave as before when the new variable is unset.

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

  1. Ran make fmt and make fmt-sh
  2. Ran make lint

@EduVencovsky
EduVencovsky requested a review from a team as a code owner September 8, 2026 13:30
Comment thread scripts/aws/setup.sh
# Skip when a role was already identified through the environment. An ARN
# is preferred (it pins the account), but a plain name is still accepted:
# this script often creates the role, so a first run may have no ARN yet.
if [ -z "${ROLE_ARN_INPUT}" ] && [ -z "${CWAGENT_AWS_ROLE_NAME:-}" ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Why do we check ROLE_ARN_INPUT and not the CWAGENT_AWS_ROLE_ARN itself since we're looking at CWAGENT_AWS_ROLE_NAME directly?

Comment thread scripts/aws/setup.sh
Comment on lines +299 to +300
# A plain if, not a trailing && list: an empty answer keeps the
# default role name and must not fail the function under set -e.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Since this script is meant to be downloaded by the user, consider trimming the comments to explanations that would be meaningful to that reader. This comment explains why we changed it, but isn't too useful when reading the script fresh IMO.

Comment thread scripts/aws/setup.sh

caller_partition=$(printf '%s' "${AWS_ARN}" | cut -d: -f2)
if [ "${arn_partition}" != "${caller_partition}" ]; then
die "the role ARN is in partition ${arn_partition} but this shell's credentials are in ${caller_partition}. Rerun with credentials for the account the role lives in"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: The mismatch in this case is the partition not just the account.

Suggested change
die "the role ARN is in partition ${arn_partition} but this shell's credentials are in ${caller_partition}. Rerun with credentials for the account the role lives in"
die "the role ARN is in partition ${arn_partition} but this shell's credentials are in ${caller_partition}. Rerun with credentials for the ${arn_partition} partition"

Comment thread scripts/aws/setup.sh
Comment on lines +1049 to +1054
# The role in play can drift from a provided ARN: aws_ec2 swaps in an
# existing instance-profile role, and a pathed ARN for a role that did not
# exist yet is created at the default path. Flag either, neutrally.
if [ -n "${ROLE_ARN_INPUT}" ] && [ "${ROLE_ARN}" != "${ROLE_ARN_INPUT}" ]; then
logwarn "using ${ROLE_ARN} instead of the provided ${ROLE_ARN_INPUT}"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like the only place that can reassign ROLE_NAME is the trust_aws_ec2. Consider moving the warning there and compare it to the existing instance-profile role.

section "Using existing instance profile..."
log "Instance profile ${PROFILE_NAME} attached to ${INSTANCE_ID}"
log "Role: ${EXISTING_ROLE}"
ROLE_NAME="${EXISTING_ROLE}"

If we also add support for pathed IAM roles, then there shouldn't be any further drift and this check can be removed.

Comment thread scripts/aws/setup.sh
Comment on lines +348 to +351
# The role name is the last path segment, so an ARN with a path
# (arn:...:role/path/Name) resolves to the plain name the IAM CLI wants.
arn_role_name="${ROLE_ARN_INPUT##*/}"
[ -n "${arn_role_name}" ] || die "invalid IAM role ARN: ${ROLE_ARN_INPUT} (empty role name)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we support ROLE_ARN as an input, do we need to be able to parse the path and support --path in the create-role call in ensure_iam_role?

Comment thread scripts/azure/setup.sh
--name "${VM_NAME}" \
--query "[[identity.principalId, storageProfile.osDisk.osType]]" -o tsv 2>/dev/null || true)
--query "[[identity.principalId, storageProfile.osDisk.osType]]" -o tsv) ||
die "cannot find VM ${VM_NAME} in resource group ${RESOURCE_GROUP} (check the names against the subscription logged above)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Consider delimiting the variables with single quotes.

Suggested change
die "cannot find VM ${VM_NAME} in resource group ${RESOURCE_GROUP} (check the names against the subscription logged above)"
die "cannot find VM '${VM_NAME}' in resource group '${RESOURCE_GROUP}' (check the names against the subscription logged above)"

Also, it could be a permission issue as well, so "cannot find" could be inaccurate.

Comment thread scripts/azure/setup.sh
if INSTALL_CMD=$(linux_install_cmd "${install_env}"); then
run_via_az "RunShellScript" "${INSTALL_CMD}"
log "Agent installed on ${VM_NAME}"
log "Agent installed on ${VM_NAME} in tenant ${TENANT_ID}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: "in tenant ${TENANT_ID}" is a bit odd to add to this message. If anything, the subscription would make more sense. If we're trying to do a better job of surfacing the tenant ID for use in the aws/setup.sh, consider repeating

log "Tenant ID (for the AWS setup): ${TENANT_ID}"

before the install returns instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants