azure-vnet: Implement GET command #161
Conversation
There was a problem hiding this comment.
-
A runtime may call GET at any time; but if GET is called for a container before an ADD or after a DEL for that container, the plugin should return error 3 to indicate the container is unknown (see Well-known Error Codes section).
-
If the previous action for the container was ADD or GET, the runtime must add a prevResult field to the configuration JSON of the plugin (or all plugins in the chain), which must be the Result of that previous ADD or GET action in JSON format
Are we ensuring the above two conditions as per spec -> https://github.com/containernetworking/cni/blob/master/SPEC.md
| @@ -144,6 +150,18 @@ func (ep *endpoint) getInfo() *EndpointInfo { | |||
| Id: ep.Id, | |||
| IPAddresses: ep.IPAddresses, | |||
| Data: make(map[string]interface{}), | |||
| MacAddress: ep.MacAddress, | |||
| SandboxKey: ep.SandboxKey, | |||
| IfIndex: 0, // Azure CNI supports only one interface | |||
There was a problem hiding this comment.
why this value is hardcoded.? Whicn interface it points to?
There was a problem hiding this comment.
hardcoded this to 0 with Sushant. Let me touch base with him again if this needs to be a declared const.
| @@ -21,6 +21,8 @@ type endpoint struct { | |||
| MacAddress net.HardwareAddr | |||
| IPAddresses []net.IPNet | |||
| Gateways []net.IP | |||
| DNS DNSInfo | |||
There was a problem hiding this comment.
can we move Data to last field in structure..also plz group same datatypes together
cni/plugin.go
Outdated
| } | ||
|
|
||
| // Acquire store lock. | ||
| err = plugin.Store.Lock(true) |
There was a problem hiding this comment.
we started following this convention now.. in old code we dint change to this..but in new code we are following this
if err := function(); err != nil {
}
cni/plugin.go
Outdated
| } | ||
|
|
||
| config.Store = plugin.Store | ||
| } |
There was a problem hiding this comment.
there should be a blank line before return
| return err | ||
| } | ||
| } | ||
| plugin.Store = nil |
cni/network/plugin/main.go
Outdated
| }() | ||
|
|
||
| err = netPlugin.Plugin.InitializeKeyValueStore(&config) | ||
|
|
There was a problem hiding this comment.
remove blank and you can check for err in same line as i said below
There was a problem hiding this comment.
Done. Changed to this stype in other places as well
| @@ -177,3 +151,39 @@ func (plugin *Plugin) Error(err error) *cniTypes.Error { | |||
| func (plugin *Plugin) Errorf(format string, args ...interface{}) *cniTypes.Error { | |||
| return plugin.Error(fmt.Errorf(format, args...)) | |||
| } | |||
|
|
|||
| // Initialize key-value store | |||
| func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error { | |||
There was a problem hiding this comment.
Why do we need this? We are already saving the state of networkManager. What happens if we get a GET call before any ADD happens? Even in this case I don't think we need to initialize json store here, as we can print an empty result.
There was a problem hiding this comment.
This initialize is called from the main. We can either check for the operation type in main and then avoid initialization, or we can always initialize and do the operation to keep logic simple. i'm fine with either.
| } | ||
|
|
||
| // Uninitialize key-value store | ||
| func (plugin *Plugin) UninitializeKeyValueStore() error { |
cni/plugin.go
Outdated
| log.Printf("[cni] Failed to create store, err:%v.", err) | ||
| return err | ||
| } | ||
|
|
cni/network/network.go
Outdated
| Version: ipVersion, | ||
| Interface: &epInfo.IfIndex, | ||
| Address: ipAddresses, | ||
| Gateway: epInfo.Gateways[0], |
| @@ -177,3 +151,39 @@ func (plugin *Plugin) Error(err error) *cniTypes.Error { | |||
| func (plugin *Plugin) Errorf(format string, args ...interface{}) *cniTypes.Error { | |||
| return plugin.Error(fmt.Errorf(format, args...)) | |||
| } | |||
|
|
|||
| // Initialize key-value store | |||
| func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error { | |||
There was a problem hiding this comment.
This initialize is called from the main. We can either check for the operation type in main and then avoid initialization, or we can always initialize and do the operation to keep logic simple. i'm fine with either.
CNI spec has added a new method : GET which allows the runtime / orchestrator to query the current state of the container networking configuration. This change adds the implementation for the GET method in azure-vnet plugin. The supported version changes to 0.4.0 to support GET API.
This PR fixes #148