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

[Tables] Expose client option allowInsecureConnection #15938

Merged
4 commits merged into from
Jun 24, 2021
Merged

[Tables] Expose client option allowInsecureConnection #15938

4 commits merged into from
Jun 24, 2021

Conversation

joheredi
Copy link
Member

In order for Tables to connect to Azurite and Storage emulator, the client needs to accept allowInsecureConnection. Also when using the emulator connection string shortcut, setting it by default.

@joheredi joheredi requested review from xirzec and ellismg June 24, 2021 00:18
@ghost ghost added the Tables label Jun 24, 2021
@ellismg
Copy link
Member

ellismg commented Jun 24, 2021

I am guessing that this is not exposed from CommonClientOptions because in general we want to make a decision about if this is "allowed" on a service by service basis?

@joheredi
Copy link
Member Author

@ellismg I just talked to @xirzec offline and we decided to move it to CommonClientOptions. This is going to be more common once more libraries migrate to CoreV2, storage libraries would run into this exact same issue

@joheredi
Copy link
Member Author

Fixes #15854

@joheredi joheredi mentioned this pull request Jun 24, 2021
Closed
6 tasks
@ellismg
Copy link
Member

ellismg commented Jun 24, 2021

This is going to be more common once more libraries migrate to CoreV2, storage libraries would run into this exact same issue

Sounds great, this will also be helpful for EventGrid, which is on CoreV2.

@ellismg
Copy link
Member

ellismg commented Jun 24, 2021

Should we add something to the changelog for core-client or do we normally do a rollup before release by looking at the commit history since the last release?

ellismg
ellismg approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

LGTM

xirzec
xirzec approved these changes Jun 24, 2021
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Looks great!

@ghost
Copy link

ghost commented Jun 24, 2021

Hello @joheredi!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

deyaaeldeen
deyaaeldeen reviewed Jun 24, 2021
View reviewed changes
sdk/core/core-client/CHANGELOG.md

### Features Added

- Moved `allowInsecureConnection` from `ServiceClientOptions` to `CommonClientOptions` [issue 15938](https://github.com/azure/azure-sdk-for-js/issues/15938)
Copy link
Member

Choose a reason for hiding this comment

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

This is a PR not an issue:

Suggested change
- Moved `allowInsecureConnection` from `ServiceClientOptions` to `CommonClientOptions` [issue 15938](https://github.com/azure/azure-sdk-for-js/issues/15938)
- Moved `allowInsecureConnection` from `ServiceClientOptions` to `CommonClientOptions` [PR 15938](https://github.com/Azure/azure-sdk-for-js/pull/15938)

@ghost ghost merged commit cbaa410 into Azure:main Jun 24, 2021
ellismg added a commit to ellismg/azure-sdk-for-js that referenced this pull request Jul 27, 2021
@ellismg
[Tables] Fix batch operations against emulators
ebc1962 
In Azure#15938 we exposed the `allowInsecureConnection` option to allow
connections to the service over HTTP instead of HTTPS. However, even
if a table client was created allowing insecure connections, we would
still require a secure connection when submitting a batch transaction,
since the implementation of batch operations interact with the
pipeline directly. We now record if the client was created allowing
insecure connections and thread that information along when creating
requests for batch operations.

With this change, you can now submit batch transactions to storage
emulators, which commonly run over HTTP instead of HTTPS.

Fixes Azure#15854
ellismg added a commit to ellismg/azure-sdk-for-js that referenced this pull request Jul 27, 2021
@ellismg
[Tables] Fix batch operations against emulators
9817211 
In Azure#15938 we exposed the `allowInsecureConnection` option to allow
connections to the service over HTTP instead of HTTPS. However, even
if a table client was created allowing insecure connections, we would
still require a secure connection when submitting a batch transaction,
since the implementation of batch operations interact with the
pipeline directly. We now record if the client was created allowing
insecure connections and thread that information along when creating
requests for batch operations.

With this change, you can now submit batch transactions to storage
emulators, which commonly run over HTTP instead of HTTPS.

Fixes Azure#15854
@ellismg ellismg mentioned this pull request Jul 27, 2021
Merged
ellismg added a commit to ellismg/azure-sdk-for-js that referenced this pull request Jul 27, 2021
@ellismg
[Tables] Fix batch operations against emulators
5b308ea 
In Azure#15938 we exposed the `allowInsecureConnection` option to allow
connections to the service over HTTP instead of HTTPS. However, even
if a table client was created allowing insecure connections, we would
still require a secure connection when submitting a batch transaction,
since the implementation of batch operations interact with the
pipeline directly. We now record if the client was created allowing
insecure connections and thread that information along when creating
requests for batch operations.

With this change, you can now submit batch transactions to storage
emulators, which commonly run over HTTP instead of HTTPS.

Fixes Azure#15854
ellismg added a commit that referenced this pull request Jul 27, 2021
@ellismg
[Tables] Fix batch operations against emulators (#16587)
c230caa 
In #15938 we exposed the `allowInsecureConnection` option to allow
connections to the service over HTTP instead of HTTPS. However, even
if a table client was created allowing insecure connections, we would
still require a secure connection when submitting a batch transaction,
since the implementation of batch operations interact with the
pipeline directly. We now record if the client was created allowing
insecure connections and thread that information along when creating
requests for batch operations.

With this change, you can now submit batch transactions to storage
emulators, which commonly run over HTTP instead of HTTPS.

Fixes #15854
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Sep 9, 2021
@levimm @charlili1234
@haitch @nilo19 @asilverman @alexeldeib @matthchr
CodeGen from PR 15954 in Azure/azure-rest-api-specs
520ab8f 
Merge Dev-containerservice-microsoft.containerservice-2021-08-01 branch to main (Azure#15954)

* Adds base for updating Microsoft.ContainerService from version stable/2021-07-01 to version 2021-08-01

* Updates readme

* Updates API version in new specs and examples

* add publicNetworkAccess property per network platform's request (Azure#15489)

* add publicNetworkAccess per network platform's request

* fix quota

Co-authored-by: Li Ma <[email protected]>

* update readme for 2021-08-01 sdk generation (Azure#15476)

* update readme for sdk generation

* update readme for sdk generation

Co-authored-by: Charlie Li <[email protected]>

* allow disabling of runcommand (Azure#15481)

* allow disabling of runcommand

* rename file

* another rename

* fix prittier check

* fix stupid prettier check

* change publicNetworkAccess to enum (Azure#15564)

Co-authored-by: Li Ma <[email protected]>

* Add CreationData property to Agentpool level in 2021-08-01 API (Azure#15563)

* Add CreationData property to Agentpool level in 2021-08-01 API

* fix json format

* fix swagger spell check

Co-authored-by: Charlie Li <[email protected]>

* chore: add enableMultipleStandardLoadBalancers to loadBalancerProfile (Azure#15579)

* Add snapshot related new APIs and properties to AKS 2021-08-01 swagger (Azure#15586)

* Add CreationData property to Agentpool level in 2021-08-01 API

* Add snapshot related APIs and properties to AKS 2021-08-01 swagger

* fix lint and spell checks

* fix lint and spell checks

* fix PrettierCheck

* Change some Nodepool to NodePool

* some changes according to ARM team's review comments

Co-authored-by: Charlie Li <[email protected]>

* fix tag typo to match tag convention (Azure#15683)

* add workload runtime to agent pool api (Azure#15726)

* add workload runtime to agent pool api

* reference example

* add custom words

* fix: workload runtime description (Azure#15782)

* fix: workload runtime description

* add wasmtime to custom words

* clarify single workload type per node

Co-authored-by: Matthew Christopher <[email protected]>

Co-authored-by: Matthew Christopher <[email protected]>

* fix typo in readme.python.md (Azure#15903)

* Add CreationData property to Agentpool level in 2021-08-01 API

* fix typos in readme.python.md

Co-authored-by: Charlie Li <[email protected]>

* merge recent custom-words.txt changes from main branch to resolve conflicts (Azure#15938)

* Add CreationData property to Agentpool level in 2021-08-01 API

* merge recent custom-words.txt changes from main branch to resolve conflicts

Co-authored-by: Charlie Li <[email protected]>

* pull custom-words.txt from main

* add a new word - NodePool

* add two more words

Co-authored-by: Super <[email protected]>
Co-authored-by: Li Ma <[email protected]>
Co-authored-by: Charlie Li <[email protected]>
Co-authored-by: Haitao Chen <[email protected]>
Co-authored-by: Qi Ni <[email protected]>
Co-authored-by: Ariel Silverman <[email protected]>
Co-authored-by: Ace Eldeib <[email protected]>
Co-authored-by: Matthew Christopher <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deyaaeldeen deyaaeldeen deyaaeldeen left review comments

@ellismg ellismg ellismg approved these changes

@xirzec xirzec xirzec approved these changes

@bterlson bterlson Awaiting requested review from bterlson bterlson is a code owner

@chradek chradek Awaiting requested review from chradek

@jeremymeng jeremymeng Awaiting requested review from jeremymeng

Assignees
No one assigned
Labels
Tables
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joheredi @ellismg @xirzec @deyaaeldeen