-
-
Notifications
You must be signed in to change notification settings - Fork 117
Ensure st2.datastore_crypto_key and st2chatop.env values are strings #241
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
Conversation
5761d15 to
c930db5
Compare
|
@armab I would like to include this one in the next release as well. |
| {{- if kindIs "string" .Values.st2.datastore_crypto_key }} | ||
| datastore_crypto_key: {{ .Values.st2.datastore_crypto_key | b64enc }} | ||
| {{- else }} | ||
| datastore_crypto_key: {{ .Values.st2.datastore_crypto_key | toJson | b64enc }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't add an option for users to shoot themselves in their foot.
Datastore crypto key is an artifact that you generate and copy-paste, instead of trying to split it into pieces or re-format with yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I shot myself in the foot. JSON is valid YAML. If you don't surround what you copy/paste with single quotes, it will be parsed into a dictionary and then things break.
This change makes the chart more forgiving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it also mean https://github.com/StackStorm/stackstorm-ha/blob/3dcd06b772ca5a2f4133a22fa9b382e03b5b422a/values.yaml#L50-L53 had initially wrong instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This change makes those instructions valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the Helm values instructions? Similar to what we had for SSH key
datastore_crypto_key: |
{"hmacKey": {"hmacKeyString": "", "size": 256}, "size": 256, "aesKeyString": "", "mode": "CBC"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
Which part of this change do you dislike? The kindIs "String" check, or passing the value through toJson?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to drop toJson here and update the docs in values.yaml instead.
I would still like to understand your intuition - Why/how does serializing the datastore_crypto_key create "an option for users to shoot themselves in their foot"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it comes from the expectations that the datastore crypto key is an immutable artifact that users just generate and copy-paste, instead of trying to split it into pieces, re-format, or modify. So it's best to keep that piece of data as is to avoid any further confusion or corner cases.
| data: | ||
| {{- range $env, $value := .Values.st2chatops.env }} | ||
| {{ $env }}: {{ $value | b64enc | quote }} | ||
| {{ $env }}: {{ $value | toString | b64enc | quote }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good to me
c930db5 to
42a4a84
Compare
arm4b
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
datastore_crypto_key and st2chatops secrets are both likely to have non-string content.
Examples:
ST2_COMMANDS_RELOAD_INTERVAL: 10instead ofST2_COMMANDS_RELOAD_INTERVAL: "10")We can easily cast these to string in the secret templates to fix this.
Resolves #229