-
Notifications
You must be signed in to change notification settings - Fork 6.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for openstack application credentials #6534
Add support for openstack application credentials #6534
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
Welcome @piequi! |
Hi @piequi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
oops ! CLA is now signed |
/ok-to-test |
I signed it |
/check-cla |
1 similar comment
/check-cla |
I tried out this branch, and I think it'd be better to read the default values from the environment, just like the other variables. In other words, add to
Diff: application-credentials.diff.txt |
Another issue I found when trying to build. The tenant-id and domain-id are not required when using application credentials. So the credential checker should have a case to check for that. |
I'm not totally sure that application credentials are commonly set in user environment variables (compared to username and password). Specifying application credentials in the Ansible environment (in openstack.yml) that will be used along with all other settings by the playbook, is more explicit.
You are right. I'll update the check regarding Thanks |
Do not check external_openstack_tenant_id when application credentials are defined
0da8abb
to
cadad2f
Compare
I haven't used app credentials yet, @alijahnas and @bl0m1 any feedback on this? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miouge1, piequi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
roles/kubernetes-apps/external_cloud_controller/openstack/tasks/openstack-credential-check.yml
Show resolved
Hide resolved
I was a bit confused at first by the checks in openstack-credential-check.yml ; the logic is a bit hard to understand because of the way the fail task is used. The 'when' conditions combine the variables that need to be checked, and the circumstances in which they need to be checked, all in the same list. What about using |
@rptaylor replacing If you think this is relevant, feel free to contribute. |
@piequi It has to do with the way the new application credential functionality is implemented; I am just suggesting it would be a cleaner way. |
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.
LGTM.
I cannot se any reasons not to accept this. looks to be compatible with occm 👍
/lgtm |
* Add support for openstack application credentials * Add some lines for readability * Update external_openstack_tenant_id check Do not check external_openstack_tenant_id when application credentials are defined * Add check for external_openstack_domain_id * Fix typo
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add support for OpenStack application credentials to authenticate against Keystone API (instead of using username and password)
Which issue(s) this PR fixes:
Fixes #6533
Special notes for your reviewer:
Does this PR introduce a user-facing change?: