open-feature / open-feature-operator

A Kubernetes feature flag operator
https://openfeature.dev
Apache License 2.0
198 stars 38 forks source link

pre 0.8.0 flagd-proxy service does not contain managed-by label #719

Open xvzf opened 5 days ago

xvzf commented 5 days ago

Changes introduced in #712 do not only validate the ownership of the flagd-proxy deployment but also the service, which leads to the service not being managed further by the OFO.

This only affects installations prior to 0.8.0 with flagd-proxy enabled.

A fix for this issue is to manually label the affected resource:

kubectl label -n open-feature-operator-system svc/flagd-proxy-svc app.kubernetes.io/managed-by=open-feature-operator
xvzf commented 5 days ago

@beeme1mr I personally think it's sufficient to add a warning to the 0.8.0 release for this as a breaking change, wdyt?

wondertroy commented 4 days ago

Does ownership have to be validated via checking for that label?

I can see in a previous version of the Service resource for flagd-proxy that it contains ownerReferences in the metadata, and the managed-by field within the selector in the spec. I'm making naive assumptions that these fields are all injected and accessible so might not be possible, but wondering if they could be used to validate ownership instead of the label?

Eg: Older version Service resource pre- 0.8.0

apiVersion: v1
kind: Service
metadata:
  creationTimestamp: XXXX-XX-XXTXX:XX:17Z
  name: flagd-proxy-svc
  namespace: XXXX
  ownerReferences:
    - apiVersion: apps/v1
      kind: Deployment
      name: open-feature-operator-controller-manager # <-- here
      uid: XXXXXXXXXX
  resourceVersion: "XXXX"
  uid: 689f2c91-b183-4d4f-a295-26f27402b5d6
spec:
  clusterIP: XX.XXX.XXX.XX
  clusterIPs:
    - XX.XXX.XXX.XX
  internalTrafficPolicy: Cluster
  ipFamilies:
    - IPv4
  ipFamilyPolicy: SingleStack
  ports:
    - xxx
  selector:
    app.kubernetes.io/managed-by: open-feature-operator # <-- here
    app.kubernetes.io/name: flagd-proxy
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}

New service resource post 0.8.0 update:

apiVersion: v1
kind: Service
metadata:
  creationTimestamp:XXXX-XX-XXTXX:XX:17Z
  labels:
    app.kubernetes.io/managed-by: open-feature-operator # <-- new / easily added manually if needed
  name: flagd-proxy-svc
  namespace: flags-system
  ownerReferences:
    - apiVersion: apps/v1
      kind: Deployment
      name: open-feature-operator-controller-manager
      uid: xxxxxx
  resourceVersion: "xxxxxx"
  uid: xxxxx xx
spec:
  clusterIP: xx.xxx.xxx.xxx
  clusterIPs:
    - xx.xxx.xxx.xxx
  internalTrafficPolicy: Cluster
  ipFamilies:
    - IPv4
  ipFamilyPolicy: SingleStack
  ports:
    - xxxx
  selector:
    app.kubernetes.io/managed-by: open-feature-operator
    app.kubernetes.io/name: flagd-proxy
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}

Example diff:

Index: common/common.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/common/common.go b/common/common.go
--- a/common/common.go  (revision bcdafd29a0bd5a64f8de35bfe573566155021226)
+++ b/common/common.go  (date 1730333262536)
@@ -84,5 +84,8 @@

 func IsManagedByOFO(obj client.Object) bool {
    val, ok := obj.GetLabels()[ManagedByAnnotationKey]
-   return ok && val == ManagedByAnnotationValue
+   ownerRefs := obj.GetOwnerReferences()
+   return ok && val == ManagedByAnnotationValue ||
+       len(ownerRefs) == 1 &&
+           ownerRefs[0].Name == OperatorDeploymentName
 }
xvzf commented 4 days ago

I'd personally use the ownerReference as well, but I assumed the decision to use a label has been taken on purpose.