Skip to content
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

Reorg config files #685

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Reorg config files #685

merged 1 commit into from
Nov 21, 2024

Conversation

loneil
Copy link
Contributor

@loneil loneil commented Nov 20, 2024

Fix (hopefully) issue with VCAuthN deployment from missing config map. Reorg things so we're just using a single config map instead of intending on one per configuration file. Renamed it "controller-config" instead of specific to "session timeout". Add the "blank" (comments, see below) placeholder user variable sub file to the config map.

Default population for this user variable py file comes from values. So keep the pattern of being able to override it with a values yaml, or operator can have whatever means up updating configmap, or the volume itself, as works for their CICD.
App continues to use environment variable to know which path to look for these files, so can override that as well.

Discussed moving location of files to /app but thinking about that didn't want to volume mount into the directory the code goes into, as that could cause confusion (and if you can alter /app it's like changing the source code anyway instead of "external" configuration), and to keep /app as deployed, built source alone.

Used /etc/controller-config instead of /home/aries
Don't really care about /etc vs /home but googled practices for config file stuff

Best Practices:
System-Wide Configuration:

Use the /etc directory for configurations that need to be available to all users or affect system-wide services.
Keep it organized and maintain proper permissions to ensure system security.

User-Specific Configuration:

Use the /home directory for configurations that are specific to individual users.
Store user preferences and application settings in the user’s home directory to avoid conflicts and maintain privacy.

Since this is a system service thing and not a user preference...🤷 again not really important. Important part is to not use "aries" naming any more if we can avoid it. If there's any weird permission thing, or if people think /home is a more obvious place to look can change it.

Tested helm install locally on minikube and things seem to be where they're expected (below). But will need to make sure OCP deployment is all good and the Python app doesn't have issues loading.
If we merge this I'll follow through on Dev env and make sure things are deploying ok and the app is picking up the values as expected.

New config map, contains both default file values:
image

Checking on controller pod:
image

Comment on lines 53 to 55
userVariableSubsitution: |-
# This is a default placeholder Python file
# Add your default content here
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default "blank" file here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Do we want to have a commented-out example of a substitution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, there's the example file in the source here
https://github.com/bcgov/vc-authn-oidc/blob/main/docker/oidc-controller/config/user_variable_substitution_example.py
so could add a function from that commented out, or just a comment saying they could look at that file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vote goes to adding a function/example for that file commented-out to make it foolproof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am tempted to say the name but also it makes it far harder to keep the examples all up to date if we chose to update the API. Just a thought to consider but chances are the API won't change any time soon

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commented out file

i5okie
i5okie previously approved these changes Nov 21, 2024
Copy link
Contributor

@i5okie i5okie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@esune esune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I think if we add the default example code to the values file for the variable substitution we can merge and test in OCP

Gavinok
Gavinok previously approved these changes Nov 21, 2024
Signed-off-by: Lucas ONeil <[email protected]>
@loneil loneil merged commit 0135bbf into main Nov 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants