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

Registration tracker #268

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

0x90-n
Copy link
Contributor

@0x90-n 0x90-n commented Jan 31, 2024

A new independent module that tracks registrations and aggregates them by their originating ASN.

@0x90-n 0x90-n requested a review from ewust January 31, 2024 04:02
Copy link
Member

@ewust ewust left a comment

Choose a reason for hiding this comment

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

Overall looks good, some minor comments that aren't blockers but could help improve the tool in general.

@@ -10,9 +10,9 @@ CFLAGS = -Wall -DENABLE_BPF -DHAVE_PF_RING -DHAVE_PF_RING_ZC -DTAPDANCE_USE_PF_R
PROTO_RS_PATH=src/signalling.rs
EXE_DIR=./bin

all: rust libtd conjure app registration-server ${PROTO_RS_PATH}
all: rust libtd conjure app registration-server registration-tracker ${PROTO_RS_PATH}
Copy link
Member

Choose a reason for hiding this comment

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

Is registration-tracker something we want built when we build the station? I would think this is more of a utility, so it wouldn't require building every time we make a production station, more for when we want to debug.

)


type transportStats struct {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but it may make more sense to have this just be a map of int64s, keyed on the transport.Name(), since that's what ends up happening in IncrementCount(). It would make it easier to add new transports (the name just has to be unique), and then printing it out is simply selecting what set of names/keys you want to output.

)
}
}
rs.v4Registrations = make(map[uint]*transportStats)
Copy link
Member

Choose a reason for hiding this comment

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

Does this leak the old rs.v4Registrations? I would guess no, but might be good to double check that we don't need to delete the old one

select {
case <- ctx.Done():
logger.Fatal("closing all ingest threads")
case msgChan <- data:
Copy link
Member

Choose a reason for hiding this comment

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

Why have regChan return an array of msgChans that you have to read from? Is there a reason to avoid it providing the messages directly?

continue
}

go func(ip net.IP, registration *cj.DecoyRegistration) {
Copy link
Member

Choose a reason for hiding this comment

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

You may want to turn this into a worker model, rather than launching a new thread per registration. New threads have the downside that you may launch too many (if a storm of new registrations comes in), and there's likely some upper-bound of useful threads doing this.

If this tool never runs on a production server it's probably okay, but if it does we would want to bound the number of concurrent threads launched by this tool to prevent us from consuming too much CPU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants