-
Notifications
You must be signed in to change notification settings - Fork 74
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
fix: credential secret does not works if set from values #113
Conversation
Thanks for this, that was exactly the problem I was getting! Here's a kustomize patch that I used in my ArgoCD app to apply those changes, that could help others before the PR is merged apiVersion: apps/v1
kind: Deployment
metadata:
name: onepassword-connect
spec:
template:
spec:
containers:
- name: connect-api
env:
- name: OP_SESSION
value: /home/opuser/.op/1password-credentials.json
valueFrom:
volumeMounts:
- name: credentials
mountPath: /home/opuser/.op/1password-credentials.json
subPath: 1password-credentials.json
- name: connect-sync
env:
- name: OP_SESSION
value: /home/opuser/.op/1password-credentials.json
valueFrom:
volumeMounts:
- name: credentials
mountPath: /home/opuser/.op/1password-credentials.json
subPath: 1password-credentials.json |
@jillianwilson |
@grhawk it's still a problem i guess-- I'm unable to use this chart. |
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
Hey @grhawk thanks for the PR for this! I know this has been sitting here for a bit, but I reviewed today and tested and all looks good. My only feedback is that you sign your commit, as commits need to be signed to merge into main. I could sign your commit on your behalf but then it would look like I wrote your commit. I want to make sure you are properly credited for your work :) |
Hi @jillianwilson, I ll sign the commit by the end of the week! Best |
I created a new PR since I could not sign the commit I did in this one (the rebase had conflicts and it was too long after I did the PR to remember what I did :)) I am closing this one and pls continue with #196 |
Hi, thank you for your nice job!
I think I found a couple of minor issues. However, it could be I did not get the right way to use the chart :).
Double encoded secret
The main repo creates the
connect-credentials
secret using thestringData
method but provides binary strings:connect-helm-charts/charts/connect/templates/connect-credentials.yaml
Lines 14 to 20 in a9e4e31
This ends up encoding the string twice making the mounted file not accessible by the software. Using the
data
method works as expected since the helm chart takes care of converting to binary when needed.1password-credential.json
stored in an environmental variableThe main repo expects the server codes to use to read the credentials directly from the
OP_SESSION
variable. Thus the helm chart reads like this:connect-helm-charts/charts/connect/templates/connect-deployment.yaml
Lines 61 to 66 in a9e4e31
This stores the value of the secret in the
OP_SESSION
variable but the docs of the op-server are quite clear on the fact that the variable only defines the path to the credential file.I fixed this issue by creating a volume out of the credential-secret and mounting it always in the expected spot. I don't see the need of allowing the users of the chart to define directly the
OP_SESSION
. In case, this can be easily modified though.