Add unary gRPC support in APPolicy

[rafwegv]

Proposed changes

This bumps the App Protect version to the last release and also introduces support for unary grpc in the Policy crd.

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 master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[Dean-Coakley]

@rafwegv We now have some automation for changelog generation: https://github.com/nginxinc/kubernetes-ingress/actions/workflows/release-drafter.yml

The resulting changelog draft should be visible to you: https://github.com/nginxinc/kubernetes-ingress/releases

I think it would be good if you could rename the PR to note that this change relates to AppProtect.

[pleshakov]

just to double check, is $ in front of the action is correct?

[rafwegv]

yes, it means that this field should be deleted from the base policy rather than added

[pleshakov]

what will happen if the file can't be downloaded from the URL? For example, the server rejects the request or times out. What is the timeout value?

can a failure that prevent NGINX Plus from reloading?

what happens if the download is slow? let's say it takes 60s. does it mean NGINX Plus will not be able to reload until the file is downloaded?

If this can cause too many problems, perhaps we should not allow external references

[rafwegv]

Valid points. Let's discuss offline.

[pleshakov]

Hi @rafwegv

Please see additional feedback:

(1) Do we need to update the CRD for APLogConf? I see at least one change - the format can support grpc.
https://docs.nginx.com/nginx-app-protect/configuration/#factory-configuration-files

(2) Do we need to use different default files for logging and/or policies for gRPC?
(based on https://docs.nginx.com/nginx-app-protect/configuration/#logging )
If that is the case, I think we need to update the template https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/version1/nginx-plus.ingress.tmpl#L67-L71

[pleshakov]

does this mean we're restoring this 1196174 ?

[rafwegv]

not yet, this must have gotten reenabled in merge somwhere. will disable.

[rafwegv]

re:

Hi @rafwegv

Please see additional feedback:

(1) Do we need to update the CRD for APLogConf? I see at least one change - the format can support grpc.
https://docs.nginx.com/nginx-app-protect/configuration/#factory-configuration-files

(2) Do we need to use different default files for logging and/or policies for gRPC?
(based on https://docs.nginx.com/nginx-app-protect/configuration/#logging )
If that is the case, I think we need to update the template https://github.com/nginxinc/kubernetes-ingress/blob/master/internal/configs/version1/nginx-plus.ingress.tmpl#L67-L71

Hi @pleshakov
(1) yes, will add grpc to the format enum, thanks for noticing.
(2) I think theese should remain as examples, and if Customers would like to create configs of of them they can.

[rafwegv]

Hi @pleshakov,
I removed the revision date times as well as links from grpc resources. Discussed with internally and decided to drop them. Cons in this use case outweigh the pros.

[pleshakov]

Hi @rafwegv

Could you possibly rebase against the master? Note that we finally removed v1beta1 CRDS.

I also noticed a few seemingly unrelated to gRPC fields that were added to the CRDs. Like signatureOverrides. Just to double check, are we adding any other features as part of this PR?

[rafwegv]

@pleshakov
Sometimes fields will be updated elswhere in the schema for various reasons. Theese in particular are kind of meta fields that are not part of a feature per-se. Alternatively we could have seperate PRs for such bug fixes in the future.