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

DetermineNetwork method always try to migrate DB when booting up #1399

Closed
upgle opened this issue Jun 7, 2021 · 2 comments
Closed

DetermineNetwork method always try to migrate DB when booting up #1399

upgle opened this issue Jun 7, 2021 · 2 comments
Assignees
Labels
bug Something is not working.

Comments

@upgle
Copy link

upgle commented Jun 7, 2021

Describe the bug

I am using a MySQL cluster managed by DBA, and migrating the DB manually.
I've discovered a potential issue in the code that could lead to one unintended DB migration.

I think Kratos and Ory ecosystem should provide an option to turn off this auto migration.

net, err := p.DetermineNetwork(ctx)

func (p *Persister) DetermineNetwork(ctx context.Context) (*networkx.Network, error) {

In this code, migration is performed whenever the application determines the network.
If multiple instances are deployed, migration will be attempted on all instances.

func (p *Persister) DetermineNetwork(ctx context.Context) (*networkx.Network, error) {
        // migration is performed here
	if err := p.p.MigrateUp(ctx); err != nil {
		return nil, err
	}
	return p.p.Determine(ctx)
}

Expected behavior

Provide an option to disable it.

@aeneasr aeneasr self-assigned this Jun 7, 2021
@aeneasr aeneasr added the bug Something is not working. label Jun 7, 2021
@aeneasr
Copy link
Member

aeneasr commented Jun 7, 2021

Thank you for the report! This migrates a table needed by the other schemas but it probably should indeed be manual anyways!

@upgle
Copy link
Author

upgle commented Jun 7, 2021

@aeneasr
Thank you for your fast feedback 👍

I think the kratos-migrate is a good tool to do this job.
https://www.ory.sh/kratos/docs/cli/kratos-migrate/

And if user use the in-memory type, kratos can migrate it on every start.

// if dsn is memory we have to run the migrations on every start
if dbal.IsMemorySQLite(m.Config(ctx).DSN()) || m.Config(ctx).DSN() == dbal.SQLiteInMemory || m.Config(ctx).DSN() == dbal.SQLiteSharedInMemory || m.Config(ctx).DSN() == "memory" {
	m.Logger().Infoln("Ory Kratos is running migrations on every startup as DSN is memory. This means your data is lost when Kratos terminates.")
	if err := p.MigrateUp(ctx); err != nil {
		m.Logger().WithError(err).Warnf("Unable to run migrations, retrying.")
		return err
	}
}

aeneasr added a commit that referenced this issue Jun 15, 2021
aeneasr added a commit that referenced this issue Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

No branches or pull requests

2 participants