robbertkl / docker-ipv6nat

Extend Docker with IPv6 NAT, similar to IPv4
MIT License
661 stars 48 forks source link

Feature Request: Log ip6tables rules beeing added or removed to stdout #31

Closed movd closed 5 years ago

movd commented 5 years ago

This tool works great! For debugging purposes, it would be nice if actions like rule changes would be logged to stdout.

robbertkl commented 5 years ago

Thanks @movd, that's true. Should be an optional flag, to not generate too many docker logs.

I'm open to a PR on this, shouldn't be that complicated.

bephinix commented 5 years ago

Ok, let's check this.

I have found the following line:

func (m *manager) applyRules(oldRules, newRules *Ruleset) error {
    oldRules = oldRules.Diff(newRules) // LINE

    if err := m.fw.EnsureRules(newRules); err != nil {
        return err
    }

    if err := m.fw.RemoveRules(oldRules); err != nil {
        return err
    }

    return nil
}

We want to log all rule changes which are (maybe) empty list of added rules and remove rules. If a rule is changed, this will be reflected by a removed and an added rule. The marked line seems to save all removed rules to oldRules. This seems to be the best place for the logging. We could simply interate over both arrays and print Removed rule: [...] or Added rule: [...] for each entry in oldRules and newRules.

@robbertkl It is not really easy to understand, that after the marked line oldRules contains rules which will be removed.

@movd Does this may solve your issue?

bephinix commented 5 years ago

The actual logging command should be inserted here and here one line after the iptables action.

robbertkl commented 5 years ago

@bephinix

The marked line seems to save all removed rules to oldRules. This seems to be the best place for the logging. We could simply interate over both arrays and print Removed rule: [...] or Added rule: [...] for each entry in oldRules and newRules.

Better to add it within EnsureRules and RemoveRules within firewall.go.

@robbertkl It is not really easy to understand, that after the marked line oldRules contains rules which will be removed.

You're right, it would be better to introduce a new variable (something like rulesToRemove) instead of re-using the oldRules variable.

The actual logging command should be inserted here and here one line after the iptables action.

Yeah, that seems like a better place.

Would still like it sitting behind an optional flag, off by default.

bephinix commented 5 years ago

37 will implement it.