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

Add support for Nginx DOS feature #2241

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Add support for Nginx DOS feature #2241

merged 1 commit into from
Dec 16, 2021

Conversation

soneillf5
Copy link
Contributor

@soneillf5 soneillf5 commented Dec 3, 2021

Proposed changes

This changes adds support for the DOS feature. It introduces custom resource definitions to hold DOS configuration. It adds logic to the ingress controller to add the DOS configuration to the nginx instance managed by the ingress controller.

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 documentation Pull requests/issues for documentation enhancement Pull requests for new features/feature enhancements labels Dec 3, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2021

Codecov Report

Merging #2241 (7e56325) into master (98239fc) will increase coverage by 0.20%.
The diff coverage is 56.69%.

❗ Current head 7e56325 differs from pull request most recent head 882879b. Consider uploading reports for the commit 882879b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2241      +/-   ##
==========================================
+ Coverage   53.41%   53.62%   +0.20%     
==========================================
  Files          43       48       +5     
  Lines       13412    14186     +774     
==========================================
+ Hits         7164     7607     +443     
- Misses       6020     6339     +319     
- Partials      228      240      +12     
Impacted Files Coverage Δ
cmd/nginx-ingress/main.go 6.31% <0.00%> (-0.36%) ⬇️
internal/configs/config_params.go 76.74% <ø> (ø)
internal/configs/version1/config.go 0.00% <ø> (ø)
internal/configs/version2/http.go 0.00% <ø> (ø)
internal/configs/warnings.go 100.00% <ø> (ø)
internal/k8s/controller.go 10.83% <0.00%> (-0.72%) ⬇️
internal/k8s/handlers.go 6.83% <0.00%> (-0.97%) ⬇️
internal/k8s/secrets/store.go 64.78% <0.00%> (ø)
internal/k8s/spiffe.go 0.00% <0.00%> (ø)
internal/k8s/task_queue.go 0.00% <0.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98239fc...882879b. Read the comment docs.

build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
deployments/helm-chart-dos-arbitrator/Chart.yaml Outdated Show resolved Hide resolved
deployments/helm-chart-dos-arbitrator/Chart.yaml Outdated Show resolved Hide resolved
deployments/helm-chart-dos-arbitrator/README.md Outdated Show resolved Hide resolved
@soneillf5
Copy link
Contributor Author

@lucacome thanks for the review, I've made the changes you requested. If you could re-review, that would be great, thanks.

@nginx-bot nginx-bot force-pushed the feature/bados branch 3 times, most recently from ea5fd8f to 390a356 Compare December 9, 2021 06:09
@soneillf5
Copy link
Contributor Author

@lucacome Hi Luca, I've made the changes you've requested. If there's anything else you'd recommend, let me know.

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 @soneillf5 Please see my comments and suggestions

Makefile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
deployments/helm-chart-dos-arbitrator/Chart.yaml Outdated Show resolved Hide resolved
internal/k8s/reference_checkers.go Show resolved Hide resolved
internal/k8s/validation_test.go Outdated Show resolved Hide resolved
internal/k8s/validation_test.go Outdated Show resolved Hide resolved
pkg/apis/configuration/validation/dos.go Outdated Show resolved Hide resolved
tests/data/virtual-server-dos/README.md Outdated Show resolved Hide resolved
@soneillf5
Copy link
Contributor Author

@soneillf5 test

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 @soneillf5 I reviewed the latest changes. I also spotted a few problems I missed during the first round

deployments/helm-chart/README.md Show resolved Hide resolved
examples/appprotect-dos/syslog.yaml Outdated Show resolved Hide resolved
examples/custom-resources/dos/syslog.yaml Outdated Show resolved Hide resolved
@@ -10,20 +10,21 @@ import (
"github.com/spiffe/go-spiffe/workload"
)

type spiffeController struct {
// SpiffeController controls spiffe
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No unfortunately the linter companies about many files, so this got fixed as part of trying to stop the linter from failing.

Copy link
Member

Choose a reason for hiding this comment

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

it shouldn't complain about files that weren't modified

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well you see, when the linting fails, it gives a very unhelpful message. i.e. file is not gofumpt -ed. This is in contrast to normal linters that say things like dos.go:123 function is missing a space before curly bracket. Also, as you can see here, https://github.com/nginxinc/kubernetes-ingress/runs/4507864465?check_suite_focus=true it doesn't show you which file is not gofumpt -ed.

This means the developer needs to run the linter locally. Unfortunately, the linter reports many, many issues. That's where we get to why SpiffeController was updated. make lint actually reports all linting issues but it seems the listing action on GitHub only fails for certain linters. If I run it locally this morning I see several errors:

tests/test-servers/tcp/main.go:44:19: Error return value of `conn.Close` is not checked (errcheck)
		defer conn.Close()
		                ^
internal/metrics/collectors/processes.go:60:19: G304: Potential file inclusion via variable (gosec)
		content, err := ioutil.ReadFile(cmdlineFile)

But interestingly these don't turn up in the Github lint action!
These means when I was trying to fix the linting errors, I didn't know which ones I need to fix. So I started fixing them all. Hence, the SpiffeController now has an additional comment.

Also, gofumpt and gofmt are not dependencies of the project. This means a developer needs to independently download those tools, figure out their arguments and run them manually.

Also, since if we touch a file, it must be perfectly linted, it places a large burden on the developer. This PR added code to ParseConfigMap which causes the linter to fail as it's cyclomatic complexity of 135 exceeds the 15 threshold. I should not have to pause development and refactor an unrelated 500 line function to get the DOS feature onto the main branch.

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 @soneillf5 the latest changes look good. However, there are a few outstanding items, that's why I'm not approving yet

build/Dockerfile Outdated Show resolved Hide resolved
examples/custom-resources/dos/apdos-protected.yaml Outdated Show resolved Hide resolved
@soneillf5
Copy link
Contributor Author

@lucacome @pleshakov can you re-review this please?

@soneillf5 soneillf5 force-pushed the feature/bados branch 2 times, most recently from fe88157 to 6f53ad4 Compare December 16, 2021 12:56
@soneillf5
Copy link
Contributor Author

soneillf5 commented Dec 16, 2021

@pleshakov I've merged Tomer's latest PR into this feature branch, it updates the access logs to use a second syslog

#2280

Does this address your concerns about the access log docs ?

.gitlab-ci.yml Outdated Show resolved Hide resolved
This change adds support for the Nginx DOS module. It includes custom resources, examples and documentation.

Co-authored-by: Tomer Pasman <t.pasman@f5.com>
@soneillf5 soneillf5 dismissed lucacome’s stale review December 16, 2021 17:22

this is blocking a merge, review comments and question were addressed

@soneillf5 soneillf5 merged commit 3e6db80 into master Dec 16, 2021
@soneillf5 soneillf5 deleted the feature/bados branch December 16, 2021 17:23
@lucacome lucacome changed the title Feature/bados Add support for Nginx DOS feature Dec 31, 2021
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 enhancement Pull requests for new features/feature enhancements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants