From 9a749a4da3671cc2fe1f38a3e5e2fae9a4f0ebab Mon Sep 17 00:00:00 2001 From: Yuwen Ma Date: Sat, 1 Jun 2024 15:46:52 +0000 Subject: [PATCH] No direct CRD labels; direct will be the expected default --- .github/workflows/presubmit.yaml | 11 +++++ apis/apikeys/v1alpha1/apikey_type.go | 2 +- .../logging/v1beta1/logmetric_types.go | 2 +- ...eyskeys.apikeys.cnrm.cloud.google.com.yaml | 2 +- ...metrics.logging.cnrm.cloud.google.com.yaml | 1 - .../apis/apikeys/v1alpha1/apikeyskey_types.go | 2 +- .../logging/v1beta1/logginglogmetric_types.go | 2 +- .../directbase/directbase_controller.go | 45 +++++++++++++++++- .../registration/registration_controller.go | 41 ++++++++-------- pkg/k8s/constants.go | 1 - .../controller/reconciler/testreconciler.go | 2 +- scripts/github-actions/tests-e2e-direct.sh | 47 +++++++++++++++++++ 12 files changed, 128 insertions(+), 30 deletions(-) create mode 100755 scripts/github-actions/tests-e2e-direct.sh diff --git a/.github/workflows/presubmit.yaml b/.github/workflows/presubmit.yaml index 81552e19c1..8603824984 100644 --- a/.github/workflows/presubmit.yaml +++ b/.github/workflows/presubmit.yaml @@ -112,6 +112,17 @@ jobs: - name: "Run pause tests" run: | ./scripts/github-actions/ga-pause-test.sh + direct-tests: + runs-on: ubuntu-22.04 + timeout-minutes: 60 + steps: + - uses: actions/checkout@v3 + - uses: actions/setup-go@v4 + with: + go-version-file: 'go.mod' + - name: "run tests-e2e-direct" + run: | + ./scripts/github-actions/tests-e2e-direct.sh tests-e2e-fixtures-vcr: runs-on: ubuntu-22.04 timeout-minutes: 60 diff --git a/apis/apikeys/v1alpha1/apikey_type.go b/apis/apikeys/v1alpha1/apikey_type.go index 68cb5f2d4c..ae5b02b17c 100644 --- a/apis/apikeys/v1alpha1/apikey_type.go +++ b/apis/apikeys/v1alpha1/apikey_type.go @@ -41,7 +41,7 @@ var ( // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:categories=gcp,shortName=gcpapikeyskey;gcpapikeyskeys // +kubebuilder:subresource:status -// +kubebuilder:metadata:labels="cnrm.cloud.google.com/directcrd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/system=true" +// +kubebuilder:metadata:labels="cnrm.cloud.google.com/tf2crd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/system=true" // +kubebuilder:printcolumn:name="Age",JSONPath=".metadata.creationTimestamp",type="date" // +kubebuilder:printcolumn:name="Ready",JSONPath=".status.conditions[?(@.type=='Ready')].status",type="string",description="When 'True', the most recent reconcile of the resource succeeded" // +kubebuilder:printcolumn:name="Status",JSONPath=".status.conditions[?(@.type=='Ready')].reason",type="string",description="The reason for the value in 'Ready'" diff --git a/apis/resources/logging/v1beta1/logmetric_types.go b/apis/resources/logging/v1beta1/logmetric_types.go index 728ab0a2e2..51b6b58e1c 100644 --- a/apis/resources/logging/v1beta1/logmetric_types.go +++ b/apis/resources/logging/v1beta1/logmetric_types.go @@ -233,7 +233,7 @@ type LoggingLogMetricStatus struct { // +kubebuilder:subresource:status // +kubebuilder:object:root=true // +kubebuilder:subresource:status -// +kubebuilder:metadata:labels="cnrm.cloud.google.com/directcrd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true" +// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true" // +kubebuilder:printcolumn:name="Age",JSONPath=".metadata.creationTimestamp",type="date" // +kubebuilder:printcolumn:name="Ready",JSONPath=".status.conditions[?(@.type=='Ready')].status",type="string",description="When 'True', the most recent reconcile of the resource succeeded" // +kubebuilder:printcolumn:name="Status",JSONPath=".status.conditions[?(@.type=='Ready')].reason",type="string",description="The reason for the value in 'Ready'" diff --git a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_apikeyskeys.apikeys.cnrm.cloud.google.com.yaml b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_apikeyskeys.apikeys.cnrm.cloud.google.com.yaml index 75ae5e821a..1269a47508 100644 --- a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_apikeyskeys.apikeys.cnrm.cloud.google.com.yaml +++ b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_apikeyskeys.apikeys.cnrm.cloud.google.com.yaml @@ -5,10 +5,10 @@ metadata: cnrm.cloud.google.com/version: 0.0.0-dev creationTimestamp: null labels: - cnrm.cloud.google.com/directcrd: "true" cnrm.cloud.google.com/managed-by-kcc: "true" cnrm.cloud.google.com/stability-level: alpha cnrm.cloud.google.com/system: "true" + cnrm.cloud.google.com/tf2crd: "true" name: apikeyskeys.apikeys.cnrm.cloud.google.com spec: group: apikeys.cnrm.cloud.google.com diff --git a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_logginglogmetrics.logging.cnrm.cloud.google.com.yaml b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_logginglogmetrics.logging.cnrm.cloud.google.com.yaml index 5c10a28659..9bcb20de6e 100644 --- a/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_logginglogmetrics.logging.cnrm.cloud.google.com.yaml +++ b/config/crds/resources/apiextensions.k8s.io_v1_customresourcedefinition_logginglogmetrics.logging.cnrm.cloud.google.com.yaml @@ -5,7 +5,6 @@ metadata: cnrm.cloud.google.com/version: 0.0.0-dev creationTimestamp: null labels: - cnrm.cloud.google.com/directcrd: "true" cnrm.cloud.google.com/managed-by-kcc: "true" cnrm.cloud.google.com/stability-level: stable cnrm.cloud.google.com/system: "true" diff --git a/pkg/clients/generated/apis/apikeys/v1alpha1/apikeyskey_types.go b/pkg/clients/generated/apis/apikeys/v1alpha1/apikeyskey_types.go index 021abe0915..a96c760f99 100644 --- a/pkg/clients/generated/apis/apikeys/v1alpha1/apikeyskey_types.go +++ b/pkg/clients/generated/apis/apikeys/v1alpha1/apikeyskey_types.go @@ -134,7 +134,7 @@ type APIKeysKeyStatus struct { // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:categories=gcp,shortName=gcpapikeyskey;gcpapikeyskeys // +kubebuilder:subresource:status -// +kubebuilder:metadata:labels="cnrm.cloud.google.com/directcrd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/system=true" +// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=alpha";"cnrm.cloud.google.com/system=true";"cnrm.cloud.google.com/tf2crd=true" // +kubebuilder:printcolumn:name="Age",JSONPath=".metadata.creationTimestamp",type="date" // +kubebuilder:printcolumn:name="Ready",JSONPath=".status.conditions[?(@.type=='Ready')].status",type="string",description="When 'True', the most recent reconcile of the resource succeeded" // +kubebuilder:printcolumn:name="Status",JSONPath=".status.conditions[?(@.type=='Ready')].reason",type="string",description="The reason for the value in 'Ready'" diff --git a/pkg/clients/generated/apis/logging/v1beta1/logginglogmetric_types.go b/pkg/clients/generated/apis/logging/v1beta1/logginglogmetric_types.go index ee196d6fad..2f3b3185e7 100644 --- a/pkg/clients/generated/apis/logging/v1beta1/logginglogmetric_types.go +++ b/pkg/clients/generated/apis/logging/v1beta1/logginglogmetric_types.go @@ -219,7 +219,7 @@ type LoggingLogMetricStatus struct { // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // +kubebuilder:resource:categories=gcp,shortName=gcplogginglogmetric;gcplogginglogmetrics // +kubebuilder:subresource:status -// +kubebuilder:metadata:labels="cnrm.cloud.google.com/directcrd=true";"cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true" +// +kubebuilder:metadata:labels="cnrm.cloud.google.com/managed-by-kcc=true";"cnrm.cloud.google.com/stability-level=stable";"cnrm.cloud.google.com/system=true" // +kubebuilder:printcolumn:name="Age",JSONPath=".metadata.creationTimestamp",type="date" // +kubebuilder:printcolumn:name="Ready",JSONPath=".status.conditions[?(@.type=='Ready')].status",type="string",description="When 'True', the most recent reconcile of the resource succeeded" // +kubebuilder:printcolumn:name="Status",JSONPath=".status.conditions[?(@.type=='Ready')].reason",type="string",description="The reason for the value in 'Ready'" diff --git a/pkg/controller/direct/directbase/directbase_controller.go b/pkg/controller/direct/directbase/directbase_controller.go index 1ae817dbab..d9ccfd6abe 100644 --- a/pkg/controller/direct/directbase/directbase_controller.go +++ b/pkg/controller/direct/directbase/directbase_controller.go @@ -35,6 +35,7 @@ import ( "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/execution" "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/k8s" "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/util" + apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "golang.org/x/sync/semaphore" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -57,7 +58,7 @@ import ( var ControllerBuilder directControllerBuilder func init() { - ControllerBuilder = directControllerBuilder{modelMapper: map[schema.GroupVersionKind]func(*controller.Config) Model{}} + ControllerBuilder = directControllerBuilder{} } type directControllerBuilder struct { @@ -65,10 +66,13 @@ type directControllerBuilder struct { } func (c *directControllerBuilder) RegisterModel(gvk schema.GroupVersionKind, modelFn func(*controller.Config) Model) { + if c.modelMapper == nil { + c.modelMapper = map[schema.GroupVersionKind]func(*controller.Config) Model{} + } c.modelMapper[gvk] = modelFn } -func (c *directControllerBuilder) Add(mgr manager.Manager, config *controller.Config, gvk schema.GroupVersionKind, deps Deps) error { +func (c *directControllerBuilder) AddController(mgr manager.Manager, config *controller.Config, gvk schema.GroupVersionKind, deps Deps) error { immediateReconcileRequests := make(chan event.GenericEvent, k8s.ImmediateReconcileRequestsBufferSize) resourceWatcherRoutines := semaphore.NewWeighted(k8s.MaxNumResourceWatcherRoutines) @@ -79,6 +83,43 @@ func (c *directControllerBuilder) Add(mgr manager.Manager, config *controller.Co return add(mgr, reconciler) } +func (c *directControllerBuilder) IsDirectByCRD(crd *apiextensions.CustomResourceDefinition) bool { + for gvk, _ := range c.modelMapper { + if gvk.Group == crd.Spec.Group && gvk.Kind == crd.Spec.Names.Kind { + for _, version := range crd.Spec.Versions { + if gvk.Version == version.Name { + return true + } + } + + } + } + return false +} + +func (c *directControllerBuilder) IsDirectByGK(gk schema.GroupKind) bool { + if c.modelMapper == nil { + return false + } + for gvk, _ := range c.modelMapper { + if gvk.Group == gk.Group && gvk.Kind == gk.Kind { + return true + } + } + return false +} + +func (c *directControllerBuilder) IsDirectByGVK(gvk schema.GroupVersionKind) bool { + if c.modelMapper == nil { + return false + } + _, ok := c.modelMapper[gvk] + if ok { + return true + } + return false +} + // NewReconciler returns a new reconcile.Reconciler. func (c *directControllerBuilder) NewReconciler(mgr manager.Manager, config *controller.Config, immediateReconcileRequests chan event.GenericEvent, resourceWatcherRoutines *semaphore.Weighted, gvk schema.GroupVersionKind, jg jitter.Generator) (*DirectReconciler, error) { diff --git a/pkg/controller/registration/registration_controller.go b/pkg/controller/registration/registration_controller.go index 191cd32231..3526915c79 100644 --- a/pkg/controller/registration/registration_controller.go +++ b/pkg/controller/registration/registration_controller.go @@ -194,7 +194,7 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf } var schemaUpdater k8s.SchemaReferenceUpdater if kccfeatureflags.UseDirectReconciler(gvk.GroupKind()) { - err := directbase.ControllerBuilder.Add(r.mgr, config, gvk, directbase.Deps{JitterGenerator: r.jitterGenerator}) + err := directbase.ControllerBuilder.AddController(r.mgr, config, gvk, directbase.Deps{JitterGenerator: r.jitterGenerator}) if err != nil { return nil, fmt.Errorf("error adding direct controller for %v to a manager: %w", crd.Spec.Names.Kind, err) } @@ -221,14 +221,14 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf } default: - // register controllers for direct CRDs - if val, ok := crd.Labels[k8s.DirectCRDLabel]; ok && val == "true" { - err := directbase.ControllerBuilder.Add(r.mgr, config, gvk, directbase.Deps{JitterGenerator: r.jitterGenerator}) - if err != nil { - return nil, fmt.Errorf("error adding direct controller for %v to a manager: %w", crd.Spec.Names.Kind, err) + // register the controller to automatically create secrets for GSA keys + if isServiceAccountKeyCRD(crd) { + logger.Info("registering the GSA-Key-to-Secret generation controller") + if err := gsakeysecretgenerator.Add(r.mgr, crd, &controller.Deps{JitterGen: r.jitterGenerator}); err != nil { + return nil, fmt.Errorf("error adding the gsa-to-secret generator for %v to a manager: %w", crd.Spec.Names.Kind, err) } - return schemaUpdater, nil } + // register controllers for dcl-based CRDs if val, ok := crd.Labels[k8s.DCL2CRDLabel]; ok && val == "true" { su, err := dclcontroller.Add(r.mgr, crd, r.dclConverter, r.dclConfig, r.smLoader, r.defaulters, r.jitterGenerator) @@ -238,22 +238,23 @@ func registerDefaultController(r *ReconcileRegistration, config *controller.Conf return su, nil } // register controllers for tf-based CRDs - if val, ok := crd.Labels[crdgeneration.TF2CRDLabel]; !ok || val != "true" { - logger.Error(fmt.Errorf("unrecognized CRD: %v", crd.Spec.Names.Kind), "skipping controller registration", "group", gvk.Group, "version", gvk.Version, "kind", gvk.Kind) - return nil, nil - } - su, err := tf.Add(r.mgr, crd, r.provider, r.smLoader, r.defaulters, r.jitterGenerator) - if err != nil { - return nil, fmt.Errorf("error adding terraform controller for %v to a manager: %w", crd.Spec.Names.Kind, err) + if val, ok := crd.Labels[crdgeneration.TF2CRDLabel]; ok && val == "true" { + su, err := tf.Add(r.mgr, crd, r.provider, r.smLoader, r.defaulters, r.jitterGenerator) + if err != nil { + return nil, fmt.Errorf("error adding terraform controller for %v to a manager: %w", crd.Spec.Names.Kind, err) + } + return su, nil } - schemaUpdater = su - // register the controller to automatically create secrets for GSA keys - if isServiceAccountKeyCRD(crd) { - logger.Info("registering the GSA-Key-to-Secret generation controller") - if err := gsakeysecretgenerator.Add(r.mgr, crd, &controller.Deps{JitterGen: r.jitterGenerator}); err != nil { - return nil, fmt.Errorf("error adding the gsa-to-secret generator for %v to a manager: %w", crd.Spec.Names.Kind, err) + // register controllers for direct CRDs + if directbase.ControllerBuilder.IsDirectByGVK(gvk) { + err := directbase.ControllerBuilder.AddController(r.mgr, config, gvk, directbase.Deps{JitterGenerator: r.jitterGenerator}) + if err != nil { + return nil, fmt.Errorf("error adding direct controller for %v to a manager: %w", crd.Spec.Names.Kind, err) } + return schemaUpdater, nil } + logger.Error(fmt.Errorf("unrecognized CRD: %v", crd.Spec.Names.Kind), "skipping controller registration", "group", gvk.Group, "version", gvk.Version, "kind", gvk.Kind) + return nil, nil } return schemaUpdater, nil } diff --git a/pkg/k8s/constants.go b/pkg/k8s/constants.go index 79530bb959..b6348bf672 100644 --- a/pkg/k8s/constants.go +++ b/pkg/k8s/constants.go @@ -125,7 +125,6 @@ var ( KCCVersionLabel = FormatAnnotation("version") ScopedNamespaceLabel = FormatAnnotation("scoped-namespace") DCL2CRDLabel = FormatAnnotation("dcl2crd") - DirectCRDLabel = FormatAnnotation("directcrd") KCCStabilityLabel = FormatAnnotation("stability-level") MutableButUnreadableFieldsAnnotation = FormatAnnotation("mutable-but-unreadable-fields") diff --git a/pkg/test/controller/reconciler/testreconciler.go b/pkg/test/controller/reconciler/testreconciler.go index 7a2d26b806..fd2f8fd5c7 100644 --- a/pkg/test/controller/reconciler/testreconciler.go +++ b/pkg/test/controller/reconciler/testreconciler.go @@ -238,7 +238,7 @@ func (r *TestReconciler) newReconcilerForCRD(crd *apiextensions.CustomResourceDe if crd.GetLabels()[k8s.DCL2CRDLabel] == "true" { return dclcontroller.NewReconciler(r.mgr, crd, r.dclConverter, r.dclConfig, r.smLoader, immediateReconcileRequests, resourceWatcherRoutines, defaulters, jg) } - if crd.GetLabels()[k8s.DirectCRDLabel] == "true" { + if directbase.ControllerBuilder.IsDirectByCRD(crd) { return directbase.ControllerBuilder.NewReconciler(r.mgr, &kcccontroller.Config{HTTPClient: r.httpClient}, immediateReconcileRequests, resourceWatcherRoutines, crd.GroupVersionKind(), jg) } } diff --git a/scripts/github-actions/tests-e2e-direct.sh b/scripts/github-actions/tests-e2e-direct.sh new file mode 100755 index 0000000000..2c3e1c23cf --- /dev/null +++ b/scripts/github-actions/tests-e2e-direct.sh @@ -0,0 +1,47 @@ +#!/bin/bash +# 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. + +set -o errexit +set -o nounset +set -o pipefail + +REPO_ROOT="$(git rev-parse --show-toplevel)" + +cd ${REPO_ROOT}/ + +echo "Downloading envtest assets..." +export KUBEBUILDER_ASSETS=$(go run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest use -p path) + +echo "Running e2e tests samples for LoggingLogMetric direct reconciliation..." + +KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \ +GOLDEN_OBJECT_CHECKS=1 \ +GOLDEN_REQUEST_CHECKS=1 \ +E2E_KUBE_TARGET=envtest RUN_E2E=1 E2E_GCP_TARGET=mock \ + go test -test.count=1 -timeout 600s -v ./tests/e2e -run 'TestAllInSeries/samples/linear-log-metric|TestAllInSeries/samples/exponential-log-metric|TestAllInSeries/samples/int-log-metric|TestAllInSeries/samples/explicit-log-metric' + +echo "Running e2e tests fixtures for LoggingLogMetric direct reconciliation..." + +KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \ +GOLDEN_OBJECT_CHECKS=1 \ +GOLDEN_REQUEST_CHECKS=1 \ +E2E_KUBE_TARGET=envtest RUN_E2E=1 E2E_GCP_TARGET=mock \ + go test -test.count=1 -timeout 600s -v ./tests/e2e -run 'TestAllInSeries/fixtures/explicitlogmetric|TestAllInSeries/fixtures/exponentiallogmetric|TestAllInSeries/fixtures/linearlogmetric|TestAllInSeries/fixtures/logbucketmetric' + +echo "Running scenarios tests for LoggingLogMetric direct reconciliation..." + +KCC_USE_DIRECT_RECONCILERS=LoggingLogMetric \ +GOLDEN_REQUEST_CHECKS=1 E2E_KUBE_TARGET=envtest E2E_GCP_TARGET=mock RUN_E2E=1 \ + go test -test.count=1 -timeout 360s -v ./tests/e2e -run TestE2EScript/scenarios/fields \ No newline at end of file