[RFC] Retire class `ConfigConstants` and rethink way of defining static configuration

Introdution

This is a proposal for changing the approach of defining configuation options in the security plugin code base. First, this issue describes the current situation and the potential issues with that situation. Afterwards, it gives a proposal for a new improved approach.

This is an RFC, so everyone is invited to share their thoughts and opinions on this.

Situation

The class ConfigConstants is a long list of mostly constant string values.

See: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/ConfigConstants.java

Some of the constants are related to settings. Usually, names of settings are defined. Sometimes default values:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/support/ConfigConstants.java#L154-L169

Most of such constants are only used two times:

First usage: At the place where it is read:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/configuration/AdminDNs.java#L58-L63

Second usage: At the place where it is registered (that's always OpenSearchSecurityPlugin):

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1445-L1452

You can see in the definition in OpenSearchSecurityPlugin, that more metadata is just given directly inside of the Setting object. That's for example the type of the setting, the scope, optionally a parser or the scope.

Often, there are a couple of further uses appear inside tests. However, it is debatable whether these constants should be used in tests (but that's another discussion).

You can also see that most of the constants are not documented by JavaDoc comments.

Sometimes, config default values are defined as additional constants in ConfigConstants. Often, they are defined directly below the constants defining the name of the constant. Sometimes, they are defined directly in OpenSearchSecurityPlugin:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1538-L1551

Additionally, many constants in ConfigConstants are actually not related to configuration. Names of thread context headers are also commonly found here. There's no directly clear way to distinguish between config constants and thread context header names:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/support/ConfigConstants.java#L75-L87

Besides names of thread context headers, there are a couple of values used for diverse purposes, like tenant names:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/support/ConfigConstants.java#L373-L375

Or privilege names:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/support/ConfigConstants.java#L363

Analysis

My opinion is: The class ConfigConstants is a bad approach to defining configuration options. It has these issues:

• Its use is unclear, indicated by the presence of many unrelated constants
• It does not facilitate code reuse, indicated by the little use by the constants (usually only two places)
• ConfigConstants is usually not enough to learn about all properties of a setting. If I want to learn about the type of a setting, I usually have to navigate to OpenSearchSecurityPlugin and search for the registration of the constant.
• It is difficult to navigate

As the class keeps growing, the problem will only get worse. This shows that there is a need to rethink things.

Proposal

This is my proposal for a new approach. It is just "IMHO", let's discuss.

• Deprecate ConfigConstants. There is no need to rewrite everything with a big bang, but one should write new code using the new pattern. Old code can be migrated step by step.
• No longer define the name of config options and the default value as separate constants. Rather, define a constant which is the instance of org.opensearch.common.settings.Setting. This class is immutable, thus it is suitable to be used as a constant. Instances of this class combine all necessary meta data: Config option name, default value, scope, even validation options. These settings definitons should be accompanied by a JavaDoc comment:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java#L58-L71

• These Setting constants should be placed in the classes that actually use/read them, not in a central bag.

• They are referenced in OpenSearchSecurityPlugin just for registration:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L2150-L2151

• The value of the settings should be read using the get() method inside the Setting instance. This gives you type safe access of the values including automatic default fallback:

https://github.com/opensearch-project/security/blob/79a663e1ca912457a5fbe29d75293a2bca92b8c7/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java#L103

• The actual name of the Setting constant should be passed to the factory method just as a plain Java String. It should be no reference to another constant and should be also not concatenated like SECURITY_AUTHCZ_ADMIN_DN = SECURITY_SETTINGS_PREFIX + "authcz.admin_dn". This gives users of the code both the ability to grep for the config names and to just directly copy the config names if they want to use it when configuring a cluster (sometimes referring to the code is better than referring to the documentationn).

Start by reading src/main/java/org/opensearch/security/support/ConfigConstants.java, the referenced AdminDNs.java and OpenSearchSecurityPlugin.java usages, and the ActionPrivileges.java example. Compare how settings are declared, registered, and read; this RFC is complete only after the project agrees on a concrete replacement pattern and migration scope, which the issue does not yet define.