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 #4136: adding support for fieldValidation #4524

Merged
merged 2 commits into from Nov 7, 2022
Merged

Conversation

shawkins
Copy link
Contributor

@shawkins shawkins commented Oct 24, 2022

Description

Fixes #4136 with general support for field validation. This is mostly of interest for users who could be dealing with multiple versions, or manually constructing generic resources, or manually constructing patches. As long as you are using the typed api that matches the server version and not setting additionalProperties - all fields should be valid.

Part of the motivation here is just to see what level of effort it would take to move more of the PatchContext to dsl methods as we move towards #3896 to add a dsl method for apply. One issue that we'll have - unless we further refine the interfaces - is that there will be a conflict between the context dsl methods and the context passed in. In some places we consider the dsl forcing - such as when you perform a watch and pass in ListOptions. Here I'm assuming that the passed in context trumps the dsl methods - https://github.com/fabric8io/kubernetes-client/compare/master...shawkins:4136?expand=1#diff-628edeb9a79ef1b9f8a55c24437fad0e1a5b8aa34887214e948524c6811cc1dbL226

Beyond this kubectl allows you to set the fieldManager and force conflicts, so this could fully look like:

resource(foo)
  .fieldValidation(Validation.STRICT).fieldManager("override the default").forceConflicts().serverSideApply();

In general the fieldManager forceConflicts only applies to serverSideApply, so only the serverSideApply would be available after those calls.

vs

resource(foo).patch(new PatchContext.Builder().withPatchType(PatchType.SERVER_SIDE_APPLY).withFieldManager("override the default").withFieldValiation("Strict").withForce(true).build());

Thoughts?

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

@shawkins
Copy link
Contributor Author

There's no integration test yet as the feature is gated.

@manusa
Copy link
Member

manusa commented Nov 4, 2022

There's no integration test yet as the feature is gated.

The gate is enabled by default in K8s 1.25.

Setting the field validation level
Provided that the ServerSideFieldValidation feature gate is enabled (disabled by default in 1.23 and 1.24, enabled by default starting in 1.25), you can take advantage of server side field validation to catch these unrecognized fields.

An IT can be added with the appropriate @RequireK8sVersionAtLeast(majorVersion = 1, minorVersion = 25) annotation.

String parameterValue;

private Validation() {
this.parameterValue = this.name().charAt(0) + this.name().toLowerCase().substring(1);
Copy link
Member

@manusa manusa Nov 4, 2022

Choose a reason for hiding this comment

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

It would probably make this more readable by providing this statically:

WARN("Warn"),
//...
private Validation(String parameterValue) {
//...

@shawkins
Copy link
Contributor Author

shawkins commented Nov 4, 2022

An IT can be added with the appropriate @RequireK8sVersionAtLeast(majorVersion = 1, minorVersion = 25) annotation.

Sure, I just hadn't seen that it was enabled yet in 1.25. I can add something if you like.

@manusa
Copy link
Member

manusa commented Nov 4, 2022

An IT can be added with the appropriate @RequireK8sVersionAtLeast(majorVersion = 1, minorVersion = 25) annotation.

Sure, I just hadn't seen that it was enabled yet in 1.25. I can add something if you like.

🙏 That'd be nice, it can be used as a reference on how to use the new feature too.

@manusa manusa added this to the 6.3.0 milestone Nov 4, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 4, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

68.3% 68.3% Coverage
0.0% 0.0% Duplication

@manusa manusa merged commit ba04a46 into fabric8io:master Nov 7, 2022
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.

Generally support fieldValidation
2 participants