Skip to content

Commit

Permalink
feat(olm-operator): prevent a CSV from installing if there is another
Browse files Browse the repository at this point in the history 
CSV in the namespace that owns the same CRD, but isn't being replaced.
  • Loading branch information
@ecordell
ecordell committed Jul 3, 2018
1 parent 4b97ca4 commit 56329dc
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 18 deletions.
3 changes: 2 additions & 1 deletion pkg/api/apis/clusterserviceversion/v1alpha1/types.go
View file
Expand Up @@ -6,8 +6,8 @@ import (
"encoding/json"
"sort"

"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis"
"github.com/coreos/go-semver/semver"
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -153,6 +153,7 @@ const (
CSVReasonRequirementsUnknown ConditionReason = "RequirementsUnknown"
CSVReasonRequirementsNotMet ConditionReason = "RequirementsNotMet"
CSVReasonRequirementsMet ConditionReason = "AllRequirementsMet"
CSVReasonOwnerConflict ConditionReason = "OwnerConflict"
CSVReasonComponentFailed ConditionReason = "InstallComponentFailed"
CSVReasonInvalidStrategy ConditionReason = "InvalidInstallStrategy"
CSVReasonWaiting ConditionReason = "InstallWaiting"
Expand Down
45 changes: 38 additions & 7 deletions pkg/controller/operators/alm/operator.go
View file
Expand Up @@ -155,32 +155,38 @@ func (a *ALMOperator) transitionCSVState(csv *v1alpha1.ClusterServiceVersion) (s
csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonRequirementsUnknown, "requirements not yet checked")
case v1alpha1.CSVPhasePending:
met, statuses := a.requirementStatus(csv)
csv.SetRequirementStatus(statuses)

if !met {
log.Info("requirements were not met")
csv.SetPhase(v1alpha1.CSVPhasePending, v1alpha1.CSVReasonRequirementsNotMet, "one or more requirements couldn't be found")
csv.SetRequirementStatus(statuses)
syncError = ErrRequirementsNotMet
return
}

// check for CRD ownership conflicts
if syncError = a.crdOwnerConflicts(csv, a.csvsInNamespace(csv.GetNamespace())); syncError != nil {
csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonOwnerConflict, fmt.Sprintf("owner conflict: %s", syncError))
return
}

log.Infof("scheduling ClusterServiceVersion for install: %s", csv.SelfLink)
csv.SetPhase(v1alpha1.CSVPhaseInstallReady, v1alpha1.CSVReasonRequirementsMet, "all requirements found, attempting install")
csv.SetRequirementStatus(statuses)
case v1alpha1.CSVPhaseInstallReady:
installer, strategy, _ := a.parseStrategiesAndUpdateStatus(csv)
if strategy == nil {
// parseStrategiesAndUpdateStatus sets CSV status
return
}

syncError = installer.Install(strategy)
if syncError == nil {
csv.SetPhase(v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonInstallSuccessful, "waiting for install components to report healthy")
a.requeueCSV(csv)
if syncError = installer.Install(strategy); syncError != nil {
csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailed, fmt.Sprintf("install strategy failed: %s", syncError))
return
}
csv.SetPhase(v1alpha1.CSVPhaseFailed, v1alpha1.CSVReasonComponentFailed, fmt.Sprintf("install strategy failed: %s", syncError))

csv.SetPhase(v1alpha1.CSVPhaseInstalling, v1alpha1.CSVReasonInstallSuccessful, "waiting for install components to report healthy")
a.requeueCSV(csv)
return
case v1alpha1.CSVPhaseInstalling:
installer, strategy, _ := a.parseStrategiesAndUpdateStatus(csv)
if strategy == nil {
Expand Down Expand Up @@ -366,6 +372,31 @@ func (a *ALMOperator) requirementStatus(csv *v1alpha1.ClusterServiceVersion) (me
return
}

func (a *ALMOperator) crdOwnerConflicts(in *v1alpha1.ClusterServiceVersion, csvsInNamespace []*v1alpha1.ClusterServiceVersion) error {
for _, crd := range in.Spec.CustomResourceDefinitions.Owned {
for _, csv := range csvsInNamespace {
if csv.OwnsCRD(crd.Name) {
// two csvs own the same CRD, only valid if there's a replacing chain between them
// TODO: this and the other replacement checking should just load the replacement chain DAG into memory
current := csv
for {
if in.Spec.Replaces == current.GetName() {
return nil
}
next := a.isBeingReplaced(current, csvsInNamespace)
if next != nil {
current = next
continue
}
// couldn't find a chain between the two csvs
return fmt.Errorf("%s and %s both own %s, but there is no replacement chain linking them", in.Name, csv.Name, crd.Name)
}
}
}
}
return nil
}

// annotateNamespace is the method that gets called when we see a namespace event in the cluster
func (a *ALMOperator) annotateNamespace(obj interface{}) (syncError error) {
namespace, ok := obj.(*corev1.Namespace)
Expand Down
143 changes: 133 additions & 10 deletions pkg/controller/operators/alm/operator_test.go
View file
Expand Up @@ -296,12 +296,12 @@ func TestCSVStateTransitionsFromNone(t *testing.T) {

func TestCSVStateTransitionsFromPending(t *testing.T) {
type clusterState struct {
crdDescriptons []*v1alpha1.CRDDescription
csvs []*v1alpha1.ClusterServiceVersion
}
tests := []struct {
in *v1alpha1.ClusterServiceVersion
out *v1alpha1.ClusterServiceVersion
state clusterState
state *clusterState
err error
description string
}{
Expand Down Expand Up @@ -414,6 +414,9 @@ func TestCSVStateTransitionsFromPending(t *testing.T) {
Message: "all requirements found, attempting install",
Reason: v1alpha1.CSVReasonRequirementsMet,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{},
},
description: "RequirementsMet/OwnedAndRequiredFound",
},
{
Expand All @@ -431,6 +434,9 @@ func TestCSVStateTransitionsFromPending(t *testing.T) {
Message: "all requirements found, attempting install",
Reason: v1alpha1.CSVReasonRequirementsMet,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{},
},
description: "RequirementsMet/OwnedFound",
},
{
Expand All @@ -448,27 +454,144 @@ func TestCSVStateTransitionsFromPending(t *testing.T) {
Message: "all requirements found, attempting install",
Reason: v1alpha1.CSVReasonRequirementsMet,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{},
},
description: "RequirementsMet/RequiredFound",
},
{
in: withStatus(withSpec(testCSV(""),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
Required: makeCRDDescriptions("found3", "found4"),
},
}),
&v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhasePending,
}),
out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhaseFailed,
Message: "owner conflict: test-csv and existing-owner both own found1, but there is no replacement chain linking them",
Reason: v1alpha1.CSVReasonOwnerConflict,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{withSpec(testCSV("existing-owner"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1"),
},
})},
},
description: "RequirementsMet/OwnedAndRequiredFound/CRDAlreadyOwnedNoReplacementChain",
err: fmt.Errorf("test-csv and existing-owner both own found1, but there is no replacement chain linking them"),
},
{
in: withStatus(withSpec(testCSV(""),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
Required: makeCRDDescriptions("found3", "found4"),
},
Replaces: "existing-owner-2",
}),
&v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhasePending,
}),
out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhaseInstallReady,
Message: "all requirements found, attempting install",
Reason: v1alpha1.CSVReasonRequirementsMet,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{
withSpec(testCSV("existing-owner-1"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1"),
},
}),
withSpec(testCSV("existing-owner-2"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
},
Replaces: "existing-owner-1",
}),
},
},
description: "RequirementsMet/OwnedAndRequiredFound/CRDOwnedInReplacementChain",
},
{
in: withStatus(withSpec(testCSV(""),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
Required: makeCRDDescriptions("found3", "found4"),
},
Replaces: "existing-owner-2",
}),
&v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhasePending,
}),
out: withStatus(testCSV(""), &v1alpha1.ClusterServiceVersionStatus{
Phase: v1alpha1.CSVPhaseInstallReady,
Message: "all requirements found, attempting install",
Reason: v1alpha1.CSVReasonRequirementsMet,
}),
state: &clusterState{
csvs: []*v1alpha1.ClusterServiceVersion{
withSpec(testCSV("existing-owner-1"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1"),
},
Replaces: "existing-owner-3",
}),
withSpec(testCSV("existing-owner-2"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
},
Replaces: "existing-owner-1",
}),
withSpec(testCSV("existing-owner-3"),
&v1alpha1.ClusterServiceVersionSpec{
CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
Owned: makeCRDDescriptions("found1", "found2"),
},
Replaces: "existing-owner-2",
}),
},
},
description: "RequirementsMet/OwnedAndRequiredFound/CRDOwnedInReplacementChainLoop",
},
}

for _, tt := range tests {
ctrl := gomock.NewController(t)
mockOp := NewMockALMOperator(ctrl)

mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Owned)
mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Required)
mockOp.MockOpClient.EXPECT().ListCustomResource(apis.GroupName, v1alpha1.GroupVersion, tt.in.GetNamespace(), v1alpha1.ClusterServiceVersionKind).Return(&operatorclient.CustomResourceList{}, nil)

// Test the transition
t.Run(tt.description, func(t *testing.T) {
ctrl := gomock.NewController(t)
mockOp := NewMockALMOperator(ctrl)

mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Owned)
mockCRDExistence(*mockOp.MockQueueOperator.MockClient, tt.in.Spec.CustomResourceDefinitions.Required)

// mock for call to short-circuit when replacing
mockCSVsInNamespace(t, mockOp.MockQueueOperator.MockClient, tt.in.Namespace, nil, nil)

// mock for pending, check that no other CSV owns the CRDs (unless being replaced)
if tt.state != nil {
mockCSVsInNamespace(t, mockOp.MockQueueOperator.MockClient, tt.in.Namespace, tt.state.csvs, nil)
}

err := mockOp.transitionCSVState(tt.in)
require.EqualValues(t, tt.err, err)
require.EqualValues(t, tt.out.Status.Phase, tt.in.Status.Phase)
require.EqualValues(t, tt.out.Status.Message, tt.in.Status.Message)
require.EqualValues(t, tt.out.Status.Reason, tt.in.Status.Reason)
ctrl.Finish()
})
ctrl.Finish()
}
}

Expand Down

0 comments on commit 56329dc

Please sign in to comment.