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

change split on whitespace to split on tab in mysql_user #1233

Merged
merged 6 commits into from
Jun 12, 2020
Merged

change split on whitespace to split on tab in mysql_user #1233

merged 6 commits into from
Jun 12, 2020

Conversation

koshatul
Copy link
Contributor

What change does this introduce?

max_user_connections, @max_connections_per_hour, @max_queries_per_hour,
      @max_updates_per_hour, ssl_type, ssl_cipher, x509_issuer, x509_subject,
      @password, @plugin = mysql_caller(query, 'regular').split(%r{\s})

splits on whitespace, so if the issuer or subject for the user certificate contains spaces it will break all following fields.

Why make this change?

When using certificate subjects or issuers with spaces, the split fails and breaks the parsing of the user from MySQL.

puppet code:

  mysql_user { "koshatul@%":
    ensure      => 'present',
    tls_options => [
      "SUBJECT /OU=Users/CN=koshatul",
      "ISSUER /O=Acme Widgets/OU=Users/CN=Intermediate Certificate Authority",
    ],
  }

will show this notice on every single puppet report.

Notice: /Stage[main]/Testing::Mysql/Testing::Mysql::Userwithssl[koshatul]/Mysql_user[koshatul@%]/tls_options: tls_options changed ['ISSUER /O=Acme', 'SUBJECT Widgets/OU=Users/CN=Intermediate'] to ['SUBJECT /OU=Users/CN=koshatul', 'ISSUER /O=Acme Widgets/OU=Users/CN=Intermediate Certificate Authority'] (corrective)

What approach will be taken?

Change \s to \t.
the command is run with -B which adds --silent.

-s, --silent Be more silent. Print results with a tab as separator,
each row on new line.

@koshatul koshatul requested a review from a team as a code owner September 24, 2019 05:16
@koshatul
Copy link
Contributor Author

I don't think any of the travis failures are related to my change.

Copy link
Contributor

@michaeltlombardi michaeltlombardi left a comment

Choose a reason for hiding this comment

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

These failures do seem to be related to your change, at least to my eyes, especially this failing test which seems to be directly failing because the different values are not tab separated, at least in the spec test itself.

@koshatul
Copy link
Contributor Author

koshatul commented Oct 8, 2019

Sorry about that, I'll see if I can find if/when the output changed for the mysql command line.

@koshatul
Copy link
Contributor Author

koshatul commented Oct 8, 2019

The documentation on mysql's website only goes back as far as 5.5, it has tab separation for the batch-mode output.

https://dev.mysql.com/doc/refman/5.5/en/mysql-command-options.html#option_mysql_batch

--batch, -B

Print results using tab as the column separator, with each row on a new line. With this option, mysql does not use the history file.

Batch mode results in nontabular output format and escaping of special characters. Escaping may be disabled by using raw mode; see the description for the --raw option.

The spec test seems to have been spaces since it was refactored from database_user, so I think this is actually broken in the real world, it's just lucky that the commonly used fields rarely if ever have spaces.

Surely I can't be the first person to have issues with the TLS config though (unless I'm also the first to use a TLS CA with spaces in the name).

@carabasdaniel
Copy link
Contributor

Hi @koshatul

Thank you for submitting this PR.

I've added a commit to fix the unit testing for this change, however it appears there are some acceptance testing failures caused by this change that you can verify in travis.

Can you verify and fix the testing for those failures ? You might be able to run the tests on your local machine with Litmus to have an easier time getting these issues solved. Please let us know if you are having trouble with this.

@david22swan
Copy link
Member

@koshatul Hey, sorry to bother you but I'm just checking to see whether or not you intended to return to this work at any point in time?

@koshatul
Copy link
Contributor Author

koshatul commented Jun 2, 2020

@david22swan sorry, been a bit busy lately.

I merged back in master and fixed the issue, I'm about to head off for the day so I'll check later or tomorrow if the tests passed.

@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2020

Codecov Report

Merging #1233 into master will increase coverage by 5.45%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1233      +/-   ##
==========================================
+ Coverage   50.23%   55.69%   +5.45%     
==========================================
  Files          22       22              
  Lines        1045     1045              
==========================================
+ Hits          525      582      +57     
+ Misses        520      463      -57     
Impacted Files Coverage Δ
lib/puppet/provider/mysql_user/mysql.rb 87.58% <25.00%> (ø)
...ppet/provider/mysql_login_path/mysql_login_path.rb 90.32% <0.00%> (+61.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23f89af...698c21c. Read the comment docs.

@koshatul
Copy link
Contributor Author

koshatul commented Jun 2, 2020

I'm writing some acceptance tests for it, but I've noticed another issue is that ISSUER and SUBJECT require a quoted parameter.

https://dev.mysql.com/doc/refman/5.7/en/create-user.html

tls_option: {
   SSL
 | X509
 | CIPHER 'cipher'
 | ISSUER 'issuer'
 | SUBJECT 'subject'
}

but tls_options returns unquoted options, so the idempotent_apply will fail in acceptance, but there's no test for it at the moment.

@koshatul
Copy link
Contributor Author

koshatul commented Jun 4, 2020

Not sure how to acknowledge or complete the change request, but I've added acceptance and spec tests and I've actually pointed the Puppetfile in my local environments at my fork to test it, so it also works for me.

Copy link
Contributor Author

@koshatul koshatul left a comment

Choose a reason for hiding this comment

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

I've changed this after merging master back in.

@david22swan
Copy link
Member

@koshatul Looking over this I can't see one thing wrong so I'm gonna go ahead and merge. :)
Thanks for putting in the work

@david22swan david22swan merged commit a09ca74 into puppetlabs:master Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants