-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Integrating MI auth for aad-pod-identity #399
Conversation
Are you advocating for switching to aa pod identity completely with this PR? Or will we still be able to allow both? |
I think it will be pretty easy to support MSI and SP indefinitely so I don't think we should remove the capability. I think the docs should advise everyone to use MSI. This pull request won't contain anything in it that switches the pipeline over to MSI. That will be coming later. |
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.
couple comments...still testing
--from-literal=AZURE_SUBSCRIPTION_ID="$AZURE_SUBSCRIPTION_ID" \ | ||
--from-literal=AZURE_TENANT_ID="$AZURE_TENANT_ID" | ||
--from-literal=AZURE_TENANT_ID="$AZURE_TENANT_ID" \ | ||
--from-literal=AZURE_USE_MSI="$AZURE_USE_MSI" | ||
``` |
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.
should probably account for both options (msi and sp)
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.
I can add few spots in this document with OR sections for using MSI/SP but I feel like it's going to clutter things up a bit. This document is just for deploying to an AKS cluster right? What do you think about the docs assuming you want MSI when deploying to AKS but SP when running 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.
Talking more with @jananivMS it seems there are several things (identity just being one of them) that should be done to a cluster to protect against a malicious pod. Should I move all of this to a Securing your cluster
section?
@@ -60,6 +58,16 @@ | |||
make install-cert-manager | |||
``` | |||
|
|||
d. Install [aad-pod-identity](https://github.com/Azure/aad-pod-identity#1-create-the-deployment) |
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.
should incorporate this in the same way cert-manager is...ie a Make target and a deployment template
install-aad-pod-identify:
kubectl apply -f https://raw.githubusercontent.com/Azure/aad-pod-identity/master/deploy/infra/deployment-rbac.yaml
create a dir in config/podidentity and put the yaml template for creating an identity...similar to certman
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.
Is there a place where the files in config\certmanager
are used? It doesn't look like make install-cert-manager
uses them to me.
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 the make file, yeah
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.
After renaming a few variables and functions, fixing up some Golang lints on error strings, and a rename or two, we should be good-to-go.
…ice-operator into aad-pod-identity * 'aad-pod-identity' of github.com:jakiefermsft/azure-service-operator: (79 commits) Fix devcontainer azcli install script (#459) makefile update addressing PR comments remove commented code moving sqlserver creation to suite test setup Increased timeout and check for SQL server Bugfix: Correct the tabbing in the make terraform target remove incorrect use of Update() to correctly remove finalizer Update Makefile error and status message refactor AzureError use refactor add helper function that returns a typed error linting refactors and azure error changes refactors and azure error changes some refactors and changing AzureError use fixed tests change way status is being sent and logged to new form integrate suggestions from PR review. Checking secret exists, differentiating between user secret and admin secret, better utilization of constants for logging changes based on PR comments ...
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.
Renames LGTM.
* Generated code should be gofmt'ed already * Temporarily point at Github for deploymentTemplate fixes
What this PR does / why we need it:
To support using aad-pod-identity we need to obtain a token via
NewServicePrincipalTokenFromMSI
and we no longer require aClientID
orClientSecret
.I had fun experimenting with go and cleaning up the env handling but I may have gone too far.
Special notes for your reviewer:
TODO:
If applicable:
closes #21