Adds support for MIC to authenticate with azure using system assigned/user assigned MSI #265
Conversation
kkmsft
changed the title
pkg/cloudprovider/cloudprovider.go
Outdated
|
|
||
| var spt *adal.ServicePrincipalToken | ||
| if azureConfig.UseManagedIdentityExtension { | ||
| // MSI endpoing is required for both types of MSI - system assigned and user assigned. |
pkg/cloudprovider/cloudprovider.go
Outdated
| // MSI endpoing is required for both types of MSI - system assigned and user assigned. | ||
| msiEndpoint, err := adal.GetMSIVMEndpoint() | ||
| if err != nil { | ||
| glog.Errorf("Failed to get msiEndpoint: %+v", err) |
There was a problem hiding this comment.
Failed to get msiEndpoint to Failed to get msiEndpoint. Error:
Following the same pattern we use in other parts of the code.
pkg/k8s/client.go
Outdated
| // TODO: Filter out the deployment owner references. | ||
| deployment := "" | ||
| if podList.Items[0].OwnerReferences[0].Kind == "ReplicaSet" { | ||
| deployment = podList.Items[0].OwnerReferences[0].Name |
There was a problem hiding this comment.
This is the replicaset name, so maybe we should change deployment to rsName to reflect the value we are getting
pkg/nmi/server/server.go
Outdated
| } | ||
| if err != nil { | ||
| logger.Errorf("failed to get service principal token for pod:%s/%s, %+v", podns, podname, err) | ||
| http.Error(w, err.Error(), http.StatusForbidden) |
There was a problem hiding this comment.
Do we want to return the status code returned by metadata ep instead of 403 since we are really just proxying the request here? I think metadata ep will return a 400 or 404. 403 is not part of retry status codes.
There was a problem hiding this comment.
Not sure yet, how to extract this info from ada. For time being have put a TODO to handle in a future PR.
kkmsft
changed the title
… or user assigned MSI. Resolves the item in Azure#261. This PR adds the capability for MIC to look at azure.json or environment variables to determine whether the system assigned or user assigned MSI has to be used for accessing azure resources. The MIC requests for token based on MSI. Also contains changes in NMI to determine if the request is originating from an MIC replicaset. If so, NMI directly generates the tokens instead of looking up the azure assigned identity for the pod-binding match.
In pod retry the object returned from listing was used to cast even when error was returned. This PR fixes this issue. The retry call has been refactored to account for other error conditions and simplify the usage. This PR introduces getPodList call and then a retry call built on top of that.
docs/readmes/README.msi.md
Outdated
| @@ -0,0 +1,50 @@ | |||
| ## Introduction | |||
|
|
|||
| The MIC component in aad-pod-identity needs to authenticate with the cloud to assign and remove user assign identities onto | |||
docs/readmes/README.msi.md
Outdated
| nodes in the Kubernetes cluster. The system/user assigned MSI needs to have role assignments authorizing such operations on the vms/vmss | ||
| and also operations on the user assigned identity. | ||
|
|
||
| After the cluster is created to perform the following steps to obtain the principal id: |
There was a problem hiding this comment.
After the cluster is created, run these commands to retrieve the principal id
docs/readmes/README.msi.md
Outdated
| and also operations on the user assigned identity. | ||
|
|
||
| After the cluster is created to perform the following steps to obtain the principal id: | ||
| for VMAs: |
pkg/cloudprovider/cloudprovider.go
Outdated
| @@ -59,6 +60,8 @@ func NewCloudProvider(configFile string) (c *Client, e error) { | |||
| azureConfig.SubscriptionID = os.Getenv("SUBSCRIPTION_ID") | |||
| azureConfig.ResourceGroupName = os.Getenv("RESOURCE_GROUP") | |||
| azureConfig.VMType = os.Getenv("VM_TYPE") | |||
| azureConfig.UseManagedIdentityExtension = strings.EqualFold(os.Getenv("USE_MSI"), "True") | |||
| azureConfig.UserAssignedIdentityID = os.Getenv("USER_ASSIGNED_MSI_CLIENTID") | |||
There was a problem hiding this comment.
I think we should split CLIENTID into CLIENT_ID to stay in sync with the convention we already have.
pkg/nmi/server/server.go
Outdated
| } | ||
| response, err := json.Marshal(*token) | ||
| if err != nil { | ||
| logger.Errorf("Failed to unmarshal service principal token. Error: %+v", err) |
There was a problem hiding this comment.
Failed to marshal service principal token
pkg/nmi/server/server.go
Outdated
| logger.Errorf("Failed to unmarshal service principal token. Error: %+v", err) | ||
| return nil, http.StatusInternalServerError, err | ||
| } | ||
| return response, 0, nil |
There was a problem hiding this comment.
I don't think 0 is a valid http status code. We should return maybe 200 OK, because request was successful.
There was a problem hiding this comment.
For success case, the return code is not used. We just write the response. In any case have changed it to http.StatusOK
This PR adds the capability for MIC to look at azure.json or environment variables
to determine whether the system assigned or user assigned MSI has to be used for accessing
azure resources. The MIC requests for token based on MSI. Also contains changes in NMI to determine if the request is originating from an MIC replicaset. If so, NMI directly generates the tokens instead of looking up the azure assigned identity for the pod-binding match.
Reason for Change:
Adds ability for MIC to authenticate using system assigned/user assigned MSI.
Issue Fixed:
Notes for Reviewers:
TODO: run the e2e on system assigned identity cluster.