Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

WIP: [stable/traefik] Keep basic auth user/passwd data in secret #726

Closed
wants to merge 1 commit into from
Closed

WIP: [stable/traefik] Keep basic auth user/passwd data in secret #726

wants to merge 1 commit into from

Conversation

krancour
Copy link
Contributor

@c-knowles you will be interested in this, I think. It's the follow-up to #414

This depends on traefik/traefik#1189 being merged and included in a future Traefik release.

@c-knowles if you want to test drive, the Docker image krancour/traefik:usersfile is built from my fork of Traefik.

@k8s-ci-robot
Copy link
Contributor

Hi @krancour. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 24, 2017
@krancour krancour changed the title WIP: Keep basic auth user/passwd data in secret WIP: [traefik] Keep basic auth user/passwd data in secret Feb 24, 2017
@krancour krancour changed the title WIP: [traefik] Keep basic auth user/passwd data in secret WIP: [stable/traefik] Keep basic auth user/passwd data in secret Feb 24, 2017
@cknowles
Copy link
Contributor

Great! Let's hope they merge that in soon. I wonder if we could cut down on some of the conditionals and always create the secret but with empty users if non are defined? We could always mount the secret too which keeps deployment.yaml simpler.

@@ -7,3 +7,7 @@ We truncate at 63 chars because some Kubernetes name fields are limited to this
{{- define "fullname" -}}
{{- printf "%s-%s" .Release.Name .Chart.Name | trunc 63 | trimSuffix "-" -}}
{{- end -}}

{{- define "dashboardUsers" -}}
{{- range $key, $value := .Values.dashboard.auth.basic -}}{{- printf "%s:%s\n" $key $value -}}{{- end -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be wrapped in {{- if .Values.dashboard.auth }}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The only thing that calls this is wrapped in those conditionals already and I don't foresee this being reused in this chart. (I didn't do it this way for the sake of reuse... this "sub-template" was the only way I could think of, at the time, to build a string by ranging over an array, and then get that string into a pipeline so I can base64 encode it... but I think I've thought of a more straightforward way to do that now, so I'll be updating this on Monday.)

@lachie83
Copy link
Contributor

@c-knowles What's the latest on this?

@cknowles
Copy link
Contributor

It looks like @krancour's PR above has been merged so I hope they include that with 1.2.0 GA. Otherwise the change looks good to me.

@lachie83
Copy link
Contributor

@c-knowles do we need to wait for that release for these changes to work?

@cknowles
Copy link
Contributor

@lachie83 I don't think @krancour's changes here would break anything existing but yep we'll have to wait for that next Traefik release for this to work.

@krancour
Copy link
Contributor Author

I have some modifications I want to make to this chart still. I don't have a real sense of urgency about it because, as noted by @c-knowles I'm waiting for my PR to Traefik to make it into a GA release.

@lachie83
Copy link
Contributor

@krancour @c-knowles shall we close this for now?

@krancour
Copy link
Contributor Author

@lachie83 depends how much it bothers you to leave it open. Personally, for me, it's better to leave it open because then it doesn't fall off my own radar. Up to you tho.

@prydonius
Copy link
Member

@krancour just checking in, we still waiting for an upstream release here?

@prydonius prydonius added the stale label Jun 1, 2017
@prydonius
Copy link
Member

@krancour ping

@viglesiasce
Copy link
Contributor

Closing as this has now been marked stale for 17 days:

https://github.com/kubernetes/charts#stale-pull-requests

@krancour
Copy link
Contributor Author

I'll have another look at this. I'm sure the feature I contributed to Traefik (which this chart depends on) must be included in 1.3.x. Not sure when I will get to this though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changes needed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants