azure: Add client certification to terraform #6250
Conversation
|
/test unit |
|
This looks great. A couple of questions/notes:
@2uasimojo @akhil-rane @abutcher @jharrington22 @wking have all been discussing how to handle setting the cluster secret and it will require some fiddling with manifests. I have been trying to think of a way that the installer could authenticate with certs and still write the client secret without it being too hacky: Perhaps we allow environment-variable based authentication for authenticating the installer/terraform with a cert and then continue to pass the cluster secret in |
rna-afk
force-pushed
the
azure-client-certs-auth
branch
2 times, most recently
from
4f7465d to
d20a37a
Compare
|
/test golint |
rna-afk
force-pushed
the
azure-client-certs-auth
branch
2 times, most recently
from
dadbbf8 to
bfc2c3f
Compare
|
/test golint |
rna-afk
force-pushed
the
azure-client-certs-auth
branch
2 times, most recently
from
f24e2b4 to
880bca6
Compare
|
/test e2e-azure |
rna-afk
force-pushed
the
azure-client-certs-auth
branch
3 times, most recently
from
d46834f to
cd12187
Compare
s-amann
left a comment
s-amann
left a comment
There was a problem hiding this comment.
please see comment regarding duplicate code, and my one open question.
rna-afk
force-pushed
the
azure-client-certs-auth
branch
2 times, most recently
from
67769b6 to
5c9a27a
Compare
jharrington22
left a comment
jharrington22
left a comment
There was a problem hiding this comment.
@rna-afk thanks for this, I had some folks review this that are closer to the authorizer work in the ARO RP and they have a few comments that we should address before merging cc/ @patrickdillon
rna-afk
force-pushed
the
azure-client-certs-auth
branch
2 times, most recently
from
bac4441 to
1626f9d
Compare
|
@2uasimojo @akhil-rane @jharrington22 Because we can't create a cluster secret from a cert, we have added validation to enforce that this can only be run with manual mode: My understanding here is that this will only prevent the installer from laying down a cluster secret, users should still be able to edit manifests to provide a secret and set the CCO to the desired mode (i.e. I do not think this validatoin enforces the CCO to run in manual mode). This seems like the simplest approach. I am still open to the idea of allowing both sets of creds to be supplied directly to the installer, but this seems like the simplest solution ATM. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
rna-afk
force-pushed
the
azure-client-certs-auth
branch
from
1626f9d to
26a6917
Compare
openshift-merge-robot
removed
the
needs-rebase
rna-afk
force-pushed
the
azure-client-certs-auth
branch
from
26a6917 to
449c561
Compare
|
/test e2e-azure |
|
@rna-afk: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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 e2e-azure-ovn |
|
/test images |
openshift-merge-robot
added
the
needs-rebase
Adding the option for the users to authenticate using client certificates instead of client secrets if they want. The certs field is already in the osServicePrincipal.json and has to be set for Azure SDK authentication. Terraform expects two extra fields to be set for authentication and is added here.
rna-afk
force-pushed
the
azure-client-certs-auth
branch
from
dde171f to
2363e67
Compare
openshift-merge-robot
removed
the
needs-rebase
rna-afk
force-pushed
the
azure-client-certs-auth
branch
from
2363e67 to
fdd8b67
Compare
There was a bug in the certs authorizer that was fixed in the Azure/auth v0.5.1. Updating the library version.
rna-afk
force-pushed
the
azure-client-certs-auth
branch
from
fdd8b67 to
ca66e04
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
@rna-afk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
7 participants
Adding the option for the users to authenticate using client
certificates instead of client secrets if they want. The certs
field is already in the osServicePrincipal.json and has to be set
for Azure SDK authentication. Terraform expects two extra fields
to be set for authentication and is added here.