search

ottoyiu / k8s-ec2-srcdst

A Kubernetes Controller that will ensure that the EC2 Source Destination Check (source-dest-check attribute) is disabled on nodes within the cluster.
Apache License 2.0
18 stars 8 forks source link

Add support for instances with multiple network interfaces #7

Closed ripta closed 5 years ago

ripta commented 6 years ago

Some CNI plugins, most notably the amazon-vpc-cni-k8s one, assigns a primary and secondary network interface.

Calling ModifyInstanceAttribute to change SourceDestCheck fails with the following error if that instance has more than one network interface:

  srcdst_controller.go:87] Fail to disable src dst check for EC2
  instance: i-0c5d0ab86783fba1e; InvalidInstanceID: There are multiple
  interfaces attached to instance 'i-0c5d0ab86783fba1e'. Please specify
  an interface ID for the operation instead.

Instead, this patch looks for each network interface belonging to that instance, and calls ModifyNetworkInterfaceAttribute. Only interfaces that need to be updated will generate that API call.

This is my first foray into the AWS Go SDK mock objects, but I've attempted to update the tests as well.

ripta commented 6 years ago

Apparently build failed because it timed out waiting for docker credentials. D:

ottoyiu commented 6 years ago

@ripta Thanks for the PR! that's weird.. I'll restart the build and see if that makes a difference.

Edit: I need to fix the build process up...

ripta commented 6 years ago

SGTM. Let me know if there's anything I can do to help!

vpm-bradleyhession commented 6 years ago

Bump. Any progress on getting this merged?

ottoyiu commented 6 years ago

@vpm-bradleyhession there's concerns with this PR where when new ENIs are brought up, the src/dst checks are not disabled properly.

ranimufid commented 5 years ago

Any updates as to when this PR will be fixed/merged?

ottoyiu commented 5 years ago

@ranimufid we have an internal re-write which we will publish soon to this repo. However, it does not serve the purpose of when there's multiple ENIs.

If you can describe your use-case, we maybe able to accommodate.

ranimufid commented 5 years ago

@ottoyiu, sadly the need to disable src/dest on multiple ENIs is my exact use case :(

ripta commented 5 years ago

Closing, because we no longer need this feature. Specifically, recent versions of amazon-vpc-cni-k8s definitely work without needing to change srcdst.

Not to mention the rewrite @ottoyiu mentioned above will most likely cause merge conflicts. If anyone wants it though, feel free to take the patch as-is!