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

Support for App Protect module #1017

Closed
wants to merge 20 commits into from
Closed

Support for App Protect module #1017

wants to merge 20 commits into from

Conversation

rafwegv
Copy link
Contributor

@rafwegv rafwegv commented Jun 30, 2020

Proposed changes

Add App Protect integration to Ingress Controller.
This adds two crd's and changes in go code that enables watching on those resources.
policies and log configuration can be added to ingresses using annotations.
CR's are read and written onto container storage as files , referenced in nginx configurations.

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

@rafwegv rafwegv added the change Pull requests that introduce a change label Jun 30, 2020
@rafwegv rafwegv self-assigned this Jun 30, 2020
@pleshakov pleshakov added the enhancement Pull requests for new features/feature enhancements label Jun 30, 2020
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 my comments and suggestions. nothing major, just small corrections/improvements

Additionally, if #1009 is merged first, we need to update in this PR how helm handles AppProtect CRDs.

build/appprotect/DockerfileWithAppProtectForPlus Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/k8s/controller.go Outdated Show resolved Hide resolved
internal/nginx/manager.go Outdated Show resolved Hide resolved
docs-web/installation/installation-with-manifests.md Outdated Show resolved Hide resolved
docs-web/technical-specifications.md Outdated Show resolved Hide resolved
docs-web/app-protect/configuration.md Show resolved Hide resolved
@Rulox
Copy link
Contributor

Rulox commented Jul 1, 2020

Let's wait until #1009 is merged and make the changes here, I think is more fair/easier.

EDIT: I'll follow up with Dean and coordinate who merges first, I don't want to delay this PR just because of the changes in Helm.

@Dean-Coakley
Copy link
Contributor

Let's wait until #1009 is merged and make the changes here, I think is more fair/easier.

EDIT: I'll follow up with Dean and coordinate who merges first, I don't want to delay this PR just because of the changes in Helm.

Just an update: I merged as agreed.

Copy link

@AvriBalofsky AvriBalofsky left a comment

Choose a reason for hiding this comment

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

The ability to use "external references" is also missing from the policy definition. Is that an intentional limitation?

deployments/common/ap-logconf-definition.yaml Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Outdated Show resolved Hide resolved
deployments/common/ap-policy-definition.yaml Show resolved Hide resolved
@Rulox Rulox force-pushed the ap-integration-master branch from c36ea8b to 93c3678 Compare July 1, 2020 16:27
@rafwegv
Copy link
Contributor Author

rafwegv commented Jul 2, 2020

I decided to add preserve unknown fields false to two fields, since they do not have a specific form (modifications) or are very complicated (signature sets).

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 🚀

I think it was my merged PR that caused conflicts. sry! :)

@Rulox
Copy link
Contributor

Rulox commented Jul 3, 2020

Closing in favour of #1035

@Rulox Rulox closed this Jul 3, 2020
@Rulox Rulox deleted the ap-integration-master branch July 3, 2020 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Pull requests that introduce a change enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants