Possibility of disabling audit log functionality per landscape #367
Conversation
kyma-bot
added
cla: yes
Disper
added
the
do-not-merge/work-in-progress
|
/hold |
kyma-bot
added
the
do-not-merge/hold
Disper
removed
the
do-not-merge/work-in-progress
mvshao
force-pushed
the
auditlog-strict
branch
from
1f22c26
to
1129976
Compare
mvshao
removed
the
do-not-merge/hold
| ) | ||
| if err != nil { //nolint:nestif | ||
| errorMessage := err.Error() | ||
| if strings.Contains(errorMessage, "auditlog config for region") { |
There was a problem hiding this comment.
Could you extract the This line relies on the content of the error message and if someone would change the "auditlog config for region" string to a const in the auditlogging.go and use the same const here?auditlogging.go then it would break this business logic (unit tests are not detecting this scenario).
Here's another approach on how it should be achieved
| @@ -90,6 +93,7 @@ func main() { | |||
| flag.BoolVar(&enableRuntimeReconciler, "runtime-reconciler-enabled", defaultRuntimeReconcilerEnabled, "Feature flag for all runtime reconciler functionalities") | |||
| flag.StringVar(&converterConfigFilepath, "converter-config-filepath", "/converter-config/converter_config.json", "A file path to the gardener shoot converter configuration.") | |||
| flag.BoolVar(&shootSpecDumpEnabled, "shoot-spec-dump-enabled", false, "Feature flag to allow persisting specs of created shoots") | |||
| flag.BoolVar(&auditLogMandatory, "audit-log-mandatory", true, "Feature flag to enable strict mode for audit log configuration") | |||
There was a problem hiding this comment.
Please document this new paramenter (link)
| errorMessage, | ||
| ) | ||
| } else { | ||
| m.log.Info(errorMessage, "Failed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured") |
There was a problem hiding this comment.
A tiny rewording proposal to make it more clear that it's a acceptable behavior. Feel free to ignore this suggestion if you don't agree.
| m.log.Info(errorMessage, "Failed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured") | |
| m.log.Info(errorMessage, "Audit Log was not configured, missing region mapping for this shoot. Continuing without error because flag `audit-log-mandatory` is disabled.") |
| "False", | ||
| errorMessage) | ||
| } else { | ||
| m.log.Info(errorMessage, "Failed to configure Audit Log, but is not mandatory to be configured") |
There was a problem hiding this comment.
During verification I've encountered a panic:
2024-09-09T15:29:14+02:00 INFO Observed a panic in reconciler: odd number of arguments passed as key-value pairs for logging {"controller": "runtime", "controllerGroup": "infrastructuremanager.kyma-project.io", "controllerKind": "Runtime", "Runtime": {"name":"kim-md-al6","namespace":"kcp-system"}, "namespace": "kcp-system", "name": "kim-md-al6", "reconcileID": "1aa26a9a-cc11-4cd7-bdf7-b6b022203af0"}
panic: odd number of arguments passed as key-value pairs for logging [recovered]
panic: odd number of arguments passed as key-value pairs for logging
Quoting logger api docs:
The key/value pairs must alternate string keys and arbitrary// values.
I wonder if just m.log.Info(errorMessage) wouldn't be enough here?
There was a problem hiding this comment.
I need to check code. In my opinion this additional info need to be present to clearly describe that Audit Log was set as not mandatory.
There was a problem hiding this comment.
It's possible to add either one parameter (errorMessage) or 3/5/7 etc with additional ones for key name and the value . (always uneven).
When I executed this part I noticed the Error during enabling Audit Logs on shoot: kim-md-al8: missing mapping for selected region in provider config was the errorMessage and passed as a string wasFailed to configure Audit Log, missing region mapping for this shoot, but is not mandatory to be configured . which indeed contains an additional information that's mandatory.
So how about such invocation:
m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory)?
The log would look like this
2024-09-10T07:31:51+02:00 INFO reqID 1 Error during enabling Audit Logs on shoot: kim-md-al8: missing mapping for selected region in provider config {"AuditLogMandatory": false}
There was a problem hiding this comment.
Logging additional information on region and provider type could make it easier to quickly fix such issues. m.log.Info(errorMessage, "AuditLogMandatory", m.RCCfg.AuditLogMandatory, "providerType", s.shoot.Spec.Provider.Type, "region", s.shoot.Spec.Region)
Description
Changes proposed in this pull request:
Related issue(s)
#286