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

Bulk ASN #159

Merged
merged 11 commits into from
Aug 18, 2023
Merged

Bulk ASN #159

merged 11 commits into from
Aug 18, 2023

Conversation

Harisabdullah
Copy link
Contributor

Flow of Control is as follows
-> ipinfo as500 -> cmd_asn_single
-> ipinfo asn -> cmd_asn
-> ipinfo asn bulk -> cmd_asn_bulk

Hence the refactor of files

Implements #56

@linear
Copy link

linear bot commented Aug 3, 2023

BE-2184 CLI ASN Bulk

Related to issue : #56

Right now we don't support looking up ASNs in bulk, but it's a useful feature to have.

In the process, we should actually support mixing together IPs & ASNs in one bulk lookup, by default.

And as a further improvement to the API, we should deprecate the bulk subcommand and allow automatically looking up things in bulk via e.g. ipinfo 1.1.1.1 8.8.8.8 if the number of inputs is more than 1.

Here's a specific checklist of things to consider:

  • support multiple inputs for the no-subcommand case by default, i.e. no need for a bulk subcommand anymore, so write a msg calling it deprecated
  • use non-bulk and support pretty format if we're still only with 1 input.
  • allow bulk ASN lookups as well, and allow this to be mixed with IP lookups.
  • if different data types are used, disallow features that won't make sense for mixed outputs, like csv format, field selection, etc.
  • support ASN range selection format such as ipinfo AS1-AS5, which gives ipinfo AS1 AS2 AS3 AS4 AS5.

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

@Harisabdullah I'll review this when we develop the generic solution for reading inputs from any place.

Restructure the codebase to enhance input handling from various sources
Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

We require essentially 'passing down' the op func to make it work in a 'streaming' way - without buffering inputs at lower levels and bringing them up higher, because otherwise it won't scale.

@@ -134,7 +134,7 @@ func cmdInit() error {
}

// save token to file.
gConfig.Token = tok
gConfig.Token = newtoken
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional?

ipinfo/cmd_asn.go Show resolved Hide resolved
INPUT_TYPE_IP INPUT_TYPE = "IP"
INPUT_TYPE_IP_RANGE INPUT_TYPE = "IPRange"
INPUT_TYPE_CIDR INPUT_TYPE = "CIDR"
INPUT_TYPE_FILE INPUT_TYPE = "File"
Copy link
Contributor

Choose a reason for hiding this comment

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

Files are a source, not an input type: the generic functionality, if told to read from files by the user, should do so by opening them and parsing their inputs for ips/ranges/cidrs/asns/etc. The same way it transparently 'opens' the 'stdin file' and reads from it when requested.

}

if isPiped || isTyping || stat.Size() > 0 {
inputs = append(inputs, ReadStringsFromStdin()...)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should actually be doing an op per string coming from stdin on the fly, instead of having it be returned. Otherwise for operations like prips this won't scale, because it'll store the whole thing in memory before proceeding and in some inputs that won't be even possible (huge IP ranges/cidrs).

}

// readStringsFromFile reads strings from a file, one per line.
func readStringsFromFile(filename string) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to ensure all of our actions are not buffering inputs into an arbitrarily large array - this will make certain use cases not scale. We need to perform the op per input in the file if we're told to open files.

}

// ReadStringsFromStdin reads strings from stdin until an empty line is entered.
func ReadStringsFromStdin() []string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same argument here

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

Nice, let's try this out in main

lib/utils_input.go Outdated Show resolved Hide resolved
lib/utils_input.go Show resolved Hide resolved
lib/utils_input.go Outdated Show resolved Hide resolved
@UmanShahzad UmanShahzad merged commit 460490c into master Aug 18, 2023
@UmanShahzad UmanShahzad deleted the haris/BE-2184 branch August 18, 2023 04:15
@UmanShahzad UmanShahzad mentioned this pull request Aug 18, 2023
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