Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix antrea-agent crashing in networkPolicyOnly mode #795

Merged
merged 1 commit into from
Jun 5, 2020

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Jun 4, 2020

The PodCIDR of NodeConfig could be nil for the networkPolicyOnly
trafficEncapMode. The AgentQuerier should check nil before converting it
to string.

Besides, this patch removes the redundant field BridgeName and fixes a
typo.

Fixes #793

@antrea-bot
Copy link
Collaborator

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

  • /test-e2e: to trigger e2e tests.
  • /skip-e2e: to skip e2e tests.
  • /test-conformance: to trigger conformance tests.
  • /skip-conformance: to skip conformance tests.
  • /test-networkpolicy: to trigger networkpolicy tests.
  • /skip-networkpolicy: to skip networkpolicy tests.
  • /test-windows-conformance: to trigger windows conformance tests.
  • /skip-windows-conformance: to skip windows conformance tests.
  • /test-all: to trigger all tests.
  • /skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

@tnqn
Copy link
Member Author

tnqn commented Jun 4, 2020

@ruicao93 @wenyingd I removed BridgeName because it's redundant with OVSBridge and never used, please help confirm it doesn't break anything.

@tnqn
Copy link
Member Author

tnqn commented Jun 4, 2020

/test-all

@tnqn
Copy link
Member Author

tnqn commented Jun 4, 2020

/test-all

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix LGTM

pkg/agent/config/node_config.go Outdated Show resolved Hide resolved
return ipNet
}

func TestAgentQuerierGetAgentInfo(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tnqn is this test related to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, AgentQuerier.GetAgentInfo is the method panic. The test ensures it can get expected result without panic (networkPolicyOnly-mode non-partial case).

pkg/agent/querier/querier_test.go Outdated Show resolved Hide resolved
@@ -503,7 +503,7 @@ func (i *Initializer) initNodeLocalConfig() error {
}

if i.networkConfig.TrafficEncapMode.IsNetworkPolicyOnly() {
i.nodeConfig = &config.NodeConfig{Name: nodeName, NodeIPAddr: localAddr, BridgeName: i.ovsBridgeClient.GetBridgeName(), UplinkNetConfig: new(config.AdapterNetConfig)}
i.nodeConfig = &config.NodeConfig{Name: nodeName, NodeIPAddr: localAddr, UplinkNetConfig: new(config.AdapterNetConfig)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add OVSBridge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This must have caused some issues in networkPolicyOnly mode too.

The PodCIDR of NodeConfig could be nil for the networkPolicyOnly
trafficEncapMode. The AgentQuerier should check nil before converting it
to string.

Besides, this patch removes the redundant field BridgeName and fixes a
typo.
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Having offline discussion with @ruicao93 , the field is not used on Windows. It is OK to delete it.

@ruicao93
Copy link
Contributor

ruicao93 commented Jun 5, 2020

@ruicao93 @wenyingd I removed BridgeName because it's redundant with OVSBridge and never used, please help confirm it doesn't break anything.

Thanks Quan. I think it's ok to remove BridgeName. It was used in windows branch and is redundant now.

@tnqn
Copy link
Member Author

tnqn commented Jun 5, 2020

/test-all

@tnqn tnqn merged commit 6399cbd into antrea-io:master Jun 5, 2020
antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Jun 5, 2020
The PodCIDR of NodeConfig could be nil for the networkPolicyOnly
trafficEncapMode. The AgentQuerier should check nil before converting it
to string.

Besides, this patch removes the redundant field BridgeName and fixes a
typo.
antoninbas pushed a commit that referenced this pull request Jun 5, 2020
The PodCIDR of NodeConfig could be nil for the networkPolicyOnly
trafficEncapMode. The AgentQuerier should check nil before converting it
to string.

Besides, this patch removes the redundant field BridgeName and fixes a
typo.
GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020
The PodCIDR of NodeConfig could be nil for the networkPolicyOnly
trafficEncapMode. The AgentQuerier should check nil before converting it
to string.

Besides, this patch removes the redundant field BridgeName and fixes a
typo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Antrea agent version v0.7.0 is crashing in EKS env
7 participants