Controller Proof of Concept

[mheese]

Not really meant to be merged, however, I really need some feedback on what is there so far.

I really would like to continue to work on this, but I don't really want to do so without any feedback on what's there so far.

@maugustosilva are you or some others able to do that?

[mpeters]

I agree that long-term it should be a go-based operator, but doing the initial prototype in helm is useful to make sure all of the pieces fit together the way we want them to.

[mheese]

@sarroutbi @mpeters what am I missing? This is a go controller. Only its installation is done with helm so that it fits with the rest of the installation.

[maugustosilva]

@mheese my apologies for the time it is taken for me to evaluate it. It should be done in the next weeks.

[mheese]

@mheese my apologies for the time it is taken for me to evaluate it. It should be done in the next weeks.

@maugustosilva I have to work on other things at the moment (for the next ~3 weeks I guess), so this is luckily not blocking me at the moment. That said, as I mentioned, I'd really like for someone to look over this (particularly with the direction that this is going) before I start adding additional things like mapping the policy model.

[sarroutbi]

@sarroutbi @mpeters what am I missing? This is a go controller. Only its installation is done with helm so that it fits with the rest of the installation.

@mheese : sorry if I did not express myself correctly. I meant I am very interested in this PR to be merged, as I believe it is the way to go.

[sarroutbi]

I would rename this variable to something like K8S_CLIENT, just in case other command (such as oc) wants to be used

[sarroutbi]

If possible, we could use golang:1.21

[sarroutbi]

this seems like bad indented. If it is not auto generated, please, fix it

[sarroutbi]

We might use go 1.21 version

[sarroutbi]

Change LGTM. I added minor comments

[mheese]

Change LGTM. I added minor comments

@sarroutbi thanks for the review! 👍 ... just fyi, that I'm also waiting for @maugustosilva and @galmasi to review this. This is supposed to be a proof of concept PR only. And it's at a state where we need to discuss UX of what's there right now, and also the direction on where we want to go next.

I will definitely incorporate your changes. But in general I will also restructure the code, go over code comments for documentation particularly, most likely change the data structures quite a bit (at the moment it essentially duplicates what's in the registrar and verifier which it shouldn't), and also write unit tests and integration tests against kubernetes before we can merge this.

[maugustosilva]

@mheese As promised, with a delay of four months, I finally had time to sit and evaluate the PR.

1. In general, I consider it essentially on the right direction, and, from your demo, we have evidence it is (already) functional.
2. The agent-crd seems to cover most cases we have discussed, and maps the relevant parameters of the configuration file (and database schema).
3. It is not clear to me if this controller would properly support agents running outside of the Kubernetes/Openshift cluster (maybe the node attribute could be arbitrary), but even if it does not, I cannot see any fundamental impediment to it in the future with this code.
4. It is also not clear how will the controller spread multiple agents across multiple verifiers , but this is an open question that I would like to address on the tenant code anyway (probably something like a consistent hashing using the UUID of the agent).
5. I am not sure if the proposal in docs/push-model-proxy.md is really doable, but between this new effort on a push model, the requirement of deploying agents outside the cluster, and the use of some sort of load balancer/routes for both multiple verifiers and agents, I believe we are covering a large majority of cases already.
6. The "registrar synchronizer" seems to reimplement portions of tenant in go, which is, I believe, a requirement, anyway. I wonder how much of it could be unfolded into a new standalone CLI, eventually.
7. This, being an initial implementation, of course does not cover the thorny issue of the other object which will require status tracking: policies. I do not believe we should (or even could) fully solve this problem before starting to implement the controller (as it will be a truly interactive process).
8. That being said, I propose we use your initial implementation as the starting point, and refine/expand on it. @sarroutbi is by far the more knowledgeable to comment on the Kubernetes/Openshift-specific aspects (such as the modeling via CRDs), but from my standpoint (Keylime) it looks like a very solid foundation.

PS: I agree with changes requested by @sarroutbi btw.

[sarroutbi]

Hello. Apart from the conflicts, it seems no operator-sdk has been used to implement this Proof of Concept. I am writing this essentially because I see no "bundle" folder has been generated in this PR.

@mheese :
1 - Are you planning to merge this PR by solving the pending conflicts?
2 - Is it expected that this can be deployed through operator-sdk?

[mheese]

@maugustosilva and @sarroutbi thanks so much for the review! 🙏

I've been on a longer break in December, so let me look at this again in the upcoming days, and ...

• obviously fix all the merge conflicts
• reply to all your comments/questions above @maugustosilva
• make a list of short term TODOs based on the previous point
• propose next steps for this
• @sarroutbi re "Operator SDK": no, this PR was using "kubebuilder" not the operator SDK for creation of the boilerplate. AFAIK, both rely on the same underlying golang library (controller runtime) for the actual Kubernetes logic implementation. However, they both diverge in bundling/building the application. I assume your interest for this obviously is around all the OpenShift integration benefits that come with it? like OpenShift marketplace etc.pp.

[sarroutbi]

Hello @mheese : thanks for your quick response. I understand now. Sorry, I was not aware of this option to build controllers. As you correctly indicated, using OLM/Operator-sdk based deployment is helpful to include the operator in OpenShift catalog, but we can postpone this (or even decide if this can be downstream only content).

[maugustosilva]

@mheese thank you. I really believe this is a very good starting point for the building of an actual operator, and we should incorporate it in to the main code as soon as possible. I am available to test it.

[mheese]

@maugustosilva @galmasi this is ready. As talked about to George on slack, I can't merge this as squash&merge isn't activated for this repo (which it should imho). If this is a problem, I'll create a new PR for it. I won't force-push a rebase because that's just an awful thing to do.

[mpeters]

I can't merge this as squash&merge isn't activated for this repo

@mheese This should be fixed now, can you try again?

[mheese]

@galmasi @maugustosilva @mpeters I had one more merge conflict with the Makefile which should be fixed now. I also changed in the values.yaml that the controller is not being deployed by default. Also George mentioned that the deployment doesn't work at the moment, and looking over it, it must be because of a lot of renames of variables in the values yaml file in the meantime which don't match anymore (ca vs cvca for example).

Also, there are the following helm charts now:

• keylime-crds - must be managed separately from any other installation, bad consequences can happen if one doesn't do it this way (there are pros/cons managing CRDs within the same chart, one usually doesn't want them in the same chart)
• keylime-controller - this one is bogus as far as I recall (mea culpa, it's been a few months since I looked at this last), and should not be used. It can probably be removed, but I have a vague memory that I kept it for a reason.
• keylime - this is still the same main keylime chart, and also contains a subchart now for the controller

I also don't have a test setup for this anymore at the moment. So I actually cannot test the installation anymore at the moment. I'm going to merge this anyway now, and we'll have to resolve potential problems that I caused with this. I'm already going to apologize in advance 🙏