Security group bind #4098
Security group bind #4098
Conversation
| metadata: | ||
| annotations: | ||
| controller-gen.kubebuilder.io/version: v0.18.0 | ||
| controller-gen.kubebuilder.io/version: v0.16.4 |
There was a problem hiding this comment.
It looks like your controller-gen is a bit outdated and that causes those unintentional diffs.
| ))) | ||
| }) | ||
|
|
||
| When("the security group is not found", func() { |
There was a problem hiding this comment.
According to the remark above, the handler should have a special handling for the forbidden error only. Therefore it makes sense to have a context when the repo returns a forbidden error, and another one when it returns any other generic error. A context for the not found error is a subset of the generic error handling, therefore it could be omitted.
| }) | ||
| }) | ||
|
|
||
| When("the security group does not exist", func() { |
There was a problem hiding this comment.
Same remark on ForbiddenAsNotFound error handling is applicable here as well.
| return ctrl.Result{}, fmt.Errorf("failed to list NetworkPolicies: %w", err) | ||
| } | ||
|
|
||
| for _, policy := range policies.Items { |
There was a problem hiding this comment.
Deleting related resources in kubernetes is generally asynchronous. When deleting them as part of the reconciliation loop is important, we have invented the pattern to requeue the reconcile event until the controller makes sure there are no more related resources and then remove the finalizer. Here is an example
This is a bit tricky, feel free to call to discuss that.
| } | ||
|
|
||
| func (r *Reconciler) reconcileNetworkPolicyForSpace(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup, space string, workloads korifiv1alpha1.SecurityGroupWorkloads) error { | ||
| policy, err := r.calicoClient.ProjectcalicoV3().NetworkPolicies(space).Get(ctx, fmt.Sprintf("default.%s", sg.Name), metav1.GetOptions{}) |
There was a problem hiding this comment.
If calico policies implement the client.Object interface and can be manipulated by the standard k8s client, you could use controllerutil.CreateOrPatch instead of implementing the if-else slalom yourself
There was a problem hiding this comment.
This is another example of a potential benefit of moving from clientset to client.Client
| } | ||
|
|
||
| func generateCIDRs(startIP, endIP string) ([]string, error) { | ||
| start, err := ipToUint32(startIP) |
There was a problem hiding this comment.
Would it make sense to move parsing and validating various ports/ips,etc into a validating webhook thus simplifying the controller code.
georgethebeatle
left a comment
georgethebeatle
left a comment
There was a problem hiding this comment.
Posting part II of the review, didn't manage to reach the end, but will continue next week
|
|
||
| type Reconciler struct { | ||
| k8sClient client.Client | ||
| calicoClient clientset.Interface |
There was a problem hiding this comment.
We could simply use client.Client here to be more idiomatic with the rest of the korifi codebase and to be able to reuse existing tooling around client.Client. This way we will also decrease the number of moving parts of this controller (only one k8s client).
Doing that will enable us to get rid of the fake calico client and test this controller using envtest, which is the preferred way of doing things in Korifi. You can see an example of how this is done here. Note how we install korifi's crds in the envtest apiserver. Once we do that we have a lightweight kubernetes api to run our controller against. But in this case we need to import calico's crds. Calico is mostly using the old fashioned extension server approach, but it looks like they publish some crd definitions as well, which should be enough to unblock the envtest approach.
If all this is new to you it might be best to pair with one of us to get you started.
| func (r *Reconciler) finalizeCFSecurityGroup(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup) (ctrl.Result, error) { | ||
| logs := logr.FromContextOrDiscard(ctx).WithName("finalize-security-group") | ||
|
|
||
| policies, err := r.calicoClient.ProjectcalicoV3().NetworkPolicies("").List(ctx, metav1.ListOptions{ |
There was a problem hiding this comment.
Again on the client.Client vs clientset topic. I managed to compile the client.Client alternative for this list and here is how it looks:
policies := &v3.NetworkPolicyList{}
err := r.k8sClient.List(ctx, policies)
if err != nil {
return ctrl.Result{}, fmt.Errorf("failed to list NetworkPolicies: %w", err)
}
This is just an illustraton on how the calicoclient is redundant.
| } | ||
|
|
||
| func (r *Reconciler) reconcileNetworkPolicyForSpace(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup, space string, workloads korifiv1alpha1.SecurityGroupWorkloads) error { | ||
| policy, err := r.calicoClient.ProjectcalicoV3().NetworkPolicies(space).Get(ctx, fmt.Sprintf("default.%s", sg.Name), metav1.GetOptions{}) |
There was a problem hiding this comment.
This is another example of a potential benefit of moving from clientset to client.Client
| } | ||
|
|
||
| func (r *Reconciler) reconcileGlobalNetworkPolicies(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup) error { | ||
| policy, err := r.calicoClient.ProjectcalicoV3().GlobalNetworkPolicies().Get(ctx, fmt.Sprintf("default.%s", sg.Name), metav1.GetOptions{}) |
There was a problem hiding this comment.
Similarly to reconcileNetworkPolicyForSpace we could simplify this code by switching to client.Client
| func (r *Reconciler) createNetworkPolicy(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup, space string, workloads korifiv1alpha1.SecurityGroupWorkloads) error { | ||
| policy := &v3.NetworkPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: sg.Name, |
There was a problem hiding this comment.
We create the networkpolicy with the same name as the cfsecuritygroup, but we are listing them with a default. prefix. We should be consistent in our naming scheme.
| func (r *Reconciler) createGlobalNetworkPolicy(ctx context.Context, sg *korifiv1alpha1.CFSecurityGroup) error { | ||
| policy := &v3.GlobalNetworkPolicy{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: sg.Name, |
There was a problem hiding this comment.
We create the globalnetworkpolicy with the same name as the cfsecuritygroup, but we are listing them with a default. prefix. We should be consistent in our naming scheme.
| "k8s.io/client-go/util/flowcontrol" | ||
| ) | ||
|
|
||
| type FakeCalicoClient struct { |
There was a problem hiding this comment.
This is not a generated fake, but one that we have to write and support ourselves. There is not official fake. This is one more reason to move to envtest flavoured testing, where we can spin up a tiny kubeapi server and etcd locally and simply target them with a real client.Client
In fact the security groups controller test suite already starts the testenv, so we end up in a pretty weird state testing half of the controller with envtest and the other half with a fake client.
4 participants
Is there a related GitHub Issue?
Security Group Implementation
What is this change about?
Implementing ASGs (Security Groups)
Does this PR introduce a breaking change?
Yes, it requires Calico to be installed.
Acceptance Steps
Should all be part of the E2E Test Scripts
Tag your pair, your PM, and/or team
@danail-branekov, @georgethebeatle