add support for application credentials #300
Conversation
gardener-robot
added
area/control-plane
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-3
added
needs/ok-to-test
There was a problem hiding this comment.
FYI: When you're specifying application credentials, especially by id and a password, there is no need to request domain/project/username. application credentials are project scoped by default. You need only the keystone auth URL, credentials ID and credentials password.
| username="{{ .Values.username }}" | ||
| password="{{ .Values.password }}" | ||
| {{- end }} | ||
| {{- if .Values.applicationCredentialID }} |
There was a problem hiding this comment.
Did you omit the applicationCredentialName by purpose to avoid end user confusion?
There was a problem hiding this comment.
I wanted to keep it simple. Especially if we want to support adding secrets in the Gardener dashboard, this can become quite cumbersome. It also produces additional test effort.
Is there any good reason to use applicationCredentialName (together with userName or a user id) instead of applicationCredentialID?
There was a problem hiding this comment.
Is there any good reason to use...
applicationCredentialName may contain a hint for its purpose, but it adds additional complexity for providing a proper username and domain name, or just a userid.
There was a problem hiding this comment.
applicationCredentialName is supported now, too.
There was a problem hiding this comment.
@MartinWeindel I'd revert the applicationCredentialName support for now, because this may add extra confusion on the user end. See the detailed explanation of 3 auth cases: gophercloud/gophercloud#1365 (comment)
There was a problem hiding this comment.
P.S. I'm not against the applicationCredentialName, but in order to speed-up the appCreds implementation I'd focus on appID only. The appName support can be added later.
There was a problem hiding this comment.
As new releases are needed for both the Openstack provider and the machine-controller-provider-openstack, I don't think it will be faster to remove the appName support now as it has already been tested.
@dkistner WDYT?
MartinWeindel
force-pushed
the
application-credentials
branch
from
e5b315f to
9adf589
Compare
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
|
@MartinWeindel You have pull request review with status CHANGES_REQUESTED, please check |
There was a problem hiding this comment.
looks good to me.
/hold
Until we have a mcm openstack provider with gardener/machine-controller-manager-provider-openstack#26
gardener-robot
added
the
reviewed/do-not-merge
gardener-robot
added
the
size/xl
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
gardener-robot
removed
the
size/l
MartinWeindel
force-pushed
the
application-credentials
branch
from
4a52a3a to
d14b8f7
Compare
gardener-robot-ci-3
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
MartinWeindel
force-pushed
the
application-credentials
branch
from
d14b8f7 to
8bcec89
Compare
gardener-robot-ci-1
added
reviewed/ok-to-test
gardener-robot
added
size/l
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
MartinWeindel
force-pushed
the
application-credentials
branch
from
e5d8ee4 to
8bcec89
Compare
gardener-robot
added
size/xl
gardener-robot-ci-2
added
the
reviewed/ok-to-test
gardener-robot
added
reviewed/lgtm
How to categorize this PR?
/area control-plane
/area security
/kind enhancement
/platform openstack
What this PR does / why we need it:
The provider secret for Openstack can contain the keys
applicationCredentialIDandapplicationCredentialSecretas alternative to authentication with username/passwordWhich issue(s) this PR fixes:
Fixes #4
Special notes for your reviewer:
This PR is depending on a new release of the machine-controller-manager-provider-openstack which includes PR gardener/machine-controller-manager-provider-openstack#26
Release note: