-
Notifications
You must be signed in to change notification settings - Fork 827
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
audit: update as of 2021-03-30 #1800
Conversation
Hi @cncf-ci. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One expected change, one unexpected
"serviceAccount:[email protected]" | ||
], | ||
"role": "roles/storage.objectCreator" | ||
"role": "roles/storage.objectAdmin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
did someone make this change manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird! (not me!)
cc @saschagrunert @mrunalp any clues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aww, yes I changed it manually. Big sorry for that, our token needs access to write a version marker to the bucket. :-/ We need to change that file for each commit. Can we request an additional bucket where we're able to edit/change files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask what the version marker is for? These buckets should just be result dumps, just trying to understand the use case here.
OTOH since we're giving humans admin access I can't think why we wouldn't give their serviceaccount the same level of acess. I would be open to a PR that makes this the default for all k8s-conform buckets, WDYT @BenTheElder ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @BenTheElder
to put the above question on your radar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can I ask what the version marker is for? These buckets should just be result dumps, just trying to understand the use case here.
Yes sure, the main intention was to use this marker for being independent from the GitHub API. We publish a binary artifact for every successful run on the CRI-O master branch and update the version marker after that. This way we can easily query the latest build without having to use the rate limited GitHub Actions API. It's more or less the same like we do it in k8s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1850 to track following up on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I mistakenly deleted this account when trying to delete the capi-openstack serviceaccount below. I believe I restored it, but let me know if you see problems on your end @saschagrunert
$ gcloud iam service-accounts delete [email protected]
deleted service account [[email protected]]
$ gcloud beta iam service-accounts undelete 118310596454734433596
restoredAccount:
email: [email protected]
etag: MDEwMjE5MjA=
name: projects/k8s-conform/serviceAccounts/[email protected]
oauth2ClientId: REDACTED
projectId: k8s-conform
uniqueId: '118310596454734433596'
@@ -517,15 +517,27 @@ | |||
"deploymentmanager.typeProviders.list", | |||
"deploymentmanager.types.list", | |||
"dialogflow.agents.list", | |||
"dialogflow.answerrecords.list", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected, came from: #1794 (comment)
e21ce65
to
669d0b3
Compare
01767b4
to
66916db
Compare
0aa5a69
to
1d42984
Compare
3c037c9
to
ece5881
Compare
3589904
to
6a68661
Compare
fefd764
to
0fc358f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
/hold cancel
kubernetes-sigs/kubetest2#117 has landed which should hopefully mean no new random ssh keys being added. So I'm merging this to see if that proves true.
I've dropped comments / opened up followup issues for anything else in here that needs resolving.
If it does, I'll PR up something to either one-time nuke the e2e project ssh-keys, or reset the ssh-keys to what we expect every time ensure-e2e-projects.sh
is run
"group:[email protected]", | ||
"user:[email protected]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a pending PR that modifies ensure_project
to automatically remove user:*
bindings for roles/owner
, this was me testing it
@@ -1,6 +1,5 @@ | |||
NAME TITLE | |||
bigquery.googleapis.com BigQuery API | |||
bigquery.googleapis.com BigQuery API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we had a dupe entry here to begin with, not sure what caused it to get removed
"serviceAccount:[email protected]" | ||
], | ||
"role": "roles/storage.objectCreator" | ||
"role": "roles/storage.objectAdmin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1850 to track following up on this
{ | ||
"displayName": "service-capi-openstack", | ||
"email": "[email protected]", | ||
"name": "projects/k8s-conform/serviceAccounts/[email protected]", | ||
"oauth2ClientId": "115191210752954465501", | ||
"projectId": "k8s-conform", | ||
"uniqueId": "115191210752954465501" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be manually deleted per #1807
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ gcloud iam service-accounts delete [email protected]
deleted service account [[email protected]]
@@ -0,0 +1 @@ | |||
{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be manually deleted per #1807
"createTime": "2021-03-24T18:14:58.836Z", | ||
"lifecycleState": "ACTIVE", | ||
"name": "k8s-staging-kubetest2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All files under projects/k8s-staging-kubetest2 are expected, a result of #1819 merging
@@ -4,7 +4,13 @@ | |||
"members": [ | |||
"group:[email protected]" | |||
], | |||
"role": "projects/kubernetes-public/roles/ServiceAccountLister" | |||
"role": "organizations/758905017065/roles/iam.serviceAccountLister" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is expected, a result of #1737
"members": [ | ||
"group:[email protected]" | ||
], | ||
"role": "organizations/758905017065/roles/secretmanager.secretLister" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was me manually trialing #1731 (comment) to help land #1696
It did not solve the problem, so I'll manually remove this binding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ gcloud projects remove-iam-policy-binding kubernetes-public --member="group:[email protected]" --role="organizations/758905017065/roles/secretmanager.secretLister"
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cncf-ci, spiffxp 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:
Approvers can indicate their approval by writing |
Audit Updates wg-k8s-infra