Skip to content

Commit

Permalink
Merge pull request #1928 from jbartosik/only-top-level-controller-cp
Browse files Browse the repository at this point in the history 
Validate that targetRef if set is a top level controller
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Apr 19, 2019
2 parents 3757233 + 56a161a commit 0908d09
Show file tree
Hide file tree
Showing 6 changed files with 636 additions and 15 deletions.
46 changes: 42 additions & 4 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"fmt"
"time"

"github.com/golang/glog"
apiv1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
Expand All @@ -30,6 +29,7 @@ import (
vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned"
vpa_api "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/typed/autoscaling.k8s.io/v1beta2"
vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1beta2"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/history"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/metrics"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/oom"
Expand Down Expand Up @@ -84,6 +84,7 @@ type ClusterStateFeederFactory struct {
OOMObserver oom.Observer
LegacySelectorFetcher target.VpaTargetSelectorFetcher
SelectorFetcher target.VpaTargetSelectorFetcher
ControllerFetcher controllerfetcher.ControllerFetcher
}

// Make creates new ClusterStateFeeder with internal data providers, based on kube client.
Expand All @@ -98,6 +99,7 @@ func (m ClusterStateFeederFactory) Make() *clusterStateFeeder {
specClient: spec.NewSpecClient(m.PodLister),
legacySelectorFetcher: m.LegacySelectorFetcher,
selectorFetcher: m.SelectorFetcher,
controllerFetcher: m.ControllerFetcher,
}
}

Expand All @@ -107,6 +109,7 @@ func NewClusterStateFeeder(config *rest.Config, clusterState *model.ClusterState
kubeClient := kube_client.NewForConfigOrDie(config)
podLister, oomObserver := NewPodListerAndOOMObserver(kubeClient)
factory := informers.NewSharedInformerFactory(kubeClient, defaultResyncPeriod)
controllerFetcher := controllerfetcher.NewControllerFetcher(config, kubeClient, factory)
return ClusterStateFeederFactory{
PodLister: podLister,
OOMObserver: oomObserver,
Expand All @@ -117,6 +120,7 @@ func NewClusterStateFeeder(config *rest.Config, clusterState *model.ClusterState
ClusterState: clusterState,
LegacySelectorFetcher: target.NewBeta1TargetSelectorFetcher(config),
SelectorFetcher: target.NewVpaTargetSelectorFetcher(config, kubeClient, factory),
ControllerFetcher: controllerFetcher,
}.Make()
}

Expand Down Expand Up @@ -195,6 +199,7 @@ type clusterStateFeeder struct {
clusterState *model.ClusterState
legacySelectorFetcher target.VpaTargetSelectorFetcher
selectorFetcher target.VpaTargetSelectorFetcher
controllerFetcher controllerfetcher.ControllerFetcher
}

func (feeder *clusterStateFeeder) InitFromHistoryProvider(historyProvider history.HistoryProvider) {
Expand Down Expand Up @@ -315,7 +320,7 @@ func (feeder *clusterStateFeeder) LoadVPAs() {
VpaName: vpaCRD.Name}

selector, conditions := feeder.getSelector(vpaCRD)
glog.Infof("Using selector %s for VPA %s/%s", selector.String(), vpaCRD.Namespace, vpaCRD.Name)
klog.Infof("Using selector %s for VPA %s/%s", selector.String(), vpaCRD.Namespace, vpaCRD.Name)

if feeder.clusterState.AddOrUpdateVpa(vpaCRD, selector) == nil {
// Successfully added VPA to the model.
Expand Down Expand Up @@ -422,14 +427,40 @@ type condition struct {
message string
}

func (feeder *clusterStateFeeder) validateTargetRef(vpa *vpa_types.VerticalPodAutoscaler) (bool, condition) {
//
if vpa.Spec.TargetRef == nil {
return false, condition{}
}
k := controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Namespace: vpa.Namespace,
Kind: vpa.Spec.TargetRef.Kind,
Name: vpa.Spec.TargetRef.Name,
},
ApiVersion: vpa.Spec.TargetRef.APIVersion,
}
top, err := feeder.controllerFetcher.FindTopLevel(&k)
if err != nil {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Error checking if target is a top level controller: %s", err)}
}
if top == nil {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: fmt.Sprintf("Unknown error during checking if target is a top level controller: %s", err)}
}
if *top != k {
return false, condition{conditionType: vpa_types.ConfigUnsupported, delete: false, message: "The targetRef controller has a parent but it should point to a top-level controller"}
}
return true, condition{}
}

func (feeder *clusterStateFeeder) getSelector(vpa *vpa_types.VerticalPodAutoscaler) (labels.Selector, []condition) {
legacySelector, fetchLegacyErr := feeder.legacySelectorFetcher.Fetch(vpa)
if fetchLegacyErr != nil {
glog.Errorf("Error while fetching legacy selector. Reason: %+v", fetchLegacyErr)
klog.Errorf("Error while fetching legacy selector. Reason: %+v", fetchLegacyErr)
}
selector, fetchErr := feeder.selectorFetcher.Fetch(vpa)
if fetchErr != nil {
glog.Errorf("Cannot get target selector from VPA's targetRef. Reason: %+v", fetchErr)
klog.Errorf("Cannot get target selector from VPA's targetRef. Reason: %+v", fetchErr)
}
if selector != nil {
if legacySelector != nil {
Expand All @@ -438,6 +469,13 @@ func (feeder *clusterStateFeeder) getSelector(vpa *vpa_types.VerticalPodAutoscal
{conditionType: vpa_types.ConfigDeprecated, delete: true},
}
}
validTargetRef, unsupportedCondition := feeder.validateTargetRef(vpa)
if !validTargetRef {
return labels.Nothing(), []condition{
unsupportedCondition,
{conditionType: vpa_types.ConfigDeprecated, delete: true},
}
}
return selector, []condition{
{conditionType: vpa_types.ConfigUnsupported, delete: true},
{conditionType: vpa_types.ConfigDeprecated, delete: true},
Expand Down
131 changes: 124 additions & 7 deletions vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ package input

import (
"fmt"
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/input/controller_fetcher"
"testing"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"

"github.com/golang/mock/gomock"
"k8s.io/api/autoscaling/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1beta2"
Expand All @@ -31,6 +33,15 @@ import (
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test"
)

type fakeControllerFetcher struct {
key *controllerfetcher.ControllerKeyWithAPIVersion
err error
}

func (f *fakeControllerFetcher) FindTopLevel(controller *controllerfetcher.ControllerKeyWithAPIVersion) (*controllerfetcher.ControllerKeyWithAPIVersion, error) {
return f.key, f.err
}

func parseLabelSelector(selector string) labels.Selector {
labelSelector, _ := metav1.ParseToLabelSelector(selector)
parsedSelector, _ := metav1.LabelSelectorAsSelector(labelSelector)
Expand All @@ -42,21 +53,37 @@ var (
unsupportedConditionTextFromFetcher = "Cannot read targetRef. Reason: targetRef not defined"
unsupportedConditionNoExtraText = "Cannot read targetRef"
unsupportedConditionBothDefined = "Both targetRef and label selector defined. Please remove label selector"
unsupportedConditionNoTargetRef = "Cannot read targetRef"
unsupportedConditionMudaMudaMuda = "Error checking if target is a top level controller: muda muda muda"
unsupportedTargetRefHasParent = "The targetRef controller has a parent but it should point to a top-level controller"
)

const (
kind = "dodokind"
name1 = "dotaro"
name2 = "doseph"
namespace = "testNamespace"
apiVersion = "stardust"
)

func TestLegacySelector(t *testing.T) {

type testCase struct {
name string
legacySelector labels.Selector
selector labels.Selector
fetchSelectorError error
targetRef *v1.CrossVersionObjectReference
topLevelKey *controllerfetcher.ControllerKeyWithAPIVersion
findTopLevelError error
expectedSelector labels.Selector
expectedConfigUnsupported *string
expectedConfigDeprecated *string
}

testCases := []testCase{
{
name: "no selector",
legacySelector: nil,
selector: nil,
fetchSelectorError: fmt.Errorf("targetRef not defined"),
Expand All @@ -65,6 +92,7 @@ func TestLegacySelector(t *testing.T) {
expectedConfigDeprecated: nil,
},
{
name: "also no selector but no error",
legacySelector: nil,
selector: nil,
fetchSelectorError: nil,
Expand All @@ -73,37 +101,122 @@ func TestLegacySelector(t *testing.T) {
expectedConfigDeprecated: nil,
},
{
name: "legacy selector no ref",
legacySelector: parseLabelSelector("app = test"),
selector: nil,
fetchSelectorError: fmt.Errorf("targetRef not defined"),
expectedSelector: labels.Nothing(),
expectedConfigUnsupported: &unsupportedConditionNoLongerSupported,
expectedConfigDeprecated: nil,
}, {
name: "targetRef selector",
// the only valid option since v1beta1 removal
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
targetRef: &v1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name1,
Namespace: namespace,
},
ApiVersion: apiVersion,
},
expectedSelector: parseLabelSelector("app = test"),
expectedConfigUnsupported: nil,
expectedConfigDeprecated: nil,
}, {
name: "new and legacy selector",
legacySelector: parseLabelSelector("app = test1"),
selector: parseLabelSelector("app = test2"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
expectedConfigUnsupported: &unsupportedConditionBothDefined,
expectedConfigDeprecated: nil,
},
{
name: "can't decide if top-level-ref",
legacySelector: nil,
selector: nil,
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
},
expectedConfigUnsupported: &unsupportedConditionNoTargetRef,
},
{
name: "non-top-level targetRef",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name2,
Namespace: namespace,
},
ApiVersion: apiVersion,
},
expectedConfigUnsupported: &unsupportedTargetRefHasParent,
},
{
name: "error checking if top-level-ref",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: labels.Nothing(),
targetRef: &v1.CrossVersionObjectReference{
Kind: "doestar",
Name: "doseph-doestar",
APIVersion: "taxonomy",
},
expectedConfigUnsupported: &unsupportedConditionMudaMudaMuda,
findTopLevelError: fmt.Errorf("muda muda muda"),
},
{
name: "top-level target ref",
legacySelector: nil,
selector: parseLabelSelector("app = test"),
fetchSelectorError: nil,
expectedSelector: parseLabelSelector("app = test"),
targetRef: &v1.CrossVersionObjectReference{
Kind: kind,
Name: name1,
APIVersion: apiVersion,
},
topLevelKey: &controllerfetcher.ControllerKeyWithAPIVersion{
ControllerKey: controllerfetcher.ControllerKey{
Kind: kind,
Name: name1,
Namespace: namespace,
},
ApiVersion: apiVersion,
},
expectedConfigUnsupported: nil,
},
}

for i, tc := range testCases {
for _, tc := range testCases {

t.Run(fmt.Sprintf("test case number: %d", i), func(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
ctrl := gomock.NewController(t)
defer ctrl.Finish()

vpa := test.VerticalPodAutoscaler().WithName("testVpa").WithContainer("container").WithNamespace("testNamespace").Get()
vpa := test.VerticalPodAutoscaler().WithName("testVpa").WithContainer("container").WithNamespace("testNamespace").WithTargetRef(tc.targetRef).Get()
vpaLister := &test.VerticalPodAutoscalerListerMock{}
vpaLister.On("List").Return([]*vpa_types.VerticalPodAutoscaler{vpa}, nil)

Expand All @@ -117,6 +230,10 @@ func TestLegacySelector(t *testing.T) {
clusterState: clusterState,
legacySelectorFetcher: legacyTargetSelectorFetcher,
selectorFetcher: targetSelectorFetcher,
controllerFetcher: &fakeControllerFetcher{
key: tc.topLevelKey,
err: tc.findTopLevelError,
},
}

// legacyTargetSelectorFetcher is called twice:
Expand Down
Loading

0 comments on commit 0908d09

Please sign in to comment.