Edit a CustomResource should result in a patch #2923

andreaTP · 2021-03-17T19:18:41Z

Description

Trying to use the edit of a CR I found a couple of issues:

it throws ClassCastException if there are external json modules registered e.g.: https://github.com/andreaTP/fabric8-edit-as-patch/blob/master/src/main/scala/Main.scala#L59

edit perform a PUT while it's supposed to be a PATCH as per:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/HasMetadataOperation.java

Lines 37 to 39 in 8d0957c

    
           public T edit(UnaryOperator<T> function) { 
        
             T item = getMandatory(); 
        
             return patch(function.apply(item));

this PR address both issues.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

I tested the final result with this demo project:
https://github.com/andreaTP/fabric8-edit-as-patch
run with: sbt run

I don't know how/where to add more exhaustive tests, I'm looking forward to guidance here 🙏

centos-ci · 2021-03-17T19:18:42Z

Can one of the admins verify this patch?

andreaTP · 2021-03-17T19:34:38Z

...src/main/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImpl.java

@@ -346,9 +360,6 @@ public RawCustomResourceOperationsImpl dryRun(boolean isDryRun) {
   * @throws IOException in case of network/serialization failures or failures from Kubernetes API
   */
  public Map<String, Object> edit(String namespace, String name, String objectAsString) throws IOException {
-    // Append resourceVersion in object metadata in order to
-    // avoid : https://github.com/fabric8io/kubernetes-client/issues/1724


This is already implemented here:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImpl.java

Line 882 in 6e439a8

metadata.put(RESOURCE_VERSION, originalResourceVersion);

rohanKanojia · 2021-03-18T06:09:48Z

A test seems to be failing:

[INFO] Results:
[INFO] 
Error:  Errors: 
Error:    CustomResourceTest.testEdit:197 » KubernetesClient Failure executing: PATCH at...
[INFO] 
Error:  Tests run: 670, Failures: 0, Errors: 1, Skipped: 4
[INFO] 
[INFO] ------------------------------------------------------------------------

andreaTP · 2021-03-18T11:22:24Z

@rohanKanojia I should have fixed the failing test, I'm having troubles with the build process, it often fails ... any hint on how to get it more reliable?

rohanKanojia · 2021-03-18T11:31:20Z

You mean running tests via ide? I agree it's painful. Could you please check if this helps #2676 (comment)

andreaTP · 2021-03-18T11:55:46Z

yesterday I was working in the IDE, today the IDE stopped to be able to compile the project and I have had to test every time with a mvn clean install command (and crazy long feedback loop cycle as you can imagine ...).
Re-importing failed at least 3 times ...

rohanKanojia · 2021-03-18T12:01:24Z

You don't have to compile everything every time. Since you've made changes to kubernetes-client only, you should be able to check kubernetes-client and kubernetes-tests build okay.

However, you might need to build a project the first time. I use -DskipTests which compiles project in 10 minutes

andreaTP · 2021-03-18T12:33:17Z

I think that the CI failure is unrelated now

rohanKanojia · 2021-03-18T12:38:40Z

Don't worry, I've restarted the workflow

andreaTP · 2021-03-22T10:19:56Z

Do I need to do anything more for this PR?

manusa · 2021-03-26T08:32:02Z

...src/main/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImpl.java


  private RawCustomResourceOperationsImpl(OkHttpClient client, Config config, CustomResourceDefinitionContext crdContext, String namespace, String name, long gracePeriodInSeconds, boolean cascading, String deletionPropagation, ListOptions listOptions, boolean dryRun) {
    super(client, config);
    this.customResourceDefinition = crdContext;
-    this.objectMapper = Serialization.jsonMapper();
+    this.objectMapper = new ObjectMapper();


Why can't we reuse the global ObjectMapper singleton instance?

You can test from my example:
https://github.com/andreaTP/fabric8-edit-as-patch/blob/8035f945a3c0be2db239c92632dc91bb4dda6d9e/src/main/scala/Main.scala#L59

After registering DefaultScalaModule the global ObjectMapper will deserialize as Maps as "scala Maps" resulting in class cast exception e.g. here:

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/RawCustomResourceOperationsImpl.java

Line 870 in e60c634

Map<String, Object> metadata = (Map<String, Object>) objectAsMap.get(METADATA);

OK, that's very specific for your use case, and I'm sure you are having issues in other places around the code-base.

We should really need to find a neater way to address the problem.

Especially because if we start creating ObjectMapper instances across the different service implementations we'll have more problems in the future.

Please just propose me a solution, I will go-ahead 🙂
Here we RELY on having a clean ObjectMapper, but, as said I'm open to suggestions!

Just to mention, no, fortunately, I have no other Serialization issues:

Cloudflow Operator handling 3 different CR, one CRD, ConfigMaps, Secrets, Volumes etc. etc.

Cloudflow CLI reading and writing again CRs, Secrets and more

@manusa any suggestion to unblock this PR?
should we use the PatchUtils ObjectMapper?

kubernetes-client/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/internal/PatchUtils.java

Line 40 in 3ed4809

public static ObjectMapper patchMapper() {

I still think that a fresh one is more appropriate here until we have a more "structural" solution.
WDYT?

Yes, that seems very reasonable.

andreaTP · 2021-04-01T12:49:54Z

rebased on the latest master, anything else I can do to get this moving?

rohanKanojia · 2021-04-05T11:07:54Z

@andreaTP : Thanks for updating as per review comments. I think we can get this merged once Marc approves it.

manusa

LGTM, thx!

sonarcloud · 2021-04-07T09:10:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

93.3% Coverage
0.0% Duplication

andreaTP commented Mar 17, 2021

View reviewed changes

manusa requested review from rohanKanojia and manusa March 22, 2021 13:30

andreaTP mentioned this pull request Mar 22, 2021

Remove local workaround for fabric8 patching lightbend/cloudflow#1006

Open

rohanKanojia approved these changes Mar 23, 2021

View reviewed changes

manusa reviewed Mar 26, 2021

View reviewed changes

andreaTP added 7 commits April 1, 2021 13:48

CustomResource edit as PATCH

20b8893

cleanup

cf76111

more cleanup

9de4505

more cleanup

ef9f4d3

remove outdated comment

1a47309

fix the test

ae80f8c

minor cleanups

e642d5c

andreaTP force-pushed the edit-cr-patch branch from e60c634 to e642d5c Compare April 1, 2021 12:48

manusa added this to the 5.3.0 milestone Apr 6, 2021

rohanKanojia requested a review from oscerd April 6, 2021 13:04

patchutils object mapper

256511f

manusa approved these changes Apr 7, 2021

View reviewed changes

manusa merged commit 07f77b1 into fabric8io:master Apr 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit a CustomResource should result in a patch #2923

Edit a CustomResource should result in a patch #2923

andreaTP commented Mar 17, 2021 •

edited

Loading

centos-ci commented Mar 17, 2021

andreaTP Mar 17, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 22, 2021

manusa Mar 26, 2021

andreaTP Mar 26, 2021 •

edited

Loading

manusa Mar 26, 2021

andreaTP Mar 26, 2021

andreaTP Mar 30, 2021

manusa Apr 6, 2021

andreaTP commented Apr 1, 2021

rohanKanojia commented Apr 5, 2021

manusa left a comment

sonarcloud bot commented Apr 7, 2021

	public T edit(UnaryOperator<T> function) {
	T item = getMandatory();
	return patch(function.apply(item));

Edit a CustomResource should result in a patch #2923

Edit a CustomResource should result in a patch #2923

Conversation

andreaTP commented Mar 17, 2021 • edited Loading

Description

Type of change

Checklist

centos-ci commented Mar 17, 2021

andreaTP Mar 17, 2021

Choose a reason for hiding this comment

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 18, 2021

rohanKanojia commented Mar 18, 2021

andreaTP commented Mar 22, 2021

manusa Mar 26, 2021

Choose a reason for hiding this comment

andreaTP Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

manusa Mar 26, 2021

Choose a reason for hiding this comment

andreaTP Mar 26, 2021

Choose a reason for hiding this comment

andreaTP Mar 30, 2021

Choose a reason for hiding this comment

manusa Apr 6, 2021

Choose a reason for hiding this comment

andreaTP commented Apr 1, 2021

rohanKanojia commented Apr 5, 2021

manusa left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Apr 7, 2021

andreaTP commented Mar 17, 2021 •

edited

Loading

andreaTP Mar 26, 2021 •

edited

Loading