Allowing setting config vars from environment by vsoch · Pull Request #409 · singularityhub/sregistry

vsoch · 2022-10-07T13:42:35Z

This is an idea to address the same issue brought up in #408 - we want to be able to easily set variables in settings from the environment. This currently just includes values in config.py (and currently can be extended). I decided to explicitly list the names and types to have better control (e.g., instead of iterating over locals().

Signed-off-by: vsoch [email protected]

Signed-off-by: vsoch <[email protected]>

cpeel

Overall I like this approach! The challenge with doing it here at the end of config.py rather than __init__.py is that we can't override other variables set in other settings/* files, like those in tasks.py (which we would want to do in our k8s scenario as we move redis elsewhere).

I also fear that we now need to keep the lists up-to-date when a new variable is added. We could add some CI that checks to see if any variable is in the file that isn't in one of the other lists.

shub/settings/config.py

vsoch · 2022-10-07T16:52:43Z

@cpeel totally agree - let me rework this a bit (probably after work) to see if I can make it a neater solution. I don't like the redundancy either!

vsoch · 2022-10-07T17:01:58Z

@cpeel I've been considering this for some time - but it might bet time to migrate the settings folder into a single settings.py to make this more clear, generally. What do you think? I'm happy to take the first shot (and I'll refactor this PR along with it!)

cpeel · 2022-10-07T19:21:47Z

I'm not sure refactoring it into a single settings.py is required. The way you've done the importing via __init__.py is nice and it lets them all be "local" to settings.py but logically grouped. If you moved the overrides into __init__.py after the imports rather than in config.py that would ensure that the overrides apply to any file.

One way to decrease the redundancy in this approach might be to iterate over all env settings and pull out all of them with SREGISTRY_ prefixes. If there is a matching local variable, set it. If it is prefixed by 'ENABLEorDISABLE` treat it as a boolean. Otherwise try to cast it as an int, otherwise treat it as a string. This has the potential to be more error prone -- and I would need to review all of the existing settings to see if this would work -- but it's extensible and removes the redundancy. We'd still need to special-case the lists in the code, but those are actually pretty few. An idea anyway.

Another possible approach would be to have the settings in a JSON file that is specified via env variable (SREGISTRY_CONFIG_FILE). The JSON would allow the user to be explicit about the type and take care of things like lists, dicts, etc. So the override code would open and iterate over the JSON and set local settings to their JSON values. In k8s we'd just create the JSON file in the manifest and pass in the env variable to pick it up. The more I type the more I like that idea.

vsoch · 2022-10-07T19:37:47Z

Another possible approach would be to have the settings in a JSON file that is specified via env variable (SREGISTRY_CONFIG_FILE). The JSON would allow the user to be explicit about the type and take care of things like lists, dicts, etc. So the override code would open and iterate over the JSON and set local settings to their JSON values. In k8s we'd just create the JSON file in the manifest and pass in the env variable to pick it up. The more I type the more I like that idea.

I also like the idea of having this as an option, although not required. And I'm also leaning towards making everything very flexible - e.g., "provide the value directly in the file, via the config file, or environment - up to you!"

Is JSON preferred to YAML? I think (for configs like this) my preference would be YAML, but maybe my config-ometer sense is off. 😆

vsoch · 2022-10-07T19:54:23Z

okay will have a PR update for you this evening!

cpeel · 2022-10-07T22:48:54Z

Is JSON preferred to YAML? I think (for configs like this) my preference would be YAML, but maybe my config-ometer sense is off.

YAML is probably better (says my professional half -- my personal half thinks YAML is rubbish 😁 ).

Signed-off-by: vsoch <[email protected]>

vsoch · 2022-10-07T23:14:32Z

YAML is probably better (says my professional half -- my personal half thinks YAML is rubbish grin ).

Haha, I totally feel that!

So I just pushed a refactor that has a single settings.py (to give more confidence we find everything in one place) and it should work to define variables from:

defaults in that file (e.g., a local or custom build)
a settings.yaml in the root OR a custom path (e.g., still local or cloud / Kubernetes)
from the environment (could also work in either case!)
still a secrets.py alongside the settings.py (I realize I enabled SECRET_KEY and we might need to move this import higher up to ensure it doesn't get overridden!)

9b25c8c

I decided to still have tight control over variables and placed them into groups based on type, as oppose to a strategy that looks for any SREGISTRY_ prefix in the environment and then tries to guess. It's an improvement on my first shot because we remove the redundancy. If we want to expose more customization (e.g., in the various dicts in settings.py) that should be fairly easy to do.

The one change I'm still thinking about doing is not having any "special prefix" for secrets as I do now (e.g., SREGISTRY_SECRET_. I was thinking that might be useful for the developer - so they can see based on the name that something should be treated more carefully. I'm thinking maybe it makes sense to try out this branch for your use case, and we can tweak / change as you see issues arise?

cpeel

The one change I'm still thinking about doing is not having any "special prefix" for secrets as I do now (e.g., SREGISTRY_SECRET_).

Yes, I was thinking that too after reviewing the code. I don't think prefixing SECRET makes the developer any more (or less) aware of the importance of that and think it would be simpler overall if we didn't do the special prefixing. And as a counter-example we're not prefixing that to the postgres DB password but that's arguably also important.

While I haven't yet tested this, the concept is solid and I think this will work for what we need to do.

One thing I am wary of is that this will break backwards compatibility for users who upgrade and in the past may have been replacing CONFIG.py with their settings. This also renames some configuration variables. I would classify this is an "API-breaking change" and would rev this as a major point release, rather than a fix update, per semver

shub/settings.py

cpeel · 2022-10-09T19:26:32Z

shub/settings.py

+
+# STORAGE
+
+MINIO_ROOT_USER = os.environ.get("MINIO_ROOT_USER")


While we're here, why not allow this to be set via the config as well? To keep backwards compatibility we could do:

MINIO_ROOT_USER = os.environ.get("MINIO_ROOT_USER", cfg.MINIO_ROOT_USER)

and make the default for MINIO_ROOT_USER in the STRING_DEFAULTS to be None.

We could definitely do that, although for the actual Minio server it is expecting MINIO_ROOT_USER explicitly. I think the use case that you have in mind is having deployed your Minio separately (and so you could use SREGISTRY_MINIO_ROOT_USER without an issue? I'm definitely happy to add that!

I also realize that I need to do a check if a value is set before setting to locals, e.g., for the case that the cfg MINIO_ROOT_USER (from SREGISTRY_MINIO_ROOT_USER) is None, we don't want it to override MINIO_ROOT_USER

Correct - I need to decouple the two. Eventually I need this to work against a genuine S3 bucket rather than a minio server, but I haven't yet gotten to testing that / making changes to support it.

Oh nice! That would be amazing to have (and based on the shared protocol hopefully not too hard to support). I can definitely help when the time comes.

shub/settings.py

requirements.txt

shub/settings.py

vsoch · 2022-10-09T22:52:25Z

okay (barring no issues with building) another round of changes! 854b7ab

The only detail we have to be careful about now is that values that are expected to be in settings (e.g., GOOGLE_ANALYTICS that can still be set to undefined are provided as empty string "" and not None - Nones are assumed to be unset and won't be added to the settings locals, whereas an empty string will.

cpeel

This is looking 💯 . Some small PR feedback. Still need to do some testing on it.

docs/_docs/install/settings.md

shub/settings.py

cpeel · 2022-10-09T23:41:17Z

docs/_docs/install/settings.md

+
+Order of preference or variables honored is:
+
+ 1. secrets.py


The way it is coded now, the order is:

secrets.py

settings.yaml

the environment

defaults directly in settings.py

Which I think is the right order, because we:

set the defaults

override any values from the env

override any values from settings.yaml

only override values already set in the file from secrets.yaml.

Environment (starting on line 226) actually comes before settings.yaml (275) , however we could switch this around so the settings.yaml is first. I was thinking that the secrets.py should take first preference for backwards compatibility, and then environment should always override everything else (e.g., a config file might already exist but the user can easily set the envar to override it) and then the defaults in settings.py are last. What do you think?

Yes, sorry, I read the code wrong. What you have here in settings.md matches the behavior in settings.py.

It might be better if the env overrides the settings.yaml file, but I'm not going to lose any sleep over what you have here.

I do agree environment should not be over-ridden - the challenge here is that we don't have a good way to determine if a variable was set in the environment (or is still a default). We can't just check if a DEFAULT[key] is not None, because this won't work for booleans, or generally any variable with a default that isn't None. It's a bit of a catch 22 because in order to set anything from settings.yaml, we need to have all the variables in one lookup, DEFAULTS, but we can't set that one variable until we've parsed the different types from the environment! So I think (for now at least) this approach is a reasonable start. If you have a good idea for an implementation though I'd definitely be down to try it!

cpeel · 2022-10-09T23:44:15Z

shub/settings.py

+    return envar_list
+
+
+# Try reading in from secrets first (no issue if not found)


I would move this to the end of the file so that the load order maps to the conceptual "define and then override" that we're doing:

defaults

environment

settings.yaml

secrets.py

The secrets currently take precedence because of this code on line 664:

# Don't set if the value is empty, or it's been set previously if value is None or key in locals() and locals()[key] is not None:

I think the order you have is correct and moving the secrets imports to line 648 makes it very clear that the file trumps everything else.

Ah ok, so you agree about environment being honored first? I think my point of confusion is that I'm loading in this order:

secrets.py

environment

settings.yaml

defaults

and the implementation does that by using that order, but never setting something that is already defined.

Agree - we can put secrets at the bottom, although I'll want to double check / think about if any of the previous settings (e.g., putting a value into a data structure) would prevent that.

okay I think this will work - with the assumption that secrets.py will work only for values you could previously put there (e.g., the new values for the various API settings that I'm adding newly will not work) and to over-ride entire datastructures (e.g., the database) you need to define the entire thing (and it's not exposed just via attributes of that).

the user can now define variables directly in settings.py, via an external secrets.py or settings.yaml, or in the environment with SREGISTRY_ as a prefix. Signed-off-by: vsoch <[email protected]>

Signed-off-by: vsoch <[email protected]>

vsoch · 2022-10-10T01:26:36Z

Note to self: don't look back at old code, because it undoubtedly will look terrible and you'll want to change everything to make it look nicer! 😭 (true story, just now...) 😆

Thanks for the back and forth today @cpeel it was fun!

cpeel · 2022-10-10T01:51:45Z

Note to self: don't look back at old code, because it undoubtedly will look terrible and you'll want to change everything to make it look nicer! 😭 (true story, just now...) 😆

lol - I've been doing software development for 22 years and this only gets worse! On the plus side this means that we're becoming better software developers!

Thanks for the back and forth today @cpeel it was fun!

Same! Appreciate all of your work in this PR! I will give this a test run ASAP. Tomorrow is busy for me so might be Tuesday.

cpeel

This is working great!

Found two small minor comments in my testing but this looks good to go on my end.

cpeel · 2022-10-11T21:16:59Z

shub/settings.py

+
+# If we don't have a secret key, no go
+if "SECRET_KEY" not in locals():
+    sys.exit("SECRET_KEY is required but not set. Set SREGISTRY_SECRET_KEY.")


Setting SECRET_KEY in the YAML file works fantastically, but setting SREGISTRY_SECRET_KEY env var doesn't work because we don't define it in the STRING_DEFAULTS. It's probably easiest to keep the functionality the way it is now and just update the message to Set SECRET_KEY in secrets.py or a YAML config file.

Any reason to not allow this in the environment? I am thinking we can just add to the STRING_DEFAULTS and be more consistent?

No reason I can think of. Adding it to STRING_DEFAULTS and updating the check for it to be not None makes sense.

cpeel · 2022-10-11T21:22:40Z

shub/settings.py

+    "AUTH_LDAP_STAFF_GROUP_FLAGS": None,  # "cn=staff,ou=django,ou=groups,dc=example,dc=com",
+    # Anyone in this group is a superuser for the app
+    "AUTH_LDAP_SUPERUSER_GROUP_FLAGS": None,  # "cn=superuser,ou=django,ou=groups,dc=example,dc=com"
+    # "cn=sregistry_admin,ou=groups,dc=example,dc=com"


Not sure what this LDAP string is for -- is it a second example for AUTH_LDAP_SUPERUSER_GROUP_FLAGS above?

yes, just a second example! I had two previously and wanted to keep both of them for the developer user. I'm not great with LDAP so I couldn't tell you the difference, I'd have to ping the plugin contributor to get details.

I would remove this line then as I don't think it provides any additional information for someone who is integrating with LDAP -- the other one is sufficient. (Says someone who worked on LDAP code for 10 years.)

vsoch · 2022-10-12T18:58:51Z

@cpeel I'll get these final tweaks in tonight - I don't typically commit to personal projects during the work day!

Signed-off-by: vsoch <[email protected]>

vsoch · 2022-10-12T23:28:51Z

okay final two changes in! b9335b6

cpeel

Ship it!

Signed-off-by: vsoch <[email protected]>

vsoch added the hacktoberfest-accepted label Oct 7, 2022

vsoch mentioned this pull request Oct 7, 2022

Enable streamlined upgrades with local.py #408

Closed

allowing setting config vars from environment

404f138

Signed-off-by: vsoch <[email protected]>

vsoch force-pushed the consolidate-config branch from bdc7994 to 404f138 Compare October 7, 2022 13:45

cpeel reviewed Oct 7, 2022

View reviewed changes

refactor to use a single settings.py, allow a settings.yaml

78e0452

Signed-off-by: vsoch <[email protected]>

vsoch force-pushed the consolidate-config branch from 9b25c8c to 78e0452 Compare October 7, 2022 23:13

cpeel reviewed Oct 9, 2022

View reviewed changes

vsoch force-pushed the consolidate-config branch 2 times, most recently from 38d1c21 to 854b7ab Compare October 9, 2022 22:46

cpeel reviewed Oct 9, 2022

View reviewed changes

consolidate secrets into same settings

4a51afb

the user can now define variables directly in settings.py, via an external secrets.py or settings.yaml, or in the environment with SREGISTRY_ as a prefix. Signed-off-by: vsoch <[email protected]>

vsoch force-pushed the consolidate-config branch from 854b7ab to 4a51afb Compare October 10, 2022 00:49

ensure linting is done for new release

ea1a148

Signed-off-by: vsoch <[email protected]>

vsoch force-pushed the consolidate-config branch from 061a6d2 to ea1a148 Compare October 10, 2022 01:25

cpeel approved these changes Oct 11, 2022

View reviewed changes

cpeel mentioned this pull request Oct 11, 2022

Consolidate container env settings #410

Closed

ensure SECRET_KEY also honored in environment

b9335b6

Signed-off-by: vsoch <[email protected]>

cpeel approved these changes Oct 12, 2022

View reviewed changes

formatting

2078b63

Signed-off-by: vsoch <[email protected]>

vsoch merged commit dfff8fd into master Oct 13, 2022

This was referenced Jun 1, 2023

config.py is no longer there. #432

Open

adding back the ability to pass passwords to the database driver #433

Open

Update scripts and docs to reflect changes in settings file. #436

Open


		# STORAGE

		MINIO_ROOT_USER = os.environ.get("MINIO_ROOT_USER")

		return envar_list


		# Try reading in from secrets first (no issue if not found)

Uh oh!

Conversation

vsoch commented Oct 7, 2022

Uh oh!

cpeel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vsoch commented Oct 7, 2022

Uh oh!

vsoch commented Oct 7, 2022

Uh oh!

cpeel commented Oct 7, 2022

Uh oh!

vsoch commented Oct 7, 2022

Uh oh!

vsoch commented Oct 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpeel commented Oct 7, 2022

Uh oh!

vsoch commented Oct 7, 2022

Uh oh!

cpeel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsoch Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vsoch commented Oct 9, 2022

Uh oh!

cpeel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsoch commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpeel commented Oct 10, 2022

vsoch commented Oct 7, 2022 •

edited

Loading

vsoch Oct 9, 2022 •

edited

Loading

vsoch commented Oct 10, 2022 •

edited

Loading