From c33e799fdd75f5359d1c05dd894a82ffe5322f19 Mon Sep 17 00:00:00 2001 From: Yashvardhan Nanavati Date: Thu, 8 Oct 2020 16:23:00 -0400 Subject: [PATCH] Add IIB_BINARY_IMAGE_CONFIG to IIB and make binary_image param optional This commit introduces a new config variable called IIB_BINARY_IMAGE_CONFIG which when specified, doesn't require the user to specify binary_image param while submitting requests to IIB. Refers to CLOUDDST-2761 --- README.md | 4 + iib/web/api_v1.py | 10 +- iib/web/app.py | 25 +++++ iib/web/config.py | 1 + ...60f89c046096_make_binary_image_optional.py | 38 +++++++ iib/web/models.py | 43 ++++---- iib/web/static/api_v1.yaml | 3 - iib/workers/tasks/build.py | 75 +++++++++++--- iib/workers/tasks/build_merge_index_image.py | 14 +-- tests/test_web/test_api_v1.py | 44 ++++----- tests/test_web/test_app.py | 35 +++++++ tests/test_workers/test_tasks/test_build.py | 98 ++++++++++++++++--- .../test_build_merge_index_image.py | 22 +++-- 13 files changed, 323 insertions(+), 89 deletions(-) create mode 100644 iib/web/migrations/versions/60f89c046096_make_binary_image_optional.py diff --git a/README.md b/README.md index 1e741385..ec7730d4 100644 --- a/README.md +++ b/README.md @@ -140,6 +140,10 @@ The custom configuration options for the REST API are listed below: * `IIB_ADDITIONAL_LOGGERS` - a list of Python loggers that should have the same log level that is set for `IIB_LOG_LEVEL`. This defaults to `[]`. +* `IIB_BINARY_IMAGE_CONFIG` - the mapping, `dict(: dict(:))`, of distribution scope + to another dictionary mapping ocp_version label to a binary image pull specification. + This is useful in setting up customized binary image for different index image images thus + reducing complexity for the end user. This defaults to `{}`. * `IIB_FORCE_OVERWRITE_FROM_INDEX` - a boolean that determines if privileged users should be forced to have `overwrite_from_index` set to `True`. This defaults to `False`. * `IIB_GREENWAVE_CONFIG` - the mapping, `dict(: dict(:))`, of celery task queues to diff --git a/iib/web/api_v1.py b/iib/web/api_v1.py index 11fa0908..579da242 100644 --- a/iib/web/api_v1.py +++ b/iib/web/api_v1.py @@ -50,13 +50,14 @@ def _get_rm_args(payload, request, overwrite_from_index): """ return [ payload['operators'], - payload['binary_image'], request.id, payload['from_index'], + payload.get('binary_image'), payload.get('add_arches'), overwrite_from_index, payload.get('overwrite_from_index_token'), request.distribution_scope, + flask.current_app.config['IIB_BINARY_IMAGE_CONFIG'], ] @@ -71,8 +72,8 @@ def _get_add_args(payload, request, overwrite_from_index, celery_queue): """ return [ payload.get('bundles', []), - payload['binary_image'], request.id, + payload.get('binary_image'), payload.get('from_index'), payload.get('add_arches'), payload.get('cnr_token'), @@ -82,6 +83,7 @@ def _get_add_args(payload, request, overwrite_from_index, celery_queue): payload.get('overwrite_from_index_token'), request.distribution_scope, flask.current_app.config['IIB_GREENWAVE_CONFIG'].get(celery_queue), + flask.current_app.config['IIB_BINARY_IMAGE_CONFIG'], ] @@ -371,6 +373,7 @@ def patch_request(request_id): ) image_keys = ( + 'binary_image', 'binary_image_resolved', 'bundle_image', 'from_bundle_image_resolved', @@ -654,14 +657,15 @@ def merge_index_image(): overwrite_target_index = payload.get('overwrite_target_index', False) celery_queue = _get_user_queue(serial=overwrite_target_index) args = [ - payload['binary_image'], payload['source_from_index'], payload.get('deprecation_list', []), request.id, + payload.get('binary_image'), payload.get('target_index'), overwrite_target_index, payload.get('overwrite_target_index_token'), payload.get('distribution_scope'), + flask.current_app.config['IIB_BINARY_IMAGE_CONFIG'], ] safe_args = _get_safe_args(args, payload) diff --git a/iib/web/app.py b/iib/web/app.py index ab983ae7..8a89c820 100644 --- a/iib/web/app.py +++ b/iib/web/app.py @@ -81,6 +81,31 @@ def validate_api_config(config): f'{queue_name} in "IIB_GREENWAVE_CONFIG"' ) + if config['IIB_BINARY_IMAGE_CONFIG']: + if not isinstance(config['IIB_BINARY_IMAGE_CONFIG'], dict): + raise ConfigError( + 'IIB_BINARY_IMAGE_CONFIG must be a dict mapping distribution_scope to ' + 'another dict mapping ocp_version to binary_image' + ) + for distribution_scope, value_dict in config['IIB_BINARY_IMAGE_CONFIG'].items(): + if not isinstance(distribution_scope, str) or distribution_scope not in ( + 'dev', + 'stage', + 'prod', + ): + raise ConfigError( + 'distribution_scope values must be one of the following' + ' "prod", "stage" or "dev" strings.' + ) + if not isinstance(value_dict, dict): + raise ConfigError( + 'Value for distribution_scope keys must be a dict mapping' + ' ocp_version to binary_image' + ) + for ocp_version, binary_image_value in value_dict.items(): + if not isinstance(ocp_version, str) or not isinstance(binary_image_value, str): + raise ConfigError('All ocp_version and binary_image values must be strings.') + # See app factory pattern: # http://flask.pocoo.org/docs/0.12/patterns/appfactories/ diff --git a/iib/web/config.py b/iib/web/config.py index 13d0138f..b0f12b0c 100644 --- a/iib/web/config.py +++ b/iib/web/config.py @@ -10,6 +10,7 @@ class Config(object): # Additional loggers to set to the level defined in IIB_LOG_LEVEL IIB_ADDITIONAL_LOGGERS = [] + IIB_BINARY_IMAGE_CONFIG = {} IIB_FORCE_OVERWRITE_FROM_INDEX = False IIB_GREENWAVE_CONFIG = {} IIB_LOG_FORMAT = '%(asctime)s %(name)s %(levelname)s %(module)s.%(funcName)s %(message)s' diff --git a/iib/web/migrations/versions/60f89c046096_make_binary_image_optional.py b/iib/web/migrations/versions/60f89c046096_make_binary_image_optional.py new file mode 100644 index 00000000..a4ac6ed1 --- /dev/null +++ b/iib/web/migrations/versions/60f89c046096_make_binary_image_optional.py @@ -0,0 +1,38 @@ +"""Make binary_image optional. + +Revision ID: 60f89c046096 +Revises: 983a81fe5e98 +Create Date: 2020-10-12 15:49:24.523019 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '60f89c046096' +down_revision = '983a81fe5e98' +branch_labels = None +depends_on = None + + +def upgrade(): + with op.batch_alter_table('request_add', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=True) + + with op.batch_alter_table('request_merge_index_image', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=True) + + with op.batch_alter_table('request_rm', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=True) + + +def downgrade(): + with op.batch_alter_table('request_rm', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=False) + + with op.batch_alter_table('request_merge_index_image', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=False) + + with op.batch_alter_table('request_add', schema=None) as batch_op: + batch_op.alter_column('binary_image_id', existing_type=sa.INTEGER(), nullable=False) diff --git a/iib/web/models.py b/iib/web/models.py index ce437574..6f8d2752 100644 --- a/iib/web/models.py +++ b/iib/web/models.py @@ -629,7 +629,7 @@ class RequestIndexImageMixin: @declared_attr def binary_image_id(cls): """Return the ID of the image that the opm binary comes from.""" - return db.Column(db.Integer, db.ForeignKey('image.id'), nullable=False) + return db.Column(db.Integer, db.ForeignKey('image.id')) @declared_attr def binary_image_resolved_id(cls): @@ -696,9 +696,10 @@ def _from_json( be created automatically. """ # Validate all required parameters are present - required_params = {'binary_image'} | set(additional_required_params or []) + required_params = set(additional_required_params or []) optional_params = { 'add_arches', + 'binary_image', 'overwrite_from_index', 'overwrite_from_index_token', 'distribution_scope', @@ -752,11 +753,14 @@ def _from_json( Architecture.validate_architecture_json(add_arches) # Validate binary_image is correctly provided - binary_image = request_kwargs.pop('binary_image') - if not isinstance(binary_image, str): + binary_image = request_kwargs.pop('binary_image', None) + if binary_image is not None and not isinstance(binary_image, str): raise ValidationError('"binary_image" must be a string') + elif not binary_image and not current_app.config['IIB_BINARY_IMAGE_CONFIG']: + raise ValidationError('The "binary_image" value must be a non-empty string') - request_kwargs['binary_image'] = Image.get_or_create(pull_specification=binary_image) + if binary_image: + request_kwargs['binary_image'] = Image.get_or_create(pull_specification=binary_image) if 'from_index' in request_kwargs: if not isinstance(request_kwargs['from_index'], str): @@ -790,7 +794,7 @@ def get_common_index_image_json(self): :rtype: dict """ return { - 'binary_image': self.binary_image.pull_specification, + 'binary_image': getattr(self.binary_image, 'pull_specification', None), 'binary_image_resolved': getattr( self.binary_image_resolved, 'pull_specification', None ), @@ -812,6 +816,7 @@ def get_index_image_mutable_keys(self): :rtype: set """ return { + 'binary_image', 'binary_image_resolved', 'from_bundle_image_resolved', 'from_index_resolved', @@ -850,16 +855,10 @@ def from_json(cls, kwargs, batch=None): '"bundles" should be either an empty array or an array of non-empty strings' ) - # Check if no bundles `from_index and `binary_image` are specified - # if no bundles and and no from index then a empty index will be created - # if no binary image and just a from_index then we are not updating anything and it would - # be a no-op - if not request_kwargs.get('bundles') and ( - not request_kwargs.get('from_index') or not request_kwargs.get('binary_image') - ): - raise ValidationError( - '"from_index" and "binary_image" must be specified if no bundles are specified' - ) + # Check if no bundles and `from_index is specified + # if no bundles and no from index then an empty index will be created which is a no-op + if not (request_kwargs.get('bundles') or request_kwargs.get('from_index')): + raise ValidationError('"from_index" must be specified if no bundles are specified') for param in ('cnr_token', 'organization'): if param not in request_kwargs: @@ -1101,7 +1100,7 @@ class RequestMergeIndexImage(Request): __tablename__ = 'request_merge_index_image' id = db.Column(db.Integer, db.ForeignKey('request.id'), autoincrement=False, primary_key=True) - binary_image_id = db.Column(db.Integer, db.ForeignKey('image.id'), nullable=False) + binary_image_id = db.Column(db.Integer, db.ForeignKey('image.id')) binary_image_resolved_id = db.Column(db.Integer, db.ForeignKey('image.id')) binary_image = db.relationship('Image', foreign_keys=[binary_image_id], uselist=False) binary_image_resolved = db.relationship( @@ -1190,10 +1189,13 @@ def from_json(cls, kwargs, batch=None): # Validate binary_image is correctly provided binary_image = request_kwargs.pop('binary_image', None) - if not isinstance(binary_image, str): + if binary_image is not None and not isinstance(binary_image, str): raise ValidationError('The "binary_image" value must be a string') + elif not binary_image and not current_app.config['IIB_BINARY_IMAGE_CONFIG']: + raise ValidationError('The "binary_image" value must be a non-empty string') - request_kwargs['binary_image'] = Image.get_or_create(pull_specification=binary_image) + if binary_image: + request_kwargs['binary_image'] = Image.get_or_create(pull_specification=binary_image) distribution_scope = request_kwargs.pop('distribution_scope', None) if distribution_scope: @@ -1226,7 +1228,7 @@ def to_json(self, verbose=True): :rtype: dict """ rv = super().to_json(verbose=verbose) - rv['binary_image'] = self.binary_image.pull_specification + rv['binary_image'] = getattr(self.binary_image, 'pull_specification', None) rv['binary_image_resolved'] = getattr( self.binary_image_resolved, 'pull_specification', None ) @@ -1254,6 +1256,7 @@ def get_mutable_keys(self): rv = super().get_mutable_keys() rv.update( { + 'binary_image', 'binary_image_resolved', 'index_image', 'source_from_index_resolved', diff --git a/iib/web/static/api_v1.yaml b/iib/web/static/api_v1.yaml index fdabc1d0..37b8c778 100644 --- a/iib/web/static/api_v1.yaml +++ b/iib/web/static/api_v1.yaml @@ -700,7 +700,6 @@ components: type: string example: 'prod' required: - - binary_image - bundles RequestRemove: type: object @@ -744,7 +743,6 @@ components: type: string example: 'prod' required: - - binary_image - from_index - operators RequestRegenerateBundle: @@ -814,7 +812,6 @@ components: type: string example: 'prod' required: - - binary_image - source_from_index RequestMergeIndexImageOutput: allOf: diff --git a/iib/workers/tasks/build.py b/iib/workers/tasks/build.py index 0e115286..fb8a0b08 100644 --- a/iib/workers/tasks/build.py +++ b/iib/workers/tasks/build.py @@ -704,9 +704,32 @@ def get_index_image_info(overwrite_from_index_token, from_index=None, default_oc return result +def get_binary_image_from_config(ocp_version, distribution_scope, binary_image_config={}): + """ + Determine the binary image to be used to build the index image. + + :param str ocp_version: the ocp_version label value of the index image. + :param str distribution_scope: the distribution_scope label value of the index image. + :param dict binary_image_config: the dict of config required to identify the appropriate + ``binary_image`` to use. + :return: pull specification of the binary_image to be used for this build. + :rtype: str + :raises IIBError: when the config value for the ocp_version and distribution_scope is missing. + """ + binary_image = binary_image_config.get(distribution_scope, {}).get(ocp_version, None) + if not binary_image: + raise IIBError( + 'IIB does not have a configured binary_image for' + f' distribution_scope : {distribution_scope} and ocp_version: {ocp_version}.' + ' Please specify a binary_image value in the request.' + ) + + return binary_image + + def _prepare_request_for_build( - binary_image, request_id, + binary_image=None, from_index=None, overwrite_from_index_token=None, add_arches=None, @@ -714,6 +737,7 @@ def _prepare_request_for_build( distribution_scope=None, source_from_index=None, target_index=None, + binary_image_config=None, ): """ Prepare the request for the index image build. @@ -742,6 +766,8 @@ def _prepare_request_for_build( that will be used as a base of the merged index image. :param str target_index: the pull specification of the container image containing the index whose new data will be added to the merged index image. + :param dict binary_image_config: the dict of config required to identify the appropriate + ``binary_image`` to use. :return: a dictionary with the keys: arches, binary_image_resolved, from_index_resolved, and ocp_version. :rtype: dict @@ -758,9 +784,6 @@ def _prepare_request_for_build( else: arches = set() - binary_image_resolved = _get_resolved_image(binary_image) - binary_image_arches = _get_image_arches(binary_image_resolved) - from_index_info = get_index_image_info( overwrite_from_index_token, from_index=from_index, default_ocp_version='v4.5' ) @@ -786,6 +809,18 @@ def _prepare_request_for_build( from_index_info['resolved_distribution_scope'], distribution_scope ) + if not binary_image: + binary_image_ocp_version = from_index_info['ocp_version'] + if source_from_index: + binary_image_ocp_version = target_index_info['ocp_version'] + + binary_image = get_binary_image_from_config( + binary_image_ocp_version, distribution_scope, binary_image_config + ) + + binary_image_resolved = _get_resolved_image(binary_image) + binary_image_arches = _get_image_arches(binary_image_resolved) + if not arches.issubset(binary_image_arches): raise IIBError( 'The binary image is not available for the following arches: {}'.format( @@ -801,6 +836,7 @@ def _prepare_request_for_build( return { 'arches': arches, + 'binary_image': binary_image, 'binary_image_resolved': binary_image_resolved, 'bundle_mapping': bundle_mapping, 'from_index_resolved': from_index_info['resolved_from_index'], @@ -828,6 +864,7 @@ def _update_index_image_build_state(request_id, prebuild_info): """ arches_str = ', '.join(sorted(prebuild_info['arches'])) payload = { + 'binary_image': prebuild_info['binary_image'], 'binary_image_resolved': prebuild_info['binary_image_resolved'], 'state': 'in_progress', 'state_reason': f'Building the index image for the following arches: {arches_str}', @@ -968,8 +1005,8 @@ def get_image_label(pull_spec, label): @request_logger def handle_add_request( bundles, - binary_image, request_id, + binary_image=None, from_index=None, add_arches=None, cnr_token=None, @@ -979,15 +1016,16 @@ def handle_add_request( overwrite_from_index_token=None, distribution_scope=None, greenwave_config=None, + binary_image_config=None, ): """ Coordinate the the work needed to build the index image with the input bundles. :param list bundles: a list of strings representing the pull specifications of the bundles to add to the index image being built. + :param int request_id: the ID of the IIB build request :param str binary_image: the pull specification of the container image where the opm binary gets copied from. - :param int request_id: the ID of the IIB build request :param str from_index: the pull specification of the container image containing the index that the index image build will be based from. :param list add_arches: the list of arches to build in addition to the arches ``from_index`` is @@ -1006,6 +1044,8 @@ def handle_add_request( :param str distribution_scope: the scope for distribution of the index image, defaults to ``None``. :param dict greenwave_config: the dict of config required to query Greenwave to gate bundles. + :param dict binary_image_config: the dict of config required to identify the appropriate + ``binary_image`` to use. :raises IIBError: if the index image build fails or legacy support is required and one of ``cnr_token`` or ``organization`` is not specified. """ @@ -1021,13 +1061,14 @@ def handle_add_request( gate_bundles(resolved_bundles, greenwave_config) prebuild_info = _prepare_request_for_build( - binary_image, request_id, + binary_image, from_index, overwrite_from_index_token, add_arches, bundles, distribution_scope, + binary_image_config=binary_image_config, ) log.info('Checking if interacting with the legacy app registry is required') @@ -1119,24 +1160,25 @@ def handle_add_request( @request_logger def handle_rm_request( operators, - binary_image, request_id, from_index, + binary_image=None, add_arches=None, overwrite_from_index=False, overwrite_from_index_token=None, distribution_scope=None, + binary_image_config=None, ): """ Coordinate the work needed to remove the input operators and rebuild the index image. :param list operators: a list of strings representing the name of the operators to remove from the index image. - :param str binary_image: the pull specification of the container image where the opm binary - gets copied from. :param int request_id: the ID of the IIB build request :param str from_index: the pull specification of the container image containing the index that the index image build will be based from. + :param str binary_image: the pull specification of the container image where the opm binary + gets copied from. :param list add_arches: the list of arches to build in addition to the arches ``from_index`` is currently built for. :param bool overwrite_from_index: if True, overwrite the input ``from_index`` with the built @@ -1146,21 +1188,30 @@ def handle_rm_request( ``overwrite_from_index``. The format of the token must be in the format "user:password". :param str distribution_scope: the scope for distribution of the index image, defaults to ``None``. + :param dict binary_image_config: the dict of config required to identify the appropriate + ``binary_image`` to use. :raises IIBError: if the index image build fails. """ _cleanup() prebuild_info = _prepare_request_for_build( - binary_image, request_id, + binary_image, from_index, overwrite_from_index_token, add_arches, distribution_scope=distribution_scope, + binary_image_config=binary_image_config, ) _update_index_image_build_state(request_id, prebuild_info) with tempfile.TemporaryDirectory(prefix='iib-') as temp_dir: - _opm_index_rm(temp_dir, operators, binary_image, from_index, overwrite_from_index_token) + _opm_index_rm( + temp_dir, + operators, + prebuild_info['binary_image'], + from_index, + overwrite_from_index_token, + ) _add_label_to_index( 'com.redhat.index.delivery.version', diff --git a/iib/workers/tasks/build_merge_index_image.py b/iib/workers/tasks/build_merge_index_image.py index 90a08c2d..21dc6eac 100644 --- a/iib/workers/tasks/build_merge_index_image.py +++ b/iib/workers/tasks/build_merge_index_image.py @@ -175,26 +175,27 @@ def _deprecate_bundles( @app.task @request_logger def handle_merge_request( - binary_image, source_from_index, deprecation_list, request_id, + binary_image=None, target_index=None, overwrite_target_index=False, overwrite_target_index_token=None, distribution_scope=None, + binary_image_config=None, ): """ Coordinate the work needed to merge old (N) index image with new (N+1) index image. - :param str binary_image: the pull specification of the container image where the opm binary - gets copied from. :param str source_from_index: pull specification to be used as the base for building the new index image. :param str target_index: pull specification of content stage index image for the corresponding target index image. :param list deprecation_list: list of deprecated bundles for the target index image. :param int request_id: the ID of the IIB build request. + :param str binary_image: the pull specification of the container image where the opm binary + gets copied from. :param bool overwrite_target_index: if True, overwrite the input ``target_index`` with the built index image. :param str overwrite_target_index_token: the token used for overwriting the input @@ -206,12 +207,13 @@ def handle_merge_request( """ _cleanup() prebuild_info = _prepare_request_for_build( - binary_image, request_id, + binary_image, overwrite_from_index_token=overwrite_target_index_token, source_from_index=source_from_index, target_index=target_index, distribution_scope=distribution_scope, + binary_image_config=binary_image_config, ) _update_index_image_build_state(request_id, prebuild_info) @@ -232,7 +234,7 @@ def handle_merge_request( source_index_bundles, target_index_bundles, temp_dir, - binary_image, + prebuild_info['binary_image'], source_from_index, request_id, arch, @@ -255,7 +257,7 @@ def handle_merge_request( _deprecate_bundles( deprecate_bundles, temp_dir, - binary_image, + prebuild_info['binary_image'], intermediate_image_name, overwrite_target_index_token, ) diff --git a/tests/test_web/test_api_v1.py b/tests/test_web/test_api_v1.py index 83c7241e..3521cae7 100644 --- a/tests/test_web/test_api_v1.py +++ b/tests/test_web/test_api_v1.py @@ -228,7 +228,7 @@ def test_get_build_logs_not_configured(client, db, minimal_request_add): 'binary_image': '', 'add_arches': ['s390x'], }, - '"binary_image" must be set', + 'The "binary_image" value must be a non-empty string', ), ( { @@ -365,20 +365,13 @@ def test_rm_operators_overwrite_not_allowed(mock_smfsc, client, db): ( ( {'bundles': ['some:thing'], 'from_index': 'pull:spec', 'add_arches': ['s390x']}, - 'Missing required parameter(s): binary_image', - ), - ( - {'from_index': 'pull:spec', 'add_arches': ['s390x']}, - '"from_index" and "binary_image" must be specified if no bundles are specified', + 'The "binary_image" value must be a non-empty string', ), ( {'add_arches': ['s390x'], 'binary_image': 'binary:image'}, - '"from_index" and "binary_image" must be specified if no bundles are specified', - ), - ( - {'add_arches': ['s390x']}, - '"from_index" and "binary_image" must be specified if no bundles are specified', + '"from_index" must be specified if no bundles are specified', ), + ({'add_arches': ['s390x']}, '"from_index" must be specified if no bundles are specified',), ( { 'bundles': ['some:thing'], @@ -406,7 +399,7 @@ def test_add_bundle_missing_required_param(mock_smfsc, data, error_msg, db, auth ( ( {'operators': ['some:thing'], 'from_index': 'pull:spec', 'add_arches': ['s390x']}, - 'Missing required parameter(s): binary_image', + 'The "binary_image" value must be a non-empty string', ), ( {'from_index': 'pull:spec', 'binary_image': 'binary:image', 'add_arches': ['s390x']}, @@ -604,7 +597,7 @@ def test_add_bundle_forced_overwrite( assert rv.status_code == 201 mock_har.apply_async.assert_called_once() # Fourth to last element in args is the overwrite_from_index parameter - assert mock_har.apply_async.call_args[1]['args'][-4] == force_overwrite + assert mock_har.apply_async.call_args[1]['args'][-5] == force_overwrite mock_smfsc.assert_called_once_with(mock.ANY, new_batch_msg=True) @@ -642,9 +635,9 @@ def test_add_bundle_overwrite_token_redacted(mock_smfsc, mock_har, app, auth_env assert rv.status_code == 201 mock_har.apply_async.assert_called_once() # Fourth to last element in args is the overwrite_from_index parameter - assert mock_har.apply_async.call_args[1]['args'][-4] is True + assert mock_har.apply_async.call_args[1]['args'][-5] is True # Third to last element in args is the overwrite_from_index_token parameter - assert mock_har.apply_async.call_args[1]['args'][-3] == token + assert mock_har.apply_async.call_args[1]['args'][-4] == token assert 'overwrite_from_index_token' not in rv_json assert token not in json.dumps(rv_json) assert token not in mock_har.apply_async.call_args[1]['argsrepr'] @@ -1122,7 +1115,7 @@ def test_remove_operator_forced_overwrite( assert rv.status_code == 201 mock_hrr.apply_async.assert_called_once() # Third to last element in args is the overwrite_from_index parameter - assert mock_hrr.apply_async.call_args[1]['args'][-3] == force_overwrite + assert mock_hrr.apply_async.call_args[1]['args'][-4] == force_overwrite mock_smfsc.assert_called_once_with(mock.ANY, new_batch_msg=True) @@ -1143,8 +1136,8 @@ def test_remove_operator_overwrite_token_redacted(mock_smfsc, mock_hrr, app, aut assert rv.status_code == 201 mock_hrr.apply_async.assert_called_once() # Third to last element in args is the overwrite_from_index parameter - assert mock_hrr.apply_async.call_args[1]['args'][-3] is True - assert mock_hrr.apply_async.call_args[1]['args'][-2] == token + assert mock_hrr.apply_async.call_args[1]['args'][-4] is True + assert mock_hrr.apply_async.call_args[1]['args'][-3] == token assert 'overwrite_from_index_token' not in rv_json assert token not in json.dumps(rv_json) assert token not in mock_hrr.apply_async.call_args[1]['argsrepr'] @@ -1449,8 +1442,8 @@ def test_add_rm_batch_success(mock_smfnbor, mock_hrr, mock_har, app, auth_env, c mock.call( args=[ ['registry-proxy/rh-osbs/lgallett-bundle:v1.0-9'], - 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', 1, + 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', 'registry-proxy/rh-osbs-stage/iib:v4.5', ['amd64'], 'no_tom_brady_anymore', @@ -1460,12 +1453,13 @@ def test_add_rm_batch_success(mock_smfnbor, mock_hrr, mock_har, app, auth_env, c 'some_token', None, None, + {}, ], argsrepr=( "[['registry-proxy/rh-osbs/lgallett-bundle:v1.0-9'], " - "'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', 1, " + "1, 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', " "'registry-proxy/rh-osbs-stage/iib:v4.5', ['amd64'], '*****', " - "'hello-operator', None, True, '*****', None, None]" + "'hello-operator', None, True, '*****', None, None, {}]" ), link_error=mock.ANY, queue=None, @@ -1477,17 +1471,19 @@ def test_add_rm_batch_success(mock_smfnbor, mock_hrr, mock_har, app, auth_env, c mock.call( args=[ ['kiali-ossm'], - 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', 2, 'registry:8443/iib-build:11', + 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5', None, None, None, None, + {}, ], argsrepr=( - "[['kiali-ossm'], 'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5'" - ", 2, 'registry:8443/iib-build:11', None, None, None, None]" + "[['kiali-ossm'], 2, 'registry:8443/iib-build:11', " + "'registry-proxy/rh-osbs/openshift-ose-operator-registry:v4.5'" + ", None, None, None, None, {}]" ), link_error=mock.ANY, queue=None, diff --git a/tests/test_web/test_app.py b/tests/test_web/test_app.py index 9362f420..3906389f 100644 --- a/tests/test_web/test_app.py +++ b/tests/test_web/test_app.py @@ -92,3 +92,38 @@ def test_load_config_prod(mock_isfile, mock_getenv): def test_validate_api_config_failure_greenwave_params(config, error_msg): with pytest.raises(ConfigError, match=error_msg): validate_api_config(config) + + +@pytest.mark.parametrize( + 'config, error_msg', + ( + ( + {'IIB_BINARY_IMAGE_CONFIG': {'tom-brady': {}}, 'IIB_GREENWAVE_CONFIG': {}}, + ( + 'distribution_scope values must be one of the following' + ' "prod", "stage" or "dev" strings.' + ), + ), + ( + {'IIB_BINARY_IMAGE_CONFIG': {'prod': []}, 'IIB_GREENWAVE_CONFIG': {}}, + ( + 'Value for distribution_scope keys must be a dict mapping' + ' ocp_version to binary_image' + ), + ), + ( + {'IIB_BINARY_IMAGE_CONFIG': {'prod': {'v4.5': 2}}, 'IIB_GREENWAVE_CONFIG': {}}, + 'All ocp_version and binary_image values must be strings.', + ), + ( + {'IIB_BINARY_IMAGE_CONFIG': ['something'], 'IIB_GREENWAVE_CONFIG': {}}, + ( + 'IIB_BINARY_IMAGE_CONFIG must be a dict mapping distribution_scope to ' + 'another dict mapping ocp_version to binary_image' + ), + ), + ), +) +def test_validate_api_config_failure_binary_image_params(config, error_msg): + with pytest.raises(ConfigError, match=error_msg): + validate_api_config(config) diff --git a/tests/test_workers/test_tasks/test_build.py b/tests/test_workers/test_tasks/test_build.py index 2c1802c0..693d5d11 100644 --- a/tests/test_workers/test_tasks/test_build.py +++ b/tests/test_workers/test_tasks/test_build.py @@ -460,47 +460,63 @@ def test_overwrite_from_index( @pytest.mark.parametrize( - 'add_arches, from_index, from_index_arches, bundles,' - 'expected_bundle_mapping, distribution_scope, resolved_distribution_scope', + 'add_arches, from_index, from_index_arches, bundles, binary_image,' + 'expected_bundle_mapping, distribution_scope, resolved_distribution_scope, binary_image_config', ( - ([], 'some-index:latest', {'amd64'}, None, {}, None, 'prod'), - (['amd64', 's390x'], None, set(), None, {}, None, 'prod'), - (['amd64'], 'some-index:latest', {'amd64'}, None, {}, None, 'prod'), + ([], 'some-index:latest', {'amd64'}, None, 'binary-image:latest', {}, None, 'prod', {}), + (['amd64', 's390x'], None, set(), None, 'binary-image:latest', {}, None, 'prod', {}), + ( + ['amd64'], + 'some-index:latest', + {'amd64'}, + None, + 'binary-image:latest', + {}, + None, + 'prod', + {}, + ), ( ['amd64'], None, set(), ['quay.io/some-bundle:v1', 'quay.io/some-bundle2:v1'], + None, { 'some-bundle': ['quay.io/some-bundle:v1'], 'some-bundle2': ['quay.io/some-bundle2:v1'], }, None, 'prod', + {'prod': {'v4.5': 'binary-image:prod'}}, ), ( ['amd64'], 'some-index:latest', set(), ['quay.io/some-bundle:v1', 'quay.io/some-bundle2:v1'], + 'binary-image:latest', { 'some-bundle': ['quay.io/some-bundle:v1'], 'some-bundle2': ['quay.io/some-bundle2:v1'], }, None, 'prod', + {}, ), ( ['amd64'], 'some-index:latest', set(), ['quay.io/some-bundle:v1', 'quay.io/some-bundle2:v1'], + 'binary-image:latest', { 'some-bundle': ['quay.io/some-bundle:v1'], 'some-bundle2': ['quay.io/some-bundle2:v1'], }, None, 'prod', + {}, ), ), ) @@ -519,9 +535,11 @@ def test_prepare_request_for_build( from_index, from_index_arches, bundles, + binary_image, expected_bundle_mapping, distribution_scope, resolved_distribution_scope, + binary_image_config, ): binary_image_resolved = 'binary-image@sha256:abcdef' from_index_resolved = None @@ -534,8 +552,8 @@ def test_prepare_request_for_build( if from_index: from_index_name = from_index.split(':', 1)[0] from_index_resolved = f'{from_index_name}@sha256:bcdefg' - mock_gri.side_effect = [binary_image_resolved, from_index_resolved] - mock_gia.side_effect = [expected_arches, from_index_arches] + mock_gri.side_effect = [from_index_resolved, binary_image_resolved] + mock_gia.side_effect = [from_index_arches, expected_arches] expected_payload_keys.add('from_index_resolved') gil_side_effect = ['v4.6', resolved_distribution_scope] ocp_version = 'v4.6' @@ -550,11 +568,22 @@ def test_prepare_request_for_build( mock_gil.side_effect = gil_side_effect rv = build._prepare_request_for_build( - 'binary-image:latest', 1, from_index, None, add_arches, bundles, distribution_scope, + 1, + binary_image, + from_index, + None, + add_arches, + bundles, + distribution_scope, + binary_image_config=binary_image_config, ) + if not binary_image: + binary_image = 'binary-image:prod' + assert rv == { 'arches': expected_arches, + 'binary_image': binary_image, 'binary_image_resolved': binary_image_resolved, 'bundle_mapping': expected_bundle_mapping, 'from_index_resolved': from_index_resolved, @@ -576,6 +605,7 @@ def test_update_index_image_build_state( ): prebuild_info = { 'arches': ['amd64', 's390x'], + 'binary_image': 'binary-image:1', 'binary_image_resolved': 'binary-image@sha256:12345', 'extra': 'ignored', } @@ -607,7 +637,12 @@ def test_prepare_request_for_build_no_arches(mock_gia, mock_gri, mock_srs): mock_gia.side_effect = [{'amd64'}] with pytest.raises(IIBError, match='No arches.+'): - build._prepare_request_for_build('binary-image:latest', 1) + build._prepare_request_for_build(1, 'binary-image:latest') + + +def test_get_binary_image_config_no_config_val(): + with pytest.raises(IIBError, match='IIB does not have a configured binary_image.+'): + build.get_binary_image_from_config('prod', 'v4.5', {'prod': {'v4.6': 'binary_image'}}) @mock.patch('iib.workers.tasks.build.set_request_state') @@ -618,7 +653,7 @@ def test_prepare_request_for_build_binary_image_no_arch(mock_gia, mock_gri, mock expected = 'The binary image is not available for the following arches.+' with pytest.raises(IIBError, match=expected): - build._prepare_request_for_build('binary-image:latest', 1, add_arches=['s390x']) + build._prepare_request_for_build(1, 'binary-image:latest', add_arches=['s390x']) @pytest.mark.parametrize('schema_version', (1, 2)) @@ -687,7 +722,9 @@ def test_skopeo_copy_fail_max_retries(mock_run_cmd): assert mock_run_cmd.call_count == 5 -@pytest.mark.parametrize('force_backport', (True, False)) +@pytest.mark.parametrize( + 'force_backport, binary_image', ((True, 'binary-image:latest'), (False, None)) +) @pytest.mark.parametrize('distribution_scope', ('dev', 'stage', 'prod')) @mock.patch('iib.workers.tasks.build._cleanup') @mock.patch('iib.workers.tasks.build._verify_labels') @@ -731,11 +768,14 @@ def test_handle_add_request( mock_vl, mock_cleanup, force_backport, + binary_image, distribution_scope, ): arches = {'amd64', 's390x'} + binary_image_config = {'prod': {'v4.5': 'some_image'}} mock_prfb.return_value = { 'arches': arches, + 'binary_image': binary_image or 'some_image', 'binary_image_resolved': 'binary-image@sha256:abcdef', 'from_index_resolved': 'from-index@sha256:bcdefg', 'ocp_version': 'v4.5', @@ -753,8 +793,8 @@ def test_handle_add_request( greenwave_config = {'some_key': 'other_value'} build.handle_add_request( bundles, - 'binary-image:latest', 3, + binary_image, 'from-index:latest', ['s390x'], cnr_token, @@ -764,11 +804,21 @@ def test_handle_add_request( None, None, greenwave_config, + binary_image_config=binary_image_config, ) mock_cleanup.assert_called_once() mock_vl.assert_called_once() - mock_prfb.assert_called_once() + mock_prfb.assert_called_once_with( + 3, + binary_image, + 'from-index:latest', + None, + ['s390x'], + ['some-bundle:2.3-1'], + None, + binary_image_config=binary_image_config, + ) mock_gb.assert_called_once() assert 2 == mock_alti.call_count mock_glsp.assert_called_once_with(['some-bundle@sha'], 3, 'v4.5', force_backport=force_backport) @@ -940,6 +990,7 @@ def test_handle_add_request_backport_failure_no_overwrite( mock_uiips.assert_not_called() +@pytest.mark.parametrize('binary_image', ('binary-image:latest', None)) @mock.patch('iib.workers.tasks.build._cleanup') @mock.patch('iib.workers.tasks.build._prepare_request_for_build') @mock.patch('iib.workers.tasks.build._update_index_image_build_state') @@ -963,19 +1014,36 @@ def test_handle_rm_request( mock_oir, mock_prfb, mock_cleanup, + binary_image, ): arches = {'amd64', 's390x'} mock_prfb.return_value = { 'arches': arches, + 'binary_image': binary_image, 'binary_image_resolved': 'binary-image@sha256:abcdef', 'from_index_resolved': 'from-index@sha256:bcdefg', 'ocp_version': 'v4.6', 'distribution_scope': 'PROD', } - build.handle_rm_request(['some-operator'], 'binary-image:latest', 3, 'from-index:latest') + binary_image_config = {'prod': {'v4.6': 'some_image'}} + build.handle_rm_request( + ['some-operator'], + 3, + 'from-index:latest', + binary_image, + binary_image_config=binary_image_config, + ) mock_cleanup.assert_called_once() - mock_prfb.assert_called_once() + mock_prfb.assert_called_once_with( + 3, + binary_image, + 'from-index:latest', + None, + None, + binary_image_config=binary_image_config, + distribution_scope=None, + ) mock_oir.assert_called_once() assert mock_alti.call_count == 2 assert mock_bi.call_count == len(arches) diff --git a/tests/test_workers/test_tasks/test_build_merge_index_image.py b/tests/test_workers/test_tasks/test_build_merge_index_image.py index 4d9e1a7e..ce4caab1 100644 --- a/tests/test_workers/test_tasks/test_build_merge_index_image.py +++ b/tests/test_workers/test_tasks/test_build_merge_index_image.py @@ -8,8 +8,11 @@ @pytest.mark.parametrize( - 'target_index, target_index_resolved', - (('target-from-index:1.0', 'target-index@sha256:resolved'), (None, None)), + 'target_index, target_index_resolved, binary_image', + ( + ('target-from-index:1.0', 'target-index@sha256:resolved', 'binary-image:1.0'), + (None, None, None), + ), ) @mock.patch('iib.workers.tasks.build_merge_index_image._update_index_image_pull_spec') @mock.patch('iib.workers.tasks.build_merge_index_image._verify_index_image') @@ -44,9 +47,11 @@ def test_handle_merge_request( mock_uiips, target_index, target_index_resolved, + binary_image, ): prebuild_info = { 'arches': {'amd64', 'other_arch'}, + 'binary_image': binary_image, 'target_ocp_version': '4.6', 'source_from_index_resolved': 'source-index@sha256:resolved', 'target_index_resolved': target_index_resolved, @@ -54,24 +59,27 @@ def test_handle_merge_request( } mock_prfb.return_value = prebuild_info mock_gbfdl.return_value = ['some-bundle:1.0'] + binary_image_config = {'prod': {'v4.5': 'some_image'}, 'stage': {'stage': 'some_other_img'}} build_merge_index_image.handle_merge_request( - 'binary-image:1.0', 'source-from-index:1.0', ['some-bundle:1.0'], 1, + binary_image, target_index, distribution_scope='stage', + binary_image_config=binary_image_config, ) mock_cleanup.assert_called_once() mock_prfb.assert_called_once_with( - 'binary-image:1.0', 1, + binary_image, overwrite_from_index_token=None, source_from_index='source-from-index:1.0', target_index=target_index, distribution_scope='stage', + binary_image_config=binary_image_config, ) mock_uiibs.assert_called_once_with(1, prebuild_info) if target_index: @@ -124,6 +132,7 @@ def test_handle_merge_request_no_deprecate( ): prebuild_info = { 'arches': {'amd64', 'other_arch'}, + 'binary_image': 'binary-image:1.0', 'target_ocp_version': '4.6', 'source_from_index_resolved': 'source-index@sha256:resolved', 'target_index_resolved': 'target-index@sha256:resolved', @@ -133,18 +142,19 @@ def test_handle_merge_request_no_deprecate( mock_gbfdl.return_value = [] build_merge_index_image.handle_merge_request( - 'binary-image:1.0', 'source-from-index:1.0', ['some-bundle:1.0'], 1, + 'binary-image:1.0', 'target-from-index:1.0', distribution_scope='stage', ) mock_cleanup.assert_called_once() mock_prfb.assert_called_once_with( - 'binary-image:1.0', 1, + 'binary-image:1.0', + binary_image_config=None, overwrite_from_index_token=None, source_from_index='source-from-index:1.0', target_index='target-from-index:1.0',