Skip to content
This repository has been archived by the owner on Feb 15, 2022. It is now read-only.

add ability to open up API server for flux when AKS configured to use it #1439

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jmspring
Copy link
Contributor

@jmspring jmspring commented Sep 7, 2020

No description provided.

Copy link

@nmiodice nmiodice left a comment

Choose a reason for hiding this comment

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

Can you help me understand how the coordination happens with the flux module? I.e., the sequence between open API server, do fluxy stuff, close API server?

;;
s)
IP_LIST=$OPTARG
USE_IP_LIST=1
Copy link

Choose a reason for hiding this comment

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

nit: spacing

CLUSTER_NAME=$OPTARG
;;
g)
RESOURCE_GROUP=$OPTARG
Copy link

Choose a reason for hiding this comment

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

I suppose all these are already guaranteed to be interpreted as a single argument, so no need to use $OPTARG? Nice!

# handle case where we are working with a single IP address
if [ -z "$IP" ]; then
IP=`curl -s https://ipchicken.com | egrep -o '([[:digit:]]{1,3}\.){3}[[:digit:]]{1,3}' | sort -u`
IP="$IP/32"
Copy link

Choose a reason for hiding this comment

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

nit: spacing

fi

# current IP address LIST
CURRENT_IP_ADDRESS_LIST=`az aks show -g jms-tst1-rg -n jmsfxclus | jq -c -r '.apiServerAccessProfile.authorizedIpRanges' | sed 's/\]//' | sed 's/\[//' | sed 's/"//g' | sed 's/,/ /g'`
Copy link

Choose a reason for hiding this comment

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

Suggested change
CURRENT_IP_ADDRESS_LIST=`az aks show -g jms-tst1-rg -n jmsfxclus | jq -c -r '.apiServerAccessProfile.authorizedIpRanges' | sed 's/\]//' | sed 's/\[//' | sed 's/"//g' | sed 's/,/ /g'`
CURRENT_IP_ADDRESS_LIST=`az aks show -g "$RESOURCE_GROUP" -n "$CLUSTER_NAME" | jq -c -r '.apiServerAccessProfile.authorizedIpRanges' | sed 's/\]//' | sed 's/\[//' | sed 's/"//g' | sed 's/,/ /g'`

Copy link

Choose a reason for hiding this comment

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

It would also be good to use --query instead of jq to reduce the dependencies of this script. The query syntax should be the same

fi

# update the list
az aks update --resource-group $RESOURCE_GROUP --name $CLUSTER_NAME --api-server-authorized-ip-ranges "$UPDATED_IP_ADDRESS_LIST" > /dev/null
Copy link

Choose a reason for hiding this comment

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

Suggested change
az aks update --resource-group $RESOURCE_GROUP --name $CLUSTER_NAME --api-server-authorized-ip-ranges "$UPDATED_IP_ADDRESS_LIST" > /dev/null
az aks update --resource-group "$RESOURCE_GROUP" --name "$CLUSTER_NAME" --api-server-authorized-ip-ranges "$UPDATED_IP_ADDRESS_LIST" > /dev/null

Copy link

Choose a reason for hiding this comment

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

Any reason to discard the commands output? Might be helpful for debugging if things go awry

command = "${local.api_access_script} ${local.close_api_server_access_args}"
}

triggers = {
Copy link

Choose a reason for hiding this comment

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

There may be a strategy to dynamically pass the triggers as a list variable, so that consumers of this can just pass the triggers in manually.

This will help an explosion in variables when we need to, for example, upgrade flux or do some other operation that is not represented by these 2 input vars.

Thoughts?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants