Added Host NC communication support in Linux #374

tamilmani1989 · 2019-07-09T20:15:25Z

What this PR does / why we need it:
This PR added support for host to NC communication and NC to host communication. These are controlled by two dnc fields AllowHostToNCCommunication and AllowNCToHostCommunication. If both are true it allows two way communication.

Created iptable package and updated all places that used iptable commands.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

…bled

…working into allowinbound_orcas

…working into allowinbound_orcas # Conflicts: # network/endpoint.go

codecov · 2019-07-09T20:17:52Z

Codecov Report

Merging #374 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #374   +/-   ##
=======================================
  Coverage   40.01%   40.01%           
=======================================
  Files          25       25           
  Lines        3546     3546           
=======================================
  Hits         1419     1419           
  Misses       1927     1927           
  Partials      200      200

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e9525c...1b6c562. Read the comment docs.

jaer-tsun

general comments

iptables/iptables.go

jaer-tsun · 2019-07-09T23:32:41Z

network/endpoint.go

+	AllowInboundFromNCToHost bool
+	NetworkNameSpace         string `json:",omitempty"`
+	ContainerID              string
+	PODName                  string `json:",omitempty"`


PodName or Podname

i didn't change this..so not changing it

jaer-tsun · 2019-07-09T23:32:56Z

network/endpoint.go

+	EnableMultiTenancy       bool
+	AllowInboundFromHostToNC bool
+	AllowInboundFromNCToHost bool
+	PODName                  string


PodName or Podname

i didn't change this..so not changing it

jaer-tsun · 2019-07-09T23:36:56Z

network/endpoint.go

+	AllowInboundFromHostToNC bool
+	AllowInboundFromNCToHost bool
+	PODName                  string
+	PODNameSpace             string


PodNamespace

i didn't change this..so not changing it

jaer-tsun · 2019-07-09T23:38:10Z

network/endpoint.go

+		IfName:       ep.IfName,
+		ContainerID:  ep.ContainerID,
+		NetNsPath:    ep.NetworkNameSpace,
+		PODName:      ep.PODName,


uncaps POD (see comments above)

network/ovssnat/ovssnat.go

ninzavivek

One generic comment, changes lack documentation. I recommend a line or two where you can do.

ninzavivek · 2019-07-13T17:37:19Z

iptables/iptables.go

+}
+
+func RuleExists(tableName, chainName, match, target string) bool {
+	cmd := fmt.Sprintf("iptables -w 60 -t %s -C %s %s -j %s", tableName, chainName, match, target)


this is lock timeout. So if iptable couldn't grab lock for 60 seconds, it will exit. i can move that to constant

ninzavivek · 2019-07-13T17:37:56Z

iptables/iptables.go

+		return nil
+	}
+
+	cmd := fmt.Sprintf("iptables -w 60 -t %s -I %s 1 %s -j %s", tableName, chainName, match, target)


I will recommend defining a constant for iptables command? It seems like you are using it at multiple places.

tamilmani1989 · 2019-07-15T18:13:47Z

One generic comment, changes lack documentation. I recommend a line or two where you can do.

sure will do it.

jaer-tsun

/lgtm

ninzavivek · 2019-07-16T22:47:26Z

iptables/iptables.go

+
+import (
+	"fmt"
+


goformat automatically does that

ninzavivek · 2019-07-16T22:53:37Z

network/epcommon/endpoint_common.go

+	if action == iptables.Insert {
+		err = iptables.InsertIptableRule(iptables.Filter, chainName, matchCondition, target)
+	} else if action == iptables.Append {
+		err = iptables.AppendIptableRule(iptables.Filter, chainName, matchCondition, target)


case statement?

…working into allowinbound_orcas # Conflicts: # network/endpoint_linux.go

jaer-tsun

/lgtm

tamilmani1989 added 8 commits March 26, 2019 13:29

allow inbound connection to container from host if the feature is ena…

2c88f49

…bled

Allowinbound from host to NC if flag is enabled

591b379

fixed old changes

0cd9e3b

Merge branch 'master' of https://github.com/Azure/azure-container-net…

d584f3e

…working into allowinbound_orcas

minor fixes

6e144d5

added NC to host commn support

1b51cb9

Merge branch 'master' of https://github.com/Azure/azure-container-net…

686013c

…working into allowinbound_orcas # Conflicts: # network/endpoint.go

added NCToHostCommunication field in cns contract

963b8f4

tamilmani1989 requested review from jaer-tsun and ninzavivek July 9, 2019 20:49

tamilmani1989 changed the title ~~Added host to NC communication support and vice-versa~~ Added host to NC communication support in Linux and vice-versa Jul 9, 2019

tamilmani1989 changed the title ~~Added host to NC communication support in Linux and vice-versa~~ Added Host NC communication support in Linux Jul 9, 2019

jaer-tsun reviewed Jul 9, 2019

View reviewed changes

addressed review comments

2af49dd

ninzavivek suggested changes Jul 13, 2019

View reviewed changes

addressed review comments

a15e9e3

jaer-tsun previously approved these changes Jul 15, 2019

View reviewed changes

addressed review comments

ba62b81

tamilmani1989 dismissed jaer-tsun’s stale review via ba62b81 July 16, 2019 23:40

ninzavivek previously approved these changes Jul 16, 2019

View reviewed changes

tamilmani1989 added 2 commits July 16, 2019 17:01

address comments

66f0a5b

Merge branch 'master' of https://github.com/Azure/azure-container-net…

8b9f318

…working into allowinbound_orcas # Conflicts: # network/endpoint_linux.go

tamilmani1989 dismissed ninzavivek’s stale review via 8b9f318 July 17, 2019 00:04

fixed space damage

1b6c562

jaer-tsun approved these changes Jul 17, 2019

View reviewed changes

tamilmani1989 merged commit edabc6d into Azure:master Jul 17, 2019


		import (
		"fmt"

Added Host NC communication support in Linux #374

Added Host NC communication support in Linux #374

Uh oh!

Conversation

tamilmani1989 commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jaer-tsun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ninzavivek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamilmani1989 commented Jul 15, 2019

Uh oh!

jaer-tsun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaer-tsun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tamilmani1989 commented Jul 9, 2019 •

edited

Loading

codecov bot commented Jul 9, 2019 •

edited

Loading