add APIVersion and Kind checks for helper.ToUnstructured() #6028

zach593 · 2025-01-08T15:42:29Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This issue blocks me to continue #6017.

In function helper.ToUnstructured(), karmada will convert other types of object to unstructured.Unstructured, but if object is missed TypeMeta, the generated unstructured.Unstructured will also missed APIVersion and Kind, of course in most of the time these are harmful.

Missing TypeMeta objects usually come from typed clients or objects that manually created in code.
In kubernetes issue kubernetes/client-go#541 talked about typed client , looks like these guys won't fix this.

This issue affected many tests, and caused some tests to falsely report success. I fixed all the affected code I could see, but there may be still some I haven't noticed. This may also have an impact in non-test code, which I need to confirm further.

After fixing all the affected codes, I think adding an APIVersion and Kind checks to helper.ToUnstructured() can prevent the impact from expanding further. However, this check obviously needs to be added after ensuring that all the affected codes have been modified to avoid causing a stronger impact.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2025-01-08T16:14:25Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 26.31579% with 14 lines in your changes missing coverage. Please review.

Project coverage is 47.98%. Comparing base (72b6bd7) to head (0d376ca).
Report is 82 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/scheduler/event_handler.go	27.27%	6 Missing and 2 partials ⚠️
pkg/util/helper/unstructured.go	25.00%	4 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6028      +/-   ##
==========================================
- Coverage   48.37%   47.98%   -0.39%     
==========================================
  Files         665      668       +3     
  Lines       54836    55303     +467     
==========================================
+ Hits        26526    26537      +11     
- Misses      26592    27038     +446     
- Partials     1718     1728      +10

Flag	Coverage Δ
unittests	`47.98% <26.31%> (-0.39%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: zach593 <[email protected]>

RainbowMango

/assign

RainbowMango · 2025-02-14T02:23:11Z

pkg/scheduler/event_handler.go

 		return
 	}

-	if unstructuredOldObj.GetGeneration() == unstructuredNewObj.GetGeneration() {
-		klog.V(4).Infof("Ignore update event of object (kind=%s, %s/%s) as specification no change", unstructuredOldObj.GetKind(), unstructuredOldObj.GetNamespace(), unstructuredOldObj.GetName())


Does this unstructuredOldObj.GetKind() always return ""? If yes, is there any bad effect in addition to the log?

Does this unstructuredOldObj.GetKind() always return ""?

Yes.

Bad effects are minimal because this function only deal with rb and crb, and with or without namespace is enough to explain what is happening here. The reason I needed to change it because I update the ToUnstructured() which was obviously affected it.

OK. I will leave my question to ToUnstructured() part.

RainbowMango · 2025-02-14T02:32:03Z

I'm trying to understand what bug this PR fixes.

This issue affected many tests, and caused some tests to falsely report success. I fixed all the affected code I could see, but there may be still some I haven't noticed. This may also have an impact in non-test code, which I need to confirm further.

Do you mean tests are invalid? always returning success due to this issue? Can you help to elaborate on it?

Signed-off-by: zach593 <[email protected]>

karmada-bot · 2025-02-14T05:58:49Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rainbowmango. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zach593 · 2025-02-14T06:11:25Z

pkg/controllers/namespace/namespace_sync_controller_test.go

 		ObjectMeta: metav1.ObjectMeta{
 			Name: "test-namespace",
+			Labels: map[string]string{
+				"overridden": "false",
+			},


Do you mean tests are invalid? always returning success due to this issue? Can you help to elaborate on it?

I should have explained the invalid test in more detail, I apologize for that.

This test is typical, the reason I needed to add this label is because without typeMeta, after casting to unstructured.Unstructured, it will lose the type data and the override will not work at all, making this test meaningless.

I just updated the test to make it more precise. @RainbowMango

Thank you for the explanation, I understand now. This test attempted to enable OverridePolicy when building the Work object. However, since the APIVersion/Kind were not set in the target object, the OverridePolicy never actually took effect. Additionally, the test still passed because there were no validation checks for the OverridePolicy behavior.

RainbowMango · 2025-02-14T06:53:12Z

pkg/util/helper/unstructured.go

+	unstructuredObj := &unstructured.Unstructured{Object: uncastObj}
+	if unstructuredObj.GetKind() == "" {
+		return nil, runtime.NewMissingKindErr(klog.KObj(unstructuredObj).String())
+	}
+	if unstructuredObj.GetAPIVersion() == "" {
+		return nil, runtime.NewMissingVersionErr(klog.KObj(unstructuredObj).String())
+	}


I still can't understand why this ToUnstructured needs to be so picky, requiring apiVersion/kind to be set. In other words, what problem would arise without this?

Basically, I'm asking why we need it, note that missing apiVersion/kind is not ToUnstructured's fault, it just do the conversion job.

note that missing apiVersion/kind is not ToUnstructured's fault, it just do the conversion job.

I thought so too. But the problem is that the root cause of this kind of error is too obscure, and even if I fix that test, there is no way to prevent the next person from writing the next invalid test.
From the perspective of preventing it from happening, if there is a way to completely prevent such errors from happening, I would welcome it no matter what it is, so I chose to modify this function, which seems to be the lowest cost solution.

I thought so too. But the problem is that the root cause of this kind of error is too obscure, and even if I fix that test, there is no way to prevent the next person from writing the next invalid test.

I agree.
Another question, is doing all this just to prevent writing ineffective unit tests? Could business code potentially run into this issue due to oversight?

RainbowMango · 2025-02-15T04:22:47Z

In kubernetes issue kubernetes/client-go#541 talked about typed client , looks like these guys won't fix this.

By the way, the issue tracks this as well kubernetes/kubernetes#80609

karmada-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Jan 8, 2025

karmada-bot requested review from chaunceyjiang, jwcesign, mrlihanbo, qianjun1993 and XiShanYongYe-Chang January 8, 2025 15:42

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 8, 2025

add APIVersion and Kind checks for helper.ToUnstructured()

3afc187

Signed-off-by: zach593 <[email protected]>

zach593 force-pushed the toUnstructured branch from b9d880b to 3afc187 Compare January 10, 2025 13:04

zach593 changed the title ~~[WIP]add APIVersion and Kind checks for helper.ToUnstructured()~~ add APIVersion and Kind checks for helper.ToUnstructured() Jan 14, 2025

karmada-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2025

zach593 mentioned this pull request Jan 22, 2025

add work mutating in ctrlutil.CreateOrUpdateWork() #6076

Open

RainbowMango reviewed Jan 23, 2025

View reviewed changes

karmada-bot assigned RainbowMango Jan 23, 2025

RainbowMango mentioned this pull request Feb 10, 2025

[Performance] Enhancements in release-1.13 #6031

Open

4 tasks

RainbowMango reviewed Feb 14, 2025

View reviewed changes

update TestController_buildWorks

0d376ca

Signed-off-by: zach593 <[email protected]>

zach593 commented Feb 14, 2025

View reviewed changes

RainbowMango reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add APIVersion and Kind checks for helper.ToUnstructured() #6028

add APIVersion and Kind checks for helper.ToUnstructured() #6028

zach593 commented Jan 8, 2025

codecov-commenter commented Jan 8, 2025 •

edited

Loading

RainbowMango left a comment

RainbowMango Feb 14, 2025

zach593 Feb 14, 2025

RainbowMango Feb 14, 2025

RainbowMango commented Feb 14, 2025

karmada-bot commented Feb 14, 2025

zach593 Feb 14, 2025

RainbowMango Feb 14, 2025

RainbowMango Feb 14, 2025

zach593 Feb 14, 2025

RainbowMango Feb 15, 2025

RainbowMango commented Feb 15, 2025

add APIVersion and Kind checks for helper.ToUnstructured() #6028

Are you sure you want to change the base?

add APIVersion and Kind checks for helper.ToUnstructured() #6028

Conversation

zach593 commented Jan 8, 2025

codecov-commenter commented Jan 8, 2025 • edited Loading

Codecov Report

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Feb 14, 2025

karmada-bot commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango commented Feb 15, 2025

codecov-commenter commented Jan 8, 2025 •

edited

Loading