-
Notifications
You must be signed in to change notification settings - Fork 431
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
use sufficient identity roles for conformance tests #5060
use sufficient identity roles for conformance tests #5060
Conversation
Signed-off-by: Jack Francis <[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.
/lgtm
/approve
/hold for conformance tests
LGTM label has been added. Git tree hash: 97fa0dea3430fbe63d0f67371a44d2e3eb3c8948
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5060 +/- ##
=======================================
Coverage 51.14% 51.14%
=======================================
Files 274 274
Lines 24669 24669
=======================================
Hits 12617 12617
Misses 11264 11264
Partials 788 788 ☔ View full report in Codecov by Sentry. |
/retest |
Signed-off-by: Jack Francis <[email protected]>
/retest |
@jackfrancis The windows-custom-builds job should be more stable now. Can you please try removing the "Storage Account Contributor" role again? |
Signed-off-by: Jack Francis <[email protected]>
Signed-off-by: Jack Francis <[email protected]>
/retest |
1 similar comment
/retest |
Signed-off-by: Jack Francis <[email protected]>
@@ -159,13 +159,10 @@ EOF | |||
AZURE_IDENTITY_ID_PRINCIPAL_ID=$(az identity show -n "${USER_IDENTITY}" -g "${AZWI_RESOURCE_GROUP}" --query principalId -o tsv) | |||
|
|||
echo "${AZURE_IDENTITY_ID}" > "${AZURE_IDENTITY_ID_FILEPATH}" | |||
until az role assignment create --assignee-object-id "${AZURE_IDENTITY_ID_PRINCIPAL_ID}" --role "Owner" --scope "/subscriptions/${AZURE_SUBSCRIPTION_ID}" --assignee-principal-type ServicePrincipal; do | |||
until az role assignment create --assignee-object-id "${AZURE_IDENTITY_ID_PRINCIPAL_ID}" --role "Contributor" --scope "/subscriptions/${AZURE_SUBSCRIPTION_ID}" --assignee-principal-type ServicePrincipal; do |
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.
The cluster-autoscaler e2e tests need permission to create role assignments (for workload identity auth from CAS) which Contributor doesn't have, but I think that gap can be filled with the "Role Based Access Control Administrator" role.
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.
Just verified cluster-autoscaler tests using an identity with Contributor + RBAC Admin passes for me locally.
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.
ACK I couldn't remember why we might need the identity to have role assignment create perms, but now I do.
I think adding an add'l role assignment (Contributor + RBAC) is the bigger downside compared to the slight add'l privileges that Owner vs Contributor carries given that we actually need the most interesting part of the delta (the ability to create new role assignments).
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.
FWIW I think Contributor + RBAC Admin has slightly fewer perms than Owner, i.e. it would not have the notActions
here (aside from the ones granted by RBAC Admin):
{
"assignableScopes": [
"/"
],
"description": "Grants full access to manage all resources, but does not allow you to assign roles in Azure RBAC, manage assignments in Azure Blueprints, or share image galleries.",
"id": "/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c",
"name": "b24988ac-6180-42a0-ab88-20f7382dd24c",
"permissions": [
{
"actions": [
"*"
],
"notActions": [
"Microsoft.Authorization/*/Delete",
"Microsoft.Authorization/*/Write",
"Microsoft.Authorization/elevateAccess/Action",
"Microsoft.Blueprint/blueprintAssignments/write",
"Microsoft.Blueprint/blueprintAssignments/delete",
"Microsoft.Compute/galleries/share/action",
"Microsoft.Purview/consents/write",
"Microsoft.Purview/consents/delete"
],
"dataActions": [],
"notDataActions": []
}
],
"roleName": "Contributor",
"roleType": "BuiltInRole",
"type": "Microsoft.Authorization/roleDefinitions"
}
The elevateAccess
one in particular might be worth avoiding if possible.
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.
fair, testing our new trifecta now
Signed-off-by: Jack Francis <[email protected]>
/lgtm will leave the hold for tests |
LGTM label has been added. Git tree hash: 2283b0daf02a5210b3de04372b3723eb611cf1f0
|
note that commits are not squashed |
/retest |
/label tide/merge-method-squash |
/hold cancel |
This job has flaked the same way recently a few times: https://storage.googleapis.com/k8s-triage/index.html?pr=1&text=Job%20default%2Fcurl-to /retest |
/cherry-pick release-1.16 |
@jackfrancis: new pull request created: #5065 In response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR configures the identity used by kind to possess only the sufficient role asssignments needed to perform all tests.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: