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

AP: enable FQDN as syslog destination #1939

Merged
merged 1 commit into from Nov 16, 2021
Merged

AP: enable FQDN as syslog destination #1939

merged 1 commit into from Nov 16, 2021

Conversation

rafwegv
Copy link
Contributor

@rafwegv rafwegv commented Sep 8, 2021

Proposed changes

Add checks that enable an FQDN be specified as the destination for security logs

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

@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Sep 8, 2021
@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 13 times, most recently from aa36fb6 to 3cd26ce Compare October 13, 2021 19:36
@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 8 times, most recently from ca25f78 to 3203d01 Compare October 20, 2021 18:14
@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 5 times, most recently from 013a8bc to 0bdd080 Compare November 1, 2021 15:33
@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 4 times, most recently from df77910 to d374f0e Compare November 3, 2021 22:18
Copy link
Contributor

@soneillf5 soneillf5 left a comment

Choose a reason for hiding this comment

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

Hi @rafwegv The changes look good

@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 3 times, most recently from 092c0b3 to 666be2a Compare November 4, 2021 19:35
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @rafwegv

Please see my comments

Additionally:

(1) Shall we update the doc for the logDest field of the WAF Policy? (https://docs.nginx.com/nginx-ingress-controller/configuration/policy-resource/#waf). the annotation doc seems fines.

(2) Would it be possible to update and simplify the AppProtect tests so that they no longer rely on fetching the svc IP address, but rather use the DNS name?

examples/appprotect/README.md Outdated Show resolved Hide resolved
internal/k8s/appprotect/app_protect_resources.go Outdated Show resolved Hide resolved
@nginx-bot nginx-bot force-pushed the ap-fqdn-logdst branch 9 times, most recently from 646789a to efbcccc Compare November 12, 2021 02:09
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

@rafwegv lgtm! 👍 sorry for the delay

@rafwegv rafwegv merged commit 07fce4c into master Nov 16, 2021
@rafwegv rafwegv deleted the ap-fqdn-logdst branch November 16, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants