-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
6cdbcba
to
7b4bd01
Compare
7b4bd01
to
a2082f9
Compare
a2082f9
to
b257989
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
f3f450d
to
2d243b6
Compare
b3ecb0e
to
c05cec6
Compare
5ade28b
to
4e71a24
Compare
@lucacome thanks for the review, I've made the changes you requested. If you could re-review, that would be great, thanks. |
ea5fd8f
to
390a356
Compare
@lucacome Hi Luca, I've made the changes you've requested. If there's anything else you'd recommend, let me know. |
4a83e52
to
b4cd9ec
Compare
There was a problem hiding this 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
b4cd9ec
to
48c2eb9
Compare
@soneillf5 test |
8822683
to
17220f2
Compare
There was a problem hiding this 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
docs/content/configuration/ingress-resources/advanced-configuration-with-annotations.md
Outdated
Show resolved
Hide resolved
docs/content/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
docs/content/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
docs/content/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
@@ -10,20 +10,21 @@ import ( | |||
"github.com/spiffe/go-spiffe/workload" | |||
) | |||
|
|||
type spiffeController struct { | |||
// SpiffeController controls spiffe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated changes?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
aa1dfa7
to
f110f19
Compare
There was a problem hiding this 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
docs/content/configuration/virtualserver-and-virtualserverroute-resources.md
Outdated
Show resolved
Hide resolved
bb802cf
to
86374f0
Compare
@lucacome @pleshakov can you re-review this please? |
fe88157
to
6f53ad4
Compare
@pleshakov I've merged Tomer's latest PR into this feature branch, it updates the access logs to use a second syslog Does this address your concerns about the access log docs ? |
This change adds support for the Nginx DOS module. It includes custom resources, examples and documentation. Co-authored-by: Tomer Pasman <[email protected]>
7e56325
to
882879b
Compare
this is blocking a merge, review comments and question were addressed
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.