Skip to content

Bug 1825330: support creating v1beta CRDs to avoid data loss #1470

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Bug 1825330: support creating v1beta CRDs to avoid data loss #1470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e6ed58
fix: support creating v1beta CRDs to avoid data loss during conversio…
exdx Apr 21, 2020
695c67f
fix: address sttts feedback
exdx Apr 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/olm/cleanup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ func TestCleanupOwnerReferences(t *testing.T) {
require.Equal(t, tt.expected.err, cleanupOwnerReferences(c, crc))

listOpts := metav1.ListOptions{}
csvs, err := crc.OperatorsV1alpha1().ClusterServiceVersions(metav1.NamespaceAll).List(listOpts)
csvs, err := crc.OperatorsV1alpha1().ClusterServiceVersions(metav1.NamespaceAll).List(context.TODO(), listOpts)
require.NoError(t, err)
require.ElementsMatch(t, tt.expected.csvs, csvs.Items)

Expand Down
84 changes: 32 additions & 52 deletions pkg/controller/operators/catalog/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
"k8s.io/apiextensions-apiserver/pkg/apiserver/validation"
extinf "k8s.io/apiextensions-apiserver/pkg/client/informers/externalversions"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -1305,30 +1306,6 @@ func (o *Operator) ResolvePlan(plan *v1alpha1.InstallPlan) error {
return nil
}

func GetCRDV1VersionsMap(crd *apiextensionsv1.CustomResourceDefinition) map[string]struct{} {
versionsMap := map[string]struct{}{}

for _, version := range crd.Spec.Versions {
versionsMap[version.Name] = struct{}{}
}
return versionsMap
}

// Ensure all existing served versions are present in new CRD
func EnsureCRDVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
newCRDVersions := GetCRDV1VersionsMap(newCRD)

for _, oldVersion := range oldCRD.Spec.Versions {
if oldVersion.Served {
_, ok := newCRDVersions[oldVersion.Name]
if !ok {
return fmt.Errorf("New CRD (%s) must contain existing served versions (%s)", oldCRD.Name, oldVersion.Name)
}
}
}
return nil
}

// Validate all existing served versions against new CRD's validation (if changed)
func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) error {
logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Versions, newCRD.Spec.Versions)
Expand Down Expand Up @@ -1364,6 +1341,36 @@ func validateV1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiexte
return nil
}

// Validate all existing served versions against new CRD's validation (if changed)
func validateV1Beta1CRDCompatibility(dynamicClient dynamic.Interface, oldCRD *apiextensionsv1beta1.CustomResourceDefinition, newCRD *apiextensionsv1beta1.CustomResourceDefinition) error {
logrus.Debugf("Comparing %#v to %#v", oldCRD.Spec.Validation, newCRD.Spec.Validation)

// TODO return early of all versions are equal
convertedCRD := &apiextensions.CustomResourceDefinition{}
if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

here is still a conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here we convert to the internal version to do static schema validation. Specifically, the NewSchemaValidator which creates an openapi schema validator for the given CRD validation, expects an internal CRD type to be provided to it, so OLM does the conversion here when validating the new CRD.

The internal CRD is never written to the cluster again. We can make a follow-up BZ to use dry-run APIs instead of using the internal validation package, if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, would prefer dry-run (follow-up). The schema validator is also a component considered private.

Copy link
Member

@ecordell ecordell Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How can we dry-run against an CRD API that has not been applied to the cluster yet?

The goal is to catch the mistake before the CRD has been created.

There seems to be little practical issue with using the internal types, correct? We could just as easily read the json schema from the CRD spec and use an off-the-shelf schema validator to accomplish the same goals.

return err
}
for _, version := range oldCRD.Spec.Versions {
if !version.Served {
gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: version.Name, Resource: oldCRD.Spec.Names.Plural}
err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
if err != nil {
return err
}
}
}

if oldCRD.Spec.Version != "" {
gvr := schema.GroupVersionResource{Group: oldCRD.Spec.Group, Version: oldCRD.Spec.Version, Resource: oldCRD.Spec.Names.Plural}
err := validateExistingCRs(dynamicClient, gvr, convertedCRD)
if err != nil {
return err
}
}
logrus.Debugf("Successfully validated CRD %s\n", newCRD.Name)
return nil
}

func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersionResource, newCRD *apiextensions.CustomResourceDefinition) error {
// make dynamic client
crList, err := dynamicClient.Resource(gvr).List(context.TODO(), metav1.ListOptions{})
Expand All @@ -1383,33 +1390,6 @@ func validateExistingCRs(dynamicClient dynamic.Interface, gvr schema.GroupVersio
return nil
}

// Attempt to remove stored versions that have been deprecated before allowing
// those versions to be removed from the new CRD.
// The function may not always succeed as storedVersions requires at least one
// version. If there is only stored version, it won't be removed until a new
// stored version is added.
func removeDeprecatedV1StoredVersions(oldCRD *apiextensionsv1.CustomResourceDefinition, newCRD *apiextensionsv1.CustomResourceDefinition) []string {
// StoredVersions requires to have at least one version.
if len(oldCRD.Status.StoredVersions) <= 1 {
return nil
}

newStoredVersions := []string{}
newCRDVersions := GetCRDV1VersionsMap(newCRD)
for _, v := range oldCRD.Status.StoredVersions {
_, ok := newCRDVersions[v]
if ok {
newStoredVersions = append(newStoredVersions, v)
}
}

if len(newStoredVersions) < 1 {
return nil
} else {
return newStoredVersions
}
}

// ExecutePlan applies a planned InstallPlan to a namespace.
func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
if plan.Status.Phase != v1alpha1.InstallPlanPhaseInstalling {
Expand All @@ -1436,7 +1416,7 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error {
}

ensurer := newStepEnsurer(kubeclient, crclient, dynamicClient)
b := newBuilder(kubeclient, dynamicClient, o.csvProvidedAPIsIndexer)
b := newBuilder(kubeclient, dynamicClient, o.csvProvidedAPIsIndexer, o.logger)

for i, step := range plan.Status.Plan {
doStep := true
Expand Down
197 changes: 0 additions & 197 deletions pkg/controller/operators/catalog/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,203 +135,6 @@ func TestTransitionInstallPlan(t *testing.T) {
}
}

func TestEnsureV1CRDVersions(t *testing.T) {
mainCRDPlural := "ins-main-abcde"

currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
{
Name: "v1alpha1",
Served: true,
Storage: true,
},
}

addedVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
{
Name: "v1alpha1",
Served: true,
Storage: false,
},
{
Name: "v1alpha2",
Served: true,
Storage: true,
},
}

missingVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
{
Name: "v1alpha2",
Served: true,
Storage: true,
},
}

tests := []struct {
name string
oldCRD apiextensionsv1.CustomResourceDefinition
newCRD apiextensionsv1.CustomResourceDefinition
expectedFailure bool
}{
{
name: "existing versions are present",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions = currentVersions
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = addedVersions
return newCRD
}(),
expectedFailure: false,
},
{
name: "missing versions in new CRD 1",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions = currentVersions
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = missingVersions
return newCRD
}(),
expectedFailure: true,
},
{
name: "missing version in new CRD 2",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions[0].Name = "v1alpha1"
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions[0].Name = "v1alpha2"
return newCRD
}(),
expectedFailure: true,
},
{
name: "existing version is present in new CRD's versions",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions[0].Name = "v1alpha1"
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = addedVersions
return newCRD
}(),
expectedFailure: false,
},
}

for _, tt := range tests {
err := EnsureCRDVersions(&tt.oldCRD, &tt.newCRD)
if tt.expectedFailure {
require.Error(t, err)
}
}
}

func TestRemoveDeprecatedV1StoredVersions(t *testing.T) {
mainCRDPlural := "ins-main-test"

currentVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
{
Name: "v1alpha1",
Served: true,
Storage: false,
},
{
Name: "v1alpha2",
Served: true,
Storage: true,
},
}

newVersions := []apiextensionsv1.CustomResourceDefinitionVersion{
{
Name: "v1alpha2",
Served: true,
Storage: false,
},
{
Name: "apiextensionsv1beta1",
Served: true,
Storage: true,
},
}

crdStatusStoredVersions := apiextensionsv1.CustomResourceDefinitionStatus{
StoredVersions: []string{},
}

tests := []struct {
name string
oldCRD apiextensionsv1.CustomResourceDefinition
newCRD apiextensionsv1.CustomResourceDefinition
expectedResult []string
}{
{
name: "only one stored version exists",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions = currentVersions
oldCRD.Status = crdStatusStoredVersions
oldCRD.Status.StoredVersions = []string{"v1alpha1"}
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = newVersions
return newCRD
}(),
expectedResult: nil,
},
{
name: "multiple stored versions with one deprecated version",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions = currentVersions
oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha2"}
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = newVersions
return newCRD
}(),
expectedResult: []string{"v1alpha2"},
},
{
name: "multiple stored versions with all deprecated version",
oldCRD: func() apiextensionsv1.CustomResourceDefinition {
oldCRD := v1crd(mainCRDPlural)
oldCRD.Spec.Versions = currentVersions
oldCRD.Status.StoredVersions = []string{"v1alpha1", "v1alpha3"}
return oldCRD
}(),
newCRD: func() apiextensionsv1.CustomResourceDefinition {
newCRD := v1crd(mainCRDPlural)
newCRD.Spec.Versions = newVersions
return newCRD
}(),
expectedResult: nil,
},
}

for _, tt := range tests {
resultCRD := removeDeprecatedV1StoredVersions(&tt.oldCRD, &tt.newCRD)
require.Equal(t, tt.expectedResult, resultCRD)
}
}

func TestExecutePlan(t *testing.T) {
namespace := "ns"

Expand Down
Loading