Adding support for service principal credential for Azure plugin #507
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@savitharaghunathan: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/test operator-unit-test |
|
@savitharaghunathan: The specified target(s) for
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test 4.7-operator-unit-test |
|
/test all |
savitharaghunathan
requested review from
shubham-pampattiwar and
dymurray
and removed request for
shawn-hurley and
rayfordj
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
There was a problem hiding this comment.
Thank you for the PR @savitharaghunathan, changes look sane to me. Added some questions and Nits,
controllers/registry.go
Outdated
| } | ||
| } else { | ||
| r.Log.Info("Checking for service principal credentials") | ||
| if len(azcreds.subscriptionID) == 0 && len(azcreds.tenantID) == 0 && len(azcreds.clientID) == 0 && len(azcreds.clientSecret) == 0 && len(azcreds.resourceGroup) == 0 { |
There was a problem hiding this comment.
Do we not need strorageAccountKey value as well for Service principal ?
There was a problem hiding this comment.
we need strorageAccountKey. Hence checking for the one in bslspec. If the config parameter for storageAccountKeyEnvVar is not present, then service principal creds needs to be validated
There was a problem hiding this comment.
Irresepctive of whatever method we need the strorageAccountKey in credential file.
There was a problem hiding this comment.
just a NIT for this... when we have this many conditionals on an if statement looks better if you do:
if len(azcreds.subscriptionID) == 0 &&
len(azcreds.tenantID) == 0 &&
len(foo)
...
| r.Log.Info(fmt.Sprintf("Azure storage key value after parsing: %s", AzureStorageKey)) | ||
| continue | ||
| azcreds.strorageAccountKey = storageKeyValue | ||
| case matchedSubscriptionId: |
* api v1 upgrade CR typo (openshift#501) * Removing kube-rbac-proxy from the containers needed (openshift#504) * fix indentation (openshift#505) * turn OCP versions from int to string (openshift#502) * Ensure velero is deleted (openshift#510) * Timeout for must-gather failed backups (openshift#497) * Adding known limitations to must-gather * Adding implementation details for timeout * remove VSPHERE env configs for csv (openshift#512) * update troubleshooting doc (openshift#509) * Adding support for service principal credential for Azure plugin (openshift#507) * Adding validation for azure creds * Adding account key check * Adding checks to validate SP if storage key is not present * Adding unit test#1 * Adding a test case for service principal * Adding review comments * Remove: logging sensitive info * make deploy velero namespace fix (openshift#506) * make deploy velero namespace fix * add changes for undeploy * add deploy-tmp-cleanup * fix aws registry env vars (openshift#515) * Azure SP docs (openshift#514) * Adding Azure SP related doc * Fixing title * Fixing nit * Registry should not be deployed when Azure SP is used (openshift#518) * Registry should not be deployed when Azure SP is used * Fixing unit tests * Adding review comments * Fixing typos * Adding registry label to BSL * Updating azure credentials documentation (openshift#519) * AWS plugin config: BSL Region not required when s3ForcePathStyle is false and BackupImages is false (openshift#517) * OADP-153, Close openshift#424 * Nil restic Config should delete previous restic daemonset * only check restic config if it is not nil * installCase wantError implement * Make err more verbose * commit metav1 * Changes for BackupImages considerations * fake client fix * removed vsphere from source manager config (openshift#520) * Update README.md * badge relocate (openshift#521) Co-authored-by: Tiger Kaovilai <[email protected]> Co-authored-by: Shawn Hurley <[email protected]> Co-authored-by: Emily McMullan <[email protected]> Co-authored-by: Savitha Raghunathan <[email protected]> Co-authored-by: Wesley Hayutin <[email protected]> Co-authored-by: Dylan Murray <[email protected]>
Adding support for service principal credential for Azure plugin