Closed zedy-wj closed 1 month ago
@zedy-wj, thanks for submitting this issue for the avm/res/machine-learning-services/workspace
module!
[!IMPORTANT] A member of the @Azure/avm-res-machinelearningservices-workspace-module-owners-bicep or @Azure/avm-res-machinelearningservices-workspace-module-contributors-bicep team will review it soon!
Question @zedy-wj, is there a technical reason why RBAC cannot be used? Wouldn't it make more sense to add a role assignment instead (even on the secret/key-level) as it can be considered a better practice? Aside from that, I guess nothing speaks per se against adding this, as long as it can be controlled via one or multiple parameters.
@AlexanderSehr - Thank you very much for your kind suggestion, but for now, all we have done is to restore the modules under infra/core
in the azd
tool as much as possible to ensure that there will be no problems when we migrate azd awesome templates
to AVM
in the future.
Back to your question, I think only when we really start to migrate templates to AVM
, we can know whether using rbac
can replace Key Vault + Access Policy
, and what problems will arise.
@jongio - We noticed that you left a comment in the issue https://github.com/Azure/Azure-Verified-Modules/issues/1228#issuecomment-2316314260. Actually, for this feature, we only need to add it to the resource module
or do a new ptn module
. Either of the two implementations is enough. Which solution do you prefer at present?
@AlexanderSehr - Thank you very much for your kind suggestion, but for now, all we have done is to restore the modules under
infra/core
in theazd
tool as much as possible to ensure that there will be no problems when we migrateazd awesome templates
toAVM
in the future.Back to your question, I think only when we really start to migrate templates to
AVM
, we can know whether usingrbac
can replaceKey Vault + Access Policy
, and what problems will arise.
Fair enough. To be on the save side, we could certainly add an implementation like in Synapse Workspace or Disk-Encryption-Set module, that is, to support both: https://github.com/Azure/bicep-registry-modules/blob/9864352023baf7c05fe605c6179233f4b71dac24/avm/res/compute/disk-encryption-set/main.bicep#L139-L151
Doesn't change the fact that I'd highly encourage the use of RBAC over Access Policies, but it would leave the choice to you :)
@jongio - We noticed that you left a comment in the issue Azure/Azure-Verified-Modules#1228 (comment). Actually, for this feature, we only need to add it to the
resource module
or do a newptn module
. Either of the two implementations is enough. Which solution do you prefer at present?
I presume with 'you' you meant anybody from the maintainers? At least I can't see any comment that I added personally 😄
In any case, if we're just talking about the role assignment, I'd subjectively see it more on the side of the pattern module - but - that's not an informed guess. For that I'd need more input from you.
The question to fundamentally answer is whether the assignment of permissions on the KeyVault is something that is needed for most if not all instances of a ML Workspace deployment. In the module, I can see that the KeyVault ResourceId is a conditional parameter that is only needed if the workspace's kind
has the value of 'Default', 'FeatureStore', or 'Hub'. That sounds pretty universal to me (which would speak to the implementation), but you & the module's owner @cecheta may be the better judges here.
@jongio - We noticed that you left a comment in the issue Azure/Azure-Verified-Modules#1228 (comment). Actually, for this feature, we only need to add it to the
resource module
or do a newptn module
. Either of the two implementations is enough. Which solution do you prefer at present?I presume with 'you' you meant anybody from the maintainers? At least I can't see any comment that I added personally 😄 In any case, if we're just talking about the role assignment, I'd subjectively see it more on the side of the pattern module - but - that's not an informed guess. For that I'd need more input from you. The question to fundamentally answer is whether the assignment of permissions on the KeyVault is something that is needed for most if not all instances of a ML Workspace deployment. In the module, I can see that the KeyVault ResourceId is a conditional parameter that is only needed if the workspace's
kind
has the value of 'Default', 'FeatureStore', or 'Hub'. That sounds pretty universal to me (which would speak to the implementation), but you & the module's owner @cecheta may be the better judges here.
@AlexanderSehr - This is just my message to jongio as I saw him leave a message in ptn module proposal, please ignore it. 😄
Because in my opinion, ptn module
and adding new features here, the purpose of both is the same, we just have to choose a way to do it.
Hopefully it didn't cause you too many misunderstandings.
@jongio - We noticed that you left a comment in the issue Azure/Azure-Verified-Modules#1228 (comment). Actually, for this feature, we only need to add it to the
resource module
or do a newptn module
. Either of the two implementations is enough. Which solution do you prefer at present?I presume with 'you' you meant anybody from the maintainers? At least I can't see any comment that I added personally 😄 In any case, if we're just talking about the role assignment, I'd subjectively see it more on the side of the pattern module - but - that's not an informed guess. For that I'd need more input from you. The question to fundamentally answer is whether the assignment of permissions on the KeyVault is something that is needed for most if not all instances of a ML Workspace deployment. In the module, I can see that the KeyVault ResourceId is a conditional parameter that is only needed if the workspace's
kind
has the value of 'Default', 'FeatureStore', or 'Hub'. That sounds pretty universal to me (which would speak to the implementation), but you & the module's owner @cecheta may be the better judges here.@AlexanderSehr - This is just my message to jongio as I saw him leave a message in ptn module proposal, please ignore it. 😄
Because in my opinion,
ptn module
and adding new features here, the purpose of both is the same, we just have to choose a way to do it.Hopefully it didn't cause you too many misunderstandings.
ah my bad, sorry 😄 . Would anyways be keen to hear your thoughts :) and @jongio's, of course
Hello @zedy-wj,
I've just had a look at this now; considering that the key vault resource is created from outside of the avm/res/machine-learning-services/workspace
module, would it not make more sense to create the access policies as a child resource of the key vault?
@jongio For machine learning services
module, should we consider this issue as a missing feature issue#3028, or as a new pattern module issue#1228? We need to have a conclusion ASAP to close one of the issues. Otherwise, we can't move forward the repair progress of ptn. Please take a look at it when you have time. Thanks a lot.
Hello @zedy-wj,
I've just had a look at this now; considering that the key vault resource is created from outside of the
avm/res/machine-learning-services/workspace
module, would it not make more sense to create the access policies as a child resource of the key vault?
I guess with "child resource of the key vault" you meant a direct deployment using native Bicep code and the Microsoft.KeyVault/vaults/accessPolicies
provider? 😉
Hello @zedy-wj, I've just had a look at this now; considering that the key vault resource is created from outside of the
avm/res/machine-learning-services/workspace
module, would it not make more sense to create the access policies as a child resource of the key vault?I guess with "child resource of the key vault" you meant a direct deployment using native Bicep code and the
Microsoft.KeyVault/vaults/accessPolicies
provider? 😉
Yes I mean using Microsoft.KeyVault/vaults/accessPolicies
in the same place where the keyvault resource is being created.
Thanks to all the participants for your discussion. We will implement the relevant changes in the ptn module: https://github.com/Azure/Azure-Verified-Modules/issues/1228. Closed it.
Check for previous/existing GitHub issues
Issue Type?
Feature Request
Module Name
avm/res/machine-learning-services/workspace
(Optional) Module Version
No response
Description
Please add the creation of 'Microsoft.KeyVault/vaults/accessPolicies' and add access policy to key vault for machine learning service. For example (azd): https://github.com/Azure/azure-dev/blob/main/templates/common/infra/bicep/core/ai/project.bicep CC: @jongio
(Optional) Correlation Id
No response