Enable outboundNAT for Windows containers. #86
Conversation
delete comment
… to test on a new cluster.
network/endpoint.go
Outdated
| log.Printf("[net] Created endpoint %+v.", ep) | ||
|
|
||
| return ep, nil | ||
|
|
network/endpoint_windows.go
Outdated
| if netNsPath != "" { | ||
| splits := strings.Split(netNsPath, ":") | ||
| if len(splits) == 2 { | ||
| epName = splits[1] |
There was a problem hiding this comment.
add comment here..briefly explaining two cases if and else?
network/endpoint_windows.go
Outdated
| epName = containerID | ||
| } | ||
| if len(epName) > 8 { | ||
| epName = epName[:8] + "-" + ifName |
There was a problem hiding this comment.
Why are we choosing first 8 characters of ContainerID? Will the first 8 characters will be unique for all containers?
There was a problem hiding this comment.
We construct endpoint ID using first 8 characters of containerID and interface name. Please check plugin.go line 162-168.
network/endpoint_windows.go
Outdated
| // newEndpointImpl creates a new endpoint in the network. | ||
| func (nw *network) newEndpointImpl(epInfo *EndpointInfo) (*endpoint, error) { | ||
| // Ignore consecutive ADD calls for the same container. | ||
| if nw.Endpoints[epInfo.Id] != nil { |
There was a problem hiding this comment.
Which one you are using as a key in Endpoints map - epName or epinfo.Id ? In line 54, you are using epName as key for same map. Are both same?
There was a problem hiding this comment.
epinfo.Id is generated from containerID of ADD call. If the ADD call is for infrastructure container, then epName == epinfo.id. Basic, when I use epName as a key for nw.Endpoints, I am handling the ADD call for workload container. I want to return the endpoint for infrastructure container, since these two containers belong to the same pod. Reference: https://kubernetes.io/docs/concepts/cluster-administration/networking/#kubernetes-model
There was a problem hiding this comment.
Thank you got it. Checking GetHNSEndpointByName not nil solves object already exists issue. Can you please add some details in function description?
| } | ||
|
|
||
| //enable outbound NAT | ||
| var enableOutBoundNat = json.RawMessage(`{"Type": "OutBoundNAT"}`) |
There was a problem hiding this comment.
Do we have unit test for testing this function? If its there, can we add one to test this property is set?
There was a problem hiding this comment.
I didn't write unit test for this. We can discuss this.
| nw.Endpoints[epInfo.Id] = ep | ||
|
|
||
| log.Printf("[net] Created endpoint %+v.", ep) | ||
|
|
There was a problem hiding this comment.
remove this empty line also. not needed
3 participants
What this PR does / why we need it:
Which issue this PR fixes