MaterializeInc / k8s-eip-operator

Apache License 2.0
11 stars 2 forks source link

EIP operator should look for annotations, not labels #224

Open benesch opened 2 years ago

benesch commented 2 years ago

@pH14 suggests that the EIP operator should use annotations rather than labels to identify the pods that need EIPs:

eip.materialize.cloud/manage=true
eip.materialize.cloud/autocreate_eip=true

I think the idea (@ph14 please correct me) is that this better fits with the Kubernetes models. External systems that act on a resource should store their metadata in annotations; only the object's one true owner should set its labels. Context: https://materializeinc.slack.com/archives/CL68GT3AT/p1658153578418489?thread_ts=1658150176.284539&cid=CL68GT3AT

If we make this change, we should take the opportunity to rename autocreate_eip to autocreate-eip to match Kubernetes convention.

pH14 commented 2 years ago

External systems that act on a resource should store their metadata in annotations; only the object's one true owner should set its labels

yep! that sounds right, some prior art here include external-dns and AWS's load balancer controller

alex-hunt-materialize commented 2 years ago

Can you filter by annotations? We use labels so that we only watch pods with those labels, not all pods. AFAIK, there isn't a way to filter by annotations in a controller's watch.

only the object's one true owner should set its labels.

The eip-operator never writes these labels. It only reads them to know that it needs to care about that pod. The owner of the pod is already the only one writing these labels.

From the docs at https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ :

Labels can be used to organize and to select subsets of objects.

Labels allow for efficient queries and watches and are ideal for use in UIs and CLIs. Non-identifying information should be recorded using annotations.

We're using eip.materialize.cloud/manage=true for identification, which seems like a textbook label use case. eip.materialize.cloud/autocreate_eip=true could be moved to an annotation, though.