IOPZ-1445 Make create-k8s-chained-sessions.sh idempotent #11

JacobEvelyn · 2023-12-11T17:04:18Z

This commit makes the create-k8s-chained-sessions.sh script idempotent (it no longer breaks or behaves weirdly if some or all chained sessions already exist). In addition, it tidies up some aspects of the script, removes unnecessary output, and makes improvements so the script now passes the ShellCheck linter.

JacobEvelyn · 2023-12-11T17:04:36Z

create-k8s-chained-sessions.sh

@@ -8,7 +8,6 @@

 # global variables
 declare PROFILE_ID
-declare CHAINED_SESSION_IDS="name,id\n"


I don't think we ever used this output so I removed it.

I'm fine with removing it if you want, but I will say I put this in here as a convenience output to transpose specific session ids to my .zshenv so I can more easily start specific sessions. That might be references in panopedia as something you can do as well

I double checked panopedia and couldn't find any references to this in the active pages we have, so we should be fine there

JacobEvelyn · 2023-12-11T17:05:41Z

create-k8s-chained-sessions.sh

@@ -20,37 +19,53 @@ declare REGION='us-east-1'
 function createLeappSession {
    parent_session_name=$1
    chained_session_name="chained-from-${parent_session_name}"
-    echo "starting session for ${parent_session_name} to get role arn"
-    # this has funky piping because `--filter` is a fuzzy lookup and `panorama-k8s-playground` fuzzy matches `panorama-k8s-playground-2` as the first result


I learned in school-supplies that you can use regex anchors (e.g. ^ and $) in --filter, so I moved this to a helper function and used that instead of sort | sed | awk.

Pat-Ayres

I ran the script initially with my full setup and it correctly did not attempt to create any new chained sessions.

However, I deleted all my chained sessions and ran the script and it no longer creates new chained sessions. I get the following output for each account

looking for existing session chained-from-panorama-k8s-playground
no existing session found; starting session for panorama-k8s-playground to get role arn
 ›   Error: No sessions found

Unable to locate credentials. You can configure credentials by running "aws configure".
 ›   Error: No sessions found
creating new profile
 ›   Error: Profile already exists
creating new session

When the script completed, I checked leapp and had no chained sessions still.

The final test I ran was to delete all the profiles and re-run the script to get a full (I think) blank slate run. This correctly created each profile, but again did not successfully create sessions.

I believe the reason it fails now is because we had Security squad add additional SSO roles to each k8s account (PanoramaK8sEngineeringDefault and EmergencyOnly were added in addition to the existing AWSAdministratorAccess). Unfortunately, Leapp doesn't allow filtering by multiple columns so I think we will need to continue doing some form of grepping to get the leapp profile with the appropriate role. Another option could be to restructure the session names to guarantee uniqueness, but I'm not sure how that syncing works so we'd have to figure that out, or ask security squad/#leapp-discussion for some assistance.

Pat-Ayres · 2023-12-11T18:12:49Z

create-k8s-chained-sessions.sh

@@ -61,8 +76,5 @@ PARENT_SESSION_NAMES="panorama-k8s-playground panorama-k8s-playground-2 panorama

 for session in $PARENT_SESSION_NAMES


Could you remove panorama-k8s-integration from the PARENT_SESSION_NAMES array as part of this? that env no longer exists.

Pat-Ayres · 2023-12-11T18:15:52Z

create-k8s-chained-sessions.sh

 }

 # function to create a leapp profile to associate with the chained k8s sessions
 # stores the new profile id in PROFILE_ID
 function createLeappProfile {
+    # The ^ and $ in the session filter are regex anchors to ensure we don't


This comment mentions the reg-ex anchors, but I don't see them used in the code directly below it within this function. Could that either be added into profile_name or the comment moved directly above the PROFILE_ID assignment? Not sure if it's a huge deal, it might just be confusing because I'm looking at it in the diff with the removed code as well...

Pat-Ayres · 2023-12-11T18:20:26Z

create-k8s-chained-sessions.sh

@@ -8,7 +8,6 @@

 # global variables
 declare PROFILE_ID
-declare CHAINED_SESSION_IDS="name,id\n"


I double checked panopedia and couldn't find any references to this in the active pages we have, so we should be fine there

I have now contributed changes per my review

parkerfinch · 2023-12-14T16:35:01Z

create-k8s-chained-sessions.sh

 # Do not run this script more than once without resetting your Leapp
 # instance beforehand.


Does this warning still apply now that this is idempotent?

No, I can remove it.

parkerfinch · 2023-12-14T16:40:36Z

create-k8s-chained-sessions.sh

-    # stop the leapp session
-    leapp session stop --sessionId $parent_session_id
+    parent_role_name=$2
+    chained_role_name=$3


Can this argument be added to the documentation list above the function?

parkerfinch · 2023-12-14T16:43:50Z

create-k8s-chained-sessions.sh

+    # check if the parent session exists for the role. We do this because
+    # regular developers won't have the AWSAdministratorAccess role, so we
+    # don't want to create a chained session for them.
+    parent_session_id=$(leappSessionId "$parent_session_name" "$parent_role_name")
+    if [[ -z "${parent_session_id}" ]]; then


What's the relationship between the parent session and the AWSAdministratrAccess role?

Are the non-admin sessions not chained, meaning only admins would ever need to run this?

Regular developers will have the parent session for PanoramaK8sDefaultEngineering role, AWSAdministratorAccess is an example of a case where the parent doesn't exist, so we just skip it.

Ah is it just to avoid erroring out when folks without access to that role run the script?

parkerfinch · 2023-12-14T16:53:30Z

create-k8s-chained-sessions.sh

-    PROFILE_ID=$(leapp profile list --columns=ID --filter="Profile Name=${profile_name}" --no-header)
+    profile_id=$(leapp profile list -x --output json --filter="Profile Name=^${profile_name}$" | jq -r '.[].id')
+    if [[ -n "${profile_id}" ]]; then
+        echo "${profile_id}"


Does this just output a UUID? I'm wondering if it would be clearer if it was something like:

echo "Profile ${profile_name} already exists with id: ${profile_id}"

It doesn't actually output anything, echo is used here to return the value of the existing profile id from the function so we can use it in the session creation.

This commit makes the `create-k8s-chained-sessions.sh` script idempotent (it no longer breaks or behaves weirdly if some or all chained sessions already exist). In addition, it tidies up some aspects of the script, removes unnecessary output, and makes improvements so the script now passes the ShellCheck linter. fixed a bug in leapp filtering now that there are multiple sso sessions with the same name per account. Also updated the script to create chained sessions for each of the k8s developer "profiles" we have i.e. "reader", "writer", and "admin" depending on what base sso roles the user has. fixed bugs in profile lookup. Removed integration env refactored profile creation cleaned up log output

JacobEvelyn commented Dec 11, 2023

View reviewed changes

JacobEvelyn force-pushed the IOPZ-1445/idempotent-leapp-setup branch 3 times, most recently from 6238f1e to b24e855 Compare December 11, 2023 17:08

Pat-Ayres previously requested changes Dec 11, 2023

View reviewed changes

Pat-Ayres force-pushed the IOPZ-1445/idempotent-leapp-setup branch from be8481a to 0863048 Compare December 13, 2023 15:51

Pat-Ayres force-pushed the IOPZ-1445/idempotent-leapp-setup branch from 3019979 to 662e671 Compare December 14, 2023 16:24

parkerfinch reviewed Dec 14, 2023

View reviewed changes

parkerfinch approved these changes Dec 14, 2023

View reviewed changes

Pat-Ayres force-pushed the IOPZ-1445/idempotent-leapp-setup branch from 5247990 to d39a6ec Compare December 14, 2023 17:50

Pat-Ayres merged commit 45ccd89 into main Dec 14, 2023
2 checks passed

Pat-Ayres deleted the IOPZ-1445/idempotent-leapp-setup branch December 14, 2023 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IOPZ-1445 Make create-k8s-chained-sessions.sh idempotent #11

IOPZ-1445 Make create-k8s-chained-sessions.sh idempotent #11

JacobEvelyn commented Dec 11, 2023 •

edited

Loading

JacobEvelyn Dec 11, 2023

Pat-Ayres Dec 11, 2023

Pat-Ayres Dec 11, 2023

JacobEvelyn Dec 11, 2023

Pat-Ayres left a comment

Pat-Ayres Dec 11, 2023

Pat-Ayres Dec 11, 2023

Pat-Ayres Dec 11, 2023

parkerfinch Dec 14, 2023

Pat-Ayres Dec 14, 2023

parkerfinch Dec 14, 2023

parkerfinch Dec 14, 2023

Pat-Ayres Dec 14, 2023

parkerfinch Dec 14, 2023

parkerfinch Dec 14, 2023

Pat-Ayres Dec 14, 2023

		@@ -61,8 +76,5 @@ PARENT_SESSION_NAMES="panorama-k8s-playground panorama-k8s-playground-2 panorama

		for session in $PARENT_SESSION_NAMES

		# Do not run this script more than once without resetting your Leapp
		# instance beforehand.

IOPZ-1445 Make create-k8s-chained-sessions.sh idempotent #11

IOPZ-1445 Make create-k8s-chained-sessions.sh idempotent #11

Conversation

JacobEvelyn commented Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pat-Ayres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobEvelyn commented Dec 11, 2023 •

edited

Loading