taskotron / resultsdb_frontend

Clone
Source Code
GIT

#17 Container support: Allow to inject SECRET_KEY and RDB_URL through env
Merged 4 years ago by frantisekz. Opened 4 years ago by frantisekz.
env_settings  into  develop

file modified
+29 -4 
@@ -22,6 +22,7 @@ 
 

  

  from flask import Flask, render_template
 

  from resultsdb_frontend import proxy
 

+ from resultsdb_frontend.exceptions import ContainerConfigIncompleteError
 

  

  import logging
 

  import logging.handlers
 
@@ -57,10 +58,6 @@ 
 

  if os.path.exists(config_file):
 

      app.config.from_pyfile(config_file)
 

  

- if app.config['PRODUCTION']:
 

-     if app.secret_key == 'replace-me-with-something-random':
 

-         raise Warning("You need to change the app.secret_key value for production")
 

- 

  # setup logging
 

  fmt = '[%(filename)s:%(lineno)d] ' if app.debug else '%(module)-12s '
 

  fmt += '%(asctime)s %(levelname)-7s %(message)s'
 
@@ -99,6 +96,34 @@ 
 

  

  setup_logging()
 

  

+ # Is this a Container deployment?
 

+ container_env = os.getenv('RDB_FE_ENVIRONMENT')
 

+ if container_env:
 

+     if container_env not in ("stg","prod"):
 

+         app.logger.error("RDB_FE_ENVIRONMENT env variable set to a wrong value. Allowed values are: stg/prod")
 

+         raise ContainerConfigIncompleteError("RDB_FE_ENVIRONMENT invalid")
 

+ 

+     if container_env == "stg":
 

+         app.config["DEBUG"] = True
 

+         app.config["PRODUCTION"] = False
 

+ 

+     elif not os.getenv('RDB_FE_SECRET_KEY'):
 

+         app.logger.error("Container mode enabled but required env variable RDB_FE_SECRET_KEY is not set.")
 

+         raise ContainerConfigIncompleteError("RDB_FE_SECRET_KEY missing")
 

+ 

+     if not os.getenv("RDB_FE_RDB_URL"):
 

+         app.logger.warning("Using default RDB URL %s as RDB_FE_RDB_URL env variable is not set." % app.config["RDB_URL"])
 

+ 

+ # Allow to load some config options from env vars
 

+ if os.getenv('RDB_FE_SECRET_KEY'):
 

+     app.config["SECRET_KEY"] = os.getenv('RDB_FE_SECRET_KEY')
 

+ if os.getenv("RDB_FE_RDB_URL"):
 

+     app.config["RDB_URL"] = os.getenv("RDB_FE_RDB_URL")
 

+ 

+ if app.config['PRODUCTION']:
 

+     if app.secret_key == 'replace-me-with-something-random':
 

+         raise Warning("You need to change the app.secret_key value for production")
 

+ 

  # register blueprints
 

  from resultsdb_frontend.controllers.main import main
 

  app.register_blueprint(main)

file added
+24 
@@ -0,0 +1,24 @@ 
 

+ #
 

+ # exceptions.py - Exceptions for resultsdb_frontend
 

+ #
 

+ # Copyright 2021, Red Hat, Inc
 

+ #
 

+ # This program is free software; you can redistribute it and/or modify
 

+ # it under the terms of the GNU General Public License as published by
 

+ # the Free Software Foundation; either version 2 of the License, or
 

+ # (at your option) any later version.
 

+ #
 

+ # This program is distributed in the hope that it will be useful,
 

+ # but WITHOUT ANY WARRANTY; without even the implied warranty of
 

+ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 

+ # GNU General Public License for more details.
 

+ #
 

+ # You should have received a copy of the GNU General Public License along
 

+ # with this program; if not, write to the Free Software Foundation, Inc.,
 

+ # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
 

+ #
 

+ # Authors:
 

+ #   Frantisek Zatloukal <fzatlouk@redhat.com>
 

+ 

+ class ContainerConfigIncompleteError(BaseException):
 

+     pass 

\ No newline at end of file

resultsdb_frontend now support loading of following env variables:

  • RDB_FE_ENVIRONMENT: either stg (staging) or prod (production)

  • RDB_FE_SECRET_KEY: string to be used as a SECRET_KEY, eg. output from python -c 'import secrets; print(secrets.token_hex())'

  • RDB_FE_RDB_URL: full url to resultsdb, eg. http://localhost:5001/api/v2.0

example command to run the image:

podman run --name rdb_front -e RDB_FE_ENVIRONMENT=prod -e RDB_FE_SECRET_KEY=so-secret -e RDB_FE_RDB_URL=http://localhost:5001/api/v2.0 -p 5002:5002 <image_hash>

I would suggest to make the OPENSHIFT_PROD environment variable more generic since it could be deployed as a stand-alone container, using Kubernetes, or etc.

One other suggestion is to namespace the environment variables (e.g. RDB_FE_SECRET_KEY).

Lastly, I think it might be worth considering moving this logic to conf/resultsdb_frontend.wsgi so that environment variables don't affect the configuration of ResultsDB Frontend when simply importing the resultsdb_frontend python package.

1 new commit added

  • Rework according to feedback
4 years ago

@mprahl thanks for the review, I've changed the code a bit, part of logic is still in init.py but the container path should now be unreachable if you import the module unless you really want to use this path.

2 new commits added

  • Rework according to feedback
  • OpenShift: Allow to inject SECRET_KEY and OPENSHIFT_PROD through env
4 years ago

@frantisekz this new approach seems like it's hacky since it's abusing the usage of sys.argv. I think relying on only environment variables was a cleaner solution.

Also, rather than sys.exit(1), I think the more Pythonic approach in this case would be to raise an exception.

1 new commit added

  • Let's not hack around argv
4 years ago

@frantisekz this new approach seems like it's hacky since it's abusing the usage of sys.argv. I think relying on only environment variables was a cleaner solution.

Okay, yeah, you're right, I've switched back to using env vars. However, it'd need a bit of thinking and changing to move the entire logic into the wsgi file, so I'd leave it as it is right now in init and revisit it in the future, if that's not a big issue for you?

Also, rather than sys.exit(1), I think the more Pythonic approach in this case would be to raise an exception.

Hmm, I've always preferred the non-zero rc exit with some message right before it. Would leaving it with this approach be an issue for your deployment?

@frantisekz this new approach seems like it's hacky since it's abusing the usage of sys.argv. I think relying on only environment variables was a cleaner solution.

Okay, yeah, you're right, I've switched back to using env vars. However, it'd need a bit of thinking and changing to move the entire logic into the wsgi file, so I'd leave it as it is right now in init and revisit it in the future, if that's not a big issue for you?

Also, rather than sys.exit(1), I think the more Pythonic approach in this case would be to raise an exception.

Hmm, I've always preferred the non-zero rc exit with some message right before it. Would leaving it with this approach be an issue for your deployment?

It is not an issue for us. I was just mentioning that it is unusual for anything other than a script to use sys.exit since it doesn't allow the caller to choose to catch the exception and handle it.

I think logging rather than using print would be better for this. It's unusual to use print outside of a script since it limits the control the caller has of the output to stderr. It also then won't include the desired log format of the caller such as determining the line number and the timestamp.

Same comment for the other print statement below.

1 new commit added

  • Raise exceptions on bad config
4 years ago

1 new commit added

  • Missed exceptions file
4 years ago

Okay, logging facility setup moved to happen before taking containerized path, so I can use it and did just that.

I've also switched the sys.exit to the custom exception. The log in such case isn't that unreadable.

This import can be removed now.

Just a suggestion... RDB_FE_ENVIRONMENT would be a better name in my opinion.

Just a suggestion... you could continue using the if os.getenv pattern here so that you also catch if RDB_FE_SECRET_KEY is an empty value.

Thanks for this PR. This should work for our use-case.

1 new commit added

  • Another round for better code
4 years ago

All should be addressed. Thanks a lot for the feedback :) !

If you can give it one final ack, I'll squash and merge it afterwards.

Optionally, you could stop calling this out as explicitly a container configuration (including the error messages and logs) since it's written in a generic way. That being said, I wouldn't think it'd be the best fit outside of a container though.

My preference would be to not make this dependent on RDB_FE_ENVIRONMENT being set, but it's okay if you want to keep it as is.

My preference would be to not make this dependent on RDB_FE_ENVIRONMENT being set, but it's okay if you want to keep it as is.

@frantisekz I left some optional comments. If you like them, please address them, otherwise, this looks good to me! :smile:

Thanks again!

rebased onto 2d964aa
4 years ago

I've addressed everything but the first optional comment. I agree that it's generic way to set some things up, I expect (famous last words following 😄 ) it'd be used only for containers though . The whole configuration section can be somehow re-structured in the future, but it's nothing too pressing.

Loading RDB_URL and SECRET_KEY from RDB_FE_RDB_URL and RDB_FE_SECRET_KEY can't hurt anything, thanks!

Pull-Request has been merged by frantisekz
4 years ago
Metadata
None
No Tags
Changes Summary 2
+29 -4
file changed
resultsdb_frontend/__init__.py
+24
file added
resultsdb_frontend/exceptions.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.