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.

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

allow supported resource overrides in the limitranger plugin #23280

Merged
merged 1 commit into from
Mar 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
76 changes: 50 additions & 26 deletions plugin/pkg/admission/limitranger/admission.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,16 @@ const (

func init() {
admission.RegisterPlugin("LimitRanger", func(client clientset.Interface, config io.Reader) (admission.Interface, error) {
return NewLimitRanger(client, Limit)
return NewLimitRanger(client, &DefaultLimitRangerActions{})
})
}

// limitRanger enforces usage limits on a per resource basis in the namespace
type limitRanger struct {
*admission.Handler
client clientset.Interface
limitFunc LimitFunc
indexer cache.Indexer
client clientset.Interface
actions LimitRangerActions
indexer cache.Indexer

// liveLookups holds the last few live lookups we've done to help ammortize cost on repeated lookup failures.
// This let's us handle the case of latent caches, by looking up actual results for a namespace on cache miss/no results.
Expand All @@ -68,14 +68,7 @@ type liveLookupEntry struct {

// Admit admits resources into cluster that do not violate any defined LimitRange in the namespace
func (l *limitRanger) Admit(a admission.Attributes) (err error) {

// Ignore all calls to subresources
if a.GetSubresource() != "" {
return nil
}

// ignore all calls that do not deal with pod resources since that is all this supports now.
if a.GetKind() != api.Kind("Pod") {
if !l.actions.SupportsAttributes(a) {
return nil
}

Expand Down Expand Up @@ -130,7 +123,12 @@ func (l *limitRanger) Admit(a admission.Attributes) (err error) {
// ensure it meets each prescribed min/max
for i := range items {
limitRange := items[i].(*api.LimitRange)
err = l.limitFunc(limitRange, a.GetResource().Resource, a.GetObject())

if !l.actions.SupportsLimit(limitRange) {
continue
}

err = l.actions.Limit(limitRange, a.GetResource().Resource, a.GetObject())
if err != nil {
return admission.NewForbidden(a, err)
}
Expand All @@ -139,7 +137,7 @@ func (l *limitRanger) Admit(a admission.Attributes) (err error) {
}

// NewLimitRanger returns an object that enforces limits based on the supplied limit function
func NewLimitRanger(client clientset.Interface, limitFunc LimitFunc) (admission.Interface, error) {
func NewLimitRanger(client clientset.Interface, actions LimitRangerActions) (admission.Interface, error) {
liveLookupCache, err := lru.New(10000)
if err != nil {
return nil, err
Expand All @@ -155,10 +153,15 @@ func NewLimitRanger(client clientset.Interface, limitFunc LimitFunc) (admission.
}
indexer, reflector := cache.NewNamespaceKeyedIndexerAndReflector(lw, &api.LimitRange{}, 0)
reflector.Run()

if actions == nil {
actions = &DefaultLimitRangerActions{}
}

return &limitRanger{
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
limitFunc: limitFunc,
actions: actions,
indexer: indexer,
liveLookupCache: liveLookupCache,
liveTTL: time.Duration(30 * time.Second),
Expand All @@ -181,17 +184,6 @@ func Max(a int64, b int64) int64 {
return b
}

// Limit enforces resource requirements of incoming resources against enumerated constraints
// on the LimitRange. It may modify the incoming object to apply default resource requirements
// if not specified, and enumerated on the LimitRange
func Limit(limitRange *api.LimitRange, resourceName string, obj runtime.Object) error {
switch resourceName {
case "pods":
return PodLimitFunc(limitRange, obj.(*api.Pod))
}
return nil
}

// defaultContainerResourceRequirements returns the default requirements for a container
// the requirement.Limits are taken from the LimitRange defaults (if specified)
// the requirement.Requests are taken from the LimitRange default request (if specified)
Expand Down Expand Up @@ -383,6 +375,38 @@ func sum(inputs []api.ResourceList) api.ResourceList {
return result
}

// DefaultLimitRangerActions is the default implementatation of LimitRangerActions.
type DefaultLimitRangerActions struct{}

// ensure DefaultLimitRangerActions implements the LimitRangerActions interface.
var _ LimitRangerActions = &DefaultLimitRangerActions{}

// Limit enforces resource requirements of incoming resources against enumerated constraints
// on the LimitRange. It may modify the incoming object to apply default resource requirements
// if not specified, and enumerated on the LimitRange
func (d *DefaultLimitRangerActions) Limit(limitRange *api.LimitRange, resourceName string, obj runtime.Object) error {
switch resourceName {
case "pods":
return PodLimitFunc(limitRange, obj.(*api.Pod))
}
return nil
}

// SupportsAttributes ignores all calls that do not deal with pod resources since that is
// all this supports now. Also ignores any call that has a subresource defined.
func (d *DefaultLimitRangerActions) SupportsAttributes(a admission.Attributes) bool {
if a.GetSubresource() != "" {
Copy link
Member

Choose a reason for hiding this comment

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

are we confident in our downstream use cases that we will not forget to include this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can certainly move it back into the main admit method if you think it's worth it.

Here were the two downstream cases I found:
https://github.com/openshift/origin/pull/7989/files#diff-5fd1547857aea1ef0cdb18cb293f88faR210
https://github.com/openshift/origin/pull/7989/files#diff-c0324af3f9610f91b1fde15a63e35bf5R106

return false
}

return a.GetKind() == api.Kind("Pod")
}

// SupportsLimit always returns true.
func (d *DefaultLimitRangerActions) SupportsLimit(limitRange *api.LimitRange) bool {
return true
}

// PodLimitFunc enforces resource requirements enumerated by the pod against
// the specified LimitRange. The pod may be modified to apply default resource
// requirements if not specified, and enumerated on the LimitRange
Expand Down
14 changes: 7 additions & 7 deletions plugin/pkg/admission/limitranger/admission_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,10 @@ func TestLimitRangerIgnoresSubresource(t *testing.T) {
client := fake.NewSimpleClientset()
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc})
handler := &limitRanger{
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
limitFunc: Limit,
indexer: indexer,
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
actions: &DefaultLimitRangerActions{},
indexer: indexer,
}

limitRange := validLimitRangeNoDefaults()
Expand Down Expand Up @@ -468,7 +468,7 @@ func TestLimitRangerCacheMisses(t *testing.T) {
handler := &limitRanger{
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
limitFunc: Limit,
actions: &DefaultLimitRangerActions{},
indexer: indexer,
liveLookupCache: liveLookupCache,
}
Expand Down Expand Up @@ -502,7 +502,7 @@ func TestLimitRangerCacheAndLRUMisses(t *testing.T) {
handler := &limitRanger{
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
limitFunc: Limit,
actions: &DefaultLimitRangerActions{},
indexer: indexer,
liveLookupCache: liveLookupCache,
}
Expand Down Expand Up @@ -532,7 +532,7 @@ func TestLimitRangerCacheAndLRUExpiredMisses(t *testing.T) {
handler := &limitRanger{
Handler: admission.NewHandler(admission.Create, admission.Update),
client: client,
limitFunc: Limit,
actions: &DefaultLimitRangerActions{},
indexer: indexer,
liveLookupCache: liveLookupCache,
}
Expand Down
13 changes: 11 additions & 2 deletions plugin/pkg/admission/limitranger/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,18 @@ limitations under the License.
package limitranger

import (
"k8s.io/kubernetes/pkg/admission"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/runtime"
)

// LimitFunc is a pluggable function to enforce limits on the object
type LimitFunc func(limitRange *api.LimitRange, kind string, obj runtime.Object) error
type LimitRangerActions interface {
// Limit is a pluggable function to enforce limits on the object.
Limit(limitRange *api.LimitRange, kind string, obj runtime.Object) error
// SupportsAttributes is a pluggable function to allow overridding what resources the limitranger
// supports.
SupportsAttributes(attr admission.Attributes) bool
// SupportsLimit is a pluggable function to allow ignoring limits that should not be applied
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is when a limit is referencing a custom type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, used to filter limit ranges that only apply to images downstream: https://github.com/openshift/origin/pull/7989/files#diff-c0324af3f9610f91b1fde15a63e35bf5R118

// for any reason.
SupportsLimit(limitRange *api.LimitRange) bool
}