#2042 add-host work even if host already tried to log in
Merged 4 months ago by tkopecek. Opened 5 months ago by tkopecek.
tkopecek/koji issue1874  into  master

file modified
+5 -5

@@ -187,6 +187,8 @@ 

      parser = OptionParser(usage=get_usage_str(usage))

      parser.add_option("--krb-principal",

                        help=_("set a non-default kerberos principal for the host"))

+     parser.add_option("--force", default=False, action="store_true",

+                       help=_("if existing used is a regular user, convert it to a host"))

      (options, args) = parser.parse_args(args)

      if len(args) < 2:

          parser.error(_("Please specify a hostname and at least one arch"))

@@ -194,15 +196,13 @@ 

      activate_session(session, goptions)

      id = session.getHost(host)

      if id:

-         print("%s is already in the database" % host)

-         return 1

+         error("%s is already in the database" % host)

      else:

-         kwargs = {}

+         kwargs = {'force': options.force}

          if options.krb_principal is not None:

              kwargs['krb_principal'] = options.krb_principal

          id = session.addHost(host, args[1:], **kwargs)

-         if id:

-             print("%s added: id %d" % (host, id))

+         print("%s added: id %d" % (host, id))

  

  

  def handle_edit_host(options, session, args):

file modified
+27 -8

@@ -12112,7 +12112,7 @@ 

                           arch=taskInfo['arch'], channel=channel['name'],

                           priority=taskInfo['priority'])

  

-     def addHost(self, hostname, arches, krb_principal=None):

+     def addHost(self, hostname, arches, krb_principal=None, force=False):

          """

          Add a builder host to the database.

  

@@ -12120,6 +12120,7 @@ 

          :param list arches: list of architectures this builder supports.

          :param str krb_principal: (optional) a non-default kerberos principal

                                    for the host.

+         :param bool force: override user type

          :returns: new host id

  

          If krb_principal is not given then that field will be generated

@@ -12133,13 +12134,31 @@ 

              raise koji.GenericError('host already exists: %s' % hostname)

          q = """SELECT id FROM channels WHERE name = 'default'"""

          default_channel = _singleValue(q)

-         if krb_principal is None:

-             fmt = context.opts.get('HostPrincipalFormat')

-             if fmt:

-                 krb_principal = fmt % hostname

-         # users entry

-         userID = context.session.createUser(hostname, usertype=koji.USERTYPES['HOST'],

-                                             krb_principal=krb_principal)

+         # builder user can already exist, if host tried to log in before adding into db

+         userinfo = {'name': hostname}

+         if krb_principal:

+             userinfo['krb_principal'] = krb_principal

+         user = get_user(userInfo=userinfo)

+         if user:

+             if user['usertype'] != koji.USERTYPES['HOST']:

+                 if force and user['usertype'] == koji.USERTYPES['NORMAL']:

+                     # override usertype in this special case

+                     update = UpdateProcessor('users',

+                                              values={'userID': user['id']},

+                                              clauses=['id = %(userID)i'])

+                     update.set(usertype=koji.USERTYPES['HOST'])

+                     update.execute()

+                 else:

+                     raise koji.GenericError(

+                         'user %s already exists and it is not a host' % hostname)

+             userID = user['id']

+         else:

+             if krb_principal is None:

+                 fmt = context.opts.get('HostPrincipalFormat')

+                 if fmt:

+                     krb_principal = fmt % hostname

+             userID = context.session.createUser(hostname, usertype=koji.USERTYPES['HOST'],

+                                                 krb_principal=krb_principal)

          # host entry

          hostID = _singleValue("SELECT nextval('host_id_seq')", strict=True)

          insert = "INSERT INTO host (id, user_id, name) VALUES (%(hostID)i, %(userID)i, " \

file modified
+12 -10

@@ -8,6 +8,7 @@ 

  except ImportError:

      import unittest

  

+ import koji

  from koji_cli.commands import handle_add_host

  

  class TestAddHost(unittest.TestCase):

@@ -24,7 +25,7 @@ 

          krb_principal = '--krb-principal=krb'

          arguments = [host] + arches

          arguments.append(krb_principal)

-         kwargs = {'krb_principal': 'krb'}

+         kwargs = {'krb_principal': 'krb', 'force': False}

          options = mock.MagicMock()

  

          # Mock out the xmlrpc server

@@ -69,12 +70,12 @@ 

          # Finally, assert that things were called as we expected.

          activate_session_mock.assert_called_once_with(session, options)

          session.getHost.assert_called_once_with(host)

-         session.addHost.assert_called_once_with(host, arches)

+         session.addHost.assert_called_once_with(host, arches, force=False)

          self.assertNotEqual(rv, 1)

  

-     @mock.patch('sys.stdout', new_callable=six.StringIO)

+     @mock.patch('sys.stderr', new_callable=six.StringIO)

      @mock.patch('koji_cli.commands.activate_session')

-     def test_handle_add_host_dupl(self, activate_session_mock, stdout):

+     def test_handle_add_host_dupl(self, activate_session_mock, stderr):

          host = 'host'

          host_id = 1

          arches = ['arch1', 'arch2']

@@ -89,15 +90,15 @@ 

          # Run it and check immediate output

          # args: host, arch1, arch2, --krb-principal=krb

          # expected: failed, host already exists

-         rv = handle_add_host(options, session, arguments)

-         actual = stdout.getvalue()

+         with self.assertRaises(SystemExit):

+             handle_add_host(options, session, arguments)

+         actual = stderr.getvalue()

          expected = 'host is already in the database\n'

          self.assertMultiLineEqual(actual, expected)

          # Finally, assert that things were called as we expected.

          activate_session_mock.assert_called_once_with(session, options)

          session.getHost.assert_called_once_with(host)

          session.addHost.assert_not_called()

-         self.assertEqual(rv, 1)

  

      @mock.patch('sys.stdout', new_callable=six.StringIO)

      @mock.patch('sys.stderr', new_callable=six.StringIO)

@@ -141,18 +142,19 @@ 

          krb_principal = '--krb-principal=krb'

          arguments = [host] + arches

          arguments.append(krb_principal)

-         kwargs = {'krb_principal': 'krb'}

+         kwargs = {'krb_principal': 'krb', 'force': False}

          options = mock.MagicMock()

  

          # Mock out the xmlrpc server

          session = mock.MagicMock()

  

          session.getHost.return_value = None

-         session.addHost.return_value = None

+         session.addHost.side_effect = koji.GenericError

          # Run it and check immediate output

          # args: host, arch1, arch2, --krb-principal=krb

          # expected: failed

-         handle_add_host(options, session, arguments)

+         with self.assertRaises(koji.GenericError):

+             handle_add_host(options, session, arguments)

          actual = stdout.getvalue()

          expected = ''

          self.assertMultiLineEqual(actual, expected)

@@ -77,3 +77,71 @@ 

          self.assertEqual(_dml.call_count, 1)

          _dml.assert_called_once_with("INSERT INTO host (id, user_id, name) VALUES (%(hostID)i, %(userID)i, %(hostname)s)",

                        {'hostID': 12, 'userID': 456, 'hostname': 'hostname'})

+ 

+     @mock.patch('kojihub.get_user')

+     @mock.patch('kojihub._dml')

+     @mock.patch('kojihub.get_host')

+     @mock.patch('kojihub._singleValue')

+     def test_add_host_wrong_user(self, _singleValue, get_host, _dml, get_user):

+         get_user.return_value = {

+             'id': 1,

+             'name': 'hostname',

+             'usertype': koji.USERTYPES['NORMAL']

+         }

+         get_host.return_value = {}

+         with self.assertRaises(koji.GenericError):

+             self.exports.addHost('hostname', ['i386', 'x86_64'])

+         _dml.assert_not_called()

+         get_user.assert_called_once_with(userInfo={'name': 'hostname'})

+         get_host.assert_called_once_with('hostname')

+         _singleValue.assert_called_once()

+         self.assertEqual(len(self.inserts), 0)

+         self.assertEqual(len(self.updates), 0)

+ 

+     @mock.patch('kojihub.get_user')

+     @mock.patch('kojihub._dml')

+     @mock.patch('kojihub.get_host')

+     @mock.patch('kojihub._singleValue')

+     def test_add_host_wrong_user_forced(self, _singleValue, get_host, _dml, get_user):

+         get_user.return_value = {

+             'id': 123,

+             'name': 'hostname',

+             'usertype': koji.USERTYPES['NORMAL']

+         }

+         get_host.return_value = {}

+ 

+         self.exports.addHost('hostname', ['i386', 'x86_64'], force=True)

+ 

+         _dml.assert_called_once()

+         get_user.assert_called_once_with(userInfo={'name': 'hostname'})

+         get_host.assert_called_once_with('hostname')

+         _singleValue.assert_called()

+         self.assertEqual(len(self.inserts), 2)

+         self.assertEqual(len(self.updates), 1)

+         update = self.updates[0]

+         self.assertEqual(update.values, {'userID': 123})

+         self.assertEqual(update.table, 'users')

+         self.assertEqual(update.clauses, ['id = %(userID)i'])

+         self.assertEqual(update.data, {'usertype': koji.USERTYPES['HOST']})

+ 

+     @mock.patch('kojihub.get_user')

+     @mock.patch('kojihub._dml')

+     @mock.patch('kojihub.get_host')

+     @mock.patch('kojihub._singleValue')

+     def test_add_host_superwrong_user_forced(self, _singleValue, get_host, _dml, get_user):

+         get_user.return_value = {

+             'id': 123,

+             'name': 'hostname',

+             'usertype': koji.USERTYPES['GROUP']

+         }

+         get_host.return_value = {}

+ 

+         with self.assertRaises(koji.GenericError):

+             self.exports.addHost('hostname', ['i386', 'x86_64'], force=True)

+ 

+         _dml.assert_not_called()

+         get_user.assert_called_once_with(userInfo={'name': 'hostname'})

+         get_host.assert_called_once_with('hostname')

+         _singleValue.assert_called()

+         self.assertEqual(len(self.inserts), 0)

+         self.assertEqual(len(self.updates), 0)

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

4 months ago

pretty please pagure-ci rebuild

4 months ago

rebased onto f1df5c298809b3444a15c6330d2109cfc0dd0d7c

4 months ago

pretty please pagure-ci rebuild

4 months ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

4 months ago

rebased onto b89e1895108da1bc616a3a815aa7a071f1cb1624

4 months ago

@mikem I've changed the logic. Let's assume, that NORMAL user was already created and let admin --force it to convert it to HOST. Does it make sense?

rebased onto 2cfa19a

4 months ago

the key in "userInfo" should be krb_principal
And is this userInfo too strict? not sure if we really care about the username

1) Fixed
2) I would be more cautious here. So, checking also the username seems ok to me. But maybe we can be more relaxed.

1 new commit added

  • fix get_user params
4 months ago

Metadata Update from @julian8628:
- Pull-request tagged with: testing-ready

4 months ago

1 new commit added

  • fix test
4 months ago

1 new commit added

  • query on krb only if it is requested
4 months ago

1 new commit added

  • check krb_principal before it is rewritten
4 months ago

Metadata Update from @jcupova:
- Pull-request tagged with: testing-done

4 months ago

1 new commit added

  • fix tests
4 months ago

Commit 934b3c9 fixes this pull-request

Pull-Request has been merged by tkopecek

4 months ago