hydra
hydra copied to clipboard
ory
Ignore unknown columns in SQL during upgrades
Preflight checklist
- [X] I could not find a solution in the existing issues, docs, nor discussions.
- [X] I agree to follow this project's Code of Conduct.
- [X] I have read and am following this repository's Contribution Guidelines.
- [ ] This issue affects my Ory Cloud project.
- [ ] I have joined the Ory Community Slack.
- [ ] I am signed up to the Ory Security Patch Newsletter.
Describe the bug
I have ORY Hydra 1.10.7 in production. I'm trying to upgrade it to 1.11.0 with blue-green deployment, so that both versions work at the same time for some time.
I've realized that after 1.11.0 is up, logout requests that are handled by 1.10.7 cause an error
missing destination name registration_access_token_signature in *[]client.Client
And the browser redirecting to
/error=error&error_description=The+error+is+unrecognizable+missing+destination+name+registration_access_token_signature+in+%2A%5B%5Dclient.Client
It seems that is because the orm fails to read a database record when an unknown column is found.
When https://github.com/ory/hydra/blob/master/persistence/sql/migrations/20211226156000000000_dynamic_registration.up.sql added a column in 1.11.0, the 1.10.7 could not read the new column when it came in response from database.
Reproducing the bug
Steps to reproduce:
- run two Hydra versions: 1.10.7 and 1.11.0 connected to the same database, emulating blue-green deployment.
- run GET /oauth2/sessions/logout for logging out
Relevant log output
missing destination name registration_access_token_signature in *[]client.Client
Relevant configuration
No response
Version
1.10.7, 1.11.0
On which operating system are you observing this issue?
Linux
In which environment are you deploying?
Kubernetes
Additional Context
No response
Thank you for the report and sorry for the trouble. We do not support online migrations at the moment. Go tooling around this still has to catch up. As we're building out our cloud service, this will change though over the next years.
@aeneasr thanks for quick answer! Is it possible at least to adjust the orm to ignore unknown fields? Or to fetch only supported fields, but not all, so that unknown ones are not received?
We might explore that in the future, and while it will solve a couple of edge cases it will not solve the general issue at hand. Due to limited resources, we also won't backport any of these changes to older versions, sorry!
@aeneasr I see, I don't expect this to be backported, but I'm really concerned that such a popular and reliable tool as Hydra does not meet such a default thing as ignoring unknown database columns.
Can the ticket be kept as open please, probably as a candidate for contribution?
I've found that github.com/jmoiron/sqlx package is used for database rows to structures mapping.
There's https://pkg.go.dev/github.com/jmoiron/sqlx#DB.Unsafe flag that will make the DB silently succeed to scan when columns in the SQL result have no fields in the destination struct.
I'm going to introduce db.ignore_unknown_table_columns configuration key of bool type in order to pass it to the db connection factory method, so that the feature could be enabled or disabled (disabled by default)
The only thing I'm not sure about, whether this is a bugfixing or a new feature since a configuration option is added
Maybe it could be useful changing the point of view. In my code, I've written a function that with reflection extracts from the query results only fields present in a go structure. My idea is that the code knows what fields it needs, and reads only them. This way, you can add whatever fields to the query, they won't create issues. When you need the new fields, just add them to the go struct receiving them. I am posting the function here, I don't know if it can be useful. It is built upon sql.Nullfields, to manage null values, but it could be changed to managing native types with a little effort. This is just a proof of concept. If needed I could write a version for native types, and with standard error management
//FillStructFromDB reads from rows returned from go sql package and fills fields in a struct
//It manages tags to remap struct fields into database fields, and to manage optional fields
func FillstructFromDB(outStruct interface{}, inStruct map[string]interface{}, boolsAreInt bool) *rerrors.RichErr {
var ok bool
var rerr = rerrors.NewRichErr(rerrors.ErrorNotInitialized)
var tagName string = "db"
// outStruct is the model we need to fill
s := reflect.ValueOf(outStruct).Elem()
typeOfoutStruct := s.Type()
// cycling on all fields of the struct
for i := 0; i < s.NumField(); i++ {
//get a reference to the i-th field
f := s.Field(i)
// get field type as string
fieldType := f.Type().String()
fieldType = fieldType[strings.LastIndex(fieldType, ".")+1:]
fieldName := strings.ToLower(typeOfoutStruct.Field(i).Name)
tag := strings.ToLower(typeOfoutStruct.Field(i).Tag.Get(tagName))
optional := false
if len(tag) > 0 {
fields := strings.Split(tag, ",")
fieldName = fields[0]
if len(fields) > 1 && strings.ToLower(fields[1]) == "optional" {
optional = true
}
}
switch fieldType {
case "NullString":
// we can't use SetInt64 or SetString because it'a a custom type
// so we get a safe pointer to the NullInt64, or NullString struct
// and we fill it
var tmp *NullString = f.Addr().Interface().(*NullString)
var inData NullString
if inData, ok = inStruct[fieldName].(NullString); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.String = inData.String
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
case "NullInt64":
var tmp *NullInt64 = f.Addr().Interface().(*NullInt64)
var inData NullInt64
if inData, ok = inStruct[fieldName].(NullInt64); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Int64 = inData.Int64
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
case "NullFloat64":
var tmp *NullFloat64 = f.Addr().Interface().(*NullFloat64)
var inData NullFloat64
if inData, ok = inStruct[fieldName].(NullFloat64); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Float64 = inData.Float64
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
case "NullBitBool":
var tmp *NullBitBool = f.Addr().Interface().(*NullBitBool)
var inData NullBitBool
if inData, ok = inStruct[fieldName].(NullBitBool); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Bool = inData.Bool
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
case "NullBool":
var tmp *NullBool = f.Addr().Interface().(*NullBool)
if boolsAreInt {
var inData NullInt64
if inData, ok = inStruct[fieldName].(NullInt64); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Bool = (inData.Int64 != 0)
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
} else {
var inData NullBool
if inData, ok = inStruct[fieldName].(NullBool); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Bool = inData.Bool
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
}
case "NullTime":
var tmp *NullTime = f.Addr().Interface().(*NullTime)
var inData NullTime
if inData, ok = inStruct[fieldName].(NullTime); !ok && !optional {
rerr.SetError(rerrors.ErrorFieldNotFoundInDBSet)
rerr.SetDescription("Fieldname: " + fieldName)
return rerr
}
if ok {
tmp.Time = inData.Time
tmp.Valid = inData.Valid
} else {
tmp.Valid = false
}
//f.SetInt(33)
}
}
return rerr
}
Thanks @alfmos but I'm not sure if that's better than using a default sqlx feature: https://github.com/ory/hydra/pull/3193/files#r922650537
Thanks @alfmos but I'm not sure if that's better than using a default sqlx feature: https://github.com/ory/hydra/pull/3193/files#r922650537
Sorry I had misunderstood what you wrote in your post
@alfmos sorry, the link is broken, here's another one: https://github.com/ory/hydra/pull/3193/files#diff-616670627adbf89ef1e3de0f0b9439c16dda54a28d3056e740bde52601356e15R118 There's a https://pkg.go.dev/github.com/jmoiron/sqlx#DB.Unsafe feature that stands effectively for this very case, when a database table has a column that is not mapped into a structure.