[windows] Fix bug in configureContainerLink

[XinShuYang]

We should use containerNetNS when deciding if a container is an infra one.

Signed-off-by: Shuyang Xin [email protected]

Fixes #3077

[codecov-commenter]

Codecov Report

Merging #3089 (ccdf733) into main (c135609) will decrease coverage by 0.70%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3089      +/-   ##
==========================================
- Coverage   58.76%   58.06%   -0.71%     
==========================================
  Files         292      293       +1     
  Lines       24724    24823      +99     
==========================================
- Hits        14530    14414     -116     
- Misses       8622     8828     +206     
- Partials     1572     1581       +9     

Flag	Coverage Δ
kind-e2e-tests	44.49% <ø> (-0.90%)	⬇️
unit-tests	40.02% <ø> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ver/registry/controlplane/nodestatssummary/rest.go	50.00% <0.00%> (-50.00%)	⬇️
...egator/apiserver/handlers/recordmetrics/handler.go	0.00% <0.00%> (-44.45%)	⬇️
pkg/apiserver/handlers/loglevel/handler.go	0.00% <0.00%> (-38.47%)	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	66.66% <0.00%> (-33.34%)	⬇️
...gregator/apiserver/handlers/flowrecords/handler.go	0.00% <0.00%> (-23.64%)	⬇️
pkg/agent/nodeportlocal/k8s/annotations.go	83.87% <0.00%> (-16.13%)	⬇️
pkg/agent/controller/networkpolicy/reject.go	75.80% <0.00%> (-12.10%)	⬇️
pkg/log/log_level.go	0.00% <0.00%> (-10.00%)	⬇️
.../listers/security/v1alpha1/clusternetworkpolicy.go	0.00% <0.00%> (-9.10%)	⬇️
pkg/controller/networkpolicy/status_controller.go	71.24% <0.00%> (-5.89%)	⬇️
... and 25 more

[tnqn]

The change itself LGTM. However, I didn't figure out how it resolves the issue in #3077, could you explain more details?

BTW, the signed email address seems wrong.

[XinShuYang]

I think using the wrong parameter in isInfraContainer() could cause errors during reattach HNS Endpoint to the container. And this test failed due to the timeout waiting for connectivity test after updating pod label.
The email address has been corrected, thanks. @tnqn

[tnqn]

I think using the wrong parameter in isInfraContainer() could cause errors during reattach HNS Endpoint to the container. And this test failed due to the timeout waiting for connectivity test after updating pod label.

I thought the changed code only affects the cleanup of HNSEndpoint when there was something wrong. When a Pod was recreated, did it encounter the attach error? Did you see the log "failed to configure container IP" when the test failed?

/test-windows-networkpolicy

The email address has been corrected, thanks. @tnqn

You also need to update the email address of your git setting and update the author and email of the patch to be consistent with the DCO sign. Currently DCO check failed:

Commit sha: 779b736, Author: Shuyang Xin, Committer: Shuyang Xin; Expected "Shuyang Xin [email protected]", but got "Shuyang Xin [email protected]".

[XinShuYang]

I think using the wrong parameter in isInfraContainer() could cause errors during reattach HNS Endpoint to the container. And this test failed due to the timeout waiting for connectivity test after updating pod label.

I thought the changed code only affects the cleanup of HNSEndpoint when there was something wrong. When a Pod was recreated, did it encounter the attach error? Did you see the log "failed to configure container IP" when the test failed?

Yes, I can see "failed to configure container IP" many times when test failed from the agent log. I thought it was because the endpoint was always removed after any non-pause container failed to attach the endpoint. This incorrect behavior happened because the isInfraContainer() function only return true at this time.

After fixing this bug, there was no incorrect endpoint removing info in the log. The test can be passed with good connection status of the client pod.

/test-windows-networkpolicy

The email address has been corrected, thanks. @tnqn

You also need to update the email address of your git setting and update the author and email of the patch to be consistent with the DCO sign. Currently DCO check failed:

Commit sha: 779b736, Author: Shuyang Xin, Committer: Shuyang Xin; Expected "Shuyang Xin [email protected]", but got "Shuyang Xin [email protected]".

The DCO check can pass this time, thanks for your reminder.

[tnqn]

@XinShuYang thanks for the explanation. Why wasn't there an issue before this change ff17f09? It seems it also removed the hnsendpoint unconditionally. Was it because of the change at ff17f09#diff-17bccd4a78564ad9445047f0815ca61a4590f9119a333b9f619a14c9c3999b9bL302?

[XinShuYang]

@XinShuYang thanks for the explanation. Why wasn't there an issue before this change ff17f09? It seems it also removed the hnsendpoint unconditionally. Was it because of the change at ff17f09#diff-17bccd4a78564ad9445047f0815ca61a4590f9119a333b9f619a14c9c3999b9bL302?

@tnqn Before ff17f09 , attachContainerLink() won't return error if non-pause container failed to attach endpoint. According to this commit, any container which failed to attach endpoint will return error, then isInfraContainer() should figure out whether it is a pause container. If it is not a pause container, we should never remove the endpoint. The new bug fix make the isInfraContainer() work correctly on windows.

[XinShuYang]

/test-windows-all

[XinShuYang]

@tnqn Hi quan, since this bug is blocking windows networkpolicy CI test, could you merge it first?

[tnqn]

LGTM

[tnqn]

/skip-e2e
/skip-conformance
/skip-networkpolicy
As it doesn't change linux code.