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

Removed !-password check for salt-cloud vultr provider #33939

Merged
merged 1 commit into from
Jun 15, 2016
Merged

Removed !-password check for salt-cloud vultr provider #33939

merged 1 commit into from
Jun 15, 2016

Conversation

bx2
Copy link
Contributor

@bx2 bx2 commented Jun 10, 2016

What does this PR do?

Fixes a problem with vultr provider for salt-cloud. The current ! check in password field caused the bootstrapping process to fail as vultr no longer follows ! in every password policy. ;)

Additionally, verifying auto-generated password by checking if it contains an exclamation mark seem like a very unreliable and issue-prone idea.

What issues does this PR fix or reference?

There was no issue reported.

Previous Behavior

Before bootstrapping a node it is waiting for the API response with a default_password field. It assumes though that if the field does not contain ! then it is not a password.

New Behavior

Before bootstrapping a node it is waiting for the API response with a default_password field. If the field has content then use it as a password.

Tests written?

No

@bx2 bx2 changed the title Removed !-password check Removed !-password check for salt-cloud vultr provider Jun 10, 2016
@cachedout
Copy link
Contributor

@techhat You wrote this originally. Could you please have a look here?

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jun 13, 2016
@rallytime
Copy link
Contributor

Also related to the change and discussion in #33940

@techhat
Copy link
Contributor

techhat commented Jun 13, 2016

If the password policy has changed, then we definitely need to update to support it.

@rallytime rallytime merged commit fab1e2b into saltstack:develop Jun 15, 2016
@bx2 bx2 deleted the vultr-password-fix branch June 15, 2016 17:07
gitebra pushed a commit to gitebra/salt that referenced this pull request Jun 16, 2016
* upstream/develop:
  removed method that doesn't exist (saltstack#34026)
  Fix wheel integration test (saltstack#34040)
  Updated latest version for doc site
  remove bad logic in wait_for_default_password (saltstack#33940)
  Removed `!`-password check (saltstack#33939)
  Added check for presence of Testinfra library (saltstack#34015)
  Always make changes to minion config if set (saltstack#34022)
  expose environment specific information stored in consul (saltstack#34027)
  Whitespace fix for develop (saltstack#34016)
  Raise a warning to a user if using Py2 byte-compiled files in Py3 (saltstack#34006)
  Fix hashlib module for python 3 (saltstack#34007)
@naegelin
Copy link
Contributor

naegelin commented Oct 6, 2016

This should be merged into 2016.3 as the vultr provider will be 100% broken without it. Additionally there are many significant improvements in the develop branch that should be moved in to 2016.3 in my opinion - right now we're stuck having to check out the develop branch version of vultr.py and then merging @bx2 's latest commit ( 9df2fd1 ) in 2016.3 in order to get a fully functional vultr provider.

@rallytime
Copy link
Contributor

@naegelin I have back-ported this change to 2016.3 with #36853.

Is there something specifically (other than this fix) that is broken in the vultr.py driver? We don't usually back-port an entire driver like that, but we can certainly fix bugs as they come up. I know that this fix is necessary and that #36768 is necessary for 2016.3.3. If there is anything else needed, we'd be happy to get that fixed up. Feel free to file an issue so we can track what needs to be done.

@naegelin
Copy link
Contributor

naegelin commented Oct 7, 2016

It seems to me the 2016.3 was hastily included (clearly not tested) in the release and definitely not feature complete. For example no support for private networking and some other decent enhancements since - see https://github.com/saltstack/salt/commits/develop/salt/cloud/clouds/vultrpy.py . Any other serious vultr customers out there will not have used the 2016.3 vultr provider in production since it was nonfunctional until this merge so why not backport a more feature complete and functional version :)

@rallytime
Copy link
Contributor

ping @cro - since this is largely your driver, what do you think of this discussion?

@cro
Copy link
Contributor

cro commented Oct 7, 2016

I don't have a vested interest in it, if the develop version is a drop-in replacement we should probably bring it back to 2016.3 and carbon so we don't have to maintain two drivers.

@cro
Copy link
Contributor

cro commented Oct 7, 2016

Or maybe just carbon, it would be a pretty big change for 2016.3.4...

rallytime pushed a commit that referenced this pull request Oct 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants