Skip to content

API discovery: Prevent overwrite of API parameters in cases where API names are same#4609

Merged
yadvr merged 1 commit intoapache:4.15from
shapeblue:api-discovery-fix
Feb 18, 2021
Merged

API discovery: Prevent overwrite of API parameters in cases where API names are same#4609
yadvr merged 1 commit intoapache:4.15from
shapeblue:api-discovery-fix

Conversation

@Pearl1594
Copy link
Contributor

Description

This PR addresses an issue that crops up during API discovery when we have the same API name pointing to 2 command classes, namely, xxxCmdByAdmin and xxxCmd.
During api discovery we create a map of the apiNames and their corresponding parameters, however, in cases of api's like createNetwork which point to both CreateNetworkCmd and CreateNetworkCmdByAdmin classes, though in ManagementServerImpl care has been taken to place the xxxCmdByAdmin classes after the regular xxxCmd classes, since we copy this list of command classes into a HashSet, is so happens, that sometimes the xxxCmdByAdmin Classes get discovered before their generic counterpart. Hence, parameters defined in the xxxCmdByAdmin class gets overwritten. This can be easily fixed by ensuring that the order of the API classes is maintained.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

Cases when this can be seen:
This issue becomes predominant in the new UI as we use discovered API parameters to define the form fields:
image

Also,when logged in as admin via cmk and after doing a sync, listing params of createNetwork sometimes doesn't list the parameters defined in the CreateNetworkCmdByAdmin class - e.g., vlan, bypassoverlapcheck, etc

(localcloud) SBCM5> > create network -h
createNetwork: Creates a network
Required params: displaytext, name, networkofferingid, zoneid, 
API Params               Type     Description
==========               ====     ===========
account                  string   account that will own the network
aclid                    uuid     Network ACL ID associated for the network
                                  
acltype                  string   Access control type; supported values are
                                   account and domain. In 3.0 all shared n
                                  etworks should have aclType=Domain, and 
                                  all isolated networks - Account. Account
                                   means that only the account owner can u
                                  se the network, domain - all accounts in
                                   the domain can use the network
displaynetwork           boolean  an optional field, whether to the display
                                   the network to the end user or not.
displaytext              string   the display text of the network
domainid                 uuid     domain ID of the account owning a network
                                  
endip                    string   the ending IP address in the network IP r
                                  ange. If not specified, will be defaulte
                                  d to startIP
endipv6                  string   the ending IPv6 address in the IPv6 netwo
                                  rk range
externalid               string   ID of the network in an external system.
gateway                  string   the gateway of the network. Required for 
                                  shared networks and isolated networks wh
                                  en it belongs to VPC
ip6cidr                  string   the CIDR of IPv6 network, must be at leas
                                  t /64
ip6gateway               string   the gateway of the IPv6 network. Required
                                   for Shared networks
isolatedpvlan            string   the isolated private VLAN for this networ
                                  k
name                     string   the name of the network
netmask                  string   the netmask of the network. Required for 
                                  shared networks and isolated networks wh
                                  en it belongs to VPC
networkdomain            string   network domain
networkofferingid        uuid     the network offering ID
physicalnetworkid        uuid     the physical network ID the network belon
                                  gs to
projectid                uuid     an optional project for the SSH key
startip                  string   the beginning IP address in the network I
                                  P range
startipv6                string   the beginning IPv6 address in the IPv6 ne
                                  twork range
subdomainaccess          boolean  Defines whether to allow subdomains to us
                                  e networks dedicated to their parent dom
                                  ain(s). Should be used with aclType=Doma
                                  in, defaulted to allow.subdomain.network
                                  .access global config if not specified
vpcid                    uuid     the VPC network belongs to
zoneid                   uuid     the zone ID for the network

However, the listAPIs API enlists the parameters.
(localcloud) SBCM5> > list apis name=createNetwork
{
  "api": [
    {
      "description": "Creates a network",
      "isasync": true,
      "name": "createNetwork",
      "params": [
        {
          "description": "the physical network ID the network belongs to",
          "length": 255,
          "name": "physicalnetworkid",
          "related": "createPhysicalNetwork",
          "required": false,
          "type": "uuid"
        },
        {
...
        {
          "description": "The vlan of the network. This parameter is visible to ROOT admins only",
          "name": "vlan",
          "type": "string"
        }
      ]
    }
  ],
  "count": 1
}

@Pearl1594 Pearl1594 changed the title API discovery: Prevent overwrite of API parameters in case the API na… API discovery: Prevent overwrite of API parameters in cases where API names are same Jan 22, 2021
@Pearl1594 Pearl1594 requested a review from yadvr January 22, 2021 09:39
@yadvr
Copy link
Member

yadvr commented Jan 22, 2021

This sounds like a bugfix can you open against 4.15 @Pearl1594 ?

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.

trivial enough, should do as described

@Pearl1594 Pearl1594 changed the base branch from master to 4.15 January 22, 2021 11:04
@DaanHoogland DaanHoogland added this to the 4.15.1.0 milestone Jan 22, 2021
@Pearl1594
Copy link
Contributor Author

This sounds like a bugfix can you open against 4.15 @Pearl1594 ?

done @rhtyd

Copy link
Member

@harikrishna-patnala harikrishna-patnala left a comment

Choose a reason for hiding this comment

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

LGTM

@yadvr
Copy link
Member

yadvr commented Jan 25, 2021

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@yadvr
Copy link
Member

yadvr commented Jan 27, 2021

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2599

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

@yadvr
Copy link
Member

yadvr commented Jan 27, 2021

Previous job failed, rekicking
@blueorangutan test

@blueorangutan
Copy link

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-3432)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46515 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4609-t3432-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_07_deploy_kubernetes_ha_cluster Failure 3619.89 test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster Failure 0.05 test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster Failure 0.04 test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown Error 63.02 test_kubernetes_clusters.py

@yadvr yadvr merged commit d6509f0 into apache:4.15 Feb 18, 2021
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.

5 participants