Add command to rotate e2e credentials #3523
Conversation
hawkowl
requested review from
jewzaam,
bennerv,
rogbas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
dofinn and
tiguelu
as code owners
hawkowl
added
priority-high
|
/azp run ci, e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Can you link the SOP? I can't seem to find the right one |
| func init() { | ||
| rootCmd.AddCommand(generateServicePrincipalsCmd) | ||
|
|
||
| generateServicePrincipalsCmd.Flags().String("keyvault", "aro-e2e-principals", "keyvault to use") |
There was a problem hiding this comment.
I like this pattern. Maybe I'm extrapolating too much (I find the cobra docs hard to parse) but if I understand it correctly we could replace a lot of the go run ...... stuff with this and "proper" CLI commands?
| msgraph_apps "github.com/Azure/ARO-RP/pkg/util/graph/graphsdk/applications" | ||
| msgraph_models "github.com/Azure/ARO-RP/pkg/util/graph/graphsdk/models" | ||
| "github.com/Azure/ARO-RP/pkg/util/graph/graphsdk/serviceprincipals" | ||
| "github.com/Azure/ARO-RP/pkg/util/keyvault" |
There was a problem hiding this comment.
We may want to use the new keyvault track2 SDK instead of the old one since the keyvault SDK track2 is merged https://github.com/Azure/ARO-RP/pull/3275/files
There was a problem hiding this comment.
Thank you for this @hawkowl. I just left some optional suggestions and a question about a for loop.
| cmd.PrintErr(err) | ||
| return err |
There was a problem hiding this comment.
IMHO having an error logged out and also returned to the upper function call is something we should avoid. I would just return the error and print it in the caller function.
| utillog "github.com/Azure/ARO-RP/pkg/util/log" | ||
| ) | ||
|
|
||
| // generateServicePrincipalsCmd represents the generateServicePrincipals command |
There was a problem hiding this comment.
I think the code is clear enough to remove this comment 🙂
| func init() { | ||
| rootCmd.AddCommand(generateServicePrincipalsCmd) | ||
|
|
||
| generateServicePrincipalsCmd.Flags().String("keyvault", "aro-e2e-principals", "keyvault to use") |
There was a problem hiding this comment.
I think it would be great if we can extract all the flag names like "keyvault" into a const on top of the file as those are repeated and could change.
| rootCmd.AddCommand(generateServicePrincipalsCmd) | ||
|
|
||
| generateServicePrincipalsCmd.Flags().String("keyvault", "aro-e2e-principals", "keyvault to use") | ||
| generateServicePrincipalsCmd.Flags().String("spn-prefix", "aro-v4-e2e-devops-spn-", "prefix for SPN name") |
|
|
||
| generateServicePrincipalsCmd.Flags().String("keyvault", "aro-e2e-principals", "keyvault to use") | ||
| generateServicePrincipalsCmd.Flags().String("spn-prefix", "aro-v4-e2e-devops-spn-", "prefix for SPN name") | ||
| generateServicePrincipalsCmd.Flags().Int("spn-count", 50, "number of SPNs to generate/use") |
| return err | ||
| } | ||
|
|
||
| if len(apps[0].GetPasswordCredentials()) > 0 { |
There was a problem hiding this comment.
We are using apps[0].GetPasswordCredentials() in 3 different places, maybe we can extract it to a variable, more performant and easier to read.
| err = keyvaultManager.SetSecret(ctx, fmt.Sprintf("%s-secret-value", spnName), azkeyvault.SecretSetParameters{ | ||
| Value: resp.GetSecretText(), | ||
| SecretAttributes: &azkeyvault.SecretAttributes{ | ||
| Expires: &expiryTime, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Same comment as before, I think it would be great to extract azkeyvault.SecretSetParameters into a variable as this is not easy to read (at least for me).
| log.Info("checking for service principal") | ||
|
|
||
| filter = fmt.Sprintf("appId eq '%s'", spnAppID) | ||
| spResult, err := graphClient.ServicePrincipals().Get(ctx, &serviceprincipals.ServicePrincipalsRequestBuilderGetRequestConfiguration{ |
There was a problem hiding this comment.
Same as before for serviceprincipals.ServicePrincipalsRequestBuilderGetRequestConfiguration
| } | ||
|
|
||
| log.Info("setting service principal ID in keyvault") | ||
| err = keyvaultManager.SetSecret(ctx, fmt.Sprintf("%s-sp-id", spnName), azkeyvault.SecretSetParameters{ |
There was a problem hiding this comment.
Same as before for azkeyvault.SecretSetParameters. Even here is not that important, I still see value in extracting it.
| }, | ||
| } | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Sadly, Cobra forces the developer to use the init() function to register the commands/subcommands and "fun" problems can arise with the usage of init() in Go. But what can we do ...? 😆
|
Please rebase pull request. |
|
I don't think we need this anymore. |
Which issue this PR addresses:
Fixes ARO-6742
What this PR does / why we need it:
Generates/rotates passwords for the SPNs
Test plan for issue:
Tested in dev, don't have permissions for INT, will require a TL to do that
Is there any documentation that needs to be updated for this PR?
SOPs for rotation
How do you know this will function as expected in production?
I don't, but it should be re-runnable...