cleanup_ci doesn't clean up auto-deploy IAM policy members #543

gabrielwen · 2019-12-12T18:58:56Z

thus we hit quota limit every week and have to manually clean it up.

zhenghuiwang · 2019-12-12T23:34:49Z

@gabrielwen can you give a link to an example of auto-deploy IAM policy members that should be GCed?

gabrielwen · 2019-12-12T23:45:13Z

an example looks like this: kf-vmaster-1211-099-user@kubeflow-ci-deployment.iam.gserviceaccount.com

this might not necessarily needs to be purged as it might not be expired. that said, I think for auto-deployed accounts, we should simply check whether the deployment is still there and delete them if they are not.

zhenghuiwang · 2019-12-13T00:55:01Z

Thanks. Having more questions about the context of kfmaster auto deployment, because I think the cleanup script is mainly for cleaning up short-lived testing resource. Is the goal of 'kf-master' to periodically deploy master? How often? Can 'kf-master' completely delete previous deployment with all service accounts and then deploy new version?

…

On Thu, Dec 12, 2019 at 15:45 Hung-Ting Wen ***@***.***> wrote: an example looks like this: ***@***.*** this might not necessarily needs to be purged as it might not be expired. that said, I think for auto-deployed accounts, we should simply check whether the deployment is still there and delete them if they are not. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#543?email_source=notifications&email_token=AB7BSKIVK7L6E4S6SKMFMHDQYLEIVA5CNFSM4J2C7UW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGYNOSY#issuecomment-565237579>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7BSKO3B75UWRLWIIWVKE3QYLEIVANCNFSM4J2C7UWQ> .

gabrielwen · 2019-12-16T18:09:08Z

Thanks. Having more questions about the context of kfmaster auto deployment, because I think the cleanup script is mainly for cleaning up short-lived testing resource. Is the goal of 'kf-master' to periodically deploy master? How often? Can 'kf-master' completely delete previous deployment with all service accounts and then deploy new version?
…
On Thu, Dec 12, 2019 at 15:45 Hung-Ting Wen @.> wrote: an example looks like this: @. this might not necessarily needs to be purged as it might not be expired. that said, I think for auto-deployed accounts, we should simply check whether the deployment is still there and delete them if they are not. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#543?email_source=notifications&email_token=AB7BSKIVK7L6E4S6SKMFMHDQYLEIVA5CNFSM4J2C7UW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGYNOSY#issuecomment-565237579>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB7BSKO3B75UWRLWIIWVKE3QYLEIVANCNFSM4J2C7UWQ .

seems it's every 12 hours currently: link
it appears that we have logic in gcp.Delete to clean up IAM policy:
https://github.com/kubeflow/kfctl/blob/335033bbc5d86fcf016fdbb372e8a142ea031cdf/pkg/kfapp/gcp/gcp.go#L1059
somehow kf-master job doesn't clean it up, but this should be a separate issue: why presubmits can clean IAM policy but not the auto-deploy jobs?

kunmingg · 2020-01-21T19:18:51Z

Clean up should include IAM bindings in project "kubeflow-ci-deployment"
@jlewi @richardsliu

jlewi · 2020-01-21T19:20:48Z

We are hitting the limit again each kubeflow/pipelines#712

Here is the current IAM policy
kubeflow.ci.deployment.policy.txt

It looks like the memberships that aren't being GC'd are of the form.

  - serviceAccount:kf-vmaster-0121-b11-user@kubeflow-ci-deployment.iam.gserviceaccount.com
  - serviceAccount:kf-vmaster-0121-ec5-user@kubeflow-ci-deployment.iam.gserviceaccount.com
  - serviceAccount:kf-vmaster-0121-fc4-user@kubeflow-ci-deployment.iam.gserviceaccount.com

jlewi · 2020-01-21T19:26:28Z

The logic to cleanup service accounts is here:

testing/py/kubeflow/testing/cleanup_ci.py

Line 782 in aabd75f

def cleanup_service_account_bindings(args):

The logic is to trim unused bindings for any service accounts that don't exist.
So the bindings likely aren't deleted because the service accounts aren't GC'd.

Here's the logic to GC service accounts

testing/py/kubeflow/testing/cleanup_ci.py

Line 782 in aabd75f

def cleanup_service_account_bindings(args):

My suspicion is that the regex for the AutoDeploy patterns is not matching these names and so the emails are skipped.

testing/py/kubeflow/testing/cleanup_ci.py

Line 23 in aabd75f

AUTO_DEPLOY_PATTERNS = [re.compile(r".*kf-vmaster-(?!n\d\d)")]

jlewi · 2020-01-21T20:10:30Z

Here's a list of service accounts currently in project: kubeflow-ci-deployment

serviceaccounts.list.txt

And here's the policy bindings

kubeflow.ci.deployment.policy.txt

It looks like the policy binding includes bindings for service accounts which don't exist and should have been GC'd.

e.g.

  - serviceAccount:kf-vmaster-0102-bef-user@kubeflow-ci-deployment.iam.gserviceaccount.com
  - serviceAccount:kf-vmaster-0102-daf-user@kubeflow-ci-deployment.iam.gserviceaccount.com
  - serviceAccount:kf-vmaster-0102-dcb-user@kubeflow-ci-deployment.iam.gserviceaccount.com
  - serviceAccount:kf-vmaster-0102-f2e-user@kubeflow-ci-deployment.iam.gserviceaccount.com

So it looks like there might be a bug in the cleanup logic which should be removing bindings for non existent service accounts.

jlewi · 2020-01-21T20:17:22Z

Here are the logs from the most recent run of the cleanup ci script.
cleanup-ci-kubeflow-ci-deployment-1579629600-zlrtq.log.txt

It looks like the code to clean up bindings is invoked and no exceptions are reported but it also isn't printing out any info that would help us debug.

jlewi · 2020-01-21T20:21:57Z

I think the bug is here:

testing/py/kubeflow/testing/cleanup_ci.py

Line 770 in aabd75f

if (not is_match(accountEmail)) or (accountEmail in accounts):

I don't think we need or want to check whether the pattern is_match. That check likely isn't matching the service accounts for our auto infra. The check that the service accounts exist should be sufficient.

* We are not properly GC'ing policy bindings for deleted service accounts. * The problem is that we only consider service accounts matching a certain regex and that regex isn't matching service accounts for our auto-deployed clusters. * Using a regex should be unnecessary. If a service account doesn't exist that should be a sufficient criterion that the policy bindings should be deleted. Related to: kubeflow#543

jlewi · 2020-01-21T20:46:06Z

Here are the logs from a one off run.
oneoff-run.txt

It looks like a bunch of old bindings were GC'd.

Here's the current policy.

kubeflow.ci.deployment.policy.txt

Looks like a bunch of bindings were pruned.

I think we can close this once #566 is merged.

* Fix cleanup logic for IAM policy bindings * We are not properly GC'ing policy bindings for deleted service accounts. * The problem is that we only consider service accounts matching a certain regex and that regex isn't matching service accounts for our auto-deployed clusters. * Using a regex should be unnecessary. If a service account doesn't exist that should be a sufficient criterion that the policy bindings should be deleted. Related to: #543 * Fix typo. * Fix syntax issue.

jlewi · 2020-01-21T23:45:30Z

Looks like I was overzealous and we deleted bindings for the service accounts created by GCP services.

We will need to restore the IAM policies.

Here's the policy for kubeflow-ci
kubeflow.ci.policy.txt

The policy for kubeflow-ci-deployment is available in the previous comment
#543 (comment)

jlewi · 2020-01-21T23:54:12Z

Going to restore the IAM policy for kubeflow ci

Here's the current policy before the restore
kubeflow-ci.20200121-155208.yaml.txt

jlewi · 2020-01-21T23:58:59Z

I rolled back the policy for kubeflow-ci;

here's the current policy
kubeflow-ci.20200121-155750.yaml.txt

jlewi · 2020-01-22T01:37:43Z

I rolled back the policy for kubeflow-ci-deployment

Prior to the rollback the policy was
kubeflow-ci-deployment.20200121-173215.yaml.txt

I set the policy to
kubeflow.ci.deployment.policy.restored.txt

This was necessary to restore the policy bindings to grant access to code running in the test cluster in kubeflow-ci to the clusters in kubeflow-ci-deployment.

This is the script I used to generate the new policy from the policy in
#543 (comment)

https://github.com/jlewi/testing/blob/cleanup_bindings_restore/py/kubeflow/testing/fix_policy.py

jlewi · 2020-01-22T02:45:42Z

As a result of
#543 (comment)

Tests would fail with permissions errors like this one

util.py                     72 INFO     time="2020-01-21T21:07:48Z" level=info msg="Creating deployment kfctl-23b8-storage" filename="gcp/gcp.go:462"
util.py                     72 INFO     Error: failed to apply:  (kubeflow.error): Code 500 with message: coordinator Apply failed for gcp:  (kubeflow.error): Code 500 with message: gcp apply could not update deployment manager Error could not update s
torage-kubeflow.yaml; Insert deployment error: googleapi: Error 403: Required 'deploymentmanager.deployments.create' permission for 'projects/kubeflow-ci-deployment/global/deployments/kfctl-23b8-storage', forbidden

From
https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubeflow_manifests/707/kubeflow-manifests-presubmit/1219724656517320704/

Hopefully this is fixed now. The test for kubeflow/manifests#707 is now running again so we will see.

stale · 2020-04-21T03:19:07Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions.

stale · 2020-04-28T03:48:18Z

This issue has been closed due to inactivity.

gabrielwen added good first issue Good for newcomers area/testing priority/p0 kind/bug effort/1-days labels Dec 12, 2019

jlewi mentioned this issue Jan 21, 2020

Add istio sidecar inject false to deployments in kubeflow ns kubeflow/manifests#712

Merged

1 task

jlewi mentioned this issue Jan 21, 2020

Fix cleanup logic for IAM policy bindings #566

Merged

stale bot added the lifecycle/stale label Apr 21, 2020

stale bot closed this as completed Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup_ci doesn't clean up auto-deploy IAM policy members #543

cleanup_ci doesn't clean up auto-deploy IAM policy members #543

gabrielwen commented Dec 12, 2019

zhenghuiwang commented Dec 12, 2019

gabrielwen commented Dec 12, 2019

zhenghuiwang commented Dec 13, 2019 via email

gabrielwen commented Dec 16, 2019

kunmingg commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 22, 2020

jlewi commented Jan 22, 2020

stale bot commented Apr 21, 2020

stale bot commented Apr 28, 2020

cleanup_ci doesn't clean up auto-deploy IAM policy members #543

cleanup_ci doesn't clean up auto-deploy IAM policy members #543

Comments

gabrielwen commented Dec 12, 2019

zhenghuiwang commented Dec 12, 2019

gabrielwen commented Dec 12, 2019

zhenghuiwang commented Dec 13, 2019 via email

gabrielwen commented Dec 16, 2019

kunmingg commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 21, 2020

jlewi commented Jan 22, 2020

jlewi commented Jan 22, 2020

stale bot commented Apr 21, 2020

stale bot commented Apr 28, 2020