Skip to content

NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM) #637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Nov 21, 2024
Merged

NE-1815: Add verifyhost to dynamic server slots for re-encrypt routes (DCM) #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions images/router/haproxy/conf/haproxy-config.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,15 @@ backend {{ genBackendNamePrefix $cfg.TLSTermination }}:{{ $cfgIdx }}
{{- else }}
{{- if not (isTrue $router_disable_http2) }} alpn h2,http/1.1
{{- end }}
{{- /*
Add "verifyhost" for the primary service only. This server is not intended
to be used for alternate backend endpoints. A reload will be triggered if
an alternate backend endpoint is added dynamically.
*/ -}}
{{- with $serviceUnit := index $.ServiceUnits $cfg.PrimaryServiceUnitKey -}}
{{- if $cfg.VerifyServiceHostname }} verifyhost {{ $serviceUnit.Hostname }}
{{- end }}
{{- end }}

@gcs278 gcs278 Nov 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't know what our philosophy is on DCM, or how much efficiency we are trying to squeeze out of it, but did you consider the strategy of always assuming dynamic servers are reserved for the primary backend (not the alternates), and change your logic to only reload if an alternate backend endpoints needs syncing?

While it might be an asymmetrical efficiency gain, it'd be a net gain to avoid reloading for primary backend endpoint changes. Net positive, but yes, asymmetrical gains (primary wins while alternate lose). Also depends the added complexity and whether that provides reasonable ROI. Not trying to demand we need to be as perfect as possible, but I'm just curious.

@alebedev87 alebedev87 Nov 19, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The idea is to get the maximum from the current DCM implementation. Ideally we would like to avoid changing the implementation. This PR is the first one which touches the router's code to be able to enable DCM (before there were only config changes). The touch is small and simple so it should not impact the router logic a lot.

I see the benefit of trying to reload only when there are endpoint updates for the alternate service. Let me see if it's doable and at what price. Upd: just recalled that the main problem was to bring the notion of alternate service to the template, all service units of a route are the same for the moment.

@gcs278 gcs278 Nov 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Upd: just recalled that the main problem was to bring the notion of alternate service to the template, all service units of a route are the same for the moment.

I see, it'd require more refactoring than probably it's worth for the added efficiency since we don't distinguish between the primary backend and alternate backends. It's something we can keep in mind for later improvements.

@alebedev87 alebedev87 Nov 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This commit can do what you proposed.

@gcs278 gcs278 Nov 20, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 Looks good, doesn't seem to add too much complexity, and it made the fix slightly more efficient.

{{- if gt (len $defaultDestinationCA) 0 }} verify required ca-file {{ $defaultDestinationCA }}
{{- else }} verify none
{{- end }}
Expand Down
190 changes: 154 additions & 36 deletions pkg/router/router_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"io/ioutil"
"os"
"path/filepath"
"reflect"
"strconv"
"strings"
"sync"
Expand Down Expand Up @@ -42,6 +41,7 @@ import (
"github.com/openshift/router/pkg/router"
"github.com/openshift/router/pkg/router/controller"
templateplugin "github.com/openshift/router/pkg/router/template"
haproxyconfigmanager "github.com/openshift/router/pkg/router/template/configmanager/haproxy"
"github.com/openshift/router/pkg/router/writerlease"
)

Expand Down Expand Up @@ -153,9 +153,13 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w==
-----END RSA PRIVATE KEY-----
`,
DefaultCertificateDir: h.dirs["certs"],
ReloadFn: func(shutdown bool) error { return nil },
TemplatePath: "../../images/router/haproxy/conf/haproxy-config.template",
ReloadInterval: reloadInterval,
// DefaultDestinationCAPath enables the host verification ("verifyhost")
// based on the service hostname for re-encrypt routes without
// DestinationCACertificate set.
DefaultDestinationCAPath: "dummy",
ReloadFn: func(shutdown bool) error { return nil },
TemplatePath: "../../images/router/haproxy/conf/haproxy-config.template",
ReloadInterval: reloadInterval,
HTTPResponseHeaders: []templateplugin.HTTPHeader{{
Name: "x-foo",
Value: "'bar'",
Expand All @@ -167,6 +171,10 @@ u3YLAbyW/lHhOCiZu2iAI8AbmXem9lW6Tr7p/97s0w==
Action: "Set",
}},
SecretManager: &fakesm.SecretManager{},
DynamicConfigManager: haproxyconfigmanager.NewHAProxyConfigManager(templateplugin.ConfigManagerOptions{
ConnectionInfo: "unix:///var/lib/dymmy",
MaxDynamicServers: 1,
}),
}
plugin, err = templateplugin.NewTemplatePlugin(pluginCfg, svcFetcher)
if err != nil {
Expand Down Expand Up @@ -670,10 +678,12 @@ func TestConfigTemplate(t *testing.T) {
},
"route with appProtocol: unknown-value": {
mustCreateWithConfig{
mustCreateEndpointSlice: mustCreateEndpointSlice{
name: "servicep1",
serviceName: "servicep1",
appProtocol: "unknown-value",
mustCreateEndpointSlices: []mustCreateEndpointSlice{
{
name: "servicep1",
serviceName: "servicep1",
appProtocol: "unknown-value",
},
},
mustCreateRoute: mustCreateRoute{
name: "p1",
Expand All @@ -692,10 +702,12 @@ func TestConfigTemplate(t *testing.T) {
},
"route with appProtocol: h2c": {
mustCreateWithConfig{
mustCreateEndpointSlice: mustCreateEndpointSlice{
name: "servicep2",
serviceName: "servicep2",
appProtocol: "h2c",
mustCreateEndpointSlices: []mustCreateEndpointSlice{
{
name: "servicep2",
serviceName: "servicep2",
appProtocol: "h2c",
},
},
mustCreateRoute: mustCreateRoute{
name: "p2",
Expand All @@ -713,10 +725,12 @@ func TestConfigTemplate(t *testing.T) {
},
"route with appProtocol: kubernetes.io/h2c": {
mustCreateWithConfig{
mustCreateEndpointSlice: mustCreateEndpointSlice{
name: "servicep3",
serviceName: "servicep3",
appProtocol: "kubernetes.io/h2c",
mustCreateEndpointSlices: []mustCreateEndpointSlice{
{
name: "servicep3",
serviceName: "servicep3",
appProtocol: "kubernetes.io/h2c",
},
},
mustCreateRoute: mustCreateRoute{
name: "p3",
Expand All @@ -732,23 +746,71 @@ func TestConfigTemplate(t *testing.T) {
},
},
},
"Verifyhost for dynamic slot": {
mustCreateWithConfig{
mustCreateEndpointSlices: []mustCreateEndpointSlice{
{
name: "serviceq",
serviceName: "serviceq",
},
},
mustCreateRoute: mustCreateRoute{
name: "q",
host: "qexample.com",
targetServiceName: "serviceq",
weight: int32(100),

@Miciah Miciah Nov 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

@alebedev87 alebedev87 Nov 21, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

time: start,
tlsTermination: routev1.TLSTerminationReencrypt,
},
mustMatchConfig: mustMatchConfig{
section: "backend",
sectionName: reencryptBackendName(h.namespace, "q"),
attribute: "server",
value: "_dynamic-pod-1 172.4.0.4:8765 weight 0 ssl disabled check inter 5000ms alpn h2,http/1.1 verifyhost serviceq.default.svc verify required ca-file dummy",
fullMatch: true,
},
},
},
"Verifyhost for dynamic slot with alternate backend": {
mustCreateWithConfig{
mustCreateEndpointSlices: []mustCreateEndpointSlice{
{
name: "serviceq1",
serviceName: "serviceq1",
},
{
name: "serviceq2",
serviceName: "serviceq2",
},
},
mustCreateRoute: mustCreateRoute{
name: "q1",
host: "q1example.com",
targetServiceName: "serviceq1",
weight: int32(50),
alternateBackend: "serviceq2",
alternateBackendWeight: int32(50),
time: start,
tlsTermination: routev1.TLSTerminationReencrypt,
},
mustMatchConfig: mustMatchConfig{
section: "backend",
sectionName: reencryptBackendName(h.namespace, "q1"),
attribute: "server",
value: "_dynamic-pod-1 172.4.0.4:8765 weight 0 ssl disabled check inter 5000ms alpn h2,http/1.1 verifyhost serviceq1.default.svc verify required ca-file dummy",
fullMatch: true,
},
},
},

@gcs278 gcs278 Nov 18, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Easy way to test VerifyHost doesn't get added with destinationCACertificate?

@alebedev87 alebedev87 Nov 19, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not really, the plugin config is created in TestMain(), I didn't want to restruct the test that far.

}

defer cleanUpRoutes(t)

for name, expectations := range tests {
for _, expectation := range expectations {
if !reflect.DeepEqual(expectation.mustCreateEndpointSlice, mustCreateEndpointSlice{}) {
err := expectation.mustCreateEndpointSlice.Apply(h)
if err != nil {
t.Fatalf("%s mustCreateEndpointSlice failed: %v", name, err)
}
}
if !reflect.DeepEqual(expectation.mustCreateRoute, mustCreateRoute{}) {
err := expectation.mustCreateRoute.Apply(h)
if err != nil {
t.Fatalf("%s mustCreateRoute failed: %v", name, err)
}
err := expectation.Apply(h)
if err != nil {
t.Fatalf("%s failed: %v", name, err)
}
}
}
Expand Down Expand Up @@ -809,6 +871,8 @@ type mustCreateRoute struct {
// targetServiceName is the spec.to.name of the route. If this field
// is empty, a name is generated based on the route's name.
targetServiceName string
// weight is the spec.to.weight of the route.
weight int32
// time is the metadata.creationTimestamp of the route.
time time.Time
// annotations is the metadata.annotations of the route.
Expand All @@ -821,12 +885,17 @@ type mustCreateRoute struct {
cert string
// httpHeaders is the spec.httpHeaders of the route.
httpHeaders routev1.RouteHTTPHeaders
// alternateBackend is the first item in spec.alternateBackends of the route.
alternateBackend string
// alternateBackendWeight is the weight of the first item in spec.alternateBackends of the route.
alternateBackendWeight int32
}

func (e mustCreateRoute) Apply(h *harness) error {
if e.name == "" {
return nil
}

annotations := map[string]string{}
if e.annotations != nil {
annotations = e.annotations
Expand All @@ -842,6 +911,23 @@ func (e mustCreateRoute) Apply(h *harness) error {
if e.targetServiceName != "" {
serviceName = e.targetServiceName
}
weight := new(int32)

@Miciah Miciah Nov 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@alebedev87 alebedev87 Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

if e.weight != int32(0) {
weight = &e.weight
}
alternateBackends := []routev1.RouteTargetReference{}
if e.alternateBackend != "" {
weighta := new(int32)
if e.alternateBackendWeight != int32(0) {
weighta = &e.alternateBackendWeight
}
alternateBackends = []routev1.RouteTargetReference{
{
Name: e.alternateBackend,
Weight: weighta,
},
}
}
route := &routev1.Route{
ObjectMeta: metav1.ObjectMeta{
CreationTimestamp: metav1.Time{Time: e.time},
Expand All @@ -855,11 +941,12 @@ func (e mustCreateRoute) Apply(h *harness) error {
Path: e.path,
To: routev1.RouteTargetReference{
Name: serviceName,
Weight: new(int32),
Weight: weight,
},
WildcardPolicy: routev1.WildcardPolicyNone,
TLS: tlsConfig,
HTTPHeaders: &e.httpHeaders,
AlternateBackends: alternateBackends,
WildcardPolicy: routev1.WildcardPolicyNone,
TLS: tlsConfig,
HTTPHeaders: &e.httpHeaders,
},
}
_, err := h.routeClient.RouteV1().Routes(route.Namespace).Create(context.TODO(), route, metav1.CreateOptions{})
Expand Down Expand Up @@ -907,11 +994,23 @@ func (e mustCreateEndpointSlice) Apply(h *harness) error {
}

type mustCreateWithConfig struct {
mustCreateEndpointSlice
mustCreateEndpointSlices []mustCreateEndpointSlice
mustCreateRoute
mustMatchConfig
}

func (e mustCreateWithConfig) Apply(h *harness) error {
if err := e.mustCreateRoute.Apply(h); err != nil {
return err
}
for _, ep := range e.mustCreateEndpointSlices {
if err := ep.Apply(h); err != nil {
return err
}
}
return nil
}

// mustMatchConfig uses HAProxy's config parser to find config snippets
type mustMatchConfig struct {
// mapFile specifies a map file to search. If empty, haproxy.config is
Expand All @@ -931,6 +1030,8 @@ type mustMatchConfig struct {
// notFound indicates whether the expectation is that value be present
// or that it be absent.
notFound bool
// fullMatch indicates whether the expectation is that value matches fully.
fullMatch bool
}

func (m mustMatchConfig) Match(parser haproxyconfparser.Parser) error {
Expand Down Expand Up @@ -970,12 +1071,24 @@ func matchConfig(m mustMatchConfig, parser haproxyconfparser.Parser) error {
}
}
case []haproxyconfparsertypes.Server:
for _, a := range data {
for _, b := range a.Params {
contains = contains || b.String() == m.value
if m.fullMatch {
for _, a := range data {
params := ""
for _, p := range a.Params {
params += " " + p.String()
}
if a.Name+" "+a.Address+params == m.value {
contains = true
break
}
}
} else {
for _, a := range data {
for _, b := range a.Params {
contains = contains || b.String() == m.value
}
}
}

}

if !contains && !m.notFound {
Expand Down Expand Up @@ -1128,3 +1241,8 @@ func edgeBackendName(ns, route string) string {
func insecureBackendName(ns, route string) string {
return "be_http:" + ns + ":" + route
}

// reencryptBackendName contructs the HAProxy config's backend name for a re-encrypt route.
func reencryptBackendName(ns, route string) string {
return "be_secure:" + ns + ":" + route
}
7 changes: 4 additions & 3 deletions pkg/router/template/plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,11 @@ func (r *TestRouter) AddRoute(route *routev1.Route) {
routeKey := getKey(route)

config := ServiceAliasConfig{
Host: route.Spec.Host,
Path: route.Spec.Path,
ServiceUnits: getServiceUnits(route),
Host: route.Spec.Host,
Path: route.Spec.Path,
}
serviceUnits, _ := getServiceUnits(route)
config.ServiceUnits = serviceUnits
config.ServiceUnitNames = r.calculateServiceWeights(config.ServiceUnits)

for key := range config.ServiceUnits {
Expand Down
Loading