Adding test-cases for ipa-cacert-manage #1849

amore17 · 2018-04-25T08:27:27Z

Scenario1: Setup external CA1 and install ipa-server with CA1.
Setup exteranal CA2 and renew ipa-server with CA2.
Get information to compare CA change for ca1 and CA2
it should show different Issuer between install
and renewal.

Scenario2: Renew CA Cert on Replica using ipa-cacert-manage
verify that replica is caRenewalMaster

Signed-off-by: Anuja More amore@redhat.com

amore17 · 2018-04-25T09:23:53Z

Since this PR is part of Extending test coverage in upstream for ipa-cacert-manage, so no pagure ticket linked with this.

slaykovsky · 2018-04-25T12:02:22Z

Hello @amore17

Can you please add a commit to your PR and add tests you've implemented into .freeipa-pr-ci.yaml file?

It would be good to remove all others tests from this file and add only ones you need to test. Also, don't delete fedora27/build task from there. Another suggestion is to increase timeout just in case if tests will take a lot of time.

Rezney

Thanks for your patch. Please check my inline comments.

Rezney · 2018-04-26T09:03:33Z

ipatests/test_integration/test_external_ca.py

+
+class TestMultipleExternalCA(IntegrationTest):
+    """setup ext-ca1 install ipa-server with ca1
+     setup ext-ca2 renew ipa-server with ext-ca2


Sounds a little confusing. Could you re-phrase pls?

In commit it is changed to "Setup externally signed ca1
install ipa-server with with externally signed ca1
Setup externally signed ca2 and renew ipa-server with
externally signed ca2 and check the difference in certificate"

Rezney · 2018-04-26T09:03:39Z

ipatests/test_integration/test_external_ca.py

+        install_server_external_ca_step1(self.master)
+        # Sign CA, transport it to the host and get ipa a root ca paths.
+
+        test_dir1 = os.path.join(self.master.config.test_dir, 'CA1')


I would recommend to create temporary directory here e.g.:
tempfile.mkdtemp(prefix="extCA1-")

This way you do not need to do cleanup (which is missing here) later.

Applies to all other directories.

Rezney · 2018-04-26T09:06:32Z

ipatests/test_integration/test_external_ca.py

+        install_server_external_ca_step2(self.master, ipa_ca_fname1,
+                                         root_ca_fname1)
+
+        str1 = "caSigningCert cert-pki-ca"


Please use a little bit descriptive variable names. E.g. instead of "str1" you can use "ca_nick".

Rezney · 2018-04-26T09:06:56Z

ipatests/test_integration/test_external_ca.py

+                                         root_ca_fname1)
+
+        str1 = "caSigningCert cert-pki-ca"
+        info1 = self.master.run_command(['certutil', '-L', '-d',


Same here e.g. "certutil_out"

Rezney · 2018-04-26T09:09:23Z

ipatests/test_integration/test_replica_promotion.py

+        self.assertCARenewalMaster(replica, replica.hostname)
+        # set master back to ca-renewal-master
+        master.run_command([paths.IPA_CACERT_MANAGE, 'renew'])
+        self.assertCARenewalMaster(master, master.hostname)


I would also check how other server (replica in this case) sees the situation. So:

self.assertCARenewalMaster(master, master.hostname)
self.assertCARenewalMaster(replica, master.hostname)

As per your suggestion, I have added check for replica also on line 498

tiran

Thanks for your patch.

The code is technical correct. I added a couple of code style nit-picks to improve readability of code.

tiran · 2018-04-27T08:55:01Z

ipatests/test_integration/test_external_ca.py

+
+
+class TestMultipleExternalCA(IntegrationTest):
+    """Setup externally signed ca1


Please use PEP 8 / PEP 257 doc strings. You are missing an empty line after the headline. The other lines are not properly indented, too.

https://www.python.org/dev/peps/pep-0008/#documentation-strings

Thanks for your review.
I have added all changes requested by you.

tiran · 2018-04-27T08:55:09Z

ipatests/test_integration/test_external_ca.py

+     externally signed ca2 and check the difference in certificate"""
+
+    def test_master_install_ca1(self):
+


unnecessary new line

tiran · 2018-04-27T08:55:29Z

ipatests/test_integration/test_external_ca.py

+        ipa_ca_fname1 = tempfile.mkdtemp(suffix='ipa_ca.crt', dir=paths.TMP)
+
+        # Get IPA CSR as bytes
+        ipa_csr = self.master.get_file_contents(paths.ROOT_IPA_CSR,)


unnecessary trailing comma

tiran · 2018-04-27T08:58:50Z

ipatests/test_integration/test_external_ca.py

+        ipa_csr = self.master.get_file_contents(paths.ROOT_IPA_CSR,)
+
+        external_ca = ExternalCA()
+        # Create root CA


Comments like Create root CA are unnecessary noise. The following code line is already self-explaining. You are literally doing

# do action do_action()

Either remove noisy comments completely or explain the reason for each code block

# Create external root CA in order to <insert action here> external_ca = ExternalCA() root_ca = external_ca.create_ca(cn='RootCA1') ipa_ca = external_ca.sign_csr(ipa_csr)

tiran · 2018-04-27T09:02:38Z

ipatests/test_integration/test_external_ca.py

+        cert_out1 = self.master.run_command(['certutil', '-L', '-d',
+                                             paths.PKI_TOMCAT_ALIAS_DIR,
+                                             '-n', cert_nick]).stdout_text
+        assert "CN=RootCA1" in cert_out1


This block is a bit hard to read because the function arguments are heavily indented. The .stdout_text attribute access is easy to mess, too. Other test codes commonly assigns the output of run_command to a name result and then checks its stdout_text property.

result = self.master.run_command([ 'certutil', '-L', '-d', paths.PKI_TOMCAT_ALIAS_DIR, '-n', "caSigningCert cert-pki-ca" ]) assert "CN=RootCA1" in result.stdout_text

tiran · 2018-04-27T09:02:58Z

ipatests/test_integration/test_external_ca.py

+        assert "CN=RootCA1" in cert_out1
+
+    def test_master_install_ca2(self):
+


Unnecessary new line

tiran · 2018-04-27T09:04:22Z

ipatests/test_integration/test_external_ca.py

+        root_ca_fname2 = tempfile.mkdtemp(suffix='root_ca.crt', dir=paths.TMP)
+        ipa_ca_fname2 = tempfile.mkdtemp(suffix='ipa_ca.crt', dir=paths.TMP)
+
+        self.master.run_command([paths.IPA_CACERT_MANAGE, 'renew',


Try to make code more readable by minimizing indention levels and keeping arguments on line line:

self.master.run_command([ paths.IPA_CACERT_MANAGE, 'renew', '--external-ca' ])

tiran · 2018-04-27T09:04:42Z

ipatests/test_integration/test_external_ca.py

+        ipa_csr = self.master.get_file_contents(paths.IPA_CA_CSR)
+
+        external_ca = ExternalCA()
+        # Create root CA


unnecessary comments like in the other function

tiran · 2018-04-27T09:05:38Z

ipatests/test_integration/test_external_ca.py

+        cert_out2 = self.master.run_command(['certutil', '-L', '-d',
+                                             paths.PKI_TOMCAT_ALIAS_DIR,
+                                             '-n', cert_nick]).stdout_text
+        assert "CN=RootCA2" in cert_out2


Reindent both run_command calls and use result.stdout_text

tiran · 2018-04-27T09:08:07Z

ipatests/test_integration/test_external_ca.py

+        root_ca_fname1 = tempfile.mkdtemp(suffix='root_ca.crt', dir=paths.TMP)
+        ipa_ca_fname1 = tempfile.mkdtemp(suffix='ipa_ca.crt', dir=paths.TMP)
+
+        # Get IPA CSR as bytes


It's a good idea to separate logical blocks by comments or empty. But both new line and comment is a bit much and bloats the code. In most cases, a new line is a sufficient visual separator.

flo-renaud

Hi @amore17
Thanks for the patch. Please find inline comments, as well as the following suggestion:
Your PR is currently removing a bunch of tests from PR-CI and adding your new tests:

if you want to run the new tests only in this PR (but not add them in PR-CI), then you may consider adding a temp commit with changes in .freeipa-pr-ci.yaml. The temp commit would have to be removed before the maintainer pushes the PR to git repo.
otherwise add the new tests in this commit, but do not remove the other ones. They guarantee that the modifications from this PR do not break anything on the other tests.

flo-renaud · 2018-04-27T10:58:11Z

.freeipa-pr-ci.yaml

@@ -11,6 +11,10 @@ topologies:
    name: master_1repl_1client
    cpu: 4
    memory: 6700
+  master_2repl_1client: &master_2repl_1client
+    name: master_1repl_1client


Is it a typo, shouldn't this new topology user master_2repl_1client instead?

I am not sure there is a need for a new topology since the test test_replica_promotion uses only a single replica.

flo-renaud · 2018-04-27T11:00:24Z

ipatests/test_integration/test_external_ca.py

+class TestMultipleExternalCA(IntegrationTest):
+    """Setup externally signed ca1
+
+    install ipa-server with with externally signed ca1


Typo with with

tiran · 2018-04-27T12:35:02Z

@amore17 please rebase your PR on master and fix the merge conflict.

tiran · 2018-04-27T12:59:03Z

Github is still complaining about merge conflicts. Please squash all commits into a single commit and force-push your branch again.

Scenario1: Setup external CA1 and install ipa-server with CA1. Setup exteranal CA2 and renew ipa-server with CA2. Get information to compare CA change for ca1 and CA2 it should show different Issuer between install and renewal. Scenario2: Renew CA Cert on Replica using ipa-cacert-manage verify that replica is caRenewalMaster Signed-off-by: Anuja More <amore@redhat.com>

amore17 · 2018-04-27T13:35:36Z

@flo-renaud Thanks for your suggestion.
I added changes as per your review. And I removed changes from .freeipa-pr-ci.yaml
@tiran I resolved conflicts.

tiran

LGTM, good work!

@Rezney are you fine with the current state of the PR, too?

Rezney · 2018-04-27T14:00:56Z

@tiran Sure, LGTM. Thanks all for your work here...

tiran · 2018-04-27T16:07:25Z

master:

51b9a82 Adding test-cases for ipa-cacert-manage

amore17 force-pushed the renew2 branch from 3bdd9ac to b246dc3 Compare April 25, 2018 08:35

amore17 requested a review from tiran April 25, 2018 08:36

amore17 requested review from Rezney and flo-renaud April 25, 2018 09:25

amore17 added the needs review Pull Request is waiting for a review label Apr 25, 2018

amore17 requested a review from rcritten April 25, 2018 09:57

felipevolpone added the re-run Trigger a new run of PR-CI label Apr 25, 2018

freeipa-pr-ci removed re-run Trigger a new run of PR-CI labels Apr 25, 2018

amore17 force-pushed the renew2 branch 2 times, most recently from 86032fc to eb176a4 Compare April 25, 2018 13:08

amore17 added the re-run Trigger a new run of PR-CI label Apr 25, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 25, 2018

amore17 force-pushed the renew2 branch from eb176a4 to 1b03be7 Compare April 26, 2018 05:46

amore17 added the re-run Trigger a new run of PR-CI label Apr 26, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 26, 2018

amore17 force-pushed the renew2 branch from 1b03be7 to cf661b5 Compare April 26, 2018 07:40

amore17 added the re-run Trigger a new run of PR-CI label Apr 26, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 26, 2018

Rezney requested changes Apr 26, 2018

View reviewed changes

amore17 force-pushed the renew2 branch from cf661b5 to 2446e28 Compare April 26, 2018 12:19

amore17 added the re-run Trigger a new run of PR-CI label Apr 26, 2018

freeipa-pr-ci removed re-run Trigger a new run of PR-CI labels Apr 26, 2018

amore17 force-pushed the renew2 branch 2 times, most recently from fda2571 to 8e3439d Compare April 26, 2018 12:45

amore17 added the re-run Trigger a new run of PR-CI label Apr 27, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 27, 2018

Rezney approved these changes Apr 27, 2018

View reviewed changes

tiran requested changes Apr 27, 2018

View reviewed changes

tiran removed the needs review Pull Request is waiting for a review label Apr 27, 2018

amore17 force-pushed the renew2 branch 2 times, most recently from 88ea20e to 6e60e5d Compare April 27, 2018 10:17

amore17 removed the request for review from rcritten April 27, 2018 10:21

flo-renaud reviewed Apr 27, 2018

View reviewed changes

amore17 force-pushed the renew2 branch from 6e60e5d to f0be664 Compare April 27, 2018 11:53

freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Apr 27, 2018

amore17 force-pushed the renew2 branch from f0be664 to 71eee05 Compare April 27, 2018 12:33

amore17 added the re-run Trigger a new run of PR-CI label Apr 27, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 27, 2018

amore17 force-pushed the renew2 branch from 6d933ea to b1a0fce Compare April 27, 2018 13:20

amore17 removed the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Apr 27, 2018

tiran approved these changes Apr 27, 2018

View reviewed changes

Rezney added the ack Pull Request approved, can be merged label Apr 27, 2018

tiran added the re-run Trigger a new run of PR-CI label Apr 27, 2018

freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Apr 27, 2018

tiran added the pushed Pull Request has already been pushed label Apr 27, 2018

tiran closed this Apr 27, 2018

amore17 deleted the renew2 branch July 27, 2021 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding test-cases for ipa-cacert-manage #1849

Adding test-cases for ipa-cacert-manage #1849

amore17 commented Apr 25, 2018 •

edited by tiran

amore17 commented Apr 25, 2018

slaykovsky commented Apr 25, 2018

Rezney left a comment

Rezney Apr 26, 2018

amore17 Apr 26, 2018

Rezney Apr 26, 2018

Rezney Apr 26, 2018

Rezney Apr 26, 2018

Rezney Apr 26, 2018

amore17 Apr 27, 2018

tiran left a comment

tiran Apr 27, 2018

amore17 Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

tiran Apr 27, 2018

flo-renaud left a comment

flo-renaud Apr 27, 2018

flo-renaud Apr 27, 2018

tiran commented Apr 27, 2018

tiran commented Apr 27, 2018

amore17 commented Apr 27, 2018

tiran left a comment

Rezney commented Apr 27, 2018

tiran commented Apr 27, 2018



		class TestMultipleExternalCA(IntegrationTest):
		"""Setup externally signed ca1

		externally signed ca2 and check the difference in certificate"""

		def test_master_install_ca1(self):

		assert "CN=RootCA1" in cert_out1

		def test_master_install_ca2(self):

Adding test-cases for ipa-cacert-manage #1849

Adding test-cases for ipa-cacert-manage #1849

Conversation

amore17 commented Apr 25, 2018 • edited by tiran

amore17 commented Apr 25, 2018

slaykovsky commented Apr 25, 2018

Rezney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flo-renaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Apr 27, 2018

tiran commented Apr 27, 2018

amore17 commented Apr 27, 2018

tiran left a comment

Choose a reason for hiding this comment

Rezney commented Apr 27, 2018

tiran commented Apr 27, 2018

amore17 commented Apr 25, 2018 •

edited by tiran