Skip to content

Commit

Permalink
Merge pull request operator-framework#375 from ecordell/check-doubleo…
Browse files Browse the repository at this point in the history 
…wner

Prevent ownership conflicts for CRDs
  • Loading branch information
@ecordell
ecordell committed Jul 5, 2018
2 parents 6d626ec + 3fd30e9 commit 58f1513
Show file tree
Hide file tree
Showing 3 changed files with 176 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
48 changes: 41 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,34 @@ 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
}
if in.Name == csv.Name {
return nil
}
// 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 58f1513

Please sign in to comment.