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

Fix #2229 - dex-passwords in common/dex/base #2588

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

kromanow94
Copy link
Contributor

The dex-passwords secret has not been defined properly and the secret wasn't added to the dex kustomization file.

Description of your changes:
The changes are done as discussed in #2229 (review).

The dex-passwords secret didn't have the secret defined properly and the secret wasn't added to the dex kustomization file.
@juliusvonkohout
Copy link
Member

Are you sure that the string is allowed like this? i am under the impression, that we have to encode it since it contains the dollar sign.

"You can define the Secret object in a manifest first, in JSON or YAML format, and then create that object. The Secret resource contains two maps: data and stringData. The data field is used to store arbitrary data, encoded using base64. The stringData field is provided for convenience, and it allows you to provide the same data as unencoded strings. The keys of data and stringData must consist of alphanumeric characters, -, _ or .."

https://kubernetes.io/docs/tasks/configmap-secret/managing-secret-using-config-file/#create-the-config-file

@kromanow94
Copy link
Contributor Author

I was able to verify that this form works:

$ git checkout fix-pull-2229
$ git reset --hard
$ git log -1
commit 8aafa9a3041e2bec2441dd44df4854288ede6a04 (HEAD -> fix-pull-2229, origin/fix-pull-2229)
Author:     Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
AuthorDate: Tue Jan 2 13:50:19 2024 +0000
Commit:     Krzysztof Romanowski <krzysztof.romanowski.kr3@roche.com>
CommitDate: Tue Jan 2 13:50:19 2024 +0000

    fix #2229 - dex-passwords in common/dex/base

$ kustomize build common/dex/base/ | kubectl apply -f - --dry-run=server -oyaml | yq '.items[] | select(.metadata.name == "dex-passwords")| .data.DEX_USER_PASSWORD' -r | base64 -d; echo
$2y$12$4K/VkmDd1q1Orb3xAt82zu8gk7Ad6ReFR4LCP9UeYE90NLiN9Df72

But, I agree that even if this works, we should comply with the documentation because in future the K8s system can be more restrictive and more in-sync with the docs.

I've made a change to define the value encoded with base64.

@kromanow94
Copy link
Contributor Author

Ah, I see that this wasn't necessary... The docs talk about the key in data and stringData. It was the value that really had the dollar sign.

The keys of data and stringData must consist of alphanumeric characters, -, _ or ..

With that in mind, I'm more in favor of going with stringData to be more explicit. I'll make the change and bring it back.

@juliusvonkohout
Copy link
Member

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juliusvonkohout, kromanow94

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

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.

2 participants