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

Fixes the secrets parameter, adds the secret_env parameter #319

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

andryyy
Copy link
Contributor

@andryyy andryyy commented Aug 31, 2023

  1. Minor fix

Minor typo fix out of context: "example :" to "example:"

  1. Secrets

The previous implementation of the secrets parameter allowed a string or Secret object to be defined.
The parameter passed to the API resulted in {"ID":str}.

While testing the secrets API I found that having only a single secret defined worked fine while more than one secret resulted in an exception:

500 Server Error: Internal Server Error (more than one result secret with prefix : secret is ambiguous)

The parameter ID was never used and a prefix of "" (empty) assumed.
Having a single entry in the Secrets Manager will always result in a single result, no matter the filter.

After losing myself in the API documentation, the correct data format I found is the following:

{
    "source": "my_secret", # name or id
    "target": "/my_secret", # optional
    "uid": 1000, # optional
    "gid": 1000, # optional
    "mode": 0o400, # optional
}

No changes for existing setups are necessary. Data (str, object, dict) is still expected within a list.

  1. Secrets as environment variables

I added the secret_env parameter to set a secret's content as environment variable:

{
  "var_name": "secret_name",
}

Inline documentation was updated. :-)

@rhatdan
Copy link
Member

rhatdan commented Aug 31, 2023

LGTM
Thanks @andryyy

@rhatdan
Copy link
Member

rhatdan commented Aug 31, 2023

@umohnani8 @jwhonce @baude PTAL

@umohnani8
Copy link
Member

Changes LGTM, but tests are unhappy

@jwhonce
Copy link
Member

jwhonce commented Aug 31, 2023

@andryyy It would appear you need #320 to merge and then rebase this PR. Your tests should be happy then.

@andryyy
Copy link
Contributor Author

andryyy commented Aug 31, 2023

Okay 👍

Thank you for looking into it.

@rhatdan
Copy link
Member

rhatdan commented Sep 2, 2023

#320 is merged

- Minor typo fix out of context: "example :" to "example:"
- Secrets were not mounted correctly
- Add parameter secret_env to mount secrets as environment variable

Signed-off-by: André Peters <[email protected]>
Signed-off-by: André Peters <[email protected]>
@andryyy
Copy link
Contributor Author

andryyy commented Sep 2, 2023

Done @rhatdan

@rhatdan
Copy link
Member

rhatdan commented Sep 4, 2023

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andryyy, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Sep 4, 2023
@openshift-merge-robot openshift-merge-robot merged commit 0d577b9 into containers:main Sep 4, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants