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..5411d4af5 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-06T06:10:10Z" 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/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/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index 13d162684..fb022237c 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -31,6 +31,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/kube" "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 +62,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 +128,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 +169,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 } @@ -332,8 +318,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 +339,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 +363,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 +593,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/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/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/multicluster/multicluster_primaryremote_test.go b/tests/e2e/multicluster/multicluster_primaryremote_test.go index 7ff6db158..92182f20f 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,10 @@ 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()) }) }) } diff --git a/tests/integration/api/istio_test.go b/tests/integration/api/istio_test.go index d939f9cd3..ce7254443 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{ diff --git a/tests/integration/api/istiorevision_test.go b/tests/integration/api/istiorevision_test.go index 9b17e63ca..5ceeed326 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{ @@ -440,7 +434,6 @@ var _ = Describe("IstioRevision resource", Ordered, func() { Name: rev2Key.Name, }, Spec: v1alpha1.IstioRevisionSpec{ - Type: v1alpha1.IstioRevisionTypeLocal, Version: supportedversion.Default, Namespace: istioNamespace, Values: &v1alpha1.Values{ 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())