Add support for NetworkConfigProxy v0 and v1 api

[katiewasnothere]

This PR adds support to ncproxy service for both the NetworkConfigProxy grpc v0 and v1 api. This is done by converting the v0 api to the v1 api before calling the underlying implementation of the various ncproxy calls and converting from the v1 responses to the v0 responses before returning the results. This PR additionally adds tests for the v0 api.

Currently with the NetworkConfigProxy v1 api, ncproxy supports two types of networking: hcn networking and ncproxy networking. NetworkConfigProxy grpc v0 api notably does not have support for the ncproxy networking types, dual stack, or the additional hcn policies that are present in the v1 api.

Note: v0 support is intended to be temporary, so this code will likely be removed in the near future.

[helsaawy]

proto is failing cause of formatting in the generated go code, which is think is difference with how go1.19 and 1.20 gofmt runs.

you could manually change the files, or use go 1.19 locally, or upgrade the pipeline to 1.20

[helsaawy]

Isn't this a pretty major breaking change, since anyone currently using the v1 API will have their code break suddenly?

Can we keep the original v1 name, but instead call the old package v1legacy or something like that?

[katiewasnothere]

Isn't this a pretty major breaking change, since anyone currently using the v1 API will have their code break suddenly?

Can we keep the original v1 name, but instead call the old package v1legacy or something like that?

Yes, this is a breaking change, however, afaik, no one is using the v2 api or even has the v2 api file pulled in, so we should be safe to change that.

[kevpar]

Isn't this a pretty major breaking change, since anyone currently using the v1 API will have their code break suddenly?
Can we keep the original v1 name, but instead call the old package v1legacy or something like that?

Yes, this is a breaking change, however, afaik, no one is using the v2 api or even has the v2 api file pulled in, so we should be safe to change that.

I'd suggest we keep the latest proto file unchanged, and just add the old one in a package dir named something like v0/legacy/beta/etc. That way it's not a breaking change to anyone.

[katiewasnothere]

I'd suggest we keep the latest proto file unchanged, and just add the old one in a package dir named something like v0/legacy/beta/etc. That way it's not a breaking change to anyone.

@kevpar and @helsaawy updated to use v0 for the old api and kept the current api as v1.

[kevpar]

I think PR title/description need to be updated.

[kevpar]

You cannot run the v0 implementation at the same time as the v1 implementation since the v0 implementation will return an error if any "ncproxy networks" or "ncproxy endpoints" are present.

I'm confused by this comment. Isn't running v0 and v1 at the same time precisely what this PR is doing?

[katiewasnothere]

You cannot run the v0 implementation at the same time as the v1 implementation since the v0 implementation will return an error if any "ncproxy networks" or "ncproxy endpoints" are present.

I'm confused by this comment. Isn't running v0 and v1 at the same time precisely what this PR is doing?

@kevpar This PR adds the ability to use v0 or v1 within the same executable for ncproxy. This was more of an implementation decision that I made. Since the code works by converting the v0 api to the v1 api and back for the response, if both apis are being used simultaneously, it's possible there are "ncproxy networks" or "ncproxy endpoints" that were created from a call to the v1 apis. In that scenario, a v0 agent making a call to something like "GetNetworks" would get an error since there are "ncproxy networks" that ncproxy is tracking which cannot be encoded in the response to the v0 "GetNetworks" api. We could instead just ignore "ncproxy networks" or "ncproxy endpoints" and add a log on our side, but I was concerned about doing that since we wouldn't be giving the node network agent the true picture of what networks or endpoints have been created through calls to ncproxy.

So basically the expectation is that if node network agent wants to use the v1 api, it must fully move all calls to ncproxy over to the v1 api, not just a subset. This will hopefully additionally prevent some bugs that could show up if they only move a subset of the calls to the newer api.

[kevpar]

@kevpar This PR adds the ability to use v0 or v1 within the same executable for ncproxy. This was more of an implementation decision that I made. Since the code works by converting the v0 api to the v1 api and back for the response, if both apis are being used simultaneously, it's possible there are "ncproxy networks" or "ncproxy endpoints" that were created from a call to the v1 apis. In that scenario, a v0 agent making a call to something like "GetNetworks" would get an error since there are "ncproxy networks" that ncproxy is tracking which cannot be encoded in the response to the v0 "GetNetworks" api. We could instead just ignore "ncproxy networks" or "ncproxy endpoints" and add a log on our side, but I was concerned about doing that since we wouldn't be giving the node network agent the true picture of what networks or endpoints have been created through calls to ncproxy.

So basically the expectation is that if node network agent wants to use the v1 api, it must fully move all calls to ncproxy over to the v1 api, not just a subset. This will hopefully additionally prevent some bugs that could show up if they only move a subset of the calls to the newer api.

Okay, it's just that callers can't use both the v0 and v1 API at the same time with a single ncproxy instance? The phrasing made it sound like we can't host both APIs at the same time.

Edit: I think it would probably be better if we can just show a limited view in the v0 API, rather than returning an error. For instance, just filter out the "ncproxy networks" in v0 GetNetworks call. This allows the v0 API to continue working, and a v0 caller doesn't need to know about "ncproxy networks", since it was never designed to care about them.

[katiewasnothere]

Okay, it's just that callers can't use both the v0 and v1 API at the same time with a single ncproxy instance? The phrasing made it sound like we can't host both APIs at the same time.

Edit: I think it would probably be better if we can just show a limited view in the v0 API, rather than returning an error. For instance, just filter out the "ncproxy networks" in v0 GetNetworks call. This allows the v0 API to continue working, and a v0 caller doesn't need to know about "ncproxy networks", since it was never designed to care about them.

@kevpar Sure, if that's what's preferred, I'll make those changes now.

[katiewasnothere]

@kevpar code now allows for running both v0 and v1 api simultaneously. IOW, no erroring if "ncproxy networks" or "ncproxy endpoints" are present.

[kevpar]

LGTM

[katiewasnothere]

Fixed a linter error in the naming of the new error variables I made.

[helsaawy]

what was the decision on log warnings in all the v0 calls that the API is marked for deprecation and users should transition to v1?

[katiewasnothere]

what was the decision on log warnings in all the v0 calls that the API is marked for deprecation and users should transition to v1?

I added a warning log in the server setup to indicate that the v0 api is deprecated. Unfortunately, as far as I could find, there isn't a nice way to indicate that a field or api call is deprecated without adding deprecated tags to the proto files and having the node network agent take that updated proto file for v0 api. We could add a log statement to all the functions for v0, but that wouldn't communicate to the node network agent that the field is deprecated, that would just add a log in our own ncproxy logs, which I'm not sure would be useful.

[katiewasnothere]

@helsaawy and @kevpar I added an additional option in the v0 protobuf file to indicate that it's deprecated. Based on the language spec https://protobuf.dev/programming-guides/proto3/, this typically just produces a warning when the file is compiled, so should be fine to add.

[kevpar]

@helsaawy and @kevpar I added an additional option in the v0 protobuf file to indicate that it's deprecated. Based on the language spec https://protobuf.dev/programming-guides/proto3/, this typically just produces a warning when the file is compiled, so should be fine to add.

The protobuf docs seem to indicate that this option is only valid at field scope, rather than file. However, both protoc-gen-gogo and protoc-gen-go seem to generate a comment based on it being set at file-scope, so I guess it's fine?

[katiewasnothere]

The protobuf docs seem to indicate that this option is only valid at field scope, rather than file. However, both protoc-gen-gogo and protoc-gen-go seem to generate a comment based on it being set at file-scope, so I guess it's fine?

Yeah, I saw a reference to using the option at a file level here. I chose to do this to avoid needing to add the option to every type in the proto, but I'm fine adding that if that's the level of verbosity we'd prefer here.

[kevpar]

The protobuf docs seem to indicate that this option is only valid at field scope, rather than file. However, both protoc-gen-gogo and protoc-gen-go seem to generate a comment based on it being set at file-scope, so I guess it's fine?

Yeah, I saw a reference to using the option at a file level here. I chose to do this to avoid needing to add the option to every type in the proto, but I'm fine adding that if that's the level of verbosity we'd prefer here.

The current approach is fine with me.