From 912907d7c29a9e2636ccaa13f38ada445219850e Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Wed, 20 Sep 2023 10:34:03 -0400 Subject: [PATCH 1/6] Fix registry viewer host alias resolution Signed-off-by: Kim Tsao --- controllers/devfileregistry_controller.go | 10 +- .../devfileregistry_controller_test.go | 60 ++++ controllers/ensure.go | 3 - controllers/suite_test.go | 58 +++- controllers/update.go | 21 +- go.mod | 2 +- hack/routecrd/route.yaml | 282 ++++++++++++++++++ pkg/registry/deployment.go | 24 -- pkg/test/test_utils.go | 11 +- 9 files changed, 422 insertions(+), 49 deletions(-) create mode 100644 controllers/devfileregistry_controller_test.go create mode 100644 hack/routecrd/route.yaml diff --git a/controllers/devfileregistry_controller.go b/controllers/devfileregistry_controller.go index 4d3e9fa..bb9f234 100644 --- a/controllers/devfileregistry_controller.go +++ b/controllers/devfileregistry_controller.go @@ -162,13 +162,6 @@ func (r *DevfileRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Error(err, "Failed to update DevfileRegistry status") return ctrl.Result{Requeue: true}, err } - - //update the config map - result, err = r.ensure(ctx, devfileRegistry, &corev1.ConfigMap{}, labels, "") - if result != nil { - return *result, err - } - } return ctrl.Result{}, nil @@ -187,8 +180,7 @@ func (r *DevfileRegistryReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&appsv1.Deployment{}). Owns(&corev1.Service{}). Owns(&corev1.PersistentVolumeClaim{}). - Owns(&networkingv1.Ingress{}). - Owns(&corev1.ConfigMap{}) + Owns(&networkingv1.Ingress{}) // If on OpenShift, mark routes as owned by the controller if config.ControllerCfg.IsOpenShift() { diff --git a/controllers/devfileregistry_controller_test.go b/controllers/devfileregistry_controller_test.go new file mode 100644 index 0000000..f34177a --- /dev/null +++ b/controllers/devfileregistry_controller_test.go @@ -0,0 +1,60 @@ +/* +Copyright Red Hat + +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. +*/ + +package controllers + +import ( + "github.com/devfile/registry-operator/api/v1alpha1" + . "github.com/devfile/registry-operator/pkg/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/types" +) + +// The following test is only intended to test creation and updates to the DevfileRegistry CR, while ignoring the fact that routes will fail because they will +// not resolve to an actual URL + +var _ = Describe("DevfileRegistry controller test", func() { + Context("When Creating and updating a DevfileRegistry CR with valid values", func() { + It("Should create and update a DevfileRegistry CR Name with Status URL not found", func() { + Expect(k8sClient.Create(ctx, getDevfileRegistryCR(devfileRegistryName, devfileRegistriesNamespace, + image))).Should(Succeed()) + drlLookupKey := types.NamespacedName{Name: devfileRegistryName, Namespace: devfileRegistriesNamespace} + dr := &v1alpha1.DevfileRegistry{} + Eventually(func() error { + return k8sClient.Get(ctx, drlLookupKey, dr) + }, Timeout, Interval).Should(Succeed()) + + Expect(dr.Name).Should(Equal(devfileRegistryName)) + Expect(dr.Status.URL).Should(Equal("")) // an empty URL is an indicator that a URL was not resolved + + // update values + dr.Spec.Telemetry.RegistryName = devfileRegistryName + dr.Spec.Telemetry.Key = "abcdefghijk" + dr.Spec.DevfileIndex.Image = "quay.io/xyz/devfile-index:next" + + Expect(k8sClient.Update(ctx, dr)).Should(Succeed()) + + //try to update the name - it's immutable, so it should fail + dr.Name = devfileRegistryName + "1" + Expect(k8sClient.Update(ctx, dr)).ShouldNot(Succeed()) + + //delete all crs + deleteCRList(drlLookupKey, DevfileRegistryType) + }) + }) + +}) diff --git a/controllers/ensure.go b/controllers/ensure.go index d0eedcd..528cff0 100644 --- a/controllers/ensure.go +++ b/controllers/ensure.go @@ -66,9 +66,6 @@ func (r *DevfileRegistryReconciler) ensure(ctx context.Context, cr *registryv1al case *networkingv1.Ingress: ingress, _ := resource.(*networkingv1.Ingress) err = r.updateIngress(ctx, cr, ingressDomain, ingress) - case *corev1.ConfigMap: - configMap, _ := resource.(*corev1.ConfigMap) - err = r.updateConfigMap(ctx, cr, configMap) } if err != nil { r.Log.Error(err, "Failed to update "+resourceType) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index bf8f951..a906e89 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -17,6 +17,8 @@ limitations under the License. package controllers import ( + routev1 "github.com/openshift/api/route/v1" + "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/scheme" "path/filepath" "testing" @@ -28,7 +30,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "k8s.io/client-go/kubernetes/scheme" + appsv1 "k8s.io/api/apps/v1" + "k8s.io/api/networking/v1" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -57,6 +60,8 @@ const ( devfileStagingRegistryName = "StagingRegistry" devfileStagingRegistryURL = "https://registry.stage.devfile.io" localRegistryName = "LocalRegistry" + devfileRegistryName = "devfile-registry-name" + image = "quay.io/devfile/devfile-index:next" ) func TestAPIs(t *testing.T) { @@ -70,7 +75,8 @@ var _ = BeforeSuite(func() { By("bootstrapping test environment") testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, + CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases"), + filepath.Join("..", "hack", "routecrd")}, //known hack to add the "external" openshift Route API to the test environment ErrorIfCRDPathMissing: true, } @@ -81,6 +87,15 @@ var _ = BeforeSuite(func() { err = v1alpha1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) + err = routev1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + err = v1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + err = appsv1.AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + //+kubebuilder:scaffold:scheme k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) @@ -105,6 +120,13 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) + err = (&DevfileRegistryReconciler{ + Client: k8sManager.GetClient(), + Log: ctrl.Log.WithName("controllers").WithName("DevfileRegistryReconciler"), + Scheme: k8sManager.GetScheme(), + }).SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) + go func() { defer GinkgoRecover() err = k8sManager.Start(ctx) @@ -125,7 +147,7 @@ func getClusterDevfileRegistriesListCR(name string, namespace string, registryNa return &ClusterDevfileRegistriesList{ TypeMeta: metav1.TypeMeta{ APIVersion: ApiVersion, - Kind: "ClusterDevfileRegistriesList", + Kind: string(ClusterListType), }, ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -148,7 +170,7 @@ func getDevfileRegistriesListCR(name string, namespace string, registryName stri return &DevfileRegistriesList{ TypeMeta: metav1.TypeMeta{ APIVersion: ApiVersion, - Kind: "DevfileRegistriesList", + Kind: string(NamespaceListType), }, ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -166,19 +188,43 @@ func getDevfileRegistriesListCR(name string, namespace string, registryName stri } +// getDevfileRegistriesListCR returns a minimally populated DevfileRegistry object for testing +func getDevfileRegistryCR(name string, namespace string, image string) *DevfileRegistry { + return &DevfileRegistry{ + TypeMeta: metav1.TypeMeta{ + APIVersion: ApiVersion, + Kind: string(DevfileRegistryType), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + Spec: DevfileRegistrySpec{ + DevfileIndex: DevfileRegistrySpecContainer{ + Image: image, + }, + }, + } +} + // deleteCRList removes the cluster or namespace CR list from the cluster func deleteCRList(drlLookupKey types.NamespacedName, f ListType) { cl := &ClusterDevfileRegistriesList{} nl := &DevfileRegistriesList{} + dl := &DevfileRegistry{} + // Delete Eventually(func() error { if f == ClusterListType { k8sClient.Get(context.Background(), drlLookupKey, cl) return k8sClient.Delete(context.Background(), cl) - } else { + } else if f == NamespaceListType { k8sClient.Get(context.Background(), drlLookupKey, nl) return k8sClient.Delete(context.Background(), nl) + } else { + k8sClient.Get(context.Background(), drlLookupKey, dl) + return k8sClient.Delete(context.Background(), dl) } }, Timeout, Interval).Should(Succeed()) @@ -186,6 +232,8 @@ func deleteCRList(drlLookupKey types.NamespacedName, f ListType) { Eventually(func() error { if f == ClusterListType { return k8sClient.Get(context.Background(), drlLookupKey, cl) + } else if f == NamespaceListType { + return k8sClient.Get(context.Background(), drlLookupKey, nl) } else { return k8sClient.Get(context.Background(), drlLookupKey, nl) } diff --git a/controllers/update.go b/controllers/update.go index bcbf0f2..f30d0fc 100644 --- a/controllers/update.go +++ b/controllers/update.go @@ -88,6 +88,23 @@ func (r *DevfileRegistryReconciler) updateDeployment(ctx context.Context, cr *re if len(dep.Spec.Template.Spec.Containers) > 2 { viewerImage := registry.GetRegistryViewerImage(cr) viewerImageContainer := dep.Spec.Template.Spec.Containers[2] + + //determine if the NEXT_PUBLIC_ANALYTICS_WRITE_KEY env needs updating + viewerKey := cr.Spec.Telemetry.RegistryViewerWriteKey + if viewerImageContainer.Env[0].Value != viewerKey { + r.Log.Info("Updating NEXT_PUBLIC_ANALYTICS_WRITE_KEY ", "value", viewerKey) + viewerImageContainer.Env[0].Value = viewerKey + needsUpdating = true + } + + //determine if the DEVFILE_REGISTRIES env needs updating. This will only occur on initial deployment since object name is unique + newDRValue := fmt.Sprintf(`[{"name": "%s","url": "http://localhost:8080","fqdn": "%s"}]`, cr.ObjectMeta.Name, cr.Status.URL) + if viewerImageContainer.Env[1].Value != newDRValue { + r.Log.Info("Updating DEVFILE_REGISTRIES ", "value", newDRValue) + viewerImageContainer.Env[1].Value = newDRValue + needsUpdating = true + } + if viewerImageContainer.Image != viewerImage { viewerImageContainer.Image = viewerImage needsUpdating = true @@ -203,7 +220,7 @@ func (r *DevfileRegistryReconciler) deleteOldPVCIfNeeded(ctx context.Context, cr return nil } -func (r *DevfileRegistryReconciler) updateConfigMap(ctx context.Context, cr *registryv1alpha1.DevfileRegistry, configMap *corev1.ConfigMap) error { +/*func (r *DevfileRegistryReconciler) updateConfigMap(ctx context.Context, cr *registryv1alpha1.DevfileRegistry, configMap *corev1.ConfigMap) error { viewerEnvfile := fmt.Sprintf(` NEXT_PUBLIC_ANALYTICS_WRITE_KEY=%s @@ -214,4 +231,4 @@ DEVFILE_REGISTRIES=[{"name":"%s","url":"http://localhost:8080","fqdn":"%s"}]`, return r.Update(ctx, configMap) -} +}*/ diff --git a/go.mod b/go.mod index 817f4bc..131dd17 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/stretchr/testify v1.8.1 gopkg.in/yaml.v2 v2.4.0 k8s.io/api v0.26.2 + k8s.io/apiextensions-apiserver v0.26.1 k8s.io/apimachinery v0.26.2 k8s.io/client-go v0.26.2 sigs.k8s.io/controller-runtime v0.14.5 @@ -87,7 +88,6 @@ require ( google.golang.org/protobuf v1.28.1 // indirect gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/apiextensions-apiserver v0.26.1 // indirect k8s.io/component-base v0.26.1 // indirect k8s.io/klog/v2 v2.80.1 // indirect k8s.io/kube-openapi v0.0.0-20221012153701-172d655c2280 // indirect diff --git a/hack/routecrd/route.yaml b/hack/routecrd/route.yaml new file mode 100644 index 0000000..2ef07b9 --- /dev/null +++ b/hack/routecrd/route.yaml @@ -0,0 +1,282 @@ + +--- +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.6.1 + creationTimestamp: null + name: routes.route.openshift.io +spec: + group: route.openshift.io + names: + kind: Route + listKind: RouteList + plural: routes + singular: route + scope: Namespaced + versions: + - name: v1 + schema: + openAPIV3Schema: + description: "A route allows developers to expose services through an HTTP(S) + aware load balancing and proxy layer via a public DNS entry. The route may + further specify TLS options and a certificate, or specify a public CNAME + that the router should also accept for HTTP and HTTPS traffic. An administrator + typically configures their router to be visible outside the cluster firewall, + and may also add additional security, caching, or traffic controls on the + service content. Routers usually talk directly to the service endpoints. + \n Once a route is created, the `host` field may not be changed. Generally, + routers use the oldest route with a given host when resolving conflicts. + \n Routers are subject to additional customization and may support additional + controls via the annotations field. \n Because administrators may configure + multiple routers, the route status field is used to return information to + clients about the names and states of the route under each router. If a + client chooses a duplicate name, for instance, the route status conditions + are used to indicate the route cannot be chosen. \n To enable HTTP/2 ALPN + on a route it requires a custom (non-wildcard) certificate. This prevents + connection coalescing by clients, notably web browsers. We do not support + HTTP/2 ALPN on routes that use the default certificate because of the risk + of connection re-use/coalescing. Routes that do not have their own custom + certificate will not be HTTP/2 ALPN-enabled on either the frontend or the + backend. \n Compatibility level 1: Stable within a major release for a minimum + of 12 months or 3 minor releases (whichever is longer)." + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: spec is the desired state of the route + properties: + alternateBackends: + description: alternateBackends allows up to 3 additional backends + to be assigned to the route. Only the Service kind is allowed, and + it will be defaulted to Service. Use the weight field in RouteTargetReference + object to specify relative preference. + items: + description: RouteTargetReference specifies the target that resolve + into endpoints. Only the 'Service' kind is allowed. Use 'weight' + field to emphasize one over others. + properties: + kind: + description: The kind of target that the route is referring + to. Currently, only 'Service' is allowed + type: string + name: + description: name of the service/target that is being referred + to. e.g. name of the service + type: string + weight: + description: weight as an integer between 0 and 256, default + 100, that specifies the target's relative weight against other + target reference objects. 0 suppresses requests to this backend. + format: int32 + type: integer + required: + - kind + - name + type: object + type: array + host: + description: host is an alias/DNS that points to the service. Optional. + If not specified a route name will typically be automatically chosen. + Must follow DNS952 subdomain conventions. + type: string + path: + description: path that the router watches for, to route traffic for + to the service. Optional + type: string + port: + description: If specified, the port to be used by the router. Most + routers will use all endpoints exposed by the service by default + - set this value to instruct routers which port to use. + properties: + targetPort: + anyOf: + - type: integer + - type: string + description: The target port on pods selected by the service this + route points to. If this is a string, it will be looked up as + a named port in the target endpoints port list. Required + x-kubernetes-int-or-string: true + required: + - targetPort + type: object + subdomain: + description: "subdomain is a DNS subdomain that is requested within + the ingress controller's domain (as a subdomain). If host is set + this field is ignored. An ingress controller may choose to ignore + this suggested name, in which case the controller will report the + assigned name in the status.ingress array or refuse to admit the + route. If this value is set and the server does not support this + field host will be populated automatically. Otherwise host is left + empty. The field may have multiple parts separated by a dot, but + not all ingress controllers may honor the request. This field may + not be changed after creation except by a user with the update routes/custom-host + permission. \n Example: subdomain `frontend` automatically receives + the router subdomain `apps.mycluster.com` to have a full hostname + `frontend.apps.mycluster.com`." + type: string + tls: + description: The tls field provides the ability to configure certificates + and termination for the route. + properties: + caCertificate: + description: caCertificate provides the cert authority certificate + contents + type: string + certificate: + description: certificate provides certificate contents. This should + be a single serving certificate, not a certificate chain. Do + not include a CA certificate. + type: string + destinationCACertificate: + description: destinationCACertificate provides the contents of + the ca certificate of the final destination. When using reencrypt + termination this file should be provided in order to have routers + use it for health checks on the secure connection. If this field + is not specified, the router may provide its own destination + CA and perform hostname validation using the short service name + (service.namespace.svc), which allows infrastructure generated + certificates to automatically verify. + type: string + insecureEdgeTerminationPolicy: + description: "insecureEdgeTerminationPolicy indicates the desired + behavior for insecure connections to a route. While each router + may make its own decisions on which ports to expose, this is + normally port 80. \n * Allow - traffic is sent to the server + on the insecure port (default) * Disable - no traffic is allowed + on the insecure port. * Redirect - clients are redirected to + the secure port." + type: string + key: + description: key provides key file contents + type: string + termination: + description: "termination indicates termination type. \n * edge + - TLS termination is done by the router and http is used to + communicate with the backend (default) * passthrough - Traffic + is sent straight to the destination without the router providing + TLS termination * reencrypt - TLS termination is done by the + router and https is used to communicate with the backend" + type: string + required: + - termination + type: object + to: + description: to is an object the route should use as the primary backend. + Only the Service kind is allowed, and it will be defaulted to Service. + If the weight field (0-256 default 100) is set to zero, no traffic + will be sent to this backend. + properties: + kind: + description: The kind of target that the route is referring to. + Currently, only 'Service' is allowed + type: string + name: + description: name of the service/target that is being referred + to. e.g. name of the service + type: string + weight: + description: weight as an integer between 0 and 256, default 100, + that specifies the target's relative weight against other target + reference objects. 0 suppresses requests to this backend. + format: int32 + type: integer + required: + - kind + - name + type: object + wildcardPolicy: + description: Wildcard policy if any for the route. Currently only + 'Subdomain' or 'None' is allowed. + type: string + required: + - to + type: object + status: + description: status is the current state of the route + properties: + ingress: + description: ingress describes the places where the route may be exposed. + The list of ingress points may contain duplicate Host or RouterName + values. Routes are considered live once they are `Ready` + items: + description: RouteIngress holds information about the places where + a route is exposed. + properties: + conditions: + description: Conditions is the state of the route, may be empty. + items: + description: RouteIngressCondition contains details for the + current condition of this route on a particular router. + properties: + lastTransitionTime: + description: RFC 3339 date and time when this condition + last transitioned + format: date-time + type: string + message: + description: Human readable message indicating details + about last transition. + type: string + reason: + description: (brief) reason for the condition's last transition, + and is usually a machine and human readable constant + type: string + status: + description: Status is the status of the condition. Can + be True, False, Unknown. + type: string + type: + description: Type is the type of the condition. Currently + only Ready. + type: string + required: + - status + - type + type: object + type: array + host: + description: Host is the host string under which the route is + exposed; this value is required + type: string + routerCanonicalHostname: + description: CanonicalHostname is the external host name for + the router that can be used as a CNAME for the host requested + for this route. This value is optional and may not be set + in all cases. + type: string + routerName: + description: Name is a name chosen by the router to identify + itself; this value is required + type: string + wildcardPolicy: + description: Wildcard policy is the wildcard policy that was + allowed where this route is exposed. + type: string + type: object + type: array + type: object + required: + - spec + type: object + served: true + storage: true + subresources: + status: {} +status: + acceptedNames: + kind: "" + plural: "" + conditions: [] + storedVersions: [] diff --git a/pkg/registry/deployment.go b/pkg/registry/deployment.go index 8f8acad..e59c5d3 100644 --- a/pkg/registry/deployment.go +++ b/pkg/registry/deployment.go @@ -280,30 +280,6 @@ func GenerateDeployment(cr *registryv1alpha1.DevfileRegistry, scheme *runtime.Sc ]`, cr.ObjectMeta.Name, cr.Status.URL), }, }, - VolumeMounts: []corev1.VolumeMount{ - { - Name: "viewer-env-file", - MountPath: "/app/.env.production", - SubPath: ".env.production", - ReadOnly: true, - }, - }, - }) - dep.Spec.Template.Spec.Volumes = append(dep.Spec.Template.Spec.Volumes, corev1.Volume{ - Name: "viewer-env-file", - VolumeSource: corev1.VolumeSource{ - ConfigMap: &corev1.ConfigMapVolumeSource{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: ConfigMapName(cr.Name), - }, - Items: []corev1.KeyToPath{ - { - Key: ".env.registry-viewer", - Path: ".env.production", - }, - }, - }, - }, }) } else { // Set environment variable to run index server in headless mode diff --git a/pkg/test/test_utils.go b/pkg/test/test_utils.go index 1709e24..6e0b232 100644 --- a/pkg/test/test_utils.go +++ b/pkg/test/test_utils.go @@ -31,11 +31,12 @@ import ( type ListType string const ( - ClusterListType ListType = "ClusterDevfileRegistriesList" - NamespaceListType ListType = "DevfileRegistriesList" - ApiVersion = "registry.devfile.io/v1alpha1" - Timeout = time.Second * 10 - Interval = time.Millisecond * 250 + ClusterListType ListType = "ClusterDevfileRegistriesList" + NamespaceListType ListType = "DevfileRegistriesList" + DevfileRegistryType ListType = "DevfileRegistry" + ApiVersion = "registry.devfile.io/v1alpha1" + Timeout = time.Second * 10 + Interval = time.Millisecond * 250 ) // GetNewUnstartedTestServer is a mock test index server that supports just the v2 index schema From a16697faefea32e1960f0248974bc662329adb6e Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Wed, 20 Sep 2023 15:06:44 -0400 Subject: [PATCH 2/6] Add a fake kubeconfig to fix CI tests Signed-off-by: Kim Tsao --- .github/mock_kube_config.sh | 17 +++++++++++++++++ .github/workflows/ci.yaml | 6 +++++- 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100755 .github/mock_kube_config.sh diff --git a/.github/mock_kube_config.sh b/.github/mock_kube_config.sh new file mode 100755 index 0000000..9565d2b --- /dev/null +++ b/.github/mock_kube_config.sh @@ -0,0 +1,17 @@ +#!/bin/bash +# copied script from the release-service: https://github.com/redhat-appstudio/release-service-bundles/blob/main/.github/scripts/mock_kube_config.sh +printf 'Creating mock kube config for controller tests\n' +mkdir -vp $HOME/.kube || true +cat <<-EOF > $HOME/.kube/config +apiVersion: v1 +kind: Config +clusters: +- cluster: + server: _ + name: _ +contexts: +- context: + cluster: _ + name: _ +current-context: _ +EOF \ No newline at end of file diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 0d4bef0..79190bf 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -67,7 +67,11 @@ jobs: - name: Check CRD manifest generation run: make manifests - - + - + name: Create mock kube config + run: .github/scripts/mock_kube_config.sh + shell: bash + - name: Run unit tests run: make test - From fdb2b9ea3a8ae37a8ca0771c9323ff5783e1e726 Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Wed, 20 Sep 2023 15:09:40 -0400 Subject: [PATCH 3/6] fix path Signed-off-by: Kim Tsao --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 79190bf..441205a 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -69,7 +69,7 @@ jobs: run: make manifests - name: Create mock kube config - run: .github/scripts/mock_kube_config.sh + run: .github/mock_kube_config.sh shell: bash - name: Run unit tests From db468c1fbc70ba1b63cf4e225ca4116c7396bfb0 Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Thu, 21 Sep 2023 18:22:11 -0400 Subject: [PATCH 4/6] Undo controller test changes Signed-off-by: Kim Tsao --- .github/mock_kube_config.sh | 17 -- .github/workflows/ci.yaml | 4 - .../devfileregistry_controller_test.go | 60 ---- controllers/suite_test.go | 51 +--- hack/routecrd/route.yaml | 282 ------------------ 5 files changed, 2 insertions(+), 412 deletions(-) delete mode 100755 .github/mock_kube_config.sh delete mode 100644 controllers/devfileregistry_controller_test.go delete mode 100644 hack/routecrd/route.yaml diff --git a/.github/mock_kube_config.sh b/.github/mock_kube_config.sh deleted file mode 100755 index 9565d2b..0000000 --- a/.github/mock_kube_config.sh +++ /dev/null @@ -1,17 +0,0 @@ -#!/bin/bash -# copied script from the release-service: https://github.com/redhat-appstudio/release-service-bundles/blob/main/.github/scripts/mock_kube_config.sh -printf 'Creating mock kube config for controller tests\n' -mkdir -vp $HOME/.kube || true -cat <<-EOF > $HOME/.kube/config -apiVersion: v1 -kind: Config -clusters: -- cluster: - server: _ - name: _ -contexts: -- context: - cluster: _ - name: _ -current-context: _ -EOF \ No newline at end of file diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 441205a..f3c489b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -67,10 +67,6 @@ jobs: - name: Check CRD manifest generation run: make manifests - - - name: Create mock kube config - run: .github/mock_kube_config.sh - shell: bash - name: Run unit tests run: make test diff --git a/controllers/devfileregistry_controller_test.go b/controllers/devfileregistry_controller_test.go deleted file mode 100644 index f34177a..0000000 --- a/controllers/devfileregistry_controller_test.go +++ /dev/null @@ -1,60 +0,0 @@ -/* -Copyright Red Hat - -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. -*/ - -package controllers - -import ( - "github.com/devfile/registry-operator/api/v1alpha1" - . "github.com/devfile/registry-operator/pkg/test" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - "k8s.io/apimachinery/pkg/types" -) - -// The following test is only intended to test creation and updates to the DevfileRegistry CR, while ignoring the fact that routes will fail because they will -// not resolve to an actual URL - -var _ = Describe("DevfileRegistry controller test", func() { - Context("When Creating and updating a DevfileRegistry CR with valid values", func() { - It("Should create and update a DevfileRegistry CR Name with Status URL not found", func() { - Expect(k8sClient.Create(ctx, getDevfileRegistryCR(devfileRegistryName, devfileRegistriesNamespace, - image))).Should(Succeed()) - drlLookupKey := types.NamespacedName{Name: devfileRegistryName, Namespace: devfileRegistriesNamespace} - dr := &v1alpha1.DevfileRegistry{} - Eventually(func() error { - return k8sClient.Get(ctx, drlLookupKey, dr) - }, Timeout, Interval).Should(Succeed()) - - Expect(dr.Name).Should(Equal(devfileRegistryName)) - Expect(dr.Status.URL).Should(Equal("")) // an empty URL is an indicator that a URL was not resolved - - // update values - dr.Spec.Telemetry.RegistryName = devfileRegistryName - dr.Spec.Telemetry.Key = "abcdefghijk" - dr.Spec.DevfileIndex.Image = "quay.io/xyz/devfile-index:next" - - Expect(k8sClient.Update(ctx, dr)).Should(Succeed()) - - //try to update the name - it's immutable, so it should fail - dr.Name = devfileRegistryName + "1" - Expect(k8sClient.Update(ctx, dr)).ShouldNot(Succeed()) - - //delete all crs - deleteCRList(drlLookupKey, DevfileRegistryType) - }) - }) - -}) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index a906e89..239742f 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -17,7 +17,6 @@ limitations under the License. package controllers import ( - routev1 "github.com/openshift/api/route/v1" "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/scheme" "path/filepath" "testing" @@ -30,8 +29,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - appsv1 "k8s.io/api/apps/v1" - "k8s.io/api/networking/v1" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -60,8 +57,6 @@ const ( devfileStagingRegistryName = "StagingRegistry" devfileStagingRegistryURL = "https://registry.stage.devfile.io" localRegistryName = "LocalRegistry" - devfileRegistryName = "devfile-registry-name" - image = "quay.io/devfile/devfile-index:next" ) func TestAPIs(t *testing.T) { @@ -75,8 +70,7 @@ var _ = BeforeSuite(func() { By("bootstrapping test environment") testEnv = &envtest.Environment{ - CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases"), - filepath.Join("..", "hack", "routecrd")}, //known hack to add the "external" openshift Route API to the test environment + CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, ErrorIfCRDPathMissing: true, } @@ -87,15 +81,6 @@ var _ = BeforeSuite(func() { err = v1alpha1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) - err = routev1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - err = v1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - err = appsv1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - //+kubebuilder:scaffold:scheme k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) @@ -120,13 +105,6 @@ var _ = BeforeSuite(func() { }).SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) - err = (&DevfileRegistryReconciler{ - Client: k8sManager.GetClient(), - Log: ctrl.Log.WithName("controllers").WithName("DevfileRegistryReconciler"), - Scheme: k8sManager.GetScheme(), - }).SetupWithManager(k8sManager) - Expect(err).ToNot(HaveOccurred()) - go func() { defer GinkgoRecover() err = k8sManager.Start(ctx) @@ -188,43 +166,20 @@ func getDevfileRegistriesListCR(name string, namespace string, registryName stri } -// getDevfileRegistriesListCR returns a minimally populated DevfileRegistry object for testing -func getDevfileRegistryCR(name string, namespace string, image string) *DevfileRegistry { - return &DevfileRegistry{ - TypeMeta: metav1.TypeMeta{ - APIVersion: ApiVersion, - Kind: string(DevfileRegistryType), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - }, - Spec: DevfileRegistrySpec{ - DevfileIndex: DevfileRegistrySpecContainer{ - Image: image, - }, - }, - } -} - // deleteCRList removes the cluster or namespace CR list from the cluster func deleteCRList(drlLookupKey types.NamespacedName, f ListType) { cl := &ClusterDevfileRegistriesList{} nl := &DevfileRegistriesList{} - dl := &DevfileRegistry{} // Delete Eventually(func() error { if f == ClusterListType { k8sClient.Get(context.Background(), drlLookupKey, cl) return k8sClient.Delete(context.Background(), cl) - } else if f == NamespaceListType { + } else { k8sClient.Get(context.Background(), drlLookupKey, nl) return k8sClient.Delete(context.Background(), nl) - } else { - k8sClient.Get(context.Background(), drlLookupKey, dl) - return k8sClient.Delete(context.Background(), dl) } }, Timeout, Interval).Should(Succeed()) @@ -232,8 +187,6 @@ func deleteCRList(drlLookupKey types.NamespacedName, f ListType) { Eventually(func() error { if f == ClusterListType { return k8sClient.Get(context.Background(), drlLookupKey, cl) - } else if f == NamespaceListType { - return k8sClient.Get(context.Background(), drlLookupKey, nl) } else { return k8sClient.Get(context.Background(), drlLookupKey, nl) } diff --git a/hack/routecrd/route.yaml b/hack/routecrd/route.yaml deleted file mode 100644 index 2ef07b9..0000000 --- a/hack/routecrd/route.yaml +++ /dev/null @@ -1,282 +0,0 @@ - ---- -apiVersion: apiextensions.k8s.io/v1 -kind: CustomResourceDefinition -metadata: - annotations: - controller-gen.kubebuilder.io/version: v0.6.1 - creationTimestamp: null - name: routes.route.openshift.io -spec: - group: route.openshift.io - names: - kind: Route - listKind: RouteList - plural: routes - singular: route - scope: Namespaced - versions: - - name: v1 - schema: - openAPIV3Schema: - description: "A route allows developers to expose services through an HTTP(S) - aware load balancing and proxy layer via a public DNS entry. The route may - further specify TLS options and a certificate, or specify a public CNAME - that the router should also accept for HTTP and HTTPS traffic. An administrator - typically configures their router to be visible outside the cluster firewall, - and may also add additional security, caching, or traffic controls on the - service content. Routers usually talk directly to the service endpoints. - \n Once a route is created, the `host` field may not be changed. Generally, - routers use the oldest route with a given host when resolving conflicts. - \n Routers are subject to additional customization and may support additional - controls via the annotations field. \n Because administrators may configure - multiple routers, the route status field is used to return information to - clients about the names and states of the route under each router. If a - client chooses a duplicate name, for instance, the route status conditions - are used to indicate the route cannot be chosen. \n To enable HTTP/2 ALPN - on a route it requires a custom (non-wildcard) certificate. This prevents - connection coalescing by clients, notably web browsers. We do not support - HTTP/2 ALPN on routes that use the default certificate because of the risk - of connection re-use/coalescing. Routes that do not have their own custom - certificate will not be HTTP/2 ALPN-enabled on either the frontend or the - backend. \n Compatibility level 1: Stable within a major release for a minimum - of 12 months or 3 minor releases (whichever is longer)." - properties: - apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' - type: string - kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' - type: string - metadata: - type: object - spec: - description: spec is the desired state of the route - properties: - alternateBackends: - description: alternateBackends allows up to 3 additional backends - to be assigned to the route. Only the Service kind is allowed, and - it will be defaulted to Service. Use the weight field in RouteTargetReference - object to specify relative preference. - items: - description: RouteTargetReference specifies the target that resolve - into endpoints. Only the 'Service' kind is allowed. Use 'weight' - field to emphasize one over others. - properties: - kind: - description: The kind of target that the route is referring - to. Currently, only 'Service' is allowed - type: string - name: - description: name of the service/target that is being referred - to. e.g. name of the service - type: string - weight: - description: weight as an integer between 0 and 256, default - 100, that specifies the target's relative weight against other - target reference objects. 0 suppresses requests to this backend. - format: int32 - type: integer - required: - - kind - - name - type: object - type: array - host: - description: host is an alias/DNS that points to the service. Optional. - If not specified a route name will typically be automatically chosen. - Must follow DNS952 subdomain conventions. - type: string - path: - description: path that the router watches for, to route traffic for - to the service. Optional - type: string - port: - description: If specified, the port to be used by the router. Most - routers will use all endpoints exposed by the service by default - - set this value to instruct routers which port to use. - properties: - targetPort: - anyOf: - - type: integer - - type: string - description: The target port on pods selected by the service this - route points to. If this is a string, it will be looked up as - a named port in the target endpoints port list. Required - x-kubernetes-int-or-string: true - required: - - targetPort - type: object - subdomain: - description: "subdomain is a DNS subdomain that is requested within - the ingress controller's domain (as a subdomain). If host is set - this field is ignored. An ingress controller may choose to ignore - this suggested name, in which case the controller will report the - assigned name in the status.ingress array or refuse to admit the - route. If this value is set and the server does not support this - field host will be populated automatically. Otherwise host is left - empty. The field may have multiple parts separated by a dot, but - not all ingress controllers may honor the request. This field may - not be changed after creation except by a user with the update routes/custom-host - permission. \n Example: subdomain `frontend` automatically receives - the router subdomain `apps.mycluster.com` to have a full hostname - `frontend.apps.mycluster.com`." - type: string - tls: - description: The tls field provides the ability to configure certificates - and termination for the route. - properties: - caCertificate: - description: caCertificate provides the cert authority certificate - contents - type: string - certificate: - description: certificate provides certificate contents. This should - be a single serving certificate, not a certificate chain. Do - not include a CA certificate. - type: string - destinationCACertificate: - description: destinationCACertificate provides the contents of - the ca certificate of the final destination. When using reencrypt - termination this file should be provided in order to have routers - use it for health checks on the secure connection. If this field - is not specified, the router may provide its own destination - CA and perform hostname validation using the short service name - (service.namespace.svc), which allows infrastructure generated - certificates to automatically verify. - type: string - insecureEdgeTerminationPolicy: - description: "insecureEdgeTerminationPolicy indicates the desired - behavior for insecure connections to a route. While each router - may make its own decisions on which ports to expose, this is - normally port 80. \n * Allow - traffic is sent to the server - on the insecure port (default) * Disable - no traffic is allowed - on the insecure port. * Redirect - clients are redirected to - the secure port." - type: string - key: - description: key provides key file contents - type: string - termination: - description: "termination indicates termination type. \n * edge - - TLS termination is done by the router and http is used to - communicate with the backend (default) * passthrough - Traffic - is sent straight to the destination without the router providing - TLS termination * reencrypt - TLS termination is done by the - router and https is used to communicate with the backend" - type: string - required: - - termination - type: object - to: - description: to is an object the route should use as the primary backend. - Only the Service kind is allowed, and it will be defaulted to Service. - If the weight field (0-256 default 100) is set to zero, no traffic - will be sent to this backend. - properties: - kind: - description: The kind of target that the route is referring to. - Currently, only 'Service' is allowed - type: string - name: - description: name of the service/target that is being referred - to. e.g. name of the service - type: string - weight: - description: weight as an integer between 0 and 256, default 100, - that specifies the target's relative weight against other target - reference objects. 0 suppresses requests to this backend. - format: int32 - type: integer - required: - - kind - - name - type: object - wildcardPolicy: - description: Wildcard policy if any for the route. Currently only - 'Subdomain' or 'None' is allowed. - type: string - required: - - to - type: object - status: - description: status is the current state of the route - properties: - ingress: - description: ingress describes the places where the route may be exposed. - The list of ingress points may contain duplicate Host or RouterName - values. Routes are considered live once they are `Ready` - items: - description: RouteIngress holds information about the places where - a route is exposed. - properties: - conditions: - description: Conditions is the state of the route, may be empty. - items: - description: RouteIngressCondition contains details for the - current condition of this route on a particular router. - properties: - lastTransitionTime: - description: RFC 3339 date and time when this condition - last transitioned - format: date-time - type: string - message: - description: Human readable message indicating details - about last transition. - type: string - reason: - description: (brief) reason for the condition's last transition, - and is usually a machine and human readable constant - type: string - status: - description: Status is the status of the condition. Can - be True, False, Unknown. - type: string - type: - description: Type is the type of the condition. Currently - only Ready. - type: string - required: - - status - - type - type: object - type: array - host: - description: Host is the host string under which the route is - exposed; this value is required - type: string - routerCanonicalHostname: - description: CanonicalHostname is the external host name for - the router that can be used as a CNAME for the host requested - for this route. This value is optional and may not be set - in all cases. - type: string - routerName: - description: Name is a name chosen by the router to identify - itself; this value is required - type: string - wildcardPolicy: - description: Wildcard policy is the wildcard policy that was - allowed where this route is exposed. - type: string - type: object - type: array - type: object - required: - - spec - type: object - served: true - storage: true - subresources: - status: {} -status: - acceptedNames: - kind: "" - plural: "" - conditions: [] - storedVersions: [] From 12e1a416aeb6f10f7e5f5a1358c7b9521a55ed0d Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Thu, 21 Sep 2023 18:23:15 -0400 Subject: [PATCH 5/6] update integration tests Signed-off-by: Kim Tsao --- .../examples/update/devfileregistry-new.yaml | 5 +- tests/integration/pkg/client/pod.go | 69 ++++++++ .../pkg/tests/devfileregistry_tests.go | 164 +++++++++++++----- 3 files changed, 195 insertions(+), 43 deletions(-) diff --git a/tests/integration/examples/update/devfileregistry-new.yaml b/tests/integration/examples/update/devfileregistry-new.yaml index a3b8669..ea262d6 100644 --- a/tests/integration/examples/update/devfileregistry-new.yaml +++ b/tests/integration/examples/update/devfileregistry-new.yaml @@ -8,4 +8,7 @@ spec: storage: enabled: false tls: - enabled: true \ No newline at end of file + enabled: true + telemetry: + key: registry-key + registryViewerWriteKey: viewer-key \ No newline at end of file diff --git a/tests/integration/pkg/client/pod.go b/tests/integration/pkg/client/pod.go index 98228ad..c1521a1 100644 --- a/tests/integration/pkg/client/pod.go +++ b/tests/integration/pkg/client/pod.go @@ -48,6 +48,31 @@ func (w *K8sClient) WaitForPodRunningByLabelWithNamespace(label string, namespac } } +// WaitForPodFailedByLabelWithNamespace waits for the pod matching the specified label in a specified namespace to become running +// An error is returned if the pod does not exist or the timeout is reached +func (w *K8sClient) WaitForPodFailedByLabelWithNamespace(label string, namespace string) (deployed bool, err error) { + timeout := time.After(6 * time.Minute) + tick := time.Tick(1 * time.Second) + + for { + select { + case <-timeout: + return false, errors.New("timed out") + case <-tick: + err := w.WaitForFailedPodBySelector(namespace, label, 3*time.Minute) + if err == nil { + return true, nil + } + } + } +} + +// WaitForPodFailedByLabel waits for the pod matching the specified label to become running +// An error is returned if the pod does not exist or the timeout is reached +func (w *K8sClient) WaitForPodFailedByLabel(label string) (deployed bool, err error) { + return w.WaitForPodFailedByLabelWithNamespace(label, config.Namespace) +} + // WaitForPodRunningByLabel waits for the pod matching the specified label to become running // An error is returned if the pod does not exist or the timeout is reached func (w *K8sClient) WaitForPodRunningByLabel(label string) (deployed bool, err error) { @@ -77,6 +102,29 @@ func (w *K8sClient) WaitForRunningPodBySelector(namespace, selector string, time return nil } +// WaitForFailedPodBySelector waits up to timeout seconds for all pods in 'namespace' with given 'selector' to enter running state. +// Returns an error if no pods are found or not all discovered pods enter running state. +func (w *K8sClient) WaitForFailedPodBySelector(namespace, selector string, timeout time.Duration) error { + podList, err := w.ListPods(namespace, selector) + if err != nil { + return err + } + if len(podList.Items) == 0 { + fmt.Println("No pods for " + selector + " in namespace " + namespace) + + return nil + } + + for _, pod := range podList.Items { + fmt.Println("Pod " + pod.Name + " created in namespace " + namespace + "...Checking for failure.") + if err := w.waitForPodFailing(namespace, pod.Name, timeout); err != nil { + return err + } + } + + return nil +} + // ListPods returns the list of currently scheduled or running pods in `namespace` with the given selector func (w *K8sClient) ListPods(namespace, selector string) (*v1.PodList, error) { listOptions := metav1.ListOptions{LabelSelector: selector} @@ -94,6 +142,12 @@ func (w *K8sClient) waitForPodRunning(namespace, podName string, timeout time.Du return wait.PollImmediate(time.Second, timeout, w.isPodRunning(podName, namespace)) } +// Poll up to timeout seconds for pod to enter running state. +// Returns an error if the pod never enters the running state. +func (w *K8sClient) waitForPodFailing(namespace, podName string, timeout time.Duration) error { + return wait.PollImmediate(time.Second, timeout, w.isPodFailing(podName, namespace)) +} + // return a condition function that indicates whether the given pod is // currently running func (w *K8sClient) isPodRunning(podName, namespace string) wait.ConditionFunc { @@ -111,3 +165,18 @@ func (w *K8sClient) isPodRunning(podName, namespace string) wait.ConditionFunc { return false, nil } } + +// return a condition function that indicates whether the given pod is +// currently running +func (w *K8sClient) isPodFailing(podName, namespace string) wait.ConditionFunc { + return func() (bool, error) { + pod, _ := w.Kube().CoreV1().Pods(namespace).Get(context.TODO(), podName, metav1.GetOptions{}) + + if pod.Status.Phase == v1.PodRunning { + return false, nil + } else { + fmt.Printf("Pod terminated %s\n", podName) + return true, nil + } + } +} diff --git a/tests/integration/pkg/tests/devfileregistry_tests.go b/tests/integration/pkg/tests/devfileregistry_tests.go index 52e85b7..1a6d9c9 100644 --- a/tests/integration/pkg/tests/devfileregistry_tests.go +++ b/tests/integration/pkg/tests/devfileregistry_tests.go @@ -24,7 +24,7 @@ import ( "github.com/devfile/registry-operator/tests/integration/pkg/client" "github.com/devfile/registry-operator/tests/integration/pkg/config" "github.com/onsi/ginkgo/v2" - "github.com/onsi/gomega" + . "github.com/onsi/gomega" ) // Integration/e2e test logic based on https://github.com/devfile/devworkspace-operator/tree/master/test/e2e @@ -42,37 +42,43 @@ var _ = ginkgo.Describe("[Create Devfile Registry resource]", func() { ginkgo.Fail("Failed to create devfileregistry instance: " + err.Error()) return } - deploy, err := K8sClient.WaitForPodRunningByLabel(label) - if !deploy { - fmt.Println("Devfile Registry didn't start properly") - } - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + waitForPodsToFullyStartInUIMode(label) // Wait for the registry instance to become ready err = K8sClient.WaitForRegistryInstance(crName, 5*time.Minute) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Retrieve the registry URL and verify the server is up and running registry, err := K8sClient.GetRegistryInstance(crName) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) err = util.WaitForServer(registry.Status.URL, 5*time.Minute, false) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) - // Verify that the index metrics endpoint is running podList, err := K8sClient.ListPods(config.Namespace, label) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) registryPod := podList.Items[0] + // Verify that the index metrics endpoint is running indexMetricsURL := "http://localhost:7071/metrics" output, err := K8sClient.CurlEndpointInContainer(registryPod.Name, "devfile-registry", indexMetricsURL) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(output).To(gomega.ContainSubstring("promhttp_metric_handler_requests_total")) + Expect(err).NotTo(HaveOccurred()) + Expect(output).To(ContainSubstring("promhttp_metric_handler_requests_total")) // Verify that the oci metrics endpoint is running ociMetricsURL := "http://localhost:5001/metrics" output, err = K8sClient.CurlEndpointInContainer(registryPod.Name, "devfile-registry", ociMetricsURL) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(output).To(gomega.ContainSubstring("registry_storage_cache_total")) + Expect(err).NotTo(HaveOccurred()) + Expect(output).To(ContainSubstring("registry_storage_cache_total")) + + //verify the registry viewer is running + registryViewerURL := "http://localhost:8080/viewer" + output, err = K8sClient.CurlEndpointInContainer(registryPod.Name, "devfile-registry", registryViewerURL) + Expect(err).NotTo(HaveOccurred()) + Expect(output).NotTo(ContainSubstring("registry viewer is not available in headless mode")) + + validateEnvVariables(label, registry.Name, registry.Status.URL, "", "") + }) var _ = ginkgo.AfterEach(func() { @@ -91,24 +97,23 @@ var _ = ginkgo.Describe("[Create Devfile Registry resource with TLS enabled]", f ginkgo.Fail("Failed to create devfileregistry instance: " + err.Error()) return } - deploy, err := K8sClient.WaitForPodRunningByLabel(label) - if !deploy { - fmt.Println("Devfile Registry didn't start properly") - } - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + waitForPodsToFullyStartInUIMode(label) // Wait for the registry instance to become ready err = K8sClient.WaitForRegistryInstance(crName, 5*time.Minute) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Retrieve the registry URL and verify that its protocol is https registry, err := K8sClient.GetRegistryInstance(crName) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(registry.Status.URL).To(gomega.ContainSubstring("https://")) + Expect(err).NotTo(HaveOccurred()) + Expect(registry.Status.URL).To(ContainSubstring("https://")) // Verify that the server is accessible. err = util.WaitForServer(registry.Status.URL, 5*time.Minute, false) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) + + validateEnvVariables(label, registry.Name, registry.Status.URL, "", "") }) var _ = ginkgo.AfterEach(func() { @@ -131,26 +136,26 @@ var _ = ginkgo.Describe("[Create Devfile Registry resource with headless enabled if !deploy { fmt.Println("Devfile Registry didn't start properly") } - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Wait for the registry instance to become ready err = K8sClient.WaitForRegistryInstance(crName, 5*time.Minute) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Retrieve the registry URL and verify the server is up and running registry, err := K8sClient.GetRegistryInstance(crName) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) err = util.WaitForServer(registry.Status.URL, 5*time.Minute, false) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Registry Viewer should not be accessible podList, err := K8sClient.ListPods(config.Namespace, label) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) registryPod := podList.Items[0] registryViewerURL := "http://localhost:8080/viewer" output, err := K8sClient.CurlEndpointInContainer(registryPod.Name, "devfile-registry", registryViewerURL) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(output).To(gomega.ContainSubstring("registry viewer is not available in headless mode")) + Expect(err).NotTo(HaveOccurred()) + Expect(output).To(ContainSubstring("registry viewer is not available in headless mode")) }) var _ = ginkgo.AfterEach(func() { @@ -169,40 +174,115 @@ var _ = ginkgo.Describe("[Update Devfile Registry resource]", func() { ginkgo.Fail("Failed to create devfileregistry instance: " + err.Error()) return } - deploy, err := K8sClient.WaitForPodRunningByLabel(label) - if !deploy { - fmt.Println("Devfile Registry didn't start properly") - } - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + waitForPodsToFullyStartInUIMode(label) // Wait for the registry instance to become ready err = K8sClient.WaitForRegistryInstance(crName, 5*time.Minute) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Retrieve the registry URL and verify the server is up and running registry, err := K8sClient.GetRegistryInstance(crName) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) err = util.WaitForServer(registry.Status.URL, 5*time.Minute, false) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) // Update the devfileregistry resource for this test case + fmt.Printf("Applying update...") err = K8sClient.OcApplyResource("tests/integration/examples/update/devfileregistry-new.yaml") if err != nil { ginkgo.Fail("Failed to create devfileregistry instance: " + err.Error()) return } + //Wait for termination and then restart after update + fmt.Println("Wait for Devfile Registry pods to terminate") + //wait for pod to terminate and run again. + failed, err := K8sClient.WaitForPodFailedByLabel(label) + if !failed { + fmt.Println("Devfile Registry did not terminate") + } + Expect(err).NotTo(HaveOccurred()) + + //wait for pod to run again + fmt.Println("Wait for Devfile Registry pods to start again") + deploy, err := K8sClient.WaitForPodRunningByLabel(label) + if !deploy { + fmt.Println("Devfile Registry didn't start properly") + } + Expect(err).NotTo(HaveOccurred()) + // Retrieve the registry URL and verify that its protocol is https url, err := K8sClient.WaitForURLChange(crName, registry.Status.URL, 5*time.Minute) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(url).To(gomega.ContainSubstring("https://")) + Expect(err).NotTo(HaveOccurred()) + Expect(url).To(ContainSubstring("https://")) // Verify that the server is accessible. err = util.WaitForServer(url, 5*time.Minute, false) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) + + validateEnvVariables(label, registry.Name, url, "viewer-key", "registry-key") }) var _ = ginkgo.AfterEach(func() { K8sClient.OcDeleteResource("tests/integration/examples/update/devfileregistry-new.yaml") }) }) + +// waitForPodsToFullyStartInUIMode. The registry viewer relies on the resolved DevfileRegistry URL which will cause an update when writing to the environment variable. +// We need to wait for pod restarts in order to fully test the values +func waitForPodsToFullyStartInUIMode(label string) { + deploy, err := K8sClient.WaitForPodRunningByLabel(label) + if !deploy { + fmt.Println("Devfile Registry didn't start properly") + } + Expect(err).NotTo(HaveOccurred()) + + fmt.Println("Wait for Devfile Registry pods to terminate") + //wait for pod to terminate and run again. + failed, err := K8sClient.WaitForPodFailedByLabel(label) + if !failed { + fmt.Println("Devfile Registry did not terminate") + } + Expect(err).NotTo(HaveOccurred()) + + //wait for pod to run again + fmt.Println("Wait for Devfile Registry pods to start again") + deploy, err = K8sClient.WaitForPodRunningByLabel(label) + if !deploy { + fmt.Println("Devfile Registry didn't start properly") + } + Expect(err).NotTo(HaveOccurred()) +} + +func validateEnvVariables(label, registryName, registryURL, viewerWriteKey, telemetryKey string) { + //Determine if the viewer pod contains the resolved DevfileRegistryURL + newPodList, err := K8sClient.ListPods(config.Namespace, label) + Expect(err).NotTo(HaveOccurred()) + registryPod := newPodList.Items[0] + containers := registryPod.Spec.Containers + if len(containers) == 3 { + //verify the telemetry key is set in the devfile registry + registryEnvVars := containers[0].Env + for _, regEnvs := range registryEnvVars { + if regEnvs.Name == "TELEMETRY_KEY" { + Expect(regEnvs.Value).Should(Equal(telemetryKey)) + } + } + + //registry viewer is the last container + viewerEnvVars := containers[2].Env + for _, env := range viewerEnvVars { + if env.Name == "NEXT_PUBLIC_ANALYTICS_WRITE_KEY" { + Expect(env.Value).Should(Equal(viewerWriteKey)) + } + + if env.Name == "DEVFILE_REGISTRIES" { + Expect(env.Value).Should(ContainSubstring(registryName)) + Expect(env.Value).Should(ContainSubstring(registryURL)) + } + } + } else { + ginkgo.Fail("There should be 3 containers, got %d ", len(containers)) + } +} From 20eeb418466fc8268b927b1a266c43dcda578188 Mon Sep 17 00:00:00 2001 From: Kim Tsao Date: Fri, 22 Sep 2023 08:55:39 -0400 Subject: [PATCH 6/6] Address review comments Signed-off-by: Kim Tsao --- controllers/update.go | 13 ------------- .../integration/pkg/tests/devfileregistry_tests.go | 1 + 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/controllers/update.go b/controllers/update.go index f30d0fc..4192a81 100644 --- a/controllers/update.go +++ b/controllers/update.go @@ -219,16 +219,3 @@ func (r *DevfileRegistryReconciler) deleteOldPVCIfNeeded(ctx context.Context, cr } return nil } - -/*func (r *DevfileRegistryReconciler) updateConfigMap(ctx context.Context, cr *registryv1alpha1.DevfileRegistry, configMap *corev1.ConfigMap) error { - - viewerEnvfile := fmt.Sprintf(` -NEXT_PUBLIC_ANALYTICS_WRITE_KEY=%s -DEVFILE_REGISTRIES=[{"name":"%s","url":"http://localhost:8080","fqdn":"%s"}]`, - cr.Spec.Telemetry.RegistryViewerWriteKey, cr.ObjectMeta.Name, cr.Status.URL) - - configMap.Data[".env.registry-viewer"] = viewerEnvfile - - return r.Update(ctx, configMap) - -}*/ diff --git a/tests/integration/pkg/tests/devfileregistry_tests.go b/tests/integration/pkg/tests/devfileregistry_tests.go index 1a6d9c9..6a26d09 100644 --- a/tests/integration/pkg/tests/devfileregistry_tests.go +++ b/tests/integration/pkg/tests/devfileregistry_tests.go @@ -255,6 +255,7 @@ func waitForPodsToFullyStartInUIMode(label string) { Expect(err).NotTo(HaveOccurred()) } +// validateEnvVariables validates the set environment variables of the devfile registry containers func validateEnvVariables(label, registryName, registryURL, viewerWriteKey, telemetryKey string) { //Determine if the viewer pod contains the resolved DevfileRegistryURL newPodList, err := K8sClient.ListPods(config.Namespace, label)