diff --git a/Makefile b/Makefile index 5d5e2ce2..a61a6443 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ NAME := popeye PACKAGE := github.com/derailed/$(NAME) -VERSION := v0.20.4 +VERSION := v0.20.5 GIT := $(shell git rev-parse --short HEAD) DATE := $(shell date +%FT%T%Z) IMG_NAME := derailed/popeye diff --git a/change_logs/release_v0.20.5.md b/change_logs/release_v0.20.5.md new file mode 100644 index 00000000..28d230eb --- /dev/null +++ b/change_logs/release_v0.20.5.md @@ -0,0 +1,27 @@ + + +# Release v0.20.5 + +## Notes + +Thank you to all that contributed with flushing out issues and enhancements for Popeye! I'll try to mark some of these issues as fixed. But if you don't mind grab the latest rev and see if we're happier with some of the fixes! If you've filed an issue please help me verify and close. Your support, kindness and awesome suggestions to make Popeye better is as ever very much noticed and appreciated! + +This project offers a GitHub Sponsor button (over here 👆). As you well know this is not pimped out by big corps with deep pockets. If you feel `Popeye` is saving you cycles diagnosing potential cluster issues please consider sponsoring this project!! It does go a long way in keeping our servers lights on and beers in our fridge. + +Also if you dig this tool, please make some noise on social! [@kitesurfer](https://twitter.com/kitesurfer) + +--- + +## Maintenance Release + +--- + +## Resolved Issues + +. [#290](https://github.com/derailed/popeye/issues/290) Secrets is not scanned on Openshift 4.12 bug +. [#289](https://github.com/derailed/popeye/issues/289) "No associated endpoint" (POP-1105) reported for service question +. [#288](https://github.com/derailed/popeye/issues/288) A Score granted despite no resources being scanned + +--- + +  © 2024 Imhotep Software LLC. All materials licensed under [Apache v2.0](http://www.apache.org/licenses/LICENSE-2.0) diff --git a/internal/client/client.go b/internal/client/client.go index a8f61d7d..9047cc9a 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -64,14 +64,18 @@ func InitConnectionOrDie(config types.Config) (*APIClient, error) { config: config, cache: cache.NewLRUExpireCache(cacheSize), } + _, err := a.serverGroups() + if err != nil { + return nil, err + } if err := a.supportsMetricsResources(); err != nil { - log.Warn().Msgf("no metrics server detected %s", err.Error()) + log.Warn().Err(err).Msgf("no metrics server detected") } return &a, nil } -func makeSAR(ns string, gvr types.GVR) *authorizationv1.SelfSubjectAccessReview { +func makeSAR(ns string, gvr types.GVR, n string) *authorizationv1.SelfSubjectAccessReview { if ns == "-" { ns = "" } @@ -83,13 +87,14 @@ func makeSAR(ns string, gvr types.GVR) *authorizationv1.SelfSubjectAccessReview Group: res.Group, Resource: res.Resource, Subresource: gvr.SubResource(), + Name: n, }, }, } } -func makeCacheKey(ns, gvr string, vv []string) string { - return ns + ":" + gvr + "::" + strings.Join(vv, ",") +func makeCacheKey(ns string, gvr types.GVR, n string, vv []string) string { + return ns + ":" + gvr.String() + ":" + n + "::" + strings.Join(vv, ",") } // ActiveContext returns the current context name. @@ -146,11 +151,11 @@ func (a *APIClient) ConnectionOK() bool { } // CanI checks if user has access to a certain resource. -func (a *APIClient) CanI(ns string, gvr types.GVR, verbs ...string) (auth bool, err error) { +func (a *APIClient) CanI(ns string, gvr types.GVR, n string, verbs []string) (auth bool, err error) { if IsClusterWide(ns) { - ns = AllNamespaces + ns = BlankNamespace } - key := makeCacheKey(ns, gvr.String(), verbs) + key := makeCacheKey(ns, gvr, n, verbs) if v, ok := a.cache.Get(key); ok { if auth, ok = v.(bool); ok { return auth, nil @@ -161,7 +166,7 @@ func (a *APIClient) CanI(ns string, gvr types.GVR, verbs ...string) (auth bool, if err != nil { return false, err } - dial, sar := c.AuthorizationV1().SelfSubjectAccessReviews(), makeSAR(ns, gvr) + dial, sar := c.AuthorizationV1().SelfSubjectAccessReviews(), makeSAR(ns, gvr, n) ctx, cancel := context.WithTimeout(context.Background(), CallTimeout) defer cancel() for _, v := range verbs { @@ -357,30 +362,38 @@ func (a *APIClient) checkCacheBool(key string) (state bool, ok bool) { return } +func (a *APIClient) serverGroups() (*metav1.APIGroupList, error) { + dial, err := a.CachedDiscovery() + if err != nil { + log.Warn().Err(err).Msgf("Unable to dial discovery API") + return nil, err + } + apiGroups, err := dial.ServerGroups() + if err != nil { + log.Warn().Err(err).Msgf("Unable to retrieve server groups") + return nil, fmt.Errorf("unable to fetch server groups: %w", err) + } + + return apiGroups, nil +} + func (a *APIClient) supportsMetricsResources() error { supported, ok := a.checkCacheBool(cacheMXAPIKey) if ok { if supported { return nil } - return errors.New("No metrics-server detected") + return errors.New("no metrics-server detected") } - defer func() { a.cache.Add(cacheMXAPIKey, supported, cacheExpiry) }() - dial, err := a.CachedDiscovery() - if err != nil { - log.Warn().Err(err).Msgf("Unable to dial discovery API") - return err - } - apiGroups, err := dial.ServerGroups() + gg, err := a.serverGroups() if err != nil { - log.Warn().Err(err).Msgf("Unable to retrieve server groups") return err } - for _, grp := range apiGroups.Groups { + for _, grp := range gg.Groups { if grp.Name != metricsapi.GroupName { continue } @@ -390,8 +403,9 @@ func (a *APIClient) supportsMetricsResources() error { } } - return errors.New("No metrics-server detected") + return errors.New("no metrics-server detected") } + func checkMetricsVersion(grp metav1.APIGroup) bool { for _, version := range grp.Versions { for _, supportedVersion := range supportedMetricsAPIVersions { diff --git a/internal/client/config.go b/internal/client/config.go index 85cd1f3f..a4c2f40d 100644 --- a/internal/client/config.go +++ b/internal/client/config.go @@ -234,7 +234,7 @@ func (c *Config) CurrentUserName() (string, error) { // CurrentNamespaceName retrieves the active namespace. func (c *Config) CurrentNamespaceName() (string, error) { - if c.flags.Namespace != nil { + if isSet(c.flags.Namespace) { return *c.flags.Namespace, nil } diff --git a/internal/client/factory.go b/internal/client/factory.go index 99d40804..5942f20d 100644 --- a/internal/client/factory.go +++ b/internal/client/factory.go @@ -50,7 +50,7 @@ func (f *Factory) Start(ns string) { } } -// Terminate stops the factory. +// Terminate terminates all watchers and forwards. func (f *Factory) Terminate() { f.mx.Lock() defer f.mx.Unlock() @@ -66,48 +66,68 @@ func (f *Factory) Terminate() { // List returns a resource collection. func (f *Factory) List(gvr types.GVR, ns string, wait bool, labels labels.Selector) ([]runtime.Object, error) { - inf, err := f.CanForResource(ns, gvr, types.MonitorAccess...) + inf, err := f.CanForResource(ns, gvr, types.ListAccess) if err != nil { return nil, err } - if wait { - f.waitForCacheSync(ns) + if IsAllNamespace(ns) { + ns = BlankNamespace } + + var oo []runtime.Object if IsClusterScoped(ns) { - return inf.Lister().List(labels) + oo, err = inf.Lister().List(labels) + } else { + oo, err = inf.Lister().ByNamespace(ns).List(labels) + } + if !wait || (wait && inf.Informer().HasSynced()) { + return oo, err } - if IsAllNamespace(ns) { - ns = AllNamespaces + f.waitForCacheSync(ns) + if IsClusterScoped(ns) { + return inf.Lister().List(labels) } return inf.Lister().ByNamespace(ns).List(labels) } +// HasSynced checks if given informer is up to date. +func (f *Factory) HasSynced(gvr types.GVR, ns string) (bool, error) { + inf, err := f.CanForResource(ns, gvr, types.ListAccess) + if err != nil { + return false, err + } + + return inf.Informer().HasSynced(), nil +} + // Get retrieves a given resource. -func (f *Factory) Get(gvr types.GVR, path string, wait bool, sel labels.Selector) (runtime.Object, error) { - ns, n := Namespaced(path) - inf, err := f.CanForResource(ns, gvr, types.GetVerb) +func (f *Factory) Get(gvr types.GVR, fqn string, wait bool, sel labels.Selector) (runtime.Object, error) { + ns, n := Namespaced(fqn) + inf, err := f.CanForResource(ns, gvr, []string{types.GetVerb}) if err != nil { return nil, err } - - if wait { - f.waitForCacheSync(ns) + var o runtime.Object + if IsClusterScoped(ns) { + o, err = inf.Lister().Get(n) + } else { + o, err = inf.Lister().ByNamespace(ns).Get(n) + } + if !wait || (wait && inf.Informer().HasSynced()) { + return o, err } + + f.waitForCacheSync(ns) if IsClusterScoped(ns) { return inf.Lister().Get(n) } - return inf.Lister().ByNamespace(ns).Get(n) } func (f *Factory) waitForCacheSync(ns string) { if IsClusterWide(ns) { - ns = AllNamespaces - } - - if f.isClusterWide() { - ns = AllNamespaces + ns = BlankNamespace } f.mx.RLock() @@ -131,7 +151,7 @@ func (f *Factory) WaitForCacheSync() { for ns, fac := range f.factories { m := fac.WaitForCacheSync(f.stopChan) for k, v := range m { - log.Debug().Msgf("CACHE %q synched %t:%s", ns, v, k) + log.Debug().Msgf("CACHE `%q Loaded %t:%s", ns, v, k) } } } @@ -148,33 +168,24 @@ func (f *Factory) FactoryFor(ns string) di.DynamicSharedInformerFactory { // SetActiveNS sets the active namespace. func (f *Factory) SetActiveNS(ns string) error { - if !f.isClusterWide() { - if _, err := f.ensureFactory(ns); err != nil { - return err - } + if f.isClusterWide() { + return nil } - - return nil + _, err := f.ensureFactory(ns) + return err } func (f *Factory) isClusterWide() bool { f.mx.RLock() defer f.mx.RUnlock() + _, ok := f.factories[BlankNamespace] - _, ok := f.factories[AllNamespaces] return ok } // CanForResource return an informer is user has access. -func (f *Factory) CanForResource(ns string, gvr types.GVR, verbs ...string) (informers.GenericInformer, error) { - // If user can access resource cluster wide, prefer cluster wide factory. - if !IsClusterWide(ns) { - auth, err := f.Client().CanI(AllNamespaces, gvr, verbs...) - if auth && err == nil { - return f.ForResource(AllNamespaces, gvr) - } - } - auth, err := f.Client().CanI(ns, gvr, verbs...) +func (f *Factory) CanForResource(ns string, gvr types.GVR, verbs []string) (informers.GenericInformer, error) { + auth, err := f.Client().CanI(ns, gvr, "", verbs) if err != nil { return nil, err } @@ -206,13 +217,14 @@ func (f *Factory) ForResource(ns string, gvr types.GVR) (informers.GenericInform func (f *Factory) ensureFactory(ns string) (di.DynamicSharedInformerFactory, error) { if IsClusterWide(ns) { - ns = AllNamespaces + ns = BlankNamespace } f.mx.Lock() defer f.mx.Unlock() if fac, ok := f.factories[ns]; ok { return fac, nil } + dial, err := f.client.DynDial() if err != nil { return nil, err diff --git a/internal/client/helpers.go b/internal/client/helpers.go index f150576d..7e1892ed 100644 --- a/internal/client/helpers.go +++ b/internal/client/helpers.go @@ -17,13 +17,13 @@ var toFileName = regexp.MustCompile(`[^(\w/\.)]`) // IsClusterWide returns true if ns designates cluster scope, false otherwise. func IsClusterWide(ns string) bool { - return ns == NamespaceAll || ns == AllNamespaces || ns == ClusterScope + return ns == NamespaceAll || ns == BlankNamespace || ns == ClusterScope } // CleanseNamespace ensures all ns maps to blank. func CleanseNamespace(ns string) string { if IsAllNamespace(ns) { - return AllNamespaces + return BlankNamespace } return ns @@ -36,7 +36,7 @@ func IsAllNamespace(ns string) bool { // IsAllNamespaces returns true if all namespaces, false otherwise. func IsAllNamespaces(ns string) bool { - return ns == NamespaceAll || ns == AllNamespaces + return ns == NamespaceAll || ns == BlankNamespace } // IsNamespaced returns true if a specific ns is given. diff --git a/internal/db/db.go b/internal/db/db.go index cc623b07..067451bf 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -10,6 +10,7 @@ import ( "github.com/derailed/popeye/internal" "github.com/derailed/popeye/internal/client" + "github.com/derailed/popeye/internal/db/schema" "github.com/derailed/popeye/types" "github.com/hashicorp/go-memdb" batchv1 "k8s.io/api/batch/v1" @@ -146,12 +147,25 @@ func (db *DB) Find(kind types.GVR, fqn string) (any, error) { defer txn.Abort() o, err := txn.First(kind.String(), "id", fqn) if err != nil || o == nil { + log.Error().Err(err).Msgf("db.find unable to find object: [%s]%s", kind, fqn) return nil, fmt.Errorf("object not found: %q", fqn) } return o, nil } +func (db *DB) Dump(gvr types.GVR) { + txn, it := db.MustITFor(gvr) + defer txn.Abort() + + log.Debug().Msgf("> Dumping %q", gvr) + for o := it.Next(); o != nil; o = it.Next() { + m := o.(schema.MetaAccessor) + log.Debug().Msgf(" o %s/%s", m.GetNamespace(), m.GetName()) + } + log.Debug().Msg("< Done") +} + func (db *DB) FindPod(ns string, sel map[string]string) (*v1.Pod, error) { txn := db.Txn(false) defer txn.Abort() @@ -273,21 +287,6 @@ func (db *DB) FindNSBySel(sel *metav1.LabelSelector) ([]*v1.Namespace, error) { return nss, nil } -func (db *DB) DumpNS() error { - txn := db.Txn(false) - defer txn.Abort() - it, err := txn.Get(internal.Glossary[internal.NS].String(), "id") - if err != nil { - return err - } - for o := it.Next(); o != nil; o = it.Next() { - ns, _ := o.(*v1.Namespace) - log.Debug().Msgf("NS %q", ns.Name) - } - - return nil -} - func (db *DB) FindNS(ns string) (*v1.Namespace, error) { txn := db.Txn(false) defer txn.Abort() diff --git a/internal/db/loader.go b/internal/db/loader.go index 7aa47124..5a25e50e 100644 --- a/internal/db/loader.go +++ b/internal/db/loader.go @@ -172,18 +172,10 @@ func (l *Loader) fetchNodesMetrics(c types.Connection) (*mv1beta1.NodeMetricsLis func loadResource(ctx context.Context, gvr types.GVR) ([]runtime.Object, error) { f := mustExtractFactory(ctx) - if strings.Contains(gvr.String(), "metrics") { - if !f.Client().HasMetrics() { - return nil, nil - } - } - if gvr.IsMetricsRes() { - var res dao.Generic - res.Init(f, gvr) - return res.List(ctx) + if strings.Contains(gvr.String(), "metrics") && !f.Client().HasMetrics() { + return nil, nil } - - var res dao.Resource + var res dao.Generic res.Init(f, gvr) return res.List(ctx) diff --git a/internal/issues/assets/codes.yaml b/internal/issues/assets/codes.yaml index cfd6917b..fc16368d 100644 --- a/internal/issues/assets/codes.yaml +++ b/internal/issues/assets/codes.yaml @@ -258,7 +258,7 @@ codes: message: Do you mean it? Type NodePort detected severity: 1 1105: - message: No associated endpoints + message: No associated endpoints found severity: 3 1106: message: No target ports match service port %s @@ -270,7 +270,7 @@ codes: message: NodePort detected but service sets externalTrafficPolicy to "Local" severity: 1 1109: - message: Only one Pod associated with this endpoint + message: Single endpoint is associated with this service severity: 2 1110: message: Match EP has no subsets diff --git a/internal/lint/svc.go b/internal/lint/svc.go index 1712b8c3..5d81825a 100644 --- a/internal/lint/svc.go +++ b/internal/lint/svc.go @@ -106,7 +106,7 @@ func (s *Service) checkEndpoints(ctx context.Context, fqn string, kind v1.Servic } o, err := s.db.Find(internal.Glossary[internal.EP], fqn) - if err != nil || o == nil { + if err != nil { s.AddCode(ctx, 1105) return } @@ -115,14 +115,9 @@ func (s *Service) checkEndpoints(ctx context.Context, fqn string, kind v1.Servic s.AddCode(ctx, 1110) return } - numEndpointAddresses := 0 - for _, s := range ep.Subsets { - numEndpointAddresses += len(s.Addresses) - if numEndpointAddresses > 1 { - return - } + if len(ep.Subsets) == 1 { + s.AddCode(ctx, 1109) } - s.AddCode(ctx, 1109) } // ---------------------------------------------------------------------------- diff --git a/internal/lint/svc_test.go b/internal/lint/svc_test.go index 76e66cdb..ebbef2be 100644 --- a/internal/lint/svc_test.go +++ b/internal/lint/svc_test.go @@ -47,7 +47,7 @@ func Test_svcCheckEndpoints(t *testing.T) { Group: "__root__", GVR: "v1/services", Level: rules.ErrorLevel, - Message: "[POP-1105] No associated endpoints", + Message: "[POP-1105] No associated endpoints found", }, }, }, @@ -62,7 +62,7 @@ func Test_svcCheckEndpoints(t *testing.T) { Group: "__root__", GVR: "v1/services", Level: rules.ErrorLevel, - Message: "[POP-1105] No associated endpoints", + Message: "[POP-1105] No associated endpoints found", }, }, }, @@ -74,7 +74,7 @@ func Test_svcCheckEndpoints(t *testing.T) { Group: "__root__", GVR: "v1/services", Level: rules.WarnLevel, - Message: "[POP-1109] Only one Pod associated with this endpoint", + Message: "[POP-1109] Single endpoint is associated with this service", }, }, }, diff --git a/pkg/config/config.go b/pkg/config/config.go index 3bd9dde8..bb5b6a52 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -47,7 +47,7 @@ func NewConfig(flags *Flags) (*Config, error) { flags.Namespace = nil } if flags.AllNamespaces != nil && *flags.AllNamespaces { - all := client.AllNamespaces + all := client.NamespaceAll flags.Namespace = &all } cfg.LintLevel = int(rules.ToIssueLevel(flags.LintLevel)) diff --git a/pkg/popeye.go b/pkg/popeye.go index 072ad2ce..54c0adf9 100644 --- a/pkg/popeye.go +++ b/pkg/popeye.go @@ -97,6 +97,7 @@ func (p *Popeye) Init() error { return err } } + if err := p.aliases.Init(p.client()); err != nil { return err } @@ -144,7 +145,7 @@ func (p *Popeye) initFactory() error { log.Debug().Msgf("Skipping linter %q", k) continue } - ok, err := clt.CanI(client.AllNamespaces, gvr, types.ReadAllAccess...) + ok, err := clt.CanI(client.AllNamespaces, gvr, "", types.ReadAllAccess) if !ok || err != nil { return fmt.Errorf("current user does not have read access for resource %q -- %w", gvr, err) } @@ -198,7 +199,7 @@ func (p *Popeye) buildCtx(ctx context.Context) context.Context { } ns, err := p.client().Config().CurrentNamespaceName() if err != nil { - ns = client.AllNamespaces + ns = client.DefaultNamespace } ctx = context.WithValue(ctx, internal.KeyNamespace, ns) diff --git a/types/types.go b/types/types.go index d8cb98e7..bb2f7fd4 100644 --- a/types/types.go +++ b/types/types.go @@ -58,7 +58,7 @@ type NamespaceNames map[string]struct{} // Authorizer checks what a user can or cannot do to a resource. type Authorizer interface { // CanI returns true if the user can use these actions for a given resource. - CanI(string, GVR, ...string) (bool, error) + CanI(string, GVR, string, []string) (bool, error) } // Config represents an api server configuration. @@ -144,7 +144,7 @@ type Factory interface { ForResource(string, GVR) (informers.GenericInformer, error) // CanForResource fetch an informer for a given resource if authorized - CanForResource(string, GVR, ...string) (informers.GenericInformer, error) + CanForResource(string, GVR, []string) (informers.GenericInformer, error) // WaitForCacheSync synchronize the cache. WaitForCacheSync()