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

Adding MultiMatch and NoWorkloadFound validations to PeerAuthentication #2808

Merged
merged 5 commits into from
May 29, 2020
Merged

Adding MultiMatch and NoWorkloadFound validations to PeerAuthentication #2808

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37d1452
Validations: Adding MultiMatch to PeerAuthentication
xeviknal May 20, 2020
2a8a60f
Validations: Add NoWorkloadFound to PeerAuthentication
xeviknal May 20, 2020
b7df219
fixup
xeviknal May 20, 2020
de94906
fixup
xeviknal May 20, 2020
ec2971e
Approaching comments
xeviknal May 26, 2020
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
47 changes: 0 additions & 47 deletions business/checkers/authorization/workload_selector_checker.go
View file
Open in desktop

This file was deleted.

81 changes: 0 additions & 81 deletions business/checkers/authorization/workload_selector_checker_test.go
View file
Open in desktop

This file was deleted.

3 changes: 2 additions & 1 deletion business/checkers/authorization_policies_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
core_v1 "k8s.io/api/core/v1"

"github.com/kiali/kiali/business/checkers/authorization"
"github.com/kiali/kiali/business/checkers/common"
"github.com/kiali/kiali/kubernetes"
"github.com/kiali/kiali/models"
)
Expand Down Expand Up @@ -36,8 +37,8 @@ func (a AuthorizationPolicyChecker) runChecks(authPolicy kubernetes.IstioObject)
serviceHosts := kubernetes.ServiceEntryHostnames(a.ServiceEntries)

enabledCheckers := []Checker{
common.SelectorNoWorkloadFoundChecker(AuthorizationPolicyCheckerType, authPolicy, a.WorkloadList),
authorization.NamespaceMethodChecker{AuthorizationPolicy: authPolicy, Namespaces: a.Namespaces.GetNames()},
authorization.WorkloadSelectorChecker{AuthorizationPolicy: authPolicy, WorkloadList: a.WorkloadList},
authorization.NoHostChecker{AuthorizationPolicy: authPolicy, Namespace: a.Namespace, Namespaces: a.Namespaces,
ServiceEntries: serviceHosts, Services: a.Services},
}
Expand Down
116 changes: 75 additions & 41 deletions .../checkers/sidecars/multi_match_checker.go → ...rs/common/multi_match_selector_checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,41 @@
package sidecars
package common

import (
"k8s.io/apimachinery/pkg/labels"

"fmt"
"github.com/kiali/kiali/kubernetes"
"github.com/kiali/kiali/models"
"k8s.io/apimachinery/pkg/labels"
)

const SidecarCheckerType = "sidecar"
type GenericMultiMatchChecker struct {
SubjectType string
Subjects []kubernetes.IstioObject
WorkloadList models.WorkloadList
HasSelector func(s kubernetes.IstioObject) bool
GetSelectorLabels func(s kubernetes.IstioObject) map[string]string
Path string
}

func SelectorMultiMatchChecker(subjectType string, subjects []kubernetes.IstioObject, workloadList models.WorkloadList) GenericMultiMatchChecker {
return GenericMultiMatchChecker{
SubjectType: subjectType,
Subjects: subjects,
WorkloadList: workloadList,
HasSelector: HasSelector,
GetSelectorLabels: GetSelectorLabels,
Path: "spec/selector",
}
}

type MultiMatchChecker struct {
Sidecars []kubernetes.IstioObject
WorkloadList models.WorkloadList
func WorkloadSelectorMultiMatchChecker(subjectType string, subjects []kubernetes.IstioObject, workloadList models.WorkloadList) GenericMultiMatchChecker {
return GenericMultiMatchChecker{
SubjectType: subjectType,
Subjects: subjects,
WorkloadList: workloadList,
HasSelector: HasWorkloadSelector,
GetSelectorLabels: GetWorkloadSelectorLabels,
Path: "spec/workloadSelector",
}
}

type KeyWithIndex struct {
Expand All @@ -33,7 +57,7 @@ func (ws ReferenceMap) HasMultipleReferences(wk models.IstioValidationKey) bool
return len(ws.Get(wk)) > 1
}

func (m MultiMatchChecker) Check() models.IstioValidations {
func (m GenericMultiMatchChecker) Check() models.IstioValidations {
validations := models.IstioValidations{}

validations.MergeValidations(m.analyzeSelectorLessSidecars())
Expand All @@ -42,22 +66,21 @@ func (m MultiMatchChecker) Check() models.IstioValidations {
return validations
}

func (m MultiMatchChecker) analyzeSelectorLessSidecars() models.IstioValidations {
swi := m.selectorLessSidecars()
return buildSelectorLessSidecarValidations(swi)
func (m GenericMultiMatchChecker) analyzeSelectorLessSidecars() models.IstioValidations {
return m.buildSelectorLessSidecarValidations(m.selectorLessSidecars())
}

func (m MultiMatchChecker) selectorLessSidecars() []KeyWithIndex {
swi := make([]KeyWithIndex, 0, len(m.Sidecars))
func (m GenericMultiMatchChecker) selectorLessSidecars() []KeyWithIndex {
swi := make([]KeyWithIndex, 0, len(m.Subjects))

for i, s := range m.Sidecars {
if !s.HasWorkloadSelectorLabels() {
for i, s := range m.Subjects {
if !m.HasSelector(s) {
swi = append(swi, KeyWithIndex{
Index: i,
Key: &models.IstioValidationKey{
Name: s.GetObjectMeta().Name,
Namespace: s.GetObjectMeta().Namespace,
ObjectType: SidecarCheckerType,
ObjectType: m.SubjectType,
},
},
)
Expand All @@ -66,7 +89,7 @@ func (m MultiMatchChecker) selectorLessSidecars() []KeyWithIndex {
return swi
}

func buildSelectorLessSidecarValidations(sidecars []KeyWithIndex) models.IstioValidations {
func (m GenericMultiMatchChecker) buildSelectorLessSidecarValidations(sidecars []KeyWithIndex) models.IstioValidations {
validations := models.IstioValidations{}

if len(sidecars) < 2 {
Expand All @@ -75,7 +98,7 @@ func buildSelectorLessSidecarValidations(sidecars []KeyWithIndex) models.IstioVa

for _, sidecarWithIndex := range sidecars {
references := extractReferences(sidecarWithIndex.Index, sidecars)
checks := models.Build("sidecar.multimatch.selectorless", "spec/workloadSelector")
checks := models.Build(fmt.Sprintf("%s.multimatch.selectorless", m.SubjectType), m.Path)
validations.MergeValidations(
models.IstioValidations{
*sidecarWithIndex.Key: &models.IstioValidation{
Expand All @@ -94,34 +117,29 @@ func buildSelectorLessSidecarValidations(sidecars []KeyWithIndex) models.IstioVa
}

func extractReferences(index int, sidecars []KeyWithIndex) []models.IstioValidationKey {
references := make([]models.IstioValidationKey, 0, len(sidecars))
filtered := make([]KeyWithIndex, 0, len(sidecars)-1)

// Exclude item at index position
filtered = append(filtered, sidecars[:index]...)
if len(sidecars) > index+1 {
filtered = append(filtered, sidecars[index+1:]...)
}
references := make([]models.IstioValidationKey, 0, len(sidecars)-1)

for _, s := range filtered {
references = append(references, *s.Key)
for _, s := range sidecars {
if s.Index != index {
references = append(references, *s.Key)
}
}

return references
}

func (m MultiMatchChecker) analyzeSelectorSidecars() models.IstioValidations {
func (m GenericMultiMatchChecker) analyzeSelectorSidecars() models.IstioValidations {
sidecars := m.multiMatchSidecars()
return m.buildSidecarValidations(sidecars)
}

func (m MultiMatchChecker) multiMatchSidecars() ReferenceMap {
func (m GenericMultiMatchChecker) multiMatchSidecars() ReferenceMap {
workloadSidecars := ReferenceMap{}

for _, s := range m.Sidecars {
sidecarKey := models.BuildKey(SidecarCheckerType, s.GetObjectMeta().Name, s.GetObjectMeta().Namespace)
for _, s := range m.Subjects {
sidecarKey := models.BuildKey(m.SubjectType, s.GetObjectMeta().Name, s.GetObjectMeta().Namespace)

selector := labels.SelectorFromSet(labels.Set(getWorkloadSelectorLabels(s)))
selector := labels.SelectorFromSet(m.GetSelectorLabels(s))
if selector.Empty() {
continue
}
Expand All @@ -139,21 +157,21 @@ func (m MultiMatchChecker) multiMatchSidecars() ReferenceMap {
return workloadSidecars
}

func (m MultiMatchChecker) buildSidecarValidations(workloadSidecar ReferenceMap) models.IstioValidations {
func (m GenericMultiMatchChecker) buildSidecarValidations(workloadSidecar ReferenceMap) models.IstioValidations {
validations := models.IstioValidations{}

for wk, scs := range workloadSidecar {
if !workloadSidecar.HasMultipleReferences(wk) {
continue
}

validations.MergeValidations(buildMultipleSidecarsValidation(scs))
validations.MergeValidations(m.buildMultipleSidecarsValidation(scs))
}

return validations
}

func buildMultipleSidecarsValidation(scs []models.IstioValidationKey) models.IstioValidations {
func (m GenericMultiMatchChecker) buildMultipleSidecarsValidation(scs []models.IstioValidationKey) models.IstioValidations {
validations := models.IstioValidations{}

for i, sck := range scs {
Expand All @@ -164,11 +182,11 @@ func buildMultipleSidecarsValidation(scs []models.IstioValidationKey) models.Ist
refs = append(refs, scs[i+1:]...)
}

checks := models.Build("sidecar.multimatch.selector", "spec/workloadSelector")
checks := models.Build(fmt.Sprintf("%s.multimatch.selector", m.SubjectType), m.Path)
validation := models.IstioValidations{
sck: &models.IstioValidation{
Name: sck.Name,
ObjectType: SidecarCheckerType,
ObjectType: m.SubjectType,
Valid: false,
References: refs,
Checks: []*models.IstioCheck{
Expand All @@ -183,8 +201,24 @@ func buildMultipleSidecarsValidation(scs []models.IstioValidationKey) models.Ist
return validations
}

func getWorkloadSelectorLabels(s kubernetes.IstioObject) map[string]string {
ws, found := s.GetSpec()["workloadSelector"]
func GetWorkloadSelectorLabels(s kubernetes.IstioObject) map[string]string {
return getLabels("workloadSelector", "labels", s)
}

func GetSelectorLabels(s kubernetes.IstioObject) map[string]string {
return getLabels("selector", "matchLabels", s)
}

func HasSelector(s kubernetes.IstioObject) bool {
return s.HasMatchLabelsSelector()
}

func HasWorkloadSelector(s kubernetes.IstioObject) bool {
return s.HasWorkloadSelectorLabels()
}

func getLabels(first, second string, s kubernetes.IstioObject) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor one.
I have started to add more comments in the helpers methods in my PRs as well.
Basically for me, but also it helps others to get more clarity and reduce ambiguity what this helper was necessary.

If you can get a couple of comments about that "first, second" means, perhaps it helps reviewer to not enter in the implementation detail to get it.

ws, found := s.GetSpec()[first]
if !found {
return nil
}
Expand All @@ -194,7 +228,7 @@ func getWorkloadSelectorLabels(s kubernetes.IstioObject) map[string]string {
return nil
}

labels, found := wsCasted["labels"]
labels, found := wsCasted[second]
if !found {
return nil
}
Expand Down