Skip to content

Commit

Permalink
Merge pull request #1172 from sanchezl/apirequestcounts-panic
Browse files Browse the repository at this point in the history
Bug 2051985: UPSTREAM: <carry>: An APIRequestCount without dots in the name can cause a panic
  • Loading branch information
openshift-merge-robot committed Apr 11, 2022
2 parents 37c5e75 + e23f142 commit d36f9d8
Show file tree
Hide file tree
Showing 7 changed files with 179 additions and 18 deletions.
@@ -0,0 +1,108 @@
package apirequestcount

import (
"fmt"
"io"
"strings"

apiv1 "github.com/openshift/api/apiserver/v1"
"k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apiserver/pkg/admission"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation"
)

const PluginName = "config.openshift.io/ValidateAPIRequestCount"

// Register registers a plugin
func Register(plugins *admission.Plugins) {
plugins.Register(PluginName, func(config io.Reader) (admission.Interface, error) {
return newValidateAPIRequestCount()
})
}

func newValidateAPIRequestCount() (admission.Interface, error) {
return customresourcevalidation.NewValidator(
map[schema.GroupResource]bool{
apiv1.GroupVersion.WithResource("apirequestcounts").GroupResource(): true,
},
map[schema.GroupVersionKind]customresourcevalidation.ObjectValidator{
apiv1.GroupVersion.WithKind("APIRequestCount"): apiRequestCountV1{},
})
}

type apiRequestCountV1 struct {
}

func toAPIRequestCountV1(uncastObj runtime.Object) (*apiv1.APIRequestCount, field.ErrorList) {
obj, ok := uncastObj.(*apiv1.APIRequestCount)
if !ok {
return nil, field.ErrorList{
field.NotSupported(field.NewPath("kind"), fmt.Sprintf("%T", uncastObj), []string{"APIRequestCount"}),
field.NotSupported(field.NewPath("apiVersion"), fmt.Sprintf("%T", uncastObj), []string{"apiserver.openshift.io/v1"}),
}
}

return obj, nil
}

func (a apiRequestCountV1) ValidateCreate(uncastObj runtime.Object) field.ErrorList {
obj, errs := toAPIRequestCountV1(uncastObj)
if len(errs) > 0 {
return errs
}
errs = append(errs, validation.ValidateObjectMeta(&obj.ObjectMeta, false, requireNameGVR, field.NewPath("metadata"))...)
return errs
}

// requireNameGVR is a name validation function that requires the name to be of the form 'resource.version.group'.
func requireNameGVR(name string, _ bool) []string {
if _, err := NameToResource(name); err != nil {
return []string{err.Error()}
}
return nil
}

// NameToResource parses a name of the form 'resource.version.group'.
func NameToResource(name string) (schema.GroupVersionResource, error) {
segments := strings.SplitN(name, ".", 3)
result := schema.GroupVersionResource{Resource: segments[0]}
switch len(segments) {
case 3:
result.Group = segments[2]
fallthrough
case 2:
result.Version = segments[1]
default:
return schema.GroupVersionResource{}, fmt.Errorf("apirequestcount %s: name must be of the form 'resource.version.group'", name)
}
return result, nil
}

func (a apiRequestCountV1) ValidateUpdate(uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList {
obj, errs := toAPIRequestCountV1(uncastObj)
if len(errs) > 0 {
return errs
}
oldObj, errs := toAPIRequestCountV1(uncastOldObj)
if len(errs) > 0 {
return errs
}
errs = append(errs, validation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata"))...)
return errs
}

func (a apiRequestCountV1) ValidateStatusUpdate(uncastObj runtime.Object, uncastOldObj runtime.Object) field.ErrorList {
obj, errs := toAPIRequestCountV1(uncastObj)
if len(errs) > 0 {
return errs
}
oldObj, errs := toAPIRequestCountV1(uncastOldObj)
if len(errs) > 0 {
return errs
}
errs = append(errs, validation.ValidateObjectMetaUpdate(&obj.ObjectMeta, &oldObj.ObjectMeta, field.NewPath("metadata"))...)
return errs
}
@@ -0,0 +1,34 @@
package apirequestcount

import (
"testing"

apiv1 "github.com/openshift/api/apiserver/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestApiRequestCountV1_ValidateCreate(t *testing.T) {
testCases := []struct {
name string
errExpected bool
}{
{"nogood", true},
{"resource.version", false},
{"resource.groupnonsense", false},
{"resource.version.group", false},
{"resource.version.group.with.dots", false},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
errs := apiRequestCountV1{}.ValidateCreate(&apiv1.APIRequestCount{ObjectMeta: metav1.ObjectMeta{Name: tc.name}})
if tc.errExpected != (len(errs) != 0) {
s := "did not expect "
if tc.errExpected {
s = "expected "
}
t.Errorf("%serrors, but got %d errors: %v", s, len(errs), errs)
}
})
}

}
Expand Up @@ -5,6 +5,7 @@ import (
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apiserver/pkg/admission"

apiv1 "github.com/openshift/api/apiserver/v1"
authorizationv1 "github.com/openshift/api/authorization/v1"
configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
Expand Down Expand Up @@ -52,4 +53,5 @@ func init() {
utilruntime.Must(quotav1.Install(supportedObjectsScheme))
utilruntime.Must(securityv1.Install(supportedObjectsScheme))
utilruntime.Must(authorizationv1.Install(supportedObjectsScheme))
utilruntime.Must(apiv1.Install(supportedObjectsScheme))
}
Expand Up @@ -2,6 +2,7 @@ package customresourcevalidationregistration

import (
"k8s.io/apiserver/pkg/admission"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/apirequestcount"

"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/apiserver"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/authentication"
Expand Down Expand Up @@ -36,6 +37,7 @@ var AllCustomResourceValidators = []string{
securitycontextconstraints.PluginName,
rolebindingrestriction.PluginName,
network.PluginName,
apirequestcount.PluginName,

// the kubecontrollermanager operator resource has to exist in order to run deployments to deploy admission webhooks.
kubecontrollermanager.PluginName,
Expand Down Expand Up @@ -66,6 +68,8 @@ func RegisterCustomResourceValidation(plugins *admission.Plugins) {
rolebindingrestriction.Register(plugins)
// This plugin validates the network.config.openshift.io object for service node port range changes
network.Register(plugins)
// This plugin validates the apiserver.openshift.io/v1 APIRequestCount resources.
apirequestcount.Register(plugins)
// this one is special because we don't work without it.
securitycontextconstraints.RegisterDefaulting(plugins)
}
Expand Up @@ -2,6 +2,7 @@ package deprecatedapirequest

import (
"context"
"fmt"
"math/rand"
"strings"
"sync"
Expand All @@ -14,6 +15,7 @@ import (
"k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/klog/v2"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/apirequestcount"
"k8s.io/kubernetes/openshift-kube-apiserver/filters/deprecatedapirequest/v1helpers"
)

Expand Down Expand Up @@ -128,7 +130,12 @@ func (c *controller) persistRequestCountForAllResources(ctx context.Context, cur
return
}
for _, arc := range arcs.Items {
countsToPersist.Resource(apiNameToResource(arc.Name))
gvr, err := apirequestcount.NameToResource(arc.Name)
if err != nil {
runtime.HandleError(fmt.Errorf("invalid APIRequestCount %s (added manually) should be deleted: %v", arc.Name, err))
continue
}
countsToPersist.Resource(gvr)
}
})

Expand Down Expand Up @@ -208,12 +215,3 @@ func resourceToAPIName(resource schema.GroupVersionResource) string {
}
return apiName
}

func apiNameToResource(name string) schema.GroupVersionResource {
segments := strings.SplitN(name, ".", 3)
result := schema.GroupVersionResource{Resource: segments[0], Version: segments[1]}
if len(segments) > 2 {
result.Group = segments[2]
}
return result
}
Expand Up @@ -18,6 +18,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
"k8s.io/kubernetes/openshift-kube-apiserver/admission/customresourcevalidation/apirequestcount"
)

func TestRemovedRelease(t *testing.T) {
Expand Down Expand Up @@ -607,6 +608,20 @@ func TestPersistRequestCountForAllResources(t *testing.T) {
)),
},
},
{
name: "IgnoreInvalidResourceName",
existing: []runtime.Object{
apiRequestCount("test-v1-invalid"),
apiRequestCount("test.v1.group"),
},
expected: []*apiv1.APIRequestCount{
apiRequestCount("test-v1-invalid"),
apiRequestCount("test.v1.group", withStatus(
withRequestLastHour(withPerNodeAPIRequestLog("node10")),
withRequestLast24hN("0,2-23", withPerNodeAPIRequestLog("node10")),
)),
},
},
{
name: "OnRestart",
existing: []runtime.Object{
Expand Down Expand Up @@ -683,7 +698,6 @@ func TestPersistRequestCountForAllResources(t *testing.T) {
for _, logRequest := range tc.requests {
logRequest(c)
}

c.persistRequestCountForAllResources(ctx, tc.currentHour)

arcs, err := c.client.List(ctx, metav1.ListOptions{})
Expand Down Expand Up @@ -822,20 +836,24 @@ func withRequestN(resource string, hour int, user, agent, verb string, n int) fu
func withRequest(resource string, hour int, user, agent, verb string) func(*controller) {
ts := time.Date(2021, 11, 9, hour, 0, 0, 0, time.UTC)
return func(c *controller) {
c.LogRequest(apiNameToResource(resource), ts, user, agent, verb)
gvr, err := apirequestcount.NameToResource(resource)
if err != nil {
panic(err)
}
c.LogRequest(gvr, ts, user, agent, verb)
}
}

func withPerUserAPIRequestCount(user, userAgent string, options ...func(*apiv1.PerUserAPIRequestCount)) func(*apiv1.PerNodeAPIRequestLog) {
return func(nodeRequestlog *apiv1.PerNodeAPIRequestLog) {
return func(nodeRequestLog *apiv1.PerNodeAPIRequestLog) {
requestUser := &apiv1.PerUserAPIRequestCount{
UserName: user,
UserAgent: userAgent,
}
for _, f := range options {
f(requestUser)
}
nodeRequestlog.ByUser = append(nodeRequestlog.ByUser, *requestUser)
nodeRequestLog.ByUser = append(nodeRequestLog.ByUser, *requestUser)
}
}

Expand Down Expand Up @@ -907,8 +925,6 @@ func apiRequestCountStatus(options ...func(*apiv1.APIRequestCountStatus)) *apiv1
return status
}

const all = "0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23"

func requestLog(options ...func(*apiv1.PerResourceAPIRequestLog)) apiv1.PerResourceAPIRequestLog {
requestLog := &apiv1.PerResourceAPIRequestLog{}
for _, f := range options {
Expand Down
Expand Up @@ -4,7 +4,6 @@ import (
"net/http"

"k8s.io/apimachinery/pkg/runtime/schema"

"k8s.io/apiserver/pkg/endpoints/request"
"k8s.io/kubernetes/openshift-kube-apiserver/filters/deprecatedapirequest"
)
Expand All @@ -14,7 +13,7 @@ func WithDeprecatedApiRequestLogging(handler http.Handler, controller deprecated
handlerFunc := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
defer handler.ServeHTTP(w, req)
info, ok := request.RequestInfoFrom(req.Context())
if !ok {
if !ok || !info.IsResourceRequest {
return
}
timestamp, ok := request.ReceivedTimestampFrom(req.Context())
Expand Down

0 comments on commit d36f9d8

Please sign in to comment.