ENI Plugin Implementation #1

aaithal · 2017-02-21T19:51:12Z

Summary

The ENI plugin assigns the ENI to a container's namespace and sets it up as per it's subnet configuration parameters. The Add() operation sets up the ENI in a container's namespace. Del() tears it down. This PR implements Add().

Implementation Details

pkg mostly contains interfaces around various dependent packages to ease testing
plugins/eni contains the implementation of the ENI plugin
- plugins/eni/commands contains the implementation of Add and Del commands
- plugins/eni/engine contains the implementation of the execution engine of the plugin

Testing Done

Unit tests executed successfully

$ make unit-tests
go test -cover -timeout 10s ./pkg/... ./plugins/...
?       github.com/aws/amazon-ecs-cni-plugins/pkg/cninswrapper  [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/cninswrapper/mocks    [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/cninswrapper/mocks_netns      [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/ec2metadata   [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/ec2metadata/mocks     [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/ioutilwrapper [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/ioutilwrapper/mocks_fileinfo  [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/ioutilwrapper/mocks_ioutilwrapper     [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper        [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper/mocks  [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/pkg/netlinkwrapper/mocks_link     [no test files]
?       github.com/aws/amazon-ecs-cni-plugins/plugins/eni       [no test files]
ok      github.com/aws/amazon-ecs-cni-plugins/plugins/eni/commands      0.005s  coverage: 94.9% of statements
ok      github.com/aws/amazon-ecs-cni-plugins/plugins/eni/engine        0.006s  coverage: 98.8% of statements
?       github.com/aws/amazon-ecs-cni-plugins/plugins/eni/engine/mocks  [no test files]
ok      github.com/aws/amazon-ecs-cni-plugins/plugins/eni/types 0.010s  coverage: 100.0% of statements
ok      github.com/aws/amazon-ecs-cni-plugins/plugins/eni/version/cnispec       0.004s  coverage: 100.0% of statements

Ran end-to-end test manually:
- Run nginx on an instance in the subnet (This will be used the server that we'll try to reach from the container
- Run container on the instance with ENI attached with the ENI plugin and ensured that it's configured correctly and is able to query the nginx server

# Run nginx on an instance
$ ip addr show eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9001 qdisc mq state UP group default qlen 1000
    link/ether 06:2e:a4:f8:02:05 brd ff:ff:ff:ff:ff:ff
    inet 10.0.1.125/26 brd 10.0.1.127 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::42e:a4ff:fef8:205/64 scope link
       valid_lft forever preferred_lft forever

$ docker run -P -d --name nginx --net host nginx

# Run container on the instance with ENI attached with the ENI plugin
$ cat /etc/cni/net.d/10-mynet.conf
{
        "cniVersion": "0.2.0",
        "name": "mynet",
        "type": "eni",
        "eni": "eni-5cbae11d",
        "ipv4-address": "10.0.1.121",
        "mac":"06:9b:7b:4a:5b:8d"
}

# busybox to print devices and routes
$ CNI_PATH=`pwd`
$ sudo CNI_PATH=$CNI_PATH ./docker-run.sh --rm -it busybox:latest
/ # ip addr
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue qlen 1
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 9001 qdisc mq qlen 1000
    link/ether 06:2b:19:31:d0:b3 brd ff:ff:ff:ff:ff:ff
    inet 10.0.1.121/26 scope global eth1
       valid_lft forever preferred_lft forever
    inet6 fe80::42b:19ff:fe31:d0b3/64 scope link
       valid_lft forever preferred_lft forever
/ # ip route
10.0.1.64/26 dev eth1  src 10.0.1.121

# curl to query the other instance in the subnet
$ sudo CNI_PATH=$CNI_PATH ./docker-run.sh --rm  tutum/curl curl 10.0.1.125
<!DOCTYPE html>
<html>
<head>
<title>Welcome to nginx!</title>
<style>
    body {
        width: 35em;
        margin: 0 auto;
        font-family: Tahoma, Verdana, Arial, sans-serif;
    }
</style>
</head>
<body>
<h1>Welcome to nginx!</h1>
<p>If you see this page, the nginx web server is successfully installed and
working. Further configuration is required.</p>

<p>For online documentation and support please refer to
<a href="http://nginx.org/">nginx.org</a>.<br/>
Commercial support is available at
<a href="http://nginx.com/">nginx.com</a>.</p>

<p><em>Thank you for using nginx.</em></p>
</body>
</html>

# list all 'dhclient' processes before and after invoking the plugin:
# before
$ ps aux | grep dhclient
root      2218  0.0  0.0  10976  3704 ?        Ss   Feb22   0:04 /sbin/dhclient -6 -nw -lf /var/lib/dhclient/dhclient6-eth0.leases -pf /var/run/dhclient6-eth0.pid eth0
root     12604  0.0  0.0   9364  1308 ?        Ss   22:52   0:00 /sbin/dhclient -q -lf /var/lib/dhclient/dhclient-eth0.leases -pf /var/run/dhclient-eth0.pid eth0
root     18067  0.0  0.0   9364  1428 ?        Ss   23:38   0:00 /sbin/dhclient -q -cf /etc/dhcp/dhclient-eth1.conf -lf /var/lib/dhclient/dhclient-eth1.leases -pf /var/run/dhclient-eth1.pid eth1
root     18169  0.0  0.0   9364  1984 ?        Ss   23:38   0:00 /sbin/dhclient -6 -nw -lf /var/lib/dhclient/dhclient6-eth1.leases -pf /var/run/dhclient6-eth1.pid eth1

# after
$ ps aux | grep dhclient
root      2218  0.0  0.0  10976  3704 ?        Ss   Feb22   0:04 /sbin/dhclient -6 -nw -lf /var/lib/dhclient/dhclient6-eth0.leases -pf /var/run/dhclient6-eth0.pid eth0
root     12604  0.0  0.0   9364  1308 ?        Ss   22:52   0:00 /sbin/dhclient -q -lf /var/lib/dhclient/dhclient-eth0.leases -pf /var/run/dhclient-eth0.pid eth0
root     18422  0.0  0.0   9364  1376 ?        Ss   23:38   0:00 dhclient -q -lf /var/lib/dhclient/ns-dhclient-eth1.leases -pf /var/run/ns-dhclient-eth1.pid eth1

The logic of spec compatibility is moved to this package from a hardcoded string in the main package

…s in tests

richardpen · 2017-02-22T23:08:14Z

plugins/eni/version/cnispec/spec_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetSpecVersionsSupported(t *testing.T) {


I think the test shouldn't depend on the value in the main package which may change in the future.
If you want to test the GetSpecVersionsSupported, you should assign specVersionSupported some random value and then test against the value. Please correct me if I'm misunderstood.

Agreed, will modify this.

richardpen · 2017-02-22T23:08:14Z

plugins/eni/version/cnispec/spec.go

+// * VERSION
+// Refer to https://github.com/containernetworking/cni/blob/master/SPEC.md
+// for details
+var specVersionSupported = version.PluginSupports("0.2.0")


Why not start from 0.0.0?

As per the description, this plugin supports add, del and version commands, which means that it's not compatible with anything less than that. 0.3.0 would mean supporting additional things, which is why this is set to 0.2.0 for now.

richardpen · 2017-02-22T23:08:14Z

plugins/eni/types/types.go

+		return nil, errors.Wrapf(err, "Failed to parse config")
+	}
+	if conf.ENIID == "" {
+		return nil, fmt.Errorf("Missing required parameter in config: '%s'", "eni")


Since you are using errors package, you can also use errors here: errors.Errorf()

yes, will modify this.

richardpen · 2017-02-22T23:08:14Z

plugins/eni/types/types.go

+	if conf.ENIID == "" {
+		return nil, fmt.Errorf("Missing required parameter in config: '%s'", "eni")
+	}
+	if conf.IPV4Address == "" {


I think we should also verify the format of the address.

Fair enough

richardpen · 2017-02-22T23:08:14Z

plugins/eni/engine/engine.go

+	var macAddresses []string
+	macs, err := engine.metadata.GetMetadata(metadataNetworkInterfacesPath)
+	if err != nil {
+		return macAddresses, errors.Wrapf(err, "Error getting all mac addresses on the instance from instance metadata")


errors.Wrap? Also won't nil work here for macAddresses?

It would, i can change it.

richardpen · 2017-02-22T23:08:14Z

plugins/eni/engine/engine.go

+		}
+	}
+
+	return "", fmt.Errorf("Network device name not found for '%s'", macAddress)


errors.Errorf?

richardpen · 2017-02-22T23:08:14Z

plugins/eni/engine/engine.go

+func (engine *engine) GetInterfaceDeviceName(macAddress string) (string, error) {
+	files, err := engine.ioutil.ReadDir(sysfsPathForNetworkDevices)
+	if err != nil {
+		return "", errors.Wrapf(err, "Error listing network devices from sys fs")


errors.Wrap?

richardpen · 2017-02-22T23:08:14Z

plugins/eni/engine/engine.go

+const (
+	metadataNetworkInterfacesPath               = "network/interfaces/macs/"
+	metadataNetworkInterfaceIDPathSuffix        = "interface-id"
+	sysfsPathForNetworkDevices                  = "/sys/class/net/"


Will this stays the same on ubuntu/coreOS?

i've verified that it's the same on ubuntu. @nmeyerhans thinks that it should be the same path as well on other distributions as it's the sysfs mount and should remain consistent.

It's probably the same for most distributions. Let's leave it as-is for now and change it if we discover a distribution that is different.

richardpen · 2017-02-22T23:08:14Z

plugins/eni/engine/engine.go

+	}
+
+	parts = strings.Split(cidr, ".")
+	if len(parts) != 4 {


I think we can use a regex to match the ip, like: [0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}

We can do better. We can just parse it using the ParseAddr api from netlink

richardpen · 2017-02-22T23:08:14Z

Makefile

@@ -0,0 +1,18 @@
+SOURCEDIR=./pkg ./plugins


When do we include the dependency inside this repo?

what do you mean?

I mean put the two dependencies in the vendor folder, so that it can be built?

I'm adding dependencies in a separate PR. Otherwise, it'll make this unreadable.

…ith methods from the net package

richardpen · 2017-02-23T21:10:47Z

plugins/eni/types/types_test.go

+		StdinData: []byte(configInvalidIPV4AddressIPv6),
+	}
+	_, err := NewConf(args)
+	t.Log(err)


Remove this line?

Yes, it slipped in somehow.

richardpen · 2017-02-23T21:10:47Z

scripts/mockgen.go

+	path, _ := filepath.Split(outputPath)
+	err := os.MkdirAll(path, os.ModeDir|0755)
+	if err != nil {
+		fmt.Println(err)


Can we wrapper this two line in a function, since it occurs multiple times?

richardpen · 2017-02-23T21:10:47Z

plugins/eni/types/types.go

+	if conf.ENIID == "" {
+		return nil, errors.Errorf("Missing required parameter in config: '%s'", "eni")
+	}
+	if conf.IPV4Address == "" {


Can this check combine with the next if?

I'll add more text into the error message. I want to keep this separate so that we can tell explicitly which parameter is missing.

Looked at the code again. It does print generate different error messages. I'll leave it as is.

samuelkarp · 2017-02-26T08:42:02Z

Makefile

+	go get golang.org/x/tools/cmd/goimports
+	go get github.com/tools/godep
+
+.PHONY: plugins


Can you make a non-PHONY target that corresponds to the generated binary? See $(LOCAL_BINARY) in https://github.com/awslabs/amazon-ecr-credential-helper/blob/master/Makefile for an example.

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/commands/commands.go

+		ec2metadata.NewEC2Metadata(), ioutilwrapper.NewIOUtil(), netlinkwrapper.NewNetLink(), cninswrapper.NewNS()))
+}
+
+// Del invokes the command to remove ENI to a container's namespace


nit: should be "from a container's namespace"

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/engine/engine.go

+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package engine


Should we have a different package name? Not that I have any specific suggestions, just curious.

Couldn't think of a better name either. How does workflow sound compared to this?

I think that's somewhat better. Still going to see if I can come up with other suggestions too.

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/commands/commands.go

+	if !ok {
+		errorMessage := "Unable to map ipv4 address of ENI to a mac address"
+		log.Error(errorMessage)
+		return errors.Errorf(errorMessage)


errors should begin with lower-case and some contextual information. Maybe "add: " + errorMessage.

I read https://golang.org/doc/effective_go.html#errors this morning. I'm mortified that I've written so many bad error messages till now (especially in terms of lacking source and package information). Will rectify this!

When feasible, error strings should identify their origin, such as by having a prefix naming the operation or package that generated the error. For example, in package image, the string representation for a decoding error due to an unknown format is "image: unknown format".

Me too. I became more aware of this recently at the Seattle Go Programmers meetup/Go 1.8 release party.

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/engine/engine.go

+const (
+	metadataNetworkInterfacesPath               = "network/interfaces/macs/"
+	metadataNetworkInterfaceIDPathSuffix        = "interface-id"
+	sysfsPathForNetworkDevices                  = "/sys/class/net/"


It's probably the same for most distributions. Let's leave it as-is for now and change it if we discover a distribution that is different.

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/engine/engine.go

+}
+
+// NewEngine creates a new Engine object
+func NewEngine(metadata ec2metadata.EC2Metadata, ioutil ioutilwrapper.IOUtil, netLink netlinkwrapper.NetLink, ns cninswrapper.NS) Engine {


nit: engine.New to avoid the stutter

samuelkarp · 2017-02-26T08:42:02Z

plugins/eni/engine/engine.go

+
+// NewEngine creates a new Engine object
+func NewEngine(metadata ec2metadata.EC2Metadata, ioutil ioutilwrapper.IOUtil, netLink netlinkwrapper.NetLink, ns cninswrapper.NS) Engine {
+	return &engine{


Should New just create the wrappers instead of having them passed in? Are you using this for more than just unit tests of the engine package?

I'll add another level of indirection here.

…/exec

Made New() a wrapper that creates the necessary fields for the 'engine' obect. Also modified the 'Makefile' so that the eni executable is built as a non PHONY target

https://golang.org/doc/effective_go.html#errors

Added IsDHClientInPath() method to the Engine interface. Updated engine create() method and tests accordingly

…h ENI Modify namespace closure execution to invoke dhclient to start renewing the lease on the IPV4 address of the ENI

richardpen · 2017-03-01T18:15:39Z

plugins/eni/engine/engine.go

@@ -56,7 +56,12 @@ type engine struct {
 }

 // NewEngine creates a new Engine object


richardpen

Lgtm, just minor comments.

richardpen · 2017-03-01T19:06:10Z

plugins/eni/commands/commands.go

@@ -38,6 +45,11 @@ func add(args *skel.CmdArgs, engine engine.Engine) error {
 		return err
 	}

+	if ok := engine.IsDHClientInPath(); !ok {
+		log.Errorf("Unable to fing the dhclient executable")


nit: log.Error("Unable to find the dhclient executable")

richardpen · 2017-03-01T19:06:10Z

plugins/eni/commands/commands.go

@@ -21,6 +21,13 @@ import (
 	"github.com/pkg/errors"
 )

+var (
+	unmappedIPV4AddressError = errors.Errorf(


Why not just reuse the newUnmappedIPV4AddressError in engine/errors. Also it should be errors.New.

That's a type and this is a variable, both intended to be used for different things. Does it matter if it's errors.New() or errors.Errorf()? I'm not sure I understand the difference between the two in terms of creating error objects from strings.

richardpen · 2017-03-01T19:06:10Z

plugins/eni/commands/commands.go

-		err = newUnmappedIPV4AddressError("add", "commands", "unable to map ipv4 address of ENI to a mac address")
-		log.Error("Unable to validate ipv4 address for ENI: %v", err)
-		return err
+		log.Error("Unable to validate ipv4 address for ENI: %v", unmappedIPV4AddressError)


nit: Errorf

richardpen · 2017-03-01T19:06:10Z

plugins/eni/engine/engine.go

+	// expect the Agent to create those directories. We can also let
+	// these be conigured via the plugin config.
+	dhclientV4LeaseFilePathPrefix    = "/var/lib/dhclient/ns-dhclient"
+	dhclientV4LeasePIDFilePathPrefix = "/var/run/ns-dhclient"


Are these different in container namespace, as I get /var/run/dhclient-xxx on the host?

The mount namespace is same as where it's being invoked from. If the plugin is invoked on the host using scripts in cni package, it'd be the mount namespace of the host. If the plugin is invoked from the ECS Agent container, it'd be the mount namespace of the agent.

Are we planning to invoke this from the ECS agent container?

aaithal · 2017-03-01T19:19:43Z

@richardpen Thanks for reviewing this. I'll address your comments in a new PR as I want to merge this today. I don't want this one to drag on as I'm continuing to make more changes.

@vsiddharth @samuelkarp Feel free to add comments in this PR. I'll address them in a separate PR.

samuelkarp

Looks good. I have a bunch of nitpicks, but none of these are critical to address immediately.

samuelkarp · 2017-03-23T07:50:08Z

Makefile

+	go generate -x ./pkg/... ./plugins/...
+
+.PHONY: unit-tests
+unit-tests: $(SOURCES)


nit: I would probably prefer test here since I find it slightly more natural to type make test. But this is not a big deal.

We'll get this when integrate #3 in.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/commands/commands.go

+)
+
+var (
+	unmappedIPV4AddressError = errors.Errorf(


nit: These should be errors.New since there is no format string here.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/commands/commands.go

+
+var (
+	unmappedIPV4AddressError = errors.Errorf(
+		"add commands: unable to map ipv4 address of ENI to a mac address")


nit: I think I'd prefer starting these with "eni add" since that encapsulates the plugin name as well as the operation and I assume these errors become visible outside the plugin.

If we use different log locations for different plugins, we might not need this. We can come back to this.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/commands/commands.go

+	// TODO: If we can get this information from the config, we can optimize
+	// the workflow by getting rid of this, or by making this optional (only
+	// in cases where mac address hasn't been specified)
+	allMACAddresses, err := engine.GetAllMACAddresses()


nit: I think we're mixing abstraction layers here and it would be nice to get them separate early on. In order to add, you need to obtain the requisite information and then call engine.SetupContainerNamespace, and in order to call that you need the MAC, device name, and subnet mask. It seems trivial, but I think it'd be good to move the MAC resolution + validation out to a separate function so that add just looks like it gathers the necessary arguments to setup the namespace and then calls in to actually set it up.

func getMACforENI(eniID string, ipv4address string, engine engine.Engine) (string,nil) { allMACAddresses, err := engine.GetAllMACAddresses() //... macAddressOfENI, err := engine.GetMACAddressOfENI(allMACAddresses, eniID) //... ok, err := engine.DoesMACAddressMapToIPV4Address(macAddressOfENI, ipv4Address) //... return macAddressOfENI, nil }

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/engine/engine.go

+	// expect the Agent to create those directories. We can also let
+	// these be conigured via the plugin config.
+	dhclientV4LeaseFilePathPrefix    = "/var/lib/dhclient/ns-dhclient"
+	dhclientV4LeasePIDFilePathPrefix = "/var/run/ns-dhclient"


Are we planning to invoke this from the ECS agent container?

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/engine/engine.go

+		return false
+	}
+
+	log.Debugf("dhclient found in: %s", dhclientPath)


nit: we might want to change this to Tracef in the future if debug gets too noisy

Are we planning to invoke this from the ECS agent container?

This is invoked as a part of setup namespace closure: https://github.com/aws/amazon-ecs-cni-plugins/blob/dev/plugins/eni/engine/nsclosure.go#L115

I'm trying to understand more about your response to @richardpen above:

The mount namespace is same as where it's being invoked from. If the plugin is invoked on the host using scripts in cni package, it'd be the mount namespace of the host. If the plugin is invoked from the ECS Agent container, it'd be the mount namespace of the agent.

Are we invoking it on the host using scripts in the cni package or from the ECS agent container?

Ah, that would be from the ECS Agent. We'd have mount the executable from the host to the agent's mount namespace as well.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/engine/engine.go

+	macs, err := engine.metadata.GetMetadata(metadataNetworkInterfacesPath)
+	if err != nil {
+		return nil, errors.Wrap(err,
+			"getAllMACAddresses engine: unable to get all mac addresses on the instance from instance metadata")


(multiple occurrences) nit: I think it's safe to just start this with "engine" as the rest of the error string is unique enough to find it in source.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/engine/engine.go

+	return nil
+}
+
+// nsClosure wraps the parameters and the method to configure the container's namespace


nit: I wonder if this might be better named as nsContext?

Yeah, I got the name from https://github.com/containernetworking/cni/blob/94e02e0768ffeeb5c4eec57184b3bc8d11650e37/pkg/ns/ns.go#L169. It's really context for executing the closure. I'll rename it to reflect that.

samuelkarp · 2017-03-23T07:50:08Z

plugins/eni/engine/error.go

+// cannot be mapped to any of the network interfaces attached to the host as
+// determined by the instance metadata
+type unmappedMACAddressError struct {
+	err *_error


(multiple occurrences) Any reason not to just embed _error here? That would free you from having to make a new Error method and deal with an extra pointer.

Adding skeleton code for the ENI plugin and the Makefile

c85ebfd

aaithal requested review from samuelkarp, vsiddharth and richardpen February 21, 2017 19:51

aaithal added 5 commits February 21, 2017 21:09

Adding types and engine for ENI plugin

ad07612

Add get-deps target to get dependencies

5146755

Add the version/cnispec package for the ENI plugin.

62a1264

The logic of spec compatibility is moved to this package from a hardcoded string in the main package

Code beautification, added comments and made a bunch of things const'…

a1719ed

…s in tests

Add a target for building plugins

af7ccb7

richardpen reviewed Feb 22, 2017

View changes

aaithal added 2 commits February 23, 2017 01:24

Addressing review coments. Replaced all the dumb ipv4 address stuff w…

dc68dee

…ith methods from the net package

Vendoring dependencies

537601f

richardpen reviewed Feb 23, 2017

View changes

Fix vendor path in imports

2cfc83a

samuelkarp reviewed Feb 26, 2017

View changes

aaithal added 2 commits February 27, 2017 17:31

pkg/execwrapper: Adding this package to wrap os.Cmd and methods in os…

0235df8

…/exec

plugins/eni: Addressing review comments.

5711d60

Made New() a wrapper that creates the necessary fields for the 'engine' obect. Also modified the 'Makefile' so that the eni executable is built as a non PHONY target

aaithal force-pushed the skeleton branch from e455af2 to 5711d60 Compare February 27, 2017 18:41

aaithal added 2 commits February 28, 2017 16:46

plugins/eni: created some error types and rewrote error messages as per

024918d

https://golang.org/doc/effective_go.html#errors

plugins/eni: check if dhclient exists in the path

4768f35

Added IsDHClientInPath() method to the Engine interface. Updated engine create() method and tests accordingly

aaithal force-pushed the skeleton branch from ae2ed38 to 4768f35 Compare February 28, 2017 21:19

plugins/eni: invoke dhclient while setting up container namespace wit…

c8a868f

…h ENI Modify namespace closure execution to invoke dhclient to start renewing the lease on the IPV4 address of the ENI

aaithal changed the title ~~Adding skeleton code for the ENI plugin and the Makefile~~ ENI Plugin Implementation Mar 1, 2017

richardpen reviewed Mar 1, 2017

View changes

richardpen approved these changes Mar 1, 2017

View changes

aaithal merged commit c8a868f into aws:dev Mar 1, 2017

samuelkarp reviewed Mar 23, 2017

View changes

lilida mentioned this pull request Feb 8, 2018

Pid and lease file format don't work on dhclient 4.3.3 #71

Closed

		@@ -56,7 +56,12 @@ type engine struct {
		}

		// NewEngine creates a new Engine object

ENI Plugin Implementation #1

ENI Plugin Implementation #1

Conversation

aaithal commented Feb 21, 2017 • edited

Summary

Implementation Details

Testing Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal commented Mar 1, 2017

samuelkarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaithal commented Feb 21, 2017 •

edited

aaithal Feb 27, 2017 •

edited