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 e2e test RejectServiceTraffic #3892

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Jun 14, 2022

createAgnhostServiceAndBackendPods should create the service with the
selector that can only selects the Pod created together with it.
Previous selector selected all Pods using agnhost image, causing
traffic destined for service to be forwarded to unexpected Pod.

This patch also changed to use same Pod selector for Service and
ClusterNetworkPolicy to avoid redundancy and inconsistency.

Signed-off-by: Quan Tian qtian@vmware.com

@tnqn tnqn requested review from GraysonWu and wenyingd June 14, 2022 06:21
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #3892 (f294668) into main (0be0c72) will increase coverage by 2.67%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3892      +/-   ##
==========================================
+ Coverage   63.63%   66.31%   +2.67%     
==========================================
  Files         293      556     +263     
  Lines       43224    87863   +44639     
==========================================
+ Hits        27504    58262   +30758     
- Misses      13484    25908   +12424     
- Partials     2236     3693    +1457     
Flag Coverage Δ
integration-tests 34.47% <ø> (?)
kind-e2e-tests 53.86% <ø> (+4.56%) ⬆️
unit-tests 63.03% <ø> (+18.46%) ⬆️

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

see 445 files with indirect coverage changes

@tnqn tnqn force-pushed the fix-testRejectServiceTraffic branch from 6c93ac0 to f44b79d Compare June 14, 2022 08:55
createAgnhostServiceAndBackendPods should create the service with the
selector that can only selects the Pod created together with it.
Previous selector selected all Pods using agnhost image, causing
traffic destined for service to be forwarded to unexpected Pod.

This patch also changed to use same Pod selector for Service and
ClusterNetworkPolicy to avoid redundancy and inconsistency.

Signed-off-by: Quan Tian <qtian@vmware.com>
@tnqn tnqn force-pushed the fix-testRejectServiceTraffic branch from f44b79d to f294668 Compare June 14, 2022 10:16
@tnqn tnqn changed the title Fix flaky e2e test RejectServiceTraffic Fix e2e test RejectServiceTraffic Jun 14, 2022
@@ -194,7 +194,7 @@ func (data *TestData) createAgnhostServiceAndBackendPods(t *testing.T, name, nam
ipProtocol = corev1.IPv6Protocol
}
require.NoError(t, data.podWaitForRunning(defaultTimeout, name, namespace))
svc, err := data.CreateService(name, namespace, 80, 80, map[string]string{"app": "agnhost"}, false, false, svcType, &ipProtocol)
svc, err := data.CreateService(name, namespace, 80, 80, map[string]string{"app": "agnhost", "antrea-e2e": name}, false, false, svcType, &ipProtocol)
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt if this change really works. Lable "antrea-e2e":$podName is also added on all Pods automaticallly which are created by function createPodOnNode. If "app": "agnhost" is matching other Pods created not in this case, it should be the same.

If the target of this change is to use a different Service selector to get accurate Pods, we should introduce a different label/selector key in the created object from the default settings?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lable "antrea-e2e":$podName is also added on all Pods automaticallly which are created by function createPodOnNode

That's the purpose of the change, podName is different, right? createPodOnNode(pod1) and createPodOnNode(pod2) will get two Pods with different labels: {"app": "agnhost", "antrea-e2e": "pod1"}, {"app": "agnhost", "antrea-e2e": "pod2"}, and service1 only selects the former and service2 only selects the latter, anything wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

@wenyingd does the explanation address your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks for the explanation.

@tnqn tnqn requested a review from wenyingd July 6, 2022 14:33
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 5, 2023
@github-actions github-actions bot closed this Apr 6, 2023
@tnqn tnqn reopened this Apr 6, 2023
@tnqn
Copy link
Member Author

tnqn commented Apr 6, 2023

@GraysonWu @wenyingd ping for review.

@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 7, 2023
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

Copy link
Contributor

@GraysonWu GraysonWu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix.

@tnqn
Copy link
Member Author

tnqn commented Apr 11, 2023

/test-e2e
/skip-networkpolicy
/skip-conformance

@tnqn
Copy link
Member Author

tnqn commented Apr 13, 2023

/skip-e2e

@tnqn tnqn merged commit 611f8d6 into antrea-io:main Apr 13, 2023
@tnqn tnqn deleted the fix-testRejectServiceTraffic branch April 13, 2023 07:39
jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023
createAgnhostServiceAndBackendPods should create the service with the
selector that can only selects the Pod created together with it.
Previous selector selected all Pods using agnhost image, causing
traffic destined for service to be forwarded to unexpected Pod.

This patch also changed to use same Pod selector for Service and
ClusterNetworkPolicy to avoid redundancy and inconsistency.

Signed-off-by: Quan Tian <qtian@vmware.com>
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.

4 participants