#1171 Clean up optional parameters and don't automatically accept any parameter that is also a ModuleBuilds table column
Merged 3 months ago by mprahl. Opened 3 months ago by mprahl.

@@ -134,14 +134,14 @@ 

          db.create_all()

          load_local_builds(local_build_nsvs)

  

-         optional_params = {}

-         optional_params["local_build"] = True

-         optional_params["default_streams"] = {}

+         params = {}

+         params["local_build"] = True

+         params["default_streams"] = {}

          for ns in default_streams:

              name, stream = ns.split(":")

-             optional_params["default_streams"][name] = stream

+             params["default_streams"][name] = stream

          if srpms:

-             optional_params["srpms"] = srpms

+             params["srpms"] = srpms

  

          username = getpass.getuser()

          if not yaml_file or not yaml_file.endswith(".yaml"):

@@ -154,7 +154,7 @@ 

              handle.filename = filename

              try:

                  modules_list = submit_module_build_from_yaml(

-                     username, handle, str(stream), skiptests, optional_params)

+                     username, handle, params, stream=str(stream), skiptests=skiptests)

              except StreamAmbigous as e:

                  logging.error(str(e))

                  logging.error(

@@ -469,8 +469,7 @@ 

      return batch

  

  

- def submit_module_build_from_yaml(username, handle, stream=None, skiptests=False,

-                                   optional_params=None):

+ def submit_module_build_from_yaml(username, handle, params, stream=None, skiptests=False):

      yaml_file = to_text_type(handle.read())

      mmd = load_mmd(yaml_file)

      dt = datetime.utcfromtimestamp(int(time.time()))

@@ -483,14 +482,15 @@ 

          buildopts = mmd.get_rpm_buildopts()

          buildopts["macros"] = buildopts.get("macros", "") + "\n\n%__spec_check_pre exit 0\n"

          mmd.set_rpm_buildopts(buildopts)

-     return submit_module_build(username, None, mmd, optional_params)

+     return submit_module_build(username, mmd, params)

  

  

  _url_check_re = re.compile(r"^[^:/]+:.*$")

  

  

- def submit_module_build_from_scm(username, url, branch, allow_local_url=False,

-                                  optional_params=None):

+ def submit_module_build_from_scm(username, params, allow_local_url=False):

+     url = params['scmurl']

+     branch = params['branch']

      # Translate local paths into file:// URL

      if allow_local_url and not _url_check_re.match(url):

          log.info(

@@ -499,20 +499,20 @@ 

          url = "file://" + url

      mmd, scm = _fetch_mmd(url, branch, allow_local_url)

  

-     return submit_module_build(username, url, mmd, optional_params)

+     return submit_module_build(username, mmd, params)

  

  

- def _apply_dep_overrides(mmd, optional_params):

+ def _apply_dep_overrides(mmd, params):

      """

      Apply the dependency override parameters (if specified) on the input modulemd.

  

      :param Modulemd.Module mmd: the modulemd to apply the overrides on

-     :param dict optional_params: the optional API parameters passed in by the user

+     :param dict params: the API parameters passed in by the user

      :raises ValidationError: if one of the overrides doesn't apply

      """

      dep_overrides = {

-         'buildrequires': optional_params.get('buildrequire_overrides', {}),

-         'requires': optional_params.get('require_overrides', {})

+         'buildrequires': params.get('buildrequire_overrides', {}),

+         'requires': params.get('require_overrides', {})

      }

  

      unused_dep_overrides = {

@@ -541,28 +541,16 @@ 

                  'The {} overrides for the following modules aren\'t applicable: {}'

                  .format(dep_type[:-1], ', '.join(sorted(unused_dep_overrides[dep_type]))))

  

-     # Delete the parameters or else they'll get past to ModuleBuild.create

-     if 'buildrequire_overrides' in optional_params:

-         del optional_params['buildrequire_overrides']

-     if 'require_overrides' in optional_params:

-         del optional_params['require_overrides']

- 

      mmd.set_dependencies(deps)

  

  

- def submit_module_build(username, url, mmd, optional_params=None):

+ def submit_module_build(username, mmd, params):

      """

      Submits new module build.

  

      :param str username: Username of the build's owner.

-     :param str url: SCM URL of submitted build.

      :param Modulemd.Module mmd: Modulemd defining the build.

-     :param dict optional_params: Dict with optional params for a build:

-         - "local_build" (bool): The module is being built locally (the MBS is

-           not running in infra, but on local developer's machine).

-         - "default_streams" (dict): Dict with name:stream mapping defining the stream

-           to choose for given module name if multiple streams are available to choose from.

-         - Any optional ModuleBuild class field (str).

+     :param dict params: the API parameters passed in by the user

      :rtype: list with ModuleBuild

      :return: List with submitted module builds.

      """

@@ -574,17 +562,14 @@ 

  

      raise_if_stream_ambigous = False

      default_streams = {}

-     if optional_params:

-         # For local builds, we want the user to choose the exact stream using the default_streams

-         # in case there are multiple streams to choose from and raise an exception otherwise.

-         if "local_build" in optional_params:

-             raise_if_stream_ambigous = True

-             del optional_params["local_build"]

-         # Get the default_streams if set.

-         if "default_streams" in optional_params:

-             default_streams = optional_params["default_streams"]

-             del optional_params["default_streams"]

-         _apply_dep_overrides(mmd, optional_params)

+     # For local builds, we want the user to choose the exact stream using the default_streams

+     # in case there are multiple streams to choose from and raise an exception otherwise.

+     if "local_build" in params:

+         raise_if_stream_ambigous = True

+     # Get the default_streams if set.

+     if "default_streams" in params:

+         default_streams = params["default_streams"]

+     _apply_dep_overrides(mmd, params)

  

      validate_mmd(mmd)

      mmds = generate_expanded_mmds(db.session, mmd, raise_if_stream_ambigous, default_streams)

@@ -614,12 +599,13 @@ 

                           "is allowed.", nsvc)

                  modules.append(module)

                  continue

-             if optional_params:

-                 rebuild_strategy = optional_params.get('rebuild_strategy')

-                 if rebuild_strategy and module.rebuild_strategy != rebuild_strategy:

-                     raise ValidationError(

-                         'You cannot change the module\'s "rebuild_strategy" when '

-                         'resuming a module build')

+ 

+             rebuild_strategy = params.get('rebuild_strategy')

+             if rebuild_strategy and module.rebuild_strategy != rebuild_strategy:

+                 raise ValidationError(

+                     'You cannot change the module\'s "rebuild_strategy" when '

+                     'resuming a module build')

+ 

              log.debug('Resuming existing module build %r' % module)

              # Reset all component builds that didn't complete

              for component in module.component_builds:

@@ -645,9 +631,11 @@ 

                  stream=mmd.get_stream(),

                  version=version_str,

                  modulemd=to_text_type(mmd.dumps()),

-                 scmurl=url,

+                 scmurl=params.get('scmurl'),

                  username=username,

-                 **(optional_params or {})

+                 rebuild_strategy=params.get('rebuild_strategy'),

+                 scratch=params.get('scratch'),

+                 srpms=params.get('srpms')

              )

              (module.ref_build_context, module.build_context, module.runtime_context,

               module.context) = module.contexts_from_mmd(module.modulemd)

file modified
+16 -25

@@ -314,6 +314,19 @@ 

  

  

  class BaseHandler(object):

+     valid_params = set([

+         'branch',

+         'buildrequire_overrides',

+         'modulemd',

+         'module_name',

+         'owner',

+         'rebuild_strategy',

+         'require_overrides',

+         'scmurl',

+         'scratch',

+         'srpms'

+     ])

+ 

      def __init__(self, request, data=None):

          self.username, self.groups = module_build_service.auth.get_user(request)

          self.data = data or _dict_from_request(request)

@@ -335,11 +348,6 @@ 

          else:

              self.data['srpms'] = []

  

-     @property

-     def optional_params(self):

-         return {k: v for k, v in self.data.items() if k not in

-                 ["owner", "scmurl", "branch", "modulemd", "module_name"]}

- 

      def _validate_dep_overrides_format(self, key):

          """

          Validate any dependency overrides provided to the API.

@@ -361,18 +369,7 @@ 

                      raise ValidationError(invalid_override_msg)

  

      def validate_optional_params(self):

-         module_build_columns = set([col.key for col in models.ModuleBuild.__table__.columns])

-         other_params = set([

-             'branch',

-             'buildrequire_overrides',

-             'modulemd',

-             'module_name',

-             'rebuild_strategy',

-             'require_overrides',

-         ])

-         valid_params = other_params | module_build_columns

- 

-         forbidden_params = [k for k in self.data if k not in valid_params]

+         forbidden_params = [k for k in self.data if k not in self.valid_params]

          if forbidden_params:

              raise ValidationError('The request contains unspecified parameters: {}'

                                    .format(", ".join(forbidden_params)))

@@ -420,12 +417,7 @@ 

              self.validate_optional_params()

  

      def post(self):

-         url = self.data["scmurl"]

-         branch = self.data["branch"]

- 

-         return submit_module_build_from_scm(self.username, url, branch,

-                                             allow_local_url=False,

-                                             optional_params=self.optional_params)

+         return submit_module_build_from_scm(self.username, self.data, allow_local_url=False)

  

  

  class YAMLFileHandler(BaseHandler):

@@ -450,8 +442,7 @@ 

                  handle.filename = "unnamed"

          else:

              handle = request.files["yaml"]

-         return submit_module_build_from_yaml(self.username, handle,

-                                              optional_params=self.optional_params)

+         return submit_module_build_from_yaml(self.username, handle, self.data)

  

  

  def _dict_from_request(request):

@@ -281,11 +281,11 @@ 

  

          with open(modulemd_file_path, "rb") as fd:

              handle = FileStorage(fd)

-             module_build_service.utils.submit_module_build_from_yaml(username, handle,

-                                                                      stream=stream, skiptests=True)

+             module_build_service.utils.submit_module_build_from_yaml(

+                 username, handle, {}, stream=stream, skiptests=True)

              mock_submit_args = mock_submit.call_args[0]

              username_arg = mock_submit_args[0]

-             mmd_arg = mock_submit_args[2]

+             mmd_arg = mock_submit_args[1]

              assert mmd_arg.get_stream() == stream

              assert "\n\n%__spec_check_pre exit 0\n" in mmd_arg.get_rpm_buildopts()['macros']

              assert username_arg == username

@@ -789,7 +789,7 @@ 

  

          generate_expanded_mmds.return_value = [mmd1, mmd2]

  

-         builds = module_build_service.utils.submit_module_build("foo", "bar", mmd1)

+         builds = module_build_service.utils.submit_module_build("foo", mmd1, {})

          ret = {b.mmd().get_context(): b.state for b in builds}

          assert ret == {

              "c1": models.BUILD_STATES['ready'],

The MBS submission API endpoint should not accept every parameter that is also a column on the ModuleBuild table. There are two reasons for this.

The first is that a user should be notified if the supplied parameter is invalid, whereas it could get silently ignored. The second reason is that a nefarious user could pass in specially crafted API parameters causing MBS to do something unexpected or undesired.

Also, since the required parameters vary based on if the modulemd comes from SCM or a direct submission, the concept of optional parameters doesn't really apply. Because of that, this PR also removes the concept of optional parameters in the code.

It would probably be best to be explicit with the keyword arguments: stream=str(stream), skiptests=skiptests

2 new commits added

  • Remove the concept of required and optional parameters
  • Clean up the valid API parameters
3 months ago

It would probably be best to be explicit with the keyword arguments: stream=str(stream), skiptests=skiptests

Good idea. I fixed that.

rebased onto bc21144e3f63eaf909a59488d66268c1a4ef26a6

3 months ago

+1, but this will conflict with PR#1165. Can you discuss with @merlinm who should be rebasing? :)

rebased onto fd20e267452947d33dd987e97d171a731ef3301e

3 months ago

rebased onto 6d61a59

3 months ago

pretty please pagure-ci rebuild

3 months ago

I merged #1165, rebased this PR, and the tests pass locally. For some reason, CentOS CI Jenkins is just ignoring rebases and pretty please pagure-ci rebuild comments.

Pull-Request has been merged by mprahl

3 months ago