Implement support for PodSandboxStatus RPC

[MikeZappa87]

This PR is to add PodSandboxStatus support to NRI.

The idea to why we want to bring this RPC in now is that we want to try and begin the process to deprecate the CNI and we need a way to return Pod IP's back to the kubelet. Without this we ether need to come up with a new RPC outside of NRI or experiment directly in the kubelet. Using NRI seems to be the best place right now. This is why the PodSandboxStatusResponse type only returns pod ip addresses, its open to more fields be added.

I put together this branch in containerd that brings this NRI branch in and disables the CNI as a test bed.

containerd/containerd@main...MikeZappa87:containerd:mzappa/knext

[LionelJouin]

This is a great thing to add, thanks @MikeZappa87 !

/cc @aojea @squeed @mskrocki

[klihub]

I have a few questions about the chosen semantics. But it might be that I just lack the necessary context here...

[klihub]

What is the reason for the chosen semantics wrt. multiple plugins ? Especially for the who comes later wins overriding everything, even if the former set only one attribute and the latter only the other, in which case a(n AFAICT) non-conflicting setting is silently lost.

I think we should consider having a similar (or using the same) conflict detection/resolution mechanism as we have/do for containers. Even if we don't put it fully in place now/yet, we could try faking it here with something like...

func (r *Adaptation) PodSandboxStatus(ctx context.Context, req *PodSandboxStatusRequest) (*PodSandboxStatusResponse, error) {
	r.Lock()
	defer r.Unlock()
	defer r.removeClosedPlugins()

    var (
        rsp                = &PodSandboxStatusResponse{}
        ipOwner            = ""
        additionalIpsOwner = ""
    )
    
	for _, plugin := range r.plugins {
		pluginRsp, err := plugin.podSandboxStatus(ctx, req)
		if err != nil {
			return nil, err
		}
        if pluginRsp == nil || (pluginRsp.Ip == "" && len(pluginRsp.AdditionaIps) == 0) {
            continue
        }
        if pluginRsp.Ip != "" {
            if ipOwner != "" {
                return nil, fmt.Errorf("plugins %q and %q both tried to set PodSandboxStatus IP address field", ipOwner, plugin.name()
            }
            rsp.Ip = pluginRsp.Ip
            rsp.ipOwner = plugin.name()
        } 
        if len(pluginRsp.AdditionalIps) > 0 {
            if additionalIpOwner != "" {
                return nil, fmt.Errorf("plugins %q and %q both tried to set PodSandboxStatus additional IP addresses field", additionalIpOwner, plugin.name()) 
            }
            rsp.AdditionalIps = pluginRsp.AdditionalIps
            additionaIpsOwner = plugin.name()
        }
	}

    return rsp, nil
}

[MikeZappa87]

This is I am going to change. I did this for the PR. I had a to do note to fix. Will resolve

[MikeZappa87]

@klihub we probably want to modify this signature or leave it? Not sure if want an envelope type here to reduce future changes?

[klihub]

@klihub we probably want to modify this signature or leave it? Not sure if want an envelope type here to reduce future changes?

Yeah, since this feels like it might get more fields added later, either just return a *api.PodSandboxStatusResponse in the stub interface, or then a wrapping/envelope type like

message PodSandboxStatus {
    string ip = 1;
    repeated strings additional_ips = 2;
}

with a corresponding PodSandboxStatus status = 1; field in the PodSandboxStatusResponse message, so then it is easy to extend this later with more fields. And you'd return a (*api.PodSandboxStatus, error) in the corresponding stub interface.

This is especially compelling, if that status might see some use on the plugin input side, or other context, some day. IOW if in some cases we could/would need to pass the same/similar info to plugins as input as well. Then it feels like a natural choice to have a dedicated wrapping type/message for it.

I guess there could be a third alternative too, which would be to limit the scope more closely/explicitly to network setup and then have this visible in naming too. IOW, to call these something else than a generic PodSandboxStatus, both in the RPC call and in the message name(s). But that's orthogonal to the 'what we should return from the stub interface' question of yours.