[Feat] dnsTarget - fallback to shoot domain (#55)

* Added controller.dnsTarget to chart * dnsTarget defaulting using cluster domain * Helm chart minor version bumped * [Misc] chart: readme updated --------- Co-authored-by: Pavan <[email protected]>
SAP · Mar 7, 2024 · a4b2c22 · a4b2c22
1 parent c4b2398
commit a4b2c22
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 42 deletions.
diff --git a/chart/Chart.yaml b/chart/Chart.yaml
@@ -1,5 +1,5 @@
 apiVersion: v2
 description: Helm chart to deploy CAP Operator https://sap.github.io/cap-operator/
 name: cap-operator
-version: 0.2.0
-appVersion: 0.2.0
+version: 0.3.0
+appVersion: 0.3.0
diff --git a/chart/README.md b/chart/README.md
@@ -1,6 +1,6 @@
 # cap-operator
 
-![Version: 0.2.0](https://img.shields.io/badge/Version-0.2.0-informational?style=flat-square) ![AppVersion: 0.2.0](https://img.shields.io/badge/AppVersion-0.2.0-informational?style=flat-square)
+![Version: 0.3.0](https://img.shields.io/badge/Version-0.3.0-informational?style=flat-square) ![AppVersion: 0.3.0](https://img.shields.io/badge/AppVersion-0.3.0-informational?style=flat-square)
 
 Helm chart to deploy CAP Operator https://sap.github.io/cap-operator/
 
@@ -33,6 +33,7 @@ Helm chart to deploy CAP Operator https://sap.github.io/cap-operator/
 | controller.resources.limits.cpu | float | `0.2` | CPU limit |
 | controller.resources.requests.memory | string | `"50Mi"` | Memory request |
 | controller.resources.requests.cpu | float | `0.02` | CPU request |
+| controller.dnsTarget | string | `""` | The dns target mentioned on the public ingress gateway service used in the cluster |
 | subscriptionServer.replicas | int | `1` | Replicas |
 | subscriptionServer.image.repository | string | `"ghcr.io/sap/cap-operator/server"` | Image repository |
 | subscriptionServer.image.tag | string | `""` | Image tag |

diff --git a/chart/templates/controller-deployment.yaml b/chart/templates/controller-deployment.yaml
@@ -64,4 +64,8 @@ spec:
             value: {{ .Capabilities.APIVersions.Has "dns.gardener.cloud/v1alpha1" | ternary "gardener" "kubernetes" }}
           - name: MTX_JOB_IMAGE
             value: "ghcr.io/sap/cap-operator/mtx-job"
+          {{- if .Values.controller.dnsTarget }}
+          - name: DNS_TARGET
+            value: {{ .Values.controller.dnsTarget }}
+          {{- end }}
       serviceAccountName: {{.Release.Name}}-controller
diff --git a/chart/values.yaml b/chart/values.yaml
@@ -55,6 +55,8 @@ controller:
       memory: 50Mi
       # -- CPU request
       cpu: 0.02
+  # -- The dns target mentioned on the public ingress gateway service used in the cluster
+  dnsTarget: ""
 
 subscriptionServer:
   # -- Replicas

diff --git a/internal/transformer/transformer.go b/internal/transformer/transformer.go
@@ -14,6 +14,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	apitypes "k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	componentoperatorruntimetypes "github.com/sap/component-operator-runtime/pkg/types"
@@ -30,6 +31,8 @@ type transformer struct {
 	client client.Client
 }
 
+var setupLog = ctrl.Log.WithName("transformer")
+
 func NewParameterTransformer(client client.Client) *transformer {
 	return &transformer{client: client}
 }
@@ -54,30 +57,52 @@ func replaceAsteriskDNSTarget(dnsTarget string) string {
 }
 
 func (t *transformer) fillDNSTarget(parameters map[string]any) error {
-	// get DNSTarget
 	subscriptionServer := parameters["subscriptionServer"].(map[string]interface{})
-	if parameters["dnsTarget"] != nil { // already filled in CRO
-		subscriptionServer["dnsTarget"] = replaceAsteriskDNSTarget(parameters["dnsTarget"].(string))
+	// DNSTarget given - use it
+	if parameters["dnsTarget"] != nil {
+		replacedDnsTarget := replaceAsteriskDNSTarget(parameters["dnsTarget"].(string))
+
+		// set the dnsTarget in the subscriptionServer
+		subscriptionServer["dnsTarget"] = replacedDnsTarget
+		// set the dnsTarget in the controller
+		parameters["controller"] = map[string]interface{}{
+			"dnsTarget": replacedDnsTarget,
+		}
+
 		delete(parameters, "dnsTarget")
 		return nil
 	}
 
 	// DNSTarget not given - read it from the load balancer service in istio namespace
 	if parameters["ingressGatewayLabels"] == nil {
-		return fmt.Errorf("cannot get dnsTarget; provide either dnsTarget or ingressGatewayLabels in the CRO")
+		return fmt.Errorf("unable to retrieve dnsTarget; please specify either dnsTarget or ingressGatewayLabels in the CAP Operator CRO")
 	}
 
-	dnsTarget, err := t.getDNSTarget(parameters["ingressGatewayLabels"].([]interface{}))
+	dnsTarget, err := t.getDNSTargetUsingIngressGatewayLabels(parameters["ingressGatewayLabels"].([]interface{}))
 	if err != nil {
-		return err
+		setupLog.Info("dnsTarget not found using ingressGatewayLabels", "error", err)
+
+		// default the dnsTarget to the x.<cluster-domain>
+		dnsTarget, err = t.getDomain("x")
+		if err != nil {
+			return err
+		}
+
+		setupLog.Info("defaulting dnsTarget to " + dnsTarget)
 	}
 
-	subscriptionServer["dnsTarget"] = replaceAsteriskDNSTarget(dnsTarget)
+	replacedDnsTarget := replaceAsteriskDNSTarget(dnsTarget)
+	// set the dnsTarget in the subscriptionServer
+	subscriptionServer["dnsTarget"] = replacedDnsTarget
+	// set the dnsTarget in the controller
+	parameters["controller"] = map[string]interface{}{
+		"dnsTarget": replacedDnsTarget,
+	}
 	delete(parameters, "ingressGatewayLabels")
 	return nil
 }
 
-func (t *transformer) getDNSTarget(ingressGatewayLabels []interface{}) (dnsTarget string, err error) {
+func (t *transformer) getDNSTargetUsingIngressGatewayLabels(ingressGatewayLabels []interface{}) (dnsTarget string, err error) {
 
 	ctx := context.TODO()
 
@@ -125,7 +150,6 @@ func (t *transformer) getDNSTarget(ingressGatewayLabels []interface{}) (dnsTarge
 		return "", fmt.Errorf("ingress gateway service not annotated with dns target name")
 	}
 
-	// Return ingress Gateway info (Namespace and DNS target)
 	return dnsTarget, nil
 }
 

diff --git a/internal/transformer/transformer_test.go b/internal/transformer/transformer_test.go
@@ -16,41 +16,69 @@ import (
 
 func TestTransformer(t *testing.T) {
 	tests := []struct {
-		name                       string
-		dnsTargetFilled            bool
-		ingressGatewayLabelsFilled bool
-		longDomain                 bool
-		expectError                bool
+		name                               string
+		dnsTargetFilled                    bool
+		ingressGatewayLabelsFilled         bool
+		longDomain                         bool
+		expectError                        bool
+		withoutIngressGatewaySvcAnnotation bool
 	}{
 		{
-			name:                       "Test with dnsTarget and without ingress gateway labels",
+			name:                       "With dnsTarget and without ingress gateway labels",
 			dnsTargetFilled:            true,
 			ingressGatewayLabelsFilled: false,
 			expectError:                false,
 		},
 		{
-			name:                       "Test without dnsTarget and with ingress gateway labels",
+			name:                       "Without dnsTarget and with ingress gateway labels",
 			dnsTargetFilled:            false,
 			ingressGatewayLabelsFilled: true,
 			expectError:                false,
 		},
 		{
-			name:                       "Test without dnsTarget and ingress gateway labels",
+			name:                       "Without dnsTarget and ingress gateway labels",
 			dnsTargetFilled:            false,
 			ingressGatewayLabelsFilled: false,
 			expectError:                true,
 		},
 		{
-			name:                       "Test with more than 64 character domain",
+			name:                       "With more than 64 character domain",
 			ingressGatewayLabelsFilled: true,
 			longDomain:                 true,
 			expectError:                true,
 		},
+		{
+			name:                               "Without annotation in ingress gateway labels",
+			ingressGatewayLabelsFilled:         true,
+			longDomain:                         false,
+			withoutIngressGatewaySvcAnnotation: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 
 			clientBuilder := fake.NewClientBuilder()
+
+			istioSvc := &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "istioingress-gateway",
+					Namespace: "istio-system",
+				},
+				Spec: corev1.ServiceSpec{
+					Type: corev1.ServiceTypeLoadBalancer,
+					Selector: map[string]string{
+						"istio": "ingress",
+						"app":   "istio-ingress",
+					},
+				},
+			}
+
+			if !tt.withoutIngressGatewaySvcAnnotation {
+				istioSvc.ObjectMeta.Annotations = map[string]string{
+					"dns.gardener.cloud/dnsnames": "public-ingress.some.cluster.sap",
+				}
+			}
+
 			clientBuilder.WithObjects(
 				&corev1.ConfigMap{
 					TypeMeta: metav1.TypeMeta{
@@ -75,31 +103,14 @@ func TestTransformer(t *testing.T) {
 						},
 					},
 				},
-				&corev1.Service{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "istioingress-gateway",
-						Namespace: "istio-system",
-						Annotations: map[string]string{
-							"dns.gardener.cloud/dnsnames": "public-ingress.some.cluster.sap",
-						},
-					},
-					Spec: corev1.ServiceSpec{
-						Type: corev1.ServiceTypeLoadBalancer,
-						Selector: map[string]string{
-							"istio": "ingress",
-							"app":   "istio-ingress",
-						},
-					},
-				})
+				istioSvc)
 
 			kubeClient := clientBuilder.Build()
 
 			transformer := NewParameterTransformer(kubeClient)
 
 			parameter := make(map[string]interface{})
 
-			parameter["spec"] = map[string]interface{}{}
-
 			parameter["subscriptionServer"] = map[string]interface{}{}
 			subscriptionServer := parameter["subscriptionServer"].(map[string]interface{})
 
@@ -138,12 +149,23 @@ func TestTransformer(t *testing.T) {
 				t.Log(err)
 				return
 			}
+
+			var expectedDnsTarget string
+			if tt.withoutIngressGatewaySvcAnnotation {
+				expectedDnsTarget = "x.some.cluster.sap"
+			} else {
+				expectedDnsTarget = "public-ingress.some.cluster.sap"
+			}
+
 			transformedParametersMap := transformedParameters.ToUnstructured()
-			if transformedParametersMap["subscriptionServer"].(map[string]interface{})["dnsTarget"].(string) != "public-ingress.some.cluster.sap" {
-				t.Error("unexpected value returned")
+			if transformedParametersMap["subscriptionServer"].(map[string]interface{})["dnsTarget"].(string) != expectedDnsTarget {
+				t.Error("unexpected value returned for subscriptionServer.dnsTarget")
 			}
 			if transformedParametersMap["subscriptionServer"].(map[string]interface{})["domain"].(string) != "cop.some.cluster.sap" {
-				t.Error("unexpected value returned")
+				t.Error("unexpected value returned for subscriptionServer.domain")
+			}
+			if transformedParametersMap["controller"].(map[string]interface{})["dnsTarget"].(string) != expectedDnsTarget {
+				t.Error("unexpected value returned for controller.dnsTarget")
 			}
 		})
 	}