Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getOpenApiHighestElevatedVersion bug for unsupported API version #683

Merged
merged 5 commits into from
Jun 12, 2024

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Jun 12, 2024

There is a bug in core OpenAPI version picking function getOpenApiHighestElevatedVersion that could choose an unsupported API version when automatic elevation mechanism endpointElevatedApiVersions is used.

This bug can start occurring when API v36.0 is no longer supported.

The bug occurs in OpenAPI handling function getOpenApiHighestElevatedVersion that picks client API versions for OpenAPI endpoints. It occurs when both of the following conditions are met:

  • Endpoint in question uses automatic elevation mechanism (has at least single entry in endpointElevatedApiVersions map)
  • Endpoint has one or more elevation records in endpointElevatedApiVersions, but all of these entries only elevate to API versions older than default minimum supported API version (37.0 at this moment)

Example

So an example when the bug occurs is: We have an endpoint 1.0.0/firewallGroups/. This endpoint leverages automatic elevation mechanism and only has a single record:
"36.0", // Adds support for Dynamic Security Groups by deprecating Type field in favor of TypeValue in endpointElevatedApiVersions.

The function getOpenApiHighestElevatedVersion has a bug when evaluating such conditions and it picks the version that is in the endpointElevatedApiVersions which is 36.0 in a given example. This is not good and will fail when minimum supported version is 37.0.

There are many, but example test that surfaces this bug is Test_NsxtFirewall

Affected tests:

  • Test_AlbVirtualService
  • Test_NsxtDynamicSecurityGroup
  • Test_NsxtEdgeVdcGroup
  • Test_NsxtFirewall
  • Test_NsxtIpSet
  • Test_NsxtNatDnat
  • Test_NsxtNatDnatExternalPortPort
  • Test_NsxtNatDnatFirewallMatchPriority
  • Test_NsxtNatNoDnat
  • Test_NsxtNatNoSnat
  • Test_NsxtNatPriorityAndFirewallMatch
  • Test_NsxtNatReflexive
  • Test_NsxtNatSnat
  • Test_NsxtOrgVdcNetworkImportedNsxtLogicalSwitch
  • Test_NsxtOrgVdcNetworkIsolated
  • Test_NsxtOrgVdcNetworkRouted
  • Test_NsxtSegmentProfileTemplate
  • Test_NsxtVdcGroupForceDelete
  • Test_VdcNetworkProfile

Signed-off-by: Dainius Serplis <[email protected]>
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius changed the title Fix getOpenApiHighestElevatedVersion bug Fix getOpenApiHighestElevatedVersion bug for unsupported API version Jun 12, 2024
@Didainius Didainius marked this pull request as ready for review June 12, 2024 08:23
Copy link
Collaborator

@adambarreiro adambarreiro left a comment

Choose a reason for hiding this comment

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

LGTM

govcd/openapi_endpoints_unit_test.go Outdated Show resolved Hide resolved
Signed-off-by: Dainius Serplis <[email protected]>
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

A suggestion inline to add more logs for this case.

govcd/openapi_endpoints_unit_test.go Show resolved Hide resolved
Signed-off-by: Dainius Serplis <[email protected]>
@Didainius Didainius merged commit 164f27a into vmware:main Jun 12, 2024
2 checks passed
@Didainius Didainius deleted the fix_openapi_version_elevation branch June 12, 2024 11:00
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