diff --git a/src/api-service/__app__/instance_config/__init__.py b/src/api-service/__app__/instance_config/__init__.py index 75b27af6f5..23c7bb9bc8 100644 --- a/src/api-service/__app__/instance_config/__init__.py +++ b/src/api-service/__app__/instance_config/__init__.py @@ -10,7 +10,7 @@ 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( diff --git a/src/api-service/__app__/onefuzzlib/azure/ip.py b/src/api-service/__app__/onefuzzlib/azure/ip.py index 78529d428c..c959d8b877 100644 --- a/src/api-service/__app__/onefuzzlib/azure/ip.py +++ b/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()) - if not subnet.network_security_group: + subnet = network.get_subnet() + if isinstance(subnet, Subnet) and not subnet.network_security_group: result = nsg.associate_subnet(network.get_vnet(), subnet) if isinstance(result, Error): return result diff --git a/src/api-service/__app__/onefuzzlib/azure/nsg.py b/src/api-service/__app__/onefuzzlib/azure/nsg.py index b202e6ccbb..320ef43ef8 100644 --- a/src/api-service/__app__/onefuzzlib/azure/nsg.py +++ b/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 @@ def create(self) -> Union[None, Error]: return create_nsg(self.name, self.region) - def delete(self) -> bool: - return delete_nsg(self.name) + def start_delete(self) -> bool: + return start_delete_nsg(self.name) def get(self) -> Optional[NetworkSecurityGroup]: return get_nsg(self.name) diff --git a/src/api-service/__app__/onefuzzlib/azure/subnet.py b/src/api-service/__app__/onefuzzlib/azure/subnet.py index 5723db10c7..a66af94f37 100644 --- a/src/api-service/__app__/onefuzzlib/azure/subnet.py +++ b/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: - return cast(str, subnet.id) + if subnet and isinstance(subnet.id, str): + return subnet.id else: return None diff --git a/src/api-service/__app__/timer_proxy/__init__.py b/src/api-service/__app__/timer_proxy/__init__.py index 3a18e4bd18..fa4f0e7482 100644 --- a/src/api-service/__app__/timer_proxy/__init__.py +++ b/src/api-service/__app__/timer_proxy/__init__.py @@ -12,10 +12,10 @@ 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) diff --git a/src/deployment/deploylib/configuration.py b/src/deployment/deploylib/configuration.py index cdeecd0767..dd1bcacd20 100644 --- a/src/deployment/deploylib/configuration.py +++ b/src/deployment/deploylib/configuration.py @@ -18,7 +18,6 @@ class InstanceConfigClient: - table_service: TableService resource_group: str @@ -65,7 +64,7 @@ def parse_nsg_json(self, config: Any) -> None: 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 @@ def parse_nsg_json(self, config: Any) -> None: 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() - or "allowed_ips" not in proxy_config.keys() - or "allowed_service_tags" not in proxy_config.keys() + "allowed_ips" not in proxy_config + or "allowed_service_tags" not in proxy_config ): raise Exception( "allowed_ips and allowed_service_tags not provided. Please Provide Valid Config." @@ -111,7 +109,6 @@ def parse_nsg_json(self, config: Any) -> None: 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 diff --git a/src/deployment/deploylib/tests/test_deploy_config.py b/src/deployment/deploylib/tests/test_deploy_config.py index 1bc816802e..6e39780ed6 100644 --- a/src/deployment/deploylib/tests/test_deploy_config.py +++ b/src/deployment/deploylib/tests/test_deploy_config.py @@ -9,11 +9,6 @@ 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 diff --git a/src/utils/check-pr/check-pr.py b/src/utils/check-pr/check-pr.py index c9bf917beb..ec59c12c44 100755 --- a/src/utils/check-pr/check-pr.py +++ b/src/utils/check-pr/check-pr.py @@ -191,7 +191,7 @@ def deploy(self, filename: str) -> None: 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"),