Fixed APPolicy CRD #4640

fabriziofiorucci · 2023-11-13T17:09:26Z

Proposed changes

Added the missing maskValueInLogs to cookies for app-protect
Added the missing useXmlResponsePage to xml-profiles for app-protect
Changed several types from string to integer in spec.policy.xml-profiles[0].defenseAttributes
Added missing enums in blocking-settings.properties.evasions.items.properties.description.enum

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

schweits · 2023-11-14T06:29:29Z

Not sure if you can change anything, but according to the NAP schema both ints and strings are valid values for those fields. Eg.

                                 "maximumAttributeValueLength" : {
                                    "default" : "1024",
                                    "oneOf" : [
                                       {
                                          "maximum" : 2147483647,
                                          "minimum" : 0,
                                          "type" : "integer"
                                       },
                                       {
                                          "enum" : [
                                             "any"
                                          ],
                                          "type" : "string"
                                       },
                                       {
                                          "pattern" : "\\d+$",
                                          "type" : "string"
                                       }
                                    ]
                                 },

fabriziofiorucci · 2023-11-14T08:50:46Z

@schweits for some reason several fields apparently need to be set to integer. Applying an APPolicy manifest containing:

  defenseAttributes:
    allowCDATA: true
    allowDTDs: true
    allowExternalReferences: true
    allowProcessingInstructions: true
    maximumAttributeValueLength: any
    maximumAttributesPerElement: 32
    maximumChildrenPerElement: 4096
    maximumDocumentDepth: 32
    maximumDocumentSize: 1024000
    maximumElements: 512000
    maximumNSDeclarations: 64
    maximumNameLength: any
    maximumNamespaceLength: any
    tolerateCloseTagShorthand: true
    tolerateLeadingWhiteSpace: true
    tolerateNumericNames: true

Triggers:

The APPolicy "nginx-policy" is invalid: 
* spec.policy.xml-profiles[0].defenseAttributes.maximumAttributesPerElement: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumAttributesPerElement in body must be of type string: "integer"
* spec.policy.xml-profiles[0].defenseAttributes.maximumElements: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumElements in body must be of type string: "integer"
* spec.policy.xml-profiles[0].defenseAttributes.maximumDocumentSize: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumDocumentSize in body must be of type string: "integer"
* spec.policy.xml-profiles[0].defenseAttributes.maximumNSDeclarations: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumNSDeclarations in body must be of type string: "integer"
* spec.policy.xml-profiles[0].defenseAttributes.maximumDocumentDepth: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumDocumentDepth in body must be of type string: "integer"
* spec.policy.xml-profiles[0].defenseAttributes.maximumChildrenPerElement: Invalid value: "integer": spec.policy.xml-profiles[0].defenseAttributes.maximumChildrenPerElement in body must be of type string: "integer"

Unless I'm missing something, the manifest snippet should be correct according to https://docs.nginx.com/nginx-app-protect-waf/declarative-policy/policy/#policy/graphql-profiles/defenseAttributes

schweits · 2023-11-14T11:36:21Z

@fabriziofiorucci Right, integers should be valid, but according to the schema (and the doc you linked too), passing in a value like "1024000" should be valid too. But with the change here, wouldn't it reject strings no matter what?

Edit: You should be able to do something like:

maximumBatchedQueries:
  pattern: any|\d+
  anyOf:
     - type: integer
     - type: string
  x-kubernetes-int-or-string: true

fabriziofiorucci · 2023-11-14T12:14:47Z

@schweits as discussed, I'll check, test and add the definitive version of the CRD yaml shortly.

galitskiy · 2023-11-14T18:09:06Z

agree with @schweits on changing type to int; but not sure if "x-kubernetes-int-or-string: true" is a way to go either (I think there was a discussion long time ago with kic team, dont remember what we agreed on). @fabriziofiorucci also from your example of the manifest there are fields with the same type (int or string) set to "any".

schweits · 2023-11-14T20:16:04Z

@galitskiy I provided @fabriziofiorucci with the CRD we're using in our internal testing repo, so that's where many of those changes came from.

config/crd/bases/appprotect.f5.com_appolicies.yaml

codecov · 2023-11-16T11:26:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7fcee3d) 51.93% compared to head (c55d000) 51.92%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4640      +/-   ##
==========================================
- Coverage   51.93%   51.92%   -0.01%     
==========================================
  Files          59       59              
  Lines       16972    16973       +1     
==========================================
- Hits         8814     8813       -1     
- Misses       7861     7862       +1     
- Partials      297      298       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vepatel · 2023-11-17T10:18:34Z

@fabriziofiorucci Also the definitions in deploy/crds-nap-waf.yaml are generated automatically by running make update-crds from project root, so you only need to:

change CRDs in config/crds/bases,
run make update-crds

vepatel

approved pending comment resolution

fabriziofiorucci · 2023-11-17T15:56:16Z

@vepatel for some reason make update-crds throws an error:

$ make update-crds
go run sigs.k8s.io/controller-tools/cmd/controller-gen crd paths=./pkg/apis/... output:crd:artifacts:config=config/crd/bases
go: downloading sigs.k8s.io/controller-tools v0.13.0
[...]
go: finding golang.org/x/text v0.13.0
build sigs.k8s.io/controller-tools/cmd/controller-gen: cannot load io/fs: malformed module path "io/fs": missing dot in first path element
make: *** [Makefile:66: update-crds] Error 1

this happens on a Ubuntu 20.04 VM. Any clue?

vepatel · 2023-11-17T16:17:44Z

@fabriziofiorucci its a local golang issue it seems, make sure you've installed go version i.e. 1.21.3

>> go version                              
go version go1.21.3 darwin/arm64

Now if it fails even after update, edit Makefile job update-crds like:

update-crds: ## Update CRDs
	@kustomize version || (code=$$?; printf "\033[0;31mError\033[0m: there was a problem with kustomize, use 'brew install kustomize' to install it\n"; exit $$code)
	kustomize build config/crd >deploy/crds.yaml
	kustomize build config/crd/app-protect-dos --load-restrictor='LoadRestrictionsNone' >deploy/crds-nap-dos.yaml
	kustomize build config/crd/app-protect-waf --load-restrictor='LoadRestrictionsNone' >deploy/crds-nap-waf.yaml

i.e. basically remove https://github.com/nginxinc/kubernetes-ingress/blob/main/Makefile#L66

shaun-nx · 2023-11-20T13:56:27Z

Hi @fabriziofiorucci
The PR is good to go now. If you rebase your branch I'll merge the PR as soon as the pipeline passes.

for more information, see https://pre-commit.ci

fabriziofiorucci

reviewed

fabriziofiorucci requested a review from a team as a code owner November 13, 2023 17:09

schweits mentioned this pull request Nov 13, 2023

Add missing app-protect maskValueInLogs and useXmlResponsePage #4638

Closed

6 tasks

brianehlert added this to the v3.4.0 milestone Nov 13, 2023

brianehlert added the backlog Pull requests/issues that are backlog items label Nov 13, 2023

danielnginx assigned vepatel Nov 15, 2023

vepatel reviewed Nov 16, 2023

View reviewed changes

config/crd/bases/appprotect.f5.com_appolicies.yaml Outdated Show resolved Hide resolved

vepatel approved these changes Nov 17, 2023

View reviewed changes

shaun-nx approved these changes Nov 20, 2023

View reviewed changes

shaun-nx self-assigned this Nov 20, 2023

fabriziofiorucci and others added 5 commits November 20, 2023 15:23

Fixed APPolicy CRD

97dbae3

Fixed APPolicy CRD

9da2fb1

[pre-commit.ci] auto fixes from pre-commit.com hooks

c37e300

for more information, see https://pre-commit.ci

Fixed APPolicy CRD

7a4ea86

[pre-commit.ci] auto fixes from pre-commit.com hooks

c55d000

for more information, see https://pre-commit.ci

fabriziofiorucci commented Nov 20, 2023

View reviewed changes

shaun-nx merged commit 8cdaf64 into nginx:main Nov 20, 2023
60 checks passed

fabriziofiorucci deleted the 4638 branch November 20, 2023 23:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed APPolicy CRD #4640

Fixed APPolicy CRD #4640

fabriziofiorucci commented Nov 13, 2023

schweits commented Nov 14, 2023

fabriziofiorucci commented Nov 14, 2023

schweits commented Nov 14, 2023 •

edited

Loading

fabriziofiorucci commented Nov 14, 2023

galitskiy commented Nov 14, 2023

schweits commented Nov 14, 2023

codecov bot commented Nov 16, 2023 •

edited

Loading

vepatel commented Nov 17, 2023 •

edited

Loading

vepatel left a comment •

edited

Loading

fabriziofiorucci commented Nov 17, 2023 •

edited

Loading

vepatel commented Nov 17, 2023 •

edited

Loading

shaun-nx commented Nov 20, 2023

fabriziofiorucci left a comment

Fixed APPolicy CRD #4640

Fixed APPolicy CRD #4640

Conversation

fabriziofiorucci commented Nov 13, 2023

Proposed changes

Checklist

schweits commented Nov 14, 2023

fabriziofiorucci commented Nov 14, 2023

schweits commented Nov 14, 2023 • edited Loading

fabriziofiorucci commented Nov 14, 2023

galitskiy commented Nov 14, 2023

schweits commented Nov 14, 2023

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

vepatel commented Nov 17, 2023 • edited Loading

vepatel left a comment • edited Loading

Choose a reason for hiding this comment

fabriziofiorucci commented Nov 17, 2023 • edited Loading

vepatel commented Nov 17, 2023 • edited Loading

shaun-nx commented Nov 20, 2023

fabriziofiorucci left a comment

Choose a reason for hiding this comment

schweits commented Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading

vepatel commented Nov 17, 2023 •

edited

Loading

vepatel left a comment •

edited

Loading

fabriziofiorucci commented Nov 17, 2023 •

edited

Loading

vepatel commented Nov 17, 2023 •

edited

Loading