Feature/setup project existing sa #87

ehmm · 2018-08-15T21:07:57Z

Allowing use of existing sa for deploy-project/driver and checking that sa has needed roles.

k8s-ci-robot · 2018-08-15T21:07:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

msau42 · 2018-08-15T21:12:01Z

/ok-to-test
/assign @davidz627

davidz627

Thanks for your PR! Just a couple suggestions and comments. Also please sign the CLA!

davidz627 · 2018-08-15T21:21:08Z

deploy/kubernetes/deploy-driver.sh

@@ -15,6 +15,30 @@ set -o errexit

 readonly PKGDIR="${GOPATH}/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver"
 readonly KUBEDEPLOY="${PKGDIR}/deploy/kubernetes"
+#readonly BIND_ROLES="roles/compute.storageAdmin roles/iam.serviceAccountUser projects/${PROJECT}/roles/gcp_compute_persistent_disk_csi_driver_custom_role"
+
+function check_service_account()


can you move the function definition to the bottom of the file

davidz627 · 2018-08-15T21:21:29Z

deploy/kubernetes/deploy-driver.sh

@@ -15,6 +15,30 @@ set -o errexit

 readonly PKGDIR="${GOPATH}/src/sigs.k8s.io/gcp-compute-persistent-disk-csi-driver"
 readonly KUBEDEPLOY="${PKGDIR}/deploy/kubernetes"
+#readonly BIND_ROLES="roles/compute.storageAdmin roles/iam.serviceAccountUser projects/${PROJECT}/roles/gcp_compute_persistent_disk_csi_driver_custom_role"


why did this get moved? can it be moved back?

We need to first extract the project sadly

davidz627 · 2018-08-15T21:27:50Z

deploy/kubernetes/deploy-driver.sh

+function check_service_account()
+{
+	# Using bash magic to parse JSON for IAM
+	IAM_NAME=$(grep -Po '"client_email": *\K"[^"]*"' ${GCE_PD_SA_DIR}/cloud-sa.json | tr -d '"')


alternatively:
jq '.client_email' ${GCE_PD_SA_DIR}/cloud-sa.json | tr -d '"'
I'm not sure if every system has jq installed but mine certainly does and I would prefer using a JSON parser for JSON over bash magic :)

Two of my standard dev machines don't have jq by default, that's why I offered to convert to python (can also be useful for Windows K8S deployments).
I also don't have a problem converting to jq and adding a message if it is missing if you prefer

davidz627 · 2018-08-15T21:29:21Z

deploy/kubernetes/deploy-driver.sh

+		fi
+	done
+	if [ "$MISSING_ROLES" = true ]; then
+		echo Cannot deploy with missing roles in service account, stopping


print out suggestion to use setup-project.sh to remedy this issue

davidz627 · 2018-08-15T21:30:28Z

deploy/setup-project.sh

-  gcloud iam roles create gcp_compute_persistent_disk_csi_driver_custom_role --quiet \
-    --project "${PROJECT}"                                                           \
-    --file "${PKGDIR}/deploy/gcp-compute-persistent-disk-csi-driver-custom-role.yaml"
+function create_new_sa()


move function definitions to bottom of file

Undid the extraction to a function as bash needs functions before usage, but it's not so bad inline

davidz627 · 2018-08-15T21:32:54Z

deploy/setup-project.sh

+
+# Check if SA exists
+CREATE_SA=true
+SA_JSON=$(gcloud iam service-accounts list --filter="name:$IAM_NAME" --format="json")


could we just do a get instead of a list and filter since we're only looking for one?

I personally prefer this as trying to describe (get) instead of list prints an error to stdout if it fails, but I can still change it if you prefer

davidz627 · 2018-08-15T21:35:55Z

deploy/setup-project.sh

+	# Delete Service Account
+	gcloud iam service-accounts delete "${IAM_NAME}" --project "${PROJECT}" --quiet || true
+
+	# Create new Service Account and Keys


nit: Create new Service Account

davidz627

just a bunch of nits

davidz627 · 2018-08-15T22:42:24Z

deploy/kubernetes/deploy-driver.sh

+		fi
+	done
+	if [ "$MISSING_ROLES" = true ]; then
+		echo Cannot deploy with missing roles in service account, please run setup-project.sh to setup Service Account


nit: quotes around strings printed by echo

davidz627 · 2018-08-15T22:45:34Z

deploy/kubernetes/deploy-driver.sh

+	# Using bash magic to parse JSON for IAM
+	IAM_NAME=$(grep -Po '"client_email": *\K"[^"]*"' ${GCE_PD_SA_DIR}/cloud-sa.json | tr -d '"')
+	PROJECT=$(grep -Po '.*@\K[^.]+'<<<$IAM_NAME)
+	GOTTEN_BIND_ROLES=$(gcloud projects get-iam-policy $PROJECT --flatten="bindings[].members" --format='table(bindings.role)' --filter="bindings.members:$IAM_NAME")


nit: all of these variables are readonly. I don't really know bash style guidelines but I'd like it to be at least internally consistent.

davidz627 · 2018-08-15T22:46:11Z

deploy/kubernetes/deploy-driver.sh

+function check_service_account()
+{
+	# Using bash magic to parse JSON for IAM
+	IAM_NAME=$(grep -Po '"client_email": *\K"[^"]*"' ${GCE_PD_SA_DIR}/cloud-sa.json | tr -d '"')


could you document in a comment what this Regex is doing approximately

davidz627 · 2018-08-15T22:47:16Z

deploy/kubernetes/deploy-driver.sh

+	GOTTEN_BIND_ROLES=$(gcloud projects get-iam-policy $PROJECT --flatten="bindings[].members" --format='table(bindings.role)' --filter="bindings.members:$IAM_NAME")
+	readonly BIND_ROLES="roles/compute.storageAdmin roles/iam.serviceAccountUser projects/${PROJECT}/roles/gcp_compute_persistent_disk_csi_driver_custom_role"
+	MISSING_ROLES=false
+	for role in $BIND_ROLES 


nit: all variables in "${var}" form for consistency. I think the quotes prevent/fix some sort of multi-part variable issues

davidz627 · 2018-08-15T22:48:58Z

deploy/setup-project.sh

+	CREATE_SA=false
+	echo "Service account $IAM_NAME exists. Would you like to create a new one (y) or reuse the existing one (n)"
+	read -p "(y/n)" -n 1 -r REPLY
+    echo    # (optional) move to a new line


nit: indentation

davidz627 · 2018-08-15T22:49:19Z

deploy/setup-project.sh

+# Check if SA exists
+CREATE_SA=true
+SA_JSON=$(gcloud iam service-accounts list --filter="name:$IAM_NAME" --format="json")
+if [ "[]" != "$SA_JSON" ]; then


nit: then on new-line for file consistency
or fix the other one to be on the same line

davidz627 · 2018-08-15T22:49:56Z

deploy/setup-project.sh

-# Create new Service Account and Keys
-gcloud iam service-accounts create "${GCE_PD_SA_NAME}" --project "${PROJECT}"
+# Bind service account to roles
+


nit: remove space

davidz627 · 2018-08-15T23:15:13Z

/lgtm
/hold
Please sign CLA and Squash commits.
Thanks again for the PR! And you're right, these bash scripts are getting really elaborate. We might want to invest in converting all of this to Python at some point in the future (#89).

Check that service account has needed roles

davidz627 · 2018-08-15T23:57:41Z

/hold cancel
/lgtm

davidz627 · 2018-08-16T20:00:11Z

/approve

davidz627 · 2018-08-16T20:00:32Z

/approve

k8s-ci-robot · 2018-08-16T20:00:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, ehmm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [davidz627]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

davidz627 · 2018-08-16T21:33:37Z

Thanks @ehmm for jumping on this and submitting this PR! Feel free to reach out or open issues if you run into any other bugs or opportunities for improvement with the driver 👍

ehmm · 2018-08-16T22:30:04Z

Sure thing @davidz627, thank you and @msau42 for the support, I will be playing around with the driver and if I see anything will definitely reach out. Also I added a link for a quick patch I'm using to support specifying a specific partition (#83), it is probably a bit rough and I'm not sure it's the direction you want to go (re the specification format) -- but for my projects it's currently good enough.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 15, 2018

k8s-ci-robot requested review from msau42 and saad-ali August 15, 2018 21:08

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 15, 2018

k8s-ci-robot assigned davidz627 Aug 15, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 15, 2018

davidz627 reviewed Aug 15, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 15, 2018

davidz627 reviewed Aug 15, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 15, 2018

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 15, 2018

Fancier setup-project for existing service accounts

33681f9

Check that service account has needed roles

ehmm force-pushed the feature/setupProjectExistingSa branch from ae4c3b0 to 33681f9 Compare August 15, 2018 23:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2018

k8s-ci-robot merged commit bd240a4 into kubernetes-sigs:master Aug 16, 2018

davidz627 mentioned this pull request Aug 17, 2018

Permission problem using gcp-pd-csi: Required 'compute.disks.get' #85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/setup project existing sa #87

Feature/setup project existing sa #87

ehmm commented Aug 15, 2018

k8s-ci-robot commented Aug 15, 2018

msau42 commented Aug 15, 2018

davidz627 left a comment •

edited

Loading

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 Aug 15, 2018

ehmm Aug 15, 2018

davidz627 left a comment

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 Aug 15, 2018

davidz627 commented Aug 15, 2018 •

edited

Loading

davidz627 commented Aug 15, 2018

davidz627 commented Aug 16, 2018 •

edited

Loading

davidz627 commented Aug 16, 2018

k8s-ci-robot commented Aug 16, 2018

davidz627 commented Aug 16, 2018

ehmm commented Aug 16, 2018

Feature/setup project existing sa #87

Feature/setup project existing sa #87

Conversation

ehmm commented Aug 15, 2018

k8s-ci-robot commented Aug 15, 2018

msau42 commented Aug 15, 2018

davidz627 left a comment • 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

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

davidz627 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

davidz627 commented Aug 15, 2018 • edited Loading

davidz627 commented Aug 15, 2018

davidz627 commented Aug 16, 2018 • edited Loading

davidz627 commented Aug 16, 2018

k8s-ci-robot commented Aug 16, 2018

davidz627 commented Aug 16, 2018

ehmm commented Aug 16, 2018

davidz627 left a comment •

edited

Loading

davidz627 commented Aug 15, 2018 •

edited

Loading

davidz627 commented Aug 16, 2018 •

edited

Loading