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

highlevel: type safety in specification #2382

Open
markus2330 opened this Issue Feb 7, 2019 · 12 comments

Comments

3 participants
@markus2330
Copy link
Contributor

markus2330 commented Feb 7, 2019

From #2374:

The parameter is intended as an alternative to a specification. If the specification is necessary anyway it is useless.

Yes, it is an alternative. But the user should be safe against wrong changes in the specification, in particular incompatible changes in types.

Not really. I don't see a situation where the highlevel API would require a 3-way merge.

You are right: with a single key the 3-way merge is trivial.

we can't avoid this redundant process

The problem is not the redundant conversion but that the user might have messed up the specification. If we check the specification (the types) at startup, this problem could not happen.

So to be more concrete, given following application:

kdb_long_t myint = elektraGetLong (elektra, "myint");

the user should be safe against an exit() from Elektra when doing:

sudo kdb setmeta /sw/org/myapp/#0/current/myint type float

Expected behavior: elektraOpen should fail, telling that the type of myint does not match with the build-in spec.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 7, 2019

Expected behavior: elektraOpen should fail, telling that the type of myint does not match with the build-in spec.

Ideally the kdb setmeta call would fail. A user of an application shouldn't be allowed to change the specification of that application at all after it has been installed. How we could implement that, I am not sure. With code generation we could mount the specification as readonly before calling elektraOpen and unmount it after elektraClose.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 7, 2019

Ideally the kdb setmeta call would fail.

Yes, of course it would fail without sudo. I changed it accordingly.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 7, 2019

it would fail without sudo

That's not what I meant. In an ideal world kdb setmeta would fail because spec/sw/org/myapp/#0/current/myint is marked as part of the specification for "myapp". Therefore it cannot be changed by the user. But the only way to implement that enforceably is, if the application it self comes with its own readonly plugin that provides the specification. Which would be a possibility for code generation but seems impractical without.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 8, 2019

This is clever, I like the idea.

Yes, code generation is useful, otherwise the spec in the application might not describe the configuration actually accessed by the application. But you could write the built-in KeySet passed to elektraOpen by hand (we obviously do not recommend to do this).

To get the behavior you described, we could mount a to-be-implemented speccheck plugin into spec/ which knows about which application lives there. Whenever someone accesses spec/, the application is executed with the argument --elektra-export-spec. When getting keys from spec/, speccheck simply returns the specification the application was compiled with (without any need to install/import the spec). When the user does a kdb setmeta, then speccheck would check if it is safe to change this part of the specification. Only a very limited number of specifications are not safe to be changed (everything related to types, but even they could be safely changed to subtypes, e.g. long_long -> long).

For everything that is allowed to be changed, speccheck has a functionality like an overlay filesystem. It only needs to store the differences of the specification.

For example:

sudo kdb mount application.ini 'spec/sw/example/highlevel/#0/current' speccheck application=highlevel ni
sudo kdb spec-mount '/sw/example/highlevel/#0/current'

kdb getmeta /sw/org/myapp/#0/current/myint # we get built-in spec from application
#> long

kdb setmeta /sw/org/myapp/#0/current/myint default 8   # it is safe to change the default value

kdb setmeta /sw/org/myapp/#0/current/myint type string
# ERROR: the application requires int here, we cannot change the specification to string

kdb setmeta /sw/org/myapp/#0/current/myint type short  # it is okay to use subtypes

In the file application.ini, only the new default value of 8 would be stored.

@Piankero What do you think?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 8, 2019

For everything that is allowed to be changed

Again, why allow anything to be changed, without recompiling the application? Allowing some changes but not others just complicates the implementation. We also need to document what can and what can't be changed. I don't see a situation where a user would ever need to change the specification of an application once it is compiled. Isn't the point of having a specification that everything is exactly the way the developer intended it and to prevent the user from breaking things?

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 9, 2019

Of course most parts of the specification is best written by the developer. She best knows how the configuration is structured, which values are allowed and what good defaults are.

But many parts in the specification can be improved by administrators, who usually do not want to recompile applications. For example, they:

  1. can make the validation even stricter because they know they follow policies how something should be (e.g. they could restrict all log files to be in /var/log, and nowhere else)
  2. they might integrate the system better, i.e., derive configuration values from other values. For example, you want all applications to accept a specific hotkey. Instead of setting the hotkey in every application every time they prefer a different hotkey, they could set symlinks (override/fallback) and have a single place where the hotkey is chosen.
  3. users maybe want to change which envvars or cmd-line options are used. E.g. if one application uses TMPDIR but the other TEMPDIR you can simply rename the env var to be more consistent.

In https://book.libelektra.org Chapter 6 there are many more examples.

@Piankero

This comment has been minimized.

Copy link
Contributor

Piankero commented Feb 10, 2019

Hey, sorry for the late response.

After reading through your conversation, let me see if I got this correctly:
The problem basically boils down to permit users to change a specification without recompiling the whole application which was elektrified. @markus2330 suggests a speccheck plugin which tracks and prohibits/permits such changes which get triggered by kdb setmeta.

I see that there are valid reasons for allowing after-compile specification changes by administrators. For my personal taste though, the effort/usability reduction does not outweigh its potential benefits you described. Also if there is a need for administrators to have a different specification they should communicate it to the devs of which the application is used. The devs could then also validate if such changes do not crash the application.

But the speccheck plugin would come with the following drawbacks for devs:

  1. Beside writing a specification, devs must also write a specification for the specification on which specifications are allowed to be changed in which way. I do not think that most devs would welcome such additional effort.
  2. I think @kodebach is correct concerning complexity issues. Having a "configuration for your configuration" seems very error prone (especially if configuration settings are merged/splitted/moved/ etc.).

For me, specifications are also more of a rigid resource which in case of changes should trigger a compilation process. Having different compiled binaries/jars/etc. for different purposes (stricter policies, different env vars, etc.) looks not too expensive for me. I think only a survey would probably enlighten us if there is a real (practical) need for such feature.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 11, 2019

The problem basically boils down to permit users to change a specification without recompiling the whole application which was elektrified.

The question is which user-interface should we provide to people who want to improve the spec.

Also if there is a need for administrators to have a different specification they should communicate it to the devs of which the application is used.

Yes, they can communicate it. But isn't it better if they also can help themselves immediately?

The devs could then also validate if such changes do not crash the application.

As said, the speccheck could guide you to only make validation stricter. If you directly patch the specification, you do not get any help.

Beside writing a specification, devs must also write a specification for the specification on which specifications are allowed to be changed in which way. I do not think that most devs would welcome such additional effort.

No, we need to do that. doc/METADATA.ini must clearly describe what is safe to change. This information then will be checked by speccheck. The information is not application-specific.

I think only a survey would probably enlighten us if there is a real (practical) need for such feature.

I am afraid a survey will not help as people generally do not know if they will use something if they do not already know it.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 11, 2019

I think we can compromise here...

Developers of components of Elektra, will have to specify in doc/METADATA.ini, how the value of a metakey can safely be made more strict (for that we need a new kind of specification). If no information is provided there, speccheck will prohibit all changes to a metakey.

I will develop the speccheck plugin that for now just requests the original specification from the application and merges it with an overlay specification of possible changes. Until we developed the specification language for doc/METADATA.ini, however, every kdbSet call to speccheck will fail.

This will allow us to finish the code generation quicker and afterwards we can still enable user changes to specifications.

I think this solution is the best we can do for now, because the specification for doc/METADATA.ini has to be quite complex, to actually be useful. While it is easy to allow none or all changes, anything more complex is difficult. Take the type metadata for example. float is more strict than double, and unsigned_long (32-bits -> max value 2^32 - 1) is more strict than double (max integer precisely represented 2^53). However, float is definitely not more strict than unsigned_long and the reverse isn't true either (max integer precisely represented by float is 2^24). So putting the values into an array and comparing indices won't work. And that's just simple enum types. Take check/range, where the rule would be the lower bound can become bigger and the upper bound can become smaller. (And that assumes we can even define any rule here because check/range is a string of two combined integers).

And then there are opt and env. At first you would think, because Elektra parses these and assigns there values to keys, they could be changed arbitrarily. But what if an application comes with a helper/wrapper script, that calls the actual application (e.g. kdb stash calls kdb rm -r). In that case the script might depend on certain options or environment variables. So either changing opt and env can never be allowed, or we do need a way for application developers to define the rules of changing the specification. The only other way would be, if the helper/wrapper script reads the specification to find the right options/environment variables, but that seems to me like a workaround not an actual solution.

To sum up: I still think allowing specification changes isn't a good idea, but we can leave the possibility open for the future and implement speccheck without it for now.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 15, 2019

I am happy with a simple hard-coded whitelist for now. So changing "description", "default", "fallback", "override", "namespace" is allowed to be changed in any way, "type", "crypto/encrypt", "env" and "opt" is only allowed to be added but not to be changed.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Feb 16, 2019

A simple whitelist won't isn't good enough for opt . Adding opt, opt/long, opt/help (could even be changed) and opt/hidden (could also be changed) is no problem, but adding opt/arg or opt/flagvalue is not always safe. The application could depend on the default values of none and 1 respectively.

If we prohibit adding opt/arg and opt/flagvalue, adding an option doesn't make much sense anymore because the new option would default to opt/arg = none and opt/flagvalue = 1. That is only useful for boolean keys.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Feb 17, 2019

Ok, then we do not allow to add any "opt*" for now.

@markus2330 markus2330 referenced this issue Feb 17, 2019

Open

METADATA.ini for tools #1374

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment