[JENKINS-71956] Octal notation fixes #1503
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Support more paths to convert modes * Allows to disable conversion for people who converted their manifests to decimal notation.
aneveux
approved these changes
Jan 19, 2024
jglick
reviewed
Jan 19, 2024
| * | ||
| * The user can also provide permissions as a decimal integer, e.g. 511. | ||
| * | ||
| * This method attempts to guess whether the user provided a decimal or octal integer, and converts to octal if needed, |
There was a problem hiding this comment.
I wonder if we could make this more reliable by checking the octal representation (reference). Actual modes will normally have the group bits match either the user bits or the other bits (it would be weird to have three different octets). And normally u ≥ g ≥ o and w ≥ r ≥ x right? For example 777 satisfies all constraints whereas 511 in octal would be -r-x--x--x which violates r ≥ x for g/o and is thus technically legal for exotic scenarios but unlikely to be what the user meant.
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Kubernetes-client uses jackson-databind to parse yaml into objects, using snakeyaml.
Kubernetes-client has migrated their serialization methods to use SnakeYAML engine (fabric8io/kubernetes-client#4836)
The problem is that snakeyaml uses yaml 1.1 while snakeyaml-engine only supports yaml 1.2.
This causes inconsistencies when dealing with integer with the octal notation, as there are breaking changes between these two versions. Additionally, FasterXML/jackson-dataformats-text#3 documents that octal notation is not properly supported through jackson.
Plugin perspective
This follows up #1342, already providing some octal notation conversion.
User yaml is still expected to use yaml 1.1, using the
0xxxnotation for octal integers. For a series of known paths in the pod spec, an automatic conversion will happen.This behaviour can be disabled using
-Dorg.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.DISABLE_OCTAL_MODES=truein case users have converted their yaml manifests to use decimal notation instead. A best effort is attempted to keep modes that have been provided using decimal notation, but this can't be foolproof.Going forward, jackson 3.0 will use SnakeYAML engine so hopefully we will eventually get a more consistent behaviour, but I expect possible new breaking changes when it happens.
This adds a few paths I missed when I initially addressed this kubernetes-client upgrade in #1342, as well as providing unit tests.
Testing done
Submitter checklist