Skip to content
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.

Sign up for GitHub

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 application credentials support for cinder #7799

Merged
merged 2 commits into from
Jul 15, 2021
Merged

Add application credentials support for cinder #7799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9923a6
csi-driver: Added possibility to use application credentials for cinder
Xartos Jul 12, 2021
7f2d8a6
external-cloud-controller: Added env vars for openstack application c…
Xartos Jul 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions roles/kubernetes-apps/csi_driver/cinder/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
cinder_auth_url: "{{ lookup('env','OS_AUTH_URL') }}"
cinder_username: "{{ lookup('env','OS_USERNAME') }}"
cinder_password: "{{ lookup('env','OS_PASSWORD') }}"
cinder_application_credential_id: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_ID') }}"
cinder_application_credential_name: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_NAME') }}"
cinder_application_credential_secret: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_SECRET') }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment variables seem good for me according to https://github.com/kubernetes/cloud-provider-openstack/blob/ab36705fb740299c57cfeeefec6f4185e5f68579/cmd/client-keystone-auth/main.go#L177-L179

cinder_region: "{{ lookup('env','OS_REGION_NAME') }}"
cinder_tenant_id: "{{ lookup('env','OS_TENANT_ID')| default(lookup('env','OS_PROJECT_ID'),true) }}"
cinder_tenant_name: "{{ lookup('env','OS_TENANT_NAME')| default(lookup('env','OS_PROJECT_NAME'),true) }}"
Expand Down
43 changes: 34 additions & 9 deletions roles/kubernetes-apps/csi_driver/cinder/tasks/cinder-credential-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,38 @@
msg: "cinder_auth_url is missing"
when: cinder_auth_url is not defined or not cinder_auth_url

- name: Cinder CSI Driver | check cinder_username value
- name: Cinder CSI Driver | check cinder_username value cinder_application_credential_name value
fail:
msg: "cinder_username is missing"
when: cinder_username is not defined or not cinder_username
msg: "you must either set cinder_username or cinder_application_credential_name"
when:
- cinder_username is not defined or not cinder_username
- cinder_application_credential_name is not defined or not cinder_application_credential_name

- name: Cinder CSI Driver | check cinder_application_credential_id value
fail:
msg: "cinder_application_credential_id is missing"
when:
- cinder_application_credential_name is defined
- cinder_application_credential_name|length > 0
- cinder_application_credential_id is not defined or not cinder_application_credential_id

- name: Cinder CSI Driver | check cinder_application_credential_secret value
fail:
msg: "cinder_application_credential_secret is missing"
when:
- cinder_application_credential_name is defined
- cinder_application_credential_name|length > 0
- cinder_application_credential_secret is not defined or not cinder_application_credential_secret

- name: Cinder CSI Driver | check cinder_password value
fail:
msg: "cinder_password is missing"
when: cinder_password is not defined or not cinder_password
when:
- cinder_username is defined
- cinder_username|length > 0
- cinder_application_credential_name is not defined or not cinder_application_credential_name
- cinder_application_credential_secret is not defined or not cinder_application_credential_secret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This cinder_application_credential_secret is checked at the task of line 22.
I guess we can remove this line 37 for checking cinder_password.

- cinder_password is not defined or not cinder_password

- name: Cinder CSI Driver | check cinder_region value
fail:
Expand All @@ -24,11 +47,13 @@
msg: "one of cinder_tenant_id or cinder_tenant_name must be specified"
when:
- cinder_tenant_id is not defined or not cinder_tenant_id
- cinder_tenant_name is not defined
- cinder_tenant_name is not defined or not cinder_tenant_name
- cinder_application_credential_name is not defined or not cinder_application_credential_name

- name: Cinder CSI Driver | check cinder_tenant_name value
- name: Cinder CSI Driver | check cinder_domain_id value
fail:
msg: "one of cinder_tenant_id or cinder_tenant_name must be specified"
msg: "one of cinder_domain_id or cinder_domain_name must be specified"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to replace cinder_tenant_id/name with cinder_domain_id/name?
I cannot get the point from the pull request message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So before this PR tenant_id/name was separated into separate steps, but I merged them to one and added this one for the domain_id/name which weren't checked at all.

So it looks like I replaced tenanat to domain, but that's just how git is interpreting it. I have basically just made this one more similar to this file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your explanation, I got the point.

when:
- cinder_tenant_name is not defined or not cinder_tenant_name
- cinder_tenant_id is not defined
- cinder_domain_id is not defined or not cinder_domain_id
- cinder_domain_name is not defined or not cinder_domain_name
- cinder_application_credential_name is not defined or not cinder_application_credential_name
11 changes: 11 additions & 0 deletions roles/kubernetes-apps/csi_driver/cinder/templates/cinder-csi-cloud-config.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
[Global]
auth-url="{{ cinder_auth_url }}"
{% if cinder_application_credential_id is not defined and cinder_application_credential_name is not defined %}
username="{{ cinder_username }}"
password="{{ cinder_password }}"
{% endif %}
{% if cinder_application_credential_id is defined and cinder_application_credential_id != "" %}
application-credential-id={{ cinder_application_credential_id }}
{% endif %}
{% if cinder_application_credential_name is defined and cinder_application_credential_name != "" %}
application-credential-name={{ cinder_application_credential_name }}
{% endif %}
{% if cinder_application_credential_secret is defined and cinder_application_credential_secret != "" %}
application-credential-secret={{ cinder_application_credential_secret }}
{% endif %}
region="{{ cinder_region }}"
{% if cinder_tenant_id is defined and cinder_tenant_id != "" %}
tenant-id="{{ cinder_tenant_id }}"
Expand Down
3 changes: 3 additions & 0 deletions roles/kubernetes-apps/external_cloud_controller/openstack/defaults/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
external_openstack_auth_url: "{{ lookup('env','OS_AUTH_URL') }}"
external_openstack_username: "{{ lookup('env','OS_USERNAME') }}"
external_openstack_password: "{{ lookup('env','OS_PASSWORD') }}"
external_openstack_application_credential_id: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_ID') }}"
external_openstack_application_credential_name: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_NAME') }}"
external_openstack_application_credential_secret: "{{ lookup('env','OS_APPLICATION_CREDENTIAL_SECRET') }}"
external_openstack_region: "{{ lookup('env','OS_REGION_NAME') }}"
external_openstack_tenant_id: "{{ lookup('env','OS_TENANT_ID')| default(lookup('env','OS_PROJECT_ID'),true) }}"
external_openstack_tenant_name: "{{ lookup('env','OS_TENANT_NAME')| default(lookup('env','OS_PROJECT_NAME'),true) }}"
Expand Down