NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM) by alebedev87 · Pull Request #637 · openshift/router

alebedev87 · 2024-11-08T14:41:47Z

This PR introduces the "verifyhost" configuration setting to dynamic server slots during template rendering. This ensures consistent behavior with static servers.

The "verifyhost" uses the FQDN of the primary service only, as it's not feasible to predict which service endpoint will be used when alternate backends are available.Consequently, dynamic servers for re-encrypt routes are not intended to handle alternate backend endpoints. A reload is forced when endpoint changes occur for alternate backend services in a re-encrypt route.

The commit which added the server certificate verification without verifyhost: 50a330d.

openshift-ci-robot · 2024-11-08T14:41:51Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR adds "verifyhost" configuration setting to dynamic server slots during template rendering to ensure consistent behavior with static servers.

The "verifyhost" with the service hostname is added only when:

The route does not have a destination CA certificate.

The route has a single service.

Limiting "verifyhost" to a single service route is essential, as it's not possible to reliably predict which service’s endpoint will be used when alternate backends are available.

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-ci-robot · 2024-11-08T14:43:13Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR adds "verifyhost" configuration setting to dynamic server slots during template rendering to ensure consistent behavior with static servers.

The "verifyhost" with the service hostname is added only when:

The route does not have a destination CA certificate.

The route has a single service.

Limiting "verifyhost" to a single service route is essential, as it's not possible to reliably predict which service’s endpoint will be used when alternate backends are available.

The commit which added the server certificate verification without verifyhost: 50a330d.

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.

Miciah · 2024-11-08T15:22:48Z

This is only part of the solution, right? We need both of the following:

Set verifyhost on the pre-allocated server line for a single-service route (this PR). This is necessary to ensure that adding an endpoint to a single-service route behaves the same with DCM as without.
Force a reload if the endpoints change for a multi-service route. This is necessary to ensure that we write out server lines with verifyhost and the correct host name for multi-service routes.

alebedev87 · 2024-11-08T15:27:55Z

Force a reload if the endpoints change for a multi-service route. This is necessary to ensure that we write out server lines with verifyhost and the correct host name for multi-service routes.

This would be the safest, yes. Let me see whether I can find a way to do this.

alebedev87 · 2024-11-13T15:31:43Z

/retest

Miciah · 2024-11-13T16:16:07Z

/assign

alebedev87 · 2024-11-13T16:23:25Z

/retitle [WIP] NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM)

Work In Progress until #637 (comment) is addressed.

alebedev87 · 2024-11-14T10:12:12Z

Force a reload if the endpoints change for a multi-service route. This is necessary to ensure that we write out server lines with verifyhost and the correct host name for multi-service routes.

Addressed here.

openshift-ci-robot · 2024-11-14T10:20:10Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR adds "verifyhost" configuration setting to dynamic server slots during template rendering to ensure consistent behavior with static servers.

The "verifyhost" with the service hostname is added only when:

The route does not have a destination CA certificate.

The route has a single active service.

Limiting "verifyhost" to a single service route is essential, as it's not possible to reliably predict which service’s endpoint will be used when alternate backends are available.

Additionally, for the same reason, a reload is triggered when a re-encrypt route has multiple active services.

The commit which added the server certificate verification without verifyhost: 50a330d.

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-ci-robot · 2024-11-14T10:20:19Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR adds "verifyhost" configuration setting to dynamic server slots during template rendering to ensure consistent behavior with static servers.

The "verifyhost" with the service hostname is added only when:

The route does not have a destination CA certificate.

The route has a single active service.

Limiting "verifyhost" to a single service route is essential, as it's not possible to reliably predict which service’s endpoint will be used when alternate backends are available. Additionally, for the same reason, a reload is triggered when a re-encrypt route has multiple active services.

The commit which added the server certificate verification without verifyhost: 50a330d.

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.

alebedev87 · 2024-11-14T14:45:42Z

/retest

openshift-ci-robot · 2024-11-14T15:43:17Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR adds "verifyhost" configuration setting to dynamic server slots during template rendering to ensure consistent behavior with static servers.

The "verifyhost" with the service hostname is added only when:

The route does not have a destination CA certificate.

The route has a single active service.

Limiting "verifyhost" to a single service route is essential, as it's not possible to reliably predict which service’s endpoint will be used when alternate backends are available. Additionally, for the same reason, a reload is forced when an endpoint change happens for a re-encrypt route which has multiple active services.

The commit which added the server certificate verification without verifyhost: 50a330d.

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.

alebedev87 · 2024-11-14T22:51:52Z

Changed TestConfigTemplate test to converge to the approach from #627.

alebedev87 · 2024-11-15T19:41:36Z

Rebased from master after #627 was merged.

alebedev87 · 2024-11-16T07:05:05Z

/retest

alebedev87 · 2024-11-18T16:39:26Z

/assign @gcs278

alebedev87 · 2024-11-20T09:38:33Z

Your recent updates look good to me, just one last minor comment. You will need to update your PR description if you plan to keep the commit (and presumably squash if you want to keep it clean).

Thanks a lot! The commits are fixed up, the description is updated.

openshift-ci-robot · 2024-11-20T09:40:37Z

@alebedev87: This pull request references NE-1815 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

Details

In response to this:

This PR introduces the "verifyhost" configuration setting to dynamic server slots during template rendering. This ensures consistent behavior with static servers.

The "verifyhost" uses the FQDN of the primary service only, as it's not feasible to predict which service endpoint will be used when alternate backends are available.Consequently, dynamic servers for re-encrypt routes are not intended to handle alternate backend endpoints. A reload is forced when endpoint changes occur for alternate backend services in a re-encrypt route.

The commit which added the server certificate verification without verifyhost: 50a330d.

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.

alebedev87 · 2024-11-20T10:15:01Z

/label docs-approved
/label px-approved

Technical gap, no need to ping the docs people.

alebedev87 · 2024-11-20T10:25:46Z

/label acknowledge-critical-fixes-only

This change impacts only the DCM enabled router which is not the case by default. The featuregate needs to be enabled to activate DCM.

alebedev87 · 2024-11-20T13:59:14Z

/test e2e-aws-serial

e2e-aws-serial passed the test but the whole job took too long and timed out:

INFO[2024-11-20T13:41:05Z] Step phase test succeeded after 2h29m19s.    
INFO[2024-11-20T13:41:05Z] Running multi-stage phase post               
INFO[2024-11-20T13:41:05Z] Running step e2e-aws-serial-gather-aws-console. 
{"component":"entrypoint","file":"sigs.k8s.io/prow/pkg/entrypoint/run.go:169","func":"sigs.k8s.io/prow/pkg/entrypoint.Options.ExecuteProcess","level":"error","msg":"Process did not finish before 4h0m0s timeout","severity":"error","time":"2024-11-20T13:41:22Z"}

gcs278 · 2024-11-20T14:30:23Z

Thanks for the updates & the responses.
/lgtm

Will let @Miciah approve.

Miciah · 2024-11-21T10:37:52Z

+					name:              "q",
+					host:              "qexample.com",
+					targetServiceName: "serviceq",
+					weight:            int32(100),


Why is weight needed in this test case? Is it used just to verify that the dynamic server does not start with the route weight?

No, I think I did it for consistency with the alternate backend case which I wanted to look "real". None of them really influence the template rendering: weight is always 0. Upd: Which indeed looks like what you said - it now verifies that the dynamic slot is created without the service weight taken into account.

Miciah · 2024-11-21T10:39:16Z

+				Weight: &fifty,
+			},
+		}
+		weight = &fifty


Is this assignment correct?

What you think is not correct? I wanted the 50-50 balance between 2 services.

The godoc for the weight field says, "weight is the spec.to.weight of the route." It seems contradictory to the godoc and surprising to me that the value in the weight field would be ignored.

Added a dedicated field for the alternate backend weight.

I see, I didn't understand your intention before, so I'm glad I asked. Thanks for changing the test expectation to be more explicit!

… (DCM) This commit introduces the "verifyhost" configuration setting to dynamic server slots during template rendering. This ensures consistent behavior with static servers. The "verifyhost" uses the FQDN of the primary service only, as it's not feasible to predict which service endpoint will be used when alternate backends are available. Consequently, dynamic servers for re-encrypt routes are not intended to handle alternate backend endpoints. A reload is forced when endpoint changes occur for alternate backend services in a re-encrypt route.

Miciah · 2024-11-21T16:01:57Z

 	if e.targetServiceName != "" {
 		serviceName = e.targetServiceName
 	}
+	weight := new(int32)


Not a big deal, but is there a reason you use weight := new(int32) instead of using var weight int32?

No, I was just inspired by the previous code which did this: Weight: new(int32).

Miciah · 2024-11-21T16:42:07Z

Thanks!
/approve
/lgtm

alebedev87 · 2024-11-21T17:41:02Z

/retest

Tests are not starting for an hour.

openshift-ci-robot · 2024-11-21T18:30:08Z

/retest-required

Remaining retests: 0 against base HEAD 7832237 and 2 for PR HEAD 68f0e2c in total

Miciah · 2024-11-21T19:14:54Z

Thanks!
/approve
/lgtm
(Hm, I thought I already posted this comment.)
Edit: Ah, now I see my previous comment. ¯\_(ツ)_/¯.

openshift-ci · 2024-11-21T19:18:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

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

alebedev87 · 2024-11-21T19:53:30Z

/test e2e-aws-serial

alebedev87 · 2024-11-21T19:54:06Z

/test e2e-upgrade

openshift-ci · 2024-11-21T23:53:50Z

@alebedev87: all tests passed!

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

openshift-bot · 2024-11-22T02:44:25Z

[ART PR BUILD NOTIFIER]

Distgit: ose-haproxy-router-base
This PR has been included in build ose-haproxy-router-base-container-v4.19.0-202411220208.p0.gbe319d0.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2024-11-22T02:56:33Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-haproxy-router
This PR has been included in build openshift-enterprise-haproxy-router-container-v4.19.0-202411220208.p0.gbe319d0.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 8, 2024

openshift-ci Bot requested review from Miciah and candita November 8, 2024 14:42

openshift-ci Bot assigned Miciah Nov 13, 2024

openshift-ci Bot changed the title ~~NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM)~~ [WIP] NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM) Nov 13, 2024

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 Nov 13, 2024

alebedev87 force-pushed the verifyhost branch 2 times, most recently from 62e99fc to 430b33c Compare November 14, 2024 10:07

alebedev87 force-pushed the verifyhost branch from 430b33c to 2d3e517 Compare November 14, 2024 10:17

alebedev87 changed the title ~~[WIP] NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM)~~ NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM) Nov 14, 2024

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2024

alebedev87 force-pushed the verifyhost branch from 2d3e517 to e43d4c9 Compare November 14, 2024 22:49

alebedev87 force-pushed the verifyhost branch from e43d4c9 to 9779c5e Compare November 15, 2024 19:40

openshift-ci Bot assigned gcs278 Nov 18, 2024

alebedev87 force-pushed the verifyhost branch from 2c89f56 to 578e830 Compare November 20, 2024 09:40

openshift-ci Bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Nov 20, 2024

openshift-ci Bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Nov 20, 2024

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 20, 2024

Miciah reviewed Nov 21, 2024

View reviewed changes

alebedev87 force-pushed the verifyhost branch from 578e830 to 68f0e2c Compare November 21, 2024 15:55

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2024

Miciah reviewed Nov 21, 2024

View reviewed changes

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 21, 2024

openshift-merge-bot Bot merged commit be319d0 into openshift:master Nov 21, 2024

Uh oh!

Conversation

alebedev87 commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 8, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 8, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah commented Nov 8, 2024

Uh oh!

alebedev87 commented Nov 8, 2024

Uh oh!

alebedev87 commented Nov 13, 2024

Uh oh!

Miciah commented Nov 13, 2024

Uh oh!

alebedev87 commented Nov 13, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Nov 14, 2024

Uh oh!

openshift-ci-robot commented Nov 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Nov 14, 2024

Uh oh!

openshift-ci-robot commented Nov 14, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Nov 15, 2024

Uh oh!

alebedev87 commented Nov 16, 2024

Uh oh!

alebedev87 commented Nov 18, 2024

Uh oh!

alebedev87 commented Nov 20, 2024

Uh oh!

openshift-ci-robot commented Nov 20, 2024 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alebedev87 commented Nov 20, 2024

Uh oh!

alebedev87 commented Nov 20, 2024

Uh oh!

alebedev87 commented Nov 20, 2024

Uh oh!

gcs278 commented Nov 20, 2024

Uh oh!

Miciah Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

alebedev87 Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

alebedev87 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Miciah Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alebedev87 Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Miciah Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

alebedev87 commented Nov 8, 2024 •

edited

Loading

openshift-ci-robot commented Nov 8, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 8, 2024 •

edited by openshift-ci Bot

Loading

alebedev87 commented Nov 13, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 14, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 14, 2024 •

edited by openshift-ci Bot

Loading

openshift-ci-robot commented Nov 14, 2024 •

edited by openshift-ci Bot

Loading

alebedev87 commented Nov 14, 2024 •

edited

Loading

openshift-ci-robot commented Nov 20, 2024 •

edited by openshift-ci Bot

Loading

alebedev87 Nov 21, 2024 •

edited

Loading

Miciah Nov 21, 2024 •

edited

Loading

Miciah commented Nov 21, 2024 •

edited

Loading