[RHPAM-4438] Sensitive Information In ENV And Pod Logs #99
Conversation
davidesalerno
force-pushed
the
RHPAM-4438
branch
from
238c990
to
ef014c4
Compare
|
@tchughesiv Can you help me reviewing this change? |
go.mod
Outdated
| @@ -14,7 +14,7 @@ require ( | |||
| github.com/pkg/errors v0.9.1 | |||
| github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.50.0 | |||
| github.com/stretchr/testify v1.7.0 | |||
| go.uber.org/zap v1.19.0 | |||
| go.uber.org/zap v1.21.0 | |||
There was a problem hiding this comment.
@ruromero This change is not strictly needed for this fix and it is due to the attempt to add some unit test using zaptest (go.uber.org/zap/zaptest) which would require huge refactoring and for this reason at the moment they are not in this PR.
I'll revert it.
pkg/resource/compare/defaults.go
Outdated
| logger.Info("Resources are not equal", "deployed", deployed, "requested", requested) | ||
| logger.Debugf("Resources are not equal", "deployed", deployed, "requested", requested) |
There was a problem hiding this comment.
For case when resources or objects are not equal. Can we keep there at least some info output. Otherwise it might be hard for users to spot what is wrong.
If the debug output is not allowed, it would be good to print some info log (or warning) as well. So it can be debug easier. I think it should be okay to just print message like "Resources are not equal. For more details use debug option for your Operator."
There was a problem hiding this comment.
@jakubschwan thank you for the suggestion, I agree with your concern and I'll do the change
davidesalerno
force-pushed
the
RHPAM-4438
branch
3 times, most recently
from
d519769
to
61525e5
Compare
There was a problem hiding this comment.
I'd rather use a conditional logging instead of logging twice. Something like this:
if logs.IsDebugLevel() {
logger.Debugf("Resources are not equal", "deployed", deployed, "requested", requested)
} else {
logger.Info("Resources are not equal. For more details set the Operator log level to DEBUG.")
}
I think this function should be enough:
// IsDebugLevel returns whether the current log level is Debug
func IsDebugLevel() bool {
return GetBoolEnv(DebugTrue.Name)
}
pkg/resource/compare/defaults.go
Outdated
| @@ -643,7 +649,8 @@ func EqualPairs(objects [][2]interface{}) bool { | |||
| func Equals(deployed interface{}, requested interface{}) bool { | |||
| equal := reflect.DeepEqual(deployed, requested) | |||
| if !equal { | |||
| logger.Info("Objects are not equal", "deployed", deployed, "requested", requested) | |||
| logger.Info("Objects are not equal. For more details set the Operato log level at DEBUG.") | |||
There was a problem hiding this comment.
| logger.Info("Objects are not equal. For more details set the Operato log level at DEBUG.") | |
| logger.Info("Objects are not equal. For more details set the Operator log level to DEBUG.") |
pkg/resource/compare/defaults.go
Outdated
| @@ -615,7 +620,8 @@ func equalBuildConfigs(deployed client.Object, requested client.Object) bool { | |||
| pairs = append(pairs, [2]interface{}{bc1.Spec, bc2.Spec}) | |||
| equal := EqualPairs(pairs) | |||
| if !equal { | |||
| logger.Info("Resources are not equal", "deployed", deployed, "requested", requested) | |||
| logger.Info("Resources are not equal. For more details set the Operato log level at DEBUG.") | |||
There was a problem hiding this comment.
| logger.Info("Resources are not equal. For more details set the Operato log level at DEBUG.") | |
| logger.Info("Resources are not equal. For more details set the Operator log level to DEBUG.") |
pkg/resource/compare/defaults.go
Outdated
| @@ -542,7 +546,8 @@ func equalSecrets(deployed client.Object, requested client.Object) bool { | |||
| pairs = append(pairs, [2]interface{}{secret1.Data, secret2.Data}) | |||
| equal := EqualPairs(pairs) | |||
| if !equal { | |||
| logger.Info("Resources are not equal", "deployed", deployed, "requested", requested) | |||
| logger.Info("Resources are not equal. For more details set the Operato log level at DEBUG.") | |||
There was a problem hiding this comment.
| logger.Info("Resources are not equal. For more details set the Operato log level at DEBUG.") | |
| logger.Info("Resources are not equal. For more details set the Operator log level to DEBUG.") |
Signed-off-by: Davide Salerno <dsalerno@redhat.com>
davidesalerno
force-pushed
the
RHPAM-4438
branch
from
61525e5
to
7a9086d
Compare
|
@ruromero Thanks for the good advices, I integrated it and it is much better than before |
|
@davidesalerno @ruromero this change is causing #110. I created the PR #111 to fix #110 partially reverting [RHPAM-4438] Sensitive Information In ENV And Pod Log. Could you review it? |
Signed-off-by: Davide Salerno dsalerno@redhat.com
See: RHPAM-4438
In order to avoid to print sensitive information in the logs in a production environment, it is better to log certain type of information only when the logger is not at DEBUG level (usually in a Prod env log level is at Info)