Add basic path policy filtering options to pathmgr #1909
Conversation
worxli
force-pushed
the
pathmgr-policies
branch
2 times, most recently
from
5d04fc9 to
06fdca9
Compare
worxli
force-pushed
the
pathmgr-policies
branch
2 times, most recently
from
a330392 to
630ab35
Compare
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @worxli and @kormat)
doc/PathPolicy.md, line 33 at r1 (raw file):
- `&` (logical AND) ## Policy
Maybe the list items in this section can link directly to their specs? I found myself jumping quite a bit between the two.
doc/PathPolicy.md, line 109 at r1 (raw file):
Path policies can be composed by extending other policies. The following example uses two sub-policies to create the top-level policy.
Expand this section with details on how composition works.
There was a problem hiding this comment.
Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @scrye and @kormat)
doc/PathPolicy.md, line 33 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Maybe the list items in this section can link directly to their specs? I found myself jumping quite a bit between the two.
Done.
doc/PathPolicy.md, line 109 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
Expand this section with details on how composition works.
Done.
There was a problem hiding this comment.
Reviewed 1 of 4 files at r3.
Reviewable status: 4 of 9 files reviewed, 9 unresolved discussions (waiting on @scrye and @worxli)
doc/PathPolicy.md, line 12 at r3 (raw file):
identifier must also be set to _0_. Interface predicates generally only refer to interfaces, if you want to create a predicate for an
The language here doesn't seem great, as most of the time specifying interfaces that aren't in the src/destination AS is probably not the correct thing to do.
doc/PathPolicy.md, line 14 at r3 (raw file):
Interface predicates generally only refer to interfaces, if you want to create a predicate for an **ISD-AS** only, the **#IF** part may be omitted. The same applies if the predicate should only apply to an **ISD**, then the **AS** identifier can be omitted.
I would add an example to illustrate the 3 different forms.
doc/PathPolicy.md, line 38 at r3 (raw file):
- [`sequence`](#Sequence) (space separated list of IFPs, may contain `..` as wildcard) - [`acl`](#ACL) (list of IFPs, preceded by `+` or `-`)
It should be specified somewhere that if there is no acl, the default is whitelist everything.
doc/PathPolicy.md, line 79 at r3 (raw file):
denying all other interfaces in ISD 1.
I would say instead that it denies all ASes in ISD 1. They are equivalent, but i think this way is clearer.
doc/PathPolicy.md, line 83 at r3 (raw file):
- policy:
Don't use policy as a policy name, it looks like a keyword. Use a different name in each example, perhaps a descriptive one.
doc/PathPolicy.md, line 85 at r3 (raw file):
- policy: acl: - '+ 1-ff00:0:133#0'
I'd drop all 0 elements from the examples, as that will be the idiomatic form.
doc/PathPolicy.md, line 93 at r3 (raw file):
### Sequence The sequence is a string of space separated IFPs. The [operators](#Operators) can be used for advanced
Line length.
doc/PathPolicy.md, line 98 at r3 (raw file):
The following example specifies a path from any interface in AS _1-ff00:0:133_ to two subsequent interfaces in AS _1-ff00:0:120_, then there are two explicit wildcards each matching any interface (in the future one `..` wildcard will be enough to match any number of interfaces). The path must
Why "in the future"?
There was a problem hiding this comment.
Reviewable status: 4 of 9 files reviewed, 9 unresolved discussions (waiting on @scrye, @kormat, and @worxli)
doc/PathPolicy.md, line 12 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The language here doesn't seem great, as most of the time specifying interfaces that aren't in the src/destination AS is probably not the correct thing to do.
Better explanation?
doc/PathPolicy.md, line 14 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I would add an example to illustrate the 3 different forms.
Done.
doc/PathPolicy.md, line 38 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It should be specified somewhere that if there is no acl, the default is whitelist everything.
Done.
doc/PathPolicy.md, line 79 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
denying all other interfaces in ISD 1.
I would say instead that it denies all ASes in ISD 1. They are equivalent, but i think this way is clearer.
Done.
doc/PathPolicy.md, line 83 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Don't use
policyas a policy name, it looks like a keyword. Use a different name in each example, perhaps a descriptive one.
But the top-level policy is a keyword. Otherwise there are many policies defined in a file and the entrypoint is unset.
doc/PathPolicy.md, line 85 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd drop all
0elements from the examples, as that will be the idiomatic form.
Done.
doc/PathPolicy.md, line 93 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Line length.
Done.
doc/PathPolicy.md, line 98 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why "in the future"?
Because the first version only supports what is currently possible with the old system. But maybe it would be better to currently not allow .. as it can be accomplished by 0-0#0.
worxli
force-pushed
the
pathmgr-policies
branch
from
c2803b6 to
5750ba2
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 9 files reviewed, 9 unresolved discussions (waiting on @kormat, @scrye, and @worxli)
doc/PathPolicy.md, line 83 at r3 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
But the top-level policy is a keyword. Otherwise there are many policies defined in a file and the entrypoint is unset.
Renamed the policies.
doc/PathPolicy.md, line 98 at r3 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Because the first version only supports what is currently possible with the old system. But maybe it would be better to currently not allow
..as it can be accomplished by0-0#0.
This wildcard was removed.
There was a problem hiding this comment.
Reviewed 2 of 9 files at r1, 1 of 4 files at r6.
Reviewable status: 4 of 9 files reviewed, 20 unresolved discussions (waiting on @kormat, @scrye, and @worxli)
go/lib/pathmgr/pathmgr.go, line 165 at r6 (raw file):
func (r *PR) QueryFilter(src, dst addr.IA, policy *pathpcy.Policy) spathmeta.AppPathSet { aps := r.Query(src, dst) // Delete paths that do not match the predicate
Needs updating.
go/lib/pathpcy/acl.go, line 27 at r6 (raw file):
and appends a match anything ACL
This seems like a bit of a trap. Why is this wanted?
I'd prefer to have an unbiased constructor that ensures that the last entry is a plain - or +, and errors if that condition is not met.
go/lib/pathpcy/acl.go, line 56 at r6 (raw file):
} } return Allow
This doesn't seem right. If nothing in the ACL matches, there's an implicit whitelist?
go/lib/pathpcy/acl.go, line 65 at r6 (raw file):
} } return Deny
Same issue here re: implicit default.
go/lib/pathpcy/policy.go, line 23 at r3 (raw file):
) var _ pktcls.Action = (*Policy)(nil)
Hmm. pktcls is getting rather far away from what this is doing, as this operates on path segments, not packets. We should consider moving some stuff out of pktcls.
go/lib/pathpcy/policy.go, line 15 at r6 (raw file):
// limitations under the License. package pathpcy
Package needs docstring.
go/lib/pathpcy/policy.go, line 32 at r6 (raw file):
ACL *ACL Sequence *Sequence Extends []*Policy
I'd put Extends directly after Name. It fits better with my mental model of "here's the base stuff, and then here's how we customize it".
go/lib/pathpcy/policy.go, line 40 at r6 (raw file):
inputSet := values.(spathmeta.AppPathSet) // Apply all extended policies p.applyExtended()
This should be done in a constructor, not in the critical path.
go/lib/pathpcy/policy.go, line 70 at r6 (raw file):
for i := len(p.Extends) - 1; i >= 0; i-- { policy := p.Extends[i] policy.applyExtended()
It feels like this should already have been done.
go/lib/pathpcy/policy.go, line 77 at r6 (raw file):
} // Replace options if (p.Options == nil || len(p.Options) == 0) &&
len() a nil slice is defined to return 0, so the first part of the check is redundant.
go/lib/sciond/types.go, line 319 at r3 (raw file):
func NewPathInterface(str string) (PathInterface, error) { tokens := strings.Split(str, "#") if len(tokens) != 2 {
Why not support the full form?
There was a problem hiding this comment.
Reviewable status: 4 of 9 files reviewed, 22 unresolved discussions (waiting on @kormat, @scrye, and @worxli)
doc/PathPolicy.md, line 109 at r1 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Done.
Wait I don't understand the example. Are the things under sub_pol_{1,2} only the extension to the respective policies or are they the actual policies? If the former, please include the base policies and if the latter then I don't understand what extends_example accomplishes.
doc/PathPolicy.md, line 107 at r7 (raw file):
behaviour
behavior
We should stick to American-style in our documentation
doc/PathPolicy.md, line 115 at r7 (raw file):
not know
not known
There was a problem hiding this comment.
Reviewable status: 4 of 9 files reviewed, 22 unresolved discussions (waiting on @kormat, @scrye, and @worxli)
doc/PathPolicy.md, line 12 at r3 (raw file):
Maybe add something like:
If a tail elements in an IFP are 0, they can be omitted. See the following examples for details.
doc/PathPolicy.md, line 30 at r7 (raw file):
- `?` (the preceding IFP may appear at most once) - `+` (the preceding IFP must appear at least once)
Might as well clarify that it's an "ISD-level IFP". Also, now that we've finally given in and added +, might as well * too.
doc/PathPolicy.md, line 37 at r7 (raw file):
## Policy A policy is defined by a policy object. It can have the following attributes:
This section is kinda hard to read. It claims to list the attributes, but the immediate list has only 2 entries. The other attributes are scattered around the rest of the section. As there are already sections later to explain extends and options, i'd just put them into the first list with a few words of description, and a link to the later section, where they can be explained properly.
doc/PathPolicy.md, line 71 at r7 (raw file):
The policy language can be in different formats, currently we want to add support for `YAML`, `JSON` and `TOML`.
I'm still not convinced by mentioning actual file formats. To me, the formatting in this doc is the equivalent of pseudo-code.
doc/PathPolicy.md, line 77 at r7 (raw file):
### ACL The ACL can be used to white- and blacklist ISDs, ASes and IFs. The first match wins and an explicit
This needs to be waaay clarified :)
doc/PathPolicy.md, line 78 at r7 (raw file):
If a policy has no
aclattribute (and doesn't inherit one from any policy it extends), then by default everything is whitelisted.
doc/PathPolicy.md, line 80 at r7 (raw file):
default action needs to be supplied. If the `acl` attribute is not used as part of a policy, by default everything is whitelisted.
Needs to state that an ACL must have a + or - last entry.
doc/PathPolicy.md, line 104 at r7 (raw file):
- sequence_example_1:
sequence: "1-ff00:0:133#0 1-ff00:0:120#2 1-ff00:0:120#1 0 0 1-ff00:0:110#0"
I'm not sure we want to support this approach, compared to the 2,1 approach below.
doc/PathPolicy.md, line 107 at r7 (raw file):
Previously, shitz wrote…
behaviourbehavior
We should stick to American-style in our documentation
🇮🇪
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7.
Reviewable status: 5 of 9 files reviewed, 22 unresolved discussions (waiting on @scrye, @worxli, and @shitz)
There was a problem hiding this comment.
Reviewable status: 5 of 9 files reviewed, 21 unresolved discussions (waiting on @scrye, @worxli, and @shitz)
go/lib/pathpcy/acl.go, line 65 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Same issue here re: implicit default.
This should never be reached. Once we have a proper constructor, this should become a panic.
There was a problem hiding this comment.
Reviewed 1 of 4 files at r6.
Reviewable status: 6 of 9 files reviewed, 28 unresolved discussions (waiting on @scrye, @worxli, and @shitz)
go/lib/pathpcy/policy.go, line 96 at r7 (raw file):
maxWeight := 0 // Go through sub policies for _, option := range p.Options {
I would sort Options by descending weight in the constructor, it simplifies the logic here.
go/lib/pathpcy/policy.go, line 120 at r7 (raw file):
} // Sequence is a list of path interfaces that a path should match
Does this need to be a struct, or could it just be a slice? That would remove all the if sequence == nil checks.
go/lib/pathpcy/policy.go, line 134 at r7 (raw file):
} func pathInterfaceFromToken(item string) sciond.PathInterface {
This function should not be between the constructor and methods for Sequence.
go/lib/pathpcy/policy.go, line 140 at r7 (raw file):
pi, err := sciond.NewPathInterface(item) if err != nil { panic(err)
This should not panic! This code is supposed to be parsing external data, malformed data should never cause a crash.
go/lib/pathpcy/policy.go, line 145 at r7 (raw file):
} func (sequence *Sequence) Length() int {
sequence is far too long for an method receiver var. s or seq would be much better.
go/lib/pathpcy/policy.go, line 156 at r7 (raw file):
func (sequence *Sequence) Eval(inputSet spathmeta.AppPathSet) spathmeta.AppPathSet { if sequence == nil || len(sequence.tokens) == 0 { return inputSet
The semantics are a bit weird - if a sequence is empty, it matches everything?
go/lib/pathpcy/policy.go, line 170 at r7 (raw file):
func pathMatches(pathInterfaces, matcherTokens []sciond.PathInterface) bool { if len(pathInterfaces) != len(matcherTokens) { return false
Wut?
There was a problem hiding this comment.
Reviewed 1 of 9 files at r1.
Reviewable status: 6 of 9 files reviewed, 28 unresolved discussions (waiting on @scrye, @worxli, and @shitz)
There was a problem hiding this comment.
Reviewable status: 6 of 9 files reviewed, 28 unresolved discussions (waiting on @scrye, @kormat, @worxli, and @shitz)
doc/PathPolicy.md, line 109 at r1 (raw file):
Previously, shitz wrote…
Wait I don't understand the example. Are the things under
sub_pol_{1,2}only the extension to the respective policies or are they the actual policies? If the former, please include the base policies and if the latter then I don't understand whatextends_exampleaccomplishes.
All of them are actual policies an can be used as such. Thus extends_example extends both the others and this inherits their acl and sequence. The extends_example policy can also have all attributes though.
doc/PathPolicy.md, line 12 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Maybe add something like:
If a tail elements in an IFP are 0, they can be omitted. See the following examples for details.
Done.
doc/PathPolicy.md, line 30 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Might as well clarify that it's an "ISD-level IFP". Also, now that we've finally given in and added
+, might as well*too.
Done.
doc/PathPolicy.md, line 37 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This section is kinda hard to read. It claims to list the attributes, but the immediate list has only 2 entries. The other attributes are scattered around the rest of the section. As there are already sections later to explain
extendsandoptions, i'd just put them into the first list with a few words of description, and a link to the later section, where they can be explained properly.
Done.
doc/PathPolicy.md, line 71 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'm still not convinced by mentioning actual file formats. To me, the formatting in this doc is the equivalent of pseudo-code.
Removed.
doc/PathPolicy.md, line 77 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This needs to be waaay clarified :)
Done.
doc/PathPolicy.md, line 78 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
If a policy has no
aclattribute (and doesn't inherit one from any policy it extends), then by default everything is whitelisted.
Done.
doc/PathPolicy.md, line 80 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Needs to state that an ACL must have a
+or-last entry.
Done.
doc/PathPolicy.md, line 104 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'm not sure we want to support this approach, compared to the
2,1approach below.
But this is the generic case... You should always be allowed to specify a list of interfaces?
doc/PathPolicy.md, line 107 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
🇮🇪
Done.
doc/PathPolicy.md, line 115 at r7 (raw file):
Previously, shitz wrote…
not knownot known
Done.
go/lib/pathmgr/pathmgr.go, line 165 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Needs updating.
Done.
go/lib/pathpcy/acl.go, line 27 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
and appends a match anything ACL
This seems like a bit of a trap. Why is this wanted?
I'd prefer to have an unbiased constructor that ensures that the last entry is a plain
-or+, and errors if that condition is not met.
Done.
go/lib/pathpcy/acl.go, line 56 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This doesn't seem right. If nothing in the ACL matches, there's an implicit whitelist?
As discussed.
go/lib/pathpcy/acl.go, line 65 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should never be reached. Once we have a proper constructor, this should become a panic.
Done.
go/lib/pathpcy/policy.go, line 23 at r3 (raw file):
pktcls is getting rather far away from what this is doing, as this operates on path segments, not packets. We should consider moving some stuff out of pktcls.
Added issue #1916
go/lib/pathpcy/policy.go, line 15 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Package needs docstring.
Done.
go/lib/pathpcy/policy.go, line 32 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd put
Extendsdirectly afterName. It fits better with my mental model of "here's the base stuff, and then here's how we customize it".
Done.
go/lib/pathpcy/policy.go, line 40 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should be done in a constructor, not in the critical path.
Done.
go/lib/pathpcy/policy.go, line 70 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It feels like this should already have been done.
With the new constructor, yes.
go/lib/pathpcy/policy.go, line 77 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
len()a nil slice is defined to return0, so the first part of the check is redundant.
Done.
go/lib/pathpcy/policy.go, line 96 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I would sort Options by descending weight in the constructor, it simplifies the logic here.
Done.
go/lib/pathpcy/policy.go, line 120 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Does this need to be a struct, or could it just be a slice? That would remove all the
if sequence == nilchecks.
Done.
go/lib/pathpcy/policy.go, line 134 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This function should not be between the constructor and methods for Sequence.
Done.
go/lib/pathpcy/policy.go, line 140 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should not panic! This code is supposed to be parsing external data, malformed data should never cause a crash.
Done.
go/lib/pathpcy/policy.go, line 145 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
sequenceis far too long for an method receiver var.sorseqwould be much better.
Done.
go/lib/pathpcy/policy.go, line 156 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The semantics are a bit weird - if a sequence is empty, it matches everything?
Empty or nil should have the same behavior. As in YAML e.g. that's the same thing?
go/lib/pathpcy/policy.go, line 170 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Wut?
Added comment.
go/lib/sciond/types.go, line 319 at r3 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Why not support the full form?
Done.
There was a problem hiding this comment.
Reviewed 1 of 14 files at r8.
Reviewable status: 2 of 16 files reviewed, 28 unresolved discussions (waiting on @kormat, @scrye, @shitz, and @worxli)
doc/PathPolicy.md, line 12 at r3 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Done.
It would be better if i hadn't ungrammared in my suggestion :) s/a tail/the tail/.
doc/PathPolicy.md, line 104 at r7 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
But this is the generic case... You should always be allowed to specify a list of interfaces?
To put it another way, if you're specifying both interfaces for a given AS, i'm proposing that you must use the 2,1 approach, and otherwise it's an error to have an AS appear twice in a given sequence expansion.
Thinking about it further, this syntax should also be supported in the acl section, so it should be described in the IFP section at the top.
doc/PathPolicy.md, line 6 at r8 (raw file):
but overlapping purposes. ## Interface Predicate (IFP)
Can we change this to "Hop Predicate"? It's no longer even specific to a given interface.
doc/PathPolicy.md, line 8 at r8 (raw file):
## Interface Predicate (IFP) An interface predicate is of the form **ISD-AS#IF**, whereas _0_ can be used as a wildcard for
s/whereas/where/
doc/PathPolicy.md, line 42 at r8 (raw file):
A policy is defined by a policy object. It can have the following attributes: - [`sequence`](#Sequence) (space separated list of IFPs, may contain operators)
In terms of ordering, i'd put:
- extends
- acl
- sequence
- options
which is roughly the order of importance.
doc/PathPolicy.md, line 66 at r8 (raw file):
The ACL can be used to deny (blacklist) or allow (whitelist) ISDs, ASes and IFs. A deny entry is of the following form `- ISD-AS#IF`, whereas the second part is an [IFP](#IFP). If a deny entry matches
s/whereas/where/
doc/PathPolicy.md, line 75 at r8 (raw file):
Every ACL must end with a blanket accept or deny (i.e.
+or-, or equivalent such as+ 0-0#0).
I think this is a bit clearer.
doc/PathPolicy.md, line 133 at r8 (raw file):
- extends_example:
I'd extend (heh) the example a bit to show an example of precedence.
doc/PathPolicy.md, line 175 at r8 (raw file):
- option_3: acl: - "- 1-0#0"
- 1
There was a problem hiding this comment.
Reviewed 1 of 14 files at r8.
Reviewable status: 3 of 16 files reviewed, 22 unresolved discussions (waiting on @kormat, @scrye, @shitz, and @worxli)
go/lib/pathpcy/policy.go, line 77 at r6 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Done.
The same applies to the second half :P
That said, if p.Options is empty, you don't actually care of policy.Options is empty or not, you can just assign it. Same goes for the p.Sequence code below.
go/lib/pathpcy/policy.go, line 156 at r7 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
Empty or nil should have the same behavior. As in YAML e.g. that's the same thing?
Yeah alright. In go we could detect the difference by using *Sequence in the Policy struct, but other languages may not have the option.
go/lib/pathpcy/policy.go, line 42 at r8 (raw file):
func NewPolicy(name string, extends []*Policy, acl *ACL, sequence Sequence, options []Option) *Policy {
Leave a blank line after multi-line function declarations.
go/lib/pathpcy/policy.go, line 116 at r8 (raw file):
if currWeight > option.Weight { subPolicySet = subPaths currWeight = option.Weight
The logic here can be simplified. Proposal:
var subPolicySet spathmeta.AppPathSet
...
if currWeight > option.Weight && len(subPolicySet) > 0 {
return subPolicySet
}
currWeight = option.Weight
subPaths := option.Policy.Act(inputSet).(spathmeta.AppPathSet)
if len(subPolicySet) == 0 {
subPolicySet = subPaths
} else {
for key, path := ...
}
go/lib/pathpcy/policy.go, line 139 at r8 (raw file):
// NewSequence creates a new sequence from a list of string tokens func NewSequence(tokens []string) (Sequence, error) { list := make(Sequence, 0)
I'd rename this from list to s, as that's the receiver var used in the methods.
There was a problem hiding this comment.
Reviewed 1 of 14 files at r8.
Reviewable status: 4 of 16 files reviewed, 24 unresolved discussions (waiting on @kormat, @scrye, @worxli, and @shitz)
go/lib/pathpcy/acl.go, line 30 at r8 (raw file):
func NewACL(entries ...*ACLEntry) (*ACL, error) { lastRule := entries[len(entries)-1].Rule if lastRule.IfID != 0 || lastRule.RawIsdas != 0 {
Hurm. RawIsdas indicates to me that we're using the wrong type. The ACL should be working on parsed IAs.
go/lib/pathpcy/acl.go, line 49 at r8 (raw file):
func (a *ACL) evalPath(path *spathmeta.AppPath) ACLAction { if a == nil || len(a.Entries) == 0 {
This should be in Eval, not evalPath.
go/lib/pathpcy/acl.go, line 62 at r8 (raw file):
func (a *ACL) evalInterface(iface sciond.PathInterface) ACLAction { for _, aclEntry := range a.Entries { if spathmeta.PPWildcardEquals(&iface, aclEntry.Rule) {
It seems odd that Entry.Path.Interfaces is []sciond.PathInterface, but PPWildcardEquals takes *sciond.PathInterfaces..
There was a problem hiding this comment.
Reviewable status: 5 of 16 files reviewed, 20 unresolved discussions (waiting on @scrye, @kormat, @shitz, and @worxli)
doc/PathPolicy.md, line 104 at r7 (raw file):
Previously, kormat (Stephen Shirley) wrote…
To put it another way, if you're specifying both interfaces for a given AS, i'm proposing that you must use the
2,1approach, and otherwise it's an error to have an AS appear twice in a given sequence expansion.Thinking about it further, this syntax should also be supported in the acl section, so it should be described in the IFP section at the top.
Done.
doc/PathPolicy.md, line 6 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Can we change this to "Hop Predicate"? It's no longer even specific to a given interface.
Done.
doc/PathPolicy.md, line 8 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
s/whereas/where/
Done.
doc/PathPolicy.md, line 42 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
In terms of ordering, i'd put:
- extends
- acl
- sequence
- options
which is roughly the order of importance.
Done.
doc/PathPolicy.md, line 66 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
s/whereas/where/
Done.
doc/PathPolicy.md, line 75 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Every ACL must end with a blanket accept or deny (i.e.
+or-, or equivalent such as+ 0-0#0).I think this is a bit clearer.
Done.
doc/PathPolicy.md, line 133 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd extend (heh) the example a bit to show an example of precedence.
Done.
doc/PathPolicy.md, line 175 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
- 1
Done.
go/lib/pathpcy/acl.go, line 49 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This should be in
Eval, notevalPath.
Done.
go/lib/pathpcy/acl.go, line 62 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
It seems odd that Entry.Path.Interfaces is []sciond.PathInterface, but PPWildcardEquals takes *sciond.PathInterfaces..
As it doesn't make a difference I changed it.
go/lib/pathpcy/policy.go, line 77 at r6 (raw file):
Previously, kormat (Stephen Shirley) wrote…
The same applies to the second half :P
That said, if
p.Optionsis empty, you don't actually care of policy.Options is empty or not, you can just assign it. Same goes for the p.Sequence code below.
Done.
go/lib/pathpcy/policy.go, line 42 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Leave a blank line after multi-line function declarations.
Done.
go/lib/pathpcy/policy.go, line 116 at r8 (raw file):
if len(subPolicySet) == 0 {
subPolicySet = subPaths
} else {
if len(subPolicySet) == 0 is not needed I think.
go/lib/pathpcy/policy.go, line 139 at r8 (raw file):
Previously, kormat (Stephen Shirley) wrote…
I'd rename this from
listtos, as that's the receiver var used in the methods.
Done.
worxli
force-pushed
the
pathmgr-policies
branch
2 times, most recently
from
df8d802 to
5ed743b
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 16 files reviewed, 11 unresolved discussions (waiting on @scrye, @kormat, @shitz, and @worxli)
go/lib/pathpcy/acl.go, line 30 at r8 (raw file):
The ACL should be working on parsed IAs.
Going to be fixed in a follow up PR.
There was a problem hiding this comment.
Reviewed 3 of 15 files at r11.
Reviewable status: 6 of 16 files reviewed, 9 unresolved discussions (waiting on @scrye, @kormat, @shitz, and @worxli)
go/lib/pathpcy/acl.go, line 15 at r11 (raw file):
// limitations under the License. package pathpcy
I've been trying to figure out why this package name bugged me - can we rename it to pathpol instead, please? :)
go/lib/pathpcy/acl.go, line 41 at r11 (raw file):
for key, path := range inputSet { // Check ACL if a == nil || len(a.Entries) == 0 || a.evalPath(path) {
The first two are not per input path, so they should be checked before the for loop.
go/lib/pathpcy/policy.go, line 116 at r8 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
if len(subPolicySet) == 0 {
subPolicySet = subPaths
} else {
if len(subPolicySet) == 0is not needed I think.
I had assumed the existing code was designed to avoid copying from a map to an empty map, so i preserved that behaviour :)
go/lib/pathpcy/policy.go, line 111 at r11 (raw file):
for _, option := range p.Options { if currWeight > option.Weight && len(subPolicySet) > 0 { return subPolicySet
Change this to break, as there's already a return subPolicySet at the end of the method.
go/lib/sciond/types.go, line 318 at r11 (raw file):
func NewPathInterface(str string) (PathInterface, error) { var iface PathInterface
My proposal:
func NewPathInterface(str string) (PathInterface, error) {
var iface PathInterface
var err error
var ia addr.IA
dashParts := strings.Split(str, "-")
ia.I, err = addr.ISDFromString(dashParts[0])
if err != nil {
return PathInterface{}, err // add some context.
}
iface.RawIsdas = ia.IAInt()
if len(dashParts) == 1 {
return iface, nil
}
if len(dashParts) != 2 {
return PathInterface{}, fmt.Errorf("Nah bro")
}
hashParts := strings.Split(dashParts[1], "#")
ia.A, err = addr.ASFromString(hashParts[0])
if err != nil {
return PathInterface{}, err // add some context.
}
iface.RawIsdas = ia.IAInt()
if len(hashParts) == 1 {
return iface, nil
}
if len(hashParts) != 2 {
return PathInterface{}, fmt.Errorf("Nah bro")
}
ifid, err := strconv.ParseUint(hashParts[1], 10, 64)
if err != nil {
return PathInterface{}, err // add some context.
}
iface.IfID = common.IFIDType(ifid)
if iface.IfID != 0 && ia.A == 0 {
return PathInterface{}, fmt.Errorf("Nah bro")
}
return iface, nil
}
5ed743b to
a948e88
Compare
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 6 unresolved discussions (waiting on @kormat, @scrye, and @worxli)
doc/PathPolicy.md, line 182 at r21 (raw file):
Previously, shitz wrote…
Ohhh this is is very tricky to parse. I thought the
extends: option_1belonged to the the second option, but now I realize that it's actually a third option with weight 0. Have you considered naming each optionoptionto make this clearer for a human?
Hm no, we wanted to not use keywords if not needed. Also I think it's quite clear because options is a list of policies and if wanted you could separate them by blank lines.
doc/PathPolicy.md, line 153 at r22 (raw file):
Previously, shitz wrote…
What does this do if the toplevel policy already defines an ACL? wouldn't this option just be ignored?
The toplevel ACL applies to all paths. When specifying options there is a choice, but one option in the list doesn't make much sense. In this case though it is not ignored, as the option's ACL is further refining the toplevel ACL.
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 5 unresolved discussions (waiting on @kormat, @scrye, and @shitz)
doc/PathPolicy.md, line 153 at r22 (raw file):
Previously, worxli (Lukas Bischofberger) wrote…
The toplevel ACL applies to all paths. When specifying options there is a choice, but one option in the list doesn't make much sense. In this case though it is not ignored, as the option's ACL is further refining the toplevel ACL.
It's not clear from the description that options are basically ANDed to the global predicates. Also, maybe don't include options already here. I don't think it's necessary for the example.
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 5 unresolved discussions (waiting on @kormat, @scrye, and @shitz)
doc/PathPolicy.md, line 153 at r22 (raw file):
Previously, shitz wrote…
It's not clear from the description that options are basically ANDed to the global predicates. Also, maybe don't include options already here. I don't think it's necessary for the example.
Done.
There was a problem hiding this comment.
Only reviewed the .md file, but that one
Reviewed 1 of 1 files at r23.
Reviewable status: 14 of 16 files reviewed, 4 unresolved discussions (waiting on @kormat and @scrye)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r21, 1 of 1 files at r23.
Reviewable status: 15 of 16 files reviewed, all discussions resolved (waiting on @scrye)
worxli
force-pushed
the
pathmgr-policies
branch
from
d7e500d to
739b40b
Compare
There was a problem hiding this comment.
Reviewed 15 of 18 files at r26.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @worxli)
go/lib/pathpol/policy_test.go, line 90 at r26 (raw file):
Src: xtest.MustParseIA("1-ff00:0:133"), Dst: xtest.MustParseIA("1-ff00:0:110"), ExpPathNum: 2,
This depends on an illegal path.
go/lib/pathpol/policy_test.go, line 101 at r26 (raw file):
{ Name: "Longer Explicit matching", List: newSequence(t, []string{"1-ff00:0:133#1018", "1-ff00:0:122#1810",
This is an illegal path, i wouldn't use it in a unit test.
go/lib/pathpol/policy_test.go, line 370 at r26 (raw file):
{Action: Deny, Rule: mustPathInterface(t, "1-ff00:0:110#0")}, {Action: Deny, Rule: mustPathInterface(t, "1-ff00:0:120#0")}, {Action: Deny, Rule: mustPathInterface(t, "1-ff00:0:111#2823")},
Looks like this depends on an illegal path.
There was a problem hiding this comment.
Reviewable status: 15 of 16 files reviewed, 3 unresolved discussions (waiting on @kormat)
go/lib/pathpol/policy_test.go, line 90 at r26 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This depends on an illegal path.
Done.
go/lib/pathpol/policy_test.go, line 101 at r26 (raw file):
Previously, kormat (Stephen Shirley) wrote…
This is an illegal path, i wouldn't use it in a unit test.
Done.
go/lib/pathpol/policy_test.go, line 370 at r26 (raw file):
Previously, kormat (Stephen Shirley) wrote…
Looks like this depends on an illegal path.
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r27.
Reviewable status:complete! all files reviewed, all discussions resolved
Co-authored-by: Sergiu Costea <[email protected]> Co-authored-by: Lukas Bischofberger <[email protected]>
worxli
force-pushed
the
pathmgr-policies
branch
from
e243019 to
8ebaeac
Compare
There was a problem hiding this comment.
Reviewed 15 of 18 files at r30.
Reviewable status:
This change is