#WAF Add extended WAF functionality to Application Gateway by jobatzil · Pull Request #1052 · Azure/azure-rest-api-specs

jobatzil · 2017-03-21T02:23:49Z

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

…les.

…03-01.

… file.

…s API call.

…rules.

msftclas · 2017-03-21T02:23:53Z

@jobatzil,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

sergey-shandar

@jobatzil As I can see. This PR may contain some breaking changes and CI still reports some errors/warnings which are not related to the changes. Do you have a link to previous PR where the errors/warnings have been discussed? https://travis-ci.org/Azure/azure-rest-api-specs/jobs/213495817

sergey-shandar · 2017-03-21T21:29:37Z

      "required": [
-        "enabled"
+        "enabled",
+        "firewallMode",


It's a breaking change.

Yes, it's adding some new functionality to the web application firewall.
We introduce it for the new Api-Version 2017-03-01 and no older ones though.

you have to release the sdk on a major version.

It looks like no SDK was published with this api-version yet (please, confirm). So it should be ok to publish. Of course, we still have to address the warnings/errors.

The azure-sdk-for-net PR is currently still open since we have to coordinate multiple Network related PRs. I will changed the Version to a new major version as @DeepakRajendranMsft requested.

@sergey-shandar Yes, no SDK with api-version 2017-03-01 was published yet

jobatzil · 2017-03-21T23:25:50Z

@sergey-shandar We will address the CI warnings later, but don't have time rn since we want to get into the upcoming release of PowerShell.

DeepakRajendranMsft · 2017-03-21T23:24:02Z

        "health": {
          "type": "string",
-          "description": "Health of backend server. Possible values are: 'Unknown', 'Up', 'Down', and 'Partial'.",
+          "description": "Health of backend server.",


why are you changing the description

i believe the documentation team did this

It get's automatically added to the auto-generated code. Otherwise the possible enum values would be twice in a row in the documentation of the property.

DeepakRajendranMsft · 2017-03-21T23:26:29Z

      "required": [
-        "enabled"
+        "enabled",
+        "firewallMode",


you have to release the sdk on a major version.

DeepakRajendranMsft · 2017-03-21T23:39:46Z

+        },
+        "ruleSetType": {
+          "type": "string",
+          "description": "The type of the web application firewall rule set. Possible values are: 'OWASP'."


why not an enum?

seems like the service is also not an enum

jobatzil · 2017-03-21T23:46:51Z

I will change the Version number in the azure-sdk-for-net to 10.0.0.0

sergey-shandar · 2017-03-22T01:12:34Z

@jobatzil @salameer @veronicagg

Ok, here's a list of errors/warnings which we need to discuss

SDKViolation:
- NonAppJsonTypeWarning, Media types other than 'application/json' has limited support, 2 warnings: 14, 18.
- ListOperationNamingWarning, Since operation '_' response has model definition 'array', it should be named as "list_*"", 2 errors: 320, 370
RPCViolation:
- OperationsAPIImplementationValidation, Operations API must be implemented for the service. 1 error.
- BooleanPropertyNotRecommended, Booleans are not descriptive and make them hard to use. Instead use string enums with allowed set of values defined. 3 warnings: 893, 1296, 1338
- TrackedResourceValidation,
  - A Tracked Resource "ApplicationGateway" must have
    - a ListByResourceGroup operation with x-ms-pageable extension.
    - a ListBySubscriptionId operation with x-ms-pageable extension.
- TrackedResourcePatchOperationValidation, Tracked resource 'ApplicationGateway' must have patch operation that at least supports the update of tags 1214
- DefinitionsPropertiesNamesCamelCase, 636, 640, 656, 740, 865, 1122, 1143, 740

Some other warnings and errors are derived from the network.json swagger file.

jobatzil · 2017-03-22T21:52:46Z

@sergey-shandar I can change the warnings that are related to the code I'm adding, but would prefer to leave the other warnings for this PR since some of them may introduce further breaking changes.

jobatzil · 2017-03-25T00:46:04Z

SDKViolation:

NonAppJsonTypeWarning, Media types other than 'application/json' has limited support, 2 warnings: 14, 18.

Removed the Text/Json support.

ListOperationNamingWarning, Since operation '' response has model definition 'array', it should be named as "list*"", 2 errors: 320, 370

Updated operationId of getAvailableWafRuleSets.
For the next bigger PR: will probably duplicate operation backendHealth with updated operationId and keep the old one as lecacy.

RPCViolation:

OperationsAPIImplementationValidation, Operations API must be implemented for the service. 1 error.

Will talk to Network team since it might be better to have one shared API call for that.

BooleanPropertyNotRecommended, Booleans are not descriptive and make them hard to use. Instead use string enums with allowed set of values defined. 3 warnings: 893, 1296, 1338

Keep existing Booleans since it would need bigger changes in the backend.
Will be taken into account in future design decisions.

TrackedResourceValidation,
A Tracked Resource "ApplicationGateway" must have
a ListByResourceGroup operation with x-ms-pageable extension.
a ListBySubscriptionId operation with x-ms-pageable extension.

Operations exist already but have different operationIds.
For next bigger PR: Duplicate existing operations with updated operationId and keep the old ones as legacy.

TrackedResourcePatchOperationValidation, Tracked resource 'ApplicationGateway' must have patch operation that at least supports the update of tags 1214

Will not be added in the near future, needs major changes in the backend.

-DefinitionsPropertiesNamesCamelCase, 636, 640, 656, 740, 865, 1122, 1143, 740

All related to ...IP... naming. Keep it like that for now, might be changed in next bigger PR.

shahabhijeet · 2017-03-27T23:06:30Z

Generated SDK Azure/azure-sdk-for-net#2992

sergey-shandar · 2017-03-28T00:06:13Z

@ravbhatnagar there are several cases when Operations API is not implemented because there are several APIs (swaggers) implemented by this service. Could you provide guidance how to provide the Operations API using composite swagger?

sergey-shandar

Please, address the errors/warnings in the next major version so we can get rid of them.

AutorestCI · 2017-03-28T00:15:19Z

No modification for NodeJS

AutorestCI · 2017-03-28T00:15:36Z

Azure/azure-sdk-for-ruby#736

AutorestCI · 2017-03-28T00:16:20Z

Azure/azure-sdk-for-python#1054

jobatzil added 10 commits March 1, 2017 16:01

#WAF Created 2017-03-01 Version folder and copied all files and examp…

689dc2d

…les.

#WAF Changed apiVersion in all examples from 2016-12-01 to 2017-03-01.

b2c4251

#WAF Changed ApiVersion in all swagger files from 2016-12-01 to 2016-…

929da04

…03-01.

#WAF changed references in compositeNetworkClient to 2017-03-01 version.

843c97d

Merge branch 'master' into feature/jobatzil/waf

4ea07ae

#WAF added fields for enhanced WAF to the application gateway swagger…

c5c1e5f

… file.

#WAF added support for application gateway get available waf rule set…

5bbfacd

…s API call.

#WAF set x-ms-flatten to true for ApplicationGatewayFirewallRuleSet.

7b356ca

Merge branch 'master' into feature/jobatzil/waf

0260d35

#WAF updated comment for ApplicationGatewayFirewallDisabledRuleGroup.…

c82b82c

…rules.

msftclas added the cla-not-required label Mar 21, 2017

jobatzil mentioned this pull request Mar 21, 2017

#WAF Added the changes of web application firewall for application gateway Azure/azure-sdk-for-net#2958

Merged

8 tasks

azuresdkci assigned sergey-shandar Mar 21, 2017

jobatzil changed the title ~~Add extended WAF functionality to Application Gateway~~ #WAF Add extended WAF functionality to Application Gateway Mar 21, 2017

jobatzil added 3 commits March 21, 2017 10:03

#WAF Added draining health state for backend health API.

76a7d9a

#WAF removed redundancy in description of enums.

ae0c854

#WAF updated required fields for getBackendHealth Api result.

02be03f

sergey-shandar reviewed Mar 21, 2017

View reviewed changes

DeepakRajendranMsft suggested changes Mar 21, 2017

View reviewed changes

sergey-shandar added the potential-sdk-breaking-change label Mar 21, 2017

DeepakRajendranMsft approved these changes Mar 22, 2017

View reviewed changes

#WAF Renamed GetAvailableWafRuleSets to ListAvailableWafRuleSets.

2ad1447

#WAF removed text/json support

a4780f5

jobatzil mentioned this pull request Mar 27, 2017

#WAF Renamed getAvailableWafRuleSets to listAvailableWafRuleSets and enhanced tests. Azure/azure-sdk-for-net#2992

Merged

6 tasks

sergey-shandar approved these changes Mar 28, 2017

View reviewed changes

veronicagg merged commit 7c15291 into Azure:master Mar 28, 2017

AutorestCI mentioned this pull request Mar 28, 2017

Automatic PR from RestAPI-PR1052 Azure/azure-sdk-for-ruby#736

Merged

AutorestCI mentioned this pull request Mar 28, 2017

[Network] WAF Add extended WAF functionality to Application Gateway Azure/azure-sdk-for-python#1054

Closed

akshaysngupta mentioned this pull request Jun 6, 2017

#ApplicationGateway Ssl Policy, Get AvailableSslOption, Redirect, Azure Websites and backendhealth nic fix #1285

Merged

10 tasks

Conversation

jobatzil commented Mar 21, 2017 • edited by sergey-shandar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Mar 21, 2017

Uh oh!

sergey-shandar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergey-shandar Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jobatzil Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jobatzil commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jobatzil commented Mar 21, 2017

Uh oh!

sergey-shandar commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jobatzil commented Mar 22, 2017

Uh oh!

jobatzil commented Mar 25, 2017

Uh oh!

shahabhijeet commented Mar 27, 2017

Uh oh!

sergey-shandar commented Mar 28, 2017

Uh oh!

sergey-shandar left a comment

Choose a reason for hiding this comment

Uh oh!

AutorestCI commented Mar 28, 2017

Uh oh!

AutorestCI commented Mar 28, 2017

Uh oh!

AutorestCI commented Mar 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jobatzil commented Mar 21, 2017 •

edited by sergey-shandar

Loading

sergey-shandar left a comment •

edited

Loading

sergey-shandar Mar 21, 2017 •

edited

Loading

jobatzil Mar 21, 2017 •

edited

Loading

sergey-shandar commented Mar 22, 2017 •

edited

Loading