Skip to content

feat!: support setting the username for the relational database #891

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 24, 2025
Merged

feat!: support setting the username for the relational database #891

merged 2 commits into from
Jul 24, 2025

Conversation

toelke
Copy link
Contributor

@toelke toelke commented Jul 23, 2025

fixes #889

@netlify Netlify
Copy link

netlify bot commented Jul 23, 2025 •
edited
Loading

✅ Deploy Preview for kamaji-documentation canceled.

Name Link
🔨 Latest commit e5ae3b4
🔍 Latest deploy log https://app.netlify.com/projects/kamaji-documentation/deploys/6881f302b6e7470008335bd6

prometherion
prometherion reviewed Jul 23, 2025
View reviewed changes
@toelke toelke marked this pull request as ready for review July 24, 2025 08:00
@toelke
Copy link
Contributor Author

toelke commented Jul 24, 2025

First tests look promising. We are base36 encoding the UUIDs we have in our namespaces + cluster names to generate schema+username.

@toelke toelke changed the title Support setting the username for the relational database feat: Support setting the username for the relational database Jul 24, 2025
prometherion
prometherion requested changes Jul 24, 2025
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small requested change, since we need to set default values as we're doing with the scheme:

kamaji/internal/webhook/handlers/tcp_defaults.go

Lines 75 to 78 in 382d327

if len(tcp.Spec.DataStoreSchema) == 0 {
dss := strings.ReplaceAll(fmt.Sprintf("%s_%s", tcp.GetNamespace(), tcp.GetName()), "-", "_")
tcp.Spec.DataStoreSchema = dss
}

Overall proposed changes are good, thanks for contributing to Kamaji!

@toelke toelke force-pushed the allow-chosing-the-database-username branch 3 times, most recently from 3c3198a to 3404da9 Compare July 24, 2025 08:33
@toelke toelke force-pushed the allow-chosing-the-database-username branch from 3404da9 to e5ae3b4 Compare July 24, 2025 08:46
@toelke toelke requested a review from prometherion July 24, 2025 08:47
prometherion
prometherion approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@prometherion prometherion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to welcome you as a new Kamaji contributor: LGTM, thanks for jumping on this quickly!

internal/resources/datastore/datastore_storage_config.go
Comment on lines +165 to +167
default:
// this can only happen on TCP creations when the webhook is not installed
return fmt.Errorf("cannot build datastore storage config, username must either exist in Spec or Status")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done for handling any edge case like this! 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it was copy+paste from code handling the schema :-D

@prometherion prometherion changed the title feat: Support setting the username for the relational database feat!: support setting the username for the relational database Jul 24, 2025
@prometherion prometherion merged commit 0990317 into clastix:master Jul 24, 2025
12 checks passed
@rpardini
Copy link
Contributor

I started hitting this after this landed.

ERROR	Reconciler error	
{"controller": "tenantcontrolplane", "controllerGroup": "kamaji.clastix.io", 
"controllerKind": "TenantControlPlane", "TenantControlPlane": {<snip>, 
"error": "cannot build datastore storage config, username must either exist in Spec or Status"}

using a mostly default Kamaji Helm deployment and etcd.

@toelke
Copy link
Contributor Author

toelke commented Jul 28, 2025

Did you update the CRDs?

@prometherion
Copy link
Member

Exactly, CRDs must updated: I marked this PR as breaking (feat!) since it must be updated manually.

I'm trying to implement an Helm Chart to manage Kamaji CRDs so they can be updated programmatically and avoid these potential "bugs".

@rpardini
Copy link
Contributor

Here's what happened: my cluster nodes come and go a lot, so I think the running Kamaji controller pod got a new version.
I've redeployed Helm manually, but it still persisted.
Finally I've forced image.tag=edge-25.7.2 and it's back to working.

@prometherion
Copy link
Member

As I said before, Helm doesn't provide a native way to manage CRDs, and using latest for Kamaji can be good for testing new features, but it's strongly not suggested for production environments.

I just proposed a change to manage CRDs natively with #894: in this way, you can manage CRDs in parity with the code.

@matofeder matofeder mentioned this pull request Aug 1, 2025
Closed
This was referenced Aug 8, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prometherion prometherion prometherion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When using long names for namespaces and TCPs, kine uses truncated username+schema
3 participants
@toelke @rpardini @prometherion