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

helm chart for transfer.sh #255

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

helm chart for transfer.sh #255

wants to merge 15 commits into from

Conversation

beyondszine
Copy link

Hi there,
This pull request tries to add helm chart feature for transfer.sh. I have currently deployed this chart in my testing cluster. Let me know in case of any changes required.

@aspacca
Copy link
Collaborator

aspacca commented Aug 17, 2019

hi @beyondszine thanks for the PR, it is really welcome

anyway I cannot merge as it is with default S3 provider: we should provide proper switch in the values.yaml to generate the template for any provider type (local, s3 and gdrive)

let me know if you have time to do it otherwise I will do as soon I'll have time based on your branch

thanks

Dockerfile Outdated
@@ -10,6 +10,7 @@ ADD . /go/src/github.com/dutchcoders/transfer.sh
WORKDIR /go/src/github.com/dutchcoders/transfer.sh

ENV GO111MODULE=on
ENV APP_PORT=80
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will work for new helm template but actually break everything else, please, keep 8080 as listener port

Copy link
Author

Choose a reason for hiding this comment

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

yes, indeed. Its my mistake. I forgot about it while pushing. Will make sure it stays same as per transfersh's convention.

@beyondszine
Copy link
Author

hi @beyondszine thanks for the PR, it is really welcome

anyway I cannot merge as it is with default S3 provider: we should provide proper switch in the values.yaml to generate the template for any provider type (local, s3 and gdrive)

let me know if you have time to do it otherwise I will do as soon I'll have time based on your branch

thanks

For sure. It makes sense.
I will update the chart for getting it executed on user's choice provided in values.yaml file.

Also, I want to ask your opinion on a few things.

  1. The mechanism to provide the binary's args.
    As of now I thought of them as being secrets ( because of AWS_ACCESS_KEY ..etc). Will that be okay for future cases too? or somethings are seen better as configMaps ?

  2. Is there a convention for replacing hyphens to underscores for params name ?
    ex-
    aws-access-key => AWS_ACCESS_KEY

  3. All Other params here that are to be provided to transfersh binary can be provided by the mechanism as asked in point 1 above too.
    So, can there be a convention that all the params are only usable by binary if you put them in corresponding secret/configMap?
    This way, every param value can be found in secret by following some mapping mechanism.
    Ex-
    http-auth-user param will be automatically fetched if there is a value in secret by the key of
    http-auth-user | upper | replace hyphens by underscores | quote

I asked above points because this way, may be a very general helm template can be made to inject param values(if they follow some standard) from values.yaml file.
Let me know of your suggestions on the same.

--

@beyondszine
Copy link
Author

hi @beyondszine thanks for the PR, it is really welcome
anyway I cannot merge as it is with default S3 provider: we should provide proper switch in the values.yaml to generate the template for any provider type (local, s3 and gdrive)
let me know if you have time to do it otherwise I will do as soon I'll have time based on your branch
thanks

For sure. It makes sense.
I will update the chart for getting it executed on user's choice provided in values.yaml file.

Also, I want to ask your opinion on a few things.

  1. The mechanism to provide the binary's args.
    As of now I thought of them as being secrets ( because of AWS_ACCESS_KEY ..etc). Will that be okay for future cases too? or somethings are seen better as configMaps ?

Added support for both. values.yaml file contains .Values.argValues.source.type can be secretKeyRef or configMapKeyRef

  1. Is there a convention for replacing hyphens to underscores for params name ?
    ex-
    aws-access-key => AWS_ACCESS_KEY

Added as there comes some issues for hyphens in some shells with env vars.

  1. All Other params here that are to be provided to transfersh binary can be provided by the mechanism as asked in point 1 above too.
    So, can there be a convention that all the params are only usable by binary if you put them in corresponding secret/configMap?
    This way, every param value can be found in secret by following some mapping mechanism.
    Ex-
    http-auth-user param will be automatically fetched if there is a value in secret by the key of
    http-auth-user | upper | replace hyphens by underscores | quote

All the values in given by user at .Values.argValues.paramNames in values.yaml will be fetched, converted to secret/configMap name & injected in container at runtime.

Let me know your views on above points.

I asked above points because this way, may be a very general helm template can be made to inject param values(if they follow some standard) from values.yaml file.
Let me know of your suggestions on the same.

--

@aspacca
Copy link
Collaborator

aspacca commented Aug 20, 2019

@beyondszine thank you for your effort. I will try to review and give proper feedback by the end of the week

@beyondszine
Copy link
Author

@beyondszine thank you for your effort. I will try to review and give proper feedback by the end of the week

Sure thing.

"type" : "secretKeyRef",
"name" : "transfersh-secrets"
},
"paramNames" :[
Copy link
Collaborator

Choose a reason for hiding this comment

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

a lot of parameters missing

Copy link
Author

Choose a reason for hiding this comment

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

umm, I couldn't get what you mean here.
The idea why I wrote this is,
whatever params are put in this array of 'paramNames' will be reflected into the secrets automatically(because of the loop). I only put a few for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but then either this is a POC or it only supports S3 provider
for me in order for the branch to be merged should support every provider/flag that transfer.sh supports

kubectl run transfersh --restart=Never --image=dutchcoders/transfer.sh:latest -- --http-auth-user my-username --http-auth-pass somepassword --provider s3 --aws-access-key $AWS_ACCESS_KEY_ID --aws-secret-key $AWS_SECRET_ACCESS_KEY --bucket $AWS_TRANSFERSH_BUCKET --s3-region $AWS_TRANSFERSH_BUCKET_REGION

# Example to manually create needed secrets for deployment params totally aligned with [Usage Params](https://github.com/dutchcoders/transfer.sh#usage-1)
kubectl create secret generic transfersh-secrets --from-literal=HTTP_AUTH_USER=$HTTP_AUTH_USER --from-literal=HTTP_AUTH_PASS=$HTTP_AUTH_PASS --from-literal=AWS_ACCESS_KEY=$AWS_ACCESS_KEY --from-literal=AWS_SECRET_KEY=$AWS_SECRET_KEY --from-literal=BUCKET=$BUCKET --from-literal=S3_REGION=$S3_REGION --from-literal=PROXY_PATH=$PROXY_PATH --from-literal=PROVIDER=$PROVIDER
Copy link
Collaborator

Choose a reason for hiding this comment

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

indeed I don't need a secret is needed, but just a config map.
anyway, that's not the problem: any solution we chose should be taken care of by the helm template, not from some previous setup directly in k8s

Copy link
Author

Choose a reason for hiding this comment

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

umm, this is just for an example of how a secret addition can be done as AWS_ACCESS & AWS_SECRET are things people may not want to put in configmap. But anyway, the option to put both configMap/secret is given by the values.yaml.

Addition of params & their values in configmap can also be given inthe readme.
Idea of putting it in REAMDE was to enable people to also be able to manually do deployment,service,secret/configmap in their cluster Other than automated helm-chart deployment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the problem is not being a secret or a config map
the problem is that in order to use the helm chart I have to create the secret directly in k8s
this should be taken care of by the chart itself

name: {{ $keyRefName }}
key: {{ . | upper | replace "-" "_"}}
{{- end }}
args: [ {{- range .Values.argValues.paramNames }} {{ printf "%s%s" $.Values.argIdentifier . | quote}},{{ printf "%s%s%s" "$(" . ")" | upper | replace "-" "_" | quote}},{{- end }} ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

for the moment, assuming we will add the missing supported params, it should work, because currently no param is mutually exclusive to each other
tomorrow this would not be true, anyway: we may have conflicting params

also, even if I've not tested, for bool param having the flag set could be enough to enable them: so passing them by default should be avoided, and rather passed or not according to their value

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I guess I understand what you mean.
But isn't the job of params validation & their compatibility should be taken care by the binary itself and not the wrapping frameworks(docker,CI/CD,kubernetes etc.).
If the params have conflict, shouldn't they be just passed through from outside to the binary and let binary handle them & give appropriate error.

For bool values, you point makes sense. But since our param names are not indicating if they are bool or not, then there must be a way in helm layer to know the difference between a bool param & otherwise.
May be you can give me some hint on some you would want Values.yaml to be(specifically declaring params), so that I can backtrack & understand better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the binary does params validation: if you pass --provider local without --basedir it won't start
tomorrow it could be the case that if you pass params related to S3 when using local provider will fail validation as well: if the chart just passes everything it won't work or will enable functionality that use doesn't want to enable (like with the bool flags)

I'm not such an expert of helm so I don't know what's the most idiomatic way to solve this issue: I guess having different values.yaml for every different provider can be a starting point. for bool flags we could use flag.enabled like you did for the ingress and then pass --set flag.enabled=true to helm

people that has a stable/immutable deployment needing can write their own custom values.yaml, others can rely on the default provided ones and the --set flag

Copy link
Author

Choose a reason for hiding this comment

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

I get it now, what you mean here & above.
Give me some time, let me get back to you on how I think what you say, can be implemented (only as a plan) & if we agree, then probably I can fix changes in code.

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

2 participants