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 x509.certficate_managed #56159

Conversation

svenseeberg
Copy link
Contributor

@svenseeberg svenseeberg commented Feb 13, 2020

What does this PR do?

This change decodes a bytes variable to an utf-8 string before concatenating to another string.

What issues does this PR fix or reference?

On Python 3.7 I get the following error for x509.certificate_managed, which is fixed in this PR:

An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 1919, in call
                  **cdata['kwargs'])
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1918, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/x509.py", line 560, in certificate_managed
                  'contents'] += __salt__['x509.get_pem_entry'](append_cert, pem_type='CERTIFICATE')
              TypeError: can only concatenate str (not "bytes") to str

Tests written?

[NOTICE] Bug fixes or features added to Salt require tests.
Please review the test documentation for details on how to implement tests into Salt's test suite.

No

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

@svenseeberg svenseeberg requested a review from a team as a code owner February 13, 2020 22:19
@ghost ghost requested a review from Akm0d February 13, 2020 22:19
@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:20
@DmitryKuzmenko
Copy link
Contributor

@svenseeberg thank you for contribution! Could you please take a look at my review comment?
Also could you please consider a possibility to write a test for your change? Ask for any help if you need an advice on that.

@DmitryKuzmenko DmitryKuzmenko added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Apr 20, 2020
@svenseeberg
Copy link
Contributor Author

@DmitryKuzmenko I will have a look at the tests. I have to admit I'm a little short on time though.

* Use salt.utils.stringutils.to_str to convert pem entry to string.
@svenseeberg svenseeberg force-pushed the bugfix/x509.certificate_managed branch from 393d6da to d6c552d Compare February 11, 2021 14:39
@svenseeberg
Copy link
Contributor Author

Two of my Debian Buster minions failed to refresh their certificates due to this. I rebased the branch to the current master.

Not sure how to test this. The problem seems to be specific to Debian Buster.

@svenseeberg
Copy link
Contributor Author

Oh it seems that these two nodes still used the versions packaged in the Debian main repos and the problem itself has been solved in current releases. I'll close the PR.

@svenseeberg
Copy link
Contributor Author

Oh no, the problem persists after upgrading to the current release:

----------
          ID: pki-client_cert
    Function: x509.certificate_managed
        Name: /etc/pki/client.crt
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 2154, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2106, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/x509.py", line 721, in certificate_managed
                  contents += append_file_contents
              TypeError: can only concatenate str (not "bytes") to str
     Started: 14:54:50.564209
    Duration: 368.706 ms
     Changes:   

The change that comes with this PR definitely fixes the problem.

@svenseeberg svenseeberg reopened this Feb 11, 2021
@sagetherage sagetherage requested review from Ch3LL and removed request for Akm0d March 6, 2021 00:08
@svenseeberg
Copy link
Contributor Author

It seems that this was now fixed with some other commits. With the newest version I can deploy minions without patching the x509.py.

@svenseeberg
Copy link
Contributor Author

Now this error is occuring on Ubuntu 18.04 nodes.

@svenseeberg svenseeberg reopened this Mar 26, 2021
@svenseeberg
Copy link
Contributor Author

It would be really cool to have this merged at some point. I need to manually fix the x509.py for each new minion.

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

this will require a changelog and test coverage to ensure this does not regress

@svenseeberg
Copy link
Contributor Author

I'll close the PR due to the rewrite: #58481 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants