Skip to content

Commit

Permalink
```
Browse files Browse the repository at this point in the history 
[MESH-5515] Add 'security.istio.io/tlsMode' label to Service Entries and Workload Entries during deployment to rollout migration usecase

- Updated various test cases to include the 'security.istio.io/tlsMode' label in Service Entries and Workload Entries.
- Modified validation logic to check for the presence of the 'security.istio.io/tlsMode' label.
- Refactored code to ensure the 'security.istio.io/tlsMode' label is added where necessary.
- Enhanced error handling in validation functions to provide detailed error messages.
  • Loading branch information
Punakshi committed Sep 13, 2024
1 parent 4b33279 commit fc13fc4
Show file tree
Hide file tree
Showing 12 changed files with 191 additions and 110 deletions.
5 changes: 4 additions & 1 deletion admiral/pkg/clusters/configwriter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,10 @@ func getServiceEntryEndpoints(
endpoint := ingressEndpoints[serverCluster]
endpoints := []*networkingV1Alpha3.WorkloadEntry{}
tmpEp := endpoint.DeepCopy()
tmpEp.Labels[typeLabel] = identityConfigEnvironment.Type
if tmpEp.Labels == nil {
tmpEp.Labels = make(map[string]string)
}
tmpEp.Labels["security.istio.io/tlsMode"] = "istio"
services := []*registry.RegistryServiceConfig{}
for _, service := range identityConfigEnvironment.Services {
services = append(services, service)
Expand Down
12 changes: 6 additions & 6 deletions admiral/pkg/clusters/configwriter_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func createMockServiceEntry(env string, identity string, endpointAddress string,
Endpoints: []*networkingV1Alpha3.WorkloadEntry{{Address: endpointAddress,
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(endpointPort)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "rollout"}}},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}},
WorkloadSelector: nil,
ExportTo: exportTo,
SubjectAltNames: []string{"spiffe://prefix/" + identity},
Expand Down Expand Up @@ -146,33 +146,33 @@ func TestGetServiceEntryEndpoints(t *testing.T) {
Address: "def-elb.us-west-2.elb.amazonaws.com.",
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(15443)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "rollout"},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
}}
remoteDeploymentEndpoints := []*networkingV1Alpha3.WorkloadEntry{{
Address: "def-elb.us-west-2.elb.amazonaws.com.",
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(15443)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "deployment"},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
}}
localEndpoints := []*networkingV1Alpha3.WorkloadEntry{{
Address: "app-1-spk-root-service.ns-1-usw2-e2e.svc.cluster.local.",
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(8090)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "rollout"},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
}}
weightedEndpoints := []*networkingV1Alpha3.WorkloadEntry{
{
Address: "app-1-spk-desired-service.ns-1-usw2-e2e.svc.cluster.local.",
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(8090)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "rollout"},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
Weight: 25,
},
{
Address: "app-1-spk-stable-service.ns-1-usw2-e2e.svc.cluster.local.",
Locality: "us-west-2",
Ports: map[string]uint32{"http": uint32(8090)},
Labels: map[string]string{"security.istio.io/tlsMode": "istio", "type": "rollout"},
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
Weight: 75,
},
}
Expand Down
22 changes: 11 additions & 11 deletions admiral/pkg/clusters/destinationrule_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ func TestGetDestinationRule(t *testing.T) {
}

se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"}, Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"},
{Address: "east.com", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}, {Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}
noGtpDr := v1alpha3.DestinationRule{
Host: "qa.myservice.global",
Expand Down Expand Up @@ -442,7 +442,7 @@ func TestGetDestinationRuleActivePassive(t *testing.T) {
seSingleEndpoint := &v1alpha3.ServiceEntry{
Hosts: []string{"qa.myservice.global"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "west.com", Locality: "us-west-2"},
{Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

noGtpDrSingleEndpoint := v1alpha3.DestinationRule{
Expand Down Expand Up @@ -484,8 +484,8 @@ func TestGetDestinationRuleActivePassive(t *testing.T) {
seMultipleEndpoint := &v1alpha3.ServiceEntry{
Hosts: []string{"qa.myservice.global"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "east.com", Locality: "us-east-2"},
{Address: "west.com", Locality: "us-west-2"},
{Address: "east.com", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

noGtpDrMultipleEndpointWest := v1alpha3.DestinationRule{
Expand Down Expand Up @@ -766,7 +766,7 @@ func TestCalculateDistribution(t *testing.T) {
seSingleEndpoint := &v1alpha3.ServiceEntry{
Hosts: []string{"qa.myservice.global"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "west.com", Locality: "us-west-2"},
{Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

singleEndpointDistribution := []*v1alpha3.LocalityLoadBalancerSetting_Distribute{
Expand All @@ -778,8 +778,8 @@ func TestCalculateDistribution(t *testing.T) {
seMultipleEndpoint := &v1alpha3.ServiceEntry{
Hosts: []string{"qa.myservice.global"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "east.com", Locality: "us-east-2"},
{Address: "west.com", Locality: "us-west-2"},
{Address: "east.com", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

multipleEndpointDistribution := []*v1alpha3.LocalityLoadBalancerSetting_Distribute{
Expand Down Expand Up @@ -906,19 +906,19 @@ func TestGetOutlierDetection(t *testing.T) {
}

se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"}, Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"},
{Address: "east.com", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}, {Address: "west.com", Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

seOneHostRemote := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"}, Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "east.com", Locality: "us-east-2"},
{Address: "east.com", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

seOneHostLocal := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"}, Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "hello.ns.svc.cluster.local", Locality: "us-east-2"},
{Address: "hello.ns.svc.cluster.local", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

seOneHostRemoteIp := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"}, Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "95.45.25.34", Locality: "us-east-2"},
{Address: "95.45.25.34", Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}}

//Struct of test case info. Name is required.
Expand Down
2 changes: 1 addition & 1 deletion admiral/pkg/clusters/registry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func TestCopyServiceEntry(t *testing.T) {
func TestCopyEndpoint(t *testing.T) {
setupForRegistryTests()
se := networking.WorkloadEntry{
Address: "127.0.0.1",
Address: "127.0.0.1", Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
}

r := copyEndpoint(&se)
Expand Down
28 changes: 19 additions & 9 deletions admiral/pkg/clusters/serviceentry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1504,14 +1504,14 @@ func AddServiceEntriesWithDrWorker(
compareLabels)
util.LogElapsedTimeSinceTask(ctxLogger, "ReconcileServiceEntry", "", "", cluster, "", start)

valid := validateLocalityInServiceEntry(newServiceEntry)
if seReconciliationRequired && valid {
valid, validityError := validateServiceEntry(newServiceEntry)
if seReconciliationRequired && valid && validityError == nil {
err = addUpdateServiceEntry(ctxLogger, ctx, newServiceEntry, oldServiceEntry, syncNamespace, rc)
addSEorDRToAClusterError = common.AppendError(addSEorDRToAClusterError, err)
}
if !valid {
ctxLogger.Errorf(LogErrFormat, "ValidateLocalityInServiceEntry", "", seDr.SeName, cluster, "failed to validate locality in service entry")
addSEorDRToAClusterError = common.AppendError(addSEorDRToAClusterError, fmt.Errorf("failed to validate locality in service entry"))
if !valid || validityError != nil {
ctxLogger.Errorf(LogErrFormat, "ValidateLocalityInServiceEntry", "", seDr.SeName, cluster, fmt.Errorf("failed to validate the service entry, received error: %v", validityError))
addSEorDRToAClusterError = common.AppendError(addSEorDRToAClusterError, fmt.Errorf("failed to validate locality in service entry, received error: %v", validityError))
}
util.LogElapsedTimeSinceTask(ctxLogger, "AdmiralCacheAddUpdateServiceEntry", "", "", cluster, "", start) // TODO: log service entry name

Expand Down Expand Up @@ -1610,14 +1610,24 @@ func AddServiceEntriesWithDrWorker(
}
}

func validateLocalityInServiceEntry(entry *v1alpha3.ServiceEntry) bool {
// loop through all endpoints and check locality
func validateServiceEntry(entry *v1alpha3.ServiceEntry) (bool, error) {
// loop through all endpoints and check locality and istio mode labels
var errorStrings []string

for _, ep := range entry.Spec.Endpoints {
if ep.Locality == "" {
return false
errorStrings = append(errorStrings, fmt.Sprintf("locality not set for endpoint with address %s", ep.Address))
}
if ep.Labels == nil || ep.Labels["security.istio.io/tlsMode"] != "istio" {
errorStrings = append(errorStrings, fmt.Sprintf("istio mode not set for endpoint with address %s", ep.Address))
}
}
return true

if len(errorStrings) > 0 {
return false, fmt.Errorf(strings.Join(errorStrings, ", "))
}

return true, nil
}

func getClusterRegion(rr *RemoteRegistry, cluster string, rc *RemoteController) (string, error) {
Expand Down
40 changes: 20 additions & 20 deletions admiral/pkg/clusters/serviceentry_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ func TestSkipDestructiveUpdate(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}
twoEndpointSeUpdated := v1alpha3.ServiceEntry{
Expand All @@ -51,8 +51,8 @@ func TestSkipDestructiveUpdate(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 90}, Locality: "us-west-2"},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 90}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}
oneEndpointSe := v1alpha3.ServiceEntry{
Expand All @@ -64,7 +64,7 @@ func TestSkipDestructiveUpdate(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}
newSeTwoEndpoints := &v1alpha32.ServiceEntry{
Expand Down Expand Up @@ -196,8 +196,8 @@ func TestAddUpdateServiceEntry(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "dummy.admiral.global-east", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}

Expand All @@ -210,7 +210,7 @@ func TestAddUpdateServiceEntry(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "dummy.admiral.global-west", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}

Expand All @@ -223,8 +223,8 @@ func TestAddUpdateServiceEntry(t *testing.T) {
Resolution: v1alpha3.ServiceEntry_DNS,
SubjectAltNames: []string{"spiffe://prefix/my-first-service"},
Endpoints: []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "test.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "test.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
},
}

Expand Down Expand Up @@ -340,21 +340,21 @@ func TestAddUpdateServiceEntry(t *testing.T) {

func TestValidateServiceEntryEndpoints(t *testing.T) {
twoValidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}

oneValidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}

dummyEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}

validAndInvalidEndpoints := []*v1alpha3.WorkloadEntry{
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"},
{Address: "dummy.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
}

twoValidEndpointsSe := &v1alpha32.ServiceEntry{
Expand Down Expand Up @@ -430,22 +430,22 @@ func TestValidateServiceEntryEndpoints(t *testing.T) {
name: "Validate SE with valid endpoint",
serviceEntry: oneValidEndpointsSe,
expectedAreEndpointsValid: true,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"}},
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}},
},
{
name: "Validate endpoint with multiple valid endpoints",
serviceEntry: twoValidEndpointsSe,
expectedAreEndpointsValid: true,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2"},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"}},
{Address: "valid1.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-west-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}},
},
{
name: "Validate endpoint with mix of valid and dummy endpoints",
serviceEntry: validAndInvalidEndpointsSe,
expectedAreEndpointsValid: false,
expectedValidEndpoints: []*v1alpha3.WorkloadEntry{
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2"}},
{Address: "valid2.admiral.global", Ports: map[string]uint32{"http": 0}, Locality: "us-east-2", Labels: map[string]string{"security.istio.io/tlsMode": "istio"}}},
},
}

Expand Down
1 change: 1 addition & 0 deletions admiral/pkg/clusters/serviceentry_od_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ func Test_modifyServiceEntryForNewServiceOrPodForOutlierDetection(t *testing.T)
"http": 0,
},
Locality: "us-west-2",
Labels: map[string]string{"security.istio.io/tlsMode": "istio"},
},
},
SubjectAltNames: []string{"spiffe://prefix/" + deployment1Identity},
Expand Down
Loading

0 comments on commit fc13fc4

Please sign in to comment.