clean up unused bindings in iam (kubeflow#319)

* clean up unused bindings in iam

* fix

* request body change

* address comment

* delete unused members for each binding

* fix pylint

* add unit test for trim bindings logic

py/kubeflow/testing/cleanup_ci.py

@@ -201,19 +201,18 @@ def cleanup_service_accounts(args):
 
     keys = keys_client.list(name=a["name"]).execute()
 
-    is_expired = False
+    is_expired = True
     for k in keys["keys"]:
       valid_time = date_parser.parse(k["validAfterTime"])
       now = datetime.datetime.now(valid_time.tzinfo)
 
       age = now - valid_time
-      if age > datetime.timedelta(hours=args.max_age_hours):
-        logging.info("Deleting account: %s", a["email"])
-        iam.projects().serviceAccounts().delete(name=a["name"]).execute()
-        is_expired = True
+      if age < datetime.timedelta(hours=args.max_age_hours):
+        is_expired = False
         break
-
     if is_expired:
+      logging.info("Deleting account: %s", a["email"])
+      iam.projects().serviceAccounts().delete(name=a["name"]).execute()
       expired_emails.append(a["email"])
     else:
       unexpired_emails.append(a["email"])
@@ -222,6 +221,50 @@ def cleanup_service_accounts(args):
   logging.info("Unexpired emails:\n%s", "\n".join(unexpired_emails))
   logging.info("expired emails:\n%s", "\n".join(expired_emails))
 
+
+def trim_unused_bindings(iamPolicy, accounts):
+  keepBindings = []
+  for binding in iamPolicy['bindings']:
+    members_to_keep = []
+    members_to_delete = []
+    for member in binding['members']:
+      if not member.startswith('serviceAccount:'):
+        members_to_keep.append(member)
+      else:
+        accountEmail = member[15:]
+        if (not is_match(accountEmail)) or (accountEmail in accounts):
+          members_to_keep.append(member)
+        else:
+          members_to_delete.append(member)
+    if members_to_keep:
+      binding['members'] = members_to_keep
+      keepBindings.append(binding)
+    if members_to_delete:
+      logging.info("Delete binding for members:\n%s", ", ".join(members_to_delete))
+  iamPolicy['bindings'] = keepBindings
+
+def cleanup_service_account_bindings(args):
+  credentials = GoogleCredentials.get_application_default()
+  iam = discovery.build('iam', 'v1', credentials=credentials)
+  accounts = []
+  next_page_token = None
+  while True:
+    service_accounts = iam.projects().serviceAccounts().list(
+      name='projects/' + args.project, pageToken=next_page_token).execute()
+    for a in service_accounts["accounts"]:
+      accounts.append(a["email"])
+    if not "nextPageToken" in service_accounts:
+      break
+    next_page_token = service_accounts["nextPageToken"]
+
+  resourcemanager = discovery.build('cloudresourcemanager', 'v1', credentials=credentials)
+  iamPolicy = resourcemanager.projects().getIamPolicy(resource=args.project).execute()
+  trim_unused_bindings(iamPolicy, accounts)
+
+  setBody = {'policy': iamPolicy}
+  resourcemanager.projects().setIamPolicy(resource=args.project, body=setBody).execute()
+
+
 def getAge(tsInRFC3339):
   # The docs say insert time will be in RFC339 format.
   # But it looks like it also includes a time zone offset in the form
@@ -353,6 +396,7 @@ def cleanup_all(args):
   cleanup_deployments(args)
   cleanup_endpoints(args)
   cleanup_service_accounts(args)
+  cleanup_service_account_bindings(args)
   cleanup_workflows(args)
   cleanup_disks(args)
 

py/kubeflow/tests/cleanup_ci_test.py

@@ -0,0 +1,38 @@
+from __future__ import print_function
+
+import unittest
+
+from kubeflow.testing import cleanup_ci
+import os
+import yaml
+
+class CleanupCiTest(unittest.TestCase):
+  def setUp(self):
+    self.test_dir = os.path.join(os.path.dirname(__file__), "test-data")
+
+  def test_wait_for_workflow(self):
+    with open(os.path.join(self.test_dir, "iam_bindings.yaml")) as bd:
+      iam_bindings = yaml.load(bd)
+      self.assertTrue(len(iam_bindings['bindings']) == 3)
+      members = iam_bindings['bindings'][0]['members']
+      for act in ['my-other-app@appspot.gserviceaccount.com',
+                  'kfctl-in-use@kubeflow-ci.iam.gserviceaccount.com',
+                  'kfctl-expired@kubeflow-ci.iam.gserviceaccount.com']:
+        self.assertTrue('serviceAccount:' + act in members)
+      cleanup_ci.trim_unused_bindings(iam_bindings,
+                                      ['kfctl-in-use@kubeflow-ci.iam.gserviceaccount.com'])
+      # One binding is deleted as it lost all members.
+      self.assertTrue(len(iam_bindings['bindings']) == 2)
+      trimed_members = iam_bindings['bindings'][0]['members']
+      # binding to non-match service account still exists as it is not created by ci tests.
+      self.assertTrue(
+        'serviceAccount:my-other-app@appspot.gserviceaccount.com'in trimed_members)
+      # binding to existing service account still exists.
+      self.assertTrue(
+        'serviceAccount:kfctl-in-use@kubeflow-ci.iam.gserviceaccount.com' in trimed_members)
+      # binding to unexist service account is deleted.
+      self.assertTrue(
+        'serviceAccount:kfctl-expired@kubeflow-ci.iam.gserviceaccount.com' not in trimed_members)
+
+if __name__ == "__main__":
+  unittest.main()

py/kubeflow/tests/test-data/iam_bindings.yaml

@@ -0,0 +1,15 @@
+bindings:
+- members:
+  - user:mike@example.com
+  - group:admins@example.com
+  - domain:google.com
+  - serviceAccount:my-other-app@appspot.gserviceaccount.com
+  - serviceAccount:kfctl-in-use@kubeflow-ci.iam.gserviceaccount.com
+  - serviceAccount:kfctl-expired@kubeflow-ci.iam.gserviceaccount.com
+  role: roles/owner
+- members:
+  - user:mike@example.com
+  role: roles/viewer
+- members:
+  - serviceAccount:kfctl-expired@kubeflow-ci.iam.gserviceaccount.com
+  role: roles/editor