Convert k8s connector config to encode in a string

[erwinvaneyk]

Currently, in the Kubernetes storage, the connector config is stored as a JSON object in a byte array. This reduces readability, and prevents creating or editing the config in the Kubernetes resource.

This PR changes this by introducing the configString field to the Kubernetes storage struct of the connector. DEX stores the configuration as a JSON object (to avoid impacting the whole system) into this field, and is able to read both a YAML or JSON-encoded config from it (ff9fc64).

By keeping the original config (for now), this change is backward-compatible. DEX will prefer configString, but fallback to reading from config to be able to support configuration using the old structure.

Old config structure:

apiVersion: dex.coreos.com/v1
kind: Connector
metadata:
  name: github
type: github
id: github
config: e30K

New config structure:

apiVersion: dex.coreos.com/v1
kind: Connector
metadata:
  name: github
id: github
type: github
configString: |
  clientID: $GITHUB_CLIENT_ID
  clientSecret: $GITHUB_CLIENT_SECRET
  redirectURI: dex.example.com/callback
  loadAllGroups: true

EDIT: configString could also be renamed to typeConfig, which might be a bit better

[venezia]

FWIW, per the docs

... These resources are only designed to store state and aren't meant to be consumed by end users. For modifying dex's state dynamically see the API documentation.

While you can store state using kubernetes custom resources, its not intended for you to actually do kubectl ... on these resources. The storage mechanism doesn't implement listwatch (nor use client-go) - it expects that the only changes done are being done by dex itself.

I'm not against the change necessarily, I just want to make sure its clear that you shouldn't be playing with the kubernetes resources directly - dex has a grpc api to do such

[erwinvaneyk]

Hey @venezia - thanks for your review! I appreciate you taking the time to do so 😁

FWIW, per the docs

... These resources are only designed to store state and aren't meant to be consumed by end users. For modifying dex's state dynamically see the API documentation.

Thanks for pointing this out, I missed this when reading the docs! Personally, I don't think we should preemptively discard improvements or alternative approaches based on a 2 year old comment in the documentation. I hope we can weigh this change purely on its own merits and demerits.

While you can store state using kubernetes custom resources, its not intended for you to actually do kubectl ... on these resources. The storage mechanism doesn't implement listwatch (nor use client-go) - it expects that the only changes done are being done by dex itself.

That is a good point, I noticed that (lack of client-go and listwatch) too. It seems to me, that the lack of client-go and listwatch is more of a legacy thing than a deliberate choice/constraint, but, then again, I may be missing the full story?

I'm not against the change necessarily, I just want to make sure its clear that you shouldn't be playing with the kubernetes resources directly - dex has a grpc api to do such

Okay, let's leave the configuring of DEX through the CRDs directly aside for now. 🙂 I think this PR has merit enough because it improves the readability/debuggability of the Connector CRD.

[venezia]

Thanks for pointing this out, I missed this when reading the docs! Personally, I don't think we should preemptively discard improvements or alternative approaches based on a 2 year old comment in the documentation. I hope we can weigh this change purely on its own merits and demerits.

Oh I'm not trying to discard improvements, I was just trying to assert that, at least at one point, this was considered and somebody decided they did not think it was a great idea. Times change and I think that we should always be open to changing things, just like you suggested that it should be evaluated on its own. I was just trying to bring in context.

That is a good point, I noticed that (lack of client-go and listwatch) too. It seems to me, that the lack of client-go and listwatch is more of a legacy thing than a deliberate choice/constraint, but, then again, I may be missing the full story?

The silly answer would be "Do you want the application to be double in size?" (gosh client-go is huge!) but in all seriousness, I do think a few things should be sorted out:

• If we're going to use client-go, we're probably going to want to use the code generators for the custom resources
  • This will pin us to a specific version of client-go, which may not be desired
  • Remember, every 1.X release of Kubernetes results in a major release of client-go, and with that the possibility of breaking changes (I always like when "Most APIs will work" is a caveat)
• Using client-go might help us when Kubernetes changes how service account tokens are handled in the future (the whole BoundServiceAccountTokenVolume thing and that service account tokens will no longer be forever tokens but rather temporal and thus the file will change over time) is logic that I'm guessing is not supported in the current implementation that dex uses
  • Do we use the dynamic client to try to side step some of this?

Okay, let's leave the configuring of DEX through the CRDs directly aside for now. 🙂 I think this PR has merit enough because it improves the readability/debuggability of the Connector CRD.

Sure thing!

[erwinvaneyk]

Oh I'm not trying to discard improvements, I was just trying to assert that, at least at one point, this was considered and somebody decided they did not think it was a great idea. Times change and I think that we should always be open to changing things, just like you suggested that it should be evaluated on its own. I was just trying to bring in context.

Maybe my comment about this was a bit too harsh, @venezia. I really appreciate the context, and it made me dig a bit more in the history to see what has been suggested.

The silly answer would be "Do you want the application to be double in size?" (gosh client-go is huge!) but in all seriousness, I do think a few things should be sorted out: [...]

Haha, that is indeed a legit concern with the Kubernetes tooling! On the other hand, there also seem to be some legitimate concerns with sticking to a self-rolled client too (maintenance cost, performance, etc). In any case, that discussion seems out of the scope of this PR. Since you put a lot of thought on this already, should we open an issue to at least catalog/centralize that discussion there?

[venezia]

Haha, that is indeed a legit concern with the Kubernetes tooling! On the other hand, there also seem to be some legitimate concerns with sticking to a self-rolled client too (maintenance cost, performance, etc). In any case, that discussion seems out of the scope of this PR. Since you put a lot of thought on this already, should we open an issue to at least catalog/centralize that discussion there?

@erwinvaneyk I actually had the same initial thought when I first looked at the dex code - "no client-go? eww! but that explains why dex is so small!" - as I almost always just use client-go for anything k8s related. After looking at it a bit more, possibly becoming more jaded with client-go, I'm unsure.

This code does work, its pretty lean, and while things come and go with any API, the overall limited engagement for how dex interacts with the API (with it truly just treating it as a storage vehicle, not treating it as a message bus or other patterns that you see with k8s api usage) - I don't think we will see the URL paths that k8s' apiserver uses for rest commands to change anytime soon. Thus for a specific section of "maintenance" I'm unsure if it applies.

However, like I mentioned in a previous comment, the intent with service account tokens is to change the token on an interval, unlike the current forever-token situation that is done. I would presume that client-go handles this well, while this code may not. So our use of client-go would be to simply update how we parse kubeconfigs/in-cluster-environments - which is kinda silly on the surface of it, but you are right to suggest that it is typically better to use someone else's library than to roll your own.

I don't think the two of us should make such a decision though - it probably needs a consensus from the maintainers, etc. For me, it falls under the "reasonable people could disagree" pool of decisions.

I would also like to note that in some situations, with some crazy configs and use cases, client-go has failed me and I had to result in just bundling kubectl into my app and shelling out to it. It made me sad and perhaps has made me feel that people who don't use client-go aren't necessarily wrong. Like I said, I have mixed feelings. Also at the time, kubectl had its own logic (note the wg-apply group trying to make apply usable by people using the rest API vs being kubectl exclusive) - so things could have changed but I'm uncertain they have.

To the point of the direct change of this PR - I don't see an obvious problem with changing how the encoding happens for this nested section of the resource.

If you take the approach of "nobody should read/modify this resource directly" then you shouldn't really care how its encoded, thus if this makes other people happy, why not?

Cheers!

[erwinvaneyk]

@erwinvaneyk I actually had the same initial thought when I first looked at the dex code - "no client-go? eww! but that explains why dex is so small!" - as I almost always just use client-go for anything k8s related. After looking at it a bit more, possibly becoming more jaded with client-go, I'm unsure.

This code does work, its pretty lean, and while things come and go with any API, the overall limited engagement for how dex interacts with the API (with it truly just treating it as a storage vehicle, not treating it as a message bus or other patterns that you see with k8s api usage) - I don't think we will see the URL paths that k8s' apiserver uses for rest commands to change anytime soon. Thus for a specific section of "maintenance" I'm unsure if it applies.

However, like I mentioned in a previous comment, the intent with service account tokens is to change the token on an interval, unlike the current forever-token situation that is done. I would presume that client-go handles this well, while this code may not. So our use of client-go would be to simply update how we parse kubeconfigs/in-cluster-environments - which is kinda silly on the surface of it, but you are right to suggest that it is typically better to use someone else's library than to roll your own.

I don't think the two of us should make such a decision though - it probably needs a consensus from the maintainers, etc. For me, it falls under the "reasonable people could disagree" pool of decisions.

I would also like to note that in some situations, with some crazy configs and use cases, client-go has failed me and I had to result in just bundling kubectl into my app and shelling out to it. It made me sad and perhaps has made me feel that people who don't use client-go aren't necessarily wrong. Like I said, I have mixed feelings. Also at the time, kubectl had its own logic (note the wg-apply group trying to make apply usable by people using the rest API vs being kubectl exclusive) - so things could have changed but I'm uncertain they have.

The client-go vs. self-rolled is indeed a whole different discussion, far too large for this minor PR. I don't have a strong opinion on it though. It is just something that you notice when diving into the dex codebase.

To the point of the direct change of this PR - I don't see an obvious problem with changing how the encoding happens for this nested section of the resource.

If you take the approach of "nobody should read/modify this resource directly" then you shouldn't really care how its encoded, thus if this makes other people happy, why not?

Can I interpret this as a "👍" from you? Great! 😜