Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust API groups for Controller API and CRDs #1122

Closed
jianjuns opened this issue Aug 20, 2020 · 15 comments · Fixed by #1147
Closed

Adjust API groups for Controller API and CRDs #1122

jianjuns opened this issue Aug 20, 2020 · 15 comments · Fixed by #1147
Labels
area/api Issues or PRs related to an API. kind/design Categorizes issue or PR as related to design. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@jianjuns
Copy link
Contributor

jianjuns commented Aug 20, 2020

Describe what you are trying to solve
Need to finalize the API group definition for Controller API and CRDs. This is also required to support new features like IPAM, egress policy, NodePortLocal.

Describe the solution you have in mind
Per discussions offline, the proposal supported by most people is:

  • “core”: common types, ExternalEntity/Node, grouping
  • “internal”: internal messages and API between components (mainly for Controller - Agent)
  • “security”: Antrea NetworkPolicy CRDs, and potentially other network security features
  • “networking”: IPAM, SNAT, NodePortLocal, and other L2-L4 networking stuff
  • “ops”: Traceflow CRD
  • "clusterinformation": Controller and Agent monitoring CRD
  • “system”: support bundle, system/component information (now mostly used by antctl).
  • “metrics”: metrics

The major change needed is to change the current "networking" group to "internal", and then some new features can start to use the "networking" group.

@jianjuns jianjuns added kind/design Categorizes issue or PR as related to design. area/api Issues or PRs related to an API. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Aug 20, 2020
@jianjuns
Copy link
Contributor Author

We cannot use "internal" as packages under "internal" directory will be treated as internal packages in Golang.
A few alternative names have been proposed are:

  • impl
  • implementation
  • controlplane
  • cp
  • ctlplaneinternal
  • backend
  • local
  • private

@abhiraut
Copy link
Contributor

+1 to controlplane

@edwardbadboy
Copy link
Contributor

controlplane sounds good.

@jianjuns
Copy link
Contributor Author

jianjuns commented Aug 25, 2020

One problem @tnqn mentioned in the community meeting - we need to use different API groups for CRDs and Controller/Agent API even they are for the same kind of information. For example, we have "clusterinformation" for monitoring CRD; "system" for system information from Controller API which includes the same information exposed via monitoring CRD; and "ops" for Traceflow CRD (while support bundle API is in the "system" group).
In this way we might end up more with groups.

@antoninbas
Copy link
Contributor

I think the issue is that we cannot use the same API group for both CRDs and an APIService object? Because the k8s apiserver uses the API group to decide which requests to forward through the aggregation layer?

That's funny because @tnqn and I were having a conversation in another issue about whether we actually needed APIServices... @tnqn, do you remember why we need an APIService for system.antrea.tanzu.vmware.com? I don't remember what's consuming this information? I guess it could be antctl, but antctl has access to the CRDs. I'm asking because if we don't have that APIService, we could stick to the original plan, since the only other APIService is for the "internal" API, and we were planning to reserve a group ("controlplane" or other name) for this anyway.

@jianjuns
Copy link
Contributor Author

antctl uses "system" group, not monitoring CRDs (one reason is we wanted to support local debugging even when K8s API is down, but probably it is not really implemented for controller API), and "system" group also includes support bundle.

@antoninbas
Copy link
Contributor

antctl uses "system" group, not monitoring CRDs (one reason is we wanted to support local debugging even when K8s API is down, but probably it is not really implemented for controller API), and "system" group also includes support bundle.

Yes but it doesn't seem to me that either of these use cases require an APIService. I'm pretty sure that "antctl supportbundle" connects directly to the Agent / Controller, without going through the k8s apiserver. So I wanted to confirm that the issue @tnqn brought up comes from the fact that we cannot use the same group for CRD + APIService.

@jianjuns
Copy link
Contributor Author

APIService just enables you to run antctl outside the K8s cluster and reaches Controller via K8s API. I know we do not do that for supportbundle, but it is the case for Controller information.

@antoninbas
Copy link
Contributor

I am aware of that, but it doesn't seem that useful here. We can use CRDs for out-of-cluster and still have a separate implementation for the local use case that you mentioned above. I think it's actually better than exposing the same info through 2 different mechanisms, which is what we seem to be doing today. So my point is that removing the system APIService may be a viable solution.

@jianjuns
Copy link
Contributor Author

Ok. I now got your point.
I think the question is why we want to treat "system" group differently from other groups. Also even you have "similar" set of information in CRD, do you still want to keep the capability of running antctl remotely for these commands (for example would you document antctl version does not work remotely?).

@antoninbas
Copy link
Contributor

I was suggesting having antctl read the information from the CRDs when running out-of-cluster. But this has diverged into a different discussion (I still think we should consider removing the APIService for system...). Regarding the original topic, I would still prefer to avoid "bundling" all the CRDs into one API group, even though I am aware that there are other projects which do that, because they are logically unrelated. That being said, if we cannot come up with a small subset of API groups that make sense and that we think is future-proof "enough", then we may reconsider. Changing API groups in the future is just going to make upgrades very painful.

@jianjuns
Copy link
Contributor Author

We can talk about "system" group separately (still not convinced we must treat it differently).

I personally want not to use a single group for CRD either. But on the other hand, I do not like so many groups, and would like to differentiate CRDs and other APIs from group names, but I do not have a good idea to achieve this.

@tnqn
Copy link
Member

tnqn commented Aug 25, 2020

controlplane sounds good to me as well.

Would it make sense to get rid of all APIServices and using NodePort service or exec endpoint to access Antrea APIs from out of cluster as discussed here #1137 (comment), given that we will need a way to access non registered API like #1137 anyway?
Then maybe we don't need to worry the routing problem of using a group for both CRD and APIService.

@jianjuns
Copy link
Contributor Author

NodePort is not convenient and in many cases not really exposed to external network. Feel Pod exec is indirect too. So, I am not convinced to remove API Service just to reduce API groups. Even now we do not have many cases yet, I would not exclude the possibility to expose public API from Antrea directly, and would leverage the power K8s apiserver provides.

@jianjuns
Copy link
Contributor Author

Anyway, from all discussion, I think at least we can start to move "networking" group to "controlplane". I will start to make the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Issues or PRs related to an API. kind/design Categorizes issue or PR as related to design. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants