From 8595a0d1cc0200b58989dbde495731826a09f38f Mon Sep 17 00:00:00 2001 From: i325261 Date: Tue, 26 Sep 2023 09:33:28 +0200 Subject: [PATCH 1/4] moved dnsTarget out of subscriptionServer --- api/v1alpha1/capoperator_types.go | 17 ++++++---- api/v1alpha1/zz_generated.deepcopy.go | 16 ++++++---- config/busola/capoperator_configMap.yaml | 12 +------ .../operator.sme.sap.com_capoperators.yaml | 32 +++++++++++-------- .../operator_v1alpha1_capoperator.yaml | 6 ++-- internal/transformer/transformer.go | 27 +++++++--------- internal/transformer/transformer_test.go | 16 +++++++--- 7 files changed, 66 insertions(+), 60 deletions(-) diff --git a/api/v1alpha1/capoperator_types.go b/api/v1alpha1/capoperator_types.go index 6c1b963..c577516 100644 --- a/api/v1alpha1/capoperator_types.go +++ b/api/v1alpha1/capoperator_types.go @@ -15,7 +15,6 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) -// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. //+kubebuilder:object:root=true //+kubebuilder:subresource:status @@ -39,18 +38,22 @@ type CAPOperatorStatus struct { type CAPOperatorSpec struct { // SubscriptionServer info SubscriptionServer SubscriptionServer `json:"subscriptionServer"` - // IngressGatewayLabels - IngressGatewayLabels IngressGatewayLabels `json:"ingressGatewayLabels"` + // +kubebuilder:validation:Pattern=^[a-z0-9-.]*$ + // Public ingress URL for the cluster Load Balancer + DNSTarget string `json:"dnsTarget,omitempty"` + // +kubebuilder:validation:MinItems=1 + // Labels used to identify the istio ingress-gateway component and its corresponding namespace. Usually {"app":"istio-ingressgateway","istio":"ingressgateway"} + IngressGatewayLabels []NameValue `json:"ingressGatewayLabels,omitempty"` } type SubscriptionServer struct { - DNSTarget string `json:"dnsTarget,omitempty"` Subdomain string `json:"subDomain"` } -type IngressGatewayLabels struct { - Istio string `json:"istio"` - App string `json:"app"` +// Generic Name/Value configuration +type NameValue struct { + Name string `json:"name"` + Value string `json:"value"` } var _ component.Component = &CAPOperator{} diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 5c4b21c..997e8ac 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -18,7 +18,7 @@ func (in *CAPOperator) DeepCopyInto(out *CAPOperator) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) in.Status.DeepCopyInto(&out.Status) } @@ -76,7 +76,11 @@ func (in *CAPOperatorList) DeepCopyObject() runtime.Object { func (in *CAPOperatorSpec) DeepCopyInto(out *CAPOperatorSpec) { *out = *in out.SubscriptionServer = in.SubscriptionServer - out.IngressGatewayLabels = in.IngressGatewayLabels + if in.IngressGatewayLabels != nil { + in, out := &in.IngressGatewayLabels, &out.IngressGatewayLabels + *out = make([]NameValue, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CAPOperatorSpec. @@ -106,16 +110,16 @@ func (in *CAPOperatorStatus) DeepCopy() *CAPOperatorStatus { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *IngressGatewayLabels) DeepCopyInto(out *IngressGatewayLabels) { +func (in *NameValue) DeepCopyInto(out *NameValue) { *out = *in } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new IngressGatewayLabels. -func (in *IngressGatewayLabels) DeepCopy() *IngressGatewayLabels { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NameValue. +func (in *NameValue) DeepCopy() *NameValue { if in == nil { return nil } - out := new(IngressGatewayLabels) + out := new(NameValue) in.DeepCopyInto(out) return out } diff --git a/config/busola/capoperator_configMap.yaml b/config/busola/capoperator_configMap.yaml index 2b6c569..d87499b 100644 --- a/config/busola/capoperator_configMap.yaml +++ b/config/busola/capoperator_configMap.yaml @@ -32,19 +32,9 @@ data: required: true name: CAP Operator Subscription Server Subdomain - simple: true - path: spec.subscriptionServer.dnsTarget + path: spec.dnsTarget required: false name: CAP Operator DNS Target - - simple: true - path: spec.ingressGatewayLabels.istio - required: false - name: Istio ingress gateway label - istio - placeholder: ingressgateway - - simple: true - path: spec.ingressGatewayLabels.app - required: false - name: Istio ingress gateway label - app - placeholder: istio-ingressgateway general: | resource: diff --git a/config/crd/operator.sme.sap.com_capoperators.yaml b/config/crd/operator.sme.sap.com_capoperators.yaml index 776d302..21a29ed 100644 --- a/config/crd/operator.sme.sap.com_capoperators.yaml +++ b/config/crd/operator.sme.sap.com_capoperators.yaml @@ -38,29 +38,35 @@ spec: spec: description: CAPOperatorSpec defines the desired state of CAPOperator properties: + dnsTarget: + description: Public ingress URL for the cluster Load Balancer + pattern: ^[a-z0-9-.]*$ + type: string ingressGatewayLabels: - description: IngressGatewayLabels - properties: - app: - type: string - istio: - type: string - required: - - app - - istio - type: object + description: Labels used to identify the istio ingress-gateway component + and its corresponding namespace. Usually {"app":"istio-ingressgateway","istio":"ingressgateway"} + items: + description: Generic Name/Value configuration + properties: + name: + type: string + value: + type: string + required: + - name + - value + type: object + minItems: 1 + type: array subscriptionServer: description: SubscriptionServer info properties: - dnsTarget: - type: string subDomain: type: string required: - subDomain type: object required: - - ingressGatewayLabels - subscriptionServer type: object status: diff --git a/config/samples/operator_v1alpha1_capoperator.yaml b/config/samples/operator_v1alpha1_capoperator.yaml index ad9e559..89efe25 100644 --- a/config/samples/operator_v1alpha1_capoperator.yaml +++ b/config/samples/operator_v1alpha1_capoperator.yaml @@ -11,6 +11,8 @@ spec: subscriptionServer: subDomain: cap-op ingressGatewayLabels: - istio: ingressgateway - app: istio-ingressgateway + - name: istio + value: ingressgateway + - name: app + value: istio-ingressgateway \ No newline at end of file diff --git a/internal/transformer/transformer.go b/internal/transformer/transformer.go index 03de176..245bd45 100644 --- a/internal/transformer/transformer.go +++ b/internal/transformer/transformer.go @@ -16,7 +16,6 @@ import ( apitypes "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" - "github.com/sap/cap-operator-lifecycle/api/v1alpha1" componentoperatorruntimetypes "github.com/sap/component-operator-runtime/pkg/types" ) @@ -60,9 +59,8 @@ func trimDNSTarget(dnsTarget string) string { func (t *transformer) fillDNSTarget(parameters map[string]any) error { // get DNSTarget - subscriptionServer := parameters["subscriptionServer"].(map[string]interface{}) - if subscriptionServer["dnsTarget"] != nil { // already filled in CRO - subscriptionServer["dnsTarget"] = trimDNSTarget(subscriptionServer["dnsTarget"].(string)) + if parameters["dnsTarget"] != nil { // already filled in CRO + parameters["dnsTarget"] = trimDNSTarget(parameters["dnsTarget"].(string)) return nil } @@ -71,27 +69,22 @@ func (t *transformer) fillDNSTarget(parameters map[string]any) error { return fmt.Errorf("cannot get dnsTarget; provide either dnsTarget or ingressGatewayLabels in the CRO") } - ingressGatewayLabels := parameters["ingressGatewayLabels"].(map[string]interface{}) - if ingressGatewayLabels["app"] == nil || ingressGatewayLabels["istio"] == nil { - return fmt.Errorf("cannot get dnsTarget; provide ingressGatewayLabels/app and ingressGatewayLabels/istio values in the CRO") - } - - dnsTarget, err := t.getDNSTarget(&v1alpha1.IngressGatewayLabels{App: ingressGatewayLabels["app"].(string), Istio: ingressGatewayLabels["istio"].(string)}) + dnsTarget, err := t.getDNSTarget(parameters["ingressGatewayLabels"].([]interface{})) if err != nil { return err } - subscriptionServer["dnsTarget"] = trimDNSTarget(dnsTarget) + parameters["dnsTarget"] = trimDNSTarget(dnsTarget) return nil } -func (t *transformer) getDNSTarget(ingressGatewayLabels *v1alpha1.IngressGatewayLabels) (dnsTarget string, err error) { +func (t *transformer) getDNSTarget(ingressGatewayLabels []interface{}) (dnsTarget string, err error) { ctx := context.TODO() // create ingress gateway selector from labels - ingressLabelSelector, err := labels.ValidatedSelectorFromSet(getIngressGatewayLabels(ingressGatewayLabels)) + ingressLabelSelector, err := labels.ValidatedSelectorFromSet(convertIngressGatewayLabelsToMap(ingressGatewayLabels)) if err != nil { return "", err } @@ -138,11 +131,13 @@ func (t *transformer) getDNSTarget(ingressGatewayLabels *v1alpha1.IngressGateway return dnsTarget, nil } -func getIngressGatewayLabels(ingressGatewayLabels *v1alpha1.IngressGatewayLabels) map[string]string { +func convertIngressGatewayLabelsToMap(ingressGatewayLabels []interface{}) map[string]string { ingressLabels := map[string]string{} - ingressLabels["app"] = ingressGatewayLabels.App - ingressLabels["istio"] = ingressGatewayLabels.Istio + for _, label := range ingressGatewayLabels { + labelMap := label.(map[string]interface{}) + ingressLabels[labelMap["name"].(string)] = labelMap["value"].(string) + } return ingressLabels } diff --git a/internal/transformer/transformer_test.go b/internal/transformer/transformer_test.go index aa13dff..1aa168e 100644 --- a/internal/transformer/transformer_test.go +++ b/internal/transformer/transformer_test.go @@ -97,13 +97,19 @@ func TestTransformer(t *testing.T) { subscriptionServer := parameter["subscriptionServer"].(map[string]interface{}) subscriptionServer["subDomain"] = "cop" if tt.dnsTargetFilled { - subscriptionServer["dnsTarget"] = "public-ingress.some.cluster.sap" + parameter["dnsTarget"] = "public-ingress.some.cluster.sap" } if tt.ingressGatewayLabelsFilled { - parameter["ingressGatewayLabels"] = map[string]interface{}{ - "istio": "ingress", - "app": "istio-ingress", + parameter["ingressGatewayLabels"] = []interface{}{ + map[string]interface{}{ + "name": "app", + "value": "istio-ingress", + }, + map[string]interface{}{ + "name": "istio", + "value": "ingress", + }, } } @@ -121,7 +127,7 @@ func TestTransformer(t *testing.T) { return } transformedParametersMap := transformedParameters.ToUnstructured() - if transformedParametersMap["subscriptionServer"].(map[string]interface{})["dnsTarget"].(string) != "public-ingress.some.cluster.sap" { + if transformedParametersMap["dnsTarget"].(string) != "public-ingress.some.cluster.sap" { t.Error("unexpected value returned") } if transformedParametersMap["subscriptionServer"].(map[string]interface{})["domain"].(string) != "cop.some.cluster.sap" { From 9ae8f5a4d407633172563f9e0cbf945e31fac158 Mon Sep 17 00:00:00 2001 From: i325261 Date: Tue, 26 Sep 2023 11:04:16 +0200 Subject: [PATCH 2/4] Added oneOf to CAPOperator CRD --- config/crd/operator.sme.sap.com_capoperators.yaml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/config/crd/operator.sme.sap.com_capoperators.yaml b/config/crd/operator.sme.sap.com_capoperators.yaml index 21a29ed..92cce16 100644 --- a/config/crd/operator.sme.sap.com_capoperators.yaml +++ b/config/crd/operator.sme.sap.com_capoperators.yaml @@ -66,8 +66,13 @@ spec: required: - subDomain type: object - required: - - subscriptionServer + oneOf: + - required: + - subscriptionServer + - ingressGatewayLabels + - required: + - subscriptionServer + - dnsTarget type: object status: properties: From 4b40cc47dbb652250ed6f323a0016d34d3572283 Mon Sep 17 00:00:00 2001 From: i325261 Date: Tue, 26 Sep 2023 12:39:53 +0200 Subject: [PATCH 3/4] dnsTarget value fix and domain length check --- internal/transformer/transformer.go | 12 +++++++++--- internal/transformer/transformer_test.go | 22 +++++++++++++++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/internal/transformer/transformer.go b/internal/transformer/transformer.go index 245bd45..58f9b51 100644 --- a/internal/transformer/transformer.go +++ b/internal/transformer/transformer.go @@ -59,8 +59,10 @@ func trimDNSTarget(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 - parameters["dnsTarget"] = trimDNSTarget(parameters["dnsTarget"].(string)) + subscriptionServer["dnsTarget"] = trimDNSTarget(parameters["dnsTarget"].(string)) + delete(parameters, "dnsTarget") return nil } @@ -74,8 +76,8 @@ func (t *transformer) fillDNSTarget(parameters map[string]any) error { return err } - parameters["dnsTarget"] = trimDNSTarget(dnsTarget) - + subscriptionServer["dnsTarget"] = trimDNSTarget(dnsTarget) + delete(parameters, "ingressGatewayLabels") return nil } @@ -200,6 +202,10 @@ func (t *transformer) fillDomain(parameters map[string]any) error { return err } + if len(domain) > 64 { + return fmt.Errorf("subscription server domain '%s' is longer than 64 characters; use a smaller subDomain", domain) + } + subscriptionServer["domain"] = domain delete(subscriptionServer, "subDomain") diff --git a/internal/transformer/transformer_test.go b/internal/transformer/transformer_test.go index 1aa168e..35994c0 100644 --- a/internal/transformer/transformer_test.go +++ b/internal/transformer/transformer_test.go @@ -19,16 +19,17 @@ func TestTransformer(t *testing.T) { name string dnsTargetFilled bool ingressGatewayLabelsFilled bool + longDomain bool expectError bool }{ { - name: "Test with all fields filled", + name: "Test with dnsTarget and without ingress gateway labels", dnsTargetFilled: true, - ingressGatewayLabelsFilled: true, + ingressGatewayLabelsFilled: false, expectError: false, }, { - name: "Test without dnsTarget", + name: "Test without dnsTarget and with ingress gateway labels", dnsTargetFilled: false, ingressGatewayLabelsFilled: true, expectError: false, @@ -39,6 +40,12 @@ func TestTransformer(t *testing.T) { ingressGatewayLabelsFilled: false, expectError: true, }, + { + name: "Test with more than 64 character domain", + ingressGatewayLabelsFilled: true, + longDomain: true, + expectError: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -95,7 +102,12 @@ func TestTransformer(t *testing.T) { parameter["subscriptionServer"] = map[string]interface{}{} subscriptionServer := parameter["subscriptionServer"].(map[string]interface{}) - subscriptionServer["subDomain"] = "cop" + + if tt.longDomain { + subscriptionServer["subDomain"] = "long-subdomain-for-the-test-to-fail-to-check-the-error-case" + } else { + subscriptionServer["subDomain"] = "cop" + } if tt.dnsTargetFilled { parameter["dnsTarget"] = "public-ingress.some.cluster.sap" } @@ -127,7 +139,7 @@ func TestTransformer(t *testing.T) { return } transformedParametersMap := transformedParameters.ToUnstructured() - if transformedParametersMap["dnsTarget"].(string) != "public-ingress.some.cluster.sap" { + if transformedParametersMap["subscriptionServer"].(map[string]interface{})["dnsTarget"].(string) != "public-ingress.some.cluster.sap" { t.Error("unexpected value returned") } if transformedParametersMap["subscriptionServer"].(map[string]interface{})["domain"].(string) != "cop.some.cluster.sap" { From e5803b7bc9ab3195662d02ad25c4d0c0635b74b6 Mon Sep 17 00:00:00 2001 From: i325261 Date: Tue, 26 Sep 2023 13:20:10 +0200 Subject: [PATCH 4/4] oneOf check in spec --- config/crd/operator.sme.sap.com_capoperators.yaml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/config/crd/operator.sme.sap.com_capoperators.yaml b/config/crd/operator.sme.sap.com_capoperators.yaml index 92cce16..ea37f0f 100644 --- a/config/crd/operator.sme.sap.com_capoperators.yaml +++ b/config/crd/operator.sme.sap.com_capoperators.yaml @@ -37,6 +37,11 @@ spec: type: object spec: description: CAPOperatorSpec defines the desired state of CAPOperator + oneOf: + - required: + - ingressGatewayLabels + - required: + - dnsTarget properties: dnsTarget: description: Public ingress URL for the cluster Load Balancer @@ -66,13 +71,8 @@ spec: required: - subDomain type: object - oneOf: - - required: - - subscriptionServer - - ingressGatewayLabels - - required: - - subscriptionServer - - dnsTarget + required: + - subscriptionServer type: object status: properties: