CRDs: additionalPrinterColumns configuration

[attiss]

This commit adds +kubebuilder:printcolumn markers to CRD structs so the generated CRDs are extended with additionalPrinterColumns properties.

---

Closes: #1459

[attiss]

BFDProfile

$ kubectl get bfdprofiles -A
NAMESPACE        NAME             PASSIVE MODE   TRANSMIT INTERVAL   RECEIVE INTERVAL   MULTIPLIER
metallb-system   passive-bfd      true           270                 380                5
metallb-system   testbfdprofile                  270                 380

---

BGPPeer

$ kubectl get bgppeers -A
NAMESPACE        NAME         ADDRESS      ASN     BFD PROFILE      MULTI HOPS
metallb-system   peer         172.18.0.5   64514                    true
metallb-system   peersample   172.30.0.3   64512   testbfdprofile

---

IPAddressPool

$ kubectl get ipaddresspools -A
NAMESPACE        NAME          AUTO ASSIGN   AVOID BUGGY IPS   ADDRESSES
metallb-system   dev-env-bgp   true          false             ["192.168.10.0/24","fc00:f853:0ccd:e799::/124"]
metallb-system   second-pool   true          false             ["192.168.20.0/24","192.168.9.6-192.168.9.10"]
metallb-system   third-pool    true          false             ["10.0.0.0/8"]

---

Community

$ kubectl get communities -A
NAMESPACE        NAME          AGE
metallb-system   communities   157m

I wanted to add a single column that provides a list of alias names that the individual resources define using the following marker:

+kubebuilder:printcolumn:name="Communities",type=string,JSONPath=`.spec.communities[*].name`

However, it doesn't work as expected, only the first item from the .spec.communities array is added to the output.
(Related kubectl issue: kubernetes/kubectl#517)

---

L2Advertisement

$ kubectl get l2advertisements -A
NAMESPACE        NAME      IPADDRESSPOOLS   IPADDRESSPOOL SELECTORS                INTERFACES
metallb-system   example   ["first-pool"]   [{"matchLabels":{"type":"private"}}]   ["eth0"]

$ kubectl get l2advertisements -A -o wide
NAMESPACE        NAME      IPADDRESSPOOLS   IPADDRESSPOOL SELECTORS                INTERFACES   NODE SELECTORS
metallb-system   example   ["first-pool"]   [{"matchLabels":{"type":"private"}}]   ["eth0"]     [{"matchLabels":{"kubernetes.io/hostname":"NodeA"}},{"matchLabels":{"kubernetes.io/hostname":"NodeB"}}]

---

BGPAdvertisement

$ kubectl get bgpadvertisements -A
NAMESPACE        NAME      IPADDRESSPOOLS                  IPADDRESSPOOL SELECTORS                PEERS
metallb-system   empty     ["dev-env-bgp"]
metallb-system   example   ["dev-env-bgp","second-pool"]                                          ["peersample"]
metallb-system   private                                   [{"matchLabels":{"type":"private"}}]   ["peersample"]

$ kubectl get bgpadvertisements -A -o wide
NAMESPACE        NAME      IPADDRESSPOOLS                  IPADDRESSPOOL SELECTORS                PEERS            NODE SELECTORS
metallb-system   empty     ["dev-env-bgp"]
metallb-system   example   ["dev-env-bgp","second-pool"]                                          ["peersample"]
metallb-system   private                                   [{"matchLabels":{"type":"private"}}]   ["peersample"]   [{"matchLabels":{"kubernetes.io/hostname":"NodeA"}}]

[fedepaol]

Any reason why we don't have the node selector here?

[attiss]

I added those fields which seemed to be the most "important" for the resources. (So the output doesn't get too wide.)
Or in other words those configurations which I'd most often check.

In case of BGPAdvertisement for me it seemed these were the most important things:

• what addresses are we advertising,
• who are we advertising to.

I'm happy to add the node selector if that's also something users would usually set and check.

[attiss]

(Btw. we can even add "hidden" fields that are only appear with -o wide.)

[fedepaol]

Yep, I was curious about the asymmetry between l2 and bgp advertisement, where we had the node selector only on the l2 one.
Regarding hidden fields, TIL. I think it's reasonable to keep the node selector hidden in both.

[fedepaol]

I am torn about the selectors, but I think the only alternative is to leave them out (which might be even more confusing).
One mitigation can be leave them last.
In case of both ipaddresspool selector / node selector I'd give this order:
.... IPAddressPools - IPAddressPool selectors - Node Selectors

So at least they are on the right when a user sees them.

[attiss]

I am torn about the selectors, but I think the only alternative is to leave them out (which might be even more confusing).
One mitigation can be leave them last.

That'd definitely a more difficult change, but I think an alternative solution would be to add (and always keep up to date) an IPAddressPool / Node array to the Status of these resources which are matching either the "static" reference or the selector criteria.

Then we could print the name of all the matching IPAddressPools or Nodes in a single column.

For example:

apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  name: example
  namespace: metallb-system
spec:
  ipAddressPools:
  - first-pool
  - second-pool
  ipAddressPoolSelectors:
  - matchLabels:
      type: example
status:
  ipAddressPools:
  - first-pool
  - second-pool
  - example-1-pool
  - example-2-pool

⬇️

$ kubectl get bgpadvertisements -A
NAMESPACE        NAME      IPADDRESSPOOLS                                                     PEERS
metallb-system   example   ["first-pool", "second-pool", "example-1-pool", "example-2-pool"]  ["peersample"]                                    

[fedepaol]

I am torn about the selectors, but I think the only alternative is to leave them out (which might be even more confusing).
One mitigation can be leave them last.

That'd definitely a more difficult change, but I think an alternative solution would be to add (and always keep up to date) an IPAddressPool / Node array to the Status of these resources which are matching either the "static" reference or the selector criteria.

Then we could print the name of all the matching IPAddressPools or Nodes in a single column.

Sounds legit. Let me add that to the list of things it may make sense to expose as status. I am working on a proposal (that will turn in a lot of issues :P to be worked) to collect all the status we may want to expose.

I'd apply the change suggested above (hide the node selectors) and go out with this first version. It's not as binding as an api in any case, and we can change it later on.

For example:

apiVersion: metallb.io/v1beta1
kind: BGPAdvertisement
metadata:
  name: example
  namespace: metallb-system
spec:
  ipAddressPools:
  - first-pool
  - second-pool
  ipAddressPoolSelectors:
  - matchLabels:
      type: example
status:
  ipAddressPools:
  - first-pool
  - second-pool
  - example-1-pool
  - example-2-pool

arrow_down

$ kubectl get bgpadvertisements -A
NAMESPACE        NAME      IPADDRESSPOOLS                                                     PEERS
metallb-system   example   ["first-pool", "second-pool", "example-1-pool", "example-2-pool"]  ["peersample"]                                    

[attiss]

I am working on a proposal (that will turn in a lot of issues :P to be worked) to collect all the status we may want to expose.

Sounds good! 🥳

I'd apply the change suggested above (hide the node selectors) and go out with this first version.

Changes applied. I also updated the output example in the comment: #1632 (comment)

[fedepaol]

LGTM!
Thanks! Let's wait for the CI