Skip to content
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.

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

Implement SslPolicies for TargetHttpsProxy #1019

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
169 changes: 168 additions & 1 deletion pkg/loadbalancers/loadbalancers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"net/http"
"reflect"
"strconv"
"strings"
"testing"
Expand All @@ -34,6 +35,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/ingress-gce/pkg/annotations"
frontendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/frontendconfig/v1beta1"
"k8s.io/ingress-gce/pkg/composite"
"k8s.io/ingress-gce/pkg/events"
"k8s.io/ingress-gce/pkg/flags"
Expand Down Expand Up @@ -86,7 +88,7 @@ func newTestJig(t *testing.T) *testJig {
if err != nil {
return &googleapi.Error{
Code: http.StatusNotFound,
Message: fmt.Sprintf("Key: %s was not found in UrlMaps", key.String()),
Message: fmt.Sprintf("Key: %s was not found in TargetHttpsProxies", key.String()),
}
}

Expand All @@ -97,6 +99,18 @@ func newTestJig(t *testing.T) *testJig {
tp.SslCertificates = request.SslCertificates
return nil
}
mockGCE.MockTargetHttpsProxies.SetSslPolicyHook = func(ctx context.Context, key *meta.Key, ref *compute.SslPolicyReference, proxies *cloud.MockTargetHttpsProxies) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this hook special? Can it be added to k8s_cloud_provider mock.go instead?

If its too much work to re-vendor then I suppose we can have this here but would be good to upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I do this in a follow up and add a TODO here? Re-vendoring always takes like a couple days to get all the approvals.

tps, err := proxies.Get(ctx, key)
fmt.Printf("tps = %+v, err = %v\n\n", tps, err)
if err != nil {
return &googleapi.Error{
Code: http.StatusNotFound,
Message: fmt.Sprintf("Key: %s was not found in TargetHttpsProxies", key.String()),
}
}
tps.SslPolicy = ref.SslPolicy
return nil
}
mockGCE.MockGlobalForwardingRules.InsertHook = InsertGlobalForwardingRuleHook

ing := newIngress()
Expand Down Expand Up @@ -973,6 +987,159 @@ func TestCreateBothLoadBalancers(t *testing.T) {
}
}

// Test setting frontendconfig Ssl policy
func TestFrontendConfigSslPolicy(t *testing.T) {
flags.F.EnableFrontendConfig = true
defer func() { flags.F.EnableFrontendConfig = false }()

j := newTestJig(t)

gceUrlMap := utils.NewGCEURLMap()
gceUrlMap.DefaultBackend = &utils.ServicePort{NodePort: 31234, BackendNamer: j.namer}
gceUrlMap.PutPathRulesForHost("bar.example.com", []utils.PathRule{{Path: "/bar", Backend: utils.ServicePort{NodePort: 30000, BackendNamer: j.namer}}})
lbInfo := &L7RuntimeInfo{
AllowHTTP: false,
TLS: []*TLSCerts{createCert("key", "cert", "name")},
UrlMap: gceUrlMap,
Ingress: newIngress(),
FrontendConfig: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("test-policy")}},
}

l7, err := j.pool.Ensure(lbInfo)
if err != nil {
t.Fatalf("j.pool.Ensure(%v) = %v, want nil", lbInfo, err)
}

tpsName := l7.tps.Name
tps, _ := composite.GetTargetHttpsProxy(j.fakeGCE, meta.GlobalKey(tpsName), meta.VersionGA)

resourceID, err := cloud.ParseResourceURL(tps.SslPolicy)
if err != nil {
t.Errorf("ParseResourceURL(%+v) = %v, want nil", tps.SslPolicy, err)
}

path := resourceID.ResourcePath()
want := "global/sslPolicies/test-policy"

if path != want {
t.Errorf("tps ssl policy = %q, want %q", path, want)
}
}

func TestGetSslPolicyLink(t *testing.T) {
t.Parallel()
j := newTestJig(t)

testCases := []struct {
desc string
fc *frontendconfigv1beta1.FrontendConfig
want *string
}{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need a case for frontend config with empty string ssl policy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
desc: "Empty frontendconfig",
fc: nil,
want: nil,
},
{
desc: "frontendconfig with no ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{}},
want: nil,
},
{
desc: "frontendconfig with ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("test-policy")}},
want: utils.NewStringPointer("global/sslPolicies/test-policy"),
},
{
desc: "frontendconfig with empty string ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("")}},
want: utils.NewStringPointer(""),
},
}

for _, tc := range testCases {
l7 := L7{runtimeInfo: &L7RuntimeInfo{FrontendConfig: tc.fc}, cloud: j.fakeGCE, scope: meta.Global}
result, err := l7.getSslPolicyLink()
if err != nil {
t.Errorf("desc: %q, l7.getSslPolicyLink() = %v, want nil", tc.desc, err)
}

if !reflect.DeepEqual(result, tc.want) {
t.Errorf("desc: %q, l7.getSslPolicyLink() = %v, want %+v", tc.desc, result, tc.want)
}
}
}

func TestEnsureSslPolicy(t *testing.T) {
t.Parallel()
j := newTestJig(t)

testCases := []struct {
desc string
fc *frontendconfigv1beta1.FrontendConfig
proxy *composite.TargetHttpsProxy
want string
}{
{
desc: "Empty frontendconfig",
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-0"},
fc: nil,
want: "",
},
{
desc: "frontendconfig with no ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{}},
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-1"},
want: "",
},
{
desc: "frontendconfig with ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("test-policy")}},
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-2"},
want: "global/sslPolicies/test-policy",
},
{
desc: "proxy with different ssl policy",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a duplicate of the test case below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("test-policy")}},
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-3", SslPolicy: "global/sslPolicies/wrong-policy"},
want: "global/sslPolicies/test-policy",
},
{
desc: "proxy with ssl policy and frontend config policy is nil",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: nil}},
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-4", SslPolicy: "global/sslPolicies/test-policy"},
want: "global/sslPolicies/test-policy",
},
{
desc: "remove ssl policy",
fc: &frontendconfigv1beta1.FrontendConfig{Spec: frontendconfigv1beta1.FrontendConfigSpec{SslPolicy: utils.NewStringPointer("")}},
proxy: &composite.TargetHttpsProxy{Name: "test-proxy-5", SslPolicy: "global/sslPolicies/wrong-policy"},
want: "",
},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You need a case where a proxy has a policy and then the frontend config policy string is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

for _, tc := range testCases {
key := meta.GlobalKey(tc.proxy.Name)
if err := composite.CreateTargetHttpsProxy(j.fakeGCE, key, tc.proxy); err != nil {
t.Error(err)
}
l7 := L7{runtimeInfo: &L7RuntimeInfo{FrontendConfig: tc.fc}, cloud: j.fakeGCE, scope: meta.Global}

if err := l7.ensureSslPolicy(tc.proxy); err != nil {
t.Errorf("desc: %q, l7.ensureSslPolicy() = %v, want nil", tc.desc, err)
}

result, err := composite.GetTargetHttpsProxy(j.fakeGCE, key, meta.VersionGA)
if err != nil {
t.Error(err)
}

if result.SslPolicy != tc.want {
t.Errorf("desc: %q, want %q, got %q", tc.desc, tc.want, result.SslPolicy)
}
}
}

// verifyURLMap gets the created URLMap and compares it against an expected one.
func verifyURLMap(t *testing.T, j *testJig, feNamer namer_util.IngressFrontendNamer, wantGCEURLMap *utils.GCEURLMap) {
t.Helper()
Expand Down
66 changes: 66 additions & 0 deletions pkg/loadbalancers/target_proxies.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package loadbalancers
import (
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
"k8s.io/ingress-gce/pkg/composite"
"k8s.io/ingress-gce/pkg/flags"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/namer"
"k8s.io/klog"
Expand Down Expand Up @@ -133,6 +134,12 @@ func (l *L7) checkHttpsProxy() (err error) {
return err
}

if flags.F.EnableFrontendConfig {
if err := l.ensureSslPolicy(newProxy); err != nil {
return err
}
}

key, err = l.CreateKey(proxyName)
if err != nil {
return err
Expand Down Expand Up @@ -171,8 +178,14 @@ func (l *L7) checkHttpsProxy() (err error) {
if err := composite.SetSslCertificateForTargetHttpsProxy(l.cloud, key, proxy, sslCertURLs); err != nil {
return err
}
}

if flags.F.EnableFrontendConfig {
if err := l.ensureSslPolicy(proxy); err != nil {
return err
}
}

l.tps = proxy
return nil
}
Expand All @@ -190,3 +203,56 @@ func (l *L7) getSslCertLinkInUse() ([]string, error) {

return proxy.SslCertificates, nil
}

// ensureSslPolicy ensures that the SslPolicy described in the frontendconfig is
// properly applied to the proxy.
func (l *L7) ensureSslPolicy(proxy *composite.TargetHttpsProxy) error {
policyLink, err := l.getSslPolicyLink()
if err != nil {
return err
}

if policyLink != nil && !utils.EqualResourceIDs(*policyLink, proxy.SslPolicy) {
key, err := l.CreateKey(proxy.Name)
if err != nil {
return err
}
if err := composite.SetSslPolicyForTargetHttpsProxy(l.cloud, key, proxy, *policyLink); err != nil {
return err
}
}
return nil
}

// getSslPolicyLink returns the ref to the ssl policy that is described by the
// frontend config. Since Ssl Policy is a *string, there are three possible I/O situations
// 1) policy is nil -> this returns nil
// 2) policy is an empty string -> this returns an empty string
// 3) policy is non-empty -> this constructs the resourcce path and returns it
func (l *L7) getSslPolicyLink() (*string, error) {
var link string

if l.runtimeInfo.FrontendConfig == nil {
return nil, nil
}

policyName := l.runtimeInfo.FrontendConfig.Spec.SslPolicy
if policyName == nil {
return nil, nil
}
if *policyName == "" {
return &link, nil
}

key, err := l.CreateKey(*policyName)
if err != nil {
return nil, err
}
resourceID := cloud.ResourceID{
Resource: "sslPolicies",
Key: key,
}
resID := resourceID.ResourcePath()

return &resID, nil
}
5 changes: 5 additions & 0 deletions pkg/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,3 +439,8 @@ func IsLegacyL4ILBService(svc *api_v1.Service) bool {
}
return false
}

// NewStringPointer returns a pointer to the provided string literal
func NewStringPointer(s string) *string {
return &s
}