-
Notifications
You must be signed in to change notification settings - Fork 228
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
fix initial setting of origin type for juniper #363
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on variable names, seems correct. Very minor q inline.
For my own edification, can you link me to some context about what we're handling here?
setOriginForNonBgp | ||
.getTrueStatements() | ||
BooleanExpr isNotBgp = new Not(isBgp); | ||
setOriginForNonBgp.setGuard(isNotBgp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reason for introducing isNotBgp
instead of inlining in setGuard
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, could just use falseStatements instead of trueStatements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new BGP advertisements, i.e. those created from non-BGP routes, an origin code must be set. Juniper sets it to IGP by default. The code to do that was there, but there were two bugs:
- The condition was inverted.
- The If statement was never added to the policy.
No description provided.