Assign predefined clustersetIP to service status

[andrewsykim]

This PR updates the current demo to to set Service.Status.LoadBalancer if clusterset IPs is predefined in ServiceImport.

This allows the implementation to rely on clusterIP for ServiceImport.Spec.IPs or pre-allocate a well known IP for the clustersetIP.

Signed-off-by: Andrew Sy Kim kim.andrewsy@gmail.com

[andrewsykim]

/assign @JeremyOT

[andrewsykim]

Some manual validation running the demo script:

$ kubectl --kubeconfig scripts/c1.kubeconfig -n demo get serviceimports
NAME    TYPE           IP          AGE
serve   ClusterSetIP   [1.2.3.4]   13m
$
$ kubectl --kubeconfig scripts/c1.kubeconfig -n demo get svc
NAME                 TYPE        CLUSTER-IP     EXTERNAL-IP   PORT(S)   AGE
derived-ovtkn0laet   ClusterIP   10.11.78.176   <none>        80/TCP    2m36s
serve                ClusterIP   10.11.174.93   <none>        80/TCP    2m49s
$ 
$ kubectl --kubeconfig scripts/c1.kubeconfig -n demo get svc derived-ovtkn0laet -o yaml
apiVersion: v1
kind: Service
metadata:
  creationTimestamp: "2020-08-11T15:22:26Z"
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:ownerReferences:
          .: {}
          k:{"uid":"edd5dedb-5ee7-4b57-8c91-9654bff70449"}:
            .: {}
            f:apiVersion: {}
            f:kind: {}
            f:name: {}
            f:uid: {}
      f:spec:
        f:ports:
          .: {}
          k:{"port":80,"protocol":"TCP"}:
            .: {}
            f:port: {}
            f:protocol: {}
            f:targetPort: {}
        f:sessionAffinity: {}
        f:type: {}
      f:status:
        f:loadBalancer:
          f:ingress: {}
    manager: controller
    operation: Update
    time: "2020-08-11T15:22:26Z"
  name: derived-ovtkn0laet
  namespace: demo
  ownerReferences:
  - apiVersion: multicluster.x-k8s.io/v1alpha1
    kind: ServiceImport
    name: serve
    uid: edd5dedb-5ee7-4b57-8c91-9654bff70449
  resourceVersion: "688"
  selfLink: /api/v1/namespaces/demo/services/derived-ovtkn0laet
  uid: a502033b-aab5-491d-8b7f-25a5ff32b3f6
spec:
  clusterIP: 10.11.78.176
  ports:
  - port: 80
    protocol: TCP
    targetPort: 80
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer:
    ingress:
    - ip: 1.2.3.4

[andrewsykim]

Based on some conversations with @JeremyOT , I've updated the PR so that we add IPs to service status if a clustersetIP is predefined. Otherwise we continue to put clusterIP into ServiceImport.Spec.IPs

$ kubectl --kubeconfig ./scripts/c1.kubeconfig get serviceimport -A
NAMESPACE   NAME             TYPE           IP                AGE
demo        serve            ClusterSetIP   [10.11.104.125]   2m6s
demo        serve-with-vip   ClusterSetIP   [1.2.3.4]         2m5s

[JeremyOT]

It's a small chance, but it seems like we want to verify the current state of the Service as well. As is, if the status update were to fail, we'd see the existing service on the next iteration and never set the LB IP. Since the updated Service controller ignores an import with a pre-existing IP, we wouldn't reconcile from the other side either

[andrewsykim]

I had the same thought as I was writing this initially 🤔

I agree with you, but I think that would require a larger refactor of this controller since we only reconcile the Service if it doesn't exist (based on line 93 - 97). So if the Service already exists, we would return early and never attempt to reconcile the status anyways. Was actually planning on adding the "update if already exists" logic for Services in a follow-up PR but I can do it here if you prefer.

[JeremyOT]

Thanks for this!

[JeremyOT]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, JeremyOT

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [JeremyOT]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment