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

Add support for Linkerd to the intents-operator #324

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

aerosouund
Copy link
Contributor

Description

Create required linkerd resources (servers, httproutes, authpolicies, mtls authentications, network authentications) based on otterize client intents.
An extra optional field has been added in the intent definition which is a port field.
It extends the same pattern used in istio policies but applies it to each respective linkerd resource

References

Testing

There is a built image in a public repo that you can deploy in a cluster with linkerd installed: aerosouund/intents-operator:0.1.34

Also include details of the environment this PR was developed in (language/platform/browser version):

  • golang 1.21

  • kubernetes 1.23 on EKS

  • Linkerd 2.x

  • This change adds test coverage for new/changed/fixed functionality

Checklist

  • I have added documentation for new/changed functionality in this PR and in github.com/otterize/docs

Copy link

github-actions bot commented Dec 30, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@aerosouund
Copy link
Contributor Author

Over the upcoming period there's still some extra small functionality i want to add to the PR, but i've created it now to work on any large changes required at the same time

Including the following:

  • don’t add the path in the name for the policies but validate if a policy should be created based on its targets, and add a random hash in the end of all policy names


  • cleaner implementation for deletion, instead of iterating over each type

  • move the creation of primary resources to the start of the loop instead of checking if it should be done for each intent

  • add support for grpc and tcp probes if they also break when a http route is created

  • create a generateHTTPRouteName method

@orishoshan orishoshan self-requested a review January 2, 2024 20:00
}

pod, err := r.serviceIdResolver.ResolveClientIntentToPod(ctx, *intents)
logrus.Infof("Got pod with name %s", pod.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this log after the err check, as pod may be nil

logrus.Infof("Reconciling Linkerd authorization policies for service %s in namespace %s",
intents.Spec.Service.Name, req.Namespace)

// TODO: implement delete all
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this TODO still relevant?

}

clientServiceAccountName := pod.Spec.ServiceAccountName
missingSideCar := !linkerdmanager.IsPodPartOfLinkerdMesh(pod)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to check using a label or annotation instead?
Using the proxy's name is implementation-specific and may break when Linkerd versions are updated. Is there a documented label or something similar we can check for instead?

return ctrl.Result{}, nil
}

err = r.linkerdManager.Create(ctx, intents, clientServiceAccountName) // the injectable recorder is null pointer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment still relevant?

EnableDatabasePolicy = "enable-database-policy-creation" // Whether to enable the new database reconciler
EnableDatabasePolicyDefault = true
EnableDatabaseReconciler = "enable-database-reconciler" // Whether to enable the new database reconciler
EnableDatabaseReconcilerDefault = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change this back :)

@@ -0,0 +1,285 @@
apiVersion: apiextensions.k8s.io/v1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these generated .patched files from Git

@@ -155,6 +157,9 @@ type Intent struct {
//+optional
Type IntentType `json:"type,omitempty" yaml:"type,omitempty"`

//+optional
Port int32 `json:"port,omitempty" yaml:"port,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need to figure out a way to do without specifying the port manually - let's try to figure this out when we talk

func (r *LinkerdReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
logrus.Info("Starting linkerd reconcile logic")

installed, err := linkerdmanager.IsLinkerdServerInstalled(ctx, r.Client)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
installed, err := linkerdmanager.IsLinkerdServerInstalled(ctx, r.Client)
installed, err := linkerdmanager.IsLinkerdInstalled(ctx, r.Client)

This checks if the Linkerd Server CRD is installed, but in fact it's checking if Linkerd itself is installed in the cluster through this check.

if intent.Type != "" && intent.Type != otterizev1alpha3.IntentTypeHTTP && intent.Port != 0 { // this will skip non http ones, db for example, skip port doesnt exist as well
continue
}
shouldCreatePolicy, err := protected_services.IsServerEnforcementEnabledDueToProtectionOrDefaultState( //TODO: check what that does
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

for _, intent := range clientIntents.GetCallsList() {
if intent.Type != "" && intent.Type != otterizev1alpha3.IntentTypeHTTP && intent.Port != 0 { // this will skip non http ones, db for example, skip port doesnt exist as well
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The port condition seems inverted, as in if port is NOT empty AND type is not HTTP it will continue. But perhaps you meant

if intent.Type != "" // if intent type is not empty
&& intent.Type != IntentTypeHTTP // and intent type is not http
|| intent.Port == "" // OR port is empty

@aquam8
Copy link

aquam8 commented Jun 12, 2024

we'd be very interested in having support for Linkerd2 - then Otterize would really be a compelling solution for us as light user of EKS & Linkerd2 (mTLS, grpc load balancing but not leveraging authorization policies in linkerd). We use kubernetes network policies but we do not like it for the same reason you highlighted in your blog post. We're making a case towards dropping support for network policies all together (we are using Calico & aws vpc-cni) and we would be interesting in assessing how linkerd2/otterize could help with authorization policies on that side of the fence instead.

@orishoshan
Copy link
Collaborator

orishoshan commented Jun 12, 2024

That's great to hear @aquam8! We've been working on a couple of infrastructure changes that are starting to come in and have blocked this PR from being merged, namely #409 and #439, so Linkerd support should be getting merged soon! Stay tuned :)

Would love to have you join us on Slack to hear more about your use case if possible: https://joinslack.otterize.com

@aquam8
Copy link

aquam8 commented Aug 12, 2024

Could this work be revised now that the dependent PRs have been merged, thank you!

@aerosouund
Copy link
Contributor Author

@evyatarmeged @orishoshan
PR rebased and cleaned, please review and let me know

This package contains an engine that creates, updates, deletes linkerd resources
from ClientIntents.
With a test suite for that feature based on mock.
The package will create a Server, AuthPolicy and MTLSAuth for a non HTTP Intent,
or a Server ,HTTPRoute , AuthPolicy and MTLSAuth for a HTTP Intent.
As well as a NetAuth for probe routes to not cause probes to break from the creation of servers on them.

Signed-off-by: aerosouund <[email protected]>
Create Linkerd mock to be used in testing

Signed-off-by: aerosouund <[email protected]>
Leverage the linkerd engine in a reconciler that watches ClientIntents

Signed-off-by: aerosouund <[email protected]>
Register the linkerd reconciler with the intents reconciler

Signed-off-by: aerosouund <[email protected]>
Define constants used by the linkerd manager

Signed-off-by: aerosouund <[email protected]>
Signed-off-by: aerosouund <[email protected]>
This is needed as pod ports are required for Linkerd Server creation and they might change
if the pod definition changes.

Signed-off-by: aerosouund <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Linkerd authorization policies
3 participants