#3407 build policy
Opened 2 months ago by tkopecek. Modified 4 days ago
tkopecek/koji issue3323  into  master

file modified
+4 -6
@@ -1028,17 +1028,15 @@ 

              'task_id': self.id,

              'build_tag': build_tag,  # id

              'skip_tag': bool(self.opts.get('skip_tag')),

+             'scratch': opts.get('scratch'),

+             'from_scm': SCM.is_scm_url(src),

+             'repo_id': opts.get('repo_id'),

          }

          if target_info:

              policy_data['target'] = target_info['name']

          if not self.opts.get('skip_tag'):

              policy_data['tag'] = dest_tag  # id

-         if not SCM.is_scm_url(src) and not opts.get('scratch'):

-             # let hub policy decide

-             self.session.host.assertPolicy('build_from_srpm', policy_data)

-         if opts.get('repo_id') is not None:

-             # use of this option is governed by policy

-             self.session.host.assertPolicy('build_from_repo_id', policy_data)

+         self.session.host.assertPolicy('build', policy_data)

          if not repo_info:

              repo_info = self.getRepo(build_tag, builds=opts.get('wait_builds'),

                                       wait=opts.get('wait_repo'))  # (subtask)

@@ -51,9 +51,13 @@ 

  * cg_import: control which content generator imports are allowed

  * vm: control which windows build tasks are allowed

  * dist_repo: control which distRepo tasks are allowed

- * build_from_srpm: control whether builds from srpm are allowed

+ * build_rpm: control whether builds are allowed, this is superceding older ``build_from_srpm``

+              to handle all task types. ``build_from_srpm`` and ``build_from_repo_id`` are now

+              deprecated and will be removed in koji 1.33. For now, default ``build`` policy

+              contains calls to both of these.

+ * build_from_srpm [deprecated]: control whether builds from srpm are allowed

  * build_from_scm: control whether builds from the SCM are allowed and the behavior of the SCM

- * build_from_repo_id: control whether builds from user-specified repos ids are allowed

+ * build_from_repo_id [deprecated]: control whether builds from user-specified repos ids are allowed

  

  Note that not all policies are access control policies.

  The ``channel`` and ``volume`` policies are used to control which channels tasks go to

file modified
+10
@@ -555,6 +555,16 @@ 

  

  

  _default_policies = {

+     'build_rpm': '''

+             scratch :: allow

+             bool from_scm !! {

+                 policy build_from_srpm :: allow

+             }

+             bool repo_id :: {

+                 policy build_from_repo_id :: allow

+             }

+             all :: deny

+             ''',

      'build_from_srpm': '''

              has_perm admin :: allow

              all :: deny Only admin can do this via default policy

New 'build' policy replacing 'build_from_srpm' and 'build_from_repo_id'.

Related: https://pagure.io/koji/issue/3323

rebased onto 379284d595e2b627fe711d184df4c21956674ac5

a month ago

The default policy is missing actions for the sub policy lines.

I started to say that the default policy needs further adjustment too, but after looking at it, I'm not sure how easily we can accomplish backwards compatibility this way.

  • policies used with {assert,check}_policy expect results of the form "(allow|deny) <reason>" and warn if the first word is anything else
  • policies used with PolicyTest only check if result.lower() in ('yes', 'true', 'allow') and do not handle reason
  • I don't see a simple, clean way to propagate the reason out of the subpolicy

Kinda wonder if the policy should be named more specifically since it is only called for rpm builds (which, granted, have 'build' as the task method name)

Before I realized you were referencing the old policies in the new default, I was thinking that we should maintain the old codepath (deprecated) for a bit to maintain compat. That is still an option, though I don't think there's a good way in kojid to check which policies are defined. Still, something like this is another way we could go.

rebased onto 6ed878a

20 days ago

pretty please pagure-ci rebuild

13 days ago

The changes I see since last review are to rename the new policy to build_rpm and to add actions to subpolicy check lines in said policy.

This still doesn't address the backwards compatibility issue that stems from the fact that invoking a policy as a subpolicy (via the policy test) has different behavior than invoking a policy via {assert,check}_policy. In particular, any use of reason args in the actions of the subpolicy will not work correctly. If a policy includes an allow with a reason, it will be treated as false result.

Hmm, finally got it :-) It would mean adding new policy aka "check_policy". Is it worth it or should we just leave those old policies for now?

I think the shortest path to a working, backwards compatible implementation is to preserve the old policy calls, but recommend that folks use the build policy instead of the older ones.

We might eventually want deprecate the old policies and tell people they have to migrate