-
Notifications
You must be signed in to change notification settings - Fork 72
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
Adding implementation for separate ui database user #225
Conversation
Thanks for this PR @brandtkeller, it seems super useful! I need to do some manual testing on this change before merging it, but probably won't be able to get to that testing until sometime next week. |
@Btodhunter Is there any support that can be given to help get this over the hump? We're currently manually managing the secret to get this same result. Having a UI specific db user is working fine, but we haven't been able to trim down the level of permissions that may be required for that specific user yet. |
@@ -15,9 +15,9 @@ metadata: | |||
type: Opaque | |||
stringData: | |||
{{- if .Values.anchoreGlobal.dbConfig.ssl }} | |||
ANCHORE_APPDB_URI: 'postgresql://{{ index .Values "postgresql" "postgresUser" }}:{{ index .Values "postgresql" "postgresPassword" }}@{{ template "db-hostname" . }}/{{ index .Values "postgresql" "postgresDatabase" }}?ssl=verify-full' | |||
ANCHORE_APPDB_URI: 'postgresql://{{ ternary (index .Values "anchoreEnterpriseUi" "dbUser") (index .Values "postgresql" "postgresUser") (hasKey .Values.anchoreEnterpriseUi "dbUser" ) }}:{{ index .Values "postgresql" "postgresPassword" }}@{{ template "db-hostname" . }}/{{ index .Values "postgresql" "postgresDatabase" }}?ssl=verify-full' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brandtkeller if .Values.anchoreEnterpriseUi.dbUser
is set, should we be handling the password separately as well? Using .Values.postgresql.postgresPassword
, which is shared with the engine db user, doesn't seem ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that is a fair assessment. We're trying to establish some ability to separate the users and their credentials - it would make sense to allow a non-shared password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added support for both user/password to be unique. It's a lengthy template with multiple ternaries - so i'm open to input or anyone doing direct modification.
@brandtkeller I'll keep my eye on this and get it through asap. |
@brandtkeller thanks for the update! I'm reviewing the template now. Can you bump the chart version? Then I'll merge this after finishing the review. |
@brandtkeller you may also need to rebase on |
Signed-off-by: Brandt Keller <[email protected]>
Signed-off-by: Brady Todhunter <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Closes #224
Relates to #195 and #196
Directly implements a configuration to allow overriding the enterpriseUI database user and providing separation for access to the database by Engine/UI to aid in traceability/logging/auditing.
Indirectly addresses some standardization of DB host across resource files.
Willing to consider another templating mechanism outside of the ternary - it's self-documenting but not syntactically pretty.