Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add ingress rule support to the GCE provider #6867
Conversation
|
Given that both open and close essentially generate a new list of firewall rules and then figure out what needs adding and deleting, would it be possible to share more of the implementation. Eg common helper to slurp up existing rules, each modifies the set separately, common helper to turn the set into new rules, compare it with existing rules and write them out again? Given what I just read about GCE firewall rules I'm a little concerned that when you call OpenPorts for MySQL that it will also open those ports for all machines that are in the same "network". I didn't quite see how we chose which firewall rules apply to which machine and the rules we seem to be creating are all Source based. I'll try to look deeper. |
|
I have intentions of generalising some for the logic for dealing with firewall rules.... but, I would rather land this bespoke and wait until we are in a position to modify other providers. It's easier to do well with more than one use case. Also, the other providers use a slightly different model and semantics so we still need to see what's applicable. My desire is to unblock the ability to start looking at the firewall worker as a main priority. The implementation does only filter based on the source of the packets, that is true. And right now as a first step, the source will be set to packets originating from any network in the consuming model. As the Juju modelling improves over time. and some of the network capabilities like binding/offering endpoints to spaces come along, we can tighten it up. Or am I missing the point of your concern? |
| + if err != nil { | ||
| + return rules, errors.Annotate(err, "bad ports from GCE") | ||
| + } | ||
| + for p := portRange.FromPort; p <= portRange.ToPort; p++ { |
axw
Feb 1, 2017
Member
This implementation of combining port ranges is not very efficient. It would be better to sort the ranges, and then join them.
Can you please split this out into a function in the network package:
func CombinePortRanges(...PortRange) []PortRangeThat way we can optimise the code in one place later if it becomes an issue, rather than have people copy & paste this between providers and have to update it everywhere.
| - return errors.Annotatef(err, "opening port(s) %+v", rules) | ||
| +// matchIgnoreSourceCIDRs returns the matching firewall name and true if any of the current | ||
| +// firewall ports (ignoring CIDR values) matches the ports to match. | ||
| +func matchIgnoreSourceCIDRs(currentPorts map[string]protocolPorts, match protocolPorts) (string, bool) { |
axw
Feb 1, 2017
Member
can this be named after what it does match, rather than what it doesn't? e.g. matchProtocolPorts
| + if !sourceCIDRMatch { | ||
| + // We already know ports don't match, so if CIDRs don't match either, we either | ||
| + // have a partial match or no match. | ||
| + // No matches are a no-np. Partial matches might required splitting firewall rules |
| + // have a partial match or no match. | ||
| + // No matches are a no-np. Partial matches might required splitting firewall rules | ||
| + // which is not supported at the moment. We'll return an error as it's better to | ||
| + // be overly cautious than accidentally leave ports open. The issue should occur |
| + // which is not supported at the moment. We'll return an error as it's better to | ||
| + // be overly cautious than accidentally leave ports open. The issue should occur | ||
| + // in practice unless people have manually played with the firewall rules. | ||
| + return errors.NotSupportedf("closing port(s) %v+ over non-matching rules", rules) |
| - return firewallList.Items[0], nil | ||
| + var result []*compute.Firewall | ||
| + for _, fw := range firewallList.Items { | ||
| + if fw.Name == namePrefix || strings.HasPrefix(fw.Name, namePrefix) { |
| @@ -196,3 +196,28 @@ func parsePortRange(portRange string) (PortRange, error) { | ||
| } | ||
| return result, nil | ||
| } | ||
| + | ||
| +// CombinePortRanges groups together all port ranges according to |
axw
Feb 1, 2017
Member
please call out in flashing capital letters that this only works on non-overlapping sets
| + | ||
| +// CombinePortRanges groups together all port ranges according to | ||
| +// protocol, and then combines then into contiguous port ranges. | ||
| +// NOTE: Juju only allows it's model to contain non-overlapping port ranges. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedJan 25, 2017
•
Edited 1 time
-
wallyworld
Jan 25, 2017
The GCE provider is updating to support the new ingress rules with source CIDRs.
The change looks bigger than is it because git decided the file move (ruleset.go from network -> provider/gce/google) was an add/remove.
The existing way the GCE provider managed port ranges was to explode them into individual ports. This is allowed for in the new implementation - we read in existing rules and collapse the port ranges. When we write out any changes, or create new rules port ranges are used.
We now create different firewalls corresponding to distinct CIDR sets to fit in with the GCE model. For the default 0.0.0.0/0 case, there is no change. As new CIDRs are added and removed, the firewall name will change to allow for distinct port ranges with their own CIDR set.
There's also a drive by tweak to the azure provider
QA
bootstrap GCE with older juju
expose an application
open ports 3100-3110
observe that the firewall rules are individual ports
upgrade juju
open ports 3120-3130
observe that the firewall is updated and now has port ranges 3100-3110,3120-3130
close ports 3100-3110
observe that the firewall is updated and now has port ranges 3120-3130