From c3ef566eb1ca1872a2f39e2e412a13530f465600 Mon Sep 17 00:00:00 2001
From: Rohit Ramkumar <rramkumar@google.com>
Date: Tue, 15 Jan 2019 13:08:09 -0800
Subject: [PATCH] [BackendConfig] CDN default cache key policy should be true
 instead of false

---
 pkg/backends/features/cdn.go      | 29 +++++++++++++----------------
 pkg/backends/features/cdn_test.go |  4 ++--
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/pkg/backends/features/cdn.go b/pkg/backends/features/cdn.go
index 541c2b6f79..09ed4f7300 100644
--- a/pkg/backends/features/cdn.go
+++ b/pkg/backends/features/cdn.go
@@ -20,7 +20,6 @@ import (
 	"reflect"
 
 	"github.com/golang/glog"
-	backendconfigv1beta1 "k8s.io/ingress-gce/pkg/apis/backendconfig/v1beta1"
 	"k8s.io/ingress-gce/pkg/composite"
 	"k8s.io/ingress-gce/pkg/utils"
 )
@@ -34,7 +33,8 @@ func EnsureCDN(sp utils.ServicePort, be *composite.BackendService) bool {
 	}
 	beTemp := &composite.BackendService{}
 	applyCDNSettings(sp, beTemp)
-	if !reflect.DeepEqual(beTemp.CdnPolicy, be.CdnPolicy) || beTemp.EnableCDN != be.EnableCDN {
+	// Only compare CdnPolicy if it was specified.
+	if (beTemp.CdnPolicy != nil && !reflect.DeepEqual(beTemp.CdnPolicy, be.CdnPolicy)) || beTemp.EnableCDN != be.EnableCDN {
 		applyCDNSettings(sp, be)
 		glog.V(2).Infof("Updated CDN settings for service %v/%v.", sp.ID.Service.Namespace, sp.ID.Service.Name)
 		return true
@@ -47,22 +47,19 @@ func EnsureCDN(sp utils.ServicePort, be *composite.BackendService) bool {
 // made to actually persist the changes.
 func applyCDNSettings(sp utils.ServicePort, be *composite.BackendService) {
 	beConfig := sp.BackendConfig
-	setCDNDefaults(beConfig)
 	// Apply the boolean switch
 	be.EnableCDN = beConfig.Spec.Cdn.Enabled
-	// Apply the cache key policies
 	cacheKeyPolicy := beConfig.Spec.Cdn.CachePolicy
-	be.CdnPolicy = &composite.BackendServiceCdnPolicy{CacheKeyPolicy: &composite.CacheKeyPolicy{}}
-	be.CdnPolicy.CacheKeyPolicy.IncludeHost = cacheKeyPolicy.IncludeHost
-	be.CdnPolicy.CacheKeyPolicy.IncludeProtocol = cacheKeyPolicy.IncludeProtocol
-	be.CdnPolicy.CacheKeyPolicy.IncludeQueryString = cacheKeyPolicy.IncludeQueryString
-	be.CdnPolicy.CacheKeyPolicy.QueryStringBlacklist = cacheKeyPolicy.QueryStringBlacklist
-	be.CdnPolicy.CacheKeyPolicy.QueryStringWhitelist = cacheKeyPolicy.QueryStringWhitelist
-}
-
-// setCDNDefaults initializes any nil pointers in CDN configuration which ensures that there are defaults for missing sub-types.
-func setCDNDefaults(beConfig *backendconfigv1beta1.BackendConfig) {
-	if beConfig.Spec.Cdn.CachePolicy == nil {
-		beConfig.Spec.Cdn.CachePolicy = &backendconfigv1beta1.CacheKeyPolicy{}
+	// Apply the cache key policies if the BackendConfig contains them.
+	if cacheKeyPolicy != nil {
+		be.CdnPolicy = &composite.BackendServiceCdnPolicy{CacheKeyPolicy: &composite.CacheKeyPolicy{}}
+		be.CdnPolicy.CacheKeyPolicy.IncludeHost = cacheKeyPolicy.IncludeHost
+		be.CdnPolicy.CacheKeyPolicy.IncludeProtocol = cacheKeyPolicy.IncludeProtocol
+		be.CdnPolicy.CacheKeyPolicy.IncludeQueryString = cacheKeyPolicy.IncludeQueryString
+		be.CdnPolicy.CacheKeyPolicy.QueryStringBlacklist = cacheKeyPolicy.QueryStringBlacklist
+		be.CdnPolicy.CacheKeyPolicy.QueryStringWhitelist = cacheKeyPolicy.QueryStringWhitelist
 	}
+	// Note that upon creation of a BackendServices, the fields 'IncludeHost',
+	// 'IncludeProtocol' and 'IncludeQueryString' all default to true if not
+	// explicitly specified.
 }
diff --git a/pkg/backends/features/cdn_test.go b/pkg/backends/features/cdn_test.go
index 1535e1cbe0..98ad7631a1 100644
--- a/pkg/backends/features/cdn_test.go
+++ b/pkg/backends/features/cdn_test.go
@@ -51,7 +51,7 @@ func TestEnsureCDN(t *testing.T) {
 			updateExpected: false,
 		},
 		{
-			desc: "cache policies are missing from spec, update needed",
+			desc: "cache policies are missing from spec, no update needed",
 			sp: utils.ServicePort{
 				BackendConfig: &backendconfigv1beta1.BackendConfig{
 					Spec: backendconfigv1beta1.BackendConfigSpec{
@@ -69,7 +69,7 @@ func TestEnsureCDN(t *testing.T) {
 					},
 				},
 			},
-			updateExpected: true,
+			updateExpected: false,
 		},
 		{
 			desc: "settings are identical, no update needed",