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.

Sign up for GitHub

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

Jump to bottom

ARM default whitelist headers #10940

Merged
merged 6 commits into from
Apr 28, 2020
Merged

ARM default whitelist headers #10940

merged 6 commits into from
Apr 28, 2020

Conversation

lmazuel
Copy link
Member

@lmazuel lmazuel commented Apr 20, 2020

No description provided.

@lmazuel lmazuel added Mgmt This issue is related to a management-plane library. Azure.Mgmt.Core Azure management core labels Apr 20, 2020
@lmazuel lmazuel added this to the [2020] May milestone Apr 20, 2020
@lmazuel lmazuel requested review from jiasli and RodgeFu April 20, 2020 22:01
@lmazuel lmazuel requested a review from zikalino as a code owner April 20, 2020 22:01
@jiasli jiasli requested a review from jsntcy April 21, 2020 03:18
jiasli
jiasli previously approved these changes Apr 21, 2020
View reviewed changes
@lmazuel @jiasli
Update sdk/core/azure-mgmt-core/CHANGELOG.md
b543593 
Co-Authored-By: Jiashuo Li <jiasli@microsoft.com>
johanste
johanste reviewed Apr 21, 2020
View reviewed changes
sdk/core/azure-mgmt-core/azure/mgmt/core/policies/__init__.py
"x-ms-ratelimit-remaining-subscription-resource-requests",
"x-ms-ratelimit-remaining-subscription-resource-entities-read",
"x-ms-ratelimit-remaining-tenant-resource-requests",
"x-ms-ratelimit-remaining-tenant-resource-entities-read",
Copy link
Member

@johanste johanste Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Compute has a additional header(s) as well.

For example, I don't see x-ms-ratelimit-remaining-resource

Copy link
Member Author

Choose a reason for hiding this comment

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

I used https://docs.microsoft.com/en-us/azure/azure-resource-manager/management/request-limits-and-throttling#remaining-requests is there anything else I should follow?

Copy link
Member

Choose a reason for hiding this comment

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

There is also https://docs.microsoft.com/en-us/azure/virtual-machines/troubleshooting/troubleshooting-throttling-errors#call-rate-informational-response-headers

Copy link
Member Author

Choose a reason for hiding this comment

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

done, anything else?

Copy link
Member

@jsntcy jsntcy Apr 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Why do we add headers for compute here, but not in compute SDK as they are specific to compute, but not for all ARM resources? @johan.stenberg@microsoft.com

In reply to: 412493196 [](ancestors = 412493196)

Copy link
Member

Choose a reason for hiding this comment

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

I'd be fine with adding the compute specific header in the compute library. Adding it to the azure-mgmt-core is significantly less broad than adding it to azure-core, however - the probability that another resource provide would add the header and that it would be sensitive is fast approaching nil. Add to this the fact that the compute library is autogenerated, and thus would need a way to inject the additional policies, it may cost more than what value we get out of moving the header into the compute specific package.

Copy link
Member

Choose a reason for hiding this comment

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

Your points make sense to me.

In reply to: 413154247 [](ancestors = 413154247)

johanste
johanste reviewed Apr 22, 2020
View reviewed changes
sdk/core/azure-mgmt-core/azure/mgmt/core/policies/__init__.py
"""HttpLoggingPolicy with ARM specific safe headers fopr loggers.
"""

DEFAULT_HEADERS_WHITELIST = HttpLoggingPolicy.DEFAULT_HEADERS_WHITELIST | set([
Copy link
Member

Choose a reason for hiding this comment

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

Let's avoid using the term WHITELIST. Call it SAFELIST or something.

Copy link
Member

Choose a reason for hiding this comment

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

I think this may cause a breaking change as HttpLoggingPolicy is already using this term:

azure-sdk-for-python/sdk/core/azure-core/azure/core/pipeline/policies/_universal.py

Line 342 in d9d5312

DEFAULT_HEADERS_WHITELIST = set([

jsntcy
jsntcy approved these changes Apr 23, 2020
View reviewed changes
Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

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

:shipit:

@lmazuel lmazuel merged commit 5d2b7b0 into master Apr 28, 2020
@lmazuel lmazuel deleted the arm_headers_logger branch April 28, 2020 22:12
xiangyan99 added a commit that referenced this pull request May 1, 2020
@xiangyan99 @xiafu-msft
@lmazuel @kristapratico @azure-sdk @iscai-msft @scbedd @AutorestCI @KieranBrantnerMagee @bryevdv @rakshith91 @yunhaoling @chlowell @praveenkuttappan @jiasli @YijunXieMS @danieljurek @changlong-liu @chenjianfei2017 @johanste
get master (#11193)
a082987 
* [DataLake][Bug]Upload is not working with umask and permissions (#10845)

* Add force generation to SwaggerToSdk (#10933)

* [formrecognizer] edits to docstrings (#11003)

* edits to docstrings

* correct date

* updating setup template (#11022)

* [formrecognizer] handle unsupervised pages better with service bug (#11017)

* handle unsupervised pages better

* python 2 oops

* Increment package version after release of azure_ai_formrecognizer (#11026)

* add regular endpoint in new env variable (#11031)

* Update Ubuntu VM Image to 18.04 (#11032)

* updating the VM image, need to update the recording as well.

* update release

* fixing merge error (#11039)

* Fixing compute test (#11036)

* trigger test

* Packaging update of azure-mgmt-compute

* fix test

* fix

* fix duplicated comment

Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com>

* Servicebus - Track2 - Remove timeout from Send (#11002)

* With retry options available, send should no longer require its own timeout.  Removes the parameter from sync and async clients, adds a note to changelog about the delta.

* rename SearchIndexClient -> SearchClient (#10964)

* Implementation for Datasources operations (#11012)

* Initial Commit

* lint + mypy

* tests

* recordings update

* Apply suggestions from code review

* typo

* remove datasource

* lint

* rename to get_datasources

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

* [ServiceBus] Update for readme and sample (#11047)

* tweak sample code

* update according to comment

* add aad sample in readme

* Update sdk/servicebus/azure-servicebus/README.md

Co-Authored-By: KieranBrantnerMagee <kibrantn@microsoft.com>

Co-authored-by: KieranBrantnerMagee <kibrantn@microsoft.com>

* Fix pip link in azure-keyvault-secrets readme (#11056)

* fixed alternative document input samples (#11078)

* Add sync/async samples to demonstrate consuming from a number of sessions at one time. (#11001)

* Add sync/async samples to demonstrate consuming from a number of sessions at one time.

* Add informational message to session pool samples regarding the exit condition and how it manifests.

* [ServiceBus] Settle non-deferred message through receiver link (#10800)

* settle non-deferred message through receiver link except dead_letter

* revert dead-letter back to t1 as well

* improve settlement and put is_deferred_letter into kwargs

* add test

* update according to comment

* fix a bug in dead_letter through receiver_link

* Search README issues (#11082)

* fix for azure.search.documents change

* indentation, naming

* SearchIndexClient -> SearchClient

* Update README.md (#11084)

* [ServiceBus] Remove exception from __init__.py (#11080)

* remove exception from __init__.py

* update changelog, fix some docstring and sample error

* Pin astroid to 2.3.3 to fix pylint failure (#11088)

* Lint error in cosmos (#11092)

* Fix TYPING cycles if TYPE_CHECKING manually forced (#10799)

* Improve mypy typing for azure core (#10653)

* first commit

* more changes

* few changes

* lint

* comments

* more changes

* fix test

* lint

* mypy

* comments

* changes

* async polling method

* async

* Accept authority option with or without scheme (#11050)

* Split Search Service Client (#11090)

* split datasources client

* split skillsets client

* split synonym maps client

* split indexes client

* cleanup

* [text analytics] Add how to get json response to sample (#11102)

* ARM default whitelist headers (#10940)

* ARM default whitelist headers

* ChangeLog

* Typo

* Syntax error

* Update sdk/core/azure-mgmt-core/CHANGELOG.md

Co-Authored-By: Jiashuo Li <jiasli@microsoft.com>

* More headers

Co-authored-by: Jiashuo Li <jiasli@microsoft.com>

* update version (#11106)

* Return pageable for Search list_indexes (#11125)

* Adjust README with comments from the per-release doc review. (#11110)

* Adjust README with comments from the per-release doc review.
* Make additional breadcrumb to samples higher in the page, duplication is OKd (and even recommended) in this case.
* Removed spurious FIFO reference.
* Added a note about preview vs. existing docs, with breadcrumb to existing libs.
* Make 0.50 doc links more specific.

* [Event Hubs] add enqueueTime to Process span links (#10932)

* remove formatter in samples/readme.md to prevent preview samples being published (#10884)

* [DataLake]Update ChangeLog (#11133)

* Add capability to send multiple events in one Send() call. (#11093)

* Add the ability for send to take a list of messages, failing if they don't fit in a single batch to maintain idempotency.
* Adds tests for multisend.
* Puts details of multisend into the changelog.
* Adjust docstring type definition for send() async.
* Make test more targeted (256KB) for too-large validation.
* Create batch helper to create from list.
* Adjust docstrings to point to proper exception location instead of under common.
* Fix failing unit test expecting the old send error type.
* Remove test segments that will likely never be readded (queue message) and make docstring type inputs for send() more precise.

* Set time out to 90 mins for regression test (#11105)

* Autorest check to run tests in autorestv3 branch for core (#11131)

* Autorest CI changes to use autorestv3

* Add DeleteAfter tag to Search test resource groups (#11136)

* add DeleteAfter tag to RG

* update recordings

* Increment version for storage releases (#11138)

* Increment package version after release of azure_storage_blob

* Increment package version after release of azure_storage_file_datalake

* [Event Hubs] `EventHubProducerClient.send_batch` accepts a list of EventData (#11079)

* add ServiceDirectory to usage of remove-test-resources.yml (#11117)

* resume subscriptions test (#11111)

* resume subscriptions test

* recording subscriptions

* Adding test for signalr (#11146)

* generated signalr test

* generated recording

* Packaging update of azure-mgmt-signalr

Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com>

* update live tests yml + add form (#11139)

* [Service Bus] Exception Handling review (#11060)

* [form recognizer] add repr (#11150)

* [ServiceBus] Support for scheduling and cancellation  (#11095)

* make schedule a property on the message

* make send api public

* schedule and cancellation

* remove iterable type hint and docstring

* update implementation

* update comment

* update docs

* Update sdk/servicebus/azure-servicebus/CHANGELOG.md

Co-authored-by: KieranBrantnerMagee <kibrantn@microsoft.com>

Co-authored-by: KieranBrantnerMagee <kibrantn@microsoft.com>

* identity_vscode_credential (#10840)

* identity_win_vscode_credential

Co-authored-by: Charles Lowell <chlowe@microsoft.com>

* Sync eng/common directory with azure-sdk-tools repository (#11007)

* ChangeLog generics for autorest v5 (#10885)

* ChangeLog generics for autorest v5

* Update changelog_generics.md

* [Event Hubs] Add params in stress test command line for receive batch and send list (#11161)

* Storageache cjf (#11122)

* Update from master

* release azure-mgmt-storagecache

* Update CHANGELOG.md

* Packaging update of azure-mgmt-storagecache

Co-authored-by: SDK Automation <sdkautomation@microsoft.com>
Co-authored-by: Your Name <you@example.com>
Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com>

* added AzureKeyCredential link to readme (#11089)

* Fix core auth (#11177)

* [Azure-Core]Auth Header missing when token credential is not expired

* Fix mypy

* Test we put the header even if we didn't tech a new token

Co-authored-by: xiafu <xiafu@microsoft.com>

* [formrecognizer] increase test coverage (#11096)

* working on more tests

* add multipage tests for custom form and training

* adding multipage tests

* fix

* add tests for bad input into custom analyze

* remove test dependency on storage

* update tests.yml

* fix for custom analyze url unlabeled tests

* update changelog

* review feedback

* pushing a commit to reset CI

* Release edits for Search (#11178)

* fix README typo

* Getting started section order

* add first search request subsection

* update CHANGELOG

* update readme with note about service version support (#11180)

* [Event Hubs] Fix a bug that sets owner level 0 (#11172)

* [ServiceBus] Docstring and Changelog adjustment (#11166)

* Identity use pbyte (#11173)

* identity_vscode_cred_format

* EnvironmentCredential correctly initializes UsernamePasswordCredential (#11127)

* User authentication API for applications (#10612)

* [Event Hubs] Version/Docs adjustment for 5.1.0 (#11179)

* [form recognizer] Add some stuff that .net has to our samples (#11187)

* re-enable schedule tests in session (#11184)

* add helpers for search fields (#11164)

* add helpers for search fields

* pylint

* fix annotation

* Define match conditions for CreateOrUpdate and Delete operations (#11116)

* sync

* async

* lint

* Update sdk/search/azure-search-documents/azure/search/documents/_service/_datasources_client.py

* tests fix

* use match conditiond

* error map

* fix test

* lint

* comments

* test_utils

* Update sdk/search/azure-search-documents/azure/search/documents/_service/_utils.py

* Update sdk/search/azure-search-documents/azure/search/documents/_service/_utils.py

Co-authored-by: Johan Stenberg (MSFT) <johan.stenberg@microsoft.com>

* Update sdk/search/azure-search-documents/azure/search/documents/_service/_utils.py

* Revert "Update sdk/search/azure-search-documents/azure/search/documents/_service/_utils.py"

This reverts commit b0ca117.

* fix

* more changes

Co-authored-by: Johan Stenberg (MSFT) <johan.stenberg@microsoft.com>

Co-authored-by: Xiaoxi Fu <49707495+xiafu-msft@users.noreply.github.com>
Co-authored-by: Laurent Mazuel <laurent.mazuel@gmail.com>
Co-authored-by: Krista Pratico <krpratic@microsoft.com>
Co-authored-by: Zim Kalinowski <zikalino@microsoft.com>
Co-authored-by: Azure SDK Bot <53356347+azure-sdk@users.noreply.github.com>
Co-authored-by: iscai-msft <43154838+iscai-msft@users.noreply.github.com>
Co-authored-by: Scott Beddall <45376673+scbedd@users.noreply.github.com>
Co-authored-by: Azure SDK Bot <aspysdk2@microsoft.com>
Co-authored-by: KieranBrantnerMagee <kibrantn@microsoft.com>
Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Co-authored-by: Rakshith Bhyravabhotla <rakshith.bhyravabhotla@gmail.com>
Co-authored-by: Adam Ling (MSFT) <47871814+yunhaoling@users.noreply.github.com>
Co-authored-by: Charles Lowell <chlowe@microsoft.com>
Co-authored-by: praveenkuttappan <55455725+praveenkuttappan@users.noreply.github.com>
Co-authored-by: Rakshith Bhyravabhotla <sabhyrav@microsoft.com>
Co-authored-by: Jiashuo Li <jiasli@microsoft.com>
Co-authored-by: Yijun Xie <48257664+YijunXieMS@users.noreply.github.com>
Co-authored-by: Daniel Jurek <djurek@microsoft.com>
Co-authored-by: changlong-liu <59815250+changlong-liu@users.noreply.github.com>
Co-authored-by: 陈箭飞 <32889038+chenjianfei2017@users.noreply.github.com>
Co-authored-by: SDK Automation <sdkautomation@microsoft.com>
Co-authored-by: Your Name <you@example.com>
Co-authored-by: xiafu <xiafu@microsoft.com>
Co-authored-by: Johan Stenberg (MSFT) <johan.stenberg@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johanste johanste johanste left review comments

@jiasli jiasli jiasli approved these changes

@jsntcy jsntcy jsntcy approved these changes

@RodgeFu RodgeFu Awaiting requested review from RodgeFu

@zikalino zikalino Awaiting requested review from zikalino

Assignees
No one assigned
Labels
Azure.Mgmt.Core Azure management core Mgmt This issue is related to a management-plane library.
Projects
None yet
Milestone
[2020] May
Development

Successfully merging this pull request may close these issues.
4 participants
@lmazuel @jiasli @johanste @jsntcy