-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[GLBC] Update firewall source ranges if outdated #574
Conversation
@aledbf You're welcome to review any PR of mine even if I assign it to others. |
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.
looks good, mostly minor things
existingCIDRs := sets.NewString(rule.SourceRanges...) | ||
|
||
// Do not update if ports and source cidrs are not outdated. | ||
// NOTE: We are not checking if nodeNames matches the firwall targetTags |
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.
sp. firwall
|
||
// Do not update if ports and source cidrs are not outdated. | ||
// NOTE: We are not checking if nodeNames matches the firwall targetTags | ||
if requiredPorts.Equal(existingPorts) && requiredCIDRs.Equal(existingCIDRs) { | ||
return nil |
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.
glog.V(4).Infof("No changes...
return nil | ||
} | ||
|
||
glog.V(3).Infof("Firewall rule %v already exists, updating nodeports %v", name, nodePorts) | ||
return fr.cloud.UpdateFirewall(suffix, "GCE L7 firewall rule", fr.srcRanges, nodePorts, nodeNames) | ||
} | ||
|
||
// Shutdown shuts down this firewall rules manager. | ||
func (fr *FirewallRules) Shutdown() error { | ||
glog.Infof("Deleting firewall rule with suffix %v", fr.namer.FrSuffix()) |
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.
[minor] technically multiple rule(s)?
@@ -0,0 +1,93 @@ | |||
package firewalls |
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.
copyright
} | ||
verifyFirewallRule(fwp, ruleName, nodePorts, nodes, l7SrcRanges, t) | ||
|
||
all := "0.0.0.0/0" |
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.
const
Addresses the second half of #197
Final task for that issue would be to consume the firewall rules from the GCE cloudprovider. Currently waiting on a PR over there...