Switch traceflow CRD validation to webhook validation. #5230

shi0rik0 · 2023-07-11T11:52:22Z

Currently, the traceflow CRD validation is executed in run-time, which is less user-friendly than the webhook validation. I moved most of the validation to the webhook validation.

Some of the validations in Agent is unchanged because they need the information of the Agent. Also, I found the current validation is insufficient, but I left the improvement as a future work.

pkg/controller/traceflow/validate.go

pkg/controller/traceflow/validate_test.go

pkg/controller/traceflow/validate.go

pkg/controller/traceflow/validate_test.go

pkg/apiserver/apiserver.go

pkg/controller/traceflow/validate.go

pkg/controller/traceflow/validate_test.go

shi0rik0 · 2023-07-12T06:44:55Z

git pull --rebase upstream main polluted the comparision. Please compare commit a6aa6d9 with ec9d9ba

https://github.com/antrea-io/antrea/compare/ec9d9ba520bcc54b23001f55fa991e381ef468b0..a6aa6d9d03ca2f8fb90500690ab8c22f368f08c9

pkg/controller/traceflow/validate.go

pkg/controller/traceflow/validate_test.go

pkg/controller/traceflow/validate.go

pkg/controller/traceflow/validate_test.go

pkg/controller/traceflow/validate.go

tnqn

LGTM. However, conflicts need to be resolved.

tnqn · 2023-07-17T11:43:56Z

LGTM. However, conflicts need to be resolved.

Ignore it, I mistook other message as conflicts.

tnqn · 2023-07-17T12:55:57Z

Several unit tests and e2e tests failed. Their expectations should be updated according to the change.

shi0rik0 · 2023-07-17T13:14:22Z

I've removed outdated unit tests. Interestingly, the unit test "timeoutHostnetworkTraceflow" in pkg/controller/traceflow/controller_test.go seems to be wrong before this patch. The timeout in the test case is 2 secs, so we need to wait for at least 2 secs to get the Failed status. I don't know why originally the test case can be passed.

shi0rik0 · 2023-07-17T14:21:51Z

There's still a failed test in "Go / Unit test (windows-2019)", but this failure seems to have nothing to do with this patch?

https://github.com/antrea-io/antrea/actions/runs/5576066018/attempts/2?pr=523

Update: the test is OK again on another try. Related to issue #5261.

tnqn · 2023-07-18T02:13:46Z

I've removed outdated unit tests. Interestingly, the unit test "timeoutHostnetworkTraceflow" in pkg/controller/traceflow/controller_test.go seems to be wrong before this patch. The timeout in the test case is 2 secs, so we need to wait for at least 2 secs to get the Failed status. I don't know why originally the test case can be passed.

It's correct before the patch. It can fail immediately once it finds out the source Pod is in hostnetwork, instead of waiting 2s. Since you removed the source Pod hostnetwork check in it, it then waits until timeout, which is actually the same as the timeoutTraceflow, and it's no longer a possible use case. I think it should be removed.

pkg/controller/traceflow/validate.go

shi0rik0 · 2023-07-18T02:56:55Z

I've removed outdated unit tests. Interestingly, the unit test "timeoutHostnetworkTraceflow" in pkg/controller/traceflow/controller_test.go seems to be wrong before this patch. The timeout in the test case is 2 secs, so we need to wait for at least 2 secs to get the Failed status. I don't know why originally the test case can be passed.

It's correct before the patch. It can fail immediately once it finds out the source Pod is in hostnetwork, instead of waiting 2s. Since you removed the source Pod hostnetwork check in it, it then waits until timeout, which is actually the same as the timeoutTraceflow, and it's no longer a possible use case. I think it should be removed.

I've removed it.

test/e2e/traceflow_test.go

gran-vmv · 2023-07-18T08:16:11Z

/test-e2e

gran-vmv

LGTM

luolanzone · 2023-07-31T15:37:06Z

@shi0rik0 please help to resolve code conflicts.

shi0rik0 · 2023-08-14T04:51:10Z

Sorry for the late reply. The conflicts are caused by the Traceflow API version promotion. I've solved the conflicts. There's a failed e2e test, but this seems to be not related to this PR because recent PRs also have this failure (#5380). @luolanzone

tnqn

LGTM, one nit

test/e2e/traceflow_test.go

Currently, the traceflow CRD validation is executed in run-time, which is less user-friendly than the webhook validation. I moved most of the validation to the webhook validation. Signed-off-by: shi0rik0 <anguuan@outlook.com>

tnqn

LGTM

tnqn · 2023-08-16T02:48:42Z

/test-all

shi0rik0 · 2023-08-16T07:00:41Z

/test-all

antoninbas · 2023-08-21T21:47:11Z

This PR broke e2e tests on IPv6-only clusters, as the new e2e test assumes that Pods always have a valid IPv4 address. I have opened a PR to fix it: #5415

When doing any non-trivial change to e2e Traceflow tests, it's probably a good idea to always run IPv6-only and dual-stack e2e test jobs.

shi0rik0 mentioned this pull request Jul 11, 2023

Fix traceflow spec validation logic. #5223

Closed

luolanzone reviewed Jul 12, 2023

View reviewed changes

shi0rik0 force-pushed the tf-webhook-valid branch from ec9d9ba to f3ea9e3 Compare July 12, 2023 06:37

shi0rik0 requested a review from luolanzone July 12, 2023 06:38

shi0rik0 force-pushed the tf-webhook-valid branch from f3ea9e3 to a6aa6d9 Compare July 12, 2023 06:43

tnqn reviewed Jul 12, 2023

View reviewed changes

shi0rik0 force-pushed the tf-webhook-valid branch from a6aa6d9 to fa1c2d3 Compare July 12, 2023 12:21

shi0rik0 requested a review from tnqn July 12, 2023 12:21

tnqn reviewed Jul 12, 2023

View reviewed changes

shi0rik0 force-pushed the tf-webhook-valid branch from fa1c2d3 to 3c1ec1a Compare July 13, 2023 04:10

shi0rik0 requested a review from tnqn July 13, 2023 04:10

tnqn reviewed Jul 17, 2023

View reviewed changes

tnqn previously approved these changes Jul 17, 2023

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Jul 17, 2023

shi0rik0 dismissed tnqn’s stale review via 14ad9b8 July 17, 2023 13:10

shi0rik0 force-pushed the tf-webhook-valid branch from 3c1ec1a to 14ad9b8 Compare July 17, 2023 13:10

shi0rik0 requested a review from tnqn July 17, 2023 13:15

luolanzone reviewed Jul 18, 2023

View reviewed changes

pkg/controller/traceflow/validate.go Show resolved Hide resolved

pkg/controller/traceflow/validate.go Outdated Show resolved Hide resolved

luolanzone requested a review from gran-vmv July 18, 2023 02:16

shi0rik0 force-pushed the tf-webhook-valid branch from 14ad9b8 to 9711d2b Compare July 18, 2023 02:56

shi0rik0 requested a review from luolanzone July 18, 2023 02:59

gran-vmv reviewed Jul 18, 2023

View reviewed changes

test/e2e/traceflow_test.go Show resolved Hide resolved

shi0rik0 force-pushed the tf-webhook-valid branch 2 times, most recently from 0d1a2d4 to ccbbb7a Compare July 18, 2023 08:32

shi0rik0 changed the title ~~Switch traceflow CRD validation to webhook validation.~~ [WIP] Switch traceflow CRD validation to webhook validation. Jul 18, 2023

shi0rik0 force-pushed the tf-webhook-valid branch 2 times, most recently from 20d2833 to d2d0699 Compare July 19, 2023 00:34

shi0rik0 changed the title ~~[WIP] Switch traceflow CRD validation to webhook validation.~~ Switch traceflow CRD validation to webhook validation. Jul 19, 2023

shi0rik0 requested a review from gran-vmv July 19, 2023 06:04

gran-vmv previously approved these changes Jul 19, 2023

View reviewed changes

shi0rik0 mentioned this pull request Jul 25, 2023

Traceflow spec update is not handled properly #5255

Closed

shi0rik0 dismissed gran-vmv’s stale review via 38c82c5 August 14, 2023 02:13

shi0rik0 force-pushed the tf-webhook-valid branch from d2d0699 to 38c82c5 Compare August 14, 2023 02:13

tnqn previously approved these changes Aug 15, 2023

View reviewed changes

test/e2e/traceflow_test.go Outdated Show resolved Hide resolved

Switch traceflow CRD validation to webhook validation.

f06fa42

Currently, the traceflow CRD validation is executed in run-time, which is less user-friendly than the webhook validation. I moved most of the validation to the webhook validation. Signed-off-by: shi0rik0 <anguuan@outlook.com>

shi0rik0 dismissed tnqn’s stale review via f06fa42 August 16, 2023 01:32

shi0rik0 force-pushed the tf-webhook-valid branch from 38c82c5 to f06fa42 Compare August 16, 2023 01:32

tnqn approved these changes Aug 16, 2023

View reviewed changes

tnqn merged commit faf8fb3 into antrea-io:main Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch traceflow CRD validation to webhook validation. #5230

Switch traceflow CRD validation to webhook validation. #5230

shi0rik0 commented Jul 11, 2023

shi0rik0 commented Jul 12, 2023 •

edited

Loading

tnqn left a comment

tnqn commented Jul 17, 2023

tnqn commented Jul 17, 2023

shi0rik0 commented Jul 17, 2023 •

edited

Loading

shi0rik0 commented Jul 17, 2023 •

edited

Loading

tnqn commented Jul 18, 2023

shi0rik0 commented Jul 18, 2023

gran-vmv commented Jul 18, 2023

gran-vmv left a comment

luolanzone commented Jul 31, 2023

shi0rik0 commented Aug 14, 2023

tnqn left a comment

tnqn left a comment

tnqn commented Aug 16, 2023

shi0rik0 commented Aug 16, 2023

antoninbas commented Aug 21, 2023

Switch traceflow CRD validation to webhook validation. #5230

Switch traceflow CRD validation to webhook validation. #5230

Conversation

shi0rik0 commented Jul 11, 2023

shi0rik0 commented Jul 12, 2023 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 17, 2023

tnqn commented Jul 17, 2023

shi0rik0 commented Jul 17, 2023 • edited Loading

shi0rik0 commented Jul 17, 2023 • edited Loading

tnqn commented Jul 18, 2023

shi0rik0 commented Jul 18, 2023

gran-vmv commented Jul 18, 2023

gran-vmv left a comment

Choose a reason for hiding this comment

luolanzone commented Jul 31, 2023

shi0rik0 commented Aug 14, 2023

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 16, 2023

shi0rik0 commented Aug 16, 2023

antoninbas commented Aug 21, 2023

shi0rik0 commented Jul 12, 2023 •

edited

Loading

shi0rik0 commented Jul 17, 2023 •

edited

Loading

shi0rik0 commented Jul 17, 2023 •

edited

Loading