Skip to content
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions 1.architectures/5.sagemaker-hyperpod/add-security_group-to-fsx-eni.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#!/bin/bash

# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: MIT-0

declare -a HELP=(
"[-p|--profile]"
"[-r|--region]"
"[-f|--fsx-id]"
"[-s|--sg-id]"
)

fsx_id=""
security_group=""
declare -a awscli_args=()

parse_args() {
local key
while [[ $# -gt 0 ]]; do
key="$1"
case $key in
-h|--help)
echo "Add security groups to existing Amazon FSx for Lustre ENI."
echo "Usage: $(basename ${BASH_SOURCE[0]}) ${HELP[@]}"
;;
-p|--profile)
awscli_args+=(--profile "$2")
shift 2
;;
-r|--region)
awscli_args+=(--region "$2")
shift 2
;;
-s|--sg-id)
security_group="$2"
shift 2
;;
-f|--fsx-id)
fsx_id="$2"
shift 2
;;
*)
[[ "$fsx_id" == "" ]] \
&& $fsx_id="$key" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be fsx_id="$key"?

Copy link
Author

Choose a reason for hiding this comment

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

you have to check for both. The syntax do:

  1. Test if fsx_id is empty
  2. If it is true, then it assigns key to fsx_id.

If the user does not provide a fsx_id as part of the arguments, then the test will be FALSE. The fsx_id won't get assigned a value which will cause the script to exit.

|| { echo "Must define one file system id." ; exit -1; }
[[ "$security_group" == "" ]] \
&& $security_group="$key" \
|| { echo "Must define at least one security group id." ; exit -1; }
shift
;;
esac
done

[[ "$fsx_id" == "" ]] || [[ "$security_group" == "" ]] && { echo "Must define at least one filesystem ID and security group ID"; exit -1; }
}

#===Style Definitions===
GREEN='\033[0;32m'
BLUE='\033[0;34m'
YELLOW='\033[1;33m'
NC='\033[0m' # No Color

escape_spaces() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this function being used?

Copy link
Author

Choose a reason for hiding this comment

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

can be removed.

local input="$1"
echo "${input// /\\ }"
}

print_header() {
echo -e "\n${BLUE}=================================================${NC}"
echo -e "\n${YELLOW} $1 "
echo -e "\n${BLUE}=================================================${NC}"
}

#### __main__ ####
parse_args $@

print_header " 🚀 Amazon Sagemaker Hyperpod 🚀 \n \
Amazon FSx for Lustre helper tool \n \
This tool will help by adding new \n \
security groups to the FSx for Lustre ENIs"

# First get one network interface then describe the network interface to get existing Security Groups attached
fsx_id_enis=$(aws fsx describe-file-systems "${awscli_args[@]}" --query 'FileSystems[0].NetworkInterfaceIds' --output text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is getting ENIs for the first fsx found, and not really for defined $fsx_id. If there are multiple fsx, this will cause issues and might cause modifying wrong SG.

We should be using $fsx_id here to make sure we are getting ENIs for correct fsx.

Copy link
Author

Choose a reason for hiding this comment

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

nope, this is getting the ENIs for the $fsx_id defined on the arguments passed to the script. If the user does not pass an fsx_id on the arguments, the script doesn't run.

when the user pass the required arguments, then the parse_args function add the $fsx_id variable to the $awscli_args variable. Then on this CLI bash will expand ${awscli_args[@]} and use the required fsx_id as part of the command. It will then query the ENIs for the fsx_id passed as an argument to this function.

This is the right syntax.

existing_sg=$(aws ec2 describe-network-interfaces "${awscli_args[@]}" --network-interface-ids $temp_eni_id --query 'NetworkInterfaces[0].Groups[*].GroupId' --output text)
Copy link
Collaborator

Choose a reason for hiding this comment

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

$temp_eni_id is undefined and is referenced here, it'll fail.

Need to define "temp_eni_id"

Copy link
Author

Choose a reason for hiding this comment

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

it needs to be fsx_id_enis (as we define in line 83 above).


if [[ -z "$fsx_id_enis" || -z "$existing_sg" ]]; then
echo -e "Error: No ENI or existing security group found. Exiting."
exit 1
fi

echo -e "Amazon FSx for Lustre filesystem: ${GREEN}${fsx_id}${NC}"
echo -e "Existing security groups attached on the filesystem: ${GREEN}${$existing_sg}${NC}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

incorrect variable syntax: ${$existing_sg} -> ${existing_sg}

Copy link
Author

Choose a reason for hiding this comment

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

done

echo -e "Adding security group ID: ${GREEN}${$security_group}${NC}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, incorrect variable syntax: ${$security_group} -> ${security_group}

Copy link
Author

Choose a reason for hiding this comment

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

done


# Finally update the ENI to add the new security groups plus the existing security groups
for i in $fsx_id_enis; do
echo -e "Adding ${GREEN}${$security_group} to ENI $GREEN}${$i}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, ${$security_group} -> ${security_group} and ${$i} -> ${i}
Additionally, missing { before GREEN, should be ${GREEN}

Copy link
Author

Choose a reason for hiding this comment

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

done

$(aws ec2 modify-network-interface-attribute "${awscli_args[@]}" --network-interface-id $i --groups $existing_sg $security_group)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the command substitution here?
We just executing the command, right? We don't need to capture the output?

shouldn't be below instead?:
aws ec2 modify-network-interface-attribute "${awscli_args[@]}" --network-interface-id $i --groups $existing_sg $security_group

Copy link
Author

Choose a reason for hiding this comment

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

Either work, but you are right. I'm not capturing the output; I care about the exit signal only.


[[ $? -ne 0 ]] && { echo "Failed adding $security_group to ENI $i"; exit -1; }
done