diff --git a/pkg/feature/manifest.go b/pkg/feature/manifest.go index e83afa83770..29f97eca971 100644 --- a/pkg/feature/manifest.go +++ b/pkg/feature/manifest.go @@ -12,8 +12,6 @@ import ( "github.com/ghodss/yaml" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" ) type Manifest interface { @@ -99,21 +97,6 @@ func (t *templateManifest) MarkAsManaged(objects []*unstructured.Unstructured) { } } -func markAsManaged(objs []*unstructured.Unstructured) { - for _, obj := range objs { - objAnnotations := obj.GetAnnotations() - if objAnnotations == nil { - objAnnotations = make(map[string]string) - } - - // If resource already has an annotation, leave it as defined - if _, exists := objAnnotations[annotations.ManagedByODHOperator]; !exists { - objAnnotations[annotations.ManagedByODHOperator] = "true" - obj.SetAnnotations(objAnnotations) - } - } -} - func loadManifestsFrom(fsys fs.FS, path string) ([]Manifest, error) { var manifests []Manifest diff --git a/pkg/feature/raw_resources.go b/pkg/feature/raw_resources.go index 2c5c3b0efda..e03853b62fc 100644 --- a/pkg/feature/raw_resources.go +++ b/pkg/feature/raw_resources.go @@ -23,7 +23,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" ) const ( @@ -56,14 +56,11 @@ func applyResources(ctx context.Context, cli client.Client, objects []*unstructu return nil } - isManaged, isAnnotated := target.GetAnnotations()[annotations.ManagedByODHOperator] - if isAnnotated && isManaged == "true" { - // reconcile the target since we manage it + if shouldReconcile(source) { if errUpdate := patchUsingApplyStrategy(ctx, cli, source, target); errUpdate != nil { - return fmt.Errorf("failed to update source %s/%s: %w", namespace, name, errUpdate) + return fmt.Errorf("failed to reconcile resource %s/%s: %w", namespace, name, errUpdate) } } - // source exists and is not manged, skip reconcile allowing users to tweak it } return nil @@ -105,3 +102,50 @@ func patchUsingMergeStrategy(ctx context.Context, cli client.Client, patch *unst return nil } + +// At this point we only look at what is defined for the resource in the desired state (source), +// so the one provided by the operator (e.g. embedded manifest files). +// +// Although the actual instance on the cluster (the target) might be in a different state, +// we intentionally do not address this scenario due to the lack of clear requirements +// on the extent to which users can modify it. Additionally, addressing this would +// necessitate extra reporting (e.g., via status.conditions on a FeatureTracker) +// to highlight discrepancies between the actual and desired state when users opt out +// of operator management/reconciliation. +func shouldReconcile(source *unstructured.Unstructured) bool { + if isManaged(source) { + return true + } + + if isUnmanaged(source) { + return false + } + + // In all the other cases preserve original behaviour + return false +} + +func markAsManaged(objs []*unstructured.Unstructured) { + for _, obj := range objs { + objLabels := obj.GetLabels() + if objLabels == nil { + objLabels = make(map[string]string) + } + + // If resource already has the label defined it should take precedence + if _, exists := objLabels[labels.ManagedByODHOperator]; !exists { + objLabels[labels.ManagedByODHOperator] = "true" + obj.SetLabels(objLabels) + } + } +} + +func isUnmanaged(obj *unstructured.Unstructured) bool { + managed, isLabeled := obj.GetLabels()[labels.ManagedByODHOperator] + return isLabeled && managed == "false" +} + +func isManaged(obj *unstructured.Unstructured) bool { + managed, isLabeled := obj.GetLabels()[labels.ManagedByODHOperator] + return isLabeled && managed == "true" +} diff --git a/pkg/metadata/annotations/annotations.go b/pkg/metadata/annotations/annotations.go index e2d4cfde922..567c0a9f09d 100644 --- a/pkg/metadata/annotations/annotations.go +++ b/pkg/metadata/annotations/annotations.go @@ -1,8 +1,5 @@ package annotations -// ManagedByODHOperator is used to denote if a resource/component should be reconciled - when true, reconcile. -const ManagedByODHOperator = "opendatahub.io/managed" - // trust CA bundler. const InjectionOfCABundleAnnotatoion = "security.opendatahub.io/inject-trusted-ca-bundle" diff --git a/pkg/metadata/labels/types.go b/pkg/metadata/labels/types.go index 382c2e38db3..f92cafa2977 100644 --- a/pkg/metadata/labels/types.go +++ b/pkg/metadata/labels/types.go @@ -1,10 +1,12 @@ package labels const ( - ODHAppPrefix = "app.opendatahub.io" - InjectTrustCA = "config.openshift.io/inject-trusted-cabundle" - SecurityEnforce = "pod-security.kubernetes.io/enforce" - ClusterMonitoring = "openshift.io/cluster-monitoring" + // ManagedByODHOperator is used to denote if a resource/component should be reconciled - when true, reconcile. + ManagedByODHOperator = "opendatahub.io/managed" + ODHAppPrefix = "app.opendatahub.io" + InjectTrustCA = "config.openshift.io/inject-trusted-cabundle" + SecurityEnforce = "pod-security.kubernetes.io/enforce" + ClusterMonitoring = "openshift.io/cluster-monitoring" ) // K8SCommon keeps common kubernetes labels [1] diff --git a/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml index 92760cdd564..e79070f3c3b 100644 --- a/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml @@ -1,9 +1,8 @@ apiVersion: v1 kind: Service metadata: - annotations: - test-annotation: "original-value" labels: + test-label: "original-value" experimental.istio.io/disable-gateway-port-translation: "true" name: knative-local-gateway namespace: {{ .ControlPlane.Namespace }} diff --git a/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml index 56654bb8863..909d2aed688 100644 --- a/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml @@ -3,9 +3,9 @@ kind: Service metadata: name: managed-svc namespace: {{ .TargetNamespace }} - annotations: + labels: opendatahub.io/managed: "true" - test-annotation: "original-value" + test-label: "original-value" spec: ports: - name: http2 diff --git a/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml b/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml index a4318bb3413..a3b8af641f7 100644 --- a/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml +++ b/tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml @@ -3,9 +3,9 @@ kind: Service metadata: name: unmanaged-svc namespace: {{ .TargetNamespace }} - annotations: + labels: opendatahub.io/managed: "false" - test-annotation: "original-value" + test-label: "original-value" spec: ports: - name: http2 diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go index 7c7404cbfed..d876eb92b62 100644 --- a/tests/integration/features/resources_int_test.go +++ b/tests/integration/features/resources_int_test.go @@ -2,6 +2,7 @@ package features_test import ( "context" + "fmt" "path" corev1 "k8s.io/api/core/v1" @@ -9,7 +10,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" "github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil" "github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures" @@ -26,9 +27,9 @@ var _ = Describe("Applying and updating resources", func() { ) const ( - testAnnotationKey = "test-annotation" - newValue = "new-value" - originalValue = "original-value" + testLabelKey = "test-label" + newValue = "new-value" + originalValue = "original-value" ) BeforeEach(func(ctx context.Context) { @@ -66,12 +67,12 @@ var _ = Describe("Applying and updating resources", func() { // expect created svc to have managed annotation service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) - Expect(service.Annotations).To( - HaveKeyWithValue(annotations.ManagedByODHOperator, "true"), + Expect(service.Labels).To( + HaveKeyWithValue(labels.ManagedByODHOperator, "true"), ) // when - service.Annotations[testAnnotationKey] = newValue + service.Labels[testLabelKey] = newValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -79,8 +80,8 @@ var _ = Describe("Applying and updating resources", func() { Expect(featuresHandler.Apply(ctx)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) - Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, originalValue), + Expect(updatedService.Labels).To( + HaveKeyWithValue(testLabelKey, originalValue), ) }) @@ -101,7 +102,7 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Labels[testLabelKey] = newValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -109,8 +110,8 @@ var _ = Describe("Applying and updating resources", func() { Expect(featuresHandler.Apply(ctx)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc") Expect(err).ToNot(HaveOccurred()) - Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, newValue), + Expect(updatedService.Labels).To( + HaveKeyWithValue(testLabelKey, newValue), ) }) @@ -133,7 +134,7 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Labels[testLabelKey] = newValue Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -141,8 +142,8 @@ var _ = Describe("Applying and updating resources", func() { Expect(featuresHandler.Apply(ctx)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway") Expect(err).ToNot(HaveOccurred()) - Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, newValue), + Expect(updatedService.Labels).To( + HaveKeyWithValue(testLabelKey, newValue), ) }) @@ -164,7 +165,10 @@ var _ = Describe("Applying and updating resources", func() { // when service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") Expect(err).ToNot(HaveOccurred()) - service.Annotations[testAnnotationKey] = newValue + service.Labels[testLabelKey] = newValue + service.Spec.ClusterIP = "" + service.Spec.Type = corev1.ServiceTypeExternalName + service.Spec.ExternalName = "test-external-name" Expect(envTestClient.Update(ctx, service)).To(Succeed()) // then @@ -172,9 +176,11 @@ var _ = Describe("Applying and updating resources", func() { Expect(featuresHandler.Apply(ctx)).To(Succeed()) updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc") Expect(err).ToNot(HaveOccurred()) - Expect(updatedService.Annotations).To( - HaveKeyWithValue(testAnnotationKey, originalValue), + Expect(updatedService.Labels).To( + HaveKeyWithValue(testLabelKey, originalValue), ) + Expect(updatedService.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP)) + fmt.Println(updatedService.Spec.ExternalName) }) })