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

ItemPaged[CertificateProperties] KeyVault Certificate list_properties_of_certificates does not honor the include_pending optional parameter after the first page #38589

Open
ahsonkhan opened this issue Nov 18, 2024 · 2 comments
Labels

Comments

@ahsonkhan
Copy link
Member

ahsonkhan commented Nov 18, 2024

The call to fetch the first page sets the appropriate query parameters based on the input parameter value:

else:
kwargs.update({"include_pending": include_pending})
return self._client.get_certificates(
vault_base_url=self._vault_url,
maxresults=max_page_size,
cls=lambda objs: [CertificateProperties._from_certificate_item(certificate_item=x) for x in objs],
**kwargs
)

But subsequent pages do not have that value set. This is done in the generated code. That means, if the include_pending param is set to true, it will not return all the certificates (including pending ones), if the pending certificate happens to be listed in a page other than the first.

if not next_link:
_request = build_key_vault_get_certificates_request(
maxresults=maxresults,
include_pending=include_pending,
api_version=self._config.api_version,
headers=_headers,
params=_params,
)
path_format_arguments = {
"vaultBaseUrl": self._serialize.url("vault_base_url", vault_base_url, "str", skip_quote=True),
}
_request.url = self._client.format_url(_request.url, **path_format_arguments)
else:
# make call to next link with the client's api-version
_parsed_next_link = urllib.parse.urlparse(next_link)
_next_request_params = case_insensitive_dict(
{
key: [urllib.parse.quote(v) for v in value]
for key, value in urllib.parse.parse_qs(_parsed_next_link.query).items()
}
)
_next_request_params["api-version"] = self._config.api_version
_request = HttpRequest(
"GET", urllib.parse.urljoin(next_link, _parsed_next_link.path), params=_next_request_params
)
path_format_arguments = {
"vaultBaseUrl": self._serialize.url("vault_base_url", vault_base_url, "str", skip_quote=True),
}
_request.url = self._client.format_url(_request.url, **path_format_arguments)
return _request

Here's the swagger (not sure if this requires some fix to the swagger):
https://github.com/Azure/azure-rest-api-specs/blob/4a4acecea9901c29e19ba50f2d4cf65b20115b69/specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.5/certificates.json#L30-L83

Sample repro:

from azure.identity import DefaultAzureCredential
from azure.keyvault.certificates import CertificateClient

# Pre-req: Create 25 certificates first so a page is full (either through the portal or programmatically)

# Case 1: Create a certificate (either on the portal or programmatically) on the first page, and run this, right away.
# Works as expected.
# Case 2: Create a certificate (either on the portal or programmatically) on any subsequent page, and run this, right away.
# Doesn't work as expected.

credential = DefaultAzureCredential()

# TODO: Set to your own KeyVault URL
certificate_client = CertificateClient(vault_url="https://<keyvault-name>.vault.azure.net/", credential=credential)

countFalse = 0
print("Certificates in the key vault (includePending = false):")
certificates = certificate_client.list_properties_of_certificates()
for certificate in certificates:
    print(certificate.name)
    countFalse += 1

countTrue = 0
print("Certificates in the key vault (includePending = true):")
certificatesTrue = certificate_client.list_properties_of_certificates(include_pending=True)
for certificate in certificatesTrue:
    print(certificate.name)
    countTrue += 1

# Expected countFalse < countTrue in both cases, since there's a certificate pending.
# Case 1: In the case where the certificate gets created on the first page:
#  -> countFalse < countTrue
# Case 2: But, in the case where the certificate gets created on any other subsequent page:
#  -> countFalse = countTrue
print(f"countFalse = {countFalse} vs countTrue = {countTrue}")

The issue is pervasive across all the ItemPaged methods that follow this pattern within the KeyVault SDKs, but list_properties_of_certificates and list_deleted_certificates (along with list_role_assignments in KeyVault Administration) seem to be the only ones that have optional parameters which are settable by the SDK methods (unlike maxResults) and hence have an actual behavioral bug here.

It's possible that some other service SDKs have similar concerns here.

Related issues in other languages:
Azure/azure-sdk-for-net#47202
Azure/azure-sdk-for-cpp#6235
#38589
Azure/azure-sdk-for-go#23772
Azure/azure-sdk-for-js#31803
Azure/azure-sdk-for-java#42988

@lmazuel
Copy link
Member

lmazuel commented Nov 18, 2024

As discussed offline, this is by design, see guidelines:
https://github.com/microsoft/api-guidelines/blob/vNext/azure/Guidelines.md#collections

@lmazuel lmazuel closed this as not planned Won't fix, can't repro, duplicate, stale Nov 18, 2024
@lmazuel lmazuel reopened this Nov 18, 2024
@lmazuel
Copy link
Member

lmazuel commented Nov 18, 2024

Re-open, as we discovered that the service was doing the wrong thing, so it may require some custom code to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants