fix(cssc): 31250186 address all comments in public repo PR (redo commit)

src/acrcssc/azext_acrcssc/_params.py

@@ -7,6 +7,7 @@
 from azure.cli.command_modules.acr._validators import validate_registry_name
 from azure.cli.core import AzCommandsLoader
 from azure.cli.core.commands.parameters import (get_resource_name_completion_list, get_three_state_flag, get_enum_type)
+from .helper._constants import CONTINUOUSPATCH_SCHEDULE_MAX_DAYS
 
 
 def load_arguments(self: AzCommandsLoader, _):
@@ -20,13 +21,13 @@ def load_arguments(self: AzCommandsLoader, _):
 
     with self.argument_context("acr supply-chain workflow create") as c:
         c.argument("config", options_list=["--config"], help="Configuration file path containing the json schema for the list of repositories and tags to filter within the registry. Schema example:{\"repositories\":[{\"repository\":\"alpine\",\"tags\":[\"tag1\",\"tag2\"],\"enabled\":true},{\"repository\":\"python\",\"tags\":[\"*\"],\"enabled\":false}], \"version\": \"v1\", \"tag-convention\": \"floating\"}. \"tag-convention\" is an optional property, values can be \"incremental\" (the default behavior, will increase the patch version of the tag, for example \"{repository}:{original-tag}-1\", \"{repository}:{original-tag}-2\", etc), or \"floating\" (will reuse the tag \"{repository}:{original-tag}-patched\" for patching)", required=True)
-        c.argument("schedule", options_list=["--schedule"], help="schedule to run the scan and patching task. E.g. `<n>d` where <n> is the number of days between each run. Max value is 30d.", required=True)
+        c.argument("schedule", options_list=["--schedule"], help=f"schedule to run the scan and patching task. E.g. `<n>d` where <n> is the number of days between each run. Max value is {CONTINUOUSPATCH_SCHEDULE_MAX_DAYS}d.", required=True)
         c.argument("run_immediately", options_list=["--run-immediately"], help="Set this flag to trigger the immediate run of the selected workflow task. Default value: false.", arg_type=get_three_state_flag(), required=False)
         c.argument("dryrun", options_list=["--dry-run"], help="Use this flag to see the qualifying repositories and tags that would be affected by the workflow. Default value: false. 'config' parameter is mandatory to provide with dry-run", arg_type=get_three_state_flag(), required=False)
 
     with self.argument_context("acr supply-chain workflow update") as c:
         c.argument("config", options_list=["--config"], help="Configuration file path containing the json schema for the list of repositories and tags to filter within the registry. Schema example:{\"repositories\":[{\"repository\":\"alpine\",\"tags\":[\"tag1\",\"tag2\"],\"enabled\":true},{\"repository\":\"python\",\"tags\":[\"*\"],\"enabled\":false}], \"version\": \"v1\", \"tag-convention\": \"floating\"}}. \"tag-convention\" is an optional property, values can be \"incremental\" (the default behavior, will increase the patch version of the tag, for example \"{repository}:{original-tag}-1\", \"{repository}:{original-tag}-2\", etc), or \"floating\" (will reuse the tag \"{repository}:{original-tag}-patched\" for patching)", required=False)
-        c.argument("schedule", options_list=["--schedule"], help="schedule to run the scan and patching task. E.g. `<n>d` where n is the number of days between each run. Max value is 30d.", required=False)
+        c.argument("schedule", options_list=["--schedule"], help=f"schedule to run the scan and patching task. E.g. `<n>d` where n is the number of days between each run. Max value is {CONTINUOUSPATCH_SCHEDULE_MAX_DAYS}d.", required=False)
         c.argument("run_immediately", options_list=["--run-immediately"], help="Set this flag to trigger the immediate run of the selected workflow task. Default value: false.", arg_type=get_three_state_flag(), required=False)
         c.argument("dryrun", options_list=["--dry-run"], help="Use this flag to see the qualifying repositories and tags that would be affected by the workflow. Default value: false. 'config' parameter is mandatory to provide with dry-run", arg_type=get_three_state_flag(), required=False)
 

src/acrcssc/azext_acrcssc/_validators.py

@@ -18,6 +18,8 @@
     CONTINUOUSPATCH_CONFIG_SCHEMA_V1,
     CONTINUOUSPATCH_CONFIG_SCHEMA_SIZE_LIMIT,
     CONTINUOUSPATCH_ALL_TASK_NAMES,
+    CONTINUOUSPATCH_SCHEDULE_MIN_DAYS,
+    CONTINUOUSPATCH_SCHEDULE_MAX_DAYS,
     ERROR_MESSAGE_INVALID_TIMESPAN_FORMAT,
     ERROR_MESSAGE_INVALID_TIMESPAN_VALUE,
     SUBSCRIPTION)
@@ -43,7 +45,7 @@ def _validate_continuouspatch_file(config_path):
     if not os.path.isfile(config_path):
         raise InvalidArgumentValueError(f"Config path file: {config_path} is not a valid file")
     if os.path.getsize(config_path) > CONTINUOUSPATCH_CONFIG_SCHEMA_SIZE_LIMIT:
-        raise InvalidArgumentValueError(f"Config path file: {config_path} is too large. Max size limit is 10 MB")
+        raise InvalidArgumentValueError(f"Config path file: {config_path} is too large. Max size limit is {CONTINUOUSPATCH_CONFIG_SCHEMA_SIZE_LIMIT / (1024 * 1024)} MB")
     if os.path.getsize(config_path) == 0:
         raise InvalidArgumentValueError(f"Config path file: {config_path} is empty")
     if not os.access(config_path, os.R_OK):
@@ -60,8 +62,6 @@ def _validate_continuouspatch_json(config_path):
     except Exception as e:
         logger.error(f"Error validating the continuous patch config file: {e}")
         raise InvalidArgumentValueError("File used for --config is not a valid config JSON file. Use --help to see the schema of the config file.")
-    finally:
-        f.close()
 
 
 def _validate_continuouspatch_config(config):
@@ -130,7 +130,7 @@ def _validate_schedule(schedule):
 
     value = int(match.group(1))
     unit = match.group(2)
-    if unit == 'd' and (value < 1 or value > 30):  # day of the month
+    if unit == 'd' and (value < CONTINUOUSPATCH_SCHEDULE_MIN_DAYS or value > CONTINUOUSPATCH_SCHEDULE_MAX_DAYS):  # day of the month
         raise InvalidArgumentValueError(error_msg=ERROR_MESSAGE_INVALID_TIMESPAN_VALUE, recommendation=RECOMMENDATION_SCHEDULE)
 
 
@@ -148,7 +148,7 @@ def validate_run_type(dryrun, run_immediately):
 
 def validate_task_type(task_type):
     if (task_type not in [item.value for item in CSSCTaskTypes]):
-        raise InvalidArgumentValueError(error_msg=ERROR_MESSAGE_INVALID_TASK)
+        raise InvalidArgumentValueError(error_msg=ERROR_MESSAGE_INVALID_TASK % task_type)
 
 
 def validate_cssc_optional_inputs(cssc_config_path, schedule):

src/acrcssc/azext_acrcssc/helper/_constants.py

@@ -56,6 +56,9 @@ class TaskRunStatus(Enum):
 CONTINUOUSPATCH_TASK_SCANREGISTRY_DESCRIPTION = f"This task will trigger the continuous patching workflow based on the schedule set during the creation. It will match the filter repositories set with config parameter and schedule vulnerability scan check using {CONTINUOUSPATCH_TASK_SCANIMAGE_NAME} task."
 CONTINUOUS_PATCHING_WORKFLOW_NAME = CSSCTaskTypes.ContinuousPatchV1.value
 DESCRIPTION = "Description"
+CONTINUOUSPATCH_SCHEDULE_MIN_DAYS = 1
+CONTINUOUSPATCH_SCHEDULE_MAX_DAYS = 30
+
 
 CONTINUOUSPATCH_ALL_TASK_NAMES = [
     CONTINUOUSPATCH_TASK_PATCHIMAGE_NAME,
@@ -69,10 +72,10 @@ class TaskRunStatus(Enum):
 
 ERROR_MESSAGE_WORKFLOW_TASKS_DOES_NOT_EXIST = f"{CONTINUOUS_PATCHING_WORKFLOW_NAME} workflow tasks does not exist. Run 'az acr supply-chain workflow create' command to create workflow tasks."
 ERROR_MESSAGE_WORKFLOW_TASKS_ALREADY_EXISTS = f"{CONTINUOUS_PATCHING_WORKFLOW_NAME} workflow tasks already exists. Use 'az acr supply-chain workflow update' command to perform updates."
-ERROR_MESSAGE_INVALID_TASK = "Workflow type is invalid"
+ERROR_MESSAGE_INVALID_TASK = "Workflow type %s is invalid"
 ERROR_MESSAGE_INVALID_TIMESPAN_VALUE = "Schedule value is invalid. "
 ERROR_MESSAGE_INVALID_TIMESPAN_FORMAT = "Schedule format is invalid. "
-RECOMMENDATION_SCHEDULE = "Schedule must be in the format of <number><unit> where unit is d for days. Example: 1d. Max value for d is 30d."
+RECOMMENDATION_SCHEDULE = f"Schedule must be in the format of <number><unit> where unit is d for days. Example: {CONTINUOUSPATCH_SCHEDULE_MIN_DAYS}d. Max value for d is {CONTINUOUSPATCH_SCHEDULE_MAX_DAYS}d."
 # this dictionary can be expanded to handle more configuration of the tasks regarding continuous patching
 # if this gets out of hand, or more types of tasks are supported, this should be a class on its own
 CONTINUOUSPATCH_TASK_DEFINITION = {

src/acrcssc/azext_acrcssc/helper/_deployment.py

@@ -131,10 +131,10 @@ def deploy_template(cmd_ctx, resource_group, deployment_name, template):
     )(poller)
     logger.debug("Finished deploying")
 
-    if deployment.properties is not None:
-        depl_props = deployment.properties
-    else:
+    if deployment.properties is None:
         raise RuntimeError("The deployment has no properties.\nAborting")
+
+    depl_props = deployment.properties
     logger.debug(f"Deployed: {deployment.name} {deployment.id} {depl_props}")
 
     if depl_props.provisioning_state != "Succeeded":

src/acrcssc/azext_acrcssc/helper/_ociartifactoperations.py

@@ -7,7 +7,6 @@
 # pylint: disable=logging-fstring-interpolation
 import os
 import dataclasses
-import tempfile
 import shutil
 import subprocess
 
@@ -17,6 +16,7 @@
 from azure.mgmt.core.tools import parse_resource_id
 from jsonschema.exceptions import ValidationError
 from knack.log import get_logger
+from tempfile import NamedTemporaryFile
 from ._constants import (
     BEARER_TOKEN_USERNAME,
     CONTINUOUSPATCH_CONFIG_SCHEMA_V1,
@@ -39,39 +39,33 @@ def create_oci_artifact_continuous_patch(registry, cssc_config_file, dryrun):
     logger.debug(f"Entering create_oci_artifact_continuouspatching with parameters: {registry.name} {cssc_config_file} {dryrun}")
 
     oras_client = None
-    temp_artifact_name = None
     try:
         oras_client = _oras_client(registry)
-        # we might have to handle the tag lock/unlock for the cssc config file,
-        # to make it harder for the user to change it by mistake
 
         # the ORAS client can only work with files under the current directory
-        temp_artifact = tempfile.NamedTemporaryFile(
+        with NamedTemporaryFile(
             prefix="cssc_config_tmp_",
             mode="w+b",
             dir=os.getcwd(),
-            delete=False)
-
-        temp_artifact_name = temp_artifact.name
-        user_artifact = open(cssc_config_file, "rb")
-        shutil.copyfileobj(user_artifact, temp_artifact)
-        temp_artifact.close()
-
-        if dryrun:
-            oci_target_name = f"{CSSC_WORKFLOW_POLICY_REPOSITORY}/{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG}:{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG_TAG_DRYRUN}"
-        else:
-            oci_target_name = f"{CSSC_WORKFLOW_POLICY_REPOSITORY}/{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG}:{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG_TAG_V1}"
-
-        oras_client.push(
-            target=oci_target_name,
-            files=[temp_artifact.name])
+            delete=False
+        ) as temp_artifact, open(cssc_config_file, "rb") as user_artifact:
+            shutil.copyfileobj(user_artifact, temp_artifact)
+            temp_artifact.flush()  # Ensure all data is written to disk
+            temp_artifact_name = temp_artifact.name
+
+            if dryrun:
+                oci_target_name = f"{CSSC_WORKFLOW_POLICY_REPOSITORY}/{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG}:{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG_TAG_DRYRUN}"
+            else:
+                oci_target_name = f"{CSSC_WORKFLOW_POLICY_REPOSITORY}/{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG}:{CONTINUOUSPATCH_OCI_ARTIFACT_CONFIG_TAG_V1}"
+
+            oras_client.push(
+                target=oci_target_name,
+                files=[temp_artifact_name])
     except Exception as exception:
         raise AzCLIError(f"Failed to push OCI artifact to ACR: {exception}")
     finally:
         if oras_client:
             oras_client.logout(hostname=str.lower(registry.login_server))
-        if temp_artifact_name and os.path.exists(temp_artifact_name):
-            os.remove(temp_artifact_name)
 
 
 def get_oci_artifact_continuous_patch(cmd, registry):
@@ -87,6 +81,7 @@ def get_oci_artifact_continuous_patch(cmd, registry):
                                          overwrite=True)
         trigger_task = get_task(cmd, registry, CONTINUOUSPATCH_TASK_SCANREGISTRY_NAME)
         file_name = oci_artifacts[0]
+        logger.debug(f"OCI artifact file name: {file_name}, trigger task: {trigger_task}")
         config = ContinuousPatchConfig().from_file(file_name, trigger_task)
     except Exception as exception:
         raise AzCLIError(f"Failed to get OCI artifact from ACR: {exception}")

src/acrcssc/azext_acrcssc/helper/_taskoperations.py

@@ -31,12 +31,12 @@
     DESCRIPTION,
     WORKFLOW_VALIDATION_MESSAGE,
     TaskRunStatus)
-from azure.cli.core.azclierror import AzCLIError, InvalidArgumentValueError
+from azure.cli.core.azclierror import AzCLIError, InvalidArgumentValueError, ResourceNotFoundError
 from azure.cli.core.commands import LongRunningOperation
 from azure.cli.core.commands.progress import IndeterminateProgressBar
 from azure.cli.command_modules.acr._utils import prepare_source_location
 from azure.cli.command_modules.acr._archive_utils import logger as acr_archive_utils_logger
-from azure.core.exceptions import ResourceNotFoundError, HttpResponseError
+from azure.core.exceptions import HttpResponseError
 from azure.mgmt.core.tools import parse_resource_id
 from azext_acrcssc._client_factory import cf_acr_tasks, cf_authorization, cf_acr_registries_tasks, cf_acr_runs
 from azext_acrcssc.helper._deployment import validate_and_deploy_template
@@ -67,7 +67,6 @@ def create_update_continuous_patch_v1(cmd,
         schedule_cron_expression = convert_timespan_to_cron(schedule)
         logger.debug(f"converted schedule to cron expression: {schedule_cron_expression}")
 
-    cssc_tasks_exists, task_list = check_continuous_task_exists(cmd, registry)
     if is_create_workflow:
         if cssc_tasks_exists:
             raise AzCLIError(f"{ERROR_MESSAGE_WORKFLOW_TASKS_ALREADY_EXISTS}")
@@ -186,15 +185,25 @@ def list_continuous_patch_v1(cmd, registry):
 
     acr_task_client = cf_acr_tasks(cmd.cli_ctx)
     resource_group_name = parse_resource_id(registry.id)[RESOURCE_GROUP]
-    tasks_list = acr_task_client.list(resource_group_name, registry.name)
-    filtered_cssc_tasks = _transform_task_list(tasks_list)
+    task_list = []
+    for task in CONTINUOUSPATCH_ALL_TASK_NAMES:
+        try:
+            task_list.append(acr_task_client.get(resource_group_name, registry.name, task))
+            logger.debug(f"Task {task} exists in registry {registry.name}")
+        except ResourceNotFoundError:
+            logger.debug(f"Task {task} does not exist in registry {registry.name}")
+
+    filtered_cssc_tasks = _transform_task_list(task_list)
     return filtered_cssc_tasks
 
 
 def acr_cssc_dry_run(cmd, registry, config_file_path, is_create=True, remove_internal_statements=True):
     logger.debug(f"Entering acr_cssc_dry_run with parameters: {registry} {config_file_path}")
     cssc_tasks_exists, _ = check_continuous_task_exists(cmd, registry)
 
+    if is_create and cssc_tasks_exists:
+        raise AzCLIError(f"{ERROR_MESSAGE_WORKFLOW_TASKS_ALREADY_EXISTS}")
+
     if config_file_path is None:
         if not cssc_tasks_exists:
             raise InvalidArgumentValueError("--config parameter is needed to perform dry-run check.")
@@ -205,9 +214,6 @@ def acr_cssc_dry_run(cmd, registry, config_file_path, is_create=True, remove_int
         if config_file_path is None:
             raise AzCLIError("Failed to retrieve the configuration file from the registry.")
 
-    if is_create and cssc_tasks_exists:
-        raise AzCLIError(f"{ERROR_MESSAGE_WORKFLOW_TASKS_ALREADY_EXISTS}")
-
     file_name = None
     tmp_folder = None
     acr_archive_utils_logger_level = acr_archive_utils_logger.getEffectiveLevel()
@@ -259,7 +265,7 @@ def acr_cssc_dry_run(cmd, registry, config_file_path, is_create=True, remove_int
                 architecture=platform_arch,
                 variant=platform_variant
             ),
-            credentials=_get_custom_registry_credentials(cmd, auth_mode=None),
+            credentials=_get_custom_registry_credentials(cmd),
             agent_pool_name=None,
             log_template=None
         )
@@ -455,7 +461,6 @@ def _delete_task(cmd, registry, task_name):
 
 def _delete_task_role_assignment(cli_ctx, acrtask_client, registry, resource_group, task_name):
     role_client = cf_authorization(cli_ctx)
-    acrtask_client = cf_acr_tasks(cli_ctx)
 
     try:
         task = acrtask_client.get(resource_group, registry.name, task_name)
@@ -473,11 +478,16 @@ def _delete_task_role_assignment(cli_ctx, acrtask_client, registry, resource_gro
         )
 
         for role in assigned_roles:
-            logger.debug(f"Deleting role assignments of task {task_name} from the registry")
-            role_client.role_assignments.delete(
-                scope=registry.id,
-                role_assignment_name=role.name
-            )
+            try:
+                logger.debug(f"Deleting role assignments of task {task_name} from the registry")
+                role_client.role_assignments.delete(
+                    scope=registry.id,
+                    role_assignment_name=role.name
+                )
+            except ResourceNotFoundError:
+                logger.debug(f"Role assignment {role.name} does not exist in registry {registry.name}")
+            except AzCLIError as exception:
+                logger.error(f"Failed to delete role assignment {role.name} from registry {registry.name} : {exception}")
 
 
 def _transform_task_list(tasks):
@@ -508,60 +518,11 @@ def _transform_task_list(tasks):
     return transformed
 
 
-def _get_custom_registry_credentials(cmd,
-                                     auth_mode=None,
-                                     login_server=None,
-                                     username=None,
-                                     password=None,
-                                     identity=None,
-                                     is_remove=False):
-    """Get the credential object from the input
-    :param str auth_mode: The login mode for the source registry
-    :param str login_server: The login server of custom registry
-    :param str username: The username for custom registry (plain text or a key vault secret URI)
-    :param str password: The password for custom registry (plain text or a key vault secret URI)
-    :param str identity: The task managed identity used for the credential
-    """
+def _get_custom_registry_credentials(cmd):
     acr_tasks_client = cf_acr_tasks(cmd.cli_ctx)
-    source_registry_credentials = None
-    if auth_mode:
-        source_registry_credentials = acr_tasks_client.models.SourceRegistryCredentials(
-            login_mode=auth_mode)
-
-    custom_registries = None
-    if login_server:
-        # if null username and password (or identity), then remove the credential
-        custom_reg_credential = None
-
-        is_identity_credential = False
-        if not username and not password:
-            is_identity_credential = identity is not None
-
-        if not is_remove:
-            if is_identity_credential:
-                custom_reg_credential = acr_tasks_client.models.CustomRegistryCredentials(
-                    identity=identity
-                )
-            else:
-                custom_reg_credential = acr_tasks_client.models.CustomRegistryCredentials(
-                    user_name=acr_tasks_client.models.SecretObject(
-                        type=acr_tasks_client.models.SecretObjectType.vaultsecret if _is_vault_secret(
-                            cmd, username)else acr_tasks_client.models.SecretObjectType.opaque,
-                        value=username
-                    ),
-                    password=acr_tasks_client.models.SecretObject(
-                        type=acr_tasks_client.models.SecretObjectType.vaultsecret if _is_vault_secret(
-                            cmd, password) else acr_tasks_client.models.SecretObjectType.opaque,
-                        value=password
-                    ),
-                    identity=identity
-                )
-
-        custom_registries = {login_server: custom_reg_credential}
-
     return acr_tasks_client.models.Credentials(
-        source_registry=source_registry_credentials,
-        custom_registries=custom_registries
+        source_registry=None,
+        custom_registries=None
     )