Skip to content

Cloud HA strategy should automatically use PROXY support when available. #84

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-robot merged 5 commits into from
Jan 25, 2019
Merged

Cloud HA strategy should automatically use PROXY support when available. #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cef3ff
Cloud HA strategy should automatically use PROXY support when available.
ramr Dec 11, 2018
1b7b0ec
Add e2e test for proxy protocol + add some details how we can shore u…
ramr Dec 18, 2018
17f0a51
Fixes based on Miciah's review comments.
ramr Dec 19, 2018
685478b
Changes (+ rebased for conflicts) to use the new infrastructure confi…
ramr Jan 13, 2019
ae89da0
Fix parameters changed to NewFactory - rebase picks up old changes.
ramr Jan 15, 2019
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
18 changes: 18 additions & 0 deletions pkg/manifests/manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ const (
// Annotation used to inform the certificate generation service to
// generate a cluster-signed certificate and populate the secret.
ServingCertSecretAnnotation = "service.alpha.openshift.io/serving-cert-secret-name"

// Annotation used to enable the proxy protocol on the AWS load balancer.
AWSLBProxyProtocolAnnotation = "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol"
Copy link
Contributor

@Miciah Miciah Dec 18, 2018

Choose a reason for hiding this comment

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

Would it be worth importing github.com/kubernetes/cloud-provider-aws/pkg/cloudprovider/providers/aws and using aws.ServiceAnnotationLoadBalancerProxyProtocol?

Copy link
Contributor Author

@ramr ramr Dec 19, 2018

Choose a reason for hiding this comment

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

Just tried to use that but it causes dep errors. See: https://gist.github.com/ramr/1573ecb990fc42f3d3b8c4de36a8c81c

)

func MustAssetReader(asset string) io.Reader {
Expand Down Expand Up @@ -147,6 +150,14 @@ func (f *Factory) RouterDeployment(cr *ingressv1alpha1.ClusterIngress) (*appsv1.
env = append(env, corev1.EnvVar{Name: "ROUTER_CANONICAL_HOSTNAME", Value: *cr.Spec.IngressDomain})
}

if cr.Spec.HighAvailability != nil && cr.Spec.HighAvailability.Type == ingressv1alpha1.CloudClusterIngressHA {
// For now, check if we are on AWS. This can really be done for
// for any external [cloud] LBs that support the proxy protocol.
if f.config.Platform == configv1.AWSPlatform {
env = append(env, corev1.EnvVar{Name: "ROUTER_USE_PROXY_PROTOCOL", Value: "true"})
}
}

if cr.Spec.NodePlacement != nil {
if cr.Spec.NodePlacement.NodeSelector != nil {
nodeSelector, err := metav1.LabelSelectorAsMap(cr.Spec.NodePlacement.NodeSelector)
Expand Down Expand Up @@ -258,6 +269,13 @@ func (f *Factory) RouterServiceCloud(cr *ingressv1alpha1.ClusterIngress) (*corev
}
s.Spec.Selector["router"] = name

if f.config.Platform == configv1.AWSPlatform {
if s.Annotations == nil {
s.Annotations = map[string]string{}
}
s.Annotations[AWSLBProxyProtocolAnnotation] = "*"
}

return s, nil
}

Expand Down
32 changes: 31 additions & 1 deletion pkg/manifests/manifests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package manifests

import (
"fmt"
"strconv"
"testing"
"time"

Expand All @@ -14,7 +15,10 @@ import (
)

func TestManifests(t *testing.T) {
config := operatorconfig.Config{RouterImage: "quay.io/openshift/router:latest"}
config := operatorconfig.Config{
RouterImage: "quay.io/openshift/router:latest",
Platform: configv1.AWSPlatform,
}
f := NewFactory(config)

ci := &ingressv1alpha1.ClusterIngress{
Expand Down Expand Up @@ -96,6 +100,19 @@ func TestManifests(t *testing.T) {
t.Error("router Deployment has no default node selector")
}

proxyProtocolEnabled := false
for _, envVar := range deployment.Spec.Template.Spec.Containers[0].Env {
if envVar.Name == "ROUTER_USE_PROXY_PROTOCOL" {
if v, err := strconv.ParseBool(envVar.Value); err == nil {
proxyProtocolEnabled = v
}
break
}
}
if proxyProtocolEnabled {
t.Errorf("router Deployment unexpected proxy protocol")
}

if deployment.Spec.Template.Spec.Volumes[0].Secret == nil {
t.Error("router Deployment has no secret volume")
}
Expand Down Expand Up @@ -141,6 +158,19 @@ func TestManifests(t *testing.T) {
t.Errorf("expected empty readiness probe host, got %q", deployment.Spec.Template.Spec.Containers[0].ReadinessProbe.Handler.HTTPGet.Host)
}

proxyProtocolEnabled = false
for _, envVar := range deployment.Spec.Template.Spec.Containers[0].Env {
if envVar.Name == "ROUTER_USE_PROXY_PROTOCOL" {
if v, err := strconv.ParseBool(envVar.Value); err == nil {
proxyProtocolEnabled = v
}
break
}
}
if !proxyProtocolEnabled {
t.Errorf("router Deployment expected proxy protocol")
}

secretName := fmt.Sprintf("secret-%v", time.Now().UnixNano())
ci.Spec.DefaultCertificateSecret = &secretName
ci.Spec.HighAvailability.Type = ingressv1alpha1.UserDefinedClusterIngressHA
Expand Down
90 changes: 90 additions & 0 deletions test/e2e/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"context"
"fmt"
"os"
"strconv"
"testing"
"time"

Expand Down Expand Up @@ -157,6 +158,95 @@ func TestRouterServiceInternalEndpoints(t *testing.T) {
}
}

func TestClusterProxyProtocol(t *testing.T) {
Copy link
Contributor

@ironcladlou ironcladlou Dec 19, 2018

Choose a reason for hiding this comment

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

Is this providing much more value than the unit test in this case?

I'm trying to think of how we could make it more black-box either here or in Origin... like, is the feature really "source IP is preserved on supported cloud environments" (PROXY being an implementation detail)?

Copy link
Contributor

@ironcladlou ironcladlou Dec 19, 2018

Choose a reason for hiding this comment

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

I don't want to block on it right now, just food for thought

Copy link
Contributor Author

@ramr ramr Dec 21, 2018

Choose a reason for hiding this comment

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

That makes sense - have to rebase anyway so will fix the test then. Probably in the next few days (too many tests + needle pricks today to be in a sane frame of mind to do this right now).

Copy link
Contributor Author

@ramr ramr Dec 22, 2018

Choose a reason for hiding this comment

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

Aah, we need a service to return back the header info. Plus there's already an extended test for router headers (in the origin repo) that checks the remote ip -- it is disabled currently - but will work to fix on it there.
Leaving this test here for now (we can at least verify that the proxy protocol works once we have all the pieces in place). Thx

Copy link
Contributor

@ironcladlou ironcladlou Jan 7, 2019

Choose a reason for hiding this comment

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

I'm fine to leave this and refactor the tests later. I'd rather have this test than no test!

Copy link
Contributor

@ironcladlou ironcladlou Jan 7, 2019

Choose a reason for hiding this comment

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

That wasn't an indictment of the quality of the test 😁

cl, ns, err := getClient()
if err != nil {
t.Fatal(err)
}

infraConfig := &configv1.Infrastructure{}
err = cl.Get(context.TODO(), types.NamespacedName{Name: "cluster"}, infraConfig)
if err != nil {
t.Fatalf("failed to get infrastructure config: %v", err)
}

if infraConfig.Status.Platform != configv1.AWSPlatform {
t.Skip("test skipped on non-aws platform")
return
}

ci := &ingressv1alpha1.ClusterIngress{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: ns, Name: "default"}, ci); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get default ClusterIngress: %v", err)
}

// Wait for the router deployment to exist.
deployment := &appsv1.Deployment{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: "openshift-ingress", Name: fmt.Sprintf("router-%s", ci.Name)}, deployment); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get default router deployment: %v", err)
}

// Ensure proxy protocol is enabled on the router deployment.
proxyProtocolEnabled := false
for _, v := range deployment.Spec.Template.Spec.Containers[0].Env {
if v.Name == "ROUTER_USE_PROXY_PROTOCOL" {
if val, err := strconv.ParseBool(v.Value); err == nil {
proxyProtocolEnabled = val
break
}
}
}

if !proxyProtocolEnabled {
t.Fatalf("expected router deployment to enable the PROXY protocol")
}

// Wait for the internal router service to exist.
internalService := &corev1.Service{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: "openshift-ingress", Name: fmt.Sprintf("router-internal-%s", ci.Name)}, internalService); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get internal router service: %v", err)
}

// TODO: Wait for interal router service selector bug to be fixed.
Copy link
Contributor

@Miciah Miciah Dec 22, 2018

Choose a reason for hiding this comment

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

#90 merged, so this TODO is stale, right?

Copy link
Contributor Author

@ramr ramr Dec 23, 2018

Choose a reason for hiding this comment

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

yeah but as per a comment from @ironcladlou previously I was debating on whether to do it here (or do it in origin as there's already a header test that checks for the *Forwarded* headers - that seems like a more complete blackbox e2e test).

Copy link
Contributor

@ironcladlou ironcladlou Jan 7, 2019

Choose a reason for hiding this comment

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

If you want to make a followup task in Jira for this test refactor we can merge using operator scoped tests in the interest of time.

// An alternative to test this would be to use an actual proxy protocol
// request to the internal router service.
// import "net"
// connection, err := net.Dial("tcp", internalService.Spec.ClusterIP)
// if err != nil {
// t.Fatalf("failed to connect to internal router service: %v", err)
// }
// defer connection.Close()

// req := []byte("LOCAL\r\nGET / HTTP/1.1\r\nHost: non.existent.test\r\n\r\n")
// req = []byte(fmt.Sprintf("PROXY TCP4 10.9.8.7 %s 54321 443\r\nGET / HTTP/1.1\r\nHost: non.existent.test\r\n\r\n", internalService.Spec.ClusterIP))
// connection.Write(req)
// data := make([]byte, 4096)
// if _, err := connection.Read(data); err != nil {
// t.Fatalf("failed to read response from internal router service: %v", err)
// } else {
// check response is a http response 503.
// }

}

// TODO: Use manifest factory to build expectations
// TODO: Find a way to do this test without mutating the default ingress?
func TestClusterIngressUpdate(t *testing.T) {
Expand Down