Fix auth unit test and integration tests

ZiyueXu77 · Sep 29, 2023 · 57fea64 · 57fea64
1 parent bb97789
commit 57fea64
Show file tree

Hide file tree

Showing 10 changed files with 101 additions and 47 deletions.
diff --git a/nvflare/private/fed/server/job_meta_validator.py b/nvflare/private/fed/server/job_meta_validator.py
@@ -55,7 +55,7 @@ def validate(self, job_name: str, job_data: bytes) -> Tuple[bool, str, dict]:
                 self._validate_resource(job_name, meta)
                 self._validate_mandatory_clients(job_name, meta, clients)
         except ValueError as e:
-            return False, secure_format_exception(e), meta
+            return False, str(e), meta
 
         return True, "", meta
 

diff --git a/nvflare/private/fed/server/server_engine.py b/nvflare/private/fed/server/server_engine.py
@@ -756,8 +756,6 @@ def check_client_resources(self, job: Job, resource_reqs, fl_ctx: FLContext) ->
     def _make_message_for_check_resource(self, job, resource_requirements, fl_ctx):
         request = Message(topic=TrainingTopic.CHECK_RESOURCE, body=resource_requirements)
         request.set_header(RequestHeader.JOB_ID, job.job_id)
-        request.set_header(RequestHeader.REQUIRE_AUTHZ, "true")
-        request.set_header(RequestHeader.ADMIN_COMMAND, AdminCommandNames.CHECK_RESOURCES)
 
         set_message_security_data(request, job, fl_ctx)
         return request

diff --git a/tests/integration_test/data/test_configs/authorization/test_diff_job_config.yml b/tests/integration_test/data/test_configs/authorization/test_diff_job_config.yml
@@ -16,7 +16,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: Multiple apps to be deployed to following sites ['site-1'] for job duplicate_clients"
+              "data": "Multiple apps to be deployed to following sites ['site-1'] for job duplicate_clients"
             }
           ]
       - "trigger":
@@ -37,7 +37,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: Multiple apps to be deployed to following sites ['server'] for job duplicate_server"
+              "data": "Multiple apps to be deployed to following sites ['server'] for job duplicate_server"
             }
           ]
       - "trigger":
@@ -58,7 +58,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: value for key site-1 in resource spec is expecting a dictionary"
+              "data": "value for key site-1 in resource spec is expecting a dictionary"
             }
           ]
       - "trigger":
@@ -79,7 +79,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: Mandatory clients {'site-1'} are not in the deploy_map for job mandatory_not_met"
+              "data": "Mandatory clients {'site-1'} are not in the deploy_map for job mandatory_not_met"
             }
           ]
       - "trigger":
@@ -100,7 +100,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: min_clients -1 must be positive for job min_clients_value_range"
+              "data": "min_clients -1 must be positive for job min_clients_value_range"
             }
           ]
       - "trigger":
@@ -121,7 +121,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: App 'sag2' in deploy_map doesn't exist for job missing_app"
+              "data": "App 'sag2' in deploy_map doesn't exist for job missing_app"
             }
           ]
       - "trigger":
@@ -142,7 +142,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: App 'sag2' will be deployed to client but client config is missing"
+              "data": "App 'sag2' will be deployed to client but client config is missing"
             }
           ]
       - "trigger":
@@ -163,7 +163,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: App 'sag1' will be deployed to server but server config is missing"
+              "data": "App 'sag1' will be deployed to server but server config is missing"
             }
           ]
       - "trigger":
@@ -184,7 +184,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: No site is specified in deploy_map for job no_deployment"
+              "data": "No site is specified in deploy_map for job no_deployment"
             }
           ]
       - "trigger":
@@ -205,7 +205,7 @@ tests:
           "data": [
             {
               "type": "error",
-              "data": "ValueError: min 4 clients required for job not_enough_clients, found 2."
+              "data": "min 4 clients required for job not_enough_clients, found 2."
             }
           ]
       - "trigger":
@@ -215,21 +215,27 @@ tests:
         "result":
           "type": "run_state"
           "data": {}
-  - test_name: "submit valid_app_as_job"
+  - test_name: "submit old_app"
     event_sequence:
       - "trigger":
           "type": "server_log"
           "data": "Server started"
-        "actions": [ "submit_job valid_app_as_job" ]
+        "actions": [ "submit_job old_app" ]
         "result":
-          "type": "job_submit_success"
+          "type": "admin_api_response"
+          "data": [
+            {
+              "type": "error",
+              "data": "meta.[json|conf|yml] not existing for job old_app, possible in legacy job format.  Please upgrade the job structure."
+            }
+          ]
       - "trigger":
-          "type": "run_state"
-          "data": { "run_finished": True }
-        "actions": [ "ensure_current_job_done" ]
+          "type": "server_log"
+          "data": "Server started"
+        "actions": [ "mark_test_done" ]
         "result":
           "type": "run_state"
-          "data": { "run_finished": True }
+          "data": {}
   - test_name: "submit valid_job"
     event_sequence:
       - "trigger":

diff --git a/tests/unit_test/data/jobs/min_clients_value_range/meta.json b/tests/unit_test/data/jobs/min_clients_value_range/meta.json
@@ -1,7 +1,7 @@
 
 { "name": "sag",
   "resource_spec": {},
-  "deploy_map": {"sag": ["server","site-1", "site-2"]},
+  "deploy_map": {"sag": ["server", "site-1", "site-2"]},
   "min_clients" : -1
 }
 
diff --git a/..._app_as_job/config/config_fed_client.json → ...obs/old_app/config/config_fed_client.json b/..._app_as_job/config/config_fed_client.json → ...obs/old_app/config/config_fed_client.json
diff --git a/..._app_as_job/config/config_fed_server.json → ...obs/old_app/config/config_fed_server.json b/..._app_as_job/config/config_fed_server.json → ...obs/old_app/config/config_fed_server.json
diff --git a/tests/unit_test/data/jobs/valid_job_wo_job_name/meta.json b/tests/unit_test/data/jobs/valid_job_wo_job_name/meta.json
@@ -0,0 +1,10 @@
+{
+  "resource_spec": {},
+  "deploy_map": {
+    "sag": [
+      "server",
+      "site-1",
+      "site-2"
+    ]
+  }
+}
diff --git a/tests/unit_test/data/jobs/valid_job_wo_job_name/sag/config/config_fed_client.json b/tests/unit_test/data/jobs/valid_job_wo_job_name/sag/config/config_fed_client.json
@@ -0,0 +1,17 @@
+{
+  "format_version": 2,
+  "executors": [
+    {
+      "tasks": [
+        "train"
+      ],
+      "executor": {
+        "path": "nvflare.app_common.np.np_trainer.NPTrainer",
+        "args": {}
+      }
+    }
+  ],
+  "task_result_filters": [],
+  "task_data_filters": [],
+  "components": []
+}
diff --git a/tests/unit_test/data/jobs/valid_job_wo_job_name/sag/config/config_fed_server.json b/tests/unit_test/data/jobs/valid_job_wo_job_name/sag/config/config_fed_server.json
@@ -0,0 +1,44 @@
+{
+  "format_version": 2,
+  "server": {
+    "heart_beat_timeout": 600
+  },
+  "task_data_filters": [],
+  "task_result_filters": [],
+  "components": [
+    {
+      "id": "persistor",
+      "path": "nvflare.app_common.np.np_model_persistor.NPModelPersistor",
+      "args": {}
+    },
+    {
+      "id": "shareable_generator",
+      "path": "nvflare.app_common.shareablegenerators.full_model_shareable_generator.FullModelShareableGenerator",
+      "args": {}
+    },
+    {
+      "id": "aggregator",
+      "path": "nvflare.app_common.aggregators.intime_accumulate_model_aggregator.InTimeAccumulateWeightedAggregator",
+      "args": {
+        "expected_data_kind": "WEIGHTS"
+      }
+    }
+  ],
+  "workflows": [
+    {
+      "id": "scatter_and_gather",
+      "path": "nvflare.app_common.workflows.scatter_and_gather.ScatterAndGather",
+      "args": {
+        "min_clients": 2,
+        "num_rounds": 1,
+        "start_round": 0,
+        "wait_time_after_min_received": 0,
+        "aggregator_id": "aggregator",
+        "persistor_id": "persistor",
+        "shareable_generator_id": "shareable_generator",
+        "train_task_name": "train",
+        "train_timeout": 6000
+      }
+    }
+  ]
+}
diff --git a/tests/unit_test/private/fed/server/job_meta_validator_test.py b/tests/unit_test/private/fed/server/job_meta_validator_test.py
@@ -21,7 +21,6 @@
 import pytest
 
 from nvflare.apis.fl_constant import JobConstants
-from nvflare.apis.utils.job_utils import convert_legacy_zipped_app_to_job
 from nvflare.fuel.utils.zip_utils import get_all_file_paths, normpath_for_zip, split_path
 from nvflare.private.fed.server.job_meta_validator import JobMetaValidator
 
@@ -52,7 +51,7 @@ def _zip_job_with_meta(folder_name: str, meta: str) -> bytes:
     bio = io.BytesIO()
     _zip_directory_with_meta(job_path, folder_name, meta, bio)
     zip_data = bio.getvalue()
-    return convert_legacy_zipped_app_to_job(zip_data)
+    return zip_data
 
 
 META_WITH_VALID_DEPLOY_MAP = [
@@ -78,7 +77,7 @@ def _zip_job_with_meta(folder_name: str, meta: str) -> bytes:
 VALID_JOBS = [
     pytest.param("valid_job", id="valid_job"),
     pytest.param("valid_job_deployment_all_idle", id="valid_job_deployment_all_idle"),
-    pytest.param("valid_app_as_job", id="valid_app_wo_meta"),
+    pytest.param("valid_job_wo_job_name", id="valid_job_wo_job_name"),
 ]
 
 
@@ -93,6 +92,7 @@ def _zip_job_with_meta(folder_name: str, meta: str) -> bytes:
     pytest.param("missing_server_in_deployment", id="missing_server_in_deploy_map"),
     pytest.param("no_deployment", id="no_deployment"),
     pytest.param("not_enough_clients", id="not_enough_clients"),
+    pytest.param("old_app", id="old_app"),
 ]
 
 
@@ -139,29 +139,8 @@ def test_invalid_min_clients_value_range(self, min_clients):
         """
         self._assert_invalid(job_name, meta)
 
-    def test_deploy_map_config_non_exists_app(self):
-        job_name = "valid_job"
-        # valid_job folder contains sag app, not hello-pt app
-        meta = """
-            { 
-              "resource_spec": { "site-a": {"gpu": 1}, "site-b": {"gpu": 1}}, 
-              "deploy_map": {"hello-pt": ["server","site-a", "site-b"]}
-            }
-        """
-        self._assert_invalid(job_name, meta)
-
-    def test_meta_missing_job_folder_name(self):
-        job_name = "valid_job"
-        meta = """
-            { 
-              "resource_spec": { "site-a": {"gpu": 1}, "site-b": {"gpu": 1}}, 
-              "deploy_map": {"sag": ["server","site-a", "site-b"]}
-            }
-        """
-        self._assert_valid(job_name, meta)
-
-    def _assert_valid(self, job_name: str, meta: str = ""):
-        data = _zip_job_with_meta(job_name, meta)
+    def _assert_valid(self, job_name: str):
+        data = _zip_job_with_meta(job_name, "")
         valid, error, meta = self.validator.validate(job_name, data)
         assert valid
         assert error == ""