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

Print warning only instead of error if no permission on ingressclass #7578

Merged
merged 3 commits into from
Oct 10, 2021
Merged

Print warning only instead of error if no permission on ingressclass #7578

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b26dc7e
skip ingressclass check if ingressclass is not enabled
Sep 2, 2021
6b0501d
reformat with gofmt
Sep 2, 2021
f799f16
Merge branch 'main' into main
yong-jie-gong Sep 16, 2021
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
5 changes: 4 additions & 1 deletion cmd/nginx/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,11 @@ func main() {
_, err = kubeClient.NetworkingV1().IngressClasses().List(context.TODO(), metav1.ListOptions{})
if err != nil {
if !errors.IsNotFound(err) {
if errors.IsUnauthorized(err) || !errors.IsForbidden(err) {
if errors.IsUnauthorized(err) {
klog.Fatalf("Error searching IngressClass: Please verify your RBAC and allow Ingress Controller to list and get Ingress Classes: %v", err)
} else if errors.IsForbidden(err) {
Comment on lines +113 to +115
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between unauthorized an forbidden?

When one can happen and other not?

After that clarification, lgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

@yongjie-gong friendly reminder

Copy link
Contributor

Choose a reason for hiding this comment

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

So the difference here is code 401 (user not authenticated) vs 403 (user without permission to do that action).

We should rely on 403 always, but anyway seems like a good safeguard also to look into 401.

I'm going to merge this, but improve the logics on a follow up PR, leaving both 401 and 403 as situations to ignore IngressClass and adding some e2e tests for this.

klog.Warningf("No permissions to list and get Ingress Classes: %v, IngressClass feature will be disabled", err)
conf.IngressClassConfiguration.IgnoreIngressClass = true
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions internal/ingress/controller/ingressclass/ingressclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,7 @@ type IngressClassConfiguration struct {
// WatchWithoutClass defines if Controller should watch to Ingress Objects that does
// not contain an IngressClass configuration
WatchWithoutClass bool
// IgnoreIngressClass defines if Controller should ignore the IngressClass Object if no permissions are
// granted on IngressClass
IgnoreIngressClass bool
}
32 changes: 22 additions & 10 deletions internal/ingress/controller/store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,21 +151,25 @@ func (e NotExistsError) Error() string {
func (i *Informer) Run(stopCh chan struct{}) {
go i.Secret.Run(stopCh)
go i.Endpoint.Run(stopCh)
go i.IngressClass.Run(stopCh)
if i.IngressClass != nil {
go i.IngressClass.Run(stopCh)
}
go i.Service.Run(stopCh)
go i.ConfigMap.Run(stopCh)

// wait for all involved caches to be synced before processing items
// from the queue
if !cache.WaitForCacheSync(stopCh,
i.Endpoint.HasSynced,
i.IngressClass.HasSynced,
i.Endpoint.HasSynced,
i.Service.HasSynced,
i.Secret.HasSynced,
i.ConfigMap.HasSynced,
) {
runtime.HandleError(fmt.Errorf("timed out waiting for caches to sync"))
}
if i.IngressClass != nil && !cache.WaitForCacheSync(stopCh, i.IngressClass.HasSynced) {
runtime.HandleError(fmt.Errorf("timed out waiting for ingress classcaches to sync"))
}

// in big clusters, deltas can keep arriving even after HasSynced
// functions have returned 'true'
Expand Down Expand Up @@ -299,9 +303,11 @@ func New(

store.informers.Ingress = infFactory.Networking().V1().Ingresses().Informer()
store.listers.Ingress.Store = store.informers.Ingress.GetStore()

store.informers.IngressClass = infFactory.Networking().V1().IngressClasses().Informer()
store.listers.IngressClass.Store = cache.NewStore(cache.MetaNamespaceKeyFunc)

if !icConfig.IgnoreIngressClass {
store.informers.IngressClass = infFactory.Networking().V1().IngressClasses().Informer()
store.listers.IngressClass.Store = cache.NewStore(cache.MetaNamespaceKeyFunc)
}

store.informers.Endpoint = infFactory.Core().V1().Endpoints().Informer()
store.listers.Endpoint.Store = store.informers.Endpoint.GetStore()
Expand Down Expand Up @@ -385,8 +391,12 @@ func New(
oldIng, _ := toIngress(old)
curIng, _ := toIngress(cur)

_, errOld := store.GetIngressClass(oldIng, icConfig)
classCur, errCur := store.GetIngressClass(curIng, icConfig)
var errOld, errCur error
var classCur string
if !icConfig.IgnoreIngressClass {
_, errOld = store.GetIngressClass(oldIng, icConfig)
classCur, errCur = store.GetIngressClass(curIng, icConfig)
}
if errOld != nil && errCur == nil {
if hasCatchAllIngressRule(curIng.Spec) && disableCatchAll {
klog.InfoS("ignoring update for catch-all ingress because of --disable-catch-all", "ingress", klog.KObj(curIng))
Expand Down Expand Up @@ -676,7 +686,9 @@ func New(
}

store.informers.Ingress.AddEventHandler(ingEventHandler)
store.informers.IngressClass.AddEventHandler(ingressClassEventHandler)
if !icConfig.IgnoreIngressClass {
store.informers.IngressClass.AddEventHandler(ingressClassEventHandler)
}
store.informers.Endpoint.AddEventHandler(epEventHandler)
store.informers.Secret.AddEventHandler(secrEventHandler)
store.informers.ConfigMap.AddEventHandler(cmEventHandler)
Expand Down Expand Up @@ -829,7 +841,7 @@ func (s *k8sStore) GetService(key string) (*corev1.Service, error) {

func (s *k8sStore) GetIngressClass(ing *networkingv1.Ingress, icConfig *ingressclass.IngressClassConfiguration) (string, error) {
// First we try ingressClassName
if ing.Spec.IngressClassName != nil {
if !icConfig.IgnoreIngressClass && ing.Spec.IngressClassName != nil {
iclass, err := s.listers.IngressClass.ByKey(*ing.Spec.IngressClassName)
if err != nil {
return "", err
Expand Down