feat: grant user assigned identity 'Reader' role for hosted cluster #1076
Conversation
|
💖 Thanks for opening your first pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix. Examples of commit messages with semantic prefixes: - |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run pr-e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: norshtein 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 |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
norshtein
force-pushed
the
hosted-master-identity
branch
from
15e02a9
to
da920fc
Compare
|
The code is auto-formatted by VSCode, seems the formatted code is not in the format the linter want, so I manually change the format to keep aligned with |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #1076 +/- ##
==========================================
- Coverage 74.35% 74.34% -0.02%
==========================================
Files 131 131
Lines 18274 18278 +4
==========================================
+ Hits 13588 13589 +1
- Misses 3905 3906 +1
- Partials 781 783 +2 |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
| msiRoleAssignment := createMSIRoleAssignment() | ||
| var msiRoleAssignment RoleAssignmentARM | ||
| if isHostedMaster { | ||
| msiRoleAssignment = createMSIRoleAssignment(IdentityReaderRole) |
There was a problem hiding this comment.
@jackfrancis @CecileRobertMichon Shall we change this ReaderRole to ContributorRole on agent node? Since we are going to support CSI driver with MSI credential. And CSI driver will be installed on agent nodes, e.g. disk provisioner of azure disk CSI driver will be installed as a StatefulsSet on one agent node, ReaderRole MSI won't work since it needs write disk permission.
also add @norshtein for any comments.
There was a problem hiding this comment.
If I understand this correctly, CSI should be an option, something like cs.Properties.****.UseCSI,
and its default value is false, so I personally prefer to add a judgment somewhere: if CSI is used, use Contributor role on agent node; otherwise, use Reader role.
There was a problem hiding this comment.
In the future, it's not an option, it's a must, I think we need to enable it now.
CSI is beta now, would step into GA soon.
There was a problem hiding this comment.
Got that. This PR only affects cluster having hosted master. For a complete cluster using user assigned identity, the identity still has Contributor role, so it won't affect CSI's usage. When CSI becomes mandatory in AKS, we can revert this PR and grant the user assigned identity Contributor role. Does that make sense?
There was a problem hiding this comment.
@norshtein then why not enabled it now? AKS v1.13.x is already available now, if we enable Contributor role, then we could test CSI driver on AKS at the first moment when MSI is enabled on AKS.
There was a problem hiding this comment.
What's the status of CSI driver on AKS? Are users able to enable(and disable) CSI?
Reason for Change:
Background: there is an upcoming feature in AKS: support authenticating to Azure using MSI. In the implementation, agent nodes will use an auto-generated user assigned identity and master components won't use this identity to authenticate. In this case, it enough to grant the generated user assigned identity only "Reader" role in the created resource group.
Issue Fixed:
Requirements:
Notes: