User-configurable overrides: add optional check to block API requests that conflict with runtime overrides #3180
Conversation
kvrhdn
force-pushed
the
kvrhdn/overrides-api-block-if-set
branch
from
bad37f0
to
880ed9c
Compare
… that conflict with runtime overrides
kvrhdn
force-pushed
the
kvrhdn/overrides-api-block-if-set
branch
from
880ed9c
to
2520b4f
Compare
kvrhdn
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
|
|
||
| MetricsGenerator *LimitsMetricsGenerator `json:"metrics_generator,omitempty"` | ||
| MetricsGenerator LimitsMetricsGenerator `yaml:"metrics_generator,omitempty" json:"metrics_generator,omitempty"` |
There was a problem hiding this comment.
What's the change here by becoming a non-pointer on the struct? I'm not sure I follow.
There was a problem hiding this comment.
I did this change to simplify comparing structs. If each child-struct is a pointer it becomes nearly impossible to compare two empty structs as the pointer values will never match.
For example, the following two structs are both empty/unset:
Limits {
MetricsGenerator: &LimitsMetricsGenerator{
DisableCollection: nil,
},
}Limits {
MetricsGenerator: nil,
}But they are not equal. Having every child-struct behind a pointer creates so many variations of 'empty'.
Making them all values instead of pointers avoids this situation + it doesn't change parsing (I added a test to check).
# Conflicts: # modules/overrides/runtime_config_overrides.go
modules/overrides/interface.go
Outdated
| @@ -18,6 +18,8 @@ type Service interface { | |||
| Interface | |||
| } | |||
|
|
|||
| type Level string | |||
There was a problem hiding this comment.
I didn't see this used, did I miss it?
There was a problem hiding this comment.
It was used in a previous iteration of the API, nice catch!
| @@ -159,6 +181,10 @@ func writeError(w http.ResponseWriter, err error) { | |||
| http.Error(w, err.Error(), http.StatusBadRequest) | |||
| return | |||
| } | |||
| if ok := errors.Is(err, errConflictingRuntimeOverrides); ok { | |||
There was a problem hiding this comment.
Does this allocate ok or is the compiler smart enough to skip it?
There was a problem hiding this comment.
Huh, I'm not entirely sure why I created a temporary variable here 😅 I probably used a different function before that returned 2 values instead of juts a boolean 🤷🏻
Again nice catch! Surprised the linter didn't pick this up
| func TestOverrides_AssertUserConfigurableOverridesAreASubsetOfRuntimeOverrides(t *testing.T) { | ||
| userConfigurableOverrides := client.Limits{} | ||
|
|
||
| err := gofakeit.Struct(&userConfigurableOverrides) |
What this PR does:
Adds an optional check that refuses setting user-configurable overrides if the tenant already has conflicting settings in runtime overrides. This is a crude check, ti doesn't check field-by-field but instead checks if any of the possible overrides are set in runtime overrides.
This check can be enabled with:
Callers can skip/bypass it by adding the query paramter
?skip-conflicting-runtime-check=true.Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]