-
Notifications
You must be signed in to change notification settings - Fork 19
Fix boolean to string convertion from cluster.yaml #774
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
base: main
Are you sure you want to change the base?
Conversation
| 'dict-option': {'foo': 'bar'}, | ||
| } | ||
| nginx['custom_headers'] = { | ||
| 'X-Enabled': False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly, that k8s ConfigMap requires this False to be specified in quotes, i.e. "False"? And if we specify it without quotes in ConfigMap, then it fails? Also, do I understand correctly that if we specify it in KubeMarine in quotes, then everything works fine, KubeMarine passes it in quoutes? If this is correct, then I do not see a problem then. We just need to note that these fields should be strings. Maybe we can enforce it with JSON schema?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ConfigMap data (and Secret stringData/binaryData) must be strings. If YAML has false unquoted, it’s parsed as a boolean and the Kubernetes API will reject it (data is map[string]string). If you put "false" (or 'false') it stays a string and works.
For KubeMarine specifically:
If you quote in cluster.yaml, it stays a string all the way through, and the manifest is valid.
If you don’t quote, it becomes a boolean in Python; without the stringification fix it can render as a boolean and fail at apply time. With the new stringification logic in utils.py, even unquoted values are normalized to strings before dumping, so it will work regardless.
So the “problem” is really about user input being typed by YAML, not about Kubernetes accepting booleans.
About enforcing via JSON schema:
Yes we can enforce type: string for the relevant maps which will push users to quote values,
If we want to be user-friendly, then we keep the schema permissive (allow string/number/boolean/null) and rely on the normalization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ConfigMap data (and Secret stringData/binaryData) must be strings. If YAML has false unquoted, it’s parsed as a boolean and the Kubernetes API will reject it (data is map[string]string). If you put "false" (or 'false') it stays a string and works.
If this is how k8s works, then I think we should do the same, without our own conversion (because conversion adds unnecessary complexity and maintenance efforts).
If we want to be user-friendly, then we keep the schema permissive (allow string/number/boolean/null) and rely on the normalization
Restricting JSON schema is user-friendly enough, I think. Users will see error immediately and will be able to fix it. We do not have much complains from users.
I suggest to implement it as JSON schema check. If it is too complicated, then not change anything at all until there is request from real user.
- https://json-schema.org/understanding-json-schema/reference/object#additionalproperties
KubeMarine/kubemarine/resources/schemas/definitions/plugins/nginx-ingress-controller.json
Lines 68 to 70 in 6ae54fb
"config_map": { "type": "object", "description": "Customize or fine tune NGINX behavior"
Description
Kubemarine takes values from cluster.yaml and injects them into Kubernetes manifests. When a value ends up under a ConfigMap.data (or Secret.stringData) block, the Kubernetes API requires it to be a plain string. When cluster.yaml used bare booleans (e.g., use-proxy-protocol: true), so when Kubemarine rendered the manifest it produced a JSON boolean. kubectl apply rejected it with error
json: cannot unmarshal bool into Go struct field ConfigMap.data of type stringThe request is invalid: patch: Invalid value: "map[data:map[use-proxy-protocol:true] metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"allow-snippet-annotations\":\"true\",\"proxy-buffer-size\":\"128k\",\"use-proxy-protocol\":true},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/component\":\"controller\",\"app.kubernetes.io/instance\":\"ingress-nginx\",\"app.kubernetes.io/name\":\"ingress-nginx\",\"app.kubernetes.io/part-of\":\"ingress-nginx\",\"app.kubernetes.io/version\":\"1.8.4\"},\"name\":\"ingress-nginx-controller\",\"namespace\":\"ingress-nginx\"}}\n] labels:map[app.kubernetes.io/version:1.8.4]]]": unrecognized type: string*
Fixes # (issue)
Solution
Added a normalization step inside the manifest.py. Before any manifest is dumped, it will deep-copy each Kubernetes object and run a new helper set in kubemarine/core/utils.py:
stringify_string_map_value converts booleans to 'true'/'false', None to '', numbers to decimal strings, and serializes lists/dicts via json.dumps.
stringify_string_map and stringify_string_data_fields walk the data, stringData, and binaryData sections of ConfigMaps/Secrets and apply those conversions.
Test Cases
TestCase 1
Test Configuration:
Steps:
Results:
Results:
Checklist
Unit tests
Indicate new or changed unit tests and what they do, if any.