From e3380dea12884c2d213205733a52eb756a4a4eb9 Mon Sep 17 00:00:00 2001 From: Geoff Franks Date: Mon, 10 Jul 2017 17:04:30 -0400 Subject: [PATCH] Fix #201 by supporting `azs` key in subnets Previously only `az` was supported. Now when specifying multiple AZs in a subnet, all IPs from that subnet can be used in any instance_group/job that is in a zone that the subnet mentioned. This can lead to interesting scenarios when using mixes of multi-az subnets and single-az subnets, where different offsets can mean the same IP in a different zone, or the same index could mean different IPs in different zones. Try not to do this, as it will likely lead to confusion down the road. However, care is made to ensure that IPs are never re-used, regardless of what subnets/azs they were allowed to be used by. This should not affect any existing IP allocations, since previously the `azs` field wasn't looked at, and the old behaviors remain the same for `az` and no-azs. --- .../multi-azs-multi-underprovision.yml | 19 ++ .../static_ips/multi-azs-multi-zone-job.yml | 21 +++ assets/static_ips/multi-azs-one-zone-job.yml | 25 +++ .../multi-azs-same-index-different-ip.yml | 26 +++ .../multi-azs-same-ip-different-index.yml | 26 +++ .../multi-azs-same-ip-different-zones.yml | 26 +++ .../multi-azs-z2-underprovision.yml | 18 ++ ci/release_notes.md | 20 ++ cmd/spruce/main_test.go | 174 ++++++++++++++++++ op_static_ips.go | 77 ++++++-- 10 files changed, 413 insertions(+), 19 deletions(-) create mode 100644 assets/static_ips/multi-azs-multi-underprovision.yml create mode 100644 assets/static_ips/multi-azs-multi-zone-job.yml create mode 100644 assets/static_ips/multi-azs-one-zone-job.yml create mode 100644 assets/static_ips/multi-azs-same-index-different-ip.yml create mode 100644 assets/static_ips/multi-azs-same-ip-different-index.yml create mode 100644 assets/static_ips/multi-azs-same-ip-different-zones.yml create mode 100644 assets/static_ips/multi-azs-z2-underprovision.yml create mode 100644 ci/release_notes.md diff --git a/assets/static_ips/multi-azs-multi-underprovision.yml b/assets/static_ips/multi-azs-multi-underprovision.yml new file mode 100644 index 00000000..2d512882 --- /dev/null +++ b/assets/static_ips/multi-azs-multi-underprovision.yml @@ -0,0 +1,19 @@ + +jobs: +- name: static_z1 + instances: 1 + azs: [z1, z2, z3] + networks: + - name: net1 + static_ips: (( static_ips(16) )) # should fail saying pool is only 16 big + # definitely shouldn't say pool is 48 big + # (# azs * size of pool == 48 in this case) +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 diff --git a/assets/static_ips/multi-azs-multi-zone-job.yml b/assets/static_ips/multi-azs-multi-zone-job.yml new file mode 100644 index 00000000..5a81812b --- /dev/null +++ b/assets/static_ips/multi-azs-multi-zone-job.yml @@ -0,0 +1,21 @@ + +jobs: +- name: static_z1 + instances: 2 + azs: [z1, z2, z3] + networks: + - name: net1 + static_ips: (( static_ips(15, 16) )) # will grab 10.1.1.1 and 10.2.2.2 + # from the 2nd/3rd subnet definitions +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 + - azs: [z2] + static: + - 10.2.2.2 diff --git a/assets/static_ips/multi-azs-one-zone-job.yml b/assets/static_ips/multi-azs-one-zone-job.yml new file mode 100644 index 00000000..cc3d62bc --- /dev/null +++ b/assets/static_ips/multi-azs-one-zone-job.yml @@ -0,0 +1,25 @@ +jobs: +- azs: + - z1 + instances: 2 + name: static_z1 + networks: + - name: net1 + static_ips: (( static_ips(0, 15) )) +networks: +- name: net1 + subnets: + - azs: + - z1 + - z2 + - z3 + static: + - 10.0.0.1 - 10.0.0.15 + - azs: + - z1 + static: + - 10.1.1.1 + - azs: + - z2 + static: + - 10.2.2.2 diff --git a/assets/static_ips/multi-azs-same-index-different-ip.yml b/assets/static_ips/multi-azs-same-index-different-ip.yml new file mode 100644 index 00000000..71a5de13 --- /dev/null +++ b/assets/static_ips/multi-azs-same-index-different-ip.yml @@ -0,0 +1,26 @@ + +jobs: +- name: static_z1 + instances: 1 + azs: [z1] + networks: + - name: net1 + static_ips: (( static_ips(15) )) +- name: static_z2 + instances: 1 + azs: [z2] + networks: + - name: net1 + static_ips: (( static_ips(15) )) +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 + - azs: [z2] + static: + - 10.2.2.2 diff --git a/assets/static_ips/multi-azs-same-ip-different-index.yml b/assets/static_ips/multi-azs-same-ip-different-index.yml new file mode 100644 index 00000000..0601b120 --- /dev/null +++ b/assets/static_ips/multi-azs-same-ip-different-index.yml @@ -0,0 +1,26 @@ + +jobs: +- name: static_z1 + instances: 1 + azs: [z1, z2, z3] + networks: + - name: net1 + static_ips: (( static_ips(16) )) +- name: static_z2 + instances: 1 + azs: [z2] + networks: + - name: net1 + static_ips: (( static_ips(15) )) +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 + - azs: [z2] + static: + - 10.2.2.2 diff --git a/assets/static_ips/multi-azs-same-ip-different-zones.yml b/assets/static_ips/multi-azs-same-ip-different-zones.yml new file mode 100644 index 00000000..1de04305 --- /dev/null +++ b/assets/static_ips/multi-azs-same-ip-different-zones.yml @@ -0,0 +1,26 @@ + +jobs: +- name: static_z1 + instances: 1 + azs: [z1] + networks: + - name: net1 + static_ips: (( static_ips(14) )) +- name: static_z2 + instances: 1 + azs: [z2] + networks: + - name: net1 + static_ips: (( static_ips(14) )) +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 + - azs: [z2] + static: + - 10.2.2.2 diff --git a/assets/static_ips/multi-azs-z2-underprovision.yml b/assets/static_ips/multi-azs-z2-underprovision.yml new file mode 100644 index 00000000..9ffb29d4 --- /dev/null +++ b/assets/static_ips/multi-azs-z2-underprovision.yml @@ -0,0 +1,18 @@ + +jobs: +- name: static_z1 + instances: 1 + azs: [z2] + networks: + - name: net1 + static_ips: (( static_ips(15) )) # should fail to grab 10.1.1.1, giving + # an index offset error - z2 only has 15 IPs +networks: +- name: net1 + subnets: + - azs: [ z1, z2, z3 ] + static: + - 10.0.0.1 - 10.0.0.15 + - azs: [z1] + static: + - 10.1.1.1 diff --git a/ci/release_notes.md b/ci/release_notes.md new file mode 100644 index 00000000..b7299c66 --- /dev/null +++ b/ci/release_notes.md @@ -0,0 +1,20 @@ +# Bug Fixes + +- Fixes #201 by supporting `azs` key in subnets + + Previously only `az` was supported. Now when specifying + multiple AZs in a subnet, all IPs from that subnet can be + used in any instance-group/job that is in a zone that the + subnet mentioned. + + This can lead to interesting scenarios when using mixes of + multi-az subnets and single-az subnets, where different offsets + can mean the same IP in a different zone, or the same index could + mean different IPs in different zones. Try not to do this, as it will + likely lead to confusion down the road. However, care is made to ensure + that IPs are never re-used, regardless of what subnets/azs they were + allowed to be used by. + + This should not affect any existing IP allocations, since previously the + `azs` field wasn't looked at, and the old behaviors remain the same + for `az` and no-azs. diff --git a/cmd/spruce/main_test.go b/cmd/spruce/main_test.go index c7705ae3..261d5a3d 100644 --- a/cmd/spruce/main_test.go +++ b/cmd/spruce/main_test.go @@ -879,6 +879,180 @@ networks: `) }) + Convey("Issue #201 - using `azs` instead of `az` in subnets", func() { + Convey("jobs in only one zone can see the IPs of all subnets that mentioned that zone", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-one-zone-job.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, "") + So(stdout, ShouldEqual, `jobs: +- azs: + - z1 + instances: 2 + name: static_z1 + networks: + - name: net1 + static_ips: + - 10.0.0.1 + - 10.1.1.1 +networks: +- name: net1 + subnets: + - azs: + - z1 + - z2 + - z3 + static: + - 10.0.0.1 - 10.0.0.15 + - azs: + - z1 + static: + - 10.1.1.1 + - azs: + - z2 + static: + - 10.2.2.2 + +`) + }) + Convey("jobs in multiple zones can see the IPs of all subnets mentioning those zones", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-multi-zone-job.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, "") + So(stdout, ShouldEqual, `jobs: +- azs: + - z1 + - z2 + - z3 + instances: 2 + name: static_z1 + networks: + - name: net1 + static_ips: + - 10.1.1.1 + - 10.2.2.2 +networks: +- name: net1 + subnets: + - azs: + - z1 + - z2 + - z3 + static: + - 10.0.0.1 - 10.0.0.15 + - azs: + - z1 + static: + - 10.1.1.1 + - azs: + - z2 + static: + - 10.2.2.2 + +`) + }) + Convey("a z2-only job cannot see z1-only IPs", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-z2-underprovision.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, `1 error(s) detected: + - $.jobs.static_z1.networks.net1.static_ips: request for static_ip(15) in a pool of only 15 (zero-indexed) static addresses + + +`) + So(stdout, ShouldEqual, "") + }) + Convey("jobs with multiple zones see one copy of available IPs, rather than one copy per zone", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-multi-underprovision.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, `1 error(s) detected: + - $.jobs.static_z1.networks.net1.static_ips: request for static_ip(16) in a pool of only 16 (zero-indexed) static addresses + + +`) + So(stdout, ShouldEqual, "") + }) + Convey("edge case - same index used for different IPs with multi-az subnets", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-same-index-different-ip.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, "") + So(stdout, ShouldEqual, `jobs: +- azs: + - z1 + instances: 1 + name: static_z1 + networks: + - name: net1 + static_ips: + - 10.1.1.1 +- azs: + - z2 + instances: 1 + name: static_z2 + networks: + - name: net1 + static_ips: + - 10.2.2.2 +networks: +- name: net1 + subnets: + - azs: + - z1 + - z2 + - z3 + static: + - 10.0.0.1 - 10.0.0.15 + - azs: + - z1 + static: + - 10.1.1.1 + - azs: + - z2 + static: + - 10.2.2.2 + +`) + }) + Convey("edge case - dont give out same IP when specified in jobs with different zones", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-same-ip-different-zones.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, `1 error(s) detected: + - $.jobs.static_z2.networks.net1.static_ips: tried to use IP '10.0.0.15', but that address is already allocated to static_z1/0 + + +`) + So(stdout, ShouldEqual, "") + }) + Convey("edge case - don't give out same IP when using different offsets", func() { + os.Args = []string{"spruce", "merge", "../../assets/static_ips/multi-azs-same-ip-different-index.yml"} + stdout = "" + stderr = "" + + main() + So(stderr, ShouldEqual, `1 error(s) detected: + - $.jobs.static_z2.networks.net1.static_ips: tried to use IP '10.2.2.2', but that address is already allocated to static_z1/0 + + +`) + So(stdout, ShouldEqual, "") + }) + }) Convey("Empty operator works", func() { diff --git a/op_static_ips.go b/op_static_ips.go index 9bac7f63..dbf77366 100644 --- a/op_static_ips.go +++ b/op_static_ips.go @@ -66,6 +66,7 @@ func (StaticIPOperator) Dependencies(ev *Evaluator, _ []*Expr, _ []*tree.Cursor, // need all the az decls track("networks.*.subnets.*.az") + track("networks.*.subnets.*.azs") track("networks.*.subnets.*.static.*") // need all the job instance count decls @@ -114,7 +115,6 @@ func instances(ev *Evaluator, job *tree.Cursor) (int, error) { } func statics(ev *Evaluator) (map[string][]string, []string, error) { - az := "z1" // default AZ to use if none specified addrs := map[string][]string{} azs := []string{} @@ -141,13 +141,39 @@ func statics(ev *Evaluator) (map[string][]string, []string, error) { return addrs, azs, err } - // look for az definition + // list of azs associated with this specific subnet + // do not confuse with `azs`, which is a list of + // all `azs` for the network. + subnet_zones := []string{} + + // look for az definition in the `az` key c, _ = tree.ParseCursor(fmt.Sprintf("%s.az", r.String())) z, err := c.ResolveString(ev.Tree) if err == nil && len(z) > 0 { - az = z + azs = append(azs, z) // to preserve subnet ordering + subnet_zones = append(subnet_zones, z) + } + + // look for az definitions in the `azs` key + c, _ = tree.ParseCursor(fmt.Sprintf("%s.azs", r.String())) + os, err := c.Resolve(ev.Tree) + if err == nil { + if zs, ok := os.([]interface{}); ok { + for _, o := range zs { + if z, ok := o.(string); ok && len(z) > 0 { + azs = append(azs, z) + subnet_zones = append(subnet_zones, z) + } + } + } + } + + // add a default zone for azs + subnet zones, if + // this network has no zones specified + if len(subnet_zones) == 0 { + azs = append(azs, "z1") + subnet_zones = append(subnet_zones, "z1") } - azs = append(azs, az) // to preserve subnet ordering c, err = tree.ParseCursor(fmt.Sprintf("%s.static.*", r.String())) if err != nil { @@ -178,23 +204,29 @@ func statics(ev *Evaluator) (map[string][]string, []string, error) { return nil, azs, ansi.Errorf("@c{%s}@R{: not a valid IP address}", segments[0]) } - addrs[az] = append(addrs[az], start.String()) - if len(segments) == 1 { - continue + for _, az := range subnet_zones { + addrs[az] = append(addrs[az], start.String()) + if len(segments) == 1 { + continue + } } - end := net.ParseIP(segments[1]) - if end == nil { - return nil, azs, ansi.Errorf("@c{%s}@R{: not a valid IP address}", segments[1]) - } + if len(segments) == 2 { + end := net.ParseIP(segments[1]) + if end == nil { + return nil, azs, ansi.Errorf("@c{%s}@R{: not a valid IP address}", segments[1]) + } - if binary.BigEndian.Uint32(start.To4()) > binary.BigEndian.Uint32(end.To4()) { - return nil, azs, ansi.Errorf("@R{Static IP pool }@c{[%s - %s]} @R{ends before it starts}", start, end) - } + if binary.BigEndian.Uint32(start.To4()) > binary.BigEndian.Uint32(end.To4()) { + return nil, azs, ansi.Errorf("@R{Static IP pool }@c{[%s - %s]} @R{ends before it starts}", start, end) + } - for !start.Equal(end) { - incrementIP(start, len(start)-1) - addrs[az] = append(addrs[az], start.String()) + for !start.Equal(end) { + incrementIP(start, len(start)-1) + for _, az := range subnet_zones { + addrs[az] = append(addrs[az], start.String()) + } + } } } } @@ -203,12 +235,19 @@ func statics(ev *Evaluator) (map[string][]string, []string, error) { func allIPs(pools map[string][]string, azs []string) []string { var ips []string + seen := map[string]bool{} + for _, az := range azs { pool, ok := pools[az] if !ok { continue } - ips = append(ips, pool...) + for _, ip := range pool { + if !seen[ip] { + ips = append(ips, ip) + seen[ip] = true + } + } } return ips } @@ -394,7 +433,7 @@ func (s StaticIPOperator) Run(ev *Evaluator, args []*Expr) (*Response, error) { } } if !found { - DEBUG(" specified az %s is not in stance_groups azs %v\n", az, azs) + DEBUG(" specified az %s is not in instance_groups azs %v\n", az, azs) return nil, ansi.Errorf("@R{could not find AZ} @c{%s} @R{in instance_groups AZS} @c{%v}", az, azs) }