NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438

miheer · 2022-12-25T12:55:50Z

images/router/haproxy/conf/haproxy-config.template Sets the
http-response and http-request stanzas to set or delete custom HTTP
Response/Request headers.
pkg/cmd/infra/router/template.go This consists the logic to get the
value from the env var from router deployment and use it for
setting/deleting HTTP response/request headers.
pkg/router/template/plugin.go Adds new fields in the
TemplatePluginConfig struct to set or delete HTTP request/reponse
headers. These are needed to set the values we get from router
deployment env var.
pkg/router/template/router.go This consists of the logic extracting
header values from the route object and then set/delete
HTTP request/reponse headers.
pkg/router/template/router_test.go This consists of units test cases
to the logic to extract headers from route object and set/delete HTTP
request/reponse headers.
pkg/router/template/types.go This adds two fields HTTPResponseHeaders
and HTTPRequestHeaders of type []HTTPHeader in the ServiceAliasConfig
which is used in the haproxy-config.template to add the set/delete
http-reponse and http-request stanzas for the header values fetched from
the route object.
This also add a struct called HTTPHeader which helps setting the HTTP
response/request values in the http-reponse/http-request stanzas in the
haproxy-config.template.

Jira Number https://issues.redhat.com/browse/NE-982

miheer · 2022-12-26T16:06:59Z

/test verify

candita · 2023-08-03T21:45:26Z

pkg/cmd/infra/router/template.go

+	// RFC 2616, section 4.2, states that the header name
+	// must be a valid token.
+	if !validTokenRE.MatchString(headerName) {
+		return fmt.Errorf("invalid HTTP header name: %v", headerName)


nit:

Suggested change

return fmt.Errorf("invalid HTTP header name: %v", headerName)

return fmt.Errorf("invalid HTTP header name: %s", headerName)

candita · 2023-08-03T21:59:10Z

pkg/cmd/infra/router/template_test.go

+				t.Errorf(" expected %s, got %s", tc.expectedValue, actualValue)
+			case tc.expectErrorMessage == "" && actualErrorMessage != nil && actualErrorMessage.Error() != "":
+				t.Fatalf("unexpected error: %v", actualErrorMessage)
+			case tc.expectErrorMessage != "" && actualErrorMessage != nil && actualErrorMessage.Error() == "":


Here you should be looking for the case where actualErrorMessage is nil or blank:

Suggested change

case tc.expectErrorMessage != "" && actualErrorMessage != nil && actualErrorMessage.Error() == "":

case tc.expectErrorMessage != "" && (actualErrorMessage == nil || actualErrorMessage.Error() == ""):

if actualErrorMessage == nil

then actualErrorMessage.Error() i.e nil.Error() will crash

It will stop processing the || conjunction if actualErrorMessage == nil.

candita · 2023-08-08T21:55:14Z

pkg/cmd/infra/router/template.go

+					if err != nil {
+						return captureHeaders, fmt.Errorf("failed to decode percent encoding: %v", parts[1])
+					}
+					sanitizedHeaderValue := templateplugin.SanitizeHeaderValue(headerValue)


Why does the header value have to be sanitized? If it has a valid reason, why isn't the header name or action sanitized?

Header name goes through a regex check so a user can't provide an invalid value.
Header action also only 2 values are allowed via api so this check happens at api level.

The SanitizeHeaderValue is escaping single quotes, which we allow in the header name per the regex. Why don't we allow single quotes in the header value?

SanitizeHeaderValue doesn't actually sanitize, but rather it quotes the string (for this reason, I've suggested changing "sanitize" to "quote" a few times). I agree, it certainly seems like the header name can contain ' and therefore needs to be quoted in the configuration file, just as the value needs to be quoted.

candita · 2023-08-08T21:59:01Z

pkg/cmd/infra/router/template.go

+func checkValidHeaderName(headerName string) error {
+	// RFC 2616, section 4.2, states that the header name
+	// must be a valid token.
+	if !validTokenRE.MatchString(headerName) {


The validTokenRE is the same now as the regex for header name in https://github.com/openshift/library-go/pull/1449/files#diff-bfc6ba95cbc94986901ce1d0e3b74ee966499eab85a96e1d1ff763d2d74a7eeeR64

How are you going to make sure changes between these get propagated? I think you probably need to choose one or the other.

This point is valid. Both check the same chars. However I can add a regex check here to maintain consistency.

I will put a comment here.

candita · 2023-08-08T22:15:51Z

pkg/cmd/infra/router/template.go

+
+func checkValidAction(action string) error {
+	if action != string(routev1.Set) && action != string(routev1.Delete) {
+		return fmt.Errorf("invalid action %s", action)


Add a semi-colon between the error message and the error value, as customary. Unit tests will need to be updated too.

Suggested change

return fmt.Errorf("invalid action %s", action)

return fmt.Errorf("invalid action: %s", action)

candita · 2023-08-08T22:19:51Z

pkg/cmd/infra/router/template.go

+	var capture templateplugin.HTTPHeader
+	var err error
+	if len(in) == 0 {
+		return captureHeaders, fmt.Errorf("encoded header string not present.")


This is the only error message with a period at the end. Please remove.

Suggested change

return captureHeaders, fmt.Errorf("encoded header string not present.")

return captureHeaders, fmt.Errorf("encoded header string not present")

gomfmt -w pkg/router/template/certmanager_test.go

…nd Route Object. `images/router/haproxy/conf/haproxy-config.template` Sets the http-response and http-request stanzas to set or delete custom HTTP Response/Request headers. `pkg/cmd/infra/router/template.go` This consists the logic to get the value from the env var from router deployment and use it for setting/deleting HTTP response/request headers. `pkg/router/template/plugin.go` Adds new fields in the TemplatePluginConfig struct to set or delete HTTP request/reponse headers. These are needed to set the values we get from router deployment env var. `pkg/router/template/router.go` This consists of the logic extracting header values from the route object and then set/delete HTTP request/reponse headers. `pkg/router/template/router_test.go` This consists of units test cases to the logic to extract headers from route object and set/delete HTTP request/reponse headers. `pkg/router/template/types.go` This adds two fields HTTPResponseHeaders and HTTPRequestHeaders of type []HTTPHeader in the ServiceAliasConfig which is used in the haproxy-config.template to add the set/delete http-reponse and http-request stanzas for the header values fetched from the route object. This also add a struct called HTTPHeader which helps setting the HTTP response/request values in the http-reponse/http-request stanzas in the haproxy-config.template.

* pkg/router/router_test.go (TestConfigTemplate): Use Fatal, not Fatalf, to print the error message from expectation.Match, which is not expected to be a formatted string.

* pkg/router/router_test.go (TestConfigTemplate): Print the HAProxy config file if the test case fails.

* pkg/router/router_test.go (TestMain): Define some global HTTP request and response headers. (TestConfigTemplate): Add test cases for global and route HTTP request and response headers to verify that the template plugin properly quotes values. (mustCreate): Add godoc comments, and add an httpHeaders field for specifying spec.httpHeaders on the route. (mustCreate): If the route name is empty, don't create a route. Copy httpHeaders from mustCreate to the route's spec.httpHeaders field. (insecureBackendName): New helper, used by TestConfigTemplate.

* pkg/router/template/router.go (configsAreEqual): Add a comment explaining why this function does not check the ServiceUnitNames, ActiveServiceUnits, or ActiveEndpoints fields. * pkg/router/template/router_test.go (Test_configsAreEqual): New test. Verify that configsAreEqual behaves correctly.

ShudiLi · 2023-08-09T12:26:14Z

/label qe-approved
If needed, I can list the test cases what I have done, thanks.

miheer · 2023-08-09T12:29:00Z

/retest

miheer · 2023-08-09T12:29:57Z

/label qe-approved If needed, I can list the test cases what I have done, thanks.

OK. If you can add them in a google doc and share us the link then that will be great.

Miciah · 2023-08-09T12:31:10Z

pkg/cmd/infra/router/template.go

+func checkValidHeaderName(headerName string) error {
+	// RFC 2616, section 4.2, states that the header name
+	// must be a valid token.
+	// Any changes made to regex of header name in route type in openshift/api and `validation.go` file in library-go shall be reflected here and
+	// vice-versa
+	permittedHeaderNameRE := regexp.MustCompile("^[-!#$%&'*+.0-9A-Z^_`a-z|~]+$")


Don't recompile the regexp every time the function is called. Don't use the word "shall". End your sentences with a period. Remember to add godoc.

Suggested change

func checkValidHeaderName(headerName string) error {

// RFC 2616, section 4.2, states that the header name

// must be a valid token.

// Any changes made to regex of header name in route type in openshift/api and `validation.go` file in library-go shall be reflected here and

// vice-versa

permittedHeaderNameRE := regexp.MustCompile("^[-!#$%&'*+.0-9A-Z^_`a-z|~]+$")

// permittedHeaderNameRE is a compiled regexp to match an HTTP header name

// as specified in RFC 2616, section 4.2.

// Any changes made to regex of header name in route type in openshift/api and `validation.go` file in library-go must be reflected here and

// vice versa.

var permittedHeaderNameRE = regexp.MustCompile("^[-!#$%&'*+.0-9A-Z^_`a-z|~]+$")

// checkValidHeaderName verifies that the given HTTP header name is valid.

func checkValidHeaderName(headerName string) error {

The header name needs to be sanitized as a single quote(') can be provided by a user for a header name which is allowed. However, passing a single quote in the http-request or http-reponse stanza in the haproxy-config causes haproxy to fail with: [ALERT] (78703) : config : Error(s) found in configuration file : ./haproxy.cfg [ALERT] (78703) : config : Fatal errors found in configuration. That is why escape and quote single quote present in a header value. For example: X'-Forwarded-For will get sanitized to 'X'\''-Forwarded-For' using escaping and quoting.

openshift-ci · 2023-08-09T17:26:52Z

@miheer: 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.

candita · 2023-08-09T22:04:43Z

/approve
/lgtm

openshift-ci · 2023-08-09T22:05:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

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~~ [candita]

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

openshift-ci bot requested review from candita and Miciah December 25, 2022 12:56

miheer force-pushed the headers branch 2 times, most recently from fca6a85 to 54f0b6e Compare December 25, 2022 13:35

miheer changed the title ~~Set/replace/delete/append Headers~~ WIP: Set/replace/delete/append Headers Dec 25, 2022

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 25, 2022

miheer force-pushed the headers branch 2 times, most recently from efc4f2a to c312c22 Compare December 26, 2022 15:23

miheer force-pushed the headers branch 4 times, most recently from ba2ad10 to 4c7e751 Compare December 27, 2022 07:26

miheer force-pushed the headers branch 9 times, most recently from 9ade113 to 066daad Compare January 13, 2023 06:12

miheer force-pushed the headers branch 2 times, most recently from 0a44eec to 975a0d9 Compare January 13, 2023 09:47

miheer force-pushed the headers branch 6 times, most recently from 7e2ad7a to 65b288d Compare February 15, 2023 00:04

candita reviewed Aug 3, 2023

View reviewed changes

miheer force-pushed the headers branch 2 times, most recently from 30f3fa2 to fbe3330 Compare August 5, 2023 08:32

candita reviewed Aug 8, 2023

View reviewed changes

miheer and others added 6 commits August 9, 2023 18:45

Vendored changes

dde2022

gomfmt -w pkg/router/template/certmanager_test.go

TestConfigTemplate: Change Fatalf to Fatal

3f7c183

* pkg/router/router_test.go (TestConfigTemplate): Use Fatal, not Fatalf, to print the error message from expectation.Match, which is not expected to be a formatted string.

TestConfigTemplate: Print haproxy.config if failed

84c7f84

* pkg/router/router_test.go (TestConfigTemplate): Print the HAProxy config file if the test case fails.

miheer force-pushed the headers branch from 6fc483f to 5abf448 Compare August 9, 2023 08:46

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

Miciah reviewed Aug 9, 2023

View reviewed changes

miheer force-pushed the headers branch from 5abf448 to 1c856e3 Compare August 9, 2023 14:27

miheer added 2 commits August 10, 2023 00:33

Fix nits made by reviewers

b51fe75

miheer force-pushed the headers branch from 1c856e3 to e68f171 Compare August 9, 2023 14:33

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2023

openshift-merge-robot merged commit 44a5304 into openshift:master Aug 9, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438

NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438

miheer commented Dec 25, 2022 •

edited

miheer commented Dec 26, 2022

candita Aug 3, 2023

miheer Aug 4, 2023

candita Aug 3, 2023

miheer Aug 4, 2023 •

edited

candita Aug 4, 2023

miheer Aug 5, 2023

candita Aug 8, 2023

miheer Aug 8, 2023

candita Aug 9, 2023

Miciah Aug 9, 2023 •

edited

candita Aug 8, 2023

miheer Aug 8, 2023

miheer Aug 8, 2023

candita Aug 8, 2023

candita Aug 8, 2023

ShudiLi commented Aug 9, 2023

miheer commented Aug 9, 2023

miheer commented Aug 9, 2023

Miciah Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

candita commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

	return fmt.Errorf("invalid HTTP header name: %v", headerName)
	return fmt.Errorf("invalid HTTP header name: %s", headerName)

	case tc.expectErrorMessage != "" && actualErrorMessage != nil && actualErrorMessage.Error() == "":
	case tc.expectErrorMessage != "" && (actualErrorMessage == nil \|\| actualErrorMessage.Error() == ""):

	return fmt.Errorf("invalid action %s", action)
	return fmt.Errorf("invalid action: %s", action)

	return captureHeaders, fmt.Errorf("encoded header string not present.")
	return captureHeaders, fmt.Errorf("encoded header string not present")

NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438

NE-1141: Adds logic for setting and deleting headers via Ingress Operator CR and Route Object. #438

Conversation

miheer commented Dec 25, 2022 • edited

miheer commented Dec 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miheer Aug 4, 2023 • 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

Miciah Aug 9, 2023 • 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

ShudiLi commented Aug 9, 2023

miheer commented Aug 9, 2023

miheer commented Aug 9, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Aug 9, 2023

candita commented Aug 9, 2023

openshift-ci bot commented Aug 9, 2023

miheer commented Dec 25, 2022 •

edited

miheer Aug 4, 2023 •

edited

Miciah Aug 9, 2023 •

edited