copr / copr

Clone

#2177 Fix creating keys and signing in docker-compose
Merged 4 months ago by praiskup. Opened 4 months ago by frostyx.
copr/ frostyx/copr fix-gpg  into  main

file modified
+1 
@@ -27,6 +27,7 @@ 
 

          None: False,
 

      }
 

      test_paths = {
 

+         "backend/tests",
 

          "cli/tests",
 

          "common/tests",
 

          "dist-git/tests",

file modified
+9 -5 
@@ -5,8 +5,8 @@ 
 

  """
 

  

  from subprocess import Popen, PIPE, SubprocessError
 

- import json
 

  import os
 

+ import time
 

  

  from packaging import version
 

  
@@ -43,8 +43,12 @@ 
 

              new_err = CoprSignError("Failed to invoke '{}'".format(cmd_pretty))
 

              raise new_err from err
 

  

-         if handle.returncode != 0 and "Connection timed out" in stderr:
 

-             log.warning("Timeout on %s, re-trying", cmd_pretty)
 

+         if handle.returncode != 0:
 

+             log.warning("Command '%s' failed with: %s",
 

+                         cmd_pretty, stderr.rstrip())
 

+             sleeptime = 20
 

+             log.warning("Going to sleep %ss and re-try.", sleeptime)
 

+             time.sleep(sleeptime)
 

              continue
 

          break
 

      return handle.returncode, stdout, stderr
 
@@ -189,10 +193,10 @@ 
 

  

      :return: None
 

      """
 

-     data = json.dumps({
 

+     data = {
 

          "name_real": "{}_{}".format(username, projectname),
 

          "name_email": create_gpg_email(username, projectname)
 

-     })
 

+     }
 

  

      log = get_redis_logger(opts, "sign", "actions")
 

      keygen_url = "http://{}/gen_key".format(opts.keygen_host)

file modified
+25 -5 
@@ -12,6 +12,7 @@ 
 

  from copr_backend.sign import (
 

      get_pubkey, _sign_one, sign_rpms_in_dir, create_user_keys,
 

      gpg_hashtype_for_chroot,
 

+     call_sign_bin,
 

  )
 

  

  STDOUT = "stdout"
 
@@ -19,6 +20,7 @@ 
 

  

  

  class TestSign(object):
 

+     # pylint: disable=too-many-public-methods
 

  

      def setup_method(self, method):
 

          self.username = "foo"
 
@@ -70,8 +72,9 @@ 
 

              get_pubkey(self.username, self.projectname, MagicMock())
 

  

  

+     @mock.patch("copr_backend.sign.time.sleep")
 

      @mock.patch("copr_backend.sign.Popen")
 

-     def test_get_pubkey_unknown_key(self, mc_popen):
 

+     def test_get_pubkey_unknown_key(self, mc_popen, _sleep):
 

          mc_handle = MagicMock()
 

          mc_handle.communicate.return_value = (STDOUT, "unknown key: foobar")
 

          mc_handle.returncode = 1
 
@@ -82,8 +85,9 @@ 
 

  

          assert "There are no gpg keys for user foo in keyring" in str(err)
 

  

+     @mock.patch("copr_backend.sign.time.sleep")
 

      @mock.patch("copr_backend.sign.Popen")
 

-     def test_get_pubkey_unknown_error(self, mc_popen):
 

+     def test_get_pubkey_unknown_error(self, mc_popen, _sleep):
 

          mc_handle = MagicMock()
 

          mc_handle.communicate.return_value = (STDOUT, STDERR)
 

          mc_handle.returncode = 1
 
@@ -134,8 +138,9 @@ 
 

          with pytest.raises(CoprSignError):
 

              _sign_one(fake_path, self.usermail, "sha256", MagicMock())
 

  

+     @mock.patch("copr_backend.sign.time.sleep")
 

      @mock.patch("copr_backend.sign.Popen")
 

-     def test_sign_one_cmd_erro(self, mc_popen):
 

+     def test_sign_one_cmd_erro(self, mc_popen, _sleep):
 

          mc_handle = MagicMock()
 

          mc_handle.communicate.return_value = (STDOUT, STDERR)
 

          mc_handle.returncode = 1
 
@@ -145,6 +150,21 @@ 
 

          with pytest.raises(CoprSignError):
 

              _sign_one(fake_path, self.usermail, "sha256", MagicMock())
 

  

+     @staticmethod
 

+     @mock.patch("copr_backend.sign.time.sleep")
 

+     @mock.patch("copr_backend.sign.Popen")
 

+     def test_call_sign_bin_repeatedly(mc_popen, _sleep):
 

+         """
 

+         Test that we attempt to run /bin/sign multiple times if it returns
 

+         non-zero exit status
 

+         """
 

+         mc_handle = MagicMock()
 

+         mc_handle.communicate.return_value = (STDOUT, STDERR)
 

+         mc_handle.returncode = 1
 

+         mc_popen.return_value = mc_handle
 

+         call_sign_bin(cmd=[], log=MagicMock())
 

+         assert mc_popen.call_count == 3
 

+ 

      @mock.patch("copr_backend.sign.SafeRequest.send")
 

      def test_create_user_keys(self, mc_request):
 

          mc_request.return_value.status_code = 200
 
@@ -153,7 +173,7 @@ 
 

          assert mc_request.called
 

          expected_call = mock.call(
 

              url="http://example.com/gen_key",
 

-             data='{"name_real": "foo_bar", "name_email": "foo_bar@copr.fedorahosted.org"}',
 

+             data={"name_real": "foo_bar", "name_email": "foo#bar@copr.fedorahosted.org"},
 

              method="post"
 

          )
 

          assert mc_request.call_args == expected_call
 
@@ -168,7 +188,7 @@ 
 

  

  

      @mock.patch("copr_backend.sign.SafeRequest.send")
 

-     def test_create_user_keys(self, mc_request):
 

+     def test_create_user_keys_err(self, mc_request):
 

          for code in [400, 401, 404, 500, 599]:
 

              mc_request.return_value.status_code = code
 

              mc_request.return_value.content = "error: {}".format(code)

  • I introduced a bug in PR#2166
  • Also, I am fixing the signing in docker-compose that currently doesn't work. I don't like the fix very much, so if you understand why it doesn't work, please let me know

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 7f11cd2262336b85a5e1a5f78d92e2d8be8fe0b6
4 months ago

Build succeeded.

2 new commits added

  • backend: attempt to sign multiple times
  • backend: leave the JSON conversion to SafeRequest
4 months ago

Also, I am fixing the signing in docker-compose that currently doesn't work. I don't like the fix very much, so if you understand why it doesn't work, please let me know

This was an error on my side, so I dropped the commit.

I've added a new commit to this PR that tries multiple attempts to sign packages.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

2 new commits added

  • backend: attempt to sign multiple times
  • backend: leave the JSON conversion to SafeRequest
4 months ago

Build succeeded.

I don't really know what caused #2133,

Seems like a typo

I'm pretty sure we should keep handling the "returncode != 0" situation.

Test case in both commits would be nice. First seems easy (just test that single serialization happened), and for the second there's the 'fake-bin-sign' helper that can react on e.g. environment variables or something like that.

rebased onto 196aff4e37904d4dc22fa25d2690cc4c5b46e065
4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

I'm pretty sure we should keep handling the "returncode != 0" situation.

True, I dropped the commit and wrote it differently.

Test case in both commits would be nice.

Added some tests, PTAL.
Zuul fails because of some tests errors but we should IMHO ignore those suggestions for tests.

Per yesterday's debate, I think we could just use:

diff --git a/backend/copr_backend/sign.py b/backend/copr_backend/sign.py
index 9c7e8e8d11..fd725a128c 100644
--- a/backend/copr_backend/sign.py
+++ b/backend/copr_backend/sign.py
@@ -43,8 +43,8 @@ def call_sign_bin(cmd, log):
             new_err = CoprSignError("Failed to invoke '{}'".format(cmd_pretty))
             raise new_err from err

-        if handle.returncode != 0 and "Connection timed out" in stderr:
-            log.warning("Timeout on %s, re-trying", cmd_pretty)
+        if handle.returncode != 0:
+            log.warning("Command %s error, re-trying", cmd_pretty)
             continue
         break
     return handle.returncode, stdout, stderr

The linter could be fixed by @staticmethod too.

rebased onto b1b810a
4 months ago

In some of the previous versions, you proposed some sleep here (to allow the system to recover) ... that still seems like a good idea!

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Nice catch here btw.

3 new commits added

  • pylint: ignore also test-only pylint errors for backend
  • backend: attempt to sign multiple times
  • backend: leave the JSON conversion to SafeRequest
4 months ago

Build succeeded.

+1 fighting with pagure today, sorry
Edited 4 months ago by praiskup

Don't merge yet

3 new commits added

  • pylint: ignore also test-only pylint errors for backend
  • backend: attempt to sign multiple times
  • backend: leave the JSON conversion to SafeRequest
4 months ago

Build succeeded.

Per discussion with @praiskup, I added sleep between attempts.

3 new commits added

  • pylint: ignore also test-only pylint errors for backend
  • backend: attempt to sign multiple times
  • backend: leave the JSON conversion to SafeRequest
4 months ago

Build succeeded.

PTAL

+1

Commit 96f7e00 fixes this pull-request

Pull-Request has been merged by praiskup
4 months ago

Commit bb08872 fixes this pull-request

Pull-Request has been merged by praiskup
4 months ago

Commit a473ba4 fixes this pull-request

Pull-Request has been merged by praiskup
4 months ago
Metadata
None
No Tags
Changes Summary 3
+1 -0
file changed
.pylintpath/pylint_copr_plugin.py
+9 -5
file changed
backend/copr_backend/sign.py
+25 -5
file changed
backend/tests/test_sign.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.