stackabletech / operator-rs

A simple wrapper/framework around kube-rs to make implementing Operators/Controllers easier
Apache License 2.0
116 stars 12 forks source link

fix: Implement custom schema for `config.affinity.nodeSelector` #752

Closed sbernauer closed 5 months ago

sbernauer commented 5 months ago

Description

Fixes https://github.com/stackabletech/issues/issues/554.

IMHO it's not worth an immediate platform bugfix release, as there is a workaround available.

The effect can be seen e.g. in Trino:

diff --git a/deploy/helm/trino-operator/crds/crds.yaml b/deploy/helm/trino-operator/crds/crds.yaml
index e41fdfb..5de8694 100644
--- a/deploy/helm/trino-operator/crds/crds.yaml
+++ b/deploy/helm/trino-operator/crds/crds.yaml
@@ -305,6 +305,8 @@ spec:
                                   type: object
                               type: object
                             nodeSelector:
+                              additionalProperties:
+                                type: string
                               nullable: true
                               type: object
                             podAffinity:
@@ -647,6 +649,8 @@ spec:
                                     type: object
                                   type: array
                               type: object
+                          required:
+                            - nodeSelector
                           type: object

Definition of Done Checklist

# Author
- [x] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Integration tests passed (for non trivial changes)
# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)
# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added
Techassi commented 5 months ago

IMHO it's not worth an immediate bugfix release, as there is a workaround available.

I would be happy to release a new version, because there are now multiple things which are unreleased.

Techassi commented 5 months ago

I would however do one additional PR regarding the changelog before bumping the version.

sbernauer commented 5 months ago

I already changed the wording the "bugfix platform version", sorry for the confusion. No objections against op-rs release whatsoever

nightkr commented 5 months ago

I tried diffing the output before/after this patch: https://gist.github.com/nightkr/9446c9e3747aa6f58ce616cd86c1f13e

As far as I can tell, the actual issue here is that schemars doesn't propagate additionalProperties when used with #[serde(flatten)]? We should probably file an upstream ticket for that and link to it in the code.

nightkr commented 5 months ago

This looks like https://github.com/GREsau/schemars/issues/259.

nightkr commented 5 months ago

https://github.com/GREsau/schemars/pull/275 generates the schema https://gist.github.com/nightkr/f7f06df7a14286a9b682ad6be7e3da14, which should also contain the correct additionalProperties.

nightkr commented 5 months ago

Another workaround is to add #[schemars(deny_unknown_fields)] to StackableNodeSelector: https://gist.github.com/nightkr/af2236b2d3cf77eb71242124d588a638

sbernauer commented 5 months ago

Many thanks for tracking this down @nightkr! What would you suggest as the next step?

  1. Merge as is
  2. Use #[schemars(deny_unknown_fields)]
  3. Wait for a few days to see if https://github.com/GREsau/schemars/pull/275 get's merged
  4. Something else
nightkr commented 5 months ago

@sbernauer I'd say 2 and add a tracking issue for 3. schemars doesn't look too lively these days so we shouldn't block on it.

sbernauer commented 5 months ago

Makes sense to me! Adopted Option 2 and created https://github.com/stackabletech/operator-rs/issues/756. A quick review would be great, so that @Techassi can release :)