Merge pull request #345 from rramkumar1/iap-cdn-changes

Modify IAP + CDN support to not touch settings if section in spec is missing
kubernetes · Jun 18, 2018 · af557c2 · af557c2
2 parents 3521550 + 400ef93
commit af557c2
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 15 deletions.
diff --git a/pkg/backends/features/cdn.go b/pkg/backends/features/cdn.go
@@ -29,6 +29,9 @@ import (
 // and applies it to the BackendService. It returns true if there were existing
 // settings on the BackendService that were overwritten.
 func EnsureCDN(sp utils.ServicePort, be *composite.BackendService) bool {
+	if sp.BackendConfig.Spec.Cdn == nil {
+		return false
+	}
 	beTemp := &composite.BackendService{}
 	applyCDNSettings(sp, beTemp)
 	if !reflect.DeepEqual(beTemp.CdnPolicy, be.CdnPolicy) || beTemp.EnableCDN != be.EnableCDN {
@@ -59,9 +62,6 @@ func applyCDNSettings(sp utils.ServicePort, be *composite.BackendService) {
 
 // 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 == nil {
-		beConfig.Spec.Cdn = &backendconfigv1beta1.CDNConfig{}
-	}
 	if beConfig.Spec.Cdn.CachePolicy == nil {
 		beConfig.Spec.Cdn.CachePolicy = &backendconfigv1beta1.CacheKeyPolicy{}
 	}

diff --git a/pkg/backends/features/cdn_test.go b/pkg/backends/features/cdn_test.go
@@ -31,6 +31,46 @@ func TestEnsureCDN(t *testing.T) {
 		be             *composite.BackendService
 		updateExpected bool
 	}{
+		{
+			desc: "cdn setting are missing from spec, no update needed",
+			sp: utils.ServicePort{
+				BackendConfig: &backendconfigv1beta1.BackendConfig{
+					Spec: backendconfigv1beta1.BackendConfigSpec{
+						Cdn: nil,
+					},
+				},
+			},
+			be: &composite.BackendService{
+				EnableCDN: true,
+				CdnPolicy: &composite.BackendServiceCdnPolicy{
+					CacheKeyPolicy: &composite.CacheKeyPolicy{
+						IncludeHost: true,
+					},
+				},
+			},
+			updateExpected: false,
+		},
+		{
+			desc: "cache policies are missing from spec, update needed",
+			sp: utils.ServicePort{
+				BackendConfig: &backendconfigv1beta1.BackendConfig{
+					Spec: backendconfigv1beta1.BackendConfigSpec{
+						Cdn: &backendconfigv1beta1.CDNConfig{
+							Enabled: true,
+						},
+					},
+				},
+			},
+			be: &composite.BackendService{
+				EnableCDN: true,
+				CdnPolicy: &composite.BackendServiceCdnPolicy{
+					CacheKeyPolicy: &composite.CacheKeyPolicy{
+						IncludeHost: true,
+					},
+				},
+			},
+			updateExpected: true,
+		},
 		{
 			desc: "settings are identical, no update needed",
 			sp: utils.ServicePort{

diff --git a/pkg/backends/features/iap.go b/pkg/backends/features/iap.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 
 	"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"
 )
@@ -30,6 +29,9 @@ import (
 // and applies it to the BackendService if it is stale. It returns true
 // if there were existing settings on the BackendService that were overwritten.
 func EnsureIAP(sp utils.ServicePort, be *composite.BackendService) bool {
+	if sp.BackendConfig.Spec.Iap == nil {
+		return false
+	}
 	beTemp := &composite.BackendService{}
 	applyIAPSettings(sp, beTemp)
 	// We need to compare the SHA256 of the client secret instead of the client secret itself
@@ -48,20 +50,9 @@ func EnsureIAP(sp utils.ServicePort, be *composite.BackendService) bool {
 // made to actually persist the changes.
 func applyIAPSettings(sp utils.ServicePort, be *composite.BackendService) {
 	beConfig := sp.BackendConfig
-	setIAPDefaults(beConfig)
 	// Apply the boolean switch
 	be.Iap = &composite.BackendServiceIAP{Enabled: beConfig.Spec.Iap.Enabled}
 	// Apply the OAuth credentials
 	be.Iap.Oauth2ClientId = beConfig.Spec.Iap.OAuthClientCredentials.ClientID
 	be.Iap.Oauth2ClientSecret = beConfig.Spec.Iap.OAuthClientCredentials.ClientSecret
 }
-
-// setIAPDefaults initializes any nil pointers in IAP configuration which ensures that there are defaults for missing sub-types.
-func setIAPDefaults(beConfig *backendconfigv1beta1.BackendConfig) {
-	if beConfig.Spec.Iap == nil {
-		beConfig.Spec.Iap = &backendconfigv1beta1.IAPConfig{}
-	}
-	if beConfig.Spec.Iap.OAuthClientCredentials == nil {
-		beConfig.Spec.Iap.OAuthClientCredentials = &backendconfigv1beta1.OAuthClientCredentials{}
-	}
-}
diff --git a/pkg/backends/features/iap_test.go b/pkg/backends/features/iap_test.go
@@ -33,6 +33,24 @@ func TestEnsureIAP(t *testing.T) {
 		be             *composite.BackendService
 		updateExpected bool
 	}{
+		{
+			desc: "iap settings are missing from spec, no update needed",
+			sp: utils.ServicePort{
+				BackendConfig: &backendconfigv1beta1.BackendConfig{
+					Spec: backendconfigv1beta1.BackendConfigSpec{
+						Iap: nil,
+					},
+				},
+			},
+			be: &composite.BackendService{
+				Iap: &composite.BackendServiceIAP{
+					Enabled:                  true,
+					Oauth2ClientId:           "foo",
+					Oauth2ClientSecretSha256: fmt.Sprintf("%x", sha256.Sum256([]byte("bar"))),
+				},
+			},
+			updateExpected: false,
+		},
 		{
 			desc: "settings are identical, no update needed",
 			sp: utils.ServicePort{