OCPBUGS-22739: Properly handle rewrite-target annotation #534

gcs278 · 2023-11-03T17:58:30Z

Process the haproxy.router.openshift.io/rewrite-target annotation value to prevent HaProxy configuration issues while maintaining API compatibility. Properly handle values with spaces, backslashes, and other special characters.

Add a new package, rewritetarget, to organize the functions that process the rewrite target annotation. Add a separate unit test package, rewritetarget_test, to enforce black-box testing principles.

images/router/haproxy/conf/haproxy-config.template: Call processRewriteTarget function in template.
pkg/router/template/rewritetarget/rewritetarget.go: New package with functions to process the rewrite target annotation.
pkg/router/template/rewritetarget/rewritetarget_test.go: Unit test SanitizeInput function.
pkg/router/template/template_helper.go: Expose SanitizeInput to as a Go Template function.

openshift-ci-robot · 2023-11-03T17:58:35Z

@gcs278: This pull request references Jira Issue OCPBUGS-22739, which is invalid:

expected the bug to target the "4.15.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Handle the haproxy.router.openshift.io/rewrite-target annotation value for API compatibility while properly handling values with spaces, backslashes, and other special characters.

images/router/haproxy/conf/haproxy-config.template: Call processRewriteTarget function in template.
pkg/router/template/template_helper.go: Add processRewriteTarget function.
pkg/router/template/template_helper_test.go: Unit test new precessRewriteTarget function.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

frobware · 2023-11-03T18:40:34Z

pkg/router/template/template_helper.go

+	// leave double %% as is, so that existing users who manually addressed the issue are not broken. Furthermore, even
+	// sequences, such as %%%% or %%%%%% were valid and should be left untouched, while odd sequences such as %%% or
+	// %%%%% were invalid, and should be doubled to ensure every % is escaped.
+	re := regexp.MustCompile(`%+`)


If this function will be called frequently then we should be somewhat wary of the time and cost to compile the RE. See: #268

Nice catch. Might as well optimize it. Done.

Using cachedRegexpCompile makes sense where the pattern is built at run-time, but in this case, you can just use a package variable to avoid the map lookup and error checking:

var ( // oneOrMorePercentSigns matches one or more literal percent signs. oneOrMorePercentSigns = regexp.MustCompile(`%+`) )

Oh I see the intention of cachedRegexpComplie now. Done.

candita · 2023-11-08T18:41:09Z

/assign @Miciah
/assign @frobware

frobware · 2023-11-10T17:19:23Z

The changes LGTM but I think somebody else should also review otherwise I'd just be rubber-stamping my own suggestions.

gcs278 · 2023-11-13T16:23:41Z

/jira refresh

openshift-ci-robot · 2023-11-13T16:23:50Z

@gcs278: This pull request references Jira Issue OCPBUGS-22739, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gcs278 · 2023-11-13T16:25:01Z

Image build failed again.
/test e2e-agnostic

gcs278 · 2023-11-13T20:06:16Z

testplatform refreshed the RPM repo:
/test e2e-agnostic

ShudiLi · 2023-11-14T02:42:06Z

tested it with 4.15.0-0.ci.test-2023-11-14-012251-ci-ln-8wxgkwb-latest
`
1.
% oc get clusterversion
NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.15.0-0.ci.test-2023-11-14-012251-ci-ln-8wxgkwb-latest True False 38m Cluster version is 4.15.0-0.ci.test-2023-11-14-012251-ci-ln-8wxgkwb-latest

% oc -n openshift-ingress rsh router-default-9d6f95d4c-9tmc
sh-4.4$ grep "http-request replace-path" haproxy-config.template
http-request replace-path ^{{ $cfg.Path }}/?(.)$ '{{ processRewriteTarget $pathRewriteTarget }}'
http-request replace-path ^{{ $cfg.Path }}(.)$ '{{ processRewriteTarget $pathRewriteTarget }}'
sh-4.4$

% oc annotate route/service-unsecure haproxy.router.openshift.io/rewrite-target="/t t$%&*(){}[].,z#:`_-|" --overwrite
route.route.openshift.io/service-unsecure annotate

% oc get route service-unsecure -o=jsonpath="{.metadata.annotations.haproxy.router.openshift.io/rewrite-target}"
/t t$%&*(){}[].,z#:_-|

ShudiLi · 2023-11-14T02:42:52Z

/label qe-approved
thanks

openshift-ci-robot · 2023-11-14T02:42:58Z

@gcs278: This pull request references Jira Issue OCPBUGS-22739, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.15.0) matches configured target version for branch (4.15.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

Handle the haproxy.router.openshift.io/rewrite-target annotation value for API compatibility while properly handling values with spaces, backslashes, and other special characters.

images/router/haproxy/conf/haproxy-config.template: Call processRewriteTarget function in template.

pkg/router/template/rewritetarget/rewritetarget.go: New package with functions to process the rewrite target annotation.

pkg/router/template/rewritetarget/rewritetarget_test.go: Unit test SanitizeInput function.

pkg/router/template/template_helper.go: Expose SanitizeInput to as a Go Template function.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gcs278 · 2024-03-06T20:41:10Z

I've asked @alebedev87 to take a look to help move this PR along (since Andy has a conflict of interest)

/assign @alebedev87

alebedev87

Overall, looks fine to me, just some small comments and questions.

alebedev87 · 2024-03-08T14:20:28Z

pkg/router/template/util/rewritetarget/rewritetarget.go

+
+// processDoubleQuotes is a processFunc that processes double quote
+// characters. It skips the double quote if it's not escaped.
+var processDoubleQuotes = func(char rune, escaped bool) runeResult {


Any particular reason this is not just:

Suggested change

var processDoubleQuotes = func(char rune, escaped bool) runeResult {

func processDoubleQuotes(char rune, escaped bool) runeResult {

?

Yea I don't see any reason these need to be a var, I'll change.

alebedev87 · 2024-03-08T14:21:35Z

pkg/router/template/util/rewritetarget/rewritetarget.go

+// string. It returns a processFunc which will set
+// encounteredCommentMarker to true and stop processing if it
+// encounters a '#' that is not escaped.
+func processHashCreator(encounteredCommentMarker *bool) processFunc {


Suggested change

func processHashCreator(encounteredCommentMarker *bool) processFunc {

func newProcessHashCreator(encounteredCommentMarker *bool) processFunc {

wdyt?

Yea that's fair. Done.

alebedev87 · 2024-03-08T14:27:59Z

pkg/router/template/util/rewritetarget/rewritetarget_test.go

+package rewritetarget_test
+
+import (
+	"github.com/openshift/router/pkg/router/template/util/rewritetarget"


SanitizeInput is in the same package, no need to import it.

It's not the same package. This is a deliberate decision. Placing tests in a separate package, package rewritetarget_test, enforces black-box testing principles. For me, key reasons to use black-box testing:

Encapsulation: It guarantees tests can only access exported package members, mirroring how external packages interact with rewritetarget. This approach reinforces the integrity of the package's encapsulation.

API Design: Black-box testing inherently leads to clearer and more stable APIs by focusing on the package's usability rather than its internals.

Resilience: Tests are less prone to break due to internal changes, as they're tied to the public API. This helps to reduce maintenance.

User Perspective: It provides insights into how the package is used, highlighting potential areas for improvement from the end-user’s standpoint.

Ah, right, I was looking at the directories. So, what's the idea behind? To test without the access to any private variables? Like if it was the user of the package?

I see this pattern is not quite uniformly implemented in util package (e.g. haproxy).

Encapsulation: It guarantees tests can only access exported package members, mirroring how external packages interact with rewritetarget. This approach reinforces the integrity of the package's encapsulation. API Design: Black-box testing inherently leads to clearer and more stable APIs by focusing on the package's usability rather than its internals. Resilience: Tests are less prone to break due to internal changes, as they're tied to the public API. This helps to reduce maintenance. User Perspective: It provides insights into how the package is used, highlighting potential areas for improvement from the end-user’s standpoint.

I see. Yes, I think it makes sense for SanitizeInput function.

Right, Andy and I had this discussion in slack. It's an interesting perspective we should consider when writing unit tests.

pkg/router/template/util/rewritetarget/rewritetarget.go

alebedev87 · 2024-03-08T15:18:41Z

pkg/router/template/util/rewritetarget/rewritetarget_test.go

+		},
+		{
+			name:   "triple percent should be doubled entirely",
+			input:  `%%%foo`,


So here we think that the user put "3 percent signs without escaping" and not "1 escaped percent sign plus one unescaped", right?

Exactly. This case is currently a syntax error, which means we don't have to maintain any sort of compatibility. with users using %%% today.

The most logical thing to do here is assume the user isn't escaping any of them and double them (to escape them).

Another interpretation of %%% input could be as a combination of %% + % inputs. Therefore, by default, we assume the user's intent is to do the right escaping (like in case of %% input) and we do the minimum necessary to ensure the correct syntax(== %%%%).

Ah yes it's all coming back to me now. You are correct, we could assume mixed intention for escaped & unescaped, taking each % by itself. However, the doubling %%% to %%%%%% then gets interpreted back to %%% again, which is exactly what the author specified in the first place, keeping some consistency.

alebedev87 · 2024-03-08T15:19:36Z

pkg/router/template/util/rewritetarget/rewritetarget.go

+	val = convertDoubleBackslashes(val)
+
+	if !encounteredCommentMarker {
+		// Add the capture group.


May be a little more info of why we are adding the capture group.

// The literal `\1` is appended to annotations without // comments to comply with HAProxy rewrite rule // expectations, which necessitate a capture group // reference (i.e., `\1`) for dynamic substitutions.

Any better?

Done. Thanks Andy, you are making this too easy for me 😄

Process the `haproxy.router.openshift.io/rewrite-target` annotation value to prevent HaProxy configuration issues while maintaining API compatibility. Properly handle values with spaces, backslashes, and other special characters. Add a new package, rewritetarget, to organize the functions that process the rewrite target annotation. Add a separate unit test package, rewritetarget_test, to enforce black-box testing principles.

gcs278 · 2024-03-13T19:15:06Z

@alebedev87 @frobware Code review changes from Andrey's comments. I also revised the git commit message.

openshift-ci-robot · 2024-03-13T19:15:45Z

@gcs278: This pull request references Jira Issue OCPBUGS-22739, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @ShudiLi

In response to this:

Process the haproxy.router.openshift.io/rewrite-target annotation value to prevent HaProxy configuration issues while maintaining API compatibility. Properly handle values with spaces, backslashes, and other special characters.

Add a new package, rewritetarget, to organize the functions that process the rewrite target annotation. Add a separate unit test package, rewritetarget_test, to enforce black-box testing principles.

images/router/haproxy/conf/haproxy-config.template: Call processRewriteTarget function in template.

pkg/router/template/rewritetarget/rewritetarget.go: New package with functions to process the rewrite target annotation.

pkg/router/template/rewritetarget/rewritetarget_test.go: Unit test SanitizeInput function.

pkg/router/template/template_helper.go: Expose SanitizeInput to as a Go Template function.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

frobware · 2024-03-14T11:35:07Z

/lgtm

frobware · 2024-03-14T11:35:13Z

/retest

ShudiLi · 2024-03-14T13:03:16Z

tested it with 4.15.0-0.ci.test-2024-03-14-121356-ci-ln-1wy2cjb-latest

% oc get clusterversion
NAME VERSION AVAILABLE PROGRESSING SINCE STATUS
version 4.15.0-0.ci.test-2024-03-14-121356-ci-ln-1wy2cjb-latest True False 5m44s Cluster version is 4.15.0-0.ci.test-2024-03-14-121356-ci-ln-1wy2cjb-latest

% oc -n openshift-ingress rsh router-default-567f677657-7gr4q
sh-5.1$ grep "http-request replace-path" haproxy-config.template
http-request replace-path ^{{ $cfg.Path }}/?(.)$ '{{ processRewriteTarget $pathRewriteTarget }}'
http-request replace-path ^{{ $cfg.Path }}(.)$ '{{ processRewriteTarget $pathRewriteTarget }}'
sh-5.1$

% oc annotate route/service-unsecure haproxy.router.openshift.io/rewrite-target="/t t$%&*(){}[].,z#:`_-|" --overwrite
route.route.openshift.io/service-unsecure annotate

% oc get route service-unsecure -o=jsonpath="{.metadata.annotations.haproxy.router.openshift.io/rewrite-target}"
/t t$%&*(){}[].,z#:`_-|%

ShudiLi · 2024-03-14T13:03:53Z

/label qe-approved
thanks

openshift-ci · 2024-03-14T14:27:22Z

@gcs278: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

alebedev87 · 2024-03-14T21:37:53Z

/approve

openshift-ci · 2024-03-14T21:38:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alebedev87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gcs278 · 2024-03-14T21:55:31Z

/hold

I want to give @Miciah a chance to look at this, as he helped me with the solution. We are in acknowledge-critical-fixes-only anyways, so there isn't a rush until after shift week.

I'll unhold after shift week if I don't hear any issues.

gcs278 · 2024-03-25T17:59:36Z

Moving forward with this fix. I'm going to look into getting docs to help me with a doc update.

/unhold

openshift-ci-robot · 2024-03-25T18:01:59Z

@gcs278: Jira Issue OCPBUGS-22739: All pull requests linked via external trackers have merged:

openshift/router#534

Jira Issue OCPBUGS-22739 has been moved to the MODIFIED state.

In response to this:

Process the haproxy.router.openshift.io/rewrite-target annotation value to prevent HaProxy configuration issues while maintaining API compatibility. Properly handle values with spaces, backslashes, and other special characters.

Add a new package, rewritetarget, to organize the functions that process the rewrite target annotation. Add a separate unit test package, rewritetarget_test, to enforce black-box testing principles.

images/router/haproxy/conf/haproxy-config.template: Call processRewriteTarget function in template.

pkg/router/template/rewritetarget/rewritetarget.go: New package with functions to process the rewrite target annotation.

pkg/router/template/rewritetarget/rewritetarget_test.go: Unit test SanitizeInput function.

pkg/router/template/template_helper.go: Expose SanitizeInput to as a Go Template function.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-03-25T22:44:12Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-haproxy-router-base-container-v4.16.0-202403251813.p0.g526f832.assembly.stream.el9 for distgit ose-haproxy-router-base.
All builds following this will include this PR.

gcs278 · 2024-03-29T14:05:35Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-03-29T14:06:18Z

@gcs278: new pull request created: #569

In response to this:

/cherry-pick release-4.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gcs278 · 2024-04-18T15:04:38Z

/cherry-pick release-4.14

openshift-cherrypick-robot · 2024-04-18T15:05:19Z

@gcs278: #534 failed to apply on top of branch "release-4.14":

Applying: OCPBUGS-22739: Properly handle rewrite-target annotation
Using index info to reconstruct a base tree...
M	pkg/router/template/template_helper.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/router/template/template_helper.go
CONFLICT (content): Merge conflict in pkg/router/template/template_helper.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 OCPBUGS-22739: Properly handle rewrite-target annotation
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot requested review from alebedev87 and knobunc November 3, 2023 17:59

frobware reviewed Nov 3, 2023

View reviewed changes

gcs278 force-pushed the OCPBUGS-22739-rewrite-percent branch from 5e9d232 to 0a5016f Compare November 6, 2023 18:38

openshift-ci bot assigned frobware and Miciah Nov 8, 2023

gcs278 force-pushed the OCPBUGS-22739-rewrite-percent branch 7 times, most recently from 8651281 to 4933f71 Compare November 10, 2023 17:06

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Nov 13, 2023

openshift-ci bot requested a review from ShudiLi November 13, 2023 16:23

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Nov 14, 2023

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 21, 2024

openshift-ci bot assigned alebedev87 Mar 6, 2024

alebedev87 reviewed Mar 8, 2024

View reviewed changes

gcs278 force-pushed the OCPBUGS-22739-rewrite-percent branch from 4933f71 to e298bb0 Compare March 13, 2024 19:14

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2024

openshift-merge-bot bot merged commit 526f832 into openshift:master Mar 25, 2024
8 checks passed

openshift-cherrypick-robot mentioned this pull request Mar 29, 2024

[release-4.15] OCPBUGS-31544: Properly handle rewrite-target annotation #569

Merged

gcs278 mentioned this pull request Apr 22, 2024

[release-4.14] OCPBUGS-32634: Properly handle rewrite-target annotation #583

Open

	var processDoubleQuotes = func(char rune, escaped bool) runeResult {
	func processDoubleQuotes(char rune, escaped bool) runeResult {

	func processHashCreator(encounteredCommentMarker *bool) processFunc {
	func newProcessHashCreator(encounteredCommentMarker *bool) processFunc {

OCPBUGS-22739: Properly handle rewrite-target annotation #534

OCPBUGS-22739: Properly handle rewrite-target annotation #534

Conversation

gcs278 commented Nov 3, 2023 • edited

openshift-ci-robot commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita commented Nov 8, 2023

frobware commented Nov 10, 2023

gcs278 commented Nov 13, 2023

openshift-ci-robot commented Nov 13, 2023

gcs278 commented Nov 13, 2023

gcs278 commented Nov 13, 2023

ShudiLi commented Nov 14, 2023

ShudiLi commented Nov 14, 2023

openshift-ci-robot commented Nov 14, 2023

gcs278 commented Mar 6, 2024

alebedev87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frobware Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcs278 commented Mar 13, 2024

openshift-ci-robot commented Mar 13, 2024

frobware commented Mar 14, 2024

frobware commented Mar 14, 2024

ShudiLi commented Mar 14, 2024

ShudiLi commented Mar 14, 2024

openshift-ci bot commented Mar 14, 2024

alebedev87 commented Mar 14, 2024

openshift-ci bot commented Mar 14, 2024

gcs278 commented Mar 14, 2024

gcs278 commented Mar 25, 2024

openshift-ci-robot commented Mar 25, 2024

openshift-bot commented Mar 25, 2024

gcs278 commented Mar 29, 2024

openshift-cherrypick-robot commented Mar 29, 2024

gcs278 commented Apr 18, 2024

openshift-cherrypick-robot commented Apr 18, 2024

gcs278 commented Nov 3, 2023 •

edited

frobware Mar 11, 2024 •

edited