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

MPCONFIG Secret Dir: support secrets in subdirectories / FISH-788 #5006

Closed
poikilotherm opened this issue Nov 24, 2020 · 7 comments · Fixed by #5007
Closed

MPCONFIG Secret Dir: support secrets in subdirectories / FISH-788 #5006

poikilotherm opened this issue Nov 24, 2020 · 7 comments · Fixed by #5007
Assignees
Labels
Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev Type: Enhancement Label issue as an enhancement request

Comments

@poikilotherm
Copy link
Contributor

Description

Let's provide support for K8s secret volume mounts in subdirectories of the MPCONFIG secret dir.

Expected Outcome

(See context first)

You should be able to mount the secrets to a folder named /secrets/dataverse.db, creating the secret files inside of it.
Then you should be able to retrieve the secret via @ConfigProperty("dataverse.db.username") etc.

Current Outcome

It will be very hard to reference these and avoid name clashing with MPCONFIG with the current implementation in https://github.com/payara/Payara/blob/master/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/source/SecretsDirConfigSource.java

You have to maintain a flat file / secret structure, which is not fostering reusing Secrets and writing them in a human-friendly way. (To stay with the example above, you could create a secret with the key "dataverse.db.username", but this wouldn't be easily reusable in other places).

Context (Optional)

When using with Kubernetes, you might create secrets for using with your application. Let's assume a Secret for Postgres:

apiVersion: v1
kind: Secret
metadata:
  name: dataverse-postgresql
type: Opaque
stringData:
  username: dataverse
  password: changeme

If you want to use this secret with Payara, you should be mounting it to a directory (env vars are insecure), let's say /secrets:

volumeMounts:
  - name: db-secret
    mountPath: /secrets
volumes:
  - name: db-secret
    secret:
      secretName: dataverse-postgresql

You will find two files in the folder of your container:

$ ls -1 /secrets
username
password

Environment

  • Payara Version: 5.2020.6
  • Edition: Full
  • JDK Version: 8
  • Operating System: Linux
  • Database: PostgreSQL
  • Application: Dataverse
@poikilotherm
Copy link
Contributor Author

poikilotherm commented Nov 24, 2020

I would be happy to provide a code contribution for this, if you agree.

It should be fairly easy to implement, depending on how much effort we agree while searching the secret file based on the property name.

@AlanRoth AlanRoth added Status: Open Issue has been triaged by the front-line engineers and is being worked on verification Type: Enhancement Label issue as an enhancement request labels Nov 24, 2020
@AlanRoth
Copy link

Hi @poikilotherm,

If you wish to contribute then we are more than happy to accept. We encourage the community to help make Payara better whenever we can! Once you submit a PR linked to this issue I will create an internal ticket and set the status of this issue as accepted.

Thank you,
Alan

@AlanRoth AlanRoth added Status: Pending Waiting on the issue requester to give more details or share a reproducer and removed Status: Open Issue has been triaged by the front-line engineers and is being worked on verification labels Nov 24, 2020
@AlanRoth AlanRoth self-assigned this Nov 24, 2020
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Nov 25, 2020
This adds support for mounting your secrets in subdirectories to create scopes.
The relative path is used as the property name. A secret file "SECRET_DIR/foo/bar/secret"
will be available as MPCONFIG value "foo.bar.secret".

You can still put a file "foo.bar.secret" in "SECRET_DIR" and have the same value available.
If both are present, the topmost file will be picked.

Mix'n'match isn't supported. Retrieving "foo.bar.secret" from "SECRET_DIR/foo/bar.secret"
will not work reliably when added at runtime.
@poikilotherm
Copy link
Contributor Author

Hi @AlanRoth I just created a PR for this.
Please let me know if anything is missing, what you folks think and if the size of the PR is fine as is. Happy to reshape if necessary.

@AlanRoth
Copy link

Hi @poikilotherm,

Thank you for your submission. The PR looks good, I have created an internal issue FISH-788, we will need you to sign a CLA which I'll send details on how to do that on the actual PR - it isn't too much bother.

Thanks again,
Alan

@AlanRoth AlanRoth added Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev and removed Status: Pending Waiting on the issue requester to give more details or share a reproducer labels Nov 25, 2020
@AlanRoth AlanRoth changed the title MPCONFIG Secret Dir: support secrets in subdirectories MPCONFIG Secret Dir: support secrets in subdirectories / FISH-788 Nov 25, 2020
@poikilotherm
Copy link
Contributor Author

@AlanRoth would you like me to add a few words to https://github.com/payara/Payara-Community-Documentation/tree/master/docs/modules/ROOT/pages/documentation/microprofile/config#config-secrets-directory-configuration about usage with sub directories? The current doc isn't very user friendly - took me a while and reading through source code to understand how it works.

@AlanRoth
Copy link

@poikilotherm, If you're happy to contribute you are more than welcome! Thank you.

@poikilotherm
Copy link
Contributor Author

poikilotherm added a commit to poikilotherm/Payara that referenced this issue Nov 30, 2020
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Nov 30, 2020
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Dec 7, 2020
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Dec 7, 2020
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 25, 2021
…ayara#5006

This is kinda major refactoring, including code tested in sample project to ensure this runs on K8s.

It's still lacking tests! This is a commit to save the status.
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 25, 2021
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 25, 2021
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 27, 2021
This avoids pathes on Windows being invalid, because concatenating
two absolute pathes is not possible on Windows.

Part of a solution for payara#5006
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 27, 2021
Introducing some mocks to enable unit testing of findDir().
Needed to extend the base class constructors to be able to inject the mock.

Relates to payara#5006

P.S. Crossing fingers this runs on Windoze.
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 27, 2021
…le and use less memory

This commits make the logic part of isLongestMatch() a static function,
much easier to unit test. The lookup part checking if the path is already
present somewhere is handled in a non-static function, so staying compatible
with using this config source for multiple locations.

It also refactored the inner DirProperty class to avoid saving the path depth
and compute it instead, reducing the waste of resources.

Relates to payara#5006
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 27, 2021
…sions

Ignore *.properties, *.yaml, *.yml, *.xml, *.json files, as those are likely
to contain more complex structures used for other config sources.

Remember: the file name compiles to the property name (up to three letter
endings silently cut off) and might contain whatever. If you put a
file with any of these extensions in the directory on purpose, it should not
be used with this config source.

- Relates to payara#5006
- Based on request from @pdudits in payara#5007
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 27, 2021
…rConfigSource

As requested by @pdudits, do not cut off file extensions silently.
People on Windows can deal with text files without a file extension,
better stay consistent.

Relates to payara#5006
AlanRoth pushed a commit that referenced this issue Jan 28, 2021
FISH-788 Support sub-directories for MPCONFIG SecretDirConfigSource. #5006
poikilotherm added a commit to poikilotherm/Payara that referenced this issue Jan 29, 2021
During QA after merging payara#5007, @lprimak stumbled over a SEVERE
log message during deploys, triggered by the non-existing directory
of DirConfigSource. The directory config value has had a default
before, also the docs say otherwise.

The level issue has been fixed by checking only default config value
being used, resulting in an INFO level message. The logic has been
refactored not to use an exception, but Optional API.

- Docs will be fixed to include the formerly hidden default value
- Part of payara#5006
lprimak pushed a commit that referenced this issue Jan 29, 2021
…5104)

* fix(mpconfig): make DirConfigSource less disrupting if not configured

During QA after merging #5007, @lprimak stumbled over a SEVERE
log message during deploys, triggered by the non-existing directory
of DirConfigSource. The directory config value has had a default
before, also the docs say otherwise.

The level issue has been fixed by checking only default config value
being used, resulting in an INFO level message. The logic has been
refactored not to use an exception, but Optional API.

- Docs will be fixed to include the formerly hidden default value
- Part of #5006

* style(mpconfig): reducing log level in DirConfigSource.findDir()

As requested by @pdudits, reducing the log level to FINE
if the directory targeted by default value (secrets) is not present.
lprimak pushed a commit that referenced this issue Feb 5, 2021
FISH-788 Support sub-directories for MPCONFIG SecretDirConfigSource. #5006
lprimak pushed a commit that referenced this issue Feb 5, 2021
…5104)

* fix(mpconfig): make DirConfigSource less disrupting if not configured

During QA after merging #5007, @lprimak stumbled over a SEVERE
log message during deploys, triggered by the non-existing directory
of DirConfigSource. The directory config value has had a default
before, also the docs say otherwise.

The level issue has been fixed by checking only default config value
being used, resulting in an INFO level message. The logic has been
refactored not to use an exception, but Optional API.

- Docs will be fixed to include the formerly hidden default value
- Part of #5006

* style(mpconfig): reducing log level in DirConfigSource.findDir()

As requested by @pdudits, reducing the log level to FINE
if the directory targeted by default value (secrets) is not present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Confirmed defect or accepted improvement to implement, issue has been escalated to Platform Dev Type: Enhancement Label issue as an enhancement request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants