Anonymize the host in case of HTTP failure (RabbitMQ Scaler) #2041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: jorturfer <[email protected]>
Signed-off-by: jorturfer <[email protected]>
coderanger
reviewed
Aug 19, 2021
pkg/scalers/rabbitmq_scaler.go
Outdated
| // Mask host for log porpouses | ||
| func (s *rabbitMQScaler) anonimizeRabbitMQError(err error) error { | ||
| errorMessage := fmt.Sprintf("error inspecting rabbitMQ: %s", err) | ||
| m1 := regexp.MustCompile(`([^ \/:]+):([^\/:]+)\@`) |
There was a problem hiding this comment.
You can move this out into a package level variable + init()
There was a problem hiding this comment.
which exactly are you talking about? The regex pattern?
There was a problem hiding this comment.
Yes:
var anonymizePattern *regexp.Regexp
func init() {
anonymizePattern = regex.MustCompile(...)
}That means it only compiles the pattern once at startup.
There was a problem hiding this comment.
Nice improvement!!!
I thought that golang cache several regex like other languages do (but I could do it wrong too, I'm learning golang right now)
I will update this with your suggestion asap :)
There was a problem hiding this comment.
Ahh, that explains why you're using a regex :) You could also do this with net/url Parse since it's a URI.
There was a problem hiding this comment.
oh, I got it! you are proposing to use something like this, right?
parsedURL, _ := url.Parse(s.metadata.host)
errorMessage := strings.ReplaceAllString(fmt.Sprintf("error inspecting rabbitMQ: %s", err),parsedURL.User.Password, "xxxx")Don't take care about the exact code, I did manually in Github and surely it wrongs, but as idea
There was a problem hiding this comment.
What you have is probably better, just do the static precompile of the pattern and I think that's okay give or take if we want to ignore the username.
There was a problem hiding this comment.
Take a look when you can please :)
I have applied your proposal :)
I'm not total sure if we can avoid it globally or we need to fix it scaler by scaler, I mean, each scaler has its own method to uses the password, in this specific case it inside the host variable, but others could have a password field |
I mean upstream as in the RabbitMQ library. |
tomkerkhove
reviewed
Aug 20, 2021
Signed-off-by: jorturfer <[email protected]>
zroubalik
reviewed
Aug 23, 2021
pkg/scalers/rabbitmq_scaler.go
Outdated
| @@ -21,6 +21,12 @@ import ( | |||
| kedautil "github.com/kedacore/keda/v2/pkg/util" | |||
| ) | |||
|
|
|||
| var anonymizePattern *regexp.Regexp | |||
There was a problem hiding this comment.
Suggested change
| var anonymizePattern *regexp.Regexp | |
| var rabbitMQAnonymizePattern *regexp.Regexp |
Nit, since this is set on a package level.
Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: Jorge Turrado <[email protected]>
arschles
suggested changes
Aug 26, 2021
pkg/scalers/rabbitmq_scaler.go
Outdated
| @@ -498,3 +505,9 @@ func getMaximum(q []queueInfo) (int, float64) { | |||
| } | |||
| return maxMessages, maxRate | |||
| } | |||
|
|
|||
| // Mask host for log porpouses | |||
There was a problem hiding this comment.
Suggested change
| // Mask host for log porpouses | |
| // Mask host for log purposes |
pkg/scalers/rabbitmq_scaler_test.go
Outdated
Comment on lines
417
to
418
| meta, _ := parseRabbitMQMetadata(&ScalerConfig{ResolvedEnv: sampleRabbitMqResolvedEnv, TriggerMetadata: metadata, AuthParams: nil}) | ||
|
|
There was a problem hiding this comment.
Suggested change
| meta, _ := parseRabbitMQMetadata(&ScalerConfig{ResolvedEnv: sampleRabbitMqResolvedEnv, TriggerMetadata: metadata, AuthParams: nil}) | |
| meta, err := parseRabbitMQMetadata(&ScalerConfig{ResolvedEnv: sampleRabbitMqResolvedEnv, TriggerMetadata: metadata, AuthParams: nil}) | |
| if err != nil { | |
| t.Fatalf("Error parsing metadata (%s)", err) | |
| } |
@JorTurFer this is not using assert.NoError(...) because I'm assuming the test should fail immediately on error.
There was a problem hiding this comment.
The test shouldn't fail there never because this part is totally outside from this specific test's scope. That's why the data should be always valid, but I agree, check it is free and can be used in case of failure :)
Co-authored-by: Aaron Schlesinger <[email protected]> Signed-off-by: jorturfer <[email protected]>
arschles
approved these changes
Aug 26, 2021
|
I'm checking this error (maybe it's an error during any merge because I didn't change anything related with it) |
|
@JorTurFer odd that the stack trace doesn't include any test code. I haven't tried running this branch locally, so as a pure guess, you could try setting the httpClient in the test rabbitMQ client to something not nil (e.g. the |
|
I'm using dev containers so maybe anything is missing in it :( |
|
it looks like there are more problems than just a nil ptr panic in that run - it's trying to start up a local cluster from scratch and can't start etcd. you could try installing that, or more likely installing all of kubebuilder first and then re-running maybe |
|
I'm trying again with kubebuilder installed, :) Thanks for the tip |
|
So it seems that rerunning it passed. I was pretty confused by the panic too, but not sure why it happened. |
|
that's a weird flake. glad the re-run passed @ahmelsayed |
nilayasiktoprak
pushed a commit
to nilayasiktoprak/keda
that referenced
this pull request
Oct 23, 2021
…e#2041) * Anonimize the host in case of HTTP failure Signed-off-by: jorturfer <[email protected]> * Update CHANGELOG and add one extra test case Signed-off-by: jorturfer <[email protected]> * Move the regex from the method to a static var Signed-off-by: jorturfer <[email protected]> * Update regex var name Co-authored-by: Zbynek Roubalik <[email protected]> Signed-off-by: Jorge Turrado <[email protected]> * Fix typo and improve test Co-authored-by: Aaron Schlesinger <[email protected]> Signed-off-by: jorturfer <[email protected]> Co-authored-by: Ahmed ElSayed <[email protected]> Signed-off-by: nilayasiktoprak <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: jorturfer [email protected]
This PR replaces the user and the password before log the host in case of failure (RabbitMQ Scaler) in KEDA metrics server
Checklist
Fixes #2040