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.

Sign up for GitHub

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

Jump to bottom

Added cross-namespace validation support for Sidecar's egress listener host, considering exported ServiceEntries. #4387

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Added cross-namespace validation support for Sidecar's egress listener host, considering exported ServiceEntries. #4387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion business/checkers/destination_rules_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type DestinationRulesChecker struct {
ExportedDestinationRules []networking_v1alpha3.DestinationRule
MTLSDetails kubernetes.MTLSDetails
ServiceEntries []networking_v1alpha3.ServiceEntry
ExportedServiceEntries []networking_v1alpha3.ServiceEntry
Namespaces []models.Namespace
}

Expand All @@ -31,7 +32,7 @@ func (in DestinationRulesChecker) Check() models.IstioValidations {
func (in DestinationRulesChecker) runGroupChecks() models.IstioValidations {
validations := models.IstioValidations{}

seHosts := kubernetes.ServiceEntryHostnames(in.ServiceEntries)
seHosts := kubernetes.ServiceEntryHostnames(append(in.ServiceEntries, in.ExportedServiceEntries...))

enabledDRCheckers := []GroupChecker{
destinationrules.MultiMatchChecker{Namespaces: in.Namespaces, DestinationRules: in.DestinationRules, ServiceEntries: seHosts, ExportedDestinationRules: in.ExportedDestinationRules},
Expand Down
35 changes: 10 additions & 25 deletions business/checkers/sidecars/egress_listener_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

core_v1 "k8s.io/api/core/v1"

"github.com/kiali/kiali/config"
"github.com/kiali/kiali/kubernetes"
"github.com/kiali/kiali/models"
)
Expand Down Expand Up @@ -62,7 +61,6 @@ func (elc EgressHostChecker) getHosts() ([]HostWithIndex, bool) {

func (elc EgressHostChecker) validateHost(host string, egrIdx, hostIdx int) ([]*models.IstioCheck, bool) {
checks := make([]*models.IstioCheck, 0)
ins := config.Get().IstioNamespace
sns := elc.Sidecar.Namespace

hostNs, dnsName, valid := getHostComponents(host)
Expand All @@ -75,44 +73,31 @@ func (elc EgressHostChecker) validateHost(host string, egrIdx, hostIdx int) ([]*
return checks, true
}

// Show cross-namespace validation
// when namespace is different to both istio control plane or sidecar namespace
if hostNs != ins && hostNs != sns && hostNs != "." {
return append(checks, buildCheck("validation.unable.cross-namespace", egrIdx, hostIdx)), true
// namespace/* is a valid scenario
if dnsName == "*" {
return checks, true
}

// Lookup services when ns is . or sidecar namespace
if hostNs == sns || hostNs == "." {
// namespace/* is a valid scenario
if dnsName == "*" {
return checks, true
}

// Parse the dnsName to a kubernetes Host
fqdn := kubernetes.ParseHost(dnsName, sns, elc.Sidecar.ClusterName)
if fqdn.Namespace != sns && fqdn.Namespace != "" {
return append(checks, buildCheck("validation.unable.cross-namespace", egrIdx, hostIdx)), true
}
fqdn := kubernetes.ParseHost(dnsName, sns, elc.Sidecar.ClusterName)

// Lookup for matching services
if !elc.HasMatchingService(fqdn, sns) {
checks = append(checks, buildCheck("sidecar.egress.servicenotfound", egrIdx, hostIdx))
}
// Lookup for matching services
if !elc.HasMatchingService(fqdn, sns) {
checks = append(checks, buildCheck("sidecar.egress.servicenotfound", egrIdx, hostIdx))
}

return checks, true
}

func (elc EgressHostChecker) HasMatchingService(host kubernetes.Host, itemNamespace string) bool {
if strings.HasPrefix(host.Service, "*") {
// Check wildcard hosts - needs to match "*" and "*.suffix" also.
if host.IsWildcard() && host.Namespace == itemNamespace {
return true
}

if kubernetes.HasMatchingServices(host.Service, elc.Services) {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking of cross-namespace validations. I think we should change this method to cover the cross-namespace at some point. It sounds better to have a match between a Host and a Service instead of a service name and a service.
However, It might be approached in another issue since the services past in this checker are only from one namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, we have a separate task for it: #4317

return true
}

return kubernetes.HasMatchingServiceEntries(host.Service, elc.ServiceEntries)
return kubernetes.HasMatchingServiceEntries(host.String(), elc.ServiceEntries)
}

func getHostComponents(host string) (string, string, bool) {
Expand Down
158 changes: 154 additions & 4 deletions business/checkers/sidecars/egress_listener_checker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestEgressHostFormatCorrect(t *testing.T) {
"~/*",
"./*",
"./reviews.bookinfo.svc.cluster.local",
"./*.bookinfo.svc.cluster.com",
"./*.bookinfo.svc.cluster.local",
Copy link
Member

Choose a reason for hiding this comment

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

I'd keep both of them. The old one was a very specific service entry that might help identify some logics for service entries. Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the "bookinfo" in this FQDN makes a confusion, and it worked before because of "if strings.HasPrefix(host.Service, "*")" and now fails because of "if host.IsWildcard() && host.Namespace == itemNamespace".

"./wikipedia.org",
"bookinfo/*",
"bookinfo/*.bookinfo.svc.cluster.local",
Expand All @@ -39,7 +39,157 @@ func TestEgressHostFormatCorrect(t *testing.T) {
assert.True(valid)
}

func TestEgressHostCrossNamespace(t *testing.T) {
func TestEgressExportedInternalServiceEntryPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshInternalServiceEntry("details-se", "bookinfo3", []string{"details.bookinfo2.svc.cluster.local"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/details.bookinfo2.svc.cluster.local",
}),
}.Check()

assert.Empty(vals)
assert.True(valid)
}

func TestEgressExportedExternalServiceEntryPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshExternalServiceEntry("details-se", "bookinfo3", []string{"www.myhost.com"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/www.myhost.com",
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it wasn't covered before, but should we try to test for a bookinfo/*.myhost.com?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returning back to this request after several researches. In the validation logic of Sidecar's hosts, the "" is ignored and not checked it's further existance, so even if host is "bookinfo/.mywronghost.com" the error message will not be shown.
The purpose of this PR is to support only cross-namespace validations without touching the existing logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

}),
}.Check()

assert.Empty(vals)
assert.True(valid)
}

func TestWildcardHostEgressExportedExternalServiceEntryNotPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshExternalServiceEntry("details-se", "bookinfo3", []string{"www.myhost.com"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/*.myhost.com",
}),
}.Check()

assert.NotEmpty(vals)
assert.True(valid)
assert.Equal(models.WarningSeverity, vals[0].Severity)
assert.Equal("spec/egress[0]/hosts[0]", vals[0].Path)
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", vals[0]))
}

func TestEgressExportedExternalWildcardServiceEntryPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshExternalServiceEntry("details-se", "bookinfo3", []string{"*.myhost.com"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/www.myhost.com",
}),
}.Check()

assert.Empty(vals)
assert.True(valid)
}

func TestEgressExportedInternalServiceEntryNotPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshInternalServiceEntry("details-se", "bookinfo3", []string{"details.bookinfo2.svc.cluster.local"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/details.bookinfo.svc.cluster.local",
}),
}.Check()

assert.NotEmpty(vals)
assert.True(valid)
assert.Equal(models.WarningSeverity, vals[0].Severity)
assert.Equal("spec/egress[0]/hosts[0]", vals[0].Path)
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", vals[0]))
}

func TestEgressExportedExternalServiceEntryNotPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshExternalServiceEntry("details-se", "bookinfo3", []string{"www.myhost.com"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/www.wrong.com",
}),
}.Check()

assert.NotEmpty(vals)
assert.True(valid)
assert.Equal(models.WarningSeverity, vals[0].Severity)
assert.Equal("spec/egress[0]/hosts[0]", vals[0].Path)
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", vals[0]))
}

func TestEgressExportedWildcardInternalServiceEntryPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshInternalServiceEntry("details-se", "bookinfo3", []string{"*.bookinfo2.svc.cluster.local"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/details.bookinfo2.svc.cluster.local",
}),
}.Check()

assert.Empty(vals)
assert.True(valid)
}

func TestEgressExportedWildcardInternalServiceEntryNotPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshInternalServiceEntry("details-se", "bookinfo3", []string{"*.bookinfo3.svc.cluster.local"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/*.bookinfo2.svc.cluster.local",
}),
}.Check()

assert.NotEmpty(vals)
assert.True(valid)
assert.Equal(models.WarningSeverity, vals[0].Severity)
assert.Equal("spec/egress[0]/hosts[0]", vals[0].Path)
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", vals[0]))
}

func TestEgressExportedNonFQDNInternalServiceEntryNotPresent(t *testing.T) {
assert := assert.New(t)

vals, valid := EgressHostChecker{
Services: []core_v1.Service{},
ServiceEntries: kubernetes.ServiceEntryHostnames([]networking_v1alpha3.ServiceEntry{*data.CreateEmptyMeshInternalServiceEntry("details-se", "bookinfo3", []string{"details"})}),
Sidecar: *sidecarWithHosts([]string{
"bookinfo/details.bookinfo2.svc.cluster.local",
}),
}.Check()

assert.NotEmpty(vals)
assert.True(valid)
assert.Equal(models.WarningSeverity, vals[0].Severity)
assert.Equal("spec/egress[0]/hosts[0]", vals[0].Path)
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", vals[0]))
}

func TestEgressHostCrossNamespaceServiceNotFound(t *testing.T) {
assert := assert.New(t)

hosts := []string{
Expand All @@ -62,9 +212,9 @@ func TestEgressHostCrossNamespace(t *testing.T) {
assert.True(valid)

for i, c := range vals {
assert.Equal(models.Unknown, c.Severity)
assert.Equal(models.WarningSeverity, c.Severity)
assert.Equal(fmt.Sprintf("spec/egress[0]/hosts[%d]", i), c.Path)
assert.NoError(validations.ConfirmIstioCheckMessage("validation.unable.cross-namespace", c))
assert.NoError(validations.ConfirmIstioCheckMessage("sidecar.egress.servicenotfound", c))
}
}

Expand Down
14 changes: 8 additions & 6 deletions business/checkers/sidecars_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@ import (
const SidecarCheckerType = "sidecar"

type SidecarChecker struct {
Sidecars []networking_v1alpha3.Sidecar
ServiceEntries []networking_v1alpha3.ServiceEntry
Services []core_v1.Service
Namespaces models.Namespaces
WorkloadList models.WorkloadList
Sidecars []networking_v1alpha3.Sidecar
ServiceEntries []networking_v1alpha3.ServiceEntry
ExportedServiceEntries []networking_v1alpha3.ServiceEntry
Services []core_v1.Service
Namespaces models.Namespaces
WorkloadList models.WorkloadList
}

func (s SidecarChecker) Check() models.IstioValidations {
Expand Down Expand Up @@ -57,11 +58,12 @@ func (s SidecarChecker) runIndividualChecks() models.IstioValidations {
func (s SidecarChecker) runChecks(sidecar networking_v1alpha3.Sidecar) models.IstioValidations {
policyName := sidecar.Name
key, rrValidation := EmptyValidValidation(policyName, sidecar.Namespace, SidecarCheckerType)
serviceHosts := kubernetes.ServiceEntryHostnames(s.ServiceEntries)
serviceHosts := kubernetes.ServiceEntryHostnames(append(s.ServiceEntries, s.ExportedServiceEntries...))
selectorLabels := make(map[string]string)
if sidecar.Spec.WorkloadSelector != nil {
selectorLabels = sidecar.Spec.WorkloadSelector.Labels
}

enabledCheckers := []Checker{
common.WorkloadSelectorNoWorkloadFoundChecker(SidecarCheckerType, selectorLabels, s.WorkloadList),
sidecars.EgressHostChecker{Sidecar: sidecar, Services: s.Services, ServiceEntries: serviceHosts},
Expand Down
4 changes: 2 additions & 2 deletions business/istio_validations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func (in *IstioValidationsService) getAllObjectCheckers(namespace string, istioC
checkers.PeerAuthenticationChecker{PeerAuthentications: mtlsDetails.PeerAuthentications, MTLSDetails: mtlsDetails, WorkloadList: workloads},
checkers.ServiceEntryChecker{ServiceEntries: istioConfigList.ServiceEntries, Namespaces: namespaces},
checkers.AuthorizationPolicyChecker{AuthorizationPolicies: rbacDetails.AuthorizationPolicies, Namespace: namespace, Namespaces: namespaces, Services: services, ServiceEntries: istioConfigList.ServiceEntries, ExportedServiceEntries: exportedResources.ServiceEntries, WorkloadList: workloads, MtlsDetails: mtlsDetails, VirtualServices: istioConfigList.VirtualServices, RegistryStatus: registryStatus},
checkers.SidecarChecker{Sidecars: istioConfigList.Sidecars, Namespaces: namespaces, WorkloadList: workloads, Services: services, ServiceEntries: istioConfigList.ServiceEntries},
checkers.SidecarChecker{Sidecars: istioConfigList.Sidecars, Namespaces: namespaces, WorkloadList: workloads, Services: services, ServiceEntries: istioConfigList.ServiceEntries, ExportedServiceEntries: exportedResources.ServiceEntries},
checkers.RequestAuthenticationChecker{RequestAuthentications: istioConfigList.RequestAuthentications, WorkloadList: workloads},
}
}
Expand Down Expand Up @@ -189,7 +189,7 @@ func (in *IstioValidationsService) GetIstioObjectValidations(namespace string, o
objectCheckers = []ObjectChecker{serviceEntryChecker}
case kubernetes.Sidecars:
sidecarsChecker := checkers.SidecarChecker{Sidecars: istioConfigList.Sidecars, Namespaces: namespaces,
WorkloadList: workloads, Services: services, ServiceEntries: istioConfigList.ServiceEntries}
WorkloadList: workloads, Services: services, ServiceEntries: istioConfigList.ServiceEntries, ExportedServiceEntries: exportedResources.ServiceEntries}
objectCheckers = []ObjectChecker{sidecarsChecker}
case kubernetes.AuthorizationPolicies:
authPoliciesChecker := checkers.AuthorizationPolicyChecker{AuthorizationPolicies: rbacDetails.AuthorizationPolicies,
Expand Down