Conversation
common/request_filter/rule.go
Outdated
There was a problem hiding this comment.
package folder name should be the same as the package name: requestfilter.
common/request_filter/sizefilter.go
Outdated
| maxSize := ms.maxSizeBytes | ||
| requestSize := requestSizeInBytes(request) | ||
| if requestSize > maxSize { | ||
| return fmt.Errorf("the request's size exceeds the maximum size") |
There was a problem hiding this comment.
Add the expected values, e.g.:
fmt.Errorf("the request's size is too big: actual=%d, max size limit=%d", actual, max)
common/request_filter/sigfilter.go
Outdated
| // Signature verification rule | ||
| // TODO: implement signature validation | ||
|
|
||
| type NoOpSigFilter struct{} |
There was a problem hiding this comment.
Implement the Rule interface, add the methods
There was a problem hiding this comment.
partially implemented
common/request_filter/rule.go
Outdated
| // PayloadNotEmptyRule - checks that the payload in the request is not nil. | ||
| var PayloadNotEmptyRule = Rule(payloadNotEmptyRule{}) | ||
|
|
||
| type payloadNotEmptyRule struct{} | ||
|
|
||
| func (a payloadNotEmptyRule) Verify(request *comm.Request) error { | ||
| if request.Payload == nil { | ||
| return fmt.Errorf("empty payload field") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (a payloadNotEmptyRule) Update(config FilterConfig) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This can be part of the size filter file or alone in its own file
| return nil | ||
| } | ||
|
|
||
| func (ms *MaxSizeFilter) Update(config FilterConfig) error { |
There was a problem hiding this comment.
Lets think of a way to make it thread safe... Either here or in the calling entity.
There was a problem hiding this comment.
added RWmutex to verifier
DorKatzelnick
force-pushed
the
RequestFilter
branch
from
9f8fcd3 to
13d8db4
Compare
common/request_filter/sizefilter.go
Outdated
| // requestSizeInBytes return the (approximated) size in bytes of a request | ||
| func requestSizeInBytes(request *comm.Request) uint64 { | ||
| return uint64(len(request.Payload) + len(request.Signature)) | ||
| } |
There was a problem hiding this comment.
this method is too small... just put it in the calling function
common/request_filter/sizefilter.go
Outdated
|
|
||
| // Verfiy checks that the size of the request does not exceeds the maximal size in bytes. | ||
| func (ms *MaxSizeFilter) Verify(request *comm.Request) error { | ||
| maxSize := ms.maxSizeBytes |
There was a problem hiding this comment.
why convert to a local var? just use the field
common/requestfilter/sizefilter.go
Outdated
| // requestSizeInBytes return the (approximated) size in bytes of a request | ||
| func requestSizeInBytes(request *comm.Request) uint64 { | ||
| return uint64(len(request.Payload) + len(request.Signature)) | ||
| } |
There was a problem hiding this comment.
This is a one line method that is only used once... just put this one line where it is called.
common/requestfilter/sizefilter.go
Outdated
| } | ||
|
|
||
| // PayloadNotEmptyRule - checks that the payload in the request is not nil. | ||
| var PayloadNotEmptyRule = Rule(payloadNotEmptyRule{}) |
There was a problem hiding this comment.
Why?
I think this
type payloadNotEmptyRule struct{}
can just be public and that's all...
| require.NoError(t, err) | ||
| } | ||
|
|
||
| func TestSigVerifyFilter(t *testing.T) { |
There was a problem hiding this comment.
The convention in Go is to place the unit tests of structs that are in a file in a separate file, i.e.
rule_test,go
sigfilter_test.go
sizefilter_test.go
etc,
here do only the tests that check the integrative nature of RulesVerifier. e.g. thread safety, atomicity of updates etc.
also a common practice is to run with coverage and make sure we have decent coverage, i.e. >90% for simple components and 80% in general for more difficult stuff.
There was a problem hiding this comment.
reorganized the tests. added a couple of concurrency tests. coverage is around 87% for now.
| type testfiltersupport struct { | ||
| maxSize uint64 | ||
| } | ||
|
|
||
| func (tfs *testfiltersupport) GetMaxSizeBytes() (uint64, error) { | ||
| return tfs.maxSize, nil | ||
| } |
There was a problem hiding this comment.
If you mock this interface with counterfeiter, you can easily inject errors and any value you want... It is generally preferred in Go.
There was a problem hiding this comment.
replaced all interfaces with mocks
after review Signed-off-by: Dor.Katzelnick <Dor.Katzelnick@ibm.com>
DorKatzelnick
force-pushed
the
RequestFilter
branch
from
13d8db4 to
2e9770d
Compare
tock-ibm
left a comment
tock-ibm
left a comment
There was a problem hiding this comment.
@DorKatzelnick I approve with some minor comment to be taken on next PR
| case <-time.After(7 * time.Second): | ||
| t.Error("concurrent verify took too long") | ||
| } |
There was a problem hiding this comment.
On normal machines you can make this assumption (all goroutines enter together, no more than 1s) but with loaded CI machines that has a (very small) potential to cause flakes.
In addition, the code above this point does not really check the goroutine enter the rw lock together as the 1s delay is after you exit Verify.
Another approach would be to use a mock filter that sleeps for a 1s when Verify is called.
The best approach would be to combine this with code that counts the maximal number of threads inside the Verify. Like:
enter verify, increment a (atomic) counter,
sleep T
update (atomic) max-of-counter
sleep T
decrement counter
exit verify
assert max-of-counter > 1
Having said all that, this is way too much... do it if you have some spare time in the next PR
2 participants
following PR #131