search

activewarehouse / activewarehouse-etl

Extract-Transform-Load library from ActiveWarehouse
MIT License
240 stars 102 forks source link

MySqlStreamer fails on with mysql 5.6.10 on OSX #129

Open keithlayne opened 11 years ago

keithlayne commented 11 years ago

MySqlStreamer fails on any warning, because it throws when anything is written to stderr. That should probably be a separate bug.

This is what I use in development:

mysql Ver 14.14 Distrib 5.6.10, for osx10.7 (x86_64) using EditLine wrapper

I'm not that concerned about failure on this warning, because it needs to be addressed:

Warning: Using a password on the command line interface can be insecure. -- you think?

I have a quick patch forthcoming to address this issue in a superficial way.

thbar commented 11 years ago

Hello Keith!

Thanks for reporting this; please send your patch and we'll discuss it.

In particular, maybe there's something to be done related to #108 at the same time.

thbar commented 11 years ago

Just saw your patch afterwards :-) Maybe we should check the output of err.gets just to be sure?

@sgrgic I know that you're using the streamer, could you also try the patch?

I'm perfectly ok to merge it otherwise, thanks!

keithlayne commented 11 years ago

I considered that, but I was unsure if mysql varied across versions in its prompt string, or if it did i18n, or anything crazy like that. What do you think would be a easy but effective way to check on it?

BTW, your response time is impressive.

Also, I realize this isn't really a security issue for most folks (although the failure would need to be addressed somehow). We are using your gem in a somewhat ... unorthodox manner.

thbar commented 11 years ago

Re: response time: it varies largely depending on my schedule ^_^ I try to do my best to reply quickly though.

Good catch on versions/i18n, that's right. On the other hand, is there a risk that the process would be stuck if something else popped out, like a failure to connect?

Maybe we need more research here to implement something robust?

Related reading: http://dev.mysql.com/doc/refman/5.0/en/password-security-user.html

keithlayne commented 11 years ago

I'm really not sure how mysql rolls, and how different versions, etc. could cause problems. A better solution would be to specify the password some other way to avoid dealing with stderr. My goal was really just to notify you of the issue, provide a workaround (at least for my use case), and start a dialog about how it could be addressed.

I'll try to get back to this soon, and I'm happy to help if I can.

thbar commented 11 years ago

I really appreciate that you shared this in all cases, this will definitely be useful to some until we can do more research!

sgrgic commented 11 years ago

@thbar - we are not using 5.6.10, let me check with my team members and I will get back to you. We could deploy this on production and try with our etls. Thanks for notification.

keithlayne commented 11 years ago

My thoughts after reading the password security link and http://dev.mysql.com/doc/refman/5.0/en/option-files.html are that a conf file would be a decent approach. You could build one on the fly in a tempfile and pass it on the command line. This could potentially help with #108 also, but I didn't look too closely at that issue.

Another possibility would be to abstract the configuration somehow, and use that to build config files on the fly, with options to override connection defaults. However, that may be overkill here, I don't know if you are using the mysql binary anywhere else.

Thanks for your help and input!

keithlayne commented 11 years ago

BTW, it doesn't look like you can use stdin for an option file, at least on my setup.

keithlayne commented 11 years ago

I have this change in production in on of our projects (with an older version of mysql) and it appears to be performing exactly as before (which is what should happen). On development, I'm using it with no problems. If you guys want, I'll make a pull request.

I think the change is pretty straightforward, but I welcome questions, suggestions, or criticisms. Thanks!

thbar commented 11 years ago

PR welcome! Let's try this out, we'll see if we get bug reports.

keithlayne commented 11 years ago

Oops, I guess I should have asked how to attach the pull request to an existing issue before submitting :(

thbar commented 11 years ago

It's ok really :-)

Envoyé de mon iPhone

Le 8 mars 2013 à 14:55, Keith Layne notifications@github.com a écrit :

Oops, I guess I should have asked how to attach the pull request to an existing issue before submitting :(

— Reply to this email directly or view it on GitHubhttps://github.com/activewarehouse/activewarehouse-etl/issues/129#issuecomment-14620305 .