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.

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

Prefer security file over username/password #21

Merged
merged 1 commit into from
Aug 25, 2016

Conversation

thotypous
Copy link
Contributor

Currently, it is impossible to use the VNX driver in cinder (master branch) with a security file, whereas the old driver (for stable/mitaka) used to work well.

The cause of the issue is as follows

  • Storops side:
    • username and password currently take precedence over sec_file if both are defined (defined meaning different from None).
  • Cinder side:

The proposed fix inverts the precedence in storops. This way, the following configuration in cinder.conf works fine:

[vnx]
storage_vnx_pool_names=Cloud_Pool
volume_backend_name=vnx
naviseccli_path=/opt/Navisphere/bin/naviseccli
volume_driver=cinder.volume.drivers.emc.vnx.driver.EMCVNXDriver
storage_vnx_security_file_dir=/var/lib/cinder
initiator_auto_registration=True
san_ip=172.16.0.102
storage_protocol=iscsi

Another possible fix would be to consider empty strings as undefined (i.e. change if self._username is None and self._password is None to if not self._username and not self._password), but this would make Mitaka's cinder.conf even more incompatible with Newton's one, as users would be required to declare san_login as an empty string.

Otherwise, things could be fixed at the cinder side, but it feels weird to convert an empty string to None before passing it to a library. On the other hand, removing default values for san_login and san_password could have implications for users of other storage backends.

@coveralls
Copy link

coveralls commented Aug 24, 2016

Coverage Status

Coverage decreased (-0.02%) to 92.271% when pulling ef37614 on thotypous:bug/cinder-secfile into 8ea8c5a on emc-openstack:master.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage remained the same at 92.294% when pulling 3c4cd91 on thotypous:bug/cinder-secfile into 8ea8c5a on emc-openstack:master.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage remained the same at 92.294% when pulling 6fc4425 on thotypous:bug/cinder-secfile into 8ea8c5a on emc-openstack:master.

@jealous
Copy link
Contributor

jealous commented Aug 25, 2016

Hi @thotypous , thanks for the contribution. This change looks fine to me.
Could you do two more things?

  1. Add your name to the contributor list in README.md.
  2. Pop the version to 0.2.19 in README.md so that I could push a new version in pypi.

@thotypous
Copy link
Contributor Author

Hi @jealous , thank you for the review! I just updated the patch including the changes to README.md.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage decreased (-0.02%) to 92.271% when pulling 87904a5 on thotypous:bug/cinder-secfile into 8ea8c5a on emc-openstack:master.

@coveralls
Copy link

coveralls commented Aug 25, 2016

Coverage Status

Coverage decreased (-0.02%) to 92.271% when pulling 04c622f on thotypous:bug/cinder-secfile into 8ea8c5a on emc-openstack:master.

@jealous jealous merged commit 47f4b56 into emc-openstack:master Aug 25, 2016
@thotypous thotypous deleted the bug/cinder-secfile branch August 25, 2016 15:43
Tianqi-Tang pushed a commit that referenced this pull request Oct 18, 2016
Use security file option if it is not empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants