Skip to content

Conversation

@sharmasushant
Copy link
Contributor

This PR contains implementation for container network service (CNS).
CNS is a service that exposes a set of REST API calls to support/manage networking for containers in Windows and Linux. It currently requires docker daemon to be running on the node, as well as azure-vnet plugins for IPAM and Network.
CNS is not yet integrated in build path. As such, it does not get built by default.

Supported Scenarios: Windows/Azure
Future Scenarios: Linux/Azure, Windows/Overlay, Linux/Overlay

Copy link
Contributor

@ofiliz ofiliz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intermediate feedback. This code should either be in its own repository, or be renamed to sf package. It doesn't fit with the rest of the code in the repo.

// Copyright 2017 Microsoft. All rights reserved.
// MIT License

package common
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to the same package as the rest of these changes. No need to put it in common. Nobody else uses this object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically a duplicate of the base plugin class renamed to service. We should really move it to your directory, rest of the code doesn't need it.

cns/api.go Outdated

// SetEnvironmentRequest describes the Request to set the environment in CNS.
type SetEnvironmentRequest struct {
Location string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you run "go fmt" on these files?

@@ -0,0 +1,198 @@
// Copyright 2017 Microsoft. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should convert these fuctions to tests, which clients can then learn from. Or maybe convert to documentation.


// GetPrimaryInterfaceInfoFromHost retrieves subnet and gateway of primary NIC from Host.
func (imdsClient *ImdsClient) GetPrimaryInterfaceInfoFromHost() (*InterfaceInfo, error) {
log.Printf("[Azure CNS] GetPrimaryInterfaceInfoFromHost")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"[Azure CNS]": Too long. Why not just call it "[sf]"? This is in reality a proxy that implements a custom service fabric interface.

@@ -0,0 +1,73 @@
package ipamclient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we even have this file? All these definitions are duplicates from ipam.api, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those request and response structures are not exposed to public. That's why I duplicated it.

package ipamclient

const (
defaultIpamPluginURL = "http://localhost:48080"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not a good idea to hard code this. You should read it from the plugin spec file. This will also allow you to remove ipamclient_windows vs _linux versions. If the spec file does not exist, it is Linux.

mux.ServeHTTP(w, req)
return w
}
func TestSetEnvironment(t *testing.T) {
Copy link
Contributor

@ofiliz ofiliz Jun 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add blank line. Multiple instances below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants