Add ability to specify multiple Keycloak client templates - #49
HappyTetrahedron wants to merge 3 commits into
Conversation
019f400 to
17d5bea
Compare
simu
left a comment
simu
left a comment
There was a problem hiding this comment.
I think this is a good start.
Is re-using the same role mapping for everything good enough?
Right now, it's probably fine, but eventually, we may want to be able to not give customers access to everything that we can access.
Is the new secret naming OK or should the path be different?
See inline suggestion for a couple alternatives.
Overall, I think we should keep this open until @bastjan has a chance to review it.
| return ctrl.Result{}, fmt.Errorf("unable to template keycloak clients: %w", err) | ||
| } | ||
| // template client roles | ||
| rolesRaw, err := jvm.EvaluateFile(r.ClientRoleMappingTemplateFile) |
There was a problem hiding this comment.
We probably could pass the templated client while evaluating the roles so we can have different mappings for different clients.
There was a problem hiding this comment.
Smart. I hope it's ok to reuse the JVM the way I did?
| return []gocloak.Client{}, fmt.Errorf("unable to evaluate jsonnet: %w", err) | ||
| } | ||
| var cs []gocloak.Client | ||
| if err := json.Unmarshal([]byte(cRaw), &cs); err != nil { |
There was a problem hiding this comment.
While good enough this could be done slightly nicer with jsontext.Decoder.PeekKind().
There was a problem hiding this comment.
This necessitated an update to go 1.27, I hope I did that right
|
Looks pretty good. But: I'd pass the client to the mapping Jsonnet (evaluate once per client) so we can have individual mappings. |
Use jsontext.PeekKind to distinguish between single client and client list Update go to 1.27.1 for jsontext support Provide current client as context when rendering client role mappings
3 participants
Summary
Open questions
Checklist
bug,enhancement,documentation,change,breaking,dependencyas they show up in the changelog