Skip to content

Fix ICMP ACL rule edge case#8464

Merged
DaanHoogland merged 1 commit intoapache:mainfrom
scclouds:main-fix-acl-icmp-rules
Feb 14, 2024
Merged

Fix ICMP ACL rule edge case#8464
DaanHoogland merged 1 commit intoapache:mainfrom
scclouds:main-fix-acl-icmp-rules

Conversation

@gpordeus
Copy link
Contributor

@gpordeus gpordeus commented Jan 8, 2024

Description

An ICMP ACL rule should not be able to have code and type null. This PR fixes an edge case, that can be reproduced by:

  • Creating a VPC and an ACL.
  • Creating a TCP rule.
  • Calling the API with updateNetworkACLItem and parameters protocol=icmp, partialupgrade=false, cidrlist="" and traffictype=ingress.

It will then set the rule with null ICMP code and type and the VR will face problems.

This PR just extends the validation made when partialupgrade is false.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

By following the steps mentioned in the description:

  • With protocol=icmp, code and type became -1.
  • With protocol=tcp and protocol=all, the rule was changed and the attributes were reset accordingly.

@kiranchavala
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@codecov
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9d4f071) 30.80% compared to head (9d3ea7f) 29.02%.
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8464      +/-   ##
============================================
- Coverage     30.80%   29.02%   -1.78%     
+ Complexity    33981    31784    -2197     
============================================
  Files          5341     5341              
  Lines        374864   374900      +36     
  Branches      54518    54530      +12     
============================================
- Hits         115485   108829    -6656     
- Misses       244114   251489    +7375     
+ Partials      15265    14582     -683     
Flag Coverage Δ
simulator-marvin-tests 22.30% <0.00%> (-2.42%) ⬇️
uitests 4.39% <ø> (ø)
unit-tests 16.46% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kiranchavala kiranchavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested manually

Before the fix

List networkacls api call

    {
      "aclid": "e79f3198-62a8-43f7-96c1-02f02c44766d",
      "aclname": "test",
      "action": "Allow",
      "cidrlist": "10.0.0.0/24",
      "fordisplay": true,
      "icmpcode": -1,
      "icmptype": -1,
      "id": "a30c97a1-4548-49a4-9b65-b45f9719cfd0",
      "number": 1,
      "protocol": "icmp",
      "reason": "",
      "state": "Active",
      "tags": [],
      "traffictype": "Ingress"
    },

After the api call (update networkaclitem ) is called

update networkaclitem protocol=icmp partialupgrade=false cidrlist="" traffictype=ingress id=a30c97a1-4548-49a4-9b65-b45f9719cfd0

The network acl is set with a null ICMP code and type

{
  "aclid": "e79f3198-62a8-43f7-96c1-02f02c44766d",
  "aclname": "test",
  "action": "Allow",
  "cidrlist": "",
  "fordisplay": true,
  "id": "a30c97a1-4548-49a4-9b65-b45f9719cfd0",
  "number": 1,
  "protocol": "icmp",
  "state": "Active",
  "tags": [],
  "traffictype": "Ingress"
},

After fix , the acl id is not changed

    {
      "aclid": "e79f3198-62a8-43f7-96c1-02f02c44766d",
      "aclname": "test",
      "action": "Allow",
      "cidrlist": "10.0.0.0/24",
      "fordisplay": true,
      "icmpcode": -1,
      "icmptype": -1,
      "id": "a30c97a1-4548-49a4-9b65-b45f9719cfd0",
      "number": 1,
      "protocol": "icmp",
      "reason": "",
      "state": "Active",
      "tags": [],
      "traffictype": "Ingress"
    },

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

Copy link
Contributor

@BryanMLima BryanMLima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLGTM

@yadvr yadvr added this to the 4.20.0.0 milestone Feb 8, 2024
@yadvr
Copy link
Member

yadvr commented Feb 8, 2024

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@DaanHoogland
Copy link
Contributor

@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-9165)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 1603 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8464-t9165-kvm-alma9.zip
Smoke tests completed. 7 look OK, 122 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
runTest Error 0.00 test_2fa.py
runTest Error 0.00 test_diagnostics.py
runTest Error 0.00 test_accounts.py
runTest Error 0.00 test_deploy_vm_extra_config_data.py
runTest Error 0.00 test_affinity_groups_projects.py
runTest Error 0.00 test_deploy_vms_with_varied_deploymentplanners.py
runTest Error 0.00 test_affinity_groups.py
runTest Error 0.00 test_annotations.py
runTest Error 0.00 test_domain_disk_offerings.py
runTest Error 0.00 test_async_job.py
runTest Error 0.00 test_attach_multiple_volumes.py
runTest Error 0.00 test_direct_download.py
runTest Error 0.00 test_backup_recovery_dummy.py
runTest Error 0.00 test_events_resource.py
runTest Error 0.00 test_backup_recovery_veeam.py
runTest Error 0.00 test_deploy_vm_iso.py
runTest Error 0.00 test_certauthority_root.py
runTest Error 0.00 test_disk_provisioning_types.py
runTest Error 0.00 test_cluster_drs.py
runTest Error 0.00 test_global_acls.py
runTest Error 0.00 test_console_endpoint.py
runTest Error 0.00 test_deploy_vm_with_userdata.py
runTest Error 0.00 test_create_list_domain_account_project.py
runTest Error 0.00 test_global_settings.py
runTest Error 0.00 test_create_network.py
runTest Error 0.00 test_deploy_vgpu_enabled_vm.py
runTest Error 0.00 test_disk_offerings.py
runTest Error 0.00 test_guest_os.py
runTest Error 0.00 test_deploy_virtio_scsi_vm.py
runTest Error 0.00 test_deploy_vm_iso_uefi.py
runTest Error 0.00 test_guest_vlan_range.py
runTest Error 0.00 test_deploy_vm_root_resize.py
runTest Error 0.00 test_deploy_vms_in_parallel.py
runTest Error 0.00 test_domain_network_offerings.py
runTest Error 0.00 test_domain_service_offerings.py
runTest Error 0.00 test_domain_vpc_offerings.py
runTest Error 0.00 test_enable_account_settings_for_domain.py
runTest Error 0.00 test_gateway_on_shared_networks.py
runTest Error 0.00 test_host_control_state.py
runTest Error 0.00 test_hostha_simulator.py
runTest Error 0.00 test_host_ping.py
runTest Error 0.00 test_image_store_object_migration.py
runTest Error 0.00 test_internal_lb.py
runTest Error 0.00 test_ipv6_infra.py
runTest Error 0.00 test_iso.py
runTest Error 0.00 test_kubernetes_clusters.py
runTest Error 0.00 test_kubernetes_supported_versions.py
runTest Error 0.00 test_list_accounts.py
runTest Error 0.00 test_list_disk_offerings.py
runTest Error 0.00 test_over_provisioning.py
runTest Error 0.00 test_list_domains.py
runTest Error 0.00 test_list_hosts.py
runTest Error 0.00 test_nonstrict_affinity_group.py
runTest Error 0.00 test_list_ids_parameter.py
runTest Error 0.00 test_list_service_offerings.py
runTest Error 0.00 test_list_storage_pools.py
runTest Error 0.00 test_list_volumes.py
runTest Error 0.00 test_password_server.py
runTest Error 0.00 test_loadbalance.py
runTest Error 0.00 test_login.py
runTest Error 0.00 test_metrics_api.py
runTest Error 0.00 test_migration.py
runTest Error 0.00 test_multipleips_per_nic.py
runTest Error 0.00 test_nested_virtualization.py
runTest Error 0.00 test_network_acl.py
runTest Error 0.00 test_portable_publicip.py
runTest Error 0.00 test_network_ipv6.py
runTest Error 0.00 test_outofbandmanagement.py
runTest Error 0.00 test_network_permissions.py
runTest Error 0.00 test_network.py
runTest Error 0.00 test_nic_adapter_type.py
runTest Error 0.00 test_nic.py
runTest Error 0.00 test_non_contigiousvlan.py
runTest Error 0.00 test_outofbandmanagement_nestedplugin.py
runTest Error 0.00 test_persistent_network.py
runTest Error 0.00 test_portforwardingrules.py
runTest Error 0.00 test_primary_storage.py
runTest Error 0.00 test_privategw_acl_ovs_gre.py
runTest Error 0.00 test_privategw_acl.py
runTest Error 0.00 test_projects.py
runTest Error 0.00 test_public_ip_range.py
runTest Error 0.00 test_pvlan.py
runTest Error 0.00 test_quarantined_ips.py
runTest Error 0.00 test_regions.py
runTest Error 0.00 test_register_userdata.py
runTest Error 0.00 test_volumes.py
runTest Error 0.00 test_reset_configuration_settings.py
runTest Error 0.00 test_reset_vm_on_reboot.py
runTest Error 0.00 test_vm_life_cycle.py
runTest Error 0.00 test_resource_accounting.py
runTest Error 0.00 test_resource_detail.py
runTest Error 0.00 test_vm_schedule.py
runTest Error 0.00 test_router_dhcphosts.py
runTest Error 0.00 test_router_dns.py
runTest Error 0.00 test_router_dnsservice.py
runTest Error 0.00 test_vm_deployment_planner.py
runTest Error 0.00 test_routers_iptables_default_policy.py
runTest Error 0.00 test_routers_network_ops.py
runTest Error 0.00 test_routers.py
runTest Error 0.00 test_vm_snapshot_kvm.py
runTest Error 0.00 test_safe_shutdown.py
runTest Error 0.00 test_scale_vm.py
runTest Error 0.00 test_secondary_storage.py
runTest Error 0.00 test_service_offerings.py
runTest Error 0.00 test_vm_snapshots.py
runTest Error 0.00 test_set_sourcenat.py
runTest Error 0.00 test_vpc_ipv6.py
runTest Error 0.00 test_snapshots.py
runTest Error 0.00 test_ssvm.py
runTest Error 0.00 test_storage_policy.py
runTest Error 0.00 test_templates.py
runTest Error 0.00 test_update_security_group.py
runTest Error 0.00 test_usage_events.py
runTest Error 0.00 test_usage.py
runTest Error 0.00 test_vnf_templates.py
runTest Error 0.00 test_vm_autoscaling.py
runTest Error 0.00 test_vm_lifecycle_unmanage_import.py
runTest Error 0.00 test_vpc_redundant.py
runTest Error 0.00 test_vpc_router_nics.py
runTest Error 0.00 test_vpc_vpn.py
runTest Error 0.00 test_host_maintenance.py
runTest Error 0.00 test_hostha_kvm.py

@DaanHoogland
Copy link
Contributor

if everything fails apply insanity:
@blueorangutan test alma9 kvm-alma9

@blueorangutan
Copy link

@DaanHoogland a [SL] Trillian-Jenkins test job (alma9 mgmt + kvm-alma9) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-9188)
Environment: kvm-alma9 (x2), Advanced Networking with Mgmt server a9
Total time taken: 50733 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8464-t9188-kvm-alma9.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@DaanHoogland DaanHoogland merged commit 4a0ca20 into apache:main Feb 14, 2024
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 20, 2024
@gpordeus gpordeus deleted the main-fix-acl-icmp-rules branch April 5, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants