Skip to content

Conversation

kogent
Copy link

@kogent kogent commented Aug 5, 2025

The use of fmt.Sprintf to build the query url doesn't handle all the query cases possible with prometheus and breaks for serverAddress configs with trailing slashes. This PR uses net/url library functions to build the query string safely.

The tests added failed prior to the change proposed and pass after. All existing tests pass.

A future PR would ideally also support POST for sending queries, but this is a good first step.

Checklist

Fixes #

Relates to #

Copy link

github-actions bot commented Aug 5, 2025

Thank you for your contribution! 🙏

Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected.

While you are waiting, make sure to:

  • Add an entry in our changelog in alphabetical order and link related issue
  • Update the documentation, if needed
  • Add unit & e2e tests for your changes
  • GitHub checks are passing
  • Is the DCO check failing? Here is how you can fix DCO issues

Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient.

Learn more about our contribution guide.

logger: logr.Discard(),
}

value, err := scaler.ExecutePromQuery(context.TODO())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:
Consider to use well-defined context

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by context-todo.

You can view more details about this finding in the Semgrep AppSec Platform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ar acceptable in test

logger: logr.Discard(),
}

value, err := scaler.ExecutePromQuery(context.TODO())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semgrep identified an issue in your code:
Consider to use well-defined context

To resolve this comment:

🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.

💬 Ignore this finding

Reply with Semgrep commands to ignore this finding.

  • /fp <comment> for false positive
  • /ar <comment> for acceptable risk
  • /other <comment> for all other reasons

Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by context-todo.

You can view more details about this finding in the Semgrep AppSec Platform.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ar acceptable in test

@zroubalik zroubalik requested a review from Copilot August 19, 2025 09:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the handling of query parameters and URL construction in the Prometheus scaler by replacing manual string formatting with the net/url library for safer URL construction. This change prevents issues when server addresses contain trailing slashes and ensures proper escaping of special characters in query parameters.

  • Replaces fmt.Sprintf URL construction with net/url package functions for safer parameter handling
  • Uses url.Values to properly encode query parameters instead of manual escaping
  • Handles trailing slash behavior consistently across different server address configurations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/scalers/prometheus_scaler.go Updates ExecutePromQuery to use net/url for URL construction and parameter encoding
pkg/scalers/prometheus_scaler_test.go Adds comprehensive tests for special characters and trailing slash handling scenarios

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@zroubalik
Copy link
Member

zroubalik commented Aug 26, 2025

/run-e2e prometheus
Update: You can check the progress here

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kogent could you please fix the DCO?

Also please update changelog (Other section)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants