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

Add/Edit Outbound Rule on Load Balancer and Examples #3592

Merged

Conversation

khannarhea
Copy link
Contributor

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Aug 7, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Aug 7, 2018

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#3087

@AutorestCI
Copy link

AutorestCI commented Aug 7, 2018

Automation for azure-sdk-for-node

Encountered a Subprocess error: (azure-sdk-for-node)

Command: ['/usr/local/bin/autorest', '/tmp/tmpkm8r8too/rest/specification/network/resource-manager/readme.md', '--license-header=MICROSOFT_MIT_NO_VERSION', '--node-sdks-folder=/tmp/tmpkm8r8too/sdk', '--nodejs', '[email protected]/autorest.nodejs@^2.1.43']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.11.3]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4283)
   Loading AutoRest extension '@microsoft.azure/autorest.nodejs' (^2.1.43->2.1.79)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.44->2.3.44)
FATAL: nodejs/generate - FAILED
FATAL: Error: AutoRest extension '@microsoft.azure/autorest.nodejs' reported error: Error: This socket is closed,[object Object],1
Process() cancelled due to exception : AutoRest extension '@microsoft.azure/autorest.nodejs' reported error: Error: This socket is closed,[object Object],1

@AutorestCI
Copy link

AutorestCI commented Aug 7, 2018

Automation for azure-sdk-for-go

Encountered a Subprocess error: (azure-sdk-for-go)

Command: ['/usr/local/bin/autorest', '/tmp/tmp54_qexl0/rest/specification/network/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmp54_qexl0/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '[email protected]/autorest.go@~2.1.110', '--use-onever', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4283; node: v8.11.3]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4283)
   Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.110->2.1.110)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2018-06"} .
Processing batch task - {"tag":"package-2018-04"} .
Shutting Down
FATAL: go/generate - FAILED
FATAL: Error: [Exception] AutoRest extension '@microsoft.azure/autorest.go' terminated.
Process() cancelled due to exception : [Exception] AutoRest extension '@microsoft.azure/autorest.go' terminated.
Failure during batch task - {"tag":"package-2018-04"} -- false.

@AutorestCI
Copy link

AutorestCI commented Aug 7, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@sergey-shandar sergey-shandar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 7, 2018
@sergey-shandar
Copy link
Contributor

@ravbhatnagar potential SDK/service breaking changes

Copy link

@xieus xieus left a comment

Choose a reason for hiding this comment

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

Do we have remove outboundNatRules from all NRP including national clouds?

Copy link

@xieus xieus left a comment

Choose a reason for hiding this comment

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

We need to add TCP RST to outbound rules. Also check idle timeout to see if the property is there already.

Copy link

@xieus xieus left a comment

Choose a reason for hiding this comment

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

As public ip prefix change is in, do we need to refer to public ip prefix somewhere for outbound rules?

@sergey-shandar
Copy link
Contributor

@ravbhatnagar @MikhailTryakhov could you review the breaking changes?

@sergey-shandar sergey-shandar removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required potential-sdk-breaking-change labels Aug 14, 2018
@sergey-shandar
Copy link
Contributor

It's not "breaking changes" because these properties are new..

@sergey-shandar
Copy link
Contributor

@khannarhea please, address @xieus comments/questions.

@khannarhea
Copy link
Contributor Author

@xieus - updated protocol, idletimeoutinminutes and enabletcpreset properties.

@khannarhea
Copy link
Contributor Author

@sergey-shandar - this could potentially be breaking for Basic Load Balancer SKU.

@christiankuhtz
Copy link

Basic SKU is frozen. These are all Standard SKU only. Why would this break?

@christiankuhtz
Copy link

@yugangw-msft fyi

@@ -1509,12 +1526,12 @@
},
"description": "Defines an external port range for inbound NAT to a single backend port on NICs associated with a load balancer. Inbound NAT rules are created automatically for each NIC associated with the Load Balancer using an external port from this range. Defining an Inbound NAT pool on your Load Balancer is mutually exclusive with defining inbound Nat rules. Inbound NAT pools are referenced from virtual machine scale sets. NICs that are associated with individual virtual machines cannot reference an inbound NAT pool. They have to reference individual inbound NAT rules."
},
"outboundNatRules": {
"outboundRules": {
Copy link
Contributor

@yugangw-msft yugangw-msft Aug 15, 2018

Choose a reason for hiding this comment

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

Any reason why we want to change the type name here? This will force client applications to update their code from new OutboundNatRule() to new OutboundRule() when migrate from old api-version to this new one. Is this necessary? Or the old name is just wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is name change for new feature which is approved by dev and pm team

Copy link
Contributor

Choose a reason for hiding this comment

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

So , functionality wise, I assume the outbound rules here will be a super set of the old NAT rules?
CC @tjprescott

Choose a reason for hiding this comment

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

@yugangw-msft NAT rules in load balancer context implies port forwarding (inbound NAT rule). These rules specify outbound translations and have nothing to do with port forwarding. To avoid this confusion, we opted for removing Nat from the rule name and not reuse what was begun 2+ years ago and never finished and never released.

Yes, OutboundRules are similar in shape, but different in how they are modeled.

OutboundNatRules were never available to anyone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification. This is a zero impact breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

They were never commands in the CLI.

@khannarhea
Copy link
Contributor Author

khannarhea commented Aug 15, 2018

@xieus @anilingle-ms I have added public ip prefix reference - please take a look if it looks good

@anilingle-ms
Copy link

anilingle-ms commented Aug 16, 2018 via email

Copy link
Contributor

@MikhailTryakhov MikhailTryakhov left a comment

Choose a reason for hiding this comment

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

This functionality didn't work any time in SDKs and wasn't implemented in PS or CLI
so we can merge it bumping SDK versions.

@sergey-shandar sergey-shandar merged commit 2ca2757 into Azure:Network-August-Release Aug 16, 2018
jianghaolu pushed a commit that referenced this pull request Aug 17, 2018
* Created folder for 2018-07-01 api version (#3509)

* Created folder for 2018-07-01 api version

* fixed incorrect encoding symbol

* Fix lint diff and improve output on failure (#3528)

Lint diff was failing on PRs with a base branch other than master. We
needed to fetch the base branch ref from the origin before trying to
resolve it to its commit hash.

* Python conf for Network 2018-06 (#3535)

* Py conf for Network 2018-07-01 (#3539)

* Introduce Publicipprefix to swagger (#3520)

* Add PublicIpPrefix

* Add PublicIpPrefix

* Add PublicIpPrefix

* Move to 2018-07-01 branch

* revertversionchange

* Fix Syntax error

* Fix IpTag description because evidently everything is global

* Fix $ref for PublicIpPrefix

* Fix Casing

* Add Exception for PublicIPPrefix from RequiredPropertiesMissingInResourceModel

* Fix Version#

* Fix Examples

* Publicipprefixaugust (#3573)

* Add PublicIpPrefix

* Add PublicIpPrefix

* Add PublicIpPrefix

* Move to 2018-07-01 branch

* revertversionchange

* Fix Syntax error

* Fix IpTag description because evidently everything is global

* Fix $ref for PublicIpPrefix

* Fix Casing

* Add Exception for PublicIPPrefix from RequiredPropertiesMissingInResourceModel

* Fix Version#

* Fix Examples

* Remove IdleTimeout parameter

* Fix comma

* Service Endpoint Policies Swagger (#3558)

* Service Endpoint Policies Swagger

* Fix review comments

* Updated Network Usage location parameter pattern (#3583)

* Add enableTcpReset property to load balancer (#3545)

* Add enableTcpReset property to load balancer.

* Update loadbalancer examples with enableTcpReset.

* added global reach flag (#3542)

* fix query packet capture status in latest version (#3601)

* Publicipprefixaugust (#3618)

* Add PublicIpPrefix

* Add PublicIpPrefix

* Add PublicIpPrefix

* Move to 2018-07-01 branch

* revertversionchange

* Fix Syntax error

* Fix IpTag description because evidently everything is global

* Fix $ref for PublicIpPrefix

* Fix Casing

* Add Exception for PublicIPPrefix from RequiredPropertiesMissingInResourceModel

* Fix Version#

* Fix Examples

* Remove IdleTimeout parameter

* Fix comma

* Add PublicIpPrefix for PublicIpAddressConfiguration

* Fix PublicIPAddress to refer to PublicIpPrefix as SubResource

* Fix Comma

* Remove compute update

* Publicipprefixaugust (#3631)

* Add PublicIpPrefix

* Add PublicIpPrefix

* Add PublicIpPrefix

* Move to 2018-07-01 branch

* revertversionchange

* Fix Syntax error

* Fix IpTag description because evidently everything is global

* Fix $ref for PublicIpPrefix

* Fix Casing

* Add Exception for PublicIPPrefix from RequiredPropertiesMissingInResourceModel

* Fix Version#

* Fix Examples

* Remove IdleTimeout parameter

* Fix comma

* Add PublicIpPrefix for PublicIpAddressConfiguration

* Fix PublicIPAddress to refer to PublicIpPrefix as SubResource

* Fix Comma

* Remove compute update

* Add PublicIpPrefix Subresource to VirtualMachineScaleSetPublicIPAddressConfigurationProperties

* Changes to Support MSEEv2 Datapath for ExpressRoute. (#3647)

Description of Changes:
    - Added a flag expressRouteGatewayBypass to the Connection
      Properties.
    - The user can set the flag to True to use MSEEv2 datapath

Related work items: #2628003

* Added new API to networkwatcher.json and examples (#3650)

* Added new API to networkwatcher.json and examples

* Added NetworkWatcherNetworkConfigurationDiagnostic example

* Fix name from ServiceEndpointDefinition to ServiceEndpointDefinitions while doing a put, get and delete (#3655)

* Add/Edit Outbound Rule on Load Balancer and Examples (#3592)

* Add/Edit Outbound Rule on Load Balancer and Examples

* Adding properties enabletcpreset, idletimeoutinminutes and protocol

* Adding a reference to Public IP Prefix

* Dummy Commit

* Removed QueryConnectionMOnitors API (#3661)

* Added new API to networkwatcher.json and examples

* Added NetworkWatcherNetworkConfigurationDiagnostic example

* removed QueryConnectionMonitors Api
Copy link
Contributor

@gmainar gmainar left a comment

Choose a reason for hiding this comment

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

Looks good

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.