NSG feature branch cleanup. (#1422)

src/api-service/__app__/instance_config/__init__.py

@@ -10,7 +10,7 @@ from onefuzztypes.enums import ErrorCode
 from onefuzztypes.models import Error
 from onefuzztypes.requests import InstanceConfigUpdate
 
-from ..onefuzzlib.azure.nsg import is_one_fuzz_nsg, list_nsgs, set_allowed
+from ..onefuzzlib.azure.nsg import is_onefuzz_nsg, list_nsgs, set_allowed
 from ..onefuzzlib.config import InstanceConfig
 from ..onefuzzlib.endpoint_authorization import call_if_user, can_modify_config
 from ..onefuzzlib.request import not_ok, ok, parse_request
@@ -52,7 +52,7 @@ def post(req: func.HttpRequest) -> func.HttpResponse:
             logging.info(
                 "Checking if nsg: %s (%s) owned by OneFuzz" % (nsg.location, nsg.name)
             )
-            if is_one_fuzz_nsg(nsg.location, nsg.name):
+            if is_onefuzz_nsg(nsg.location, nsg.name):
                 result = set_allowed(nsg.location, request.config.proxy_nsg_config)
                 if isinstance(result, Error):
                     return not_ok(

src/api-service/__app__/onefuzzlib/azure/ip.py

@@ -5,7 +5,7 @@
 
 import logging
 import os
-from typing import Any, Dict, Optional, Union, cast
+from typing import Any, Dict, Optional, Union
 from uuid import UUID
 
 from azure.core.exceptions import ResourceNotFoundError
@@ -108,8 +108,8 @@ def create_public_nic(
         return None
 
     if nsg:
-        subnet = cast(Subnet, network.get_subnet())
+        subnet = network.get_subnet()
-        if not subnet.network_security_group:
+        if isinstance(subnet, Subnet) and not subnet.network_security_group:
             result = nsg.associate_subnet(network.get_vnet(), subnet)
             if isinstance(result, Error):
                 return result

src/api-service/__app__/onefuzzlib/azure/nsg.py

@@ -39,7 +39,7 @@ def get_nsg(name: str) -> Optional[NetworkSecurityGroup]:
         nsg = network_client.network_security_groups.get(resource_group, name)
         return cast(NetworkSecurityGroup, nsg)
     except (ResourceNotFoundError, CloudError) as err:
-        logging.debug("nsg %s does not exist: %s", name, err)
+        logging.error("nsg %s does not exist: %s", name, err)
         return None
 
 
@@ -102,15 +102,19 @@ def update_nsg(nsg: NetworkSecurityGroup) -> Union[None, Error]:
     return None
 
 
+# Return True if NSG is created using OneFuzz naming convention.
+# Therefore NSG belongs to OneFuzz.
 def ok_to_delete(active_regions: Set[Region], nsg_region: str, nsg_name: str) -> bool:
     return nsg_region not in active_regions and nsg_region == nsg_name
 
 
-def is_one_fuzz_nsg(nsg_region: str, nsg_name: str) -> bool:
+def is_onefuzz_nsg(nsg_region: str, nsg_name: str) -> bool:
     return nsg_region == nsg_name
 
 
-def delete_nsg(name: str) -> bool:
+# Returns True if deletion completed (thus resource not found) or successfully started.
+# Returns False if failed to start deletion.
+def start_delete_nsg(name: str) -> bool:
     # NSG can be only deleted if no other resource is associated with it
     resource_group = get_base_resource_group()
 
@@ -221,6 +225,9 @@ def associate_nic(name: str, nic: NetworkInterface) -> Union[None, Error]:
         )
 
     if nic.network_security_group and nic.network_security_group.id == nsg.id:
+        logging.info(
+            "NIC %s and NSG %s already associated, not updating", nic.name, name
+        )
         return None
 
     logging.info("associating nic %s with nsg: %s %s", nic.name, resource_group, name)
@@ -331,8 +338,10 @@ def associate_subnet(
             ],
         )
 
-    #  this is noop, since correct NSG is already assigned
     if subnet.network_security_group and subnet.network_security_group.id == nsg.id:
+        logging.info(
+            "Subnet %s and NSG %s already associated, not updating", subnet.name, name
+        )
         return None
 
     logging.info(
@@ -446,8 +455,8 @@ class NSG(BaseModel):
 
         return create_nsg(self.name, self.region)
 
-    def delete(self) -> bool:
+    def start_delete(self) -> bool:
-        return delete_nsg(self.name)
+        return start_delete_nsg(self.name)
 
     def get(self) -> Optional[NetworkSecurityGroup]:
         return get_nsg(self.name)

src/api-service/__app__/onefuzzlib/azure/subnet.py

@@ -46,8 +46,8 @@ def get_subnet(
 
 def get_subnet_id(resource_group: str, name: str, subnet_name: str) -> Optional[str]:
     subnet = get_subnet(resource_group, name, subnet_name)
-    if subnet:
+    if subnet and isinstance(subnet.id, str):
-        return cast(str, subnet.id)
+        return subnet.id
     else:
         return None
 

src/api-service/__app__/timer_proxy/__init__.py

@@ -12,10 +12,10 @@ from onefuzztypes.models import Error
 from ..onefuzzlib.azure.network import Network
 from ..onefuzzlib.azure.nsg import (
     associate_subnet,
-    delete_nsg,
     get_nsg,
     list_nsgs,
     ok_to_delete,
+    start_delete_nsg,
 )
 from ..onefuzzlib.orm import process_state_updates
 from ..onefuzzlib.proxy import PROXY_LOG_PREFIX, Proxy
@@ -82,4 +82,4 @@ def main(mytimer: func.TimerRequest) -> None:  # noqa: F841
     for nsg in list_nsgs():
         if ok_to_delete(regions, nsg.location, nsg.name):
             if nsg.network_interfaces is None and nsg.subnets is None:
-                delete_nsg(nsg.name)
+                start_delete_nsg(nsg.name)

src/deployment/deploylib/configuration.py

@@ -18,7 +18,6 @@ logger = logging.getLogger("deploy")
 
 
 class InstanceConfigClient:
-
     table_service: TableService
     resource_group: str
 
@@ -65,7 +64,7 @@ class NetworkSecurityConfig:
             raise Exception(
                 "Empty Configuration File Provided. Please Provide Valid Config."
             )
-        if None in config.keys() or "proxy_nsg_config" not in config.keys():
+        if "proxy_nsg_config" not in config:
             raise Exception(
                 "proxy_nsg_config not provided as valid key. Please Provide Valid Config."
             )
@@ -74,14 +73,13 @@ class NetworkSecurityConfig:
             raise Exception(
                 "Inner Configuration is not a Dictionary. Please provide Valid Config."
             )
-        if len(proxy_config.keys()) == 0:
+        if len(proxy_config) == 0:
             raise Exception(
                 "Empty Inner Configuration File Provided. Please Provide Valid Config."
             )
         if (
-            None in proxy_config.keys()
+            "allowed_ips" not in proxy_config
-            or "allowed_ips" not in proxy_config.keys()
+            or "allowed_service_tags" not in proxy_config
-            or "allowed_service_tags" not in proxy_config.keys()
         ):
             raise Exception(
                 "allowed_ips and allowed_service_tags not provided. Please Provide Valid Config."
@@ -111,7 +109,6 @@ class NetworkSecurityConfig:
 
 
 class NsgRule:
-
     rule: str
     is_tag: bool
 
@@ -197,7 +194,7 @@ def parse_rules(proxy_config: NetworkSecurityConfig) -> List[NsgRule]:
             nsg_rules.append(nsg_rule)
         except Exception:
             raise ValueError(
-                "One or more input ips was invalid: %s. Please enter a comma-separted list of valid sources.",
+                "One or more input ips was invalid: %s. Please enter a comma-separated list of valid sources.",
                 rule,
             )
     for rule in allowed_service_tags:
@@ -206,7 +203,7 @@ def parse_rules(proxy_config: NetworkSecurityConfig) -> List[NsgRule]:
             nsg_rules.append(nsg_rule)
         except Exception:
             raise ValueError(
-                "One or more input tags was invalid: %s. Please enter a comma-separted list of valid sources.",
+                "One or more input tags was invalid: %s. Please enter a comma-separated list of valid sources.",
                 rule,
             )
     return nsg_rules

src/deployment/deploylib/tests/test_deploy_config.py

@@ -9,11 +9,6 @@ from typing import Any, List
 from deploylib.configuration import NetworkSecurityConfig
 
 
-class TestNetworkSecurityConfig:
-    allowed_ips: List[str]
-    allowed_service_tags: List[str]
-
-
 class DeployTests(unittest.TestCase):
     def test_config(self) -> None:
         ## Test Invalid Configs

src/utils/check-pr/check-pr.py

@@ -191,7 +191,7 @@ class Deployer:
         subprocess.check_call(f"python -mvenv {venv}", shell=True)
         pip = venv_path(venv, "pip")
         py = venv_path(venv, "python")
-        config = os.getcwd() + "/config.json"
+        config = os.path.join(os.getcwd(), "config.json")
         commands = [
             ("extracting release-artifacts", f"unzip -qq {filename}"),
             ("extracting deployment", "unzip -qq onefuzz-deployment*.zip"),