Skip to content

Added StructuredStats method to returned structured stat data#56

Merged
squeed merged 2 commits intocoreos:masterfrom
asilvr:structured-stats
May 29, 2019
Merged

Added StructuredStats method to returned structured stat data#56
squeed merged 2 commits intocoreos:masterfrom
asilvr:structured-stats

Conversation

@asilvr
Copy link

@asilvr asilvr commented Mar 14, 2019

This additional method maintains the currently available Stats method while adding a StructuredStats method, which returns structured data instead of a slice of string slices with statistics. This allows the caller to use a defined data structure for accessing fields in a specific statistic.

Although a caller could peer into the existing Stats method to determine what each field is, providing this abstraction allows for more portability/usability.

@asilvr asilvr closed this Mar 14, 2019
@asilvr asilvr deleted the structured-stats branch March 14, 2019 22:49
@asilvr asilvr restored the structured-stats branch March 14, 2019 22:50
@asilvr asilvr reopened this Mar 14, 2019
@asilvr asilvr changed the title Added StructStats method to returned structured data in a Stat slice Added StructuredStats method to returned structured stat data Mar 14, 2019
@asilvr asilvr force-pushed the structured-stats branch from ba9e3cf to f5321a7 Compare March 14, 2019 23:59

structStats := []Stat{}
for _, rawStat := range rawStats {
if len(rawStat) != 10 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that this will ever be 11? In other words, could iptables add new stats?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think this is likely, but it is possible.

I can omit this check and instead leave it to the parsing (mentioned below) to handle returning an error if the datatypes are mismatched or the number of fields is not within the expected range.

Copy link
Author

Choose a reason for hiding this comment

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

I moved this check into the ParseStat method, but made it a minimum of 10 stats in a row to allow for forward compatibility.


// Stat represents a structured statistic entry.
type Stat struct {
Packets string `json:"pkts"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you parse all numbers accordingly?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I will make this change.


// StructuredStats returns statistics as structured data which may be further
// parsed and marshaled.
func (ipt *IPTables) StructuredStats(table, chain string) ([]Stat, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest you add a separate ParseStats function that just takes a []string and parses the stats accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

I originally was going to do exactly that. The only issue I foresee is that exposing a ParseStats method that accepts an arbitrary []string (or [][]string) may be subject to misuse.

Anyway, two questions I'm looking for your feedback on:

  1. I can make a ParseStat(stat []string) (Stat, error) and/or a ParseStats(stats [][]string) ([]Stat, error). Any preference for either or both?

  2. Would you like me to remove StructuredStats(table, chain string) ([]Stat, error) completely?

Copy link
Author

Choose a reason for hiding this comment

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

BTW, I pushed up a contribution with ParseStat(stat []string) (Stat, error) and left StructuredStats(table, chain string) ([]Stat, error) for now pending feedback.

Opt string `json:"opt"`
Input string `json:"in"`
Output string `json:"out"`
Source string `json:"source"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I forget - can these be hostnames, or do they have to be IP addresses? If they have to be IP addresses, can you parse them accordingly?

Copy link
Author

Choose a reason for hiding this comment

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

The method Stats(...) uses the -n flag. From the man pages:

--numeric -n numeric output of addresses and ports

This shows the source and destination in CIDR format. Below is some output:

$ iptables --version
iptables v1.4.21
$ iptables -t filter -L INPUT -n -v -x
Chain INPUT (policy ACCEPT 12 packets, 857 bytes)
    pkts      bytes target     prot opt in     out     source               destination
       2      128 ACCEPT     tcp  --  *      *       0.0.0.0/0            0.0.0.0/0            tcp dpt:3332
$ iptables -t filter -L INPUT  -v -x
Chain INPUT (policy ACCEPT 43 packets, 3073 bytes)
    pkts      bytes target     prot opt in     out     source               destination
       2      128 ACCEPT     tcp  --  any    any     anywhere             anywhere             tcp dpt:mcs-mailsvr

From this, we know it will be CIDR notation IP addresses.

We can call this struct NumericalStats to be more accurate. Since it's already granular, I don't think this is necessary.

@squeed
Copy link
Contributor

squeed commented Mar 15, 2019

FYI, I won't have computer access for a few weeks. So apologies in advance for the radio silence.

@asilvr
Copy link
Author

asilvr commented May 27, 2019

Hi @squeed, just a friendly request to revisit this if appropriate. Thanks!

@squeed
Copy link
Contributor

squeed commented May 29, 2019

@asilvr sorry, this fell off the stack. Looks good to me!

@squeed squeed merged commit 2ed0620 into coreos:master May 29, 2019
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