Closed cvvz closed 9 months ago
cc @andyzhangx
@cvvz can you add more description in the PR? e.g. current /etc/kubernetes/azure.json
contains what fields, and the trident driver should mount that file and read specific fields to get managed identity support.
I've added more details to the change description.
@clintonk @adkerr @based64god @ameade @ntap-arorar @nazneeninc Could you please kindly take a look? Thanks!
I think even useAzureAuthConfig
is not set in TridentBackendConfig
, if backend-tbc-anf-secret
is not defined, and /etc/kubernetes/azure.json
exists on the node, then this driver should get azure credentials from /etc/kubernetes/azure.json
, that's consistent behavior as other Azure CSI drivers.
So I think useAzureAuthConfig
config is not required, it depends on whether azure cloud config file exists.
OK, I've removed useAzureAuthConfig
option, when user does not provide credential, it will get credential from azure.json
.
@cvvz I understand that this PR is consistent with the other Azure CSI drivers. I am curious whether Azure AD Workload Identity is an alternative way to solve this problem. Could a future enhancement implement that as well? Why is one preferable to the other? Annotating the Trident controller service account with a managed identity seems simple and elegant.
@clintonk This PR actually already support workload identity. We get identity from cloud-provider sdk in here, which support 4 ways for authentication: service principal, certificate, managed identity and workload identity federation.
FYI, you can refer to the implementation of azurefile https://github.com/kubernetes-sigs/azurefile-csi-driver/blob/master/docs/workload-identity.md. This PR is the first step to support workload identity, after that, we should add necessary field to trident-controller manifest accordingly if we choose to authenticate with workload identity. Anyway, this PR is the prerequisite for supporting workload identity.
@cvvz For workload identity, I would have expected an annotation on the Trident controller service account. That requires an OIDC issuer, but it appeals because it works similarly for multiple managed K8s services. Is that what you envision?
AKS already support enable oidc issuer by option: --enable-oidc-issuer
.
Let me make this clearer, if you want to support workload identity for trident, here are the things need to be done:
--enable-oidc-issuer --enable-workload-identity
@cvvz , could you please attach the results for the tests(Create ,Delete etc) you've run on "azure-netapp-files" and "azure-netapp-files-subvolume" drivers for NFS and SMB?
The changes were pulled into Trident's internal repo. These were tested and validated and have been made available in Trident release v23.10.
Change description
### feat: support getting credentials from azure auth config Application running on AKS can use [cloud provider azure configuration](https://kubernetes-sigs.github.io/cloud-provider-azure/install/configs/), which is at `/etc/kubernetes/azure.json` in worker node, to get credential. [Currently, there are three authentication methods](https://kubernetes-sigs.github.io/cloud-provider-azure/install/configs/#:~:text=Note%3A%20Cloud%20provider%20currently%20supports%20three%20authentication%20methods%2C%20you%20can%20choose%20one%20combination%20of%20them%3A): ![image](https://github.com/NetApp/trident/assets/44308864/89714b11-c88c-4401-b7ea-66a2f5773329) The Minimal configuration of tbc for Azure can be: ```yaml apiVersion: trident.netapp.io/v1 kind: TridentBackendConfig metadata: name: backend-tbc-anf namespace: trident spec: version: 1 storageDriverName: azure-netapp-files ``` users can leave `clientID`, `clientSecret`, `tenantID`, `subscriptionID` and `location` empty, all these values will be got from azure auth config file and credential can be generated. ## Project trackingN/A
Do any added TODOs have an issue in the backlog?
N/A
Did you add unit tests? Why not?
No ut for the entire function.
Does this code need functional testing?
Is a code review walkthrough needed? why or why not?
Should additional test coverage be executed in addition to pre-merge?
Does this code need a note in the changelog?
feat(Azure): support getting credentials from azure auth config
Does this code require documentation changes?
Yes.
clientID
,clientSecret
,tenantID
,subscriptionID
andlocation
are now can be empty for the minimal configuration.Additional Information