From 15c0014f3211df022775c09b75a73d77f9fd11ea Mon Sep 17 00:00:00 2001 From: Barni S Date: Tue, 3 Sep 2024 17:59:01 -0400 Subject: [PATCH] Implicit Getter - Automatically add last applied objects into the values passed to expanders - We add the objects under path: values... This may have conflicts - on conflict we choose the first seen object - Refactor Reconciler() function to separate evaluation code into evaluate() function to reduce cyclomatic complexity - Add test: TestImplicitGetter --- experiments/compositions/composition/go.mod | 2 +- experiments/compositions/composition/go.sum | 4 +- .../controller/expander_reconciler.go | 91 +++++---- .../composition/pkg/applier/applier.go | 68 +++++++ .../data/TestCustomStatusCELRule/input.yaml | 2 +- .../tests/data/TestImplicitGetter/input.yaml | 186 ++++++++++++++++++ .../tests/data/TestImplicitGetter/output.yaml | 105 ++++++++++ .../tests/testcases/simple_test.go | 17 ++ 8 files changed, 437 insertions(+), 38 deletions(-) create mode 100644 experiments/compositions/composition/tests/data/TestImplicitGetter/input.yaml create mode 100644 experiments/compositions/composition/tests/data/TestImplicitGetter/output.yaml diff --git a/experiments/compositions/composition/go.mod b/experiments/compositions/composition/go.mod index 555ea33826..e12a22b1ee 100644 --- a/experiments/compositions/composition/go.mod +++ b/experiments/compositions/composition/go.mod @@ -26,7 +26,7 @@ require ( sigs.k8s.io/cli-utils v0.35.0 sigs.k8s.io/controller-runtime v0.17.2 sigs.k8s.io/kubebuilder-declarative-pattern v0.15.0-beta.1.0.20240614185435-a248ed1e894c - sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240813172011-a8a1382c63c4 + sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240830014331-1a63d5a3bb9d sigs.k8s.io/kustomize/kstatus v0.0.2-0.20200509233124-065f70705d4d tailscale.com v1.62.0 ) diff --git a/experiments/compositions/composition/go.sum b/experiments/compositions/composition/go.sum index 9c5aa2bf6f..040d39e071 100644 --- a/experiments/compositions/composition/go.sum +++ b/experiments/compositions/composition/go.sum @@ -580,8 +580,8 @@ sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMm sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= sigs.k8s.io/kubebuilder-declarative-pattern v0.15.0-beta.1.0.20240614185435-a248ed1e894c h1:oDDOYsfrwJlLZ0pyzZiG7L/rF2JuQvvut+vFOYYZKQQ= sigs.k8s.io/kubebuilder-declarative-pattern v0.15.0-beta.1.0.20240614185435-a248ed1e894c/go.mod h1:56THnwsHGyrijk2GYKsTzcagxDoevccrdl+gBJWNocs= -sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240813172011-a8a1382c63c4 h1:1q3oCBqWse+jT0xqxsRhmka3pPJCrUrxVzO8xzPrGCA= -sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240813172011-a8a1382c63c4/go.mod h1:HReseUdDPJYAh1jxBreYJsh/mJ9l7AtvoSB30Jdlfmc= +sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240830014331-1a63d5a3bb9d h1:9Re+DU/Qzv7mIz6qDFmVFvYqfFSAuc6ssi1CH/2yQX8= +sigs.k8s.io/kubebuilder-declarative-pattern/applylib v0.0.0-20240830014331-1a63d5a3bb9d/go.mod h1:HReseUdDPJYAh1jxBreYJsh/mJ9l7AtvoSB30Jdlfmc= sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver v0.0.0-20230303024857-d1f76c15e05b h1:VcgyLj8SawHulvC24SRgI37mnhMmR3hW+ZVqZGBmkbc= sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver v0.0.0-20230303024857-d1f76c15e05b/go.mod h1:smCBkOX4Z3K9+MUGAscbschHAYgbuRZ+Pri49+x2zTc= sigs.k8s.io/kustomize/kstatus v0.0.2-0.20200509233124-065f70705d4d h1:k+m3LgwVsvSDvcUWoaG2yUGcyN/HKR6Dq2lb0FYgbq8= diff --git a/experiments/compositions/composition/internal/controller/expander_reconciler.go b/experiments/compositions/composition/internal/controller/expander_reconciler.go index b0308abdc9..fe000c6098 100644 --- a/experiments/compositions/composition/internal/controller/expander_reconciler.go +++ b/experiments/compositions/composition/internal/controller/expander_reconciler.go @@ -246,53 +246,34 @@ func (r *ExpanderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c loggerCR := logger stagesApplied := []string{} values := map[string]interface{}{} - waitRequested := false + requeueAgain := false + + // ---------- Evaluate and Apply expanders in order --------------------- for index, expander := range compositionCR.Spec.Expanders { planUpdated := false + reason := "" // ------------------- EVALUATION SECTION ----------------------- - logger = loggerCR.WithName(expander.Name).WithName("Expand") - - uri, ev, reason, err := r.getExpanderConfig(ctx, expander.Version, expander.Type) - if err != nil { - logger.Error(err, "Error getting expander version", "expander", expander.Type, - "version", expander.Version, "reason", reason) - newStatus.AppendErrorCondition(expander.Name, err.Error(), reason) - return ctrl.Result{}, err - } - - logger.Info("Got valid expander uri", "uri", uri) - - if expanderDebugLogsEnabled { - r.Recorder.Event(&inputcr, "Normal", fmt.Sprintf("Running expander stage %d: %s", index, expander.Name), expanderDebugLog(&inputcr)) - } - if ev.Spec.Type == compositionv1alpha1.ExpanderTypeJob { - reason, err = r.runJob(ctx, logger, &inputcr, expander.Name, planNN.Name, uri, ev.Spec.ImageRegistry) - } else { - values, planUpdated, reason, err = r.evaluateAndSavePlan(ctx, logger, &inputcr, values, expander, planNN, ev, uri, expanderDebugLogsEnabled) - _, iswaitErr := err.(*EvaluateWaitError) - if iswaitErr { - newStatus.AppendWaitingCondition(expander.Name, err.Error(), reason) - // Subsume the error - waitRequested = true - // continue to apply phase - break - } + values, planUpdated, reason, err = r.evaluate(ctx, logger, &inputcr, planNN, expander, values, expanderDebugLogsEnabled) + _, iswaitErr := err.(*EvaluateWaitError) + if iswaitErr { + newStatus.AppendWaitingCondition(expander.Name, err.Error(), reason) + // Subsume the error + requeueAgain = true + break } if err != nil { - newStatus.AppendErrorCondition(expander.Name, err.Error(), reason) // Skip apply phase and return + newStatus.AppendErrorCondition(expander.Name, err.Error(), reason) return ctrl.Result{}, err } + // Inject plan.Ready Condition with list of expanders newStatus.ClearCondition(compositionv1alpha1.Ready) message := fmt.Sprintf("Evaluated stage: %s", expander.Name) newStatus.AppendCondition(compositionv1alpha1.Ready, metav1.ConditionFalse, message, "EvaluationPending") - if expanderDebugLogsEnabled { - r.Recorder.Event(&inputcr, "Normal", fmt.Sprintf("Evaluated expander stage %d: %s", index, expander.Name), expanderDebugLog(&inputcr)) - } // ------------------- APPLIER SECTION ----------------------- @@ -390,10 +371,16 @@ func (r *ExpanderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c logger.Info("Applied successfully but some resources did not become healthy") // Inject plan.Waiting Condition newStatus.AppendWaitingCondition(expander.Name, "Not all resources are healthy", "WaitingForAppliedResources") - return ctrl.Result{}, fmt.Errorf("Some applied resources are not healthy") + + // Request a re-reconcile + requeueAgain = true + break } logger.Info("Applied resources successfully.") + // Implicit getter: Make the applied objects available in the values passed to subsequent stages + values = applier.AddAppliedObjectsIntoValues(values) + stagesApplied = append(stagesApplied, expander.Name) r.Recorder.Event(&inputcr, "Normal", "ResourcesReconciled", fmt.Sprintf("All applied resources were reconciled. name: %s", expander.Name)) // Inject plan.Ready Condition with list of expanders @@ -418,12 +405,48 @@ func (r *ExpanderReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c newStatus.Generation = plancr.GetGeneration() newStatus.CompositionGeneration = compositionCR.GetGeneration() newStatus.CompositionUID = compositionCR.GetUID() - if waitRequested { + if requeueAgain { return ctrl.Result{RequeueAfter: time.Second * 5}, nil } return ctrl.Result{}, nil } +func (r *ExpanderReconciler) evaluate(ctx context.Context, logger logr.Logger, + cr *unstructured.Unstructured, planNN types.NamespacedName, + expander compositionv1alpha1.Expander, values map[string]interface{}, + expanderDebugLogsEnabled bool) (map[string]interface{}, bool, string, error) { + + planUpdated := false + + logger = logger.WithName(expander.Name).WithName("Expand") + + uri, ev, reason, err := r.getExpanderConfig(ctx, expander.Version, expander.Type) + if err != nil { + logger.Error(err, "Error getting expander version", "expander", expander.Type, + "version", expander.Version, "reason", reason) + return values, planUpdated, reason, err + } + + logger.Info("Got valid expander uri", "uri", uri) + + if expanderDebugLogsEnabled { + r.Recorder.Event(cr, "Normal", fmt.Sprintf("Running expander stage: %s", expander.Name), expanderDebugLog(cr)) + } + + if ev.Spec.Type == compositionv1alpha1.ExpanderTypeJob { + reason, err = r.runJob(ctx, logger, cr, expander.Name, planNN.Name, uri, ev.Spec.ImageRegistry) + } else { + values, planUpdated, reason, err = r.evaluateAndSavePlan(ctx, logger, cr, values, expander, planNN, ev, uri, expanderDebugLogsEnabled) + } + + if err == nil && expanderDebugLogsEnabled { + r.Recorder.Event(cr, "Normal", fmt.Sprintf("Evaluated expander stage: %s", expander.Name), expanderDebugLog(cr)) + } + + return values, planUpdated, reason, err + +} + func (r *ExpanderReconciler) getExpanderConfig( ctx context.Context, inputExpanderVersion string, expanderType string, ) (string, *compositionv1alpha1.ExpanderVersion, string, error) { diff --git a/experiments/compositions/composition/pkg/applier/applier.go b/experiments/compositions/composition/pkg/applier/applier.go index 0815193029..2e83ba7178 100644 --- a/experiments/compositions/composition/pkg/applier/applier.go +++ b/experiments/compositions/composition/pkg/applier/applier.go @@ -17,6 +17,7 @@ package applier import ( "context" "fmt" + "strings" compositionv1alpha1 "github.com/GoogleCloudPlatform/k8s-config-connector/experiments/compositions/composition/api/v1alpha1" "github.com/GoogleCloudPlatform/k8s-config-connector/experiments/compositions/composition/pkg/cel" @@ -394,3 +395,70 @@ func (a *Applier) AreResourcesReady() (bool, error) { return allReady, nil } + +func (a *Applier) AddAppliedObjectsIntoValues(values map[string]interface{}) map[string]interface{} { + for _, resultObj := range a.results.Objects { + if resultObj.Apply.IsPruned { + continue + } + obj := resultObj.LastApplied + name := obj.GetName() + gvk := obj.GroupVersionKind() + kind := strings.ToLower(gvk.Kind) + + // short path: values... May clash + _, ok := values[kind] + if !ok { + values[kind] = map[string]interface{}{} + } + ref := values[kind].(map[string]interface{}) + + _, ok = ref[name] + if ok { + // Clash !! We will ignore + a.logger.Info("Clash when adding applied objects to values.", "kind", kind, "name", name) + } else { + ref[name] = obj.Object + } + + // long path: values.... will not clash + // Long path may not be practical since the namespace is not part of the composition and + // most templating languages dont support nested templatable variable. + // ex: {{ values.deployment.teampage.status.something }} will work in jinja2 + // but this {{ values.apps.deployment.{{teampage.metadata.namespace}}.status.something }} wont work + // So leaving this code commented for now. + /* + group := strings.ReplaceAll(strings.ToLower(gvk.Group), ".", "_") + namespace := obj.GetNamespace() + + if group == "" { + group = "core" + } + _, ok = values[group] + if !ok { + values[group] = map[string]interface{}{} + } + ref = values[group].(map[string]interface{}) + + _, ok = ref[kind] + if !ok { + ref[kind] = map[string]interface{}{} + } + ref = ref[kind].(map[string]interface{}) + + _, ok = ref[namespace] + if !ok { + ref[namespace] = map[string]interface{}{} + } + ref = ref[namespace].(map[string]interface{}) + + _, ok = ref[name] + if !ok { + ref[name] = map[string]interface{}{} + } + ref[name] = obj.Object + */ + + } + return values +} diff --git a/experiments/compositions/composition/tests/data/TestCustomStatusCELRule/input.yaml b/experiments/compositions/composition/tests/data/TestCustomStatusCELRule/input.yaml index b4da0df7a2..fea00d1928 100644 --- a/experiments/compositions/composition/tests/data/TestCustomStatusCELRule/input.yaml +++ b/experiments/compositions/composition/tests/data/TestCustomStatusCELRule/input.yaml @@ -124,7 +124,7 @@ spec: - name: v1alpha1 schema: openAPIV3Schema: - description: Create GCS Bucket with CORS and retention + description: Team Page Creation properties: apiVersion: type: string diff --git a/experiments/compositions/composition/tests/data/TestImplicitGetter/input.yaml b/experiments/compositions/composition/tests/data/TestImplicitGetter/input.yaml new file mode 100644 index 0000000000..e1cfe24919 --- /dev/null +++ b/experiments/compositions/composition/tests/data/TestImplicitGetter/input.yaml @@ -0,0 +1,186 @@ +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +apiVersion: composition.google.com/v1alpha1 +kind: Composition +metadata: + name: team2-page + namespace: default +spec: + inputAPIGroup: team2pages.idp.mycompany.com # Facade API + readiness: + - group: apps + version: v1 + kind: Deployment + readyIf: "status.readyReplicas==status.replicas" + expanders: + - type: jinja2 # pluggable expander + name: server # stage + template: | + apiVersion: apps/v1 + kind: Deployment + metadata: + # Use the team2pages Facade's name + name: team2deployment + # The namespace is set to the facade's namespace + namespace: default + labels: + # use facade's name in the label + app: nginx-{{ team2pages.metadata.name }} + spec: + replicas: 1 + selector: + matchLabels: + app: nginx-{{ team2pages.metadata.name }} + template: + metadata: + labels: + # use facade name in the pod's label + app: nginx-{{ team2pages.metadata.name }} + spec: + containers: + - name: server + image: nginx:1.16.0 + ports: + - name: http + containerPort: 80 + protocol: TCP + volumeMounts: + - name: index + mountPath: /usr/share/nginx/html/ + volumes: + - name: index + configMap: + # use the configmap created by this composition + name: team2-{{ team2pages.metadata.name }}-page + --- + apiVersion: v1 + kind: Service + metadata: + # include the facade name in the service name + name: team2service + namespace: default + labels: + app: nginx-{{ team2pages.metadata.name }} + spec: + ports: + - port: 80 + protocol: TCP + selector: + # match the web-server pod + app: nginx-{{ team2pages.metadata.name }} + --- + apiVersion: v1 + kind: ConfigMap + metadata: + name: team2-{{ team2pages.metadata.name }}-page + namespace: default + data: + index.html: | + +

{{ team2pages.metadata.name }}

+ + + + + + {% for member in team2pages.spec.members %} + + + + + {% endfor %} +
NameRole
{{ member.name }}{{ member.role }}
+ + - type: jinja2 # pluggable expander + name: configmap # stage + template: | + apiVersion: v1 + kind: ConfigMap + metadata: + name: team2config + namespace: default + data: + ports: "{% for port in values.service.team2service.spec.ports %}{{ port.port }}({{ port.protocol }}),{% endfor %}" + pods: "{{ values.deployment.team2deployment.status.readyReplicas }}" +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: team2pages.idp.mycompany.com +spec: + group: idp.mycompany.com + names: + categories: + - facade + - facades + kind: Team2Page + listKind: Team2PageList + plural: team2pages + singular: team2page + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: Team page creation + properties: + apiVersion: + type: string + kind: + type: string + metadata: + type: object + spec: + type: object + required: + - members + properties: + members: + type: array + items: + type: object + required: + - role + - name + properties: + role: + type: string + name: + type: string + type: object + served: true + storage: true + subresources: + status: {} +--- +apiVersion: v1 +kind: Namespace +metadata: + name: my-team2 +--- +apiVersion: idp.mycompany.com/v1alpha1 +kind: Team2Page +metadata: + name: landing + namespace: my-team2 +spec: + members: + - name: Jo + role: Eng Manager + - name: Jane + role: Lead + - name: Bob + role: Developer \ No newline at end of file diff --git a/experiments/compositions/composition/tests/data/TestImplicitGetter/output.yaml b/experiments/compositions/composition/tests/data/TestImplicitGetter/output.yaml new file mode 100644 index 0000000000..bfd65c19df --- /dev/null +++ b/experiments/compositions/composition/tests/data/TestImplicitGetter/output.yaml @@ -0,0 +1,105 @@ +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: nginx-landing + name: team2deployment + namespace: my-team2 +spec: + progressDeadlineSeconds: 600 + replicas: 1 + revisionHistoryLimit: 10 + selector: + matchLabels: + app: nginx-landing + strategy: + rollingUpdate: + maxSurge: 25% + maxUnavailable: 25% + type: RollingUpdate + template: + metadata: + creationTimestamp: null + labels: + app: nginx-landing + spec: + containers: + - image: nginx:1.16.0 + imagePullPolicy: IfNotPresent + name: server + ports: + - containerPort: 80 + name: http + protocol: TCP + resources: {} + terminationMessagePath: /dev/termination-log + terminationMessagePolicy: File + volumeMounts: + - mountPath: /usr/share/nginx/html/ + name: index + dnsPolicy: ClusterFirst + restartPolicy: Always + schedulerName: default-scheduler + securityContext: {} + terminationGracePeriodSeconds: 30 + volumes: + - configMap: + defaultMode: 420 + name: team2-landing-page + name: index +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: team2-landing-page + namespace: my-team2 +data: + index.html: |- + +

landing

+ + + + + + + + + + + + + + + + + + + + + +
NameRole
JoEng Manager
JaneLead
BobDeveloper
+ +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: team2config + namespace: my-team2 +data: + ports: 80(TCP), + pods: "1" \ No newline at end of file diff --git a/experiments/compositions/composition/tests/testcases/simple_test.go b/experiments/compositions/composition/tests/testcases/simple_test.go index 39d64448e4..5085cbd22f 100644 --- a/experiments/compositions/composition/tests/testcases/simple_test.go +++ b/experiments/compositions/composition/tests/testcases/simple_test.go @@ -439,6 +439,23 @@ func TestCustomStatusCELRule(t *testing.T) { // Verify plan has no errors } +func TestImplicitGetter(t *testing.T) { + s := scenario.NewBasic(t) + defer s.Cleanup() + s.Setup() + + // Check plan object has no error + plan := utils.GetPlanObj("my-team2", "team2pages-landing") + condition := utils.GetReadyCondition("ProcessedAllStages", "") + s.C.MustHaveCondition(plan, condition, 5*scenario.CompositionReconcileTimeout) + + // Verify that all resources objects are created + s.VerifyOutputExists() + s.VerifyOutputSpecMatches() + + // Verify plan has no errors +} + func TestMultipleCompositionsDisallowedForSameGVK(t *testing.T) { s := scenario.NewBasic(t) defer s.Cleanup()