-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[ACR Query Extension] Bug fix: omitting login server endpoint suffix #6606
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
Changes from all commits
5c3206e
8c736ae
85501f5
0eab77a
b774c6e
f8f0e7c
127f385
d3efb70
db718ca
c057918
6792bb4
d994f35
e2c10f3
0e698c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,13 @@ | |
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||
| # -------------------------------------------------------------------------------------------- | ||
|
|
||
| from azure.cli.command_modules.acr._validators import validate_registry_name | ||
|
|
||
|
|
||
| def load_arguments(self, _): | ||
|
|
||
| with self.argument_context('acr query') as c: | ||
| c.argument('registry_name', options_list=['--name', '-n'], help='The name of the container registry that the query is run against.') | ||
| c.argument('registry_name', options_list=['--name', '-n'], validator=validate_registry_name, help='The name of the container registry that the query is run against.') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question, do we need update history.rst? |
||
| c.argument('repository', help='The repository that the query is run against. If no repository is provided, the query is run at the registry level.') | ||
| c.argument('kql_query', options_list=['--kql-query', '-q'], help='The KQL query to execute.') | ||
| c.argument('skip_token', help='Skip token to get the next page of the query if applicable.') | ||
|
|
||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,52 +3,99 @@ | |
| # Licensed under the MIT License. See License.txt in the project root for license information. | ||
| # -------------------------------------------------------------------------------------------- | ||
|
|
||
| from azure.cli.testsdk import ScenarioTest | ||
| import unittest | ||
| from unittest import mock | ||
| import json | ||
|
|
||
| from azext_acrquery.query import ( | ||
| create_query | ||
| ) | ||
|
|
||
| from azure.cli.command_modules.acr._docker_utils import ( | ||
| EMPTY_GUID | ||
| EMPTY_GUID, | ||
| get_authorization_header, | ||
| ) | ||
|
|
||
| from azure.cli.core.mock import DummyCli | ||
|
|
||
|
|
||
| class AcrQueryCommandsTests(unittest.TestCase): | ||
|
|
||
| @mock.patch('azext_acrquery.query.get_access_credentials', autospec=True) | ||
| @mock.patch('requests.request', autospec=True) | ||
| def test_acrquery(self, mock_requests_get, mock_get_access_credentials): | ||
| cmd = self._setup_cmd() | ||
|
|
||
| response = mock.MagicMock() | ||
| response.headers = {} | ||
| response.status_code = 200 | ||
| response.content = json.dumps({'repositories': ['testrepo1', 'testrepo2']}).encode() | ||
| mock_requests_get.return_value = response | ||
|
|
||
| # Basic auth | ||
| mock_get_access_credentials.return_value = 'testregistry.azurecr.io', 'username', 'password' | ||
| create_query(cmd, 'testregistry', 'get') | ||
| mock_requests_get.assert_called_with( | ||
| method='post', | ||
| url='https://testregistry.azurecr.io/acr/v1/_metadata/_query', | ||
| headers=get_authorization_header('username', 'password'), | ||
| params=None, | ||
| json={ | ||
| 'query': 'get' | ||
| }, | ||
| timeout=300, | ||
| verify=mock.ANY) | ||
|
|
||
| # Bearer auth | ||
| mock_get_access_credentials.return_value = 'testregistry.azurecr.io', EMPTY_GUID, 'password' | ||
| create_query(cmd, 'testregistry', 'get') | ||
| mock_requests_get.assert_called_with( | ||
| method='post', | ||
| url='https://testregistry.azurecr.io/acr/v1/_metadata/_query', | ||
| headers=get_authorization_header(EMPTY_GUID, 'password'), | ||
| params=None, | ||
| json={ | ||
| 'query': 'get' | ||
| }, | ||
| timeout=300, | ||
| verify=mock.ANY) | ||
|
|
||
| # Filter by repository | ||
| mock_get_access_credentials.return_value = 'testregistry.azurecr.io', EMPTY_GUID, 'password' | ||
| create_query(cmd, 'testregistry', 'get', repository='repository') | ||
| mock_requests_get.assert_called_with( | ||
| method='post', | ||
| url='https://testregistry.azurecr.io/acr/v1/repository/_metadata/_query', | ||
| headers=get_authorization_header(EMPTY_GUID, 'password'), | ||
| params=None, | ||
| json={ | ||
| 'query': 'get' | ||
| }, | ||
| timeout=300, | ||
| verify=mock.ANY) | ||
|
|
||
| # Request with skip token | ||
| mock_get_access_credentials.return_value = 'testregistry.azurecr.io', EMPTY_GUID, 'password' | ||
| create_query(cmd, 'testregistry', 'get', repository='repository', skip_token='12345678') | ||
| mock_requests_get.assert_called_with( | ||
| method='post', | ||
| url='https://testregistry.azurecr.io/acr/v1/repository/_metadata/_query', | ||
| headers=get_authorization_header(EMPTY_GUID, 'password'), | ||
| params=None, | ||
| json={ | ||
| 'query': 'get', | ||
| 'options': { | ||
| '$skipToken': '12345678' | ||
| } | ||
| }, | ||
| timeout=300, | ||
| verify=mock.ANY) | ||
|
|
||
| class AcrQueryTests(ScenarioTest): | ||
|
|
||
| def test_acrquery(self): | ||
| self.kwargs.update({ | ||
| 'registry_name': 'metadataunittest', | ||
| 'resource_group': 'cabarker', | ||
| 'repository_name': 'nginx', | ||
| 'query': '"Manifests | limit 1"', | ||
| 'username': 'user', | ||
| 'password': 'password', | ||
| 'password_name': 'password' | ||
| }) | ||
|
|
||
| # Query with bearer auth and filter by count | ||
| credentials = self.cmd( | ||
| 'acr credential show -n {registry_name} -g {resource_group}').get_output_in_json() | ||
|
|
||
| self.kwargs['username'] = credentials['username'] | ||
| self.kwargs['password'] = credentials['passwords'][0]['value'] | ||
|
|
||
| self.cmd( | ||
| 'acr query -n {registry_name} -q {query} --username {username} --password {password} ', | ||
| checks=[self.check('count', 1)]) | ||
|
|
||
| # Query with basic auth and filter by repository | ||
| token = self.cmd('acr login -n {registry_name} --expose-token').get_output_in_json() | ||
| self.kwargs['username'] = EMPTY_GUID | ||
| self.kwargs['password'] = token["accessToken"] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @zhoxing-ms, is there a secure way for me to pass in credentials so that they do not show up in our recording files? This particular service is blocked by a feature flag that we cannot enable during the test--requiring us to log in to a pre-existing registry. If not, I can remove it temporarily and we can replace with mocks until the feature flag is removed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, you can try using the customized
If this is only a one-time temporary need, it is also acceptable
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Preparer generates a new resource within a specified or new resource group correct? In our case, the resource would need to exist already prior to running the tests (and we would need to login to it). Would it still be possible to use Preparer?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test scenario we have is that we have a pre-created resources because the feature is blocked by default we need to enable it in a specific resources. Should we avoid using pre-created resources for testing?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @CarolineNB Yes, please use the dynamically created resources in the test instead of pre-created resources to avoid the problem of dependent resources being deleted will cause the test to be failed |
||
| self.kwargs['query'] = '"Manifests"' | ||
| self.kwargs['repository_name'] = 'test/new' | ||
|
|
||
| self.cmd( | ||
| 'acr query -n {registry_name} --repository {repository_name} -q {query} --username {username} --password {password} ', | ||
| checks=[self.check('count', 12)]) | ||
|
|
||
| # Renew credentials | ||
| self.cmd( | ||
| 'acr credential renew -n {registry_name} --password-name {password_name} ') | ||
|
|
||
| # Filter by size | ||
| self.kwargs['query'] = '"Manifests | where imageSize > 100000000"' | ||
| self.cmd( | ||
| 'acr query -n {registry_name} -q {query}', checks=[self.check('count', 3)]) | ||
| def _setup_cmd(self): | ||
| cmd = mock.MagicMock() | ||
| cmd.cli_ctx = DummyCli() | ||
| mock_sku = mock.MagicMock() | ||
| mock_sku.classic.value = 'Classic' | ||
| mock_sku.basic.value = 'Basic' | ||
| cmd.get_models.return_value = mock_sku | ||
| return cmd | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test on it?