[stable/dex] - Dex fix install

[sstarcher]

In dex's current state it's fully non-functional. The chart requires secrets to be created which the chart can do, but it's in a post-install hook which is never reachable as the deployment hangs for secrets.

This moves the hooks to pre-install along with creating the roles in the pre-install stage to ensure the jobs can run.

By default SSL certs are created by HTTPS was not exposed this is no longer the case.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sstarcher
To complete the pull request process, please assign desaintmartin
You can assign the PR to them by writing /assign @desaintmartin in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• stable/dex/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sstarcher]

/assign @desaintmartin

[sstarcher]

/ok-to-test

[desaintmartin]

Thanks!

Could you add a test file in the ci directory (see elasticsearch for example) to "test" this?

[sstarcher]

@desaintmartin any reason we need a test file? We don't have any settings that need to be set for lint to pass.

[desaintmartin]

Indeed, I thought it was a custom value different from the default one.
I would need to dig deeper here as I use this chart without problem with .Values.certs.grpc.create set to defaults.

[desaintmartin]

This seems related to #6679. @ccojocar, @unguiculus, any idea before in have a deeper look?

[sstarcher]

@desaintmartin Fairly certain that's not possible. And this issue existed prior to #6679. Prior you have two ways to install this and have it work.

1. Install with replicas = 0
2. Create the secrets by hand

[desaintmartin]

Good catch.

[desaintmartin]

helm install stable/dex --name dex-test works for me without any issue. While I understand the reasons of changing this I just would like to reproduce the problem before merging this.

Maybe you are using --wait ?

[sstarcher]

Looks like I was mistaken as to the root cause as to why I saw this issue. It is related to --wait and replicas=2. I assumed that it was blocked due to the service not being healthy, but it was actually blocking because the deployment was not fully rolled out.

helm install stable/dex --name dex-test --set replicas=2 --wait

[desaintmartin]

Thanks for investigating. So do we still need a change?

[sstarcher]

@desaintmartin I think you do. This chart should install with any proper settings. It's currently taking advantage of odd behavior in how the deployment functions with --wait. My changes allow it to work properly in all situations.

[sstarcher]

Anything else I need to do besides updating the Chart.yaml again?

[desaintmartin]

Note that this does not validate against Deployment schema, as "port" does not exist in ContainerPort. It might be worth filtering this is deployment.yaml

[sstarcher]

fixed

[desaintmartin]

My concern is that we are creating a lot of roles (clusterrole, clusterrolebinding, role, rolebinding) that are not cleaned up after deletion. Maybe duplicating them and adding a hook-succeeded could work, but it becomes to be hard to maintain.

Couldn't we switch to a initContainer approach where it generates what we need if it does not already exist?

[sstarcher]

@desaintmartin If that's your concern we are perfectly fine. These hooks get cleaned up on deletion.

 k get clusterrole -o yaml dex-test | grep hook                                                                                                                     
    helm.sh/hook: pre-install
    helm.sh/hook-delete-policy: before-hook-creation
    helm.sh/hook-weight: "-1"
    
 helm delete --purge dex-test                                                                                                                                      
release "dex-test" deleted

 helm get clusterole dex-test                                                                                                                                       
Error: release: "clusterole" not found

[desaintmartin]

That's weird, this is not what I get, and https://github.com/helm/helm/blob/master/docs/charts_hooks.md\#hook-resources-are-not-managed-with-corresponding-releases states the opposite.

$ helm github install --repo https://github.com/sstarcher/charts --path stable/dex --ref dex-fix-install --name test-to-be-removed
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Already up to date.
Switched to branch 'dex-fix-install'
Your branch is behind 'origin/dex-fix-install' by 2 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)
/Users/cedricdesaintmartin/.helm
Installing the Helm chart from the GitHub repo
k get roleNAME:   test-to-be-removed
LAST DEPLOYED: Tue Jul  2 18:21:44 2019
NAMESPACE: default
STATUS: DEPLOYED

RESOURCES:
==> v1/Pod(related)
NAME                                     READY  STATUS             RESTARTS  AGE
test-to-be-removed-dex-5f8d4d844f-r29jk  0/1    ContainerCreating  0         0s

==> v1/Secret
NAME                    TYPE    DATA  AGE
test-to-be-removed-dex  Opaque  1     0s

==> v1/Service
NAME                    TYPE       CLUSTER-IP   EXTERNAL-IP  PORT(S)                             AGE
test-to-be-removed-dex  ClusterIP  10.233.9.61  <none>       8080/TCP,443/TCP,5000/TCP,5558/TCP  0s

==> v1beta2/Deployment
NAME                    READY  UP-TO-DATE  AVAILABLE  AGE
test-to-be-removed-dex  0/1    1           0          0s


NOTES:
1. Get the application URL by running these commands:
  export POD_NAME=$(kubectl get pods --namespace default -l "app=dex,release=test-to-be-removed" -o jsonpath="{.items[0].metadata.name}")
  echo "Visit https://127.0.0.1:8080/.well-known/openid-configuration to use your application"
  kubectl port-forward $POD_NAME 8080:5556

18:22:10 cedricdesaintmartin@Cedrics-MBP.lan .helm k get roles
NAME                      AGE
dusty-olm-dex             30m
test-to-be-removed-dex    31s
wiremind-roles            34d
wiremind-roles-operator   125d

$ helm delete --purge test-to-be-removed                                                                                                           
release "test-to-be-removed" deleted

$ k get role
NAME                      AGE
test-to-be-removed-dex    59s

[sstarcher]

Yep, you are correct. In the above, I mistakenly ran helm not kubectl... Overall I don't see the issue adding 2 additional things that don't get removed on purge, but I'll leave that up to you. As is users must always install with a single replica or run into the issue I had.

[desaintmartin]

Those "two additional things" are giving access to accounts. This is a security issue.

[sstarcher]

Giving access to accounts only if someone has permissions to bind them. I'll go ahead and close this out.