Check the length of parts while using ParseNamespacedName and ParseNamespacedNameContainer #2141

FingerLeader · 2021-07-22T08:21:43Z

What problem does this PR solve?

Issue Number: close #1933

Problem Summary:

What is changed and how it works?

Before returning value, check the length of parts to avoid panic.

Checklist

Tests

Unit test
E2E test
Manual test (add detailed scripts or steps below)
No code

ti-chi-bot · 2021-07-22T08:21:52Z

Welcome @FingerLeader!

It looks like this is your first PR to chaos-mesh/chaos-mesh 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to chaos-mesh/chaos-mesh. 😃

Signed-off-by: FingerLeader <FingerLeader@gmail.com>

Signed-off-by: FingerLeader <FingerLeader@gmail.com> add length check of parts

* fix: deadline reconciler, keep deadline exceed with true Signed-off-by: STRRL <str_ruiling@outlook.com> * chore: enriching log things Signed-off-by: STRRL <str_ruiling@outlook.com> * test: testcases about this issue Signed-off-by: STRRL <str_ruiling@outlook.com> * test: finish TODO Signed-off-by: STRRL <str_ruiling@outlook.com> * test: finished TODO Signed-off-by: STRRL <str_ruiling@outlook.com> * chore: refine codes and appending test case Signed-off-by: STRRL <str_ruiling@outlook.com> * chore: make linters happy Signed-off-by: STRRL <str_ruiling@outlook.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: FingerLeader <wanxfinger@gmail.com>

YangKeao · 2021-07-22T12:02:19Z

/run-e2e-tests

codecov-commenter · 2021-07-22T12:14:06Z

Codecov Report

Merging #2141 (7192928) into master (7e9ff3f) will decrease coverage by 12.81%.
The diff coverage is 39.32%.

@@             Coverage Diff             @@
##           master    #2141       +/-   ##
===========================================
- Coverage   55.78%   42.96%   -12.82%     
===========================================
  Files          68      128       +60     
  Lines        4383     8865     +4482     
===========================================
+ Hits         2445     3809     +1364     
- Misses       1768     4727     +2959     
- Partials      170      329      +159

Impacted Files	Coverage Δ
api/v1alpha1/awschaos_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/dnschaos_type.go	`0.00% <0.00%> (ø)`
api/v1alpha1/dnschaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/gcpchaos_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/httpchaos_types.go	`0.00% <0.00%> (ø)`
api/v1alpha1/httpchaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/iochaos_types.go	`0.00% <0.00%> (-40.00%)`	⬇️
api/v1alpha1/jvmchaos_webhook.go	`0.00% <0.00%> (ø)`
api/v1alpha1/kernelchaos_types.go	`0.00% <0.00%> (-20.00%)`	⬇️
api/v1alpha1/kinds.go	`23.52% <0.00%> (-3.14%)`	⬇️
... and 188 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d8c35b...7192928. Read the comment docs.

YangKeao · 2021-07-26T03:05:56Z

/run-e2e-tests

FingerLeader · 2021-07-26T04:07:06Z

/run-e2e-tests

STRRL · 2021-07-26T05:37:16Z

/run-e2e-tests

…ontainer Signed-off-by: FingerLeader <wanxfinger@gmail.com>

sonatype-lift · 2021-07-26T09:18:40Z

controllers/utils/controller/key.go

-	}, strings.Join(parts[2:], "")
+		Namespace: "",
+		Name:      "",
+	}, "", errors.New("Too Few Parts of NamespacedName")


ST1005: error strings should not be capitalized
(at-me in a reply with help or ignore)

FingerLeader · 2021-08-02T02:46:57Z

Hi @FingerLeader ,

I've been reverted the unexpected changes towards some files, and I have some suggestions for git operations:

check out a specific branch for changes, rather than directly using the master branch of your forked repo, and each branch should only do one thing, after the PR merged, the branch should be also deleted;

try git diff xxx > patch and git apply patch such techniques if you want to quickly apply some changes on the source commit/branch/tag/files;

Happy hack!

Thanks for your suggestion! I'll have a try next time

controllers/common/fx.go

Colstuwjx · 2021-08-02T03:06:00Z

controllers/chaosimpl/utils/record.go

+	podId, containerName, err := controller.ParseNamespacedNameContainer(record.Id)
+	if err != nil {
+		// TODO: organize the error in a better way
+		err = NewFailToFindContainer(pod.Namespace, pod.Name, containerName, err)


It's still an issue.

FingerLeader · 2021-08-02T04:02:46Z

/run-e2e-test

Colstuwjx · 2021-08-02T07:02:55Z

The file mode changes are unexpected behavior, please revert them, @FingerLeader.

FingerLeader · 2021-08-02T07:26:53Z

Actually, I don't know why the file modes are changed. After reseting my branch to 084d57a, the modes are still change to 10755 when I commit the branch.

Colstuwjx · 2021-08-02T07:49:10Z

I wonder if it's due to sth like git index cache, you could try the following commands to rollback to the upstream branch state:

git remote add upstream git@github.com:chaos-mesh/chaos-mesh.git
git fetch upstream
git diff -p -R --no-ext-diff --no-color upstream/master| grep -E "^(diff|(old|new) mode)" --color=never > patch
git apply patch

For a simple solution about this, I think you could just delete your local repo and re-clone it?

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

FingerLeader · 2021-08-02T08:00:11Z

I set filemode = false in .git/config, and it seems work.

controllers/common/fx.go

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

FingerLeader · 2021-08-02T08:24:41Z

/run-e2e-test

Colstuwjx

LGTM!

FingerLeader · 2021-08-02T11:07:20Z

LGTM!

It's really a long way to get this LGTM. Thanks for your help!

YangKeao · 2021-08-03T03:42:34Z

So len(parts) > 1 (which includes xx/xx/xx) are all regarded as a valid NamespacedName?

@STRRL suggested that the xx/xx/xx input should be illegal for the ParseNamespacedName. Will you modify it in the future PR or just prefer the current choice?

FingerLeader · 2021-08-03T07:12:10Z

So len(parts) > 1 (which includes xx/xx/xx) are all regarded as a valid NamespacedName?

@STRRL suggested that the xx/xx/xx input should be illegal for the ParseNamespacedName. Will you modify it in the future PR or just prefer the current choice?

But IOChaos uses ParseNamespacedName with input in the format of xx/xx/xx, I have no idea about solving this now. I perfer to modify this PR first. And I'll try to find a method to select correct function according to Chaos' typy later.

YangKeao

LGTM

YangKeao · 2021-08-04T04:36:42Z

But IOChaos uses ParseNamespacedName with input in the format of xx/xx/xx, I have no idea about solving this now. I perfer to modify this PR first. And I'll try to find a method to select correct function according to Chaos' typy later.

Please also record an issue as the TODO (just in case you forget).

/merge

ti-chi-bot · 2021-08-04T04:36:44Z

This pull request has been accepted and is ready to merge.

Commit hash: 209de76

YangKeao · 2021-08-04T04:36:48Z

/merge

ti-srebot · 2021-08-04T05:05:59Z

cherry pick to release-1.2 failed

ti-srebot · 2021-08-04T05:08:56Z

cherry pick to release-2.0 failed

ti-chi-bot requested review from Colstuwjx and g1eny0ung July 22, 2021 08:21

ti-chi-bot added contribution first-time-contributor labels Jul 22, 2021

ti-chi-bot added the size/S label Jul 22, 2021

cwen0 requested a review from YangKeao July 22, 2021 08:33

check length of parts

16154e7

Signed-off-by: FingerLeader <FingerLeader@gmail.com>

ti-chi-bot added size/XXL and removed size/S labels Jul 22, 2021

FingerLeader and others added 3 commits July 22, 2021 19:36

check length of parts

26c79b5

Signed-off-by: FingerLeader <FingerLeader@gmail.com> add length check of parts

chore: disable scope in advance (#2135)

a5aaae1

Signed-off-by: Yue Yang <g1enyy0ung@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Signed-off-by: FingerLeader <wanxfinger@gmail.com>

Merge branch 'master' into master

1fd55dd

ti-chi-bot added size/S and removed size/XXL labels Jul 26, 2021

Merge branch 'master' into master

857fb88

YangKeao added needs-cherry-pick-1.2 needs-cherry-pick-release-2.0 labels Jul 26, 2021

FingerLeader added 2 commits July 26, 2021 17:00

adding error throwing to ParseNamespacedName and ParseNamespacedNameC…

337e063

…ontainer Signed-off-by: FingerLeader <wanxfinger@gmail.com>

Merge branch 'master' of https://github.com/FingerLeader/chaos-mesh

dac2f8b

ti-chi-bot added size/XL and removed size/S labels Jul 26, 2021

Merge branch 'master' into master

a71b8e1

sonatype-lift bot reviewed Jul 26, 2021

View reviewed changes

Colstuwjx requested changes Aug 2, 2021

View reviewed changes

FingerLeader requested a review from Colstuwjx August 2, 2021 06:24

change content of setupLog

90ad42c

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

Colstuwjx reviewed Aug 2, 2021

View reviewed changes

controllers/common/fx.go Show resolved Hide resolved

change content of setupLog

209de76

Signed-off-by: FingerLeader <wanxfinger@gmail.com>

FingerLeader requested a review from Colstuwjx August 2, 2021 10:49

Colstuwjx approved these changes Aug 2, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 label Aug 2, 2021

YangKeao approved these changes Aug 4, 2021

View reviewed changes

ti-chi-bot added status/LGT2 and removed status/LGT1 labels Aug 4, 2021

ti-chi-bot added the status/can-merge label Aug 4, 2021

ti-chi-bot added 2 commits August 4, 2021 12:36

Merge branch 'master' into master

128c47e

Merge branch 'master' into master

7192928

ti-chi-bot merged commit cff3ed6 into chaos-mesh:master Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check the length of parts while using ParseNamespacedName and ParseNamespacedNameContainer #2141

Check the length of parts while using ParseNamespacedName and ParseNamespacedNameContainer #2141

FingerLeader commented Jul 22, 2021 •

edited

ti-chi-bot commented Jul 22, 2021

YangKeao commented Jul 22, 2021

codecov-commenter commented Jul 22, 2021 •

edited

YangKeao commented Jul 26, 2021

FingerLeader commented Jul 26, 2021

STRRL commented Jul 26, 2021

sonatype-lift bot Jul 26, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx Aug 2, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx left a comment

FingerLeader commented Aug 2, 2021

YangKeao commented Aug 3, 2021

FingerLeader commented Aug 3, 2021

YangKeao left a comment

YangKeao commented Aug 4, 2021

ti-chi-bot commented Aug 4, 2021

YangKeao commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

Check the length of parts while using ParseNamespacedName and ParseNamespacedNameContainer #2141

Check the length of parts while using ParseNamespacedName and ParseNamespacedNameContainer #2141

Conversation

FingerLeader commented Jul 22, 2021 • edited

What problem does this PR solve?

What is changed and how it works?

Checklist

ti-chi-bot commented Jul 22, 2021

YangKeao commented Jul 22, 2021

codecov-commenter commented Jul 22, 2021 • edited

Codecov Report

YangKeao commented Jul 26, 2021

FingerLeader commented Jul 26, 2021

STRRL commented Jul 26, 2021

sonatype-lift bot Jul 26, 2021

Choose a reason for hiding this comment

FingerLeader commented Aug 2, 2021

Colstuwjx Aug 2, 2021

Choose a reason for hiding this comment

FingerLeader commented Aug 2, 2021

Colstuwjx commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

FingerLeader commented Aug 2, 2021

Colstuwjx left a comment

Choose a reason for hiding this comment

FingerLeader commented Aug 2, 2021

YangKeao commented Aug 3, 2021

FingerLeader commented Aug 3, 2021

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao commented Aug 4, 2021

ti-chi-bot commented Aug 4, 2021

YangKeao commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

FingerLeader commented Jul 22, 2021 •

edited

codecov-commenter commented Jul 22, 2021 •

edited