-
Notifications
You must be signed in to change notification settings - Fork 34
Refactor hashType: []byte -> uint64 #228
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ package conntrack | |
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/binary" | ||
| "encoding/gob" | ||
| "fmt" | ||
| "hash" | ||
|
|
@@ -28,78 +29,50 @@ import ( | |
| log "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| type hashType []byte | ||
|
|
||
| // TODO: what's a better name for this struct? | ||
| type totalHashType struct { | ||
| hashA hashType | ||
| hashB hashType | ||
| hashTotal hashType | ||
| } | ||
|
|
||
| func areHashEqual(h1, h2 hashType) bool { | ||
| if len(h1) != len(h2) { | ||
| return false | ||
| } | ||
| for i := range h1 { | ||
| if h1[i] != h2[i] { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func copyTotalHash(h totalHashType) totalHashType { | ||
| newHashA := make([]byte, len(h.hashA)) | ||
| newHashB := make([]byte, len(h.hashB)) | ||
| newHashTotal := make([]byte, len(h.hashTotal)) | ||
| copy(newHashA, h.hashA) | ||
| copy(newHashB, h.hashB) | ||
| copy(newHashTotal, h.hashTotal) | ||
| return totalHashType{ | ||
| hashA: newHashA, | ||
| hashB: newHashB, | ||
| hashTotal: newHashTotal, | ||
| } | ||
| hashA uint64 | ||
| hashB uint64 | ||
| hashTotal uint64 | ||
| } | ||
|
|
||
| // ComputeHash computes the hash of a flow log according to keyDefinition. | ||
| // Two flow logs will have the same hash if they belong to the same connection. | ||
| func ComputeHash(flowLog config.GenericMap, keyDefinition api.KeyDefinition, hasher hash.Hash) (*totalHashType, error) { | ||
| fieldGroup2hash := make(map[string]hashType) | ||
| func ComputeHash(flowLog config.GenericMap, keyDefinition api.KeyDefinition, hasher hash.Hash64) (totalHashType, error) { | ||
| fieldGroup2hash := make(map[string]uint64) | ||
|
|
||
| // Compute the hash of each field group | ||
| for _, fg := range keyDefinition.FieldGroups { | ||
| h, err := computeHashFields(flowLog, fg.Fields, hasher) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("compute hash: %w", err) | ||
| return totalHashType{}, fmt.Errorf("compute hash: %w", err) | ||
| } | ||
| fieldGroup2hash[fg.Name] = h | ||
| } | ||
|
|
||
| // Compute the total hash | ||
| th := &totalHashType{} | ||
| th := totalHashType{} | ||
| hasher.Reset() | ||
| for _, fgName := range keyDefinition.Hash.FieldGroupRefs { | ||
| hasher.Write(fieldGroup2hash[fgName]) | ||
| hasher.Write(uint64ToBytes(fieldGroup2hash[fgName])) | ||
| } | ||
| if keyDefinition.Hash.FieldGroupARef != "" { | ||
| th.hashA = fieldGroup2hash[keyDefinition.Hash.FieldGroupARef] | ||
| th.hashB = fieldGroup2hash[keyDefinition.Hash.FieldGroupBRef] | ||
| // Determine order between A's and B's hash to get the same hash for both flow logs from A to B and from B to A. | ||
| if bytes.Compare(th.hashA, th.hashB) < 0 { | ||
| hasher.Write(th.hashA) | ||
| hasher.Write(th.hashB) | ||
| if th.hashA < th.hashB { | ||
| hasher.Write(uint64ToBytes(th.hashA)) | ||
| hasher.Write(uint64ToBytes(th.hashB)) | ||
| } else { | ||
| hasher.Write(th.hashB) | ||
| hasher.Write(th.hashA) | ||
| hasher.Write(uint64ToBytes(th.hashB)) | ||
| hasher.Write(uint64ToBytes(th.hashA)) | ||
| } | ||
| } | ||
| th.hashTotal = hasher.Sum([]byte{}) | ||
| th.hashTotal = hasher.Sum64() | ||
| return th, nil | ||
| } | ||
|
|
||
| func computeHashFields(flowLog config.GenericMap, fieldNames []string, hasher hash.Hash) (hashType, error) { | ||
| func computeHashFields(flowLog config.GenericMap, fieldNames []string, hasher hash.Hash64) (uint64, error) { | ||
| hasher.Reset() | ||
| for _, fn := range fieldNames { | ||
| f, ok := flowLog[fn] | ||
|
|
@@ -109,11 +82,17 @@ func computeHashFields(flowLog config.GenericMap, fieldNames []string, hasher ha | |
| } | ||
| bytes, err := toBytes(f) | ||
| if err != nil { | ||
| return nil, err | ||
| return 0, err | ||
| } | ||
| hasher.Write(bytes) | ||
| } | ||
| return hasher.Sum([]byte{}), nil | ||
| return hasher.Sum64(), nil | ||
| } | ||
|
|
||
| func uint64ToBytes(data uint64) []byte { | ||
| b := make([]byte, 8) | ||
| binary.BigEndian.PutUint64(b, data) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the encoding order (little vs big endian) does not matter here, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right. |
||
| return b | ||
| } | ||
|
|
||
| func toBytes(data interface{}) ([]byte, error) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since this is a private type and attribute, maybe the
getHash()method is not needed anymore and you can invoke directlyc.hashThere 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.
I want the API of connection to have the property that once a connection object is created, its hash is read-only.
The other types that interact with connection (e.g.
conntrackImpland aggregators) are doing so using theconnectioninterface rather than theconnTypestruct. So in the current implementation I can't remove the method.