Skip to content

Commit

Permalink
Fix case where Istio policies were created with the wrong label in th…
Browse files Browse the repository at this point in the history 
…eir pod selectors (#373)
  • Loading branch information
@omris94
omris94 authored Mar 6, 2024
1 parent af2b447 commit b3338f3
Show file tree
Hide file tree
Showing 8 changed files with 137 additions and 139 deletions.
2 changes: 1 addition & 1 deletion src/operator/api/v1alpha2/intents_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ import (
"encoding/json"
"fmt"
"github.com/otterize/intents-operator/src/shared/errors"
"k8s.io/apimachinery/pkg/labels"
"strconv"
"strings"

"github.com/otterize/intents-operator/src/shared/otterizecloud/graphqlclient"
"github.com/samber/lo"
"github.com/sirupsen/logrus"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/util/sets"
)

Expand Down
28 changes: 14 additions & 14 deletions src/operator/controllers/external_traffic/network_policy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package external_traffic

import (
"context"
otterizev1alpha2 "github.com/otterize/intents-operator/src/operator/api/v1alpha2"
otterizev1alpha3 "github.com/otterize/intents-operator/src/operator/api/v1alpha3"
"github.com/otterize/intents-operator/src/shared/operatorconfig/allowexternaltraffic"
"github.com/otterize/intents-operator/src/shared/testbase"
"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -34,14 +34,14 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
Name: "coolPolicy",
Namespace: serviceNamespace,
Labels: map[string]string{
otterizev1alpha2.OtterizeNetworkPolicy: serviceName,
otterizev1alpha3.OtterizeNetworkPolicy: serviceName,
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
otterizev1alpha2.OtterizeServerLabelKey: serviceName,
otterizev1alpha3.OtterizeServiceLabelKey: serviceName,
},
},
},
Expand All @@ -58,14 +58,14 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
Name: "coolPolicy",
Namespace: serviceNamespace,
Labels: map[string]string{
otterizev1alpha2.OtterizeNetworkPolicy: serviceName,
otterizev1alpha3.OtterizeNetworkPolicy: serviceName,
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
otterizev1alpha2.OtterizeServerLabelKey: serviceName,
otterizev1alpha3.OtterizeServiceLabelKey: serviceName,
},
},
},
Expand All @@ -76,21 +76,21 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
Name: "externalPolicy",
Namespace: serviceNamespace,
Labels: map[string]string{
otterizev1alpha2.OtterizeNetworkPolicyExternalTraffic: serviceName,
otterizev1alpha3.OtterizeNetworkPolicyExternalTraffic: serviceName,
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
otterizev1alpha2.OtterizeServerLabelKey: serviceName,
otterizev1alpha3.OtterizeServiceLabelKey: serviceName,
},
},
},
}

firstList := s.Client.EXPECT().List(
gomock.Any(), gomock.Eq(&v1.NetworkPolicyList{}), client.MatchingLabels{otterizev1alpha2.OtterizeNetworkPolicy: serviceName}, &client.ListOptions{Namespace: serviceNamespace},
gomock.Any(), gomock.Eq(&v1.NetworkPolicyList{}), client.MatchingLabels{otterizev1alpha3.OtterizeNetworkPolicy: serviceName}, &client.ListOptions{Namespace: serviceNamespace},
).DoAndReturn(
func(_ any, list *v1.NetworkPolicyList, _ ...any) error {
list.Items = []v1.NetworkPolicy{*toBeRemovedPolicy}
Expand All @@ -99,7 +99,7 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
)
secondList := s.Client.EXPECT().List(
gomock.Any(), gomock.Eq(&v1.NetworkPolicyList{}),
client.MatchingLabels{otterizev1alpha2.OtterizeNetworkPolicyExternalTraffic: serviceName},
client.MatchingLabels{otterizev1alpha3.OtterizeNetworkPolicyExternalTraffic: serviceName},
&client.ListOptions{Namespace: serviceNamespace},
).DoAndReturn(
func(_ any, list *v1.NetworkPolicyList, _ ...any) error {
Expand All @@ -122,14 +122,14 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
Name: "coolPolicy",
Namespace: serviceNamespace,
Labels: map[string]string{
otterizev1alpha2.OtterizeNetworkPolicy: serviceName,
otterizev1alpha3.OtterizeNetworkPolicy: serviceName,
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
otterizev1alpha2.OtterizeServerLabelKey: serviceName,
otterizev1alpha3.OtterizeServiceLabelKey: serviceName,
},
},
},
Expand All @@ -140,21 +140,21 @@ func (s *NetworkPolicyHandlerTestSuite) TestNetworkPolicyHandler_HandleBeforeAcc
Name: "coolPolicy",
Namespace: serviceNamespace,
Labels: map[string]string{
otterizev1alpha2.OtterizeNetworkPolicy: serviceName,
otterizev1alpha3.OtterizeNetworkPolicy: serviceName,
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchLabels: map[string]string{
otterizev1alpha2.OtterizeServerLabelKey: serviceName,
otterizev1alpha3.OtterizeServiceLabelKey: serviceName,
},
},
},
}

s.Client.EXPECT().List(
gomock.Any(), gomock.Eq(&v1.NetworkPolicyList{}), client.MatchingLabels{otterizev1alpha2.OtterizeNetworkPolicy: serviceName}, &client.ListOptions{Namespace: serviceNamespace},
gomock.Any(), gomock.Eq(&v1.NetworkPolicyList{}), client.MatchingLabels{otterizev1alpha3.OtterizeNetworkPolicy: serviceName}, &client.ListOptions{Namespace: serviceNamespace},
).DoAndReturn(
func(_ any, list *v1.NetworkPolicyList, _ ...any) error {
// two policies
Expand Down
8 changes: 4 additions & 4 deletions src/operator/controllers/external_traffic/network_policy_uploader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package external_traffic
import (
"context"
"fmt"
"github.com/otterize/intents-operator/src/operator/api/v1alpha2"
"github.com/otterize/intents-operator/src/operator/api/v1alpha3"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/otterize/intents-operator/src/shared/injectablerecorder"
"github.com/otterize/intents-operator/src/shared/operator_cloud_client"
Expand Down Expand Up @@ -131,13 +131,13 @@ func filterOtterizeNetworkPolicy() predicate.Predicate {
return predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
labels := e.Object.GetLabels()
_, isExternalTrafficPolicy := labels[v1alpha2.OtterizeNetworkPolicyExternalTraffic]
_, isExternalTrafficPolicy := labels[v1alpha3.OtterizeNetworkPolicyExternalTraffic]

return isExternalTrafficPolicy
},
UpdateFunc: func(e event.UpdateEvent) bool {
labels := e.ObjectNew.GetLabels()
_, isExternalTrafficPolicy := labels[v1alpha2.OtterizeNetworkPolicyExternalTraffic]
_, isExternalTrafficPolicy := labels[v1alpha3.OtterizeNetworkPolicyExternalTraffic]

return isExternalTrafficPolicy
},
Expand All @@ -147,7 +147,7 @@ func filterOtterizeNetworkPolicy() predicate.Predicate {
}

labels := e.Object.GetLabels()
_, isExternalTrafficPolicy := labels[v1alpha2.OtterizeNetworkPolicyExternalTraffic]
_, isExternalTrafficPolicy := labels[v1alpha3.OtterizeNetworkPolicyExternalTraffic]

return isExternalTrafficPolicy
},
Expand Down
10 changes: 5 additions & 5 deletions src/operator/controllers/external_traffic/network_policy_uploader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package external_traffic

import (
"context"
"github.com/otterize/intents-operator/src/operator/api/v1alpha2"
"github.com/otterize/intents-operator/src/operator/api/v1alpha3"
"github.com/otterize/intents-operator/src/shared/otterizecloud/graphqlclient"
otterizecloudmocks "github.com/otterize/intents-operator/src/shared/otterizecloud/mocks"
"github.com/otterize/intents-operator/src/shared/testbase"
Expand Down Expand Up @@ -54,15 +54,15 @@ func (s *NetworkPolicyReconcilerTestSuite) TestUploadNetworkPolicy() {
Name: "external-access-to-client-A",
Namespace: testNamespace,
Labels: map[string]string{
v1alpha2.OtterizeNetworkPolicyExternalTraffic: "client-A",
v1alpha3.OtterizeNetworkPolicyExternalTraffic: "client-A",
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: v1alpha2.OtterizeServerLabelKey,
Key: v1alpha3.OtterizeServiceLabelKey,
Operator: metav1.LabelSelectorOpExists,
},
},
Expand Down Expand Up @@ -130,15 +130,15 @@ func (s *NetworkPolicyReconcilerTestSuite) TestNoUploadIfNoPods() {
Name: "external-access-to-client-A",
Namespace: testNamespace,
Labels: map[string]string{
v1alpha2.OtterizeNetworkPolicyExternalTraffic: "client-A",
v1alpha3.OtterizeNetworkPolicyExternalTraffic: "client-A",
},
},
Spec: v1.NetworkPolicySpec{
PolicyTypes: []v1.PolicyType{v1.PolicyTypeIngress},
PodSelector: metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: v1alpha2.OtterizeServerLabelKey,
Key: v1alpha3.OtterizeServiceLabelKey,
Operator: metav1.LabelSelectorOpExists,
},
},
Expand Down
41 changes: 20 additions & 21 deletions src/operator/controllers/istiopolicy/policy_manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"encoding/json"
"fmt"
"github.com/amit7itz/goset"
"github.com/otterize/intents-operator/src/operator/api/v1alpha2"
"github.com/otterize/intents-operator/src/operator/api/v1alpha3"
"github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/consts"
"github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/protected_services"
Expand Down Expand Up @@ -71,12 +70,12 @@ func (c *PolicyManagerImpl) DeleteAll(
ctx context.Context,
clientIntents *v1alpha3.ClientIntents,
) error {
clientFormattedIdentity := v1alpha2.GetFormattedOtterizeIdentity(clientIntents.Spec.Service.Name, clientIntents.Namespace)
clientFormattedIdentity := v1alpha3.GetFormattedOtterizeIdentity(clientIntents.Spec.Service.Name, clientIntents.Namespace)

var existingPolicies v1beta1.AuthorizationPolicyList
err := c.client.List(ctx,
&existingPolicies,
client.MatchingLabels{v1alpha2.OtterizeIstioClientAnnotationKey: clientFormattedIdentity})
client.MatchingLabels{v1alpha3.OtterizeIstioClientAnnotationKey: clientFormattedIdentity})
if client.IgnoreNotFound(err) != nil {
return errors.Wrap(err)
}
Expand All @@ -95,12 +94,12 @@ func (c *PolicyManagerImpl) Create(
clientIntents *v1alpha3.ClientIntents,
clientServiceAccount string,
) error {
clientFormattedIdentity := v1alpha2.GetFormattedOtterizeIdentity(clientIntents.Spec.Service.Name, clientIntents.Namespace)
clientFormattedIdentity := v1alpha3.GetFormattedOtterizeIdentity(clientIntents.Spec.Service.Name, clientIntents.Namespace)

var existingPolicies v1beta1.AuthorizationPolicyList
err := c.client.List(ctx,
&existingPolicies,
client.MatchingLabels{v1alpha2.OtterizeIstioClientAnnotationKey: clientFormattedIdentity})
client.MatchingLabels{v1alpha3.OtterizeIstioClientAnnotationKey: clientFormattedIdentity})
if err != nil {
c.recorder.RecordWarningEventf(clientIntents, ReasonGettingIstioPolicyFailed, "Could not get Istio policies: %s", err.Error())
return errors.Wrap(err)
Expand Down Expand Up @@ -190,7 +189,7 @@ func (c *PolicyManagerImpl) setServersWithoutSidecar(ctx context.Context, client
updatedIntents.Annotations = make(map[string]string)
}

updatedIntents.Annotations[v1alpha2.OtterizeServersWithoutSidecarAnnotation] = string(serversValues)
updatedIntents.Annotations[v1alpha3.OtterizeServersWithoutSidecarAnnotation] = string(serversValues)
err = c.client.Patch(ctx, updatedIntents, client.MergeFrom(clientIntents))
if err != nil {
return errors.Wrap(err)
Expand All @@ -199,7 +198,7 @@ func (c *PolicyManagerImpl) setServersWithoutSidecar(ctx context.Context, client
}

func (c *PolicyManagerImpl) saveServiceAccountName(ctx context.Context, clientIntents *v1alpha3.ClientIntents, clientServiceAccount string) error {
serviceAccountLabelValue, ok := clientIntents.Annotations[v1alpha2.OtterizeClientServiceAccountAnnotation]
serviceAccountLabelValue, ok := clientIntents.Annotations[v1alpha3.OtterizeClientServiceAccountAnnotation]
if ok && serviceAccountLabelValue == clientServiceAccount {
return nil
}
Expand All @@ -209,7 +208,7 @@ func (c *PolicyManagerImpl) saveServiceAccountName(ctx context.Context, clientIn
updatedIntents.Annotations = make(map[string]string)
}

updatedIntents.Annotations[v1alpha2.OtterizeClientServiceAccountAnnotation] = clientServiceAccount
updatedIntents.Annotations[v1alpha3.OtterizeClientServiceAccountAnnotation] = clientServiceAccount
err := c.client.Patch(ctx, updatedIntents, client.MergeFrom(clientIntents))
if err != nil {
logrus.WithError(err).Errorln("Failed updating intent with service account name")
Expand All @@ -222,7 +221,7 @@ func (c *PolicyManagerImpl) saveServiceAccountName(ctx context.Context, clientIn
}

func (c *PolicyManagerImpl) saveSideCarStatus(ctx context.Context, clientIntents *v1alpha3.ClientIntents, missingSideCar bool) error {
oldValue, ok := clientIntents.Annotations[v1alpha2.OtterizeMissingSidecarAnnotation]
oldValue, ok := clientIntents.Annotations[v1alpha3.OtterizeMissingSidecarAnnotation]
if ok && oldValue == strconv.FormatBool(missingSideCar) {
return nil
}
Expand All @@ -232,7 +231,7 @@ func (c *PolicyManagerImpl) saveSideCarStatus(ctx context.Context, clientIntents
updatedIntents.Annotations = make(map[string]string)
}

updatedIntents.Annotations[v1alpha2.OtterizeMissingSidecarAnnotation] = strconv.FormatBool(missingSideCar)
updatedIntents.Annotations[v1alpha3.OtterizeMissingSidecarAnnotation] = strconv.FormatBool(missingSideCar)
err := c.client.Patch(ctx, updatedIntents, client.MergeFrom(clientIntents))
if err != nil {
return errors.Wrap(err)
Expand All @@ -251,7 +250,7 @@ func (c *PolicyManagerImpl) updateSharedServiceAccountsInNamespace(ctx context.C
}

clientsByServiceAccount := lo.GroupBy(namespacesClientIntents.Items, func(intents v1alpha3.ClientIntents) string {
return intents.Annotations[v1alpha2.OtterizeClientServiceAccountAnnotation]
return intents.Annotations[v1alpha3.OtterizeClientServiceAccountAnnotation]
})

for clientServiceAccountName, clientIntents := range clientsByServiceAccount {
Expand Down Expand Up @@ -282,7 +281,7 @@ func (c *PolicyManagerImpl) updateServiceAccountSharedStatus(ctx context.Context
if updatedIntents.Annotations == nil {
updatedIntents.Annotations = make(map[string]string)
}
updatedIntents.Annotations[v1alpha2.OtterizeSharedServiceAccountAnnotation] = sharedAccountValue
updatedIntents.Annotations[v1alpha3.OtterizeSharedServiceAccountAnnotation] = sharedAccountValue
err := c.client.Patch(ctx, updatedIntents, client.MergeFrom(&intents))
if err != nil {
return errors.Wrap(err)
Expand All @@ -296,12 +295,12 @@ func (c *PolicyManagerImpl) updateServiceAccountSharedStatus(ctx context.Context
}

func shouldUpdateStatus(intents v1alpha3.ClientIntents, currentName string, currentSharedStatus string) bool {
oldName, annotationExists := intents.Annotations[v1alpha2.OtterizeClientServiceAccountAnnotation]
oldName, annotationExists := intents.Annotations[v1alpha3.OtterizeClientServiceAccountAnnotation]
if !annotationExists || oldName != currentName {
return true
}

oldSharedStatus, annotationExists := intents.Annotations[v1alpha2.OtterizeSharedServiceAccountAnnotation]
oldSharedStatus, annotationExists := intents.Annotations[v1alpha3.OtterizeSharedServiceAccountAnnotation]
if !annotationExists || oldSharedStatus != currentSharedStatus {
return true
}
Expand Down Expand Up @@ -378,7 +377,7 @@ func (c *PolicyManagerImpl) createOrUpdatePolicies(

func (c *PolicyManagerImpl) findPolicy(existingPolicies v1beta1.AuthorizationPolicyList, newPolicy *v1beta1.AuthorizationPolicy) (*v1beta1.AuthorizationPolicy, bool) {
for _, policy := range existingPolicies.Items {
if policy.Labels[v1alpha2.OtterizeServerLabelKey] == newPolicy.Labels[v1alpha2.OtterizeServerLabelKey] {
if policy.Labels[v1alpha3.OtterizeServiceLabelKey] == newPolicy.Labels[v1alpha3.OtterizeServiceLabelKey] {
return policy, true
}
}
Expand Down Expand Up @@ -422,7 +421,7 @@ func (c *PolicyManagerImpl) getPolicyName(intents *v1alpha3.ClientIntents, inten
}

func (c *PolicyManagerImpl) isPolicyEqual(existingPolicy *v1beta1.AuthorizationPolicy, newPolicy *v1beta1.AuthorizationPolicy) bool {
sameServer := existingPolicy.Spec.Selector.MatchLabels[v1alpha2.OtterizeServerLabelKey] == newPolicy.Spec.Selector.MatchLabels[v1alpha2.OtterizeServerLabelKey]
sameServer := existingPolicy.Spec.Selector.MatchLabels[v1alpha3.OtterizeServiceLabelKey] == newPolicy.Spec.Selector.MatchLabels[v1alpha3.OtterizeServiceLabelKey]
samePrincipals := existingPolicy.Spec.Rules[0].From[0].Source.Principals[0] == newPolicy.Spec.Rules[0].From[0].Source.Principals[0]
sameHTTPRules := compareHTTPRules(existingPolicy.Spec.Rules[0].To, newPolicy.Spec.Rules[0].To)

Expand Down Expand Up @@ -464,8 +463,8 @@ func (c *PolicyManagerImpl) generateAuthorizationPolicy(
logrus.Infof("Creating Istio policy %s for intent %s", policyName, intent.GetTargetServerName())

serverNamespace := intent.GetTargetServerNamespace(clientIntents.Namespace)
formattedTargetServer := v1alpha2.GetFormattedOtterizeIdentity(intent.GetTargetServerName(), serverNamespace)
clientFormattedIdentity := v1alpha2.GetFormattedOtterizeIdentity(clientIntents.GetServiceName(), clientIntents.Namespace)
formattedTargetServer := v1alpha3.GetFormattedOtterizeIdentity(intent.GetTargetServerName(), serverNamespace)
clientFormattedIdentity := v1alpha3.GetFormattedOtterizeIdentity(clientIntents.GetServiceName(), clientIntents.Namespace)

var ruleTo []*v1beta1security.Rule_To
if intent.Type == v1alpha3.IntentTypeHTTP {
Expand All @@ -484,14 +483,14 @@ func (c *PolicyManagerImpl) generateAuthorizationPolicy(
Name: policyName,
Namespace: serverNamespace,
Labels: map[string]string{
v1alpha2.OtterizeServerLabelKey: formattedTargetServer,
v1alpha2.OtterizeIstioClientAnnotationKey: clientFormattedIdentity,
v1alpha3.OtterizeServiceLabelKey: formattedTargetServer,
v1alpha3.OtterizeIstioClientAnnotationKey: clientFormattedIdentity,
},
},
Spec: v1beta1security.AuthorizationPolicy{
Selector: &v1beta1type.WorkloadSelector{
MatchLabels: map[string]string{
v1alpha2.OtterizeServerLabelKey: formattedTargetServer,
v1alpha3.OtterizeServiceLabelKey: formattedTargetServer,
},
},
Action: v1beta1security.AuthorizationPolicy_ALLOW,
Expand Down
Loading

0 comments on commit b3338f3

Please sign in to comment.