diff --git a/PROJECT b/PROJECT index 4a9317002..a1996da46 100644 --- a/PROJECT +++ b/PROJECT @@ -19,14 +19,6 @@ resources: kind: Istio path: github.com/istio-ecosystem/sail-operator/api/v1alpha1 version: v1alpha1 -- api: - crdVersion: v1 - namespaced: false - controller: true - domain: sailoperator.io - kind: RemoteIstio - path: github.com/istio-ecosystem/sail-operator/api/v1alpha1 - version: v1alpha1 - api: crdVersion: v1 namespaced: false diff --git a/api/v1alpha1/istio_types.go b/api/v1alpha1/istio_types.go index 0b9715a5f..f20e027a7 100644 --- a/api/v1alpha1/istio_types.go +++ b/api/v1alpha1/istio_types.go @@ -51,10 +51,10 @@ type IstioSpec struct { // +sail:profile // The built-in installation configuration profile to use. // The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - // Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + // Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. // +++PROFILES-DROPDOWN-HIDDEN-UNTIL-WE-FULLY-IMPLEMENT-THEM+++operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Profile",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:fieldGroup:General", "urn:alm:descriptor:com.tectonic.ui:select:ambient", "urn:alm:descriptor:com.tectonic.ui:select:default", "urn:alm:descriptor:com.tectonic.ui:select:demo", "urn:alm:descriptor:com.tectonic.ui:select:empty", "urn:alm:descriptor:com.tectonic.ui:select:external", "urn:alm:descriptor:com.tectonic.ui:select:minimal", "urn:alm:descriptor:com.tectonic.ui:select:preview", "urn:alm:descriptor:com.tectonic.ui:select:remote"} // +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:hidden"} - // +kubebuilder:validation:Enum=ambient;default;demo;empty;openshift-ambient;openshift;preview;stable + // +kubebuilder:validation:Enum=ambient;default;demo;empty;openshift-ambient;openshift;preview;remote;stable Profile string `json:"profile,omitempty"` // Namespace to which the Istio components should be installed. Note that this field is immutable. @@ -227,6 +227,9 @@ const ( // IstioReasonIstiodNotReady indicates that the control plane is fully reconciled, but istiod is not ready. IstioReasonIstiodNotReady IstioConditionReason = "IstiodNotReady" + // IstioReasonRemoteIstiodNotReady indicates that the control plane is fully reconciled, but the remote istiod is not ready. + IstioReasonRemoteIstiodNotReady IstioConditionReason = "RemoteIstiodNotReady" + // IstioReasonReadinessCheckFailed indicates that readiness could not be ascertained. IstioReasonReadinessCheckFailed IstioConditionReason = "ReadinessCheckFailed" ) diff --git a/api/v1alpha1/istiocni_types.go b/api/v1alpha1/istiocni_types.go index aa56044a1..5a9c5d754 100644 --- a/api/v1alpha1/istiocni_types.go +++ b/api/v1alpha1/istiocni_types.go @@ -37,10 +37,10 @@ type IstioCNISpec struct { // +sail:profile // The built-in installation configuration profile to use. // The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - // Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + // Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. // +++PROFILES-DROPDOWN-HIDDEN-UNTIL-WE-FULLY-IMPLEMENT-THEM+++operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Profile",xDescriptors={"urn:alm:descriptor:com.tectonic.ui:fieldGroup:General", "urn:alm:descriptor:com.tectonic.ui:select:ambient", "urn:alm:descriptor:com.tectonic.ui:select:default", "urn:alm:descriptor:com.tectonic.ui:select:demo", "urn:alm:descriptor:com.tectonic.ui:select:empty", "urn:alm:descriptor:com.tectonic.ui:select:external", "urn:alm:descriptor:com.tectonic.ui:select:minimal", "urn:alm:descriptor:com.tectonic.ui:select:preview", "urn:alm:descriptor:com.tectonic.ui:select:remote"} // +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors={"urn:alm:descriptor:com.tectonic.ui:hidden"} - // +kubebuilder:validation:Enum=ambient;default;demo;empty;openshift-ambient;openshift;preview;stable + // +kubebuilder:validation:Enum=ambient;default;demo;empty;openshift-ambient;openshift;preview;remote;stable Profile string `json:"profile,omitempty"` // Namespace to which the Istio CNI component should be installed. diff --git a/api/v1alpha1/istiorevision_types.go b/api/v1alpha1/istiorevision_types.go index 675af9450..b3b103dfe 100644 --- a/api/v1alpha1/istiorevision_types.go +++ b/api/v1alpha1/istiorevision_types.go @@ -28,11 +28,6 @@ const ( // IstioRevisionSpec defines the desired state of IstioRevision // +kubebuilder:validation:XValidation:rule="self.values.global.istioNamespace == self.__namespace__",message="spec.values.global.istioNamespace must match spec.namespace" type IstioRevisionSpec struct { - // Type indicates whether this revision represents a local or a remote control plane installation. - // +kubebuilder:default=Local - // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable" - Type IstioRevisionType `json:"type"` - // +sail:version // Defines the version of Istio to install. // Must be one of: v1.23.2. @@ -178,16 +173,6 @@ const ( IstioRevisionReasonHealthy IstioRevisionConditionReason = "Healthy" ) -type IstioRevisionType string - -const ( - // IstioRevisionTypeLocal indicates that the revision represents a local control plane installation. - IstioRevisionTypeLocal IstioRevisionType = "Local" - - // IstioRevisionTypeRemote indicates that the revision represents a remote control plane installation. - IstioRevisionTypeRemote IstioRevisionType = "Remote" -) - // +kubebuilder:object:root=true // +kubebuilder:resource:scope=Cluster,shortName=istiorev,categories=istio-io // +kubebuilder:subresource:status diff --git a/bundle/manifests/sailoperator.io_istiocnis.yaml b/bundle/manifests/sailoperator.io_istiocnis.yaml index 29623eb00..d7ecca2eb 100644 --- a/bundle/manifests/sailoperator.io_istiocnis.yaml +++ b/bundle/manifests/sailoperator.io_istiocnis.yaml @@ -70,7 +70,7 @@ spec: description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. enum: - ambient - default @@ -79,6 +79,7 @@ spec: - openshift-ambient - openshift - preview + - remote - stable type: string values: diff --git a/bundle/manifests/sailoperator.io_istiorevisions.yaml b/bundle/manifests/sailoperator.io_istiorevisions.yaml index 8e871f2ee..60bb82f60 100644 --- a/bundle/manifests/sailoperator.io_istiorevisions.yaml +++ b/bundle/manifests/sailoperator.io_istiorevisions.yaml @@ -78,14 +78,6 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf - type: - default: Local - description: Type indicates whether this revision represents a local - or a remote control plane installation. - type: string - x-kubernetes-validations: - - message: Value is immutable - rule: self == oldSelf values: description: Defines the values to be passed to the Helm charts when installing Istio. @@ -9386,7 +9378,6 @@ spec: type: string required: - namespace - - type - version type: object x-kubernetes-validations: diff --git a/bundle/manifests/sailoperator.io_istios.yaml b/bundle/manifests/sailoperator.io_istios.yaml index 642746603..d7d969c44 100644 --- a/bundle/manifests/sailoperator.io_istios.yaml +++ b/bundle/manifests/sailoperator.io_istios.yaml @@ -95,7 +95,7 @@ spec: description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. enum: - ambient - default @@ -104,6 +104,7 @@ spec: - openshift-ambient - openshift - preview + - remote - stable type: string updateStrategy: diff --git a/bundle/manifests/servicemeshoperator3.clusterserviceversion.yaml b/bundle/manifests/servicemeshoperator3.clusterserviceversion.yaml index 0e7193635..c0e09e50f 100644 --- a/bundle/manifests/servicemeshoperator3.clusterserviceversion.yaml +++ b/bundle/manifests/servicemeshoperator3.clusterserviceversion.yaml @@ -34,7 +34,7 @@ metadata: capabilities: Seamless Upgrades categories: OpenShift Optional, Integration & Delivery, Networking, Security containerImage: quay.io/maistra-dev/sail-operator:3.0-latest - createdAt: "2024-10-23T21:09:47Z" + createdAt: "2024-11-12T12:13:40Z" description: The OpenShift Service Mesh Operator enables you to install, configure, and manage an instance of Red Hat OpenShift Service Mesh. OpenShift Service Mesh is based on the open source Istio project. @@ -133,6 +133,9 @@ spec: - kind: WorkloadGroup name: workloadgroups.networking.istio.io version: v1beta1 + - kind: RemoteIstio + name: remoteistios.sailoperator.io + version: v1alpha1 - kind: AuthorizationPolicy name: authorizationpolicies.security.istio.io version: v1 @@ -178,7 +181,7 @@ spec: - description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. displayName: Profile path: profile x-descriptors: @@ -276,7 +279,7 @@ spec: - description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. displayName: Profile path: profile x-descriptors: @@ -290,75 +293,6 @@ spec: displayName: Helm Values path: values version: v1alpha1 - - description: |- - RemoteIstio represents a remote Istio Service Mesh deployment consisting of one or more - remote control plane instances (represented by one or more IstioRevision objects). - displayName: Remote Istio - kind: RemoteIstio - name: remoteistios.sailoperator.io - specDescriptors: - - description: "Type of strategy to use. Can be \"InPlace\" or \"RevisionBased\". - When the \"InPlace\" strategy\nis used, the existing Istio control plane - is updated in-place. The workloads therefore\ndon't need to be moved from - one control plane instance to another. When the \"RevisionBased\"\nstrategy - is used, a new Istio control plane instance is created for every change - to the\nIstio.spec.version field. The old control plane remains in place - until all workloads have\nbeen moved to the new control plane instance.\n\n\nThe - \"InPlace\" strategy is the default.\tTODO: change default to \"RevisionBased\"" - displayName: Type - path: updateStrategy.type - x-descriptors: - - urn:alm:descriptor:com.tectonic.ui:select:InPlace - - urn:alm:descriptor:com.tectonic.ui:select:RevisionBased - - description: |- - Defines the version of Istio to install. - Must be one of: v1.23.2. - displayName: Istio Version - path: version - x-descriptors: - - urn:alm:descriptor:com.tectonic.ui:fieldGroup:General - - urn:alm:descriptor:com.tectonic.ui:select:v1.23.2 - - description: |- - Defines how many seconds the operator should wait before removing a non-active revision after all - the workloads have stopped using it. You may want to set this value on the order of minutes. - The minimum is 0 and the default value is 30. - displayName: Inactive Revision Deletion Grace Period (seconds) - path: updateStrategy.inactiveRevisionDeletionGracePeriodSeconds - x-descriptors: - - urn:alm:descriptor:com.tectonic.ui:number - - description: |- - Defines whether the workloads should be moved from one control plane instance to another - automatically. If updateWorkloads is true, the operator moves the workloads from the old - control plane instance to the new one after the new control plane is ready. - If updateWorkloads is false, the user must move the workloads manually by updating the - istio.io/rev labels on the namespace and/or the pods. - Defaults to false. - displayName: Update Workloads Automatically - path: updateStrategy.updateWorkloads - x-descriptors: - - urn:alm:descriptor:com.tectonic.ui:booleanSwitch - - description: Namespace to which the Istio components should be installed. - displayName: Namespace - path: namespace - x-descriptors: - - urn:alm:descriptor:io.kubernetes:Namespace - - description: |- - The built-in installation configuration profile to use. - The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. - displayName: Profile - path: profile - x-descriptors: - - urn:alm:descriptor:com.tectonic.ui:hidden - - description: Defines the update strategy to use when the version in the RemoteIstio - CR is updated. - displayName: Update Strategy - path: updateStrategy - - description: Defines the values to be passed to the Helm charts when installing - Istio. - displayName: Helm Values - path: values - version: v1alpha1 description: |- Red Hat OpenShift Service Mesh is a platform that provides behavioral insight and operational control over a service mesh, providing a uniform way to connect, secure, and monitor microservice applications. diff --git a/chart/crds/sailoperator.io_istiocnis.yaml b/chart/crds/sailoperator.io_istiocnis.yaml index 1acbea7bb..8cbda3137 100644 --- a/chart/crds/sailoperator.io_istiocnis.yaml +++ b/chart/crds/sailoperator.io_istiocnis.yaml @@ -70,7 +70,7 @@ spec: description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. enum: - ambient - default @@ -79,6 +79,7 @@ spec: - openshift-ambient - openshift - preview + - remote - stable type: string values: diff --git a/chart/crds/sailoperator.io_istiorevisions.yaml b/chart/crds/sailoperator.io_istiorevisions.yaml index ee7a4eac0..aa6be149b 100644 --- a/chart/crds/sailoperator.io_istiorevisions.yaml +++ b/chart/crds/sailoperator.io_istiorevisions.yaml @@ -78,14 +78,6 @@ spec: x-kubernetes-validations: - message: Value is immutable rule: self == oldSelf - type: - default: Local - description: Type indicates whether this revision represents a local - or a remote control plane installation. - type: string - x-kubernetes-validations: - - message: Value is immutable - rule: self == oldSelf values: description: Defines the values to be passed to the Helm charts when installing Istio. @@ -9386,7 +9378,6 @@ spec: type: string required: - namespace - - type - version type: object x-kubernetes-validations: diff --git a/chart/crds/sailoperator.io_istios.yaml b/chart/crds/sailoperator.io_istios.yaml index 2cd9cf0a0..35ef4ba6c 100644 --- a/chart/crds/sailoperator.io_istios.yaml +++ b/chart/crds/sailoperator.io_istios.yaml @@ -95,7 +95,7 @@ spec: description: |- The built-in installation configuration profile to use. The 'default' profile is always applied. On OpenShift, the 'openshift' profile is also applied on top of 'default'. - Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, stable. + Must be one of: ambient, default, demo, empty, openshift-ambient, openshift, preview, remote, stable. enum: - ambient - default @@ -104,6 +104,7 @@ spec: - openshift-ambient - openshift - preview + - remote - stable type: string updateStrategy: diff --git a/chart/samples/remoteistio-sample.yaml b/chart/samples/remoteistio-sample.yaml deleted file mode 100644 index 66407f939..000000000 --- a/chart/samples/remoteistio-sample.yaml +++ /dev/null @@ -1,15 +0,0 @@ -apiVersion: sailoperator.io/v1alpha1 -kind: RemoteIstio -metadata: - name: default -spec: - version: latest - namespace: istio-system - updateStrategy: - type: InPlace - inactiveRevisionDeletionGracePeriodSeconds: 30 - values: - istiodRemote: - injectionPath: /inject/cluster/cluster2/net/network1 - global: - remotePilotAddress: 1.2.3.4 \ No newline at end of file diff --git a/cmd/main.go b/cmd/main.go index d713a8131..1d8b4cac2 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -23,7 +23,6 @@ import ( "github.com/istio-ecosystem/sail-operator/controllers/istio" "github.com/istio-ecosystem/sail-operator/controllers/istiocni" "github.com/istio-ecosystem/sail-operator/controllers/istiorevision" - "github.com/istio-ecosystem/sail-operator/controllers/remoteistio" "github.com/istio-ecosystem/sail-operator/controllers/webhook" "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" @@ -144,13 +143,6 @@ func main() { os.Exit(1) } - err = remoteistio.NewReconciler(reconcilerCfg, mgr.GetClient(), mgr.GetScheme()). - SetupWithManager(mgr) - if err != nil { - setupLog.Error(err, "unable to create controller", "controller", "RemoteIstio") - os.Exit(1) - } - err = istiorevision.NewReconciler(reconcilerCfg, mgr.GetClient(), mgr.GetScheme(), chartManager). SetupWithManager(mgr) if err != nil { diff --git a/common/config/.golangci.yml b/common/config/.golangci.yml index 2c0565492..b5a0349b9 100644 --- a/common/config/.golangci.yml +++ b/common/config/.golangci.yml @@ -18,7 +18,7 @@ linters: disable-all: true enable: - errcheck - - exportloopref + - copyloopvar - depguard - gocritic - gofumpt diff --git a/controllers/istio/istio_controller.go b/controllers/istio/istio_controller.go index 24043a2ec..b013c9bf1 100644 --- a/controllers/istio/istio_controller.go +++ b/controllers/istio/istio_controller.go @@ -115,7 +115,6 @@ func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha return revision.CreateOrUpdate(ctx, r.Client, getActiveRevisionName(istio), - v1alpha1.IstioRevisionTypeLocal, istio.Spec.Version, istio.Spec.Namespace, values, metav1.OwnerReference{ APIVersion: v1alpha1.GroupVersion.String(), @@ -324,6 +323,8 @@ func convertConditionReason(reason v1alpha1.IstioRevisionConditionReason) v1alph return v1alpha1.IstioReasonReadinessCheckFailed case v1alpha1.IstioRevisionReasonReconcileError: return v1alpha1.IstioReasonReconcileError + case v1alpha1.IstioRevisionReasonRemoteIstiodNotReady: + return v1alpha1.IstioReasonRemoteIstiodNotReady default: panic(fmt.Sprintf("can't convert IstioRevisionConditionReason: %s", reason)) } diff --git a/controllers/istio/istio_controller_test.go b/controllers/istio/istio_controller_test.go index f91a9db54..129acf0ea 100644 --- a/controllers/istio/istio_controller_test.go +++ b/controllers/istio/istio_controller_test.go @@ -527,7 +527,6 @@ func TestDetermineStatus(t *testing.T) { initObjs := []client.Object{istio} for _, rev := range tc.revisions { - rev := rev initObjs = append(initObjs, &rev) } @@ -728,7 +727,6 @@ func TestUpdateStatus(t *testing.T) { initObjs := []client.Object{istio} for _, rev := range tc.revisions { - rev := rev initObjs = append(initObjs, &rev) } diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index d8b031b1f..23f558083 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -30,6 +30,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/istiovalues" "github.com/istio-ecosystem/sail-operator/pkg/kube" + "github.com/istio-ecosystem/sail-operator/pkg/predicate" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" "github.com/istio-ecosystem/sail-operator/pkg/validation" appsv1 "k8s.io/api/apps/v1" @@ -40,6 +41,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -225,7 +227,11 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&corev1.ConfigMap{}, ownedResourceHandler). Watches(&appsv1.DaemonSet{}, ownedResourceHandler). Watches(&corev1.ResourceQuota{}, ownedResourceHandler). - Watches(&corev1.ServiceAccount{}, ownedResourceHandler). + + // We use predicate.IgnoreUpdate() so that we skip the reconciliation when a pull secret is added to the ServiceAccount. + // This is necessary so that we don't remove the newly-added secret. + // TODO: this is a temporary hack until we implement the correct solution on the Helm-render side + Watches(&corev1.ServiceAccount{}, ownedResourceHandler, builder.WithPredicates(predicate.IgnoreUpdate())). // TODO: only register NetAttachDef if the CRD is installed (may also need to watch for CRD creation) // Owns(&multusv1.NetworkAttachmentDefinition{}). diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 13d162684..f845833ef 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -30,7 +30,9 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/errlist" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/kube" + predicate2 "github.com/istio-ecosystem/sail-operator/pkg/predicate" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" + "github.com/istio-ecosystem/sail-operator/pkg/revision" "github.com/istio-ecosystem/sail-operator/pkg/validation" admissionv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -61,8 +63,7 @@ const ( IstioRevLabel = "istio.io/rev" IstioSidecarInjectLabel = "sidecar.istio.io/inject" - istiodChartName = "istiod" - istiodRemoteChartName = "istiod-remote" + istiodChartName = "istiod" ) // Reconciler reconciles an IstioRevision object @@ -128,9 +129,6 @@ func (r *Reconciler) Finalize(ctx context.Context, rev *v1alpha1.IstioRevision) } func (r *Reconciler) validate(ctx context.Context, rev *v1alpha1.IstioRevision) error { - if rev.Spec.Type == "" { - return reconciler.NewValidationError("spec.type not set") - } if rev.Spec.Version == "" { return reconciler.NewValidationError("spec.version not set") } @@ -172,33 +170,22 @@ func (r *Reconciler) installHelmCharts(ctx context.Context, rev *v1alpha1.IstioR _, err := r.ChartManager.UpgradeOrInstallChart(ctx, r.getChartDir(rev), values, rev.Spec.Namespace, getReleaseName(rev), ownerReference) if err != nil { - return fmt.Errorf("failed to install/update Helm chart %q: %w", getChartName(rev), err) + return fmt.Errorf("failed to install/update Helm chart %q: %w", istiodChartName, err) } return nil } func getReleaseName(rev *v1alpha1.IstioRevision) string { - return fmt.Sprintf("%s-%s", rev.Name, getChartName(rev)) + return fmt.Sprintf("%s-%s", rev.Name, istiodChartName) } func (r *Reconciler) getChartDir(rev *v1alpha1.IstioRevision) string { - return path.Join(r.Config.ResourceDirectory, rev.Spec.Version, "charts", getChartName(rev)) -} - -func getChartName(rev *v1alpha1.IstioRevision) string { - switch rev.Spec.Type { - case v1alpha1.IstioRevisionTypeLocal: - return istiodChartName - case v1alpha1.IstioRevisionTypeRemote: - return istiodRemoteChartName - default: - panic(badIstioRevisionType(rev)) - } + return path.Join(r.Config.ResourceDirectory, rev.Spec.Version, "charts", istiodChartName) } func (r *Reconciler) uninstallHelmCharts(ctx context.Context, rev *v1alpha1.IstioRevision) error { if _, err := r.ChartManager.UninstallChart(ctx, getReleaseName(rev), rev.Spec.Namespace); err != nil { - return fmt.Errorf("failed to uninstall Helm chart %q: %w", getChartName(rev), err) + return fmt.Errorf("failed to uninstall Helm chart %q: %w", istiodChartName, err) } return nil } @@ -246,7 +233,11 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { Watches(&appsv1.Deployment{}, ownedResourceHandler). // we don't ignore the status here because we use it to calculate the IstioRevision status Watches(&corev1.Endpoints{}, ownedResourceHandler). Watches(&corev1.Service{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). - Watches(&corev1.ServiceAccount{}, ownedResourceHandler). + + // We use predicate.IgnoreUpdate() so that we skip the reconciliation when a pull secret is added to the ServiceAccount. + // This is necessary so that we don't remove the newly-added secret. + // TODO: this is a temporary hack until we implement the correct solution on the Helm-render side + Watches(&corev1.ServiceAccount{}, ownedResourceHandler, builder.WithPredicates(predicate2.IgnoreUpdate())). Watches(&rbacv1.Role{}, ownedResourceHandler). Watches(&rbacv1.RoleBinding{}, ownedResourceHandler). Watches(&policyv1.PodDisruptionBudget{}, ownedResourceHandler, builder.WithPredicates(ignoreStatusChange())). @@ -332,8 +323,7 @@ func (r *Reconciler) determineReadyCondition(ctx context.Context, rev *v1alpha1. Status: metav1.ConditionFalse, } - switch rev.Spec.Type { - case v1alpha1.IstioRevisionTypeLocal: + if !revision.IsUsingRemoteControlPlane(rev) { istiod := appsv1.Deployment{} if err := r.Client.Get(ctx, istiodDeploymentKey(rev), &istiod); err == nil { if istiod.Status.Replicas == 0 { @@ -354,7 +344,7 @@ func (r *Reconciler) determineReadyCondition(ctx context.Context, rev *v1alpha1. c.Message = fmt.Sprintf("failed to get readiness: %v", err) return c, fmt.Errorf("get failed: %w", err) } - case v1alpha1.IstioRevisionTypeRemote: + } else { webhook := admissionv1.MutatingWebhookConfiguration{} webhookKey := injectionWebhookKey(rev) if err := r.Client.Get(ctx, webhookKey, &webhook); err == nil { @@ -378,8 +368,6 @@ func (r *Reconciler) determineReadyCondition(ctx context.Context, rev *v1alpha1. c.Message = fmt.Sprintf("failed to get readiness: %v", err) return c, fmt.Errorf("get failed: %w", err) } - default: - panic(badIstioRevisionType(rev)) } return c, nil } @@ -610,10 +598,6 @@ func clearIgnoredFields(obj client.Object) { } } -func badIstioRevisionType(rev *v1alpha1.IstioRevision) string { - return fmt.Sprintf("unknown IstioRevisionType: %s", rev.Spec.Type) -} - func wrapEventHandler(logger logr.Logger, handler handler.EventHandler) handler.EventHandler { return enqueuelogger.WrapIfNecessary(v1alpha1.IstioRevisionKind, logger, handler) } diff --git a/controllers/istiorevision/istiorevision_controller_test.go b/controllers/istiorevision/istiorevision_controller_test.go index c27b76129..7c2f21b97 100644 --- a/controllers/istiorevision/istiorevision_controller_test.go +++ b/controllers/istiorevision/istiorevision_controller_test.go @@ -60,7 +60,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", Values: &v1alpha1.Values{ @@ -73,25 +72,6 @@ func TestValidate(t *testing.T) { objects: []client.Object{ns}, expectErr: "", }, - { - name: "no type", - rev: &v1alpha1.IstioRevision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - }, - Spec: v1alpha1.IstioRevisionSpec{ - Version: supportedversion.Default, - Namespace: "istio-system", - Values: &v1alpha1.Values{ - Global: &v1alpha1.GlobalConfig{ - IstioNamespace: ptr.Of("istio-system"), - }, - }, - }, - }, - objects: []client.Object{ns}, - expectErr: `spec.type not set`, - }, { name: "no version", rev: &v1alpha1.IstioRevision{ @@ -99,7 +79,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Namespace: "istio-system", }, }, @@ -113,7 +92,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, }, }, @@ -127,7 +105,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", }, @@ -142,7 +119,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", }, @@ -157,7 +133,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", Values: &v1alpha1.Values{ @@ -177,7 +152,6 @@ func TestValidate(t *testing.T) { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", Values: &v1alpha1.Values{ @@ -198,7 +172,6 @@ func TestValidate(t *testing.T) { Name: "my-revision", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: "istio-system", Values: &v1alpha1.Values{ @@ -293,7 +266,6 @@ func TestDetermineReadyCondition(t *testing.T) { testCases := []struct { name string - revType v1alpha1.IstioRevisionType values *v1alpha1.Values clientObjects []client.Object interceptors interceptor.Funcs @@ -418,9 +390,8 @@ func TestDetermineReadyCondition(t *testing.T) { expectErr: true, }, { - name: "Istiod-remote ready", - revType: v1alpha1.IstioRevisionTypeRemote, - values: nil, + name: "Istiod-remote ready", + values: &v1alpha1.Values{Profile: ptr.Of("remote")}, clientObjects: []client.Object{ &admissionv1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -437,9 +408,8 @@ func TestDetermineReadyCondition(t *testing.T) { }, }, { - name: "Istiod-remote not ready", - revType: v1alpha1.IstioRevisionTypeRemote, - values: nil, + name: "Istiod-remote not ready", + values: &v1alpha1.Values{Profile: ptr.Of("remote")}, clientObjects: []client.Object{ &admissionv1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -458,9 +428,8 @@ func TestDetermineReadyCondition(t *testing.T) { }, }, { - name: "Istiod-remote no readiness probe status annotation", - revType: v1alpha1.IstioRevisionTypeRemote, - values: nil, + name: "Istiod-remote no readiness probe status annotation", + values: &v1alpha1.Values{Profile: ptr.Of("remote")}, clientObjects: []client.Object{ &admissionv1.MutatingWebhookConfiguration{ ObjectMeta: metav1.ObjectMeta{ @@ -478,8 +447,7 @@ func TestDetermineReadyCondition(t *testing.T) { }, { name: "Istiod-remote webhook config not found", - revType: v1alpha1.IstioRevisionTypeRemote, - values: nil, + values: &v1alpha1.Values{Profile: ptr.Of("remote")}, clientObjects: []client.Object{}, expected: v1alpha1.IstioRevisionCondition{ Type: v1alpha1.IstioRevisionConditionReady, @@ -490,7 +458,7 @@ func TestDetermineReadyCondition(t *testing.T) { }, { name: "Istiod-remote client error on get", - revType: v1alpha1.IstioRevisionTypeRemote, + values: &v1alpha1.Values{Profile: ptr.Of("remote")}, clientObjects: []client.Object{}, interceptors: interceptor.Funcs{ Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { @@ -515,16 +483,12 @@ func TestDetermineReadyCondition(t *testing.T) { r := NewReconciler(cfg, cl, scheme.Scheme, nil) - if tt.revType == "" { - tt.revType = v1alpha1.IstioRevisionTypeLocal - } rev := &v1alpha1.IstioRevision{ ObjectMeta: metav1.ObjectMeta{ Name: "my-istio", }, Spec: v1alpha1.IstioRevisionSpec{ Namespace: "istio-system", - Type: tt.revType, Values: tt.values, }, } diff --git a/controllers/remoteistio/remoteistio_controller.go b/controllers/remoteistio/remoteistio_controller.go deleted file mode 100644 index fe92ebd00..000000000 --- a/controllers/remoteistio/remoteistio_controller.go +++ /dev/null @@ -1,315 +0,0 @@ -// Copyright Istio Authors -// -// 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 remoteistio - -import ( - "context" - "errors" - "fmt" - "reflect" - "strings" - "time" - - "github.com/go-logr/logr" - "github.com/istio-ecosystem/sail-operator/api/v1alpha1" - "github.com/istio-ecosystem/sail-operator/pkg/config" - "github.com/istio-ecosystem/sail-operator/pkg/errlist" - "github.com/istio-ecosystem/sail-operator/pkg/kube" - "github.com/istio-ecosystem/sail-operator/pkg/reconciler" - "github.com/istio-ecosystem/sail-operator/pkg/revision" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller" - logf "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - - "istio.io/istio/pkg/ptr" -) - -// Reconciler reconciles a RemoteIstio object -type Reconciler struct { - Config config.ReconcilerConfig - client.Client - Scheme *runtime.Scheme -} - -func NewReconciler(cfg config.ReconcilerConfig, client client.Client, scheme *runtime.Scheme) *Reconciler { - return &Reconciler{ - Config: cfg, - Client: client, - Scheme: scheme, - } -} - -// +kubebuilder:rbac:groups=sailoperator.io,resources=remoteistios,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=sailoperator.io,resources=remoteistios/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=sailoperator.io,resources=remoteistios/finalizers,verbs=update - -// Reconcile is part of the main kubernetes reconciliation loop which aims to -// move the current state of the cluster closer to the desired state. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.1/pkg/reconcile -func (r *Reconciler) Reconcile(ctx context.Context, istio *v1alpha1.RemoteIstio) (ctrl.Result, error) { - log := logf.FromContext(ctx) - - log.Info("Reconciling") - result, reconcileErr := r.doReconcile(ctx, istio) - - log.Info("Reconciliation done. Updating status.") - statusErr := r.updateStatus(ctx, istio, reconcileErr) - - return result, errors.Join(reconcileErr, statusErr) -} - -// doReconcile is the function that actually reconciles the Istio object. Any error reported by this -// function should get reported in the status of the Istio object by the caller. -func (r *Reconciler) doReconcile(ctx context.Context, istio *v1alpha1.RemoteIstio) (result ctrl.Result, err error) { - if err := validate(istio); err != nil { - return ctrl.Result{}, err - } - - if err = r.reconcileActiveRevision(ctx, istio); err != nil { - return ctrl.Result{}, err - } - - return revision.PruneInactive(ctx, r.Client, istio.UID, getActiveRevisionName(istio), getPruningGracePeriod(istio)) -} - -func validate(istio *v1alpha1.RemoteIstio) error { - if istio.Spec.Version == "" { - return reconciler.NewValidationError("spec.version not set") - } - if istio.Spec.Namespace == "" { - return reconciler.NewValidationError("spec.namespace not set") - } - return nil -} - -func (r *Reconciler) reconcileActiveRevision(ctx context.Context, istio *v1alpha1.RemoteIstio) error { - values, err := revision.ComputeValues( - istio.Spec.Values, istio.Spec.Namespace, istio.Spec.Version, - r.Config.Platform, r.Config.DefaultProfile, istio.Spec.Profile, - r.Config.ResourceDirectory, getActiveRevisionName(istio)) - if err != nil { - return err - } - - return revision.CreateOrUpdate(ctx, r.Client, - getActiveRevisionName(istio), - v1alpha1.IstioRevisionTypeRemote, - istio.Spec.Version, istio.Spec.Namespace, values, - metav1.OwnerReference{ - APIVersion: v1alpha1.GroupVersion.String(), - Kind: v1alpha1.RemoteIstioKind, - Name: istio.Name, - UID: istio.UID, - Controller: ptr.Of(true), - BlockOwnerDeletion: ptr.Of(true), - }) -} - -func getPruningGracePeriod(istio *v1alpha1.RemoteIstio) time.Duration { - strategy := istio.Spec.UpdateStrategy - period := int64(v1alpha1.DefaultRevisionDeletionGracePeriodSeconds) - if strategy != nil && strategy.InactiveRevisionDeletionGracePeriodSeconds != nil { - period = *strategy.InactiveRevisionDeletionGracePeriodSeconds - } - if period < v1alpha1.MinRevisionDeletionGracePeriodSeconds { - period = v1alpha1.MinRevisionDeletionGracePeriodSeconds - } - return time.Duration(period) * time.Second -} - -func (r *Reconciler) getActiveRevision(ctx context.Context, istio *v1alpha1.RemoteIstio) (v1alpha1.IstioRevision, error) { - rev := v1alpha1.IstioRevision{} - err := r.Client.Get(ctx, getActiveRevisionKey(istio), &rev) - if err != nil { - return rev, fmt.Errorf("get failed: %w", err) - } - return rev, nil -} - -func getActiveRevisionKey(istio *v1alpha1.RemoteIstio) types.NamespacedName { - return types.NamespacedName{ - Name: getActiveRevisionName(istio), - } -} - -func getActiveRevisionName(istio *v1alpha1.RemoteIstio) string { - var strategy v1alpha1.UpdateStrategyType - if istio.Spec.UpdateStrategy != nil { - strategy = istio.Spec.UpdateStrategy.Type - } - - switch strategy { - default: - fallthrough - case v1alpha1.UpdateStrategyTypeInPlace: - return istio.Name - case v1alpha1.UpdateStrategyTypeRevisionBased: - return istio.Name + "-" + strings.ReplaceAll(istio.Spec.Version, ".", "-") - } -} - -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - WithOptions(controller.Options{ - LogConstructor: func(req *reconcile.Request) logr.Logger { - log := mgr.GetLogger().WithName("ctrlr").WithName("remoteistio") - if req != nil { - log = log.WithValues("RemoteIstio", req.Name) - } - return log - }, - }). - For(&v1alpha1.RemoteIstio{}). - Owns(&v1alpha1.IstioRevision{}). - Complete(reconciler.NewStandardReconciler[*v1alpha1.RemoteIstio](r.Client, r.Reconcile)) -} - -func (r *Reconciler) determineStatus(ctx context.Context, istio *v1alpha1.RemoteIstio, reconcileErr error) (v1alpha1.RemoteIstioStatus, error) { - var errs errlist.Builder - status := *istio.Status.DeepCopy() - status.ObservedGeneration = istio.Generation - - // set Reconciled and Ready conditions - if reconcileErr != nil { - status.SetCondition(v1alpha1.RemoteIstioCondition{ - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonReconcileError, - Message: reconcileErr.Error(), - }) - status.SetCondition(v1alpha1.RemoteIstioCondition{ - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionUnknown, - Reason: v1alpha1.RemoteIstioReasonReconcileError, - Message: "cannot determine readiness due to reconciliation error", - }) - status.State = v1alpha1.RemoteIstioReasonReconcileError - } else { - status.ActiveRevisionName = getActiveRevisionName(istio) - rev, err := r.getActiveRevision(ctx, istio) - if apierrors.IsNotFound(err) { - revisionNotFound := func(conditionType v1alpha1.RemoteIstioConditionType) v1alpha1.RemoteIstioCondition { - return v1alpha1.RemoteIstioCondition{ - Type: conditionType, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - } - } - - status.SetCondition(revisionNotFound(v1alpha1.RemoteIstioConditionReconciled)) - status.SetCondition(revisionNotFound(v1alpha1.RemoteIstioConditionReady)) - status.State = v1alpha1.RemoteIstioReasonRevisionNotFound - } else if err == nil { - status.SetCondition(convertCondition(rev.Status.GetCondition(v1alpha1.IstioRevisionConditionReconciled))) - status.SetCondition(convertCondition(rev.Status.GetCondition(v1alpha1.IstioRevisionConditionReady))) - status.State = convertConditionReason(rev.Status.State) - } else { - activeRevisionGetFailed := func(conditionType v1alpha1.RemoteIstioConditionType) v1alpha1.RemoteIstioCondition { - return v1alpha1.RemoteIstioCondition{ - Type: conditionType, - Status: metav1.ConditionUnknown, - Reason: v1alpha1.RemoteIstioReasonFailedToGetActiveRevision, - Message: fmt.Sprintf("failed to get active IstioRevision: %s", err), - } - } - status.SetCondition(activeRevisionGetFailed(v1alpha1.RemoteIstioConditionReconciled)) - status.SetCondition(activeRevisionGetFailed(v1alpha1.RemoteIstioConditionReady)) - status.State = v1alpha1.RemoteIstioReasonFailedToGetActiveRevision - errs.Add(fmt.Errorf("failed to get active IstioRevision: %w", err)) - } - } - - // count the ready, in-use, and total revisions - if revs, err := revision.ListOwned(ctx, r.Client, istio.UID); err == nil { - status.Revisions.Total = int32(len(revs)) - status.Revisions.Ready = 0 - status.Revisions.InUse = 0 - for _, rev := range revs { - if rev.Status.GetCondition(v1alpha1.IstioRevisionConditionReady).Status == metav1.ConditionTrue { - status.Revisions.Ready++ - } - if rev.Status.GetCondition(v1alpha1.IstioRevisionConditionInUse).Status == metav1.ConditionTrue { - status.Revisions.InUse++ - } - } - } else { - status.Revisions.Total = -1 - status.Revisions.Ready = -1 - status.Revisions.InUse = -1 - errs.Add(err) - } - return status, errs.Error() -} - -func (r *Reconciler) updateStatus(ctx context.Context, istio *v1alpha1.RemoteIstio, reconcileErr error) error { - var errs errlist.Builder - status, err := r.determineStatus(ctx, istio, reconcileErr) - if err != nil { - errs.Add(fmt.Errorf("failed to determine status: %w", err)) - } - - if !reflect.DeepEqual(istio.Status, status) { - if err := r.Client.Status().Patch(ctx, istio, kube.NewStatusPatch(status)); err != nil { - errs.Add(fmt.Errorf("failed to patch status: %w", err)) - } - } - return errs.Error() -} - -func convertCondition(condition v1alpha1.IstioRevisionCondition) v1alpha1.RemoteIstioCondition { - return v1alpha1.RemoteIstioCondition{ - Type: convertConditionType(condition), - Status: condition.Status, - Reason: convertConditionReason(condition.Reason), - Message: condition.Message, - } -} - -func convertConditionType(condition v1alpha1.IstioRevisionCondition) v1alpha1.RemoteIstioConditionType { - switch condition.Type { - case v1alpha1.IstioRevisionConditionReconciled: - return v1alpha1.RemoteIstioConditionReconciled - case v1alpha1.IstioRevisionConditionReady: - return v1alpha1.RemoteIstioConditionReady - default: - panic(fmt.Sprintf("can't convert IstioRevisionConditionType: %s", condition.Type)) - } -} - -func convertConditionReason(reason v1alpha1.IstioRevisionConditionReason) v1alpha1.RemoteIstioConditionReason { - switch reason { - case "": - return "" - case v1alpha1.IstioRevisionReasonRemoteIstiodNotReady: - return v1alpha1.RemoteIstioReasonIstiodNotReady - case v1alpha1.IstioRevisionReasonHealthy: - return v1alpha1.RemoteIstioReasonHealthy - case v1alpha1.IstioRevisionReasonReadinessCheckFailed: - return v1alpha1.RemoteIstioReasonReadinessCheckFailed - case v1alpha1.IstioRevisionReasonReconcileError: - return v1alpha1.RemoteIstioReasonReconcileError - default: - panic(fmt.Sprintf("can't convert IstioRevisionConditionReason: %s", reason)) - } -} diff --git a/controllers/remoteistio/remoteistio_controller_test.go b/controllers/remoteistio/remoteistio_controller_test.go deleted file mode 100644 index 8b74879f2..000000000 --- a/controllers/remoteistio/remoteistio_controller_test.go +++ /dev/null @@ -1,927 +0,0 @@ -// Copyright Istio Authors -// -// 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 remoteistio - -import ( - "context" - "fmt" - "runtime/debug" - "testing" - "time" - - "github.com/google/go-cmp/cmp" - "github.com/istio-ecosystem/sail-operator/api/v1alpha1" - "github.com/istio-ecosystem/sail-operator/pkg/config" - "github.com/istio-ecosystem/sail-operator/pkg/scheme" - "github.com/istio-ecosystem/sail-operator/pkg/test/testtime" - "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" - . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" - "sigs.k8s.io/controller-runtime/pkg/client/interceptor" - - "istio.io/istio/pkg/ptr" -) - -var ( - ctx = context.Background() - istioNamespace = "my-istio-namespace" - istioName = "my-istio" - istioKey = types.NamespacedName{ - Name: istioName, - } - istioUID = types.UID("my-istio-uid") - objectMeta = metav1.ObjectMeta{ - Name: istioKey.Name, - } -) - -func TestReconcile(t *testing.T) { - cfg := newReconcilerTestConfig(t) - - t.Run("returns error when Istio version not set", func(t *testing.T) { - istio := &v1alpha1.RemoteIstio{ - ObjectMeta: objectMeta, - } - - cl := newFakeClientBuilder(). - WithObjects(istio). - Build() - reconciler := NewReconciler(cfg, cl, scheme.Scheme) - - _, err := reconciler.Reconcile(ctx, istio) - if err == nil { - t.Errorf("Expected an error, but got nil") - } - - Must(t, cl.Get(ctx, istioKey, istio)) - - if istio.Status.State != v1alpha1.RemoteIstioReasonReconcileError { - t.Errorf("Expected status.state to be %q, but got %q", v1alpha1.RemoteIstioReasonReconcileError, istio.Status.State) - } - - reconciledCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReconciled) - if reconciledCond.Status != metav1.ConditionFalse { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionFalse, reconciledCond.Status) - } - - readyCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReady) - if readyCond.Status != metav1.ConditionUnknown { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionUnknown, readyCond.Status) - } - }) - - t.Run("returns error when computeIstioRevisionValues fails", func(t *testing.T) { - istio := &v1alpha1.RemoteIstio{ - ObjectMeta: objectMeta, - Spec: v1alpha1.RemoteIstioSpec{ - Version: "my-version", - }, - } - - cl := newFakeClientBuilder(). - WithStatusSubresource(&v1alpha1.RemoteIstio{}). - WithObjects(istio). - Build() - cfg := newReconcilerTestConfig(t) - cfg.DefaultProfile = "invalid-profile" - reconciler := NewReconciler(cfg, cl, scheme.Scheme) - - _, err := reconciler.Reconcile(ctx, istio) - if err == nil { - t.Errorf("Expected an error, but got nil") - } - - Must(t, cl.Get(ctx, istioKey, istio)) - - if istio.Status.State != v1alpha1.RemoteIstioReasonReconcileError { - t.Errorf("Expected status.state to be %q, but got %q", v1alpha1.RemoteIstioReasonReconcileError, istio.Status.State) - } - - reconciledCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReconciled) - if reconciledCond.Status != metav1.ConditionFalse { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionFalse, reconciledCond.Status) - } - - readyCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReady) - if readyCond.Status != metav1.ConditionUnknown { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionUnknown, readyCond.Status) - } - }) - - t.Run("returns error when reconcileActiveRevision fails", func(t *testing.T) { - istio := &v1alpha1.RemoteIstio{ - ObjectMeta: objectMeta, - Spec: v1alpha1.RemoteIstioSpec{ - Version: "my-version", - }, - } - - cl := newFakeClientBuilder(). - WithObjects(istio). - WithInterceptorFuncs(interceptor.Funcs{ - Create: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.CreateOption) error { - return fmt.Errorf("internal error") - }, - }). - Build() - reconciler := NewReconciler(cfg, cl, scheme.Scheme) - - _, err := reconciler.Reconcile(ctx, istio) - if err == nil { - t.Errorf("Expected an error, but got nil") - } - - Must(t, cl.Get(ctx, istioKey, istio)) - - if istio.Status.State != v1alpha1.RemoteIstioReasonReconcileError { - t.Errorf("Expected status.state to be %q, but got %q", v1alpha1.RemoteIstioReasonReconcileError, istio.Status.State) - } - - reconciledCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReconciled) - if reconciledCond.Status != metav1.ConditionFalse { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionFalse, reconciledCond.Status) - } - - readyCond := istio.Status.GetCondition(v1alpha1.RemoteIstioConditionReady) - if readyCond.Status != metav1.ConditionUnknown { - t.Errorf("Expected Reconciled condition status to be %q, but got %q", metav1.ConditionUnknown, readyCond.Status) - } - }) -} - -func TestValidate(t *testing.T) { - testCases := []struct { - name string - istio *v1alpha1.RemoteIstio - expectErr string - }{ - { - name: "success", - istio: &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: supportedversion.Default, - Namespace: "istio-system", - }, - }, - expectErr: "", - }, - { - name: "no version", - istio: &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - }, - Spec: v1alpha1.RemoteIstioSpec{ - Namespace: "istio-system", - }, - }, - expectErr: "spec.version not set", - }, - { - name: "no namespace", - istio: &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default", - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: supportedversion.Default, - }, - }, - expectErr: "spec.namespace not set", - }, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - g := NewWithT(t) - - err := validate(tc.istio) - if tc.expectErr == "" { - g.Expect(err).ToNot(HaveOccurred()) - } else { - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring(tc.expectErr)) - } - }) - } -} - -func TestDetermineStatus(t *testing.T) { - cfg := newReconcilerTestConfig(t) - - generation := int64(100) - - ownedByIstio := metav1.OwnerReference{ - APIVersion: v1alpha1.GroupVersion.String(), - Kind: v1alpha1.RemoteIstioKind, - Name: istioName, - UID: istioUID, - Controller: ptr.Of(true), - BlockOwnerDeletion: ptr.Of(true), - } - - ownedByAnotherIstio := metav1.OwnerReference{ - APIVersion: v1alpha1.GroupVersion.String(), - Kind: v1alpha1.RemoteIstioKind, - Name: "some-other-Istio", - UID: "some-other-uid", - Controller: ptr.Of(true), - BlockOwnerDeletion: ptr.Of(true), - } - - revision := func(name string, ownerRef metav1.OwnerReference, reconciled, ready, inUse bool) v1alpha1.IstioRevision { - return v1alpha1.IstioRevision{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - OwnerReferences: []metav1.OwnerReference{ownerRef}, - }, - Spec: v1alpha1.IstioRevisionSpec{Namespace: istioNamespace}, - Status: v1alpha1.IstioRevisionStatus{ - State: v1alpha1.IstioRevisionReasonHealthy, - Conditions: []v1alpha1.IstioRevisionCondition{ - {Type: v1alpha1.IstioRevisionConditionReconciled, Status: toConditionStatus(reconciled)}, - {Type: v1alpha1.IstioRevisionConditionReady, Status: toConditionStatus(ready)}, - {Type: v1alpha1.IstioRevisionConditionInUse, Status: toConditionStatus(inUse)}, - }, - }, - } - } - - testCases := []struct { - name string - reconciliationErr error - istio *v1alpha1.RemoteIstio - revisions []v1alpha1.IstioRevision - interceptorFuncs *interceptor.Funcs - wantErr bool - expectedStatus v1alpha1.RemoteIstioStatus - }{ - { - name: "reconciliation error", - reconciliationErr: fmt.Errorf("reconciliation error"), - wantErr: false, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonReconcileError, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonReconcileError, - Message: "reconciliation error", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionUnknown, - Reason: v1alpha1.RemoteIstioReasonReconcileError, - Message: "cannot determine readiness due to reconciliation error", - }, - }, - }, - }, - { - name: "mirrors status of active revision", - wantErr: false, - revisions: []v1alpha1.IstioRevision{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name, - OwnerReferences: []metav1.OwnerReference{ownedByIstio}, - }, - Spec: v1alpha1.IstioRevisionSpec{ - Namespace: istioNamespace, - }, - Status: v1alpha1.IstioRevisionStatus{ - State: v1alpha1.IstioRevisionReasonHealthy, - Conditions: []v1alpha1.IstioRevisionCondition{ - { - Type: v1alpha1.IstioRevisionConditionReconciled, - Status: metav1.ConditionTrue, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "reconciled message", - }, - { - Type: v1alpha1.IstioRevisionConditionReady, - Status: metav1.ConditionTrue, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "ready message", - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name + "-not-active", - OwnerReferences: []metav1.OwnerReference{ownedByIstio}, - }, - Spec: v1alpha1.IstioRevisionSpec{ - Namespace: istioNamespace, - }, - Status: v1alpha1.IstioRevisionStatus{ - State: v1alpha1.IstioRevisionReasonHealthy, - Conditions: []v1alpha1.IstioRevisionCondition{ - { - Type: v1alpha1.IstioRevisionConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "shouldn't mirror this revision", - }, - { - Type: v1alpha1.IstioRevisionConditionReady, - Status: metav1.ConditionFalse, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "shouldn't mirror this revision", - }, - }, - }, - }, - }, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonHealthy, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "reconciled message", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "ready message", - }, - }, - ActiveRevisionName: istioKey.Name, - Revisions: v1alpha1.RevisionSummary{ - Total: 2, - Ready: 1, - InUse: 0, - }, - }, - }, - { - name: "shows correct revision counts", - wantErr: false, - revisions: []v1alpha1.IstioRevision{ - // owned by the Istio under test; 3 todal, 2 ready, 1 in use - revision(istioKey.Name, ownedByIstio, true, true, true), - revision(istioKey.Name+"-old1", ownedByIstio, true, true, false), - revision(istioKey.Name+"-old2", ownedByIstio, true, false, false), - // not owned by the Istio being tested; shouldn't affect counts - revision("some-other-istio", ownedByAnotherIstio, true, true, true), - }, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonHealthy, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionTrue, - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionTrue, - }, - }, - ActiveRevisionName: istioKey.Name, - Revisions: v1alpha1.RevisionSummary{ - Total: 3, - Ready: 2, - InUse: 1, - }, - }, - }, - { - name: "active revision not found", - wantErr: false, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonRevisionNotFound, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - }, - ActiveRevisionName: istioKey.Name, - }, - }, - { - name: "get active revision error", - interceptorFuncs: &interceptor.Funcs{ - Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, obj client.Object, _ ...client.GetOption) error { - if _, ok := obj.(*v1alpha1.IstioRevision); ok { - return fmt.Errorf("simulated error") - } - return nil - }, - }, - wantErr: true, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonFailedToGetActiveRevision, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionUnknown, - Reason: v1alpha1.RemoteIstioReasonFailedToGetActiveRevision, - Message: "failed to get active IstioRevision: get failed: simulated error", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionUnknown, - Reason: v1alpha1.RemoteIstioReasonFailedToGetActiveRevision, - Message: "failed to get active IstioRevision: get failed: simulated error", - }, - }, - ActiveRevisionName: istioKey.Name, - Revisions: v1alpha1.RevisionSummary{}, - }, - }, - { - name: "list revisions error", - interceptorFuncs: &interceptor.Funcs{ - List: func(_ context.Context, _ client.WithWatch, list client.ObjectList, _ ...client.ListOption) error { - if _, ok := list.(*v1alpha1.IstioRevisionList); ok { - return fmt.Errorf("simulated error") - } - return nil - }, - }, - wantErr: true, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonRevisionNotFound, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - }, - ActiveRevisionName: istioKey.Name, - Revisions: v1alpha1.RevisionSummary{ - Total: -1, - Ready: -1, - InUse: -1, - }, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var interceptorFuncs interceptor.Funcs - if tc.interceptorFuncs != nil { - interceptorFuncs = *tc.interceptorFuncs - } - - istio := tc.istio - if istio == nil { - istio = &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name, - UID: istioUID, - Generation: 100, - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: "my-version", - Namespace: istioNamespace, - }, - } - } - - initObjs := []client.Object{istio} - for _, rev := range tc.revisions { - rev := rev - initObjs = append(initObjs, &rev) - } - - cl := newFakeClientBuilder(). - WithObjects(initObjs...). - WithInterceptorFuncs(interceptorFuncs). - Build() - reconciler := NewReconciler(cfg, cl, scheme.Scheme) - - status, err := reconciler.determineStatus(ctx, istio, tc.reconciliationErr) - if (err != nil) != tc.wantErr { - t.Errorf("determineStatus() error = %v, wantErr %v", err, tc.wantErr) - } - - if diff := cmp.Diff(tc.expectedStatus, clearTimestamps(status)); diff != "" { - t.Errorf("returned status wasn't as expected; diff (-expected, +actual):\n%v", diff) - } - }) - } -} - -func TestUpdateStatus(t *testing.T) { - cfg := newReconcilerTestConfig(t) - - generation := int64(100) - oneMinuteAgo := testtime.OneMinuteAgo() - - testCases := []struct { - name string - reconciliationErr error - istio *v1alpha1.RemoteIstio - revisions []v1alpha1.IstioRevision - interceptorFuncs *interceptor.Funcs - disallowWrites bool - wantErr bool - expectedStatus v1alpha1.RemoteIstioStatus - - skipInterceptors bool // used internally by test implementation when it wants to get around the interceptor - }{ - { - name: "updates status even when determineStatus returns error", - interceptorFuncs: &interceptor.Funcs{ - List: func(_ context.Context, _ client.WithWatch, list client.ObjectList, _ ...client.ListOption) error { - if _, ok := list.(*v1alpha1.IstioRevisionList); ok { - return fmt.Errorf("simulated error") - } - return nil - }, - }, - wantErr: true, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonRevisionNotFound, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionFalse, - Reason: v1alpha1.RemoteIstioReasonRevisionNotFound, - Message: "active IstioRevision not found", - }, - }, - ActiveRevisionName: istioKey.Name, - Revisions: v1alpha1.RevisionSummary{ - Total: -1, - Ready: -1, - InUse: -1, - }, - }, - }, - { - name: "skips update when status unchanged", - istio: &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name, - UID: istioUID, - Generation: 100, - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: "my-version", - Namespace: istioNamespace, - }, - Status: v1alpha1.RemoteIstioStatus{ - ObservedGeneration: 100, - State: v1alpha1.RemoteIstioReasonHealthy, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "reconciled message", - LastTransitionTime: *oneMinuteAgo, - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "ready message", - LastTransitionTime: *oneMinuteAgo, - }, - }, - ActiveRevisionName: istioKey.Name, - }, - }, - revisions: []v1alpha1.IstioRevision{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name, - }, - Spec: v1alpha1.IstioRevisionSpec{ - Namespace: istioNamespace, - }, - Status: v1alpha1.IstioRevisionStatus{ - State: v1alpha1.IstioRevisionReasonHealthy, - Conditions: []v1alpha1.IstioRevisionCondition{ - { - Type: v1alpha1.IstioRevisionConditionReconciled, - Status: metav1.ConditionTrue, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "reconciled message", - LastTransitionTime: *oneMinuteAgo, - }, - { - Type: v1alpha1.IstioRevisionConditionReady, - Status: metav1.ConditionTrue, - Reason: v1alpha1.IstioRevisionReasonHealthy, - Message: "ready message", - LastTransitionTime: *oneMinuteAgo, - }, - }, - }, - }, - }, - expectedStatus: v1alpha1.RemoteIstioStatus{ - State: v1alpha1.RemoteIstioReasonHealthy, - ObservedGeneration: generation, - Conditions: []v1alpha1.RemoteIstioCondition{ - { - Type: v1alpha1.RemoteIstioConditionReconciled, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "reconciled message", - }, - { - Type: v1alpha1.RemoteIstioConditionReady, - Status: metav1.ConditionTrue, - Reason: v1alpha1.RemoteIstioReasonHealthy, - Message: "ready message", - }, - }, - ActiveRevisionName: istioKey.Name, - }, - disallowWrites: true, - wantErr: false, - }, - { - name: "returns status update error", - interceptorFuncs: &interceptor.Funcs{ - SubResourcePatch: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error { - return fmt.Errorf("patch status error") - }, - }, - wantErr: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - var interceptorFuncs interceptor.Funcs - if tc.disallowWrites { - if tc.interceptorFuncs != nil { - panic("can't use disallowWrites and interceptorFuncs at the same time") - } - interceptorFuncs = noWrites(t) - } else if tc.interceptorFuncs != nil { - interceptorFuncs = *tc.interceptorFuncs - } - - istio := tc.istio - if istio == nil { - istio = &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: istioKey.Name, - UID: istioUID, - Generation: 100, - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: "my-version", - Namespace: istioNamespace, - }, - } - } - - initObjs := []client.Object{istio} - for _, rev := range tc.revisions { - rev := rev - initObjs = append(initObjs, &rev) - } - - cl := newFakeClientBuilder(). - WithObjects(initObjs...). - WithInterceptorFuncs(interceptorFuncs). - Build() - reconciler := NewReconciler(cfg, cl, scheme.Scheme) - - err := reconciler.updateStatus(ctx, istio, tc.reconciliationErr) - if (err != nil) != tc.wantErr { - t.Errorf("updateStatus() error = %v, wantErr %v", err, tc.wantErr) - } - - Must(t, cl.Get(ctx, istioKey, istio)) - if diff := cmp.Diff(tc.expectedStatus, clearTimestamps(istio.Status)); diff != "" { - t.Errorf("returned status wasn't as expected; diff (-expected, +actual):\n%v", diff) - } - }) - } -} - -func clearTimestamps(status v1alpha1.RemoteIstioStatus) v1alpha1.RemoteIstioStatus { - for i := range status.Conditions { - status.Conditions[i].LastTransitionTime = metav1.Time{} - } - return status -} - -func toConditionStatus(b bool) metav1.ConditionStatus { - if b { - return metav1.ConditionTrue - } - return metav1.ConditionFalse -} - -func TestGetActiveRevisionName(t *testing.T) { - tests := []struct { - name string - version string - updateStrategyType *v1alpha1.UpdateStrategyType - expectedRevisionName string - }{ - { - name: "No update strategy specified", - version: "1.0.0", - updateStrategyType: nil, - expectedRevisionName: "test-istio", - }, - { - name: "InPlace", - version: "1.0.0", - updateStrategyType: ptr.Of(v1alpha1.UpdateStrategyTypeInPlace), - expectedRevisionName: "test-istio", - }, - { - name: "RevisionBased v1.0.0", - version: "1.0.0", - updateStrategyType: ptr.Of(v1alpha1.UpdateStrategyTypeRevisionBased), - expectedRevisionName: "test-istio-1-0-0", - }, - { - name: "RevisionBased v2.0.0", - version: "2.0.0", - updateStrategyType: ptr.Of(v1alpha1.UpdateStrategyTypeRevisionBased), - expectedRevisionName: "test-istio-2-0-0", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - istio := &v1alpha1.RemoteIstio{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-istio", - }, - Spec: v1alpha1.RemoteIstioSpec{ - Version: tt.version, - }, - } - if tt.updateStrategyType != nil { - istio.Spec.UpdateStrategy = &v1alpha1.IstioUpdateStrategy{ - Type: *tt.updateStrategyType, - } - } - actual := getActiveRevisionName(istio) - if actual != tt.expectedRevisionName { - t.Errorf("getActiveRevisionName() = %v, want %v", actual, tt.expectedRevisionName) - } - }) - } -} - -func newFakeClientBuilder() *fake.ClientBuilder { - return fake.NewClientBuilder(). - WithScheme(scheme.Scheme). - WithStatusSubresource(&v1alpha1.RemoteIstio{}) -} - -func TestGetPruningGracePeriod(t *testing.T) { - tests := []struct { - name string - updateStrategy *v1alpha1.IstioUpdateStrategy - expected time.Duration - }{ - { - name: "Nil update strategy", - updateStrategy: nil, - expected: v1alpha1.DefaultRevisionDeletionGracePeriodSeconds * time.Second, - }, - { - name: "Nil grace period", - updateStrategy: &v1alpha1.IstioUpdateStrategy{}, - expected: v1alpha1.DefaultRevisionDeletionGracePeriodSeconds * time.Second, - }, - { - name: "Grace period less than minimum", - updateStrategy: &v1alpha1.IstioUpdateStrategy{ - InactiveRevisionDeletionGracePeriodSeconds: ptr.Of(int64(v1alpha1.MinRevisionDeletionGracePeriodSeconds - 10)), - }, - expected: v1alpha1.MinRevisionDeletionGracePeriodSeconds * time.Second, - }, - { - name: "Grace period more than minimum", - updateStrategy: &v1alpha1.IstioUpdateStrategy{ - InactiveRevisionDeletionGracePeriodSeconds: ptr.Of(int64(v1alpha1.MinRevisionDeletionGracePeriodSeconds + 10)), - }, - expected: (v1alpha1.MinRevisionDeletionGracePeriodSeconds + 10) * time.Second, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - istio := &v1alpha1.RemoteIstio{ - Spec: v1alpha1.RemoteIstioSpec{ - UpdateStrategy: tt.updateStrategy, - }, - } - got := getPruningGracePeriod(istio) - if got != tt.expected { - t.Errorf("getPruningGracePeriod() = %v, want %v", got, tt.expected) - } - }) - } -} - -func Must(t *testing.T, err error) { - t.Helper() - if err != nil { - t.Fatal(err) - } -} - -func noWrites(t *testing.T) interceptor.Funcs { - return interceptor.Funcs{ - Create: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.CreateOption) error { - t.Fatal("unexpected call to Create in", string(debug.Stack())) - return nil - }, - Update: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.UpdateOption) error { - t.Fatal("unexpected call to Update in", string(debug.Stack())) - return nil - }, - Delete: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteOption) error { - t.Fatal("unexpected call to Delete in", string(debug.Stack())) - return nil - }, - Patch: func(_ context.Context, _ client.WithWatch, _ client.Object, _ client.Patch, _ ...client.PatchOption) error { - t.Fatal("unexpected call to Patch in", string(debug.Stack())) - return nil - }, - DeleteAllOf: func(_ context.Context, _ client.WithWatch, _ client.Object, _ ...client.DeleteAllOfOption) error { - t.Fatal("unexpected call to DeleteAllOf in", string(debug.Stack())) - return nil - }, - SubResourceCreate: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ client.Object, _ ...client.SubResourceCreateOption) error { - t.Fatal("unexpected call to SubResourceCreate in", string(debug.Stack())) - return nil - }, - SubResourceUpdate: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ ...client.SubResourceUpdateOption) error { - t.Fatal("unexpected call to SubResourceUpdate in", string(debug.Stack())) - return nil - }, - SubResourcePatch: func(_ context.Context, _ client.Client, _ string, obj client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error { - t.Fatalf("unexpected call to SubResourcePatch with the object %+v: %v", obj, string(debug.Stack())) - return nil - }, - } -} - -func newReconcilerTestConfig(t *testing.T) config.ReconcilerConfig { - return config.ReconcilerConfig{ - ResourceDirectory: t.TempDir(), - Platform: config.PlatformKubernetes, - DefaultProfile: "", - } -} diff --git a/controllers/webhook/webhook_controller.go b/controllers/webhook/webhook_controller.go index fe6ef714b..142a60b44 100644 --- a/controllers/webhook/webhook_controller.go +++ b/controllers/webhook/webhook_controller.go @@ -30,6 +30,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/constants" "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" + "github.com/istio-ecosystem/sail-operator/pkg/revision" admissionv1 "k8s.io/api/admissionregistration/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -74,15 +75,21 @@ func NewReconciler(client client.Client, scheme *runtime.Scheme) *Reconciler { // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.14.1/pkg/reconcile func (r *Reconciler) Reconcile(ctx context.Context, webhook *admissionv1.MutatingWebhookConfiguration) (ctrl.Result, error) { + log := logf.FromContext(ctx) + isReady, err := r.probe(ctx, webhook) + reason := "" if err != nil { - isReady = false + log.V(3).Error(err, "Probe failed") + reason = err.Error() } if webhook.Annotations == nil { webhook.Annotations = make(map[string]string) } webhook.Annotations[constants.WebhookReadinessProbeStatusAnnotationKey] = strconv.FormatBool(isReady) + webhook.Annotations[constants.WebhookReadinessProbeStatusReasonAnnotationKey] = reason + err = r.Client.Update(ctx, webhook) if err != nil { return ctrl.Result{}, err @@ -91,7 +98,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, webhook *admissionv1.Mutatin } func doProbe(ctx context.Context, webhook *admissionv1.MutatingWebhookConfiguration) (bool, error) { - log := logf.FromContext(ctx).V(3) + log := logf.FromContext(ctx) if len(webhook.Webhooks) == 0 { return false, errors.New("mutatingwebhookconfiguration contains no webhooks") } @@ -129,13 +136,13 @@ func doProbe(ctx context.Context, webhook *admissionv1.MutatingWebhookConfigurat return false, err } - log.Info("Executing readiness probe on remote control plane", "url", req.URL.String()) + log.V(3).Info("Executing readiness probe on remote control plane", "url", req.URL.String()) resp, err := httpClient.Do(req) if err != nil { - log.Info("Probe failed", "error", err) + log.V(3).Info("Probe failed", "error", err) return false, err } - log.Info("Probe response", "response", resp.StatusCode) + log.V(3).Info("Probe response", "response", resp.StatusCode) return resp.StatusCode == http.StatusOK, nil } @@ -178,36 +185,37 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { // we use the Watches function instead of For(), so that we can wrap the handler so that events that cause the object to be enqueued are logged // +lint-watches:ignore: IstioRevision (not found in charts, but this is the main resource watched by this controller) - Watches(&admissionv1.MutatingWebhookConfiguration{}, objectHandler, builder.WithPredicates(ownedByRemoteIstioPredicate(mgr.GetClient()))). + Watches(&admissionv1.MutatingWebhookConfiguration{}, objectHandler, builder.WithPredicates(ownedByRemoteIstioRevisionPredicate(mgr.GetClient()))). Named("mutatingwebhookconfiguration"). Complete(reconciler.NewStandardReconciler[*admissionv1.MutatingWebhookConfiguration](r.Client, r.Reconcile)) } -func ownedByRemoteIstioPredicate(cl client.Client) predicate.Predicate { +func ownedByRemoteIstioRevisionPredicate(cl client.Client) predicate.Predicate { return predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { - return isOwnedByRemoteIstio(cl, e.Object) + return IsOwnedByRevisionWithRemoteControlPlane(cl, e.Object) }, UpdateFunc: func(e event.UpdateEvent) bool { - return isOwnedByRemoteIstio(cl, e.ObjectNew) + return IsOwnedByRevisionWithRemoteControlPlane(cl, e.ObjectNew) }, DeleteFunc: func(e event.DeleteEvent) bool { - return isOwnedByRemoteIstio(cl, e.Object) + return IsOwnedByRevisionWithRemoteControlPlane(cl, e.Object) }, GenericFunc: func(e event.GenericEvent) bool { - return isOwnedByRemoteIstio(cl, e.Object) + return IsOwnedByRevisionWithRemoteControlPlane(cl, e.Object) }, } } -func isOwnedByRemoteIstio(cl client.Client, obj client.Object) bool { +func IsOwnedByRevisionWithRemoteControlPlane(cl client.Client, obj client.Object) bool { for _, ownerRef := range obj.GetOwnerReferences() { if ownerRef.APIVersion == v1alpha1.GroupVersion.String() && ownerRef.Kind == v1alpha1.IstioRevisionKind { rev := &v1alpha1.IstioRevision{} err := cl.Get(context.Background(), client.ObjectKey{Name: ownerRef.Name}, rev) if err != nil { - // TODO log error - } else if rev.Spec.Type == v1alpha1.IstioRevisionTypeRemote { + return false + } + if revision.IsUsingRemoteControlPlane(rev) { return true } } diff --git a/controllers/webhook/webhook_controller_test.go b/controllers/webhook/webhook_controller_test.go index 2e2b98813..054c316cf 100644 --- a/controllers/webhook/webhook_controller_test.go +++ b/controllers/webhook/webhook_controller_test.go @@ -380,7 +380,7 @@ func TestDoProbe(t *testing.T) { } } -func TestIsOwnedByRemoteIstio(t *testing.T) { +func TestIsOwnedByRevisionWithRemoteControlPlane(t *testing.T) { tests := []struct { name string ownerRefs []metav1.OwnerReference @@ -431,7 +431,7 @@ func TestIsOwnedByRemoteIstio(t *testing.T) { expected: false, }, { - name: "IstioRevision type not remote", + name: "IstioRevision not using remote profile", ownerRefs: []metav1.OwnerReference{ { APIVersion: v1alpha1.GroupVersion.String(), @@ -444,15 +444,13 @@ func TestIsOwnedByRemoteIstio(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: "revision1", }, - Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, - }, + Spec: v1alpha1.IstioRevisionSpec{}, }, }, expected: false, }, { - name: "IstioRevision type is remote", + name: "IstioRevision uses remote profile", ownerRefs: []metav1.OwnerReference{ { APIVersion: v1alpha1.GroupVersion.String(), @@ -466,7 +464,9 @@ func TestIsOwnedByRemoteIstio(t *testing.T) { Name: "revision1", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeRemote, + Values: &v1alpha1.Values{ + Profile: ptr.Of("remote"), + }, }, }, }, @@ -489,7 +489,7 @@ func TestIsOwnedByRemoteIstio(t *testing.T) { }, } - result := isOwnedByRemoteIstio(cl, obj) + result := IsOwnedByRevisionWithRemoteControlPlane(cl, obj) g.Expect(result).To(Equal(tt.expected)) }) } diff --git a/docs/README.md b/docs/README.md index 828976cf5..136954d3c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -7,7 +7,6 @@ - [Istio resource](#istio-resource) - [IstioRevision resource](#istiorevision-resource) - [IstioCNI resource](#istiocni-resource) - - [RemoteIstio resource](#remoteistio-resource) - [API Reference documentation](#api-reference-documentation) - [Getting Started](#getting-started) - [Installation on OpenShift](#installation-on-openshift) @@ -64,7 +63,7 @@ kind: Istio metadata: name: default spec: - version: v1.22.3 + version: v1.23.2 namespace: istio-system updateStrategy: type: InPlace @@ -98,7 +97,7 @@ kind: IstioCNI metadata: name: default spec: - version: v1.22.3 + version: v1.23.2 namespace: istio-cni values: cni: @@ -107,32 +106,6 @@ spec: - kube-system ``` -### RemoteIstio resource -The `RemoteIstio` resource is used to connect the local cluster to an external Istio control plane. -When you create a `RemoteIstio` resource, the operator deploys the `istiod-remote` Helm chart. -Instead of deploying the entire Istio control plane, this chart deploys only the sidecar injector webhook, allowing you to inject the Istio proxy into your workloads and have this proxy managed by the Istio control plane running outside the cluster (typically in another Kubernetes cluster). - -The `RemoteIstio` resource is very similar to the `Istio` resource, with the most notable difference being the `istiodRemote` field in the `values` section, which allows you to configure the address of the remote Istio control plane: - -```yaml -apiVersion: sailoperator.io/v1alpha1 -kind: RemoteIstio -metadata: - name: default -spec: - version: v1.22.3 - namespace: istio-system - updateStrategy: - type: InPlace - values: - istiodRemote: - injectionPath: /inject/cluster/cluster2/net/network1 - global: - remotePilotAddress: 1.2.3.4 -``` - -For more information on how to use the `RemoteIstio` resource, refer to the [multi-cluster](#multi-cluster) section. - ## API Reference documentation The Sail Operator API reference documentation can be found [here](https://github.com/istio-ecosystem/sail-operator/tree/main/docs/api-reference/sailoperator.io.md). @@ -231,7 +204,7 @@ spec: values: pilot: traceSampling: 0.1 - version: v1.23.0 + version: v1.23.2 ``` Note that the only field that was added is the `spec.version` field. There are a few situations however where the APIs are different and require different approaches to achieve the same outcome. @@ -244,10 +217,6 @@ Sail Operator's Istio resource does not have a `spec.components` field. Instead, The CNI plugin's lifecycle is managed separately from the control plane. You will have to create a [IstioCNI resource](#istiocni-resource) to use CNI. -### istiod-remote - -The functionality of the istiod-remote chart is exposed through the [RemoteIstio resource](#remoteistio-resource). - ## Gateways [Gateways in Istio](https://istio.io/latest/docs/concepts/traffic-management/#gateways) are used to manage inbound and outbound traffic for the mesh. The Sail Operator does not deploy or manage Gateways. You can deploy a gateway either through [gateway-api](https://istio.io/latest/docs/tasks/traffic-management/ingress/gateway-api/) or through [gateway injection](https://istio.io/latest/docs/setup/additional-setup/gateway/#deploying-a-gateway). As you are following the gateway installation instructions, skip the step to install Istio since this is handled by the Sail Operator. @@ -288,7 +257,7 @@ Steps: namespace: istio-system updateStrategy: type: InPlace - version: v1.21.0 + version: v1.22.5 EOF ``` @@ -296,9 +265,10 @@ Steps: ```console $ kubectl get istio -n istio-system - NAME READY STATUS IN USE VERSION AGE - default True Healthy True v1.21.0 2m + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 0 default Healthy v1.22.5 23s ``` + Note: `IN USE` field shows as 0, as `Istio` is yet installed. 4. Create namespace `bookinfo` and deploy bookinfo application. @@ -309,27 +279,36 @@ Steps: ``` Note: if the `Istio` resource name is other than `default`, you need to set the `istio.io/rev` label to the name of the `Istio` resource instead of adding the `istio-injection=enabled` label. -5. Perform the update of the control plane by changing the version in the Istio resource. +5. Review the `Istio` resource after application deployment. + + ```console + $ kubectl get istio -n istio-system + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 1 default Healthy v1.22.5 115s + ``` + Note: `IN USE` field shows as 1, after application being deployed. + +6. Perform the update of the control plane by changing the version in the Istio resource. ```bash - kubectl patch istio default -n istio-system --type='merge' -p '{"spec":{"version":"v1.21.2"}}' + kubectl patch istio default -n istio-system --type='merge' -p '{"spec":{"version":"v1.23.2"}}' ``` -6. Confirm the `Istio` resource version was updated. +7. Confirm the `Istio` resource version was updated. ```console $ kubectl get istio -n istio-system - NAME REVISIONS READY IN USE ACTIVE REVISION VERSION AGE - default 1 1 1 Healthy v1.21.2 12m + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 1 default Healthy v1.23.2 4m50s ``` -7. Delete `bookinfo` pods to trigger sidecar injection with the new version. +8. Delete `bookinfo` pods to trigger sidecar injection with the new version. ```bash kubectl rollout restart deployment -n bookinfo ``` -8. Confirm that the new version is used in the sidecar. +9. Confirm that the new version is used in the sidecar. ```bash istioctl proxy-status @@ -366,7 +345,7 @@ Steps: updateStrategy: type: RevisionBased inactiveRevisionDeletionGracePeriodSeconds: 30 - version: v1.21.0 + version: v1.22.5 EOF ``` @@ -374,16 +353,17 @@ Steps: ```console $ kubectl get istio -n istio-system - NAME READY STATUS IN USE VERSION AGE - default True Healthy True v1.21.0 2m + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 0 default-v1-22-5 Healthy v1.22.5 52s ``` + Note: `IN USE` field shows as 0, as `Istio` is yet installed. 4. Get the `IstioRevision` name. ```console $ kubectl get istiorevision -n istio-system - NAME READY STATUS IN USE VERSION AGE - default-v1-21-0 True Healthy False v1.21.0 114s + NAME TYPE READY STATUS IN USE VERSION AGE + default-v1-22-5 Local True Healthy False v1.22.5 3m4s ``` Note: `IstioRevision` name is in the format `-`. @@ -391,7 +371,7 @@ Steps: ```bash kubectl create namespace bookinfo - kubectl label namespace bookinfo istio.io/rev=default-v1-21-0 + kubectl label namespace bookinfo istio.io/rev=default-v1-22-5 ``` 6. Deploy bookinfo application. @@ -400,78 +380,91 @@ Steps: kubectl apply -n bookinfo -f https://raw.githubusercontent.com/istio/istio/release-1.22/samples/bookinfo/platform/kube/bookinfo.yaml ``` -7. Confirm that the proxy version matches the control plane version. +7. Review the `Istio` resource after application deployment. + + ```console + $ kubectl get istio -n istio-system + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 1 default-v1-22-5 Healthy v1.22.5 5m13s + ``` + Note: `IN USE` field shows as 1, after application being deployed. + +8. Confirm that the proxy version matches the control plane version. ```bash istioctl proxy-status ``` The column `VERSION` should match the control plane version. -8. Update the control plane to a new version. +9. Update the control plane to a new version. ```bash - kubectl patch istio default -n istio-system --type='merge' -p '{"spec":{"version":"v1.21.2"}}' + kubectl patch istio default -n istio-system --type='merge' -p '{"spec":{"version":"v1.23.2"}}' ``` -9. Verify the `Istio` and `IstioRevision` resources. There will be a new revision created with the new version. +10. Verify the `Istio` and `IstioRevision` resources. There will be a new revision created with the new version. ```console $ kubectl get istio -n istio-system - NAME REVISIONS READY IN USE ACTIVE REVISION VERSION AGE - default 2 2 1 Healthy v1.21.2 23m + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 2 2 1 default-v1-23-2 Healthy v1.23.2 9m23s $ kubectl get istiorevision -n istio-system - NAME READY STATUS IN USE VERSION AGE - default-v1-21-0 True Healthy True v1.21.0 27m - default-v1-21-2 True Healthy False v1.21.2 4m45s + NAME TYPE READY STATUS IN USE VERSION AGE + default-v1-22-5 Local True Healthy True v1.22.5 10m + default-v1-23-2 Local True Healthy False v1.23.2 66s ``` -10. Confirm there are two control plane pods running, one for each revision. +11. Confirm there are two control plane pods running, one for each revision. ```console $ kubectl get pods -n istio-system NAME READY STATUS RESTARTS AGE - istiod-default-v1-21-0-69d6df7f9c-grm24 1/1 Running 0 28m - istiod-default-v1-21-2-7c4f4674c5-4g7n7 1/1 Running 0 6m9s + istiod-default-v1-22-5-c98fd9675-r7bfw 1/1 Running 0 10m + istiod-default-v1-23-2-7495cdc7bf-v8t4g 1/1 Running 0 113s ``` -11. Confirm the proxy sidecar version remains the same: +12. Confirm the proxy sidecar version remains the same: ```bash istioctl proxy-status ``` The column `VERSION` should still match the old control plane version. -12. Change the label of the `bookinfo` namespace to use the new revision. +13. Change the label of the `bookinfo` namespace to use the new revision. ```bash - kubectl label namespace bookinfo istio.io/rev=default-v1-21-2 --overwrite + kubectl label namespace bookinfo istio.io/rev=default-v1-23-2 --overwrite ``` The existing workload sidecars will continue to run and will remain connected to the old control plane instance. They will not be replaced with a new version until the pods are deleted and recreated. -13. Delete all the pods in the `bookinfo` namespace. +14. Delete all the pods in the `bookinfo` namespace. ```bash kubectl rollout restart deployment -n bookinfo ``` -14. Confirm the new version is used in the sidecars. +15. Confirm the new version is used in the sidecars. ```bash istioctl proxy-status ``` The column `VERSION` should match the updated control plane version. -15. Confirm the old control plane and revision deletion. +16. Confirm the old control plane and revision deletion. ```console $ kubectl get pods -n istio-system NAME READY STATUS RESTARTS AGE - istiod-default-v1-21-2-7c4f4674c5-4g7n7 1/1 Running 0 94m + istiod-default-v1-23-2-7495cdc7bf-v8t4g 1/1 Running 0 4m40s + + $ kubectl get istio -n istio-system + NAME REVISIONS READY IN USE ACTIVE REVISION STATUS VERSION AGE + default 1 1 1 default-v1-23-2 Healthy v1.23.2 5m $ kubectl get istiorevision -n istio-system - NAME READY STATUS IN USE VERSION AGE - default-v1-21-2 True Healthy True v1.21.2 94m + NAME TYPE READY STATUS IN USE VERSION AGE + default-v1-23-2 Local True Healthy True v1.23.2 5m31s ``` The old `IstioRevision` resource and the old control plane will be deleted when the grace period specified in the `Istio` resource field `spec.updateStrategy.inactiveRevisionDeletionGracePeriodSeconds` expires. @@ -853,17 +846,18 @@ In this setup there is a Primary cluster (`cluster1`) and a Remote cluster (`clu kubectl --context "${CTX_CLUSTER1}" apply -n istio-system -f https://raw.githubusercontent.com/istio-ecosystem/sail-operator/main/docs/multicluster/expose-services.yaml ``` -5. Create `RemoteIstio` resource on `cluster2`. +5. Create an `Istio` on `cluster2` with the `remote` profile. ```sh kubectl apply --context "${CTX_CLUSTER2}" -f - < ingress-gateway.yaml + $ oc apply -f ingress-gateway.yaml ``` 2. Configure the `bookinfo` application with the new gateway: diff --git a/enhancements/SEP2-revision-tags.md b/enhancements/SEP2-revision-tags.md new file mode 100644 index 000000000..3ef946596 --- /dev/null +++ b/enhancements/SEP2-revision-tags.md @@ -0,0 +1,136 @@ +|Status | Authors | Created | +|---------------------------------------------------|--------------|------------| +|Implementation | @dgn | 2024-07-17 | + +# Revision Tag Support + +## Overview +Upstream Istio supports the use of [stable revision tags](https://istio.io/latest/blog/2021/revision-tags/) for multi-revision deployments and canary upgrades of Istio control planes. These tags serve as aliases for revisions and allow users to use stable revision names (e.g. `prod` or `default`), so they don't have to change their namespace and pod labels (in this case `istio.io/rev=prod` or `istio-injection=enabled`) when switching to a new version. Instead, by tagging a new revision with the correct tag and restarting their workloads, they can perform an Istio update without having to change their labels. This is especially useful in situations where the team managing the Istio control plane is separate from the teams managing the workloads. + +Revision tags can have any name, there is only one special case: revisions tagged `default` are treated as if they had an empty revision name, thereby allowing the use of the standard namespace injection label `istio-injection=enabled`. + +Each revision tag only ever points to exactly one Istio revision. Upstream, revision tags are created manually using `istioctl` and- as they only affect injection- are represented in the cluster by a MutatingWebhookConfiguration. + +Here's an example how to create a `default` revision tag that points to the `1-21-1` revision using `istioctl`: + +```bash +istioctl tag set default --revision 1-21-1 +``` + +## Goals +* Provide revision tag support in Sail Operator APIs so users don't have to use istioctl for basic revision tag operations + +## Non-goals +* Compatibility with manual revision tag creation using istioctl. There might be a way to disable the operator functionality to avoid conflicts when creating revision tags manually, but that's it - you either do it yourself or let the operator do it + +## Design + +### User Stories +1. As a user of Sail Operator's RevisionBased update strategy, I want to be able to use the `istio-injection=enabled` label on my namespaces. +1. As a platform engineer, I want my application teams to be able to use a fixed label for proxy injection without having to know which version of Istio is running in the cluster, so that I can perform upgrades in the background without the application teams having to make changes to use the new version. + +### API Changes +We will add a new CRD called `IstioRevisionTag` that consistly most of a `spec.targetRef` field and a `status` subresource. + +#### IstioRevisionTag resource +Here's an example YAML for the new resource: +```yaml +apiGroup: sailoperator.io/v1alpha1 +kind: IstioRevisionTag +metadata: + name: default +spec: + targetRef: + kind: Istio # can also be IstioRevision + name: default +status: + observedGeneration: 1 + conditions: [] + state: Healthy + istiodNamespace: istio-system + istioRevision: default-v1.24.0 +``` + +In the `spec.targetRef` field, users can specify the `IstioRevision` or `Istio` resource that the `IstioRevisionTag` references. In case of referencing a `IstioRevision` resource, the created tag will point to the exact Istio control plane revision that is represented by the `IstioRevision` resource and any update of the tag will have to be made manually by changing the `spec.targetRef` to point to another `IstioRevision`. As long as a `IstioRevisionTag` exists that references a `IstioRevision`, that `IstioRevision` will be considered "in-use" by the Sail Operator, preventing its automatic deletion during a control plane update (see details below under [InUse detection](#inuse-detection)) + +If the `spec.targetRef` is used to reference an `Istio` resource, the Sail Operator will automatically update the revision tag when a new `IstioRevision` is created as part of a version update of the `Istio` resource. In this case, the `IstioRevisionTag` resource behaves like a floating tag, always referencing the active `IstioRevision` of an `Istio` resource. When it comes to InUse detection, the existence of a floating tag will also cause the active `IstioRevision` of the `Istio` resource to be considered InUse. However, it will not prevent automatic deletion, because the reference is updated immediately when the active revision changes during an update. + +#### IstioRevisionTag Status + +The `status.state` field gives a quick hint as to whether a tag has been reconciled and is InUse ("Healthy") or if there are any problems with it. + +The `status.istiodNamespace` and `status.istioRevision` fields are used by the Sail Operator controllers to store information about the Istio control plane that is referenced by this `IstioRevisionTag`. This is especially useful when it is referencing an `Istio` resource, to see which underlying `IstioRevision` is considered referenced by the operator. + +Possible conditions for `status.conditions` are: + +##### Reconciled +`true` when the tag's helm chart has been installed successfully. Possible error reasons are: +__RefNotFound__: the resource referenced by the `spec.targetRef` field was not found +__NameAlreadyExists__: there already is an `IstioRevision` with this name +__ReconcileError__: there was an error installing the chart + +##### InUse +`true` when the `IstioRevisionTag` is referenced by a namespace or pod. Possible reasons when `false` are: +__NotReferencedByAnything__: no namespace or pod is referencing the tag +__UsageCheckFailed__: there was a problem during InUse detection + +#### InUse detection + +An `IstioRevisionTag` is considered InUse when + +* there's a pod or namespace that explicitly references the `IstioRevisionTag` in an `istio.io/rev` label +* the name of the `IstioRevisionTag` name is 'default' and there's a pod or namespace with the `istio-injection: enabled` label + +Note that a pod's `istio.io/rev` annotation will not be considered as that will always have the name of the referenced `IstioRevision` rather than the name of the `IstioRevisionTag`! The labels however are added by users and reflect usage intent, ie the user will use the name of the `IstioRevisionTag`. + +Even if the referenced `IstioRevision` of an `IstioRevisionTag` is considered InUse, that does not suffice to make the `IstioRevisionTag` considered InUse by the operator! It is considered an unused alias for an InUse `IstioRevision`. + +Additionally, the introduction of `IstioRevisionTag` also adds another condition to the InUse detection of `IstioRevision`: being referenced by an `IstioRevisionTag` will now always lead to an `IstioRevision` being considered InUse! For this, it does not matter if the `IstioRevisionTag` is itself considered InUse. + +For completeness' sake, here's an overview of the conditions for an `IstioRevision` to be considered InUse (new condition in bold): + +* there's a pod that explicitly references the `IstioRevision` in an `istio.io/rev` annotation or label +* there's a namespace that explicitly references the `IstioRevision` in an `istio.io/rev` label +* the name of the `IstioRevision` is 'default' and there's a pod or namespace with the `istio-injection: enabled` label +* __there's an `IstioRevisionTag` referencing this `IstioRevision`__ + +#### Changes to existing APIs +We will need to remove the `values.revisionTags` field from our API, which is how the upstream charts expose this feature. + +### Architecture +We will need to update the mechanism to detect revisions that are being used. Today, we only look at the `istio.io/rev` annotation's value to check which revisions are in use. But when revision tags are used, those values will point to the referenced revision instead of the tags, so we have to improve our detection mechanism. The most correct way is probably to look at the revision label on the pods and namespace that is set to configure injection. + +Revision tags and revision names can be used interchangably, so they must never overlap. Therefore, we'll need a `status` on the `IstioRevisionTag` resource that can show the user an error if the name they chose is already taken by a `IstioRevision`. Another case that needs to be covered is when an `IstioRevision` is being reconciled and it would be assigned the same name as an existing tag. In this case, reconciliation of the `IstioRevision` should fail, with an error message that tells the users why this happened, ie "the name is already used by an `IstioRevisionTag`". + +## Alternatives Considered + +### Reuse `IstioRevision`'s type field for revision tags +We could add a `type` of `Tag` to the `IstioRevision` CRD and use that to manage tags. It would have the benefit that the user could list all revisions and tags using `kubectl get istiorevisions` and name-uniqueness would be handled by Kubernetes. The disadvantage though is that revision tags share no other fields with `IstioRevision` and it would be quite confusing for users that 99% of the CRD's fields are not to be used in this case, whereas there would be one new field that is only to be used for `IstioRevisions` with type=Tag. + +Note that the `type` field has since been removed with the removal of `RemoteIstio`. + +### managing tags in Istio resource +In a previous iteration of this SEP, the tags that point to an Istio control plane would have been managed in the `Istio` revision itself, in a `spec.updateStrategy.revisionTags` field. That would have meant that they are always referencing a `Istio` resource while at the same time being copied to every underlying `IstioRevision` resource. + +### values.revisionTags +Istio has a `values.revisionTags` field that we even currently expose in our APIs. The problem is that we copy all values from the `Istio` resource to every `IstioRevision` and that means we would be facing duplicate revision tags when we create additional revisions in the Sail Operator - so, some logic would be required to work around this problem. As it is a similar amount of effort, I prefer the explicit version of adding the field to the `Istio` CRD. + +### Automatic creation of default revision tag +Previously, we had this paragraph in the Architecture section: +> When the very first `IstioRevision` is created in a cluster from a `RevisionBased` Istio resource, the Sail Operator will create a `IstioRevisionTag` with the name `default`, referencing that `IstioRevision`. This way, the standard namespace injection label `istio-injection=enabled` will work out of the box for RevisionBased deployments (see second paragraph of the [Overview](#overview)). + +We have since dropped this from the design as we faced some problems with this approach. Most importantly, it is very hard to detect whether the user is not creating a default IstioRevisionTag or we have simply not seen its creation event yet. We have discussed multiple possible solutions to this, among them usage of a 'virtual' revision tag that is not created on the API server but only exists in-memory, leading to the creation of the respective Kubernetes resources only. This would avoid a race between the operator and the user trying to create tags with the same name simultaneously. + +Due to the complexity of the task we have moved it into a separate ticket: [#439 Create default revision tag automatically](https://github.com/istio-ecosystem/sail-operator/issues/439). + +## Implementation Plan +v1alpha1 +- [ ] Initial implementation & tests (https://github.com/istio-ecosystem/sail-operator/pull/413) +- [ ] Documentation + +v1beta1 +- [ ] [#439 Create default revision tag automatically](https://github.com/istio-ecosystem/sail-operator/issues/439) +- [ ] [#471 Support revision tags in multicluster topologies](https://github.com/istio-ecosystem/sail-operator/issues/471) + +## Test Plan +Functionality will be tested in integration tests. diff --git a/hack/download-charts.sh b/hack/download-charts.sh index 799cc719c..ad01c97bf 100755 --- a/hack/download-charts.sh +++ b/hack/download-charts.sh @@ -96,9 +96,6 @@ function patchIstioCharts() { } function convertIstioProfiles() { - # delete the remote profile, because it isn't needed (we have the RemoteIstio resource instead) - [ -f "${PROFILES_DIR}"/remote.yaml ] && rm "${PROFILES_DIR}"/remote.yaml - # delete the minimal profile, because it ends up being empty after the conversion [ -f "${PROFILES_DIR}"/minimal.yaml ] && rm "${PROFILES_DIR}"/minimal.yaml diff --git a/hack/update-istio.sh b/hack/update-istio.sh index b85508e7e..43d7efb43 100755 --- a/hack/update-istio.sh +++ b/hack/update-istio.sh @@ -26,6 +26,12 @@ VERSIONS_YAML_FILE=${VERSIONS_YAML_FILE:-"versions.yaml"} # The new entry will be placed immediately before the old one function add_stable_version() { echo "Adding new stable version: ${1}" + # we want to add the istiod-remote chart only for 1.23 + istiod_remote_line="" + if [[ ${1} == 1.23.* ]] + then + istiod_remote_line="\"https://istio-release.storage.googleapis.com/charts/istiod-remote-${1}.tgz\"," + fi template=$(cat <<-END { "name": "v${1}", @@ -35,7 +41,7 @@ function add_stable_version() { "charts": [ "https://istio-release.storage.googleapis.com/charts/base-${1}.tgz", "https://istio-release.storage.googleapis.com/charts/istiod-${1}.tgz", - "https://istio-release.storage.googleapis.com/charts/istiod-remote-${1}.tgz", + ${istiod_remote_line} "https://istio-release.storage.googleapis.com/charts/gateway-${1}.tgz", "https://istio-release.storage.googleapis.com/charts/cni-${1}.tgz", "https://istio-release.storage.googleapis.com/charts/ztunnel-${1}.tgz" @@ -43,6 +49,7 @@ function add_stable_version() { } END ) + # Insert the new key above the old one (https://stackoverflow.com/questions/74368503/is-it-possible-to-insert-an-element-into-a-middle-of-array-in-yaml-using-yq) # shellcheck disable=SC2016 yq -i '.versions |= ( diff --git a/hack/update-profiles-list.sh b/hack/update-profiles-list.sh index d2d41a016..b838b1f49 100755 --- a/hack/update-profiles-list.sh +++ b/hack/update-profiles-list.sh @@ -39,4 +39,4 @@ sed -i -E \ -e "/\+sail:profile/,/Profile string/ s/(\/\/ \+operator-sdk:csv:customresourcedefinitions:type=spec,displayName=\"Profile\",xDescriptors=\{.*fieldGroup:General\")[^}]*(})/\1$selectValues}/g" \ -e "/\+sail:profile/,/Profile string/ s/(\/\/ \+kubebuilder:validation:Enum=)(.*)/\1$enumValues/g" \ -e "/\+sail:profile/,/Profile string/ s/(\/\/ Must be one of:)(.*)/\1 ${profiles//,/, }./g" \ - api/v1alpha1/istio_types.go api/v1alpha1/remoteistio_types.go api/v1alpha1/istiocni_types.go + api/v1alpha1/istio_types.go api/v1alpha1/istiocni_types.go diff --git a/hack/update-version-list.sh b/hack/update-version-list.sh index 54ab56e2f..fdf39b3a9 100755 --- a/hack/update-version-list.sh +++ b/hack/update-version-list.sh @@ -31,7 +31,7 @@ function updateVersionsInIstioTypeComment() { -e "/\+sail:version/,/Version string/ s/(\/\/ \+kubebuilder:default=)(.*)/\1$defaultVersion/g" \ -e "/\+sail:version/,/Version string/ s/(\/\/ \Must be one of:)(.*)/\1 $versions./g" \ -e "s/(\+kubebuilder:default=.*version: \")[^\"]*\"/\1$defaultVersion\"/g" \ - api/v1alpha1/istio_types.go api/v1alpha1/remoteistio_types.go api/v1alpha1/istiorevision_types.go api/v1alpha1/istiocni_types.go + api/v1alpha1/istio_types.go api/v1alpha1/istiorevision_types.go api/v1alpha1/istiocni_types.go } function updateVersionsInCSVDescription() { diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 5f88662fa..39ca7fa06 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -65,6 +65,10 @@ const ( // reports whether the remote control plane is ready or not WebhookReadinessProbeStatusAnnotationKey = MetadataNamespace + "/readinessProbe.status" + // WebhookReadinessProbeStatusReasonAnnotationKey is an annotation on the istio-sidecar-injection MutatingWebhookConfiguration that + // reports why the remote control plane is not ready + WebhookReadinessProbeStatusReasonAnnotationKey = MetadataNamespace + "/readinessProbe.reason" + // WebhookReadinessProbePeriodSecondsAnnotationKey is an annotation on the istio-sidecar-injection MutatingWebhookConfiguration that // specifies the period for the readiness probe WebhookReadinessProbePeriodSecondsAnnotationKey = MetadataNamespace + "/readinessProbe.periodSeconds" diff --git a/pkg/predicate/predicate.go b/pkg/predicate/predicate.go new file mode 100644 index 000000000..fc4f7466c --- /dev/null +++ b/pkg/predicate/predicate.go @@ -0,0 +1,27 @@ +// Copyright Istio Authors +// +// 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 predicate + +import ( + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" +) + +// ignoreUpdate returns a predicate that ignores update events. +func IgnoreUpdate() predicate.Funcs { + return predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { return false }, + } +} diff --git a/pkg/revision/reconcile.go b/pkg/revision/reconcile.go index a7fcea3fb..e78d1fd0d 100644 --- a/pkg/revision/reconcile.go +++ b/pkg/revision/reconcile.go @@ -26,7 +26,8 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" ) -func CreateOrUpdate(ctx context.Context, cl client.Client, revName string, revType v1alpha1.IstioRevisionType, version string, namespace string, +func CreateOrUpdate( + ctx context.Context, cl client.Client, revName string, version string, namespace string, values *v1alpha1.Values, ownerRef metav1.OwnerReference, ) error { log := logf.FromContext(ctx) @@ -53,7 +54,6 @@ func CreateOrUpdate(ctx context.Context, cl client.Client, revName string, revTy OwnerReferences: []metav1.OwnerReference{ownerRef}, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: revType, Version: version, Namespace: namespace, Values: values, diff --git a/pkg/revision/reconcile_test.go b/pkg/revision/reconcile_test.go index 86a350b22..f48f94bb9 100644 --- a/pkg/revision/reconcile_test.go +++ b/pkg/revision/reconcile_test.go @@ -98,8 +98,7 @@ func TestReconcileActiveRevision(t *testing.T) { Controller: ptr.Of(true), BlockOwnerDeletion: ptr.Of(true), } - revType := v1alpha1.IstioRevisionTypeLocal - err := CreateOrUpdate(ctx, cl, "my-revision", revType, version, "istio-system", &tc.istioValues, ownerRef) + err := CreateOrUpdate(ctx, cl, "my-revision", version, "istio-system", &tc.istioValues, ownerRef) if err != nil { t.Errorf("Expected no error, but got: %v", err) } diff --git a/pkg/revision/remote.go b/pkg/revision/remote.go new file mode 100644 index 000000000..068963cc7 --- /dev/null +++ b/pkg/revision/remote.go @@ -0,0 +1,25 @@ +// Copyright Istio Authors +// +// 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 revision + +import "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + +// IsUsingRemoteControlPlane returns true if the IstioRevision is configured to +// connect to a remote rather than deploy a local control plane. +func IsUsingRemoteControlPlane(rev *v1alpha1.IstioRevision) bool { + // TODO: we should use values.istiodRemote.enabled instead of the profile, but we can't get the final set of values because of new profiles implementation + values := rev.Spec.Values + return values != nil && values.Profile != nil && *values.Profile == "remote" +} diff --git a/pkg/version/semverutils.go b/pkg/version/semverutils.go new file mode 100644 index 000000000..4417ccc6b --- /dev/null +++ b/pkg/version/semverutils.go @@ -0,0 +1,27 @@ +// Copyright Istio Authors +// +// 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 version + +import "github.com/Masterminds/semver/v3" + +// VersionConstraint returns a semver constraint for the given string or panics +// if the string is not a valid semver constraint. +func Constraint(constraint string) semver.Constraints { + c, err := semver.NewConstraint(constraint) + if err == nil { + return *c + } + panic(err) +} diff --git a/pkg/version/semverutils_test.go b/pkg/version/semverutils_test.go new file mode 100644 index 000000000..846485e6a --- /dev/null +++ b/pkg/version/semverutils_test.go @@ -0,0 +1,34 @@ +// Copyright Istio Authors +// +// 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 version + +import ( + "testing" +) + +func TestConstraint(t *testing.T) { + t.Run("valid constraint", func(t *testing.T) { + _ = Constraint(">1.0.0") + }) + + t.Run("invalid constraint", func(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("Expected panic for invalid constraint") + } + }() + _ = Constraint("invalid_version") + }) +} diff --git a/resources/v1.23.2/profiles/remote.yaml b/resources/v1.23.2/profiles/remote.yaml new file mode 100644 index 000000000..d72fd66e4 --- /dev/null +++ b/resources/v1.23.2/profiles/remote.yaml @@ -0,0 +1,5 @@ +# The remote profile is used to configure a mesh cluster without a locally deployed control plane. +# Only the injector mutating webhook configuration is installed. +apiVersion: sailoperator.io/v1alpha1 +kind: Istio +spec: {} diff --git a/tests/e2e/controlplane/control_plane_test.go b/tests/e2e/controlplane/control_plane_test.go index e55967f80..c3e9dcc1e 100644 --- a/tests/e2e/controlplane/control_plane_test.go +++ b/tests/e2e/controlplane/control_plane_test.go @@ -117,9 +117,6 @@ metadata: Describe("given Istio version", func() { for _, version := range supportedversion.List { - // Note: This var version is needed to avoid the closure of the loop - version := version - Context(version.Name, func() { BeforeAll(func() { Expect(k.CreateNamespace(controlPlaneNamespace)).To(Succeed(), "Istio namespace failed to be created") diff --git a/tests/e2e/dualstack/dualstack_test.go b/tests/e2e/dualstack/dualstack_test.go index 9a7768438..15ccb350d 100644 --- a/tests/e2e/dualstack/dualstack_test.go +++ b/tests/e2e/dualstack/dualstack_test.go @@ -70,9 +70,6 @@ var _ = Describe("DualStack configuration ", Ordered, func() { Describe("for supported versions", func() { for _, version := range supportedversion.List { - // Note: This var version is needed to avoid the closure of the loop - version := version - // The minimum supported version is 1.23 (and above) if version.Version.LessThan(semver.MustParse("1.23.0")) { continue diff --git a/tests/e2e/multicluster/multicluster_primaryremote_test.go b/tests/e2e/multicluster/multicluster_primaryremote_test.go index 7ff6db158..ef8275cb8 100644 --- a/tests/e2e/multicluster/multicluster_primaryremote_test.go +++ b/tests/e2e/multicluster/multicluster_primaryremote_test.go @@ -25,6 +25,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/kube" . "github.com/istio-ecosystem/sail-operator/pkg/test/util/ginkgo" "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" + "github.com/istio-ecosystem/sail-operator/pkg/version" "github.com/istio-ecosystem/sail-operator/tests/e2e/util/certs" "github.com/istio-ecosystem/sail-operator/tests/e2e/util/common" . "github.com/istio-ecosystem/sail-operator/tests/e2e/util/gomega" @@ -68,14 +69,14 @@ var _ = Describe("Multicluster deployment models", Ordered, func() { Describe("Primary-Remote - Multi-Network configuration", func() { // Test the Primary-Remote - Multi-Network configuration for each supported Istio version - for _, version := range supportedversion.List { - // The Primary-Remote - Multi-Network configuration is only supported in Istio 1.23, because that's the only - // version that has the istiod-remote chart. For 1.24, we need to rewrite the support for RemoteIstio. - if !(version.Version.Major() == 1 && version.Version.Minor() == 23) { + for _, v := range supportedversion.List { + // The Primary-Remote - Multi-Network configuration is only supported in Istio 1.24+. + if version.Constraint("<1.24").Check(v.Version) { + Log(fmt.Sprintf("Skipping test, because Istio version %s does not support Primary-Remote Multi-Network configuration", v.Version)) continue } - Context(fmt.Sprintf("Istio version %s", version.Version), func() { + Context(fmt.Sprintf("Istio version %s", v.Version), func() { When("Istio resources are created in both clusters", func() { BeforeAll(func(ctx SpecContext) { Expect(k1.CreateNamespace(controlPlaneNamespace)).To(Succeed(), "Namespace failed to be created") @@ -115,7 +116,7 @@ spec: multiCluster: clusterName: %s network: %s` - multiclusterPrimaryYAML := fmt.Sprintf(PrimaryYAML, version.Name, controlPlaneNamespace, "mesh1", "cluster1", "network1") + multiclusterPrimaryYAML := fmt.Sprintf(PrimaryYAML, v.Name, controlPlaneNamespace, "mesh1", "cluster1", "network1") Log("Istio CR Primary: ", multiclusterPrimaryYAML) Expect(k1.CreateFromString(multiclusterPrimaryYAML)).To(Succeed(), "Istio Resource creation failed on Primary Cluster") }) @@ -131,7 +132,7 @@ spec: Eventually(common.GetObject). WithArguments(ctx, clPrimary, kube.Key("istiod", controlPlaneNamespace), &appsv1.Deployment{}). Should(HaveCondition(appsv1.DeploymentAvailable, metav1.ConditionTrue), "Istiod is not Available on Primary; unexpected Condition") - Expect(common.GetVersionFromIstiod()).To(Equal(version.Version), "Unexpected istiod version") + Expect(common.GetVersionFromIstiod()).To(Equal(v.Version), "Unexpected istiod version") Success("Istiod is deployed in the namespace and Running on Primary Cluster") }) }) @@ -154,17 +155,18 @@ spec: }) }) - When("RemoteIstio is created in Remote cluster", func() { + When("Istio is created in Remote cluster", func() { BeforeAll(func(ctx SpecContext) { - RemoteYAML := ` + istioYAMLTemplate := ` apiVersion: sailoperator.io/v1alpha1 -kind: RemoteIstio +kind: Istio metadata: name: default spec: version: %s namespace: istio-system values: + profile: remote istiodRemote: injectionPath: /inject/cluster/remote/net/network2 global: @@ -173,10 +175,10 @@ spec: remotePilotAddress, err := common.GetSVCLoadBalancerAddress(ctx, clPrimary, controlPlaneNamespace, "istio-eastwestgateway") Expect(remotePilotAddress).NotTo(BeEmpty(), "Remote Pilot Address is empty") Expect(err).NotTo(HaveOccurred(), "Error getting Remote Pilot Address") - remoteIstioYAML := fmt.Sprintf(RemoteYAML, version.Name, remotePilotAddress) - Log("RemoteIstio CR: ", remoteIstioYAML) - By("Creating RemoteIstio CR on Remote Cluster") - Expect(k2.CreateFromString(remoteIstioYAML)).To(Succeed(), "RemoteIstio Resource creation failed on Remote Cluster") + istioYAML := fmt.Sprintf(istioYAMLTemplate, v.Name, remotePilotAddress) + Log("Istio CR: ", istioYAML) + By("Creating Istio CR on Remote Cluster") + Expect(k2.CreateFromString(istioYAML)).To(Succeed(), "Istio Resource creation failed on Remote Cluster") // Set the controlplane cluster and network for Remote namespace By("Patching the istio-system namespace on Remote Cluster") @@ -196,13 +198,13 @@ spec: To(Succeed(), "Error patching istio-system namespace") // To be able to access the remote cluster from the primary cluster, we need to create a secret in the primary cluster - // RemoteIstio resource will not be Ready until the secret is created + // Remote Istio resource will not be Ready until the secret is created // Get the internal IP of the control plane node in Remote cluster internalIPRemote, err := k2.GetInternalIP("node-role.kubernetes.io/control-plane") Expect(internalIPRemote).NotTo(BeEmpty(), "Internal IP is empty for Remote Cluster") Expect(err).NotTo(HaveOccurred()) - // Wait for the RemoteIstio CR to be created, this can be moved to a condition verification, but the resource it not will be Ready at this point + // Wait for the remote Istio CR to be created, this can be moved to a condition verification, but the resource it not will be Ready at this point time.Sleep(5 * time.Second) // Install a remote secret in Primary cluster that provides access to the Remote cluster API server. @@ -219,11 +221,11 @@ spec: Success("Remote secret is created in Primary cluster") }) - It("updates RemoteIstio CR status to Ready", func(ctx SpecContext) { + It("updates remote Istio CR status to Ready", func(ctx SpecContext) { Eventually(common.GetObject). - WithArguments(ctx, clRemote, kube.Key(istioName), &v1alpha1.RemoteIstio{}). + WithArguments(ctx, clRemote, kube.Key(istioName), &v1alpha1.Istio{}). Should(HaveCondition(v1alpha1.IstioConditionReady, metav1.ConditionTrue), "Istio is not Ready on Remote; unexpected Condition") - Success("RemoteIstio CR is Ready on Remote Cluster") + Success("Istio CR is Ready on Remote Cluster") }) }) @@ -244,7 +246,7 @@ spec: When("sample apps are deployed in both clusters", func() { BeforeAll(func(ctx SpecContext) { // Deploy the sample app in both clusters - deploySampleApp("sample", version) + deploySampleApp("sample", v) Success("Sample app is deployed in both clusters") }) @@ -279,11 +281,11 @@ spec: }) }) - When("Istio CR and RemoteIstio CR are deleted in both clusters", func() { + When("Istio CR is deleted in both clusters", func() { BeforeEach(func() { - Expect(k1.WithNamespace(controlPlaneNamespace).Delete("istio", istioName)).To(Succeed(), "Istio CR failed to be deleted") - Expect(k2.WithNamespace(controlPlaneNamespace).Delete("remoteistio", istioName)).To(Succeed(), "RemoteIstio CR failed to be deleted") - Success("Istio and RemoteIstio are deleted") + Expect(k1.WithNamespace(controlPlaneNamespace).Delete("istio", istioName)).To(Succeed(), "primary Istio CR failed to be deleted") + Expect(k2.WithNamespace(controlPlaneNamespace).Delete("istio", istioName)).To(Succeed(), "remote Istio CR failed to be deleted") + Success("Primary and Remote Istio resources are deleted") }) It("removes istiod on Primary", func(ctx SpecContext) { @@ -313,6 +315,12 @@ spec: Expect(k1.WaitNamespaceDeleted("sample")).To(Succeed()) Expect(k2.WaitNamespaceDeleted("sample")).To(Succeed()) Success("Sample app is deleted in both clusters") + + // Delete the resources created by istioctl create-remote-secret + Expect(k2.Delete("ClusterRoleBinding", "istiod-clusterrole-istio-system")).To(Succeed()) + Expect(k2.Delete("ClusterRole", "istiod-clusterrole-istio-system")).To(Succeed()) + Expect(k2.Delete("ClusterRoleBinding", "istiod-gateway-controller-istio-system")).To(Succeed()) + Expect(k2.Delete("ClusterRole", "istiod-gateway-controller-istio-system")).To(Succeed()) }) }) } diff --git a/tests/integration/api/istio_test.go b/tests/integration/api/istio_test.go index d939f9cd3..20084d952 100644 --- a/tests/integration/api/istio_test.go +++ b/tests/integration/api/istio_test.go @@ -137,7 +137,6 @@ var _ = Describe("Istio resource", Ordered, func() { }).Should(Succeed()) Expect(rev.Spec).To(Equal(v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: istio.Spec.Version, Namespace: istio.Spec.Namespace, Values: &v1alpha1.Values{ @@ -174,7 +173,6 @@ var _ = Describe("Istio resource", Ordered, func() { Eventually(k8sClient.Get).WithArguments(ctx, revKey, rev).Should(Succeed()) Expect(rev.GetOwnerReferences()).To(ContainElement(NewOwnerReference(istio))) Expect(rev.Spec).To(Equal(v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: istio.Spec.Version, Namespace: istio.Spec.Namespace, Values: &v1alpha1.Values{ @@ -224,8 +222,6 @@ var _ = Describe("Istio resource", Ordered, func() { }) for _, withWorkloads := range []bool{true, false} { - withWorkloads := withWorkloads - Context(generateContextName(withWorkloads), func() { if withWorkloads { BeforeAll(func() { diff --git a/tests/integration/api/istiocni_test.go b/tests/integration/api/istiocni_test.go index f43756ae3..c207d4b11 100644 --- a/tests/integration/api/istiocni_test.go +++ b/tests/integration/api/istiocni_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/istio-ecosystem/sail-operator/api/v1alpha1" + "github.com/istio-ecosystem/sail-operator/pkg/enqueuelogger" "github.com/istio-ecosystem/sail-operator/pkg/kube" . "github.com/istio-ecosystem/sail-operator/pkg/test/util/ginkgo" "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" @@ -43,6 +44,8 @@ var _ = Describe("IstioCNI", Ordered, func() { SetDefaultEventuallyPollingInterval(time.Second) SetDefaultEventuallyTimeout(30 * time.Second) + enqueuelogger.LogEnqueueEvents = true + ctx := context.Background() namespace := &corev1.Namespace{ @@ -236,6 +239,32 @@ var _ = Describe("IstioCNI", Ordered, func() { }).Should(Succeed()) }) }) + + It("skips reconcile when a pull secret is added to service account", func() { + waitForInFlightReconcileToFinish() + + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-cni", + Namespace: cniNamespace, + }, + } + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed()) + + beforeCount := getIstioCNIReconcileCount(Default) + + By("adding pull secret to ServiceAccount") + sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "other-pull-secret"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Consistently(func(g Gomega) { + afterCount := getIstioCNIReconcileCount(g) + g.Expect(afterCount).To(Equal(beforeCount)) + }, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been") + + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed()) + Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"})) + }) }) When("the resource is deleted", func() { diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index 9b17e63ca..4bfd819a3 100644 --- a/tests/integration/api/istiorevision_test.go +++ b/tests/integration/api/istiorevision_test.go @@ -98,7 +98,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: revName, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -118,7 +117,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: revName, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -138,7 +136,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -158,7 +155,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: "default", }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -182,7 +178,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: revName, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: nsName, Values: &v1alpha1.Values{ @@ -249,7 +244,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: revName, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -321,8 +315,8 @@ var _ = Describe("IstioRevision resource", Ordered, func() { DescribeTable("reconciles owned resource", func(obj client.Object, modify func(obj client.Object), validate func(g Gomega, obj client.Object)) { By("on update", func() { - // ensure all in-flight reconcile operations finish before the test; unfortunately, sleeping seems to be the only option to achieve this - time.Sleep(5 * time.Second) + // ensure all in-flight reconcile operations finish before the test + waitForInFlightReconcileToFinish() Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) @@ -386,20 +380,18 @@ var _ = Describe("IstioRevision resource", Ordered, func() { DescribeTable("skips reconcile when only the status of the owned resource is updated", func(obj client.Object, modify func(obj client.Object)) { - // wait for the in-flight reconcile operations to finish - // unfortunately, I don't see a good way to do this other than by waiting - time.Sleep(5 * time.Second) + waitForInFlightReconcileToFinish() Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(obj), obj)).To(Succeed()) - beforeCount := getReconcileCount(Default) + beforeCount := getIstioRevisionReconcileCount(Default) By("modifying object") modify(obj) Expect(k8sClient.Status().Update(ctx, obj)).To(Succeed()) Consistently(func(g Gomega) { - afterCount := getReconcileCount(g) + afterCount := getIstioRevisionReconcileCount(g) g.Expect(afterCount).To(Equal(beforeCount)) }, 5*time.Second).Should(Succeed()) }, @@ -429,6 +421,34 @@ var _ = Describe("IstioRevision resource", Ordered, func() { ), ) + It("skips reconcile when a pull secret is added to service account", func() { + waitForInFlightReconcileToFinish() + + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istiod-" + revName, + Namespace: istioNamespace, + }, + } + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed()) + + GinkgoWriter.Println("sa:", sa) + + beforeCount := getIstioRevisionReconcileCount(Default) + + By("adding pull secret to ServiceAccount") + sa.ImagePullSecrets = append(sa.ImagePullSecrets, corev1.LocalObjectReference{Name: "other-pull-secret"}) + Expect(k8sClient.Update(ctx, sa)).To(Succeed()) + + Consistently(func(g Gomega) { + afterCount := getIstioRevisionReconcileCount(g) + g.Expect(afterCount).To(Equal(beforeCount)) + }, 5*time.Second).Should(Succeed(), "IstioRevision was reconciled when it shouldn't have been") + + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sa), sa)).To(Succeed()) + Expect(sa.ImagePullSecrets).To(ContainElement(corev1.LocalObjectReference{Name: "other-pull-secret"})) + }) + It("supports concurrent deployment of two control planes", func() { rev2Name := revName + "2" rev2Key := client.ObjectKey{Name: rev2Name} @@ -440,7 +460,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: rev2Key.Name, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ @@ -473,7 +492,21 @@ var _ = Describe("IstioRevision resource", Ordered, func() { }) }) -func getReconcileCount(g Gomega) float64 { +func waitForInFlightReconcileToFinish() { + // wait for the in-flight reconcile operations to finish + // unfortunately, I don't see a good way to do this other than by waiting + time.Sleep(5 * time.Second) +} + +func getIstioRevisionReconcileCount(g Gomega) float64 { + return getReconcileCount(g, "istiorevision") +} + +func getIstioCNIReconcileCount(g Gomega) float64 { + return getReconcileCount(g, "istiocni") +} + +func getReconcileCount(g Gomega, controllerName string) float64 { resp, err := http.Get("http://localhost:8080/metrics") g.Expect(err).NotTo(HaveOccurred()) defer resp.Body.Close() @@ -487,7 +520,7 @@ func getReconcileCount(g Gomega) float64 { sum := float64(0) for _, metric := range mf.Metric { for _, l := range metric.Label { - if *l.Name == "controller" && *l.Value == "istiorevision" { + if *l.Name == "controller" && *l.Value == controllerName { sum += metric.GetCounter().GetValue() } } diff --git a/tests/integration/api/suite_test.go b/tests/integration/api/suite_test.go index f7cd9e7f4..662b9ea80 100644 --- a/tests/integration/api/suite_test.go +++ b/tests/integration/api/suite_test.go @@ -24,7 +24,6 @@ import ( "github.com/istio-ecosystem/sail-operator/controllers/istio" "github.com/istio-ecosystem/sail-operator/controllers/istiocni" "github.com/istio-ecosystem/sail-operator/controllers/istiorevision" - "github.com/istio-ecosystem/sail-operator/controllers/remoteistio" "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/scheme" @@ -83,7 +82,6 @@ var _ = BeforeSuite(func() { cl := mgr.GetClient() scheme := mgr.GetScheme() Expect(istio.NewReconciler(cfg, cl, scheme).SetupWithManager(mgr)).To(Succeed()) - Expect(remoteistio.NewReconciler(cfg, cl, scheme).SetupWithManager(mgr)).To(Succeed()) Expect(istiorevision.NewReconciler(cfg, cl, scheme, chartManager).SetupWithManager(mgr)).To(Succeed()) Expect(istiocni.NewReconciler(cfg, cl, scheme, chartManager).SetupWithManager(mgr)).To(Succeed())