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

Adding labels to admiral generated service entries #281

Merged
merged 2 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions admiral/pkg/clusters/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -564,21 +564,25 @@ func addUpdateServiceEntry(ctx context.Context, obj *v1alpha3.ServiceEntry, exis
} else {
log.Errorf(LogFormat+" SE=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "Creation of SE skipped as endpoints are not valid", obj.Spec.String())
}
} else if areEndpointsValid { //update will happen only when all the endpoints are valid
exist.Labels = obj.Labels
exist.Annotations = obj.Annotations
} else {
op = "Update"
skipUpdate, diff = skipDestructiveUpdate(rc, obj, exist)
if diff != "" {
log.Infof(LogFormat+" diff=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "Diff in update", diff)
}
if skipUpdate {
log.Infof(LogFormat, op, "ServiceEntry", obj.Name, rc.ClusterID, "Update skipped as it was destructive during Admiral's bootup phase")
return
if areEndpointsValid { //update will happen only when all the endpoints are valid
exist.Labels = obj.Labels
exist.Annotations = obj.Annotations
skipUpdate, diff = skipDestructiveUpdate(rc, obj, exist)
if diff != "" {
log.Infof(LogFormat+" diff=%s", op, "ServiceEntry", obj.Name, rc.ClusterID, "Diff in update", diff)
}
if skipUpdate {
log.Infof(LogFormat, op, "ServiceEntry", obj.Name, rc.ClusterID, "Update skipped as it was destructive during Admiral's bootup phase")
return
} else {
//nolint
exist.Spec = obj.Spec
_, err = rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries(namespace).Update(ctx, exist, v12.UpdateOptions{})
}
} else {
//nolint
exist.Spec = obj.Spec
_, err = rc.ServiceEntryController.IstioClient.NetworkingV1alpha3().ServiceEntries(namespace).Update(ctx, exist, v12.UpdateOptions{})
log.Infof(LogFormat, op, "ServiceEntry", obj.Name, rc.ClusterID, "SE could not be updated as all the recived endpoints are not valid.")
}
}

Expand Down
34 changes: 25 additions & 9 deletions admiral/pkg/clusters/serviceentry.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@ import (
)

type SeDrTuple struct {
SeName string
DrName string
ServiceEntry *networking.ServiceEntry
DestinationRule *networking.DestinationRule
SeName string
DrName string
ServiceEntry *networking.ServiceEntry
DestinationRule *networking.DestinationRule
SeDnsPrefix string
SeDrGlobalTrafficPolicyName string
}

const (
Expand Down Expand Up @@ -523,7 +525,19 @@ func AddServiceEntriesWithDr(ctx context.Context, rr *RemoteRegistry, sourceClus
//nolint
newServiceEntry := createServiceEntrySkeletion(*seDr.ServiceEntry, seDr.SeName, syncNamespace)
if newServiceEntry != nil {
newServiceEntry.Labels = map[string]string{common.GetWorkloadIdentifier(): fmt.Sprintf("%v", identityId)}
newServiceEntry.Labels = map[string]string{
common.GetWorkloadIdentifier(): fmt.Sprintf("%v", identityId),
common.GetEnvKey(): fmt.Sprintf("%v", env),
}
if newServiceEntry.Annotations == nil {
newServiceEntry.Annotations = map[string]string{}
}
if seDr.SeDnsPrefix != "" && seDr.SeDnsPrefix != common.Default {
newServiceEntry.Annotations["dns-prefix"] = seDr.SeDnsPrefix
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on this spec for label names, the above might not work. We can have dns-prefix?
Also why labels and not an annotations? Are you using this as filter for querying?

Copy link
Collaborator Author

@vrushalijoshi vrushalijoshi Feb 7, 2023

Choose a reason for hiding this comment

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

Good catch will update the name, having said that when I tested it on a test cluster it did create SE with this label. I don't see use case where we might need to filter based on dns prefix or GTP, will update these to be annotation.
Will keep env as label, since identity is today getting added and label, and may be required to filter endpoints, let me know if this is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool, updated code based on this

if seDr.SeDrGlobalTrafficPolicyName != "" {
newServiceEntry.Annotations["associated-gtp"] = seDr.SeDrGlobalTrafficPolicyName
}
addUpdateServiceEntry(ctx, newServiceEntry, oldServiceEntry, syncNamespace, rc)
cache.SeClusterCache.Put(newServiceEntry.Spec.Hosts[0], rc.ClusterID, rc.ClusterID)
}
Expand Down Expand Up @@ -572,10 +586,12 @@ func createSeAndDrSetFromGtp(ctx context.Context, env, region string, se *networ
modifiedSe.Addresses[0] = getUniqueAddress(ctx, cache, host)
}
var seDr = &SeDrTuple{
DrName: drName,
SeName: seName,
DestinationRule: getDestinationRule(modifiedSe, region, gtpTrafficPolicy),
ServiceEntry: modifiedSe,
DrName: drName,
SeName: seName,
DestinationRule: getDestinationRule(modifiedSe, region, gtpTrafficPolicy),
ServiceEntry: modifiedSe,
SeDnsPrefix: gtpTrafficPolicy.DnsPrefix,
SeDrGlobalTrafficPolicyName: globalTrafficPolicy.Name,
}
seDrSet[host] = seDr
}
Expand Down
137 changes: 116 additions & 21 deletions admiral/pkg/clusters/serviceentry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -31,7 +32,6 @@ import (
v14 "k8s.io/api/apps/v1"
coreV1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
v12 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/rest"
Expand Down Expand Up @@ -448,14 +448,62 @@ func TestIsGeneratedByAdmiral(t *testing.T) {
func TestAddServiceEntriesWithDr(t *testing.T) {
admiralCache := AdmiralCache{}

cacheWithNoEntry := ServiceEntryAddressStore{
EntryAddresses: map[string]string{"prefix.e2e.foo.global-se": "test"},
Addresses: []string{},
}

admiralCache.SeClusterCache = common.NewMapOfMaps()
admiralCache.ServiceEntryAddressStore = &cacheWithNoEntry

cnameIdentityCache := sync.Map{}
cnameIdentityCache.Store("dev.bar.global", "bar")
cnameIdentityCache.Store("dev.newse.global", "newse")
cnameIdentityCache.Store("e2e.foo.global", "foo")
admiralCache.CnameIdentityCache = &cnameIdentityCache

trafficPolicyOverride := &model.TrafficPolicy{
LbType: model.TrafficPolicy_FAILOVER,
DnsPrefix: common.Default,
Target: []*model.TrafficGroup{
{
Region: "us-west-2",
Weight: 100,
},
},
}

defaultGtp := &v13.GlobalTrafficPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test.dev.bar-gtp",
},
Spec: model.GlobalTrafficPolicy{
Policy: []*model.TrafficPolicy{
trafficPolicyOverride,
},
},
}

prefixedTrafficPolicy := &model.TrafficPolicy{
LbType: model.TrafficPolicy_TOPOLOGY,
DnsPrefix: "prefix",
}

prefixedGtp := &v13.GlobalTrafficPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test.e2e.foo-gtp",
},
Spec: model.GlobalTrafficPolicy{
Policy: []*model.TrafficPolicy{
prefixedTrafficPolicy,
},
},
}

gtpCache := &globalTrafficCache{}
gtpCache.identityCache = make(map[string]*v13.GlobalTrafficPolicy)
gtpCache.identityCache["dev.bar"] = defaultGtp
gtpCache.identityCache["e2e.foo"] = prefixedGtp
gtpCache.mutex = &sync.Mutex{}
admiralCache.GlobalTrafficCache = gtpCache

Expand All @@ -466,6 +514,14 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
},
}

newPrefixedSE := istioNetworkingV1Alpha3.ServiceEntry{
Addresses: []string{"240.10.1.0"},
Hosts: []string{"e2e.foo.global"},
Endpoints: []*istioNetworkingV1Alpha3.WorkloadEntry{
{Address: "127.0.0.1", Ports: map[string]uint32{"https": 80}, Labels: map[string]string{}, Network: "mesh1", Locality: "us-west", Weight: 100},
},
}

se := istioNetworkingV1Alpha3.ServiceEntry{
Hosts: []string{"dev.bar.global"},
Endpoints: []*istioNetworkingV1Alpha3.WorkloadEntry{
Expand Down Expand Up @@ -601,9 +657,14 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
rr.PutRemoteController("cl1", rc)
rr.AdmiralCache = &admiralCache

destinationRuleFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error {
destinationRuleFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, dnsPrefix string) error {
for _, serviceEntry := range serviceEntries {
drName := getIstioResourceName(serviceEntry.Hosts[0], "-default-dr")
var drName string
if dnsPrefix != "" && dnsPrefix != "default" {
drName = getIstioResourceName(serviceEntry.Hosts[0], "-dr")
} else {
drName = getIstioResourceName(serviceEntry.Hosts[0], "-default-dr")
}
dr, err := fakeIstioClient.NetworkingV1alpha3().DestinationRules("ns").Get(ctx, drName, v12.GetOptions{})
if err != nil {
return err
Expand All @@ -618,7 +679,7 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
return nil
}

destinationRuleNotFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error {
destinationRuleNotFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, dnsPrefix string) error {
for _, serviceEntry := range serviceEntries {
drName := getIstioResourceName(serviceEntry.Hosts[0], "-default-dr")
_, err := fakeIstioClient.NetworkingV1alpha3().DestinationRules("ns").Get(ctx, drName, v12.GetOptions{})
Expand All @@ -629,7 +690,7 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
return nil
}

serviceEntryFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error {
serviceEntryFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, expectedLabels map[string]string) error {
for _, serviceEntry := range serviceEntries {
seName := getIstioResourceName(serviceEntry.Hosts[0], "-se")
se, err := fakeIstioClient.NetworkingV1alpha3().ServiceEntries("ns").Get(ctx, seName, v12.GetOptions{})
Expand All @@ -642,10 +703,13 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
if !reflect.DeepEqual(expectedAnnotations, se.Annotations) {
return fmt.Errorf("expected SE annotations %v but got %v", expectedAnnotations, se.Annotations)
}
if !reflect.DeepEqual(expectedLabels, se.Labels) {
return fmt.Errorf("expected SE labels %v but got %v", expectedLabels, se.Labels)
}
}
return nil
}
serviceEntryNotFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error {
serviceEntryNotFoundAssertion := func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, expectedLabels map[string]string) error {
for _, serviceEntry := range serviceEntries {
seName := getIstioResourceName(serviceEntry.Hosts[0], "-se")
_, err := fakeIstioClient.NetworkingV1alpha3().ServiceEntries("ns").Get(ctx, seName, v12.GetOptions{})
Expand All @@ -659,23 +723,40 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
testCases := []struct {
name string
serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry
serviceEntryAssertion func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error
destinationRuleAssertion func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string) error
expectedAnnotations map[string]string
dnsPrefix string
serviceEntryAssertion func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, expectedLabels map[string]string) error
destinationRuleAssertion func(ctx context.Context, fakeIstioClient *istiofake.Clientset, serviceEntries map[string]*istioNetworkingV1Alpha3.ServiceEntry, expectedAnnotations map[string]string, dnsPrefix string) error
expectedDRAnnotations map[string]string
expectedSEAnnotations map[string]string
expectedLabels map[string]string
}{
{
name: "given a serviceEntry that does not exists, when AddServiceEntriesWithDr is called, then the se is created and the corresponding dr is created",
serviceEntries: map[string]*istioNetworkingV1Alpha3.ServiceEntry{"se1": &newSE},
serviceEntryAssertion: serviceEntryFoundAssertion,
destinationRuleAssertion: destinationRuleFoundAssertion,
expectedAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedDRAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedSEAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedLabels: map[string]string{"env": "dev", "identity": "newse"},
},
{
name: "given a serviceEntry that already exists in the sync ns, when AddServiceEntriesWithDr is called, then the se is updated and the corresponding dr is updated as well",
serviceEntries: map[string]*istioNetworkingV1Alpha3.ServiceEntry{"se1": &se},
serviceEntryAssertion: serviceEntryFoundAssertion,
destinationRuleAssertion: destinationRuleFoundAssertion,
expectedAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedDRAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedSEAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue, "associated-gtp": "test.dev.bar-gtp"},
expectedLabels: map[string]string{"env": "dev", "identity": "bar"},
},
{
name: "given a serviceEntry that does not exists and gtp with dnsPrefix is configured, when AddServiceEntriesWithDr is called, then the se is created and the corresponding dr is created as well",
serviceEntries: map[string]*istioNetworkingV1Alpha3.ServiceEntry{"se1": &newPrefixedSE},
serviceEntryAssertion: serviceEntryFoundAssertion,
destinationRuleAssertion: destinationRuleFoundAssertion,
dnsPrefix: "prefix",
expectedDRAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue},
expectedSEAnnotations: map[string]string{resourceCreatedByAnnotationLabel: resourceCreatedByAnnotationValue, "dns-prefix": "prefix", "associated-gtp": "test.e2e.foo-gtp"},
expectedLabels: map[string]string{"env": "e2e", "identity": "foo"},
},
{
name: "given a serviceEntry that already exists in the sync ns and the serviceEntry does not have any valid endpoints, when AddServiceEntriesWithDr is called, then the se should be deleted along with the corresponding dr",
Expand All @@ -694,18 +775,23 @@ func TestAddServiceEntriesWithDr(t *testing.T) {
serviceEntries: map[string]*istioNetworkingV1Alpha3.ServiceEntry{"admiralOverrideSE": &admiralOverrideSE.Spec},
serviceEntryAssertion: serviceEntryFoundAssertion,
destinationRuleAssertion: destinationRuleFoundAssertion,
expectedAnnotations: nil,
expectedDRAnnotations: nil,
expectedSEAnnotations: nil,
expectedLabels: nil,
},
}

for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
AddServiceEntriesWithDr(ctx, rr, map[string]string{"cl1": "cl1"}, tt.serviceEntries)
err := tt.serviceEntryAssertion(context.Background(), fakeIstioClient, tt.serviceEntries, tt.expectedAnnotations)
if tt.dnsPrefix != "" && tt.dnsPrefix != "default" {
tt.serviceEntries["se1"].Hosts = []string{tt.dnsPrefix + ".e2e.foo.global"}
}
err := tt.serviceEntryAssertion(context.Background(), fakeIstioClient, tt.serviceEntries, tt.expectedSEAnnotations, tt.expectedLabels)
if err != nil {
t.Error(err)
}
err = tt.destinationRuleAssertion(context.Background(), fakeIstioClient, tt.serviceEntries, tt.expectedAnnotations)
err = tt.destinationRuleAssertion(context.Background(), fakeIstioClient, tt.serviceEntries, tt.expectedDRAnnotations, tt.dnsPrefix)
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -789,6 +875,9 @@ func TestCreateSeAndDrSetFromGtp(t *testing.T) {
}

gTPDefaultOverride := &v13.GlobalTrafficPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "gTPDefaultOverrideName",
},
Spec: model.GlobalTrafficPolicy{
Policy: []*model.TrafficPolicy{
trafficPolicyDefaultOverride,
Expand All @@ -797,6 +886,9 @@ func TestCreateSeAndDrSetFromGtp(t *testing.T) {
}

gTPMultipleDns := &v13.GlobalTrafficPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "gTPMultipleDnsName",
},
Spec: model.GlobalTrafficPolicy{
Policy: []*model.TrafficPolicy{
defaultPolicy, trafficPolicyWest, trafficPolicyEast,
Expand All @@ -818,33 +910,33 @@ func TestCreateSeAndDrSetFromGtp(t *testing.T) {
locality: "us-west-2",
se: se,
gtp: nil,
seDrSet: map[string]*SeDrTuple{host: nil},
seDrSet: map[string]*SeDrTuple{host: &SeDrTuple{}},
},
{
name: "Should handle a GTP with default overide",
env: "dev",
locality: "us-west-2",
se: se,
gtp: gTPDefaultOverride,
seDrSet: map[string]*SeDrTuple{host: nil},
seDrSet: map[string]*SeDrTuple{host: &SeDrTuple{SeDnsPrefix: "default", SeDrGlobalTrafficPolicyName: "gTPDefaultOverrideName"}},
},
{
name: "Should handle a GTP with multiple Dns",
env: "dev",
locality: "us-west-2",
se: se,
gtp: gTPMultipleDns,
seDrSet: map[string]*SeDrTuple{host: nil, common.GetCnameVal([]string{west, host}): nil,
common.GetCnameVal([]string{east, host}): nil},
seDrSet: map[string]*SeDrTuple{host: &SeDrTuple{SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"}, common.GetCnameVal([]string{west, host}): &SeDrTuple{SeDnsPrefix: "west", SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"},
common.GetCnameVal([]string{east, host}): &SeDrTuple{SeDnsPrefix: "east", SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"}},
},
{
name: "Should handle a GTP with Dns prefix with Caps",
env: "dev",
locality: "us-west-2",
se: se,
gtp: gTPMultipleDns,
seDrSet: map[string]*SeDrTuple{host: nil, common.GetCnameVal([]string{west, host}): nil,
strings.ToLower(common.GetCnameVal([]string{eastWithCaps, host})): nil},
seDrSet: map[string]*SeDrTuple{host: &SeDrTuple{SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"}, common.GetCnameVal([]string{west, host}): &SeDrTuple{SeDnsPrefix: "west", SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"},
strings.ToLower(common.GetCnameVal([]string{eastWithCaps, host})): &SeDrTuple{SeDnsPrefix: "east", SeDrGlobalTrafficPolicyName: "gTPMultipleDnsName"}},
},
}
ctx := context.Background()
Expand All @@ -861,11 +953,14 @@ func TestCreateSeAndDrSetFromGtp(t *testing.T) {
t.Fatalf("Generated hosts %v is missing the required host: %v", generatedHosts, host)
} else if !isLower(result[host].SeName) || !isLower(result[host].DrName) {
t.Fatalf("Generated istio resource names %v %v are not all lowercase", result[host].SeName, result[host].DrName)
} else if result[host].SeDnsPrefix != c.seDrSet[host].SeDnsPrefix {
t.Fatalf("Expected seDrSet entry dnsPrefix %s does not match the result %s", c.seDrSet[host].SeDnsPrefix, result[host].SeDnsPrefix)
} else if result[host].SeDrGlobalTrafficPolicyName != c.seDrSet[host].SeDrGlobalTrafficPolicyName {
t.Fatalf("Expected seDrSet entry global traffic policy name %s does not match the result %s", c.seDrSet[host].SeDrGlobalTrafficPolicyName, result[host].SeDrGlobalTrafficPolicyName)
}
}
})
}

}

func TestCreateServiceEntryForNewServiceOrPod(t *testing.T) {
Expand Down