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

Add dc.Spec.Config validation for properties #224

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

burmanm
Copy link
Contributor

@burmanm burmanm commented Nov 2, 2021

What this PR does:
Adds scripts to generate configuration from Cassandra's source code for all the possible values. Also adds validation to webhook and the reconcile function to check that values that are not available in that version are also not passed to the config generator.

To run the generator:

make generate-configs

Supports OSS Cassandra only at this point, for versions 4.0., 3.11. and the trunk.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CHANGELOG.md updated (not required for documentation PRs)
  • CLA Signed: DataStax CLA

@burmanm burmanm requested a review from a team as a code owner November 2, 2021 20:23
@burmanm burmanm force-pushed the config_test branch 3 times, most recently from 94b5591 to ffe1cc2 Compare November 2, 2021 20:28
@burmanm
Copy link
Contributor Author

burmanm commented Nov 2, 2021

@jsanda @miles This work should support removing the config-builder also at some point. All we need is templating something else instead of only maps (or we could push default values here and other stuff the config-builder does). Maybe that could help with the non-root issue also?

For future work, we could also of course help users by cleaning the config file from values that are not allowed and simply make events when those occur. At the same time, we could add notifications that the values are deprecated also and user should migrate to something else.

@Miles-Garnsey
Copy link
Member

Miles-Garnsey commented Jan 10, 2022

I'm just getting around to looking at this properly, apologies it has taken me so long. This is, in general, a really good idea. Having a structured way to address questions like the following will be really helpful:

  • What configs are allowed?
  • What configs are deprecated?
  • Do user configs cause conflicts with those that we need to control?
  • How do we version configuration between major versions?

help users by cleaning the config file from values that are not allowed and simply make events when those occur

I'd be cautious about creating resources which don't fully satisfy the user's requested spec. Where we can raise errors via the webhook I think this offers fast failure and should be preferred. Sometimes, having an unexpected configuration can cause quite serious issues (e.g. if audit logging was not enabled, that could escalate to being a security issue).

I was originally envisaging doing something similar to this using Java's reflection capabilities, but I think the choice of Python is also totally fine.

My main questions would be:

  • Should we think about outputting the configs into an intermediate form (e.g. protobuf) so that we can access the data structures from any language? I wonder if autogenerating Go directly (without relying on a language-agnostic intermediate representation) might trip us up later if we need to analyse configs from yet another language.
  • Related: would it be feasible to use structs instead of maps here? That might provide a little bit more structure and avoid a lot of the verbose if !ok{...} logic we see (especially in my own code!) If we used protobufs that'd probably offer us automatic struct creation anyway.
  • Irrespective of whether we go for protobuf-based struct code generation, or something else, we probably need to think about using tags or protobuf equivalent to ensure we can adhere to Go's camelcase field naming requirements while preserving the original yaml names.
  • I need to investigate further here, but I'm wondering how we're coping with data types in the current setup. E.g. how do we make sure that a field that the Cassandra config needs to be an int is not provided as a string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants