types: fix marshalling of omitted "interfaces" key in IPConfig JSON #477
Conversation
This file contains 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
Some libraries address this by providing a helper function, e.g. // Int returns a pointer to the int value passed in.
func Int(v int) *int {
return &v
}[ref I don't love it. But it's worth considering. |
|
We really have no other choice. It's an optional field. (something something type unions something rust something) |
dcbw
force-pushed
the
fix-interface-index-marshaling
branch
from
489e9e5
to
edbbde7
Compare
|
@rosenhouse yeah, that's the least bad option. I disliked having to ensure Interface: was -1, I feel like that would often be forgotten. Your way is better. PTAL thanks! |
squeed
reviewed
Jun 21, 2017
pkg/types/current/types.go
Outdated
| // IPConfig contains values necessary to configure an IP address on an interface | ||
| type IPConfig struct { | ||
| // IP version, either "4" or "6" | ||
| Version string | ||
| // Index into Result structs Interfaces list | ||
| Interface int | ||
| // Index into Result structs Interfaces list; -1 if no interface is given |
Plugins that don't have knowledge of interfaces, like host-local or other IPAM plugins, should not set the 'interfaces' key of their returned "Result" JSON. This should then not be translated into an interface index of 0, which it was due to the int marshaling and omitempty. Instead, ensure that an omitted 'interface' in JSON ends up being nil in the IPConfig structure, and that a nil ensures that no 'interfaces' key is present in the JSON. Yes, this means that plugins must call the 'current.Int(x)' method when setting the Interfaces member. Oh well. Fixes: containernetworking#404
dcbw
force-pushed
the
fix-interface-index-marshaling
branch
from
edbbde7
to
30ab805
Compare
rosenhouse
approved these changes
Jun 21, 2017
squeed
approved these changes
Jun 22, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Plugins that don't have knowledge of interfaces, like host-local or
other IPAM plugins, should not set the 'interfaces' key of their
returned "Result" JSON. This should then not be translated into
an interface index of 0, which it was due to the int marshaling and
omitempty.
Instead, ensure that an omitted 'interface' in JSON ends up being
-1 in the IPConfig structure, and that a -1 ensures that no 'interfaces'
key is present in the JSON.
Yes, this means that you must set Interface:-1 in IPConfig in your
plugin, but if you weren't already doing that and your plugin didn't
deal with interface indexes, you already had a problem.
@containernetworking/cni-maintainers
A bit ugly, but we can't also change the IPConfig.Interface field to a *int because then it's really, really icky to actually set that member, since you can't &0 and such. Thoughts?
Will require a plugins revendor and update as well.
Supercedes: #434
Fixes: #404