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

Update Venafi module #55286

Closed
wants to merge 44 commits into from
Closed

Update Venafi module #55286

wants to merge 44 commits into from

Conversation

arykalin
Copy link
Contributor

What does this PR do?

Transitions solution to Venafi preferred integration interface (VCert-Python) and updated to align with Venafi standard use cases for DevOps

Copy of #54695 merged with master branch

What issues does this PR fix or reference?

Restores interoperability with Venafi Cloud

Previous Behavior

Supported only Venafi Cloud

New Behavior

Adds support for Venafi Trust Protection Platform

Tests written?

Yes

Commits signed with GPG?

No

@arykalin arykalin requested a review from a team as a code owner November 13, 2019 15:01
@ghost ghost requested a review from dwoz November 13, 2019 15:01
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

@arykalin thanks for the PR and getting everything shifted to master!

This is mostly looking good - there are a couple of minor style changes that will make this 💯- I don't see any glaring issues.

Though there are some test failures, that I believe are due to requirements. IIRC we need to run pre-commit to rebuild the dependencies. You should be able to just run pre-commit run -av, which will say "failed" the first time.

Let me know if you run into any problems!

tests/integration/externalapi/test_venafiapi.py Outdated Show resolved Hide resolved
salt/runners/venafiapi.py Outdated Show resolved Hide resolved
salt/runners/venafiapi.py Show resolved Hide resolved
salt/runners/venafiapi.py Outdated Show resolved Hide resolved
tests/integration/externalapi/test_venafiapi.py Outdated Show resolved Hide resolved
tests/integration/externalapi/test_venafiapi.py Outdated Show resolved Hide resolved
tests/integration/externalapi/test_venafiapi.py Outdated Show resolved Hide resolved
tests/integration/externalapi/test_venafiapi.py Outdated Show resolved Hide resolved
@arykalin arykalin requested a review from waynew November 27, 2019 10:28
@dwoz
Copy link
Contributor

dwoz commented Dec 10, 2019

@arykalin The conflicts need to be resolved, The requirements from master should be used.

@arykalin
Copy link
Contributor Author

@dwoz this requirements was added by pre-commit run -av which @waynew recommended to run since our tests require vcert dependency. How I can add this dependency to tests?

…e-venafi-module

# Conflicts:
#	requirements/static/py2.7/linux.txt
#	requirements/static/py3.4/linux.txt
#	requirements/static/py3.5/linux.txt
#	requirements/static/py3.6/linux.txt
#	requirements/static/py3.7/linux.txt
@arykalin
Copy link
Contributor Author

I merged with master and run pre-commit run -av again. Will it fit?

@waynew waynew mentioned this pull request Dec 17, 2019
@waynew
Copy link
Contributor

waynew commented Dec 17, 2019

Hey @arykalin thanks for the updates! There is one issue unrelated to this PR in the failed tests, but there are a couple of failures that are related to Venafi:

https://jenkinsci.saltstack.com/job/pr-ubuntu1804-py3/job/PR-55286/18/testReport/

Traceback (most recent call last):
  File "/tmp/kitchen/testing/tests/integration/externalapi/test_venafiapi.py", line 38, in wrapper
    return func(self, _random_name(prefix='salt-test-'), *args, **kwargs)
  File "/tmp/kitchen/testing/tests/integration/externalapi/test_venafiapi.py", line 59, in test_request
    cert = x509.load_pem_x509_certificate(cert_output.encode(), default_backend())
  File "/tmp/kitchen/testing/.nox/runtests-parametrized-3-coverage-true-crypto-none-transport-zeromq/lib/python3.6/site-packages/cryptography/x509/base.py", line 50, in load_pem_x509_certificate
    return backend.load_pem_x509_certificate(data)
  File "/tmp/kitchen/testing/.nox/runtests-parametrized-3-coverage-true-crypto-none-transport-zeromq/lib/python3.6/site-packages/cryptography/hazmat/backends/openssl/backend.py", line 1170, in load_pem_x509_certificate
    "Unable to load certificate. See https://cryptography.io/en/la"
ValueError: Unable to load certificate. See https://cryptography.io/en/latest/faq/#why-can-t-i-import-my-pem-file for more details.	

Do you need any help looking into that?

@arykalin
Copy link
Contributor Author

@waynew fixed tests

@waynew
Copy link
Contributor

waynew commented Jan 2, 2020

Almost there - looks like https://jenkinsci.saltstack.com/job/pr-macosxmojave-py2/job/PR-55286/19/ failed as did the py3 Mojave build.

@waynew
Copy link
Contributor

waynew commented Jan 7, 2020

@arykalin Looks like a dependency isn't getting installed?

 ('return is', {u'fun': 'venafi.request', u'jid': u'20200106233150621490', u'return': u"'venafi.request' is not available.", u'out': ["'venafi.request' is not available."]})
cert out is: '

@arykalin
Copy link
Contributor Author

arykalin commented Jan 9, 2020

@arykalin Looks like a dependency isn't getting installed?

 ('return is', {u'fun': 'venafi.request', u'jid': u'20200106233150621490', u'return': u"'venafi.request' is not available.", u'out': ["'venafi.request' is not available."]})
cert out is: '

@waynew yes, I didn't add vcert to darwin.in file

@waynew
Copy link
Contributor

waynew commented Jan 9, 2020

We can probably remove the print lines now, too. Or we can wait until the mac builds run 🙃

If you do remove them, you could rebase the changes on master, too.

@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #55286 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #55286   +/-   ##
=======================================
  Coverage   18.79%   18.79%           
=======================================
  Files         821      821           
  Lines      175155   175155           
  Branches    37695    37695           
=======================================
  Hits        32910    32910           
  Misses     139568   139568           
  Partials     2677     2677
Flag Coverage Δ
#archlts 18.07% <0%> (ø) ⬆️
#centos7 23.74% <0%> (ø) ⬆️
#proxy 23.78% <0%> (ø) ⬆️
#py2 18.59% <0%> (ø) ⬆️
#py3 18.42% <0%> (ø) ⬆️
#runtests 18.79% <0%> (ø) ⬆️
#ubuntu1604 23.71% <0%> (ø) ⬆️
#zeromq 18.79% <0%> (ø) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #55286 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #55286   +/-   ##
=======================================
  Coverage   18.79%   18.79%           
=======================================
  Files         821      821           
  Lines      175155   175155           
  Branches    37695    37695           
=======================================
  Hits        32910    32910           
  Misses     139568   139568           
  Partials     2677     2677
Flag Coverage Δ
#archlts 18.07% <0%> (ø) ⬆️
#centos7 23.74% <0%> (ø) ⬆️
#proxy 23.78% <0%> (ø) ⬆️
#py2 18.59% <0%> (ø) ⬆️
#py3 18.42% <0%> (ø) ⬆️
#runtests 18.79% <0%> (ø) ⬆️
#ubuntu1604 23.71% <0%> (ø) ⬆️
#zeromq 18.79% <0%> (ø) ⬆️

@codecov
Copy link

codecov bot commented Jan 9, 2020

Codecov Report

Merging #55286 into master will increase coverage by 0.83%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #55286      +/-   ##
==========================================
+ Coverage   17.97%   18.79%   +0.83%     
==========================================
  Files        1238      821     -417     
  Lines      240782   175155   -65627     
  Branches    52795    37695   -15100     
==========================================
- Hits        43265    32910   -10355     
+ Misses     193686   139568   -54118     
+ Partials     3831     2677    -1154
Flag Coverage Δ
#archlts 18.07% <ø> (+1.82%) ⬆️
#centos7 23.74% <ø> (-0.95%) ⬇️
#cloud ?
#proxy 23.78% <ø> (-0.03%) ⬇️
#py2 18.59% <ø> (+1.76%) ⬆️
#py3 18.42% <ø> (+0.73%) ⬆️
#runtests 18.79% <ø> (+0.83%) ⬆️
#ubuntu1604 23.71% <ø> (-0.03%) ⬇️
#zeromq 18.79% <ø> (+1.8%) ⬆️
Impacted Files Coverage Δ
salt/utils/xmlutil.py 13.64% <0%> (-40.9%) ⬇️
salt/utils/smb.py 19.84% <0%> (-39.27%) ⬇️
salt/utils/cloud.py 9.32% <0%> (-31.96%) ⬇️
salt/utils/vt.py 10.76% <0%> (-28.27%) ⬇️
salt/cloud/__init__.py 7.78% <0%> (-20.82%) ⬇️
salt/output/yaml_out.py 45% <0%> (-20%) ⬇️
salt/config/__init__.py 27.63% <0%> (-16.84%) ⬇️
salt/utils/http.py 11.31% <0%> (-15.3%) ⬇️
salt/utils/validate/path.py 40% <0%> (-10%) ⬇️
salt/utils/decorators/__init__.py 50.43% <0%> (-8.35%) ⬇️
... and 546 more

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@511eaa9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #55286   +/-   ##
=========================================
  Coverage          ?   18.79%           
=========================================
  Files             ?      821           
  Lines             ?   175155           
  Branches          ?    37695           
=========================================
  Hits              ?    32910           
  Misses            ?   139568           
  Partials          ?     2677
Flag Coverage Δ
#archlts 18.07% <0%> (?)
#centos7 23.74% <0%> (?)
#proxy 23.78% <0%> (?)
#py2 18.59% <0%> (?)
#py3 18.42% <0%> (?)
#runtests 18.79% <0%> (?)
#ubuntu1604 23.71% <0%> (?)
#zeromq 18.79% <0%> (?)

@arykalin
Copy link
Contributor Author

Hmm, can't understand why pylint fails, it was passing before :(

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@511eaa9). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #55286   +/-   ##
=========================================
  Coverage          ?   18.79%           
=========================================
  Files             ?      821           
  Lines             ?   175155           
  Branches          ?    37695           
=========================================
  Hits              ?    32910           
  Misses            ?   139568           
  Partials          ?     2677
Flag Coverage Δ
#archlts 18.07% <0%> (?)
#centos7 23.74% <0%> (?)
#proxy 23.78% <0%> (?)
#py2 18.59% <0%> (?)
#py3 18.42% <0%> (?)
#runtests 18.79% <0%> (?)
#ubuntu1604 23.71% <0%> (?)
#zeromq 18.79% <0%> (?)

salt/runners/venafiapi.py Outdated Show resolved Hide resolved
salt/runners/venafiapi.py Outdated Show resolved Hide resolved
@arykalin
Copy link
Contributor Author

@waynew ci/py3/ubuntu1604 build failing with unknown reason, could you look?

@waynew
Copy link
Contributor

waynew commented Jan 13, 2020

Not sure exactly what failed there either - I went ahead and triggered a rebuild.

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🎉

@waynew
Copy link
Contributor

waynew commented Jan 13, 2020

@arykalin woohoo! We've got the build passing now, success!

Copy link
Collaborator

@s0undt3ch s0undt3ch left a comment

Choose a reason for hiding this comment

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

There's a bug when our compile requirement pre-commit script runs in some situations.

I'll update requirements and push the fixed requirements.

@s0undt3ch s0undt3ch mentioned this pull request Jan 14, 2020
@s0undt3ch
Copy link
Collaborator

This PR was resubmitted with a more concise git history and fixed requirements files in #55858 since I didn't have push access to the venafi fork.

@s0undt3ch s0undt3ch closed this Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants