Allow user to specify default address pools for docker networks #36396
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package opts | ||
|
|
||
| import ( | ||
| "encoding/csv" | ||
| "encoding/json" | ||
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| types "github.com/docker/libnetwork/ipamutils" | ||
| ) | ||
|
|
||
| // PoolsOpt is a Value type for parsing the default address pools definitions | ||
| type PoolsOpt struct { | ||
|
There was a problem hiding this comment. Seems like we really want There was a problem hiding this comment. @selansen could you have a look at this one? (also why we need a pointer)? There was a problem hiding this comment. I had discussion with Brian over phone on this. when we expand this feature to add global scope we will be adding more attribute to this structure and hence keeping struct as it is. |
||
| values []*types.NetworkToSplit | ||
| } | ||
|
|
||
| // UnmarshalJSON fills values structure info from JSON input | ||
| func (p *PoolsOpt) UnmarshalJSON(raw []byte) error { | ||
| return json.Unmarshal(raw, &(p.values)) | ||
| } | ||
|
|
||
| // Set predefined pools | ||
| func (p *PoolsOpt) Set(value string) error { | ||
|
There was a problem hiding this comment. I have newly added 6 more integration tests now to cover all combination. There was a problem hiding this comment. added unit test also for Set function |
||
| csvReader := csv.NewReader(strings.NewReader(value)) | ||
| fields, err := csvReader.Read() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| poolsDef := types.NetworkToSplit{} | ||
|
|
||
| for _, field := range fields { | ||
| parts := strings.SplitN(field, "=", 2) | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid field '%s' must be a key=value pair", field) | ||
| } | ||
|
|
||
| key := strings.ToLower(parts[0]) | ||
| value := strings.ToLower(parts[1]) | ||
|
There was a problem hiding this comment. not a blocker (should check what we do for other options), but we should consider removing these, and be strict (i.e. only lowercase if that's what the option is named) |
||
|
|
||
| switch key { | ||
| case "base": | ||
| poolsDef.Base = value | ||
| case "size": | ||
| size, err := strconv.Atoi(value) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid size value: %q (must be integer): %v", value, err) | ||
| } | ||
| poolsDef.Size = size | ||
| default: | ||
| return fmt.Errorf("unexpected key '%s' in '%s'", key, field) | ||
| } | ||
| } | ||
|
|
||
| p.values = append(p.values, &poolsDef) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Type returns the type of this option | ||
| func (p *PoolsOpt) Type() string { | ||
| return "pool-options" | ||
| } | ||
|
|
||
| // String returns a string repr of this option | ||
| func (p *PoolsOpt) String() string { | ||
| pools := []string{} | ||
| for _, pool := range p.values { | ||
| repr := fmt.Sprintf("%s %d", pool.Base, pool.Size) | ||
| pools = append(pools, repr) | ||
| } | ||
| return strings.Join(pools, ", ") | ||
| } | ||
|
|
||
| // Value returns the mounts | ||
| func (p *PoolsOpt) Value() []*types.NetworkToSplit { | ||
| return p.values | ||
| } | ||
|
There was a problem hiding this comment. Ok, I just checked: what's needed to make diff --git a/opts/address_pools.go b/opts/address_pools.go
index 1e995c6779..d2cffc0ee5 100644
--- a/opts/address_pools.go
+++ b/opts/address_pools.go
@@ -60,7 +60,7 @@ func (p *PoolsOpt) Set(value string) error {
// Type returns the type of this option
func (p *PoolsOpt) Type() string {
- return "default-address-pool"
+ return "pool-options"
}
// String returns a string repr of this option
@@ -77,3 +77,10 @@ func (p *PoolsOpt) String() string {
func (p *PoolsOpt) Value() []*types.NetworkToSplit {
return p.values
}
+
+// Name returns the flag name of this option
+func (p *PoolsOpt) Name() string {
+ return "default-address-pools"
+}
+
+var _ NamedOption = &PoolsOpt{}(last line is optional, and only to verify we implement the Adding the above, and both of these will work: As flag (singular) In daemon.json (plural): {"default-address-pools":[{"base":"172.80.0.0/16","size":24},{"base":"172.90.0.0/16","size":24}]} |
||
|
|
||
| // Name returns the flag name of this option | ||
| func (p *PoolsOpt) Name() string { | ||
| return "default-address-pools" | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package opts // import "github.com/docker/docker/opts" | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestAddressPoolOpt(t *testing.T) { | ||
| poolopt := &PoolsOpt{} | ||
| var addresspool = "base=175.30.0.0/16,size=16" | ||
| var invalidAddresspoolString = "base=175.30.0.0/16,size=16, base=175.33.0.0/16,size=24" | ||
|
|
||
| if err := poolopt.Set(addresspool); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| if err := poolopt.Set(invalidAddresspoolString); err == nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
These flushes are pretty generic. Can we instead delete the rules that are needed?
Alternatively maybe start the tests in a custom network namespace to prevent leaking to other tests.
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.
one of the key test for this feature is to delete docker0 network and see if it comes up with new IP Subnetwork range. So we need this.
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.
But it's flushing the whole table.
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.
This is helper function and it is used only in test API. when we delete interface we clear NAT rules associated to have clear start for testing purpose. I did some code reading and looks like already same function is added in helper.go. Pls take a look at it
moby/integration/network/helpers.go
Line 31 in 18bfe3c
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.
If it works I guess it doesn't matter 🤷♂️