Skip to content

Merge typed role and role group configs #425

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

Closed
wants to merge 31 commits into from

Conversation

maltesander
Copy link
Member

@maltesander maltesander commented Jun 23, 2022

Description

This PR adds a macro Optional that must be used in the FooConfig(s) of our operators.

# use stackable_operator::config::optional::Optional;
const DEFAULT_PORT: u16 = 11111;
// For example, this:
#[derive(Optional)]
struct FooConfig {
    #[optional(default_value = "DEFAULT_PORT")]
    port: u16,
}
// Expands to (roughly) the following:
#[derive(Merge)]
struct OptionalFooConfig {
    port: Option<u16>,
}
impl From<OptionalFooConfig> for FooConfig {
   fn from(c: OptionalFooConfig) -> Self {
       Self {
           port: c.port.unwrap_or(DEFAULT_PORT),
       }
   }
}

The two created Configs are contained in the CommonConfiguration in an enum like:

pub enum Config<O, M>
where
    O: Clone + Default + Merge,
    M: Configuration + From<O>,
{
    #[serde(rename = "config")]
    Optional(O),
    #[serde(skip)]
    Merged(M),
}

The Optional part representing OptionalFooConfig will be exposed in the CRD. After deserilization and merging the operators work with the Merged part containing the original FooConfig. To retrieve the "original" merged FooConfig in the code, a get method must be called. This is breaking as well as the new Role parametrization:

let port = rolegroup.config.config.get().port;

This means we now need to configure our Role in the FooClusterSpec like:

#[derive(Clone, CustomResource, Debug, Default, Deserialize, JsonSchema, PartialEq, Serialize)]
#[kube(
    group = "foo.stackable.tech",
    version = "v1alpha1",
    kind = "FooCluster",
    plural = "fooclusters",
    shortname = "foo",
    namespaced,
    crates(
        kube_core = "stackable_operator::kube::core",
        k8s_openapi = "stackable_operator::k8s_openapi",
        schemars = "stackable_operator::schemars"
    )
)]
#[serde(rename_all = "camelCase")]
pub struct FooClusterSpec {
   #[serde(default, skip_serializing_if = "Option::is_none")]
    pub foo_server: Option<Role<OptionalFooConfig, FooConfig>>,
}

Additionally, the Role and RoleGroup have a custom deserializer that merges the Role and RoleGroups OptionalFooConfig including the defaults. That means after deserialization no other merging has to be done.

FYI: It works with ZooKeeper, im not fully happy with the created macro which still has some flaws (e.g. the derives of the OptionalFooConfig are currently hardcoded). There are still alot of clones in the code which i wasnt able yet to remove. Additionally, the merge is not working yet for complex structs. They are Optional in the OptionalFooConfig and due to the Merge trait need an impl for Atomic (which basically stops the merge and just takes whatever is behind the option).

@teozkr I really need you to have a look at the general idea/structure and especially the macro :)

Operator implementation: stackabletech/zookeeper-operator#508

fixes: #284

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@maltesander maltesander requested review from razvan, nightkr, sbernauer and a team June 23, 2022 10:52
nightkr added a commit that referenced this pull request Jul 5, 2022
A slightly different take of #425, which uses the existing Atomic infra
and some trait magic to determine "complex" types.

It also tries to provide nice error messages for validation failures.
@maltesander
Copy link
Member Author

will be handeld with a different approach by @teozkr. Closing for now.

bors bot pushed a commit that referenced this pull request Oct 17, 2022
## Description

This is an alternate take on #425. Essentially, we define a new macro `Fragment`, which creates a "parallel fragment universe" where everything is optional. For example, given the following types:

```rust
#[derive(Fragment)]
struct Outer {
    foo: u32,
    inner: Inner,
}

#[derive(Fragment)]
struct Inner {
    required: String,
    optional: Option<String>,
}
```

we generate the following fragment variants:

```rust
struct OuterFragment {
    foo: Option<u32>,
    inner: InnerFragment, // not an atomic type, so make every subfield optional instead
}

struct InnerFragment {
    required: Option<String>, // was non-optional, so make it optional
    optional: Option<String>, // was already optional, so preserve the type
}
```

as well as validation functions that turn `OuterFragment` into `Result<Outer>` (and an equivalent for `Inner`), while giving us the full context when a validation error occurs.

This allows us to safely deserialize and merge the fragment variants, and only afterwards validate that all required options are present in the final merged configuration.

This is a pretty big and breaking change, since it moves all `Serialize` and `Deserialize` impls on the resources into the fragment variants.



Co-authored-by: Teo Klestrup Röijezon <teo.roijezon@stackable.de>
@lfrancke lfrancke deleted the merge_typed_role_and_role_group_configs branch April 26, 2023 09:26
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.

Merge typed role and rolegroup configs before converting to product-config maps
1 participant