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

WIP: add env var for configuration #27

Closed
wants to merge 1 commit into from

Conversation

akoserwal
Copy link
Contributor

PR Template:

Describe your changes

Add ENV variables in config.yaml

  • ...

Ticket reference (if applicable)

Fixes #

Checklist

  • Are the agreed upon acceptance criteria fulfilled?

  • Was the 4-eye-principle applied? (async PR review, pairing, ensembling)

  • Do your changes have passing automated tests and sufficient observability?

  • Are the work steps you introduced repeatable by others, either through automation or documentation?

    • If automation is possible but not done due to other constraints, a ticket to the tech debt sprint is added
    • An SOP (Standard Operating Procedure) was created
  • The Changes were automatically built, tested, and - if needed, behind a feature flag - deployed to our production environment. (Please check this when the new deployment is done and you could verify it.)

  • Are the agreed upon coding/architectural practices applied?

  • Are security needs fullfilled? (e.g. no internal URL)

  • Is the corresponding Ticket in the right state? (should be on "review" now, put to done when this change made it to production)

  • For changes to the public API / code dependencies: Was the whole team (or a sufficient amount of ppl) able to review?

@akoserwal
Copy link
Contributor Author

@akoserwal akoserwal changed the title add env var for configuration WIP: add env var for configuration Feb 14, 2024
timeout: 1s
data:
spiceDb:
useTLS: false
endpoint: spicedb:50051
token: "${SPICEDB_PRESHARED:foobar}"
endpoint: "${ENDPOINT:spicedb:50051}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this config been tested with make rebac, etc. It looks odd, but if it works it works. :)

@@ -21,6 +21,10 @@ COPY --from=builder /workspace/bin/ciam-rebac /usr/local/bin/
COPY --from=builder /workspace/configs/config.yaml /usr/local/bin/

ENV SPICEDB_PRESHARED $SPICEDB_PRESHARED
ENV SPICEDB_ENDPOINT $SPICEDB_ENDPOINT
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth having a discussion about the use of env vars here? We were pretty committed to using file mounts before (no env vars in previous Dockerfile: https://github.com/RedHatInsights/authz/blob/main/Dockerfile).

I seem to remember people mentioning security concerns around the lines of it being easier to expose an app's environment that the contents of a file.

@shenqidebaozi
Copy link

@akoserwal @merlante

I'm glad to see that RedHat is using Kratos. What are the reasons for using Kratos? We would like to know why users choose to use Kratos to help improve the project continuously.

Can you also add a use case, go-kratos/kratos#969

If you have any questions, suggestions or opinions while using it, please feel free to give me feedback.

@akoserwal
Copy link
Contributor Author

@akoserwal @merlante

I'm glad to see that RedHat is using Kratos. What are the reasons for using Kratos? We would like to know why users choose to use Kratos to help improve the project continuously.

Can you also add a use case, go-kratos/kratos#969

If you have any questions, suggestions or opinions while using it, please feel free to give me feedback.

Hi @shenqidebaozi thank you for reaching out.

We are evaluating Kratos for a POC project. Kratos enabled us to develop a proto-to-open API spec workflow and build APIs quickly.

Suggestions: The discord go-kratos forum isn't active much. Is there any other forum where I can engage for doubts/discussions?

As we go along with the development, I'll definitely identify areas to improve and where we see an opportunity to contribute.

@shenqidebaozi
Copy link

@akoserwal Due to certain network restrictions, Discord cannot be directly accessed within China. At present, our main focus is on maintaining the community on WeChat. But you can also add my Discord(baozi6562) for communication, and I am also considering how to improve the issue you encountered when using config env.

Since the values of environment variables are all strings, it is necessary to convert them to the expected target type when using them. I am considering how to implement this feature. Do you have any ideas?

@akoserwal
Copy link
Contributor Author

Since the values of environment variables are all strings, it is necessary to convert them to the expected target type when using them. I am considering how to implement this feature.

@akoserwal Due to certain network restrictions, Discord cannot be directly accessed within China. At present, our main focus is on maintaining the community on WeChat. But you can also add my Discord(baozi6562) for communication, and I am also considering how to improve the issue you encountered when using config env.

Since the values of environment variables are all strings, it is necessary to convert them to the expected target type when using them. I am considering how to implement this feature. Do you have any ideas?

One way would be to define the type: int along the fields in config.yaml and, post-reading the environment variable, do the conversion like this:

envVar := os.Getenv("ENV_VAR_INT")
var1, err := strconv.Atoi(envVar)

Or allow users to read all environment variables as strings, and then they can do the conversion.

@akoserwal akoserwal closed this Mar 6, 2024
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.

None yet

3 participants