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

Create Dex helm chart #2

Closed
2 of 3 tasks
sagikazarmark opened this issue Jan 14, 2021 · 17 comments · Fixed by #3
Closed
2 of 3 tasks

Create Dex helm chart #2

sagikazarmark opened this issue Jan 14, 2021 · 17 comments · Fixed by #3
Assignees

Comments

@sagikazarmark
Copy link
Member

sagikazarmark commented Jan 14, 2021

Fixes dexidp/dex#1709

Blocked by:

Potential blockers / nice to haves:

We should probably sort dexidp/dex#1893 out as well before releasing a chart to avoid breaks later.

@TJM
Copy link

TJM commented Feb 8, 2021

Could we start with the last (real) version that was published to "stable/dex" , 2.5.0?

For anyone else who is getting constant warnings from helm-operator or if you need to make a change and can't get the chart anymore, I have switched over to https://repo.chartcenter.io/ and the chart name is stable/dex ... but again, it is deprecated, and I don't know how it will fare after helm/charts goes away, so the sooner the better on this... even if it is not "all the things" to "all the people" ;)

~tommy

@Raboo
Copy link

Raboo commented Feb 9, 2021

I'm using the old deprecated git repo for helm-operator until a new one is made.

spec:
  releaseName: dex
  chart:
    git: https://github.com/helm/charts
    ref: master
    path: charts/stable/dex

But I agree with @TJM, it's not always necessary to wait for a bunch of blockers. That's why semantic versioning exist, to alert people that a breaking change has occurred when a new major version is released. The current state is worse, i.e. not having an official helm chart.

@sagikazarmark
Copy link
Member Author

I've submitted a PR for an initial version of the new chart: #3

Can you please give it a try and share some feedback?

@TJM @Raboo

@jeanlucmongrain
Copy link

@sagikazarmark https://charts.dexidp.io/index.yaml is empty

Error: dex chart not found in repo https://charts.dexidp.io

@sagikazarmark
Copy link
Member Author

It's not yet merged. You should clone the repo and checkout the dex branch.

@Raboo
Copy link

Raboo commented Feb 15, 2021

@sagikazarmark
I tried upgrading from the old helm chart using helm-operator. And it didn't work.

caller=release.go:79 component=release release=dex targetNamespace=dex resource=dex:helmrelease/dex helmVersion=v3 info="starting sync run"
caller=release.go:353 component=release release=dex targetNamespace=dex resource=dex:helmrelease/dex helmVersion=v3 info="running upgrade" action=upgrade
caller=helm.go:69 component=helm version=v3 info="preparing upgrade for dex" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="resetting values to the chart's original version" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="performing update for dex" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="creating upgraded release for dex" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="checking 5 resources for changes" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="Created a new Secret called \"dex-config\" in dex\n" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="error updating the resource \"dex\":\n\t cannot patch \"dex\" with kind Deployment: Deployment.apps \"dex\" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\"app.kubernetes.io/instance\":\"dex\", \"app.kubernetes.io/name\":\"dex\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="Created a new Ingress called \"dex\" in dex\n" targetNamespace=dex release=dex
caller=helm.go:69 component=helm version=v3 info="warning: Upgrade \"dex\" failed: cannot patch \"dex\" with kind Deployment: Deployment.apps \"dex\" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\"app.kubernetes.io/instance\":\"dex\", \"app.kubernetes.io/name\":\"dex\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable" targetNamespace=dex release=dex
caller=release.go:357 component=release release=dex targetNamespace=dex resource=dex:helmrelease/dex helmVersion=v3 error="upgrade failed: cannot patch \"dex\" with kind Deployment: Deployment.apps \"dex\" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{\"app.kubernetes.io/instance\":\"dex\", \"app.kubernetes.io/name\":\"dex\"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable" action=upgrade

Deleting, recreating works however.
My setup looks like this now for reference.

---
apiVersion: helm.fluxcd.io/v1
kind: HelmRelease
metadata:
  name: dex
  namespace: dex
spec:
  releaseName: dex
  chart:
    git: https://github.com/dexidp/helm-charts
    ref: 4d4bff2ca1
    path: charts/dex
  values:
    ...

@sagikazarmark
Copy link
Member Author

@Raboo Thanks for your feedback.

Chances are directly upgrading won't work. This is an entirely new chart, so forward compatibility is not a requirement.

@Raboo
Copy link

Raboo commented Feb 15, 2021

Thank you for creating the chart!

It might be good to mention in the README that there is no upgrade path from the old official chart.
Proper migration would be to create a new dex deployment with this new chart and then swap the ingress url once the "new" dex is up and deleting the old afterwards to avoid downtime.

Or should one assume that people working with Kubernetes should be skilled enough to figure it out by themselves?

@sagikazarmark
Copy link
Member Author

It might be good to mention in the README that there is no upgrade path from the old official chart.

I think that makes sense, thanks!

@TJM
Copy link

TJM commented Feb 16, 2021

Sorry, I have been away from my desk...

I agree, If an "upgrade" from the old official stable/dex to this new dexidp/dex chart is not supported, the "README" should certainly outline the correct replacement procedure. You might as well put it under the "Upgrading" section, even if the first sentence just says that upgrading is not supported, do this, then that, then this other thing instead, as that is what everyone is "expecting" to be able to do.

I would also recommend re-hosting all the old versions that are soon to be "archived" from stable/dex. Just leaving people hanging, not able to upgrade, but not able to re-install their current version is a bit rough.

Thoughts?

In our case, dex is basically stateless, so a rip-out and replacement might work, so long as we can apply the same configuration???

@Raboo
Copy link

Raboo commented Feb 16, 2021

The dex configuration has not change, it's still dex. The helm chart have however changed some of the configuration values. So you can not take the values straight off, some values have moved and such. Some new values are added, some might have be removed.

Yes, you can do a "rip-out and replacement". But your replacement needs different helm values.

@sagikazarmark
Copy link
Member Author

I've added a short note that upgrading is not possible from the old chart.

I would also recommend re-hosting all the old versions that are soon to be "archived" from stable/dex. Just leaving people hanging, not able to upgrade, but not able to re-install their current version is a bit rough.

AFAIK these charts are already archived, but they are not going anywhere.

Note from the repo:

This git repository will remain as an archive.

Stable charts are also available on GitHub: https://charts.helm.sh/stable/

@sagikazarmark
Copy link
Member Author

sagikazarmark commented Feb 16, 2021

I've merged the PR into master, so you should be able to pull the chart from the repository.

This is not considered to be a stable chart yet. The first stable version will be 0.1.0.

Please share feedback, so we can improve the chart. Thanks!

@TJM
Copy link

TJM commented Feb 16, 2021

    kubeVersion: '>=1.16.0-0'

... guess I can't test it, our clusters are stuck on 1.15.x for "reasons" :)
Is there really anything in this chart that would prevent it from working on 1.15.x?

@sagikazarmark
Copy link
Member Author

Unfortunately I can't test it on a cluster older than 1.16 :)

I guess it can run on 1.15 currently, but once we start switching to newer Ingress resources, support for old Kubernetes versions will have to be dropped.

Let me check if we can add more Kubernetes versions to the test pipeline.

@TJM
Copy link

TJM commented Feb 16, 2021

I understand. 1.15.x is very old, I hate these clusters :)

@sagikazarmark
Copy link
Member Author

@TJM checkout 0.0.2

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 a pull request may close this issue.

4 participants