From 749ccb062a2947871bf027f27204c1081d4cc822 Mon Sep 17 00:00:00 2001 From: David Eads Date: Tue, 9 Apr 2019 16:24:11 -0400 Subject: [PATCH] resource-apply: add release.openshift.io/create-only annotation If a resource to apply specifies this annotation it is created if missing, but it is never updated. We want to use this for empty config.openshift.io and operator.openshift.io resources, but support is uniform. --- lib/resourceapply/apiext.go | 8 +++ lib/resourceapply/apireg.go | 4 ++ lib/resourceapply/apps.go | 16 ++++++ lib/resourceapply/batch.go | 4 ++ lib/resourceapply/core.go | 16 ++++++ lib/resourceapply/cv.go | 8 +++ lib/resourceapply/interface.go | 18 +++++++ lib/resourceapply/rbac.go | 16 ++++++ lib/resourceapply/security.go | 4 ++ pkg/cvo/internal/generic.go | 6 +++ pkg/cvo/internal/generic_test.go | 84 ++++++++++++++++++++++++++++++++ 11 files changed, 184 insertions(+) create mode 100644 lib/resourceapply/interface.go create mode 100644 pkg/cvo/internal/generic_test.go diff --git a/lib/resourceapply/apiext.go b/lib/resourceapply/apiext.go index 49ee566301..5ef3a78720 100644 --- a/lib/resourceapply/apiext.go +++ b/lib/resourceapply/apiext.go @@ -19,6 +19,10 @@ func ApplyCustomResourceDefinition(client apiextclientv1beta1.CustomResourceDefi if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureCustomResourceDefinition(modified, existing, *required) @@ -39,6 +43,10 @@ func ApplyCustomResourceDefinitionFromCache(lister apiextlistersv1beta1.CustomRe if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } existing = existing.DeepCopy() modified := pointer.BoolPtr(false) diff --git a/lib/resourceapply/apireg.go b/lib/resourceapply/apireg.go index eaa115d041..dc87e7e1e0 100644 --- a/lib/resourceapply/apireg.go +++ b/lib/resourceapply/apireg.go @@ -18,6 +18,10 @@ func ApplyAPIService(client apiregclientv1.APIServicesGetter, required *apiregv1 if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureAPIService(modified, existing, *required) diff --git a/lib/resourceapply/apps.go b/lib/resourceapply/apps.go index d366fd9787..334eb20809 100644 --- a/lib/resourceapply/apps.go +++ b/lib/resourceapply/apps.go @@ -20,6 +20,10 @@ func ApplyDeployment(client appsclientv1.DeploymentsGetter, required *appsv1.Dep if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureDeployment(modified, existing, *required) @@ -41,6 +45,10 @@ func ApplyDeploymentFromCache(lister appslisterv1.DeploymentLister, client appsc if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } existing = existing.DeepCopy() modified := pointer.BoolPtr(false) @@ -63,6 +71,10 @@ func ApplyDaemonSet(client appsclientv1.DaemonSetsGetter, required *appsv1.Daemo if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureDaemonSet(modified, existing, *required) @@ -84,6 +96,10 @@ func ApplyDaemonSetFromCache(lister appslisterv1.DaemonSetLister, client appscli if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } existing = existing.DeepCopy() modified := pointer.BoolPtr(false) diff --git a/lib/resourceapply/batch.go b/lib/resourceapply/batch.go index 95adba2a7c..26fee6dba0 100644 --- a/lib/resourceapply/batch.go +++ b/lib/resourceapply/batch.go @@ -19,6 +19,10 @@ func ApplyJob(client batchclientv1.JobsGetter, required *batchv1.Job) (*batchv1. if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureJob(modified, existing, *required) diff --git a/lib/resourceapply/core.go b/lib/resourceapply/core.go index e465556b6a..b5acd27f5d 100644 --- a/lib/resourceapply/core.go +++ b/lib/resourceapply/core.go @@ -21,6 +21,10 @@ func ApplyNamespace(client coreclientv1.NamespacesGetter, required *corev1.Names if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) @@ -44,6 +48,10 @@ func ApplyService(client coreclientv1.ServicesGetter, required *corev1.Service) if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) @@ -69,6 +77,10 @@ func ApplyServiceAccount(client coreclientv1.ServiceAccountsGetter, required *co if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) @@ -90,6 +102,10 @@ func ApplyConfigMap(client coreclientv1.ConfigMapsGetter, required *corev1.Confi if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureConfigMap(modified, existing, *required) diff --git a/lib/resourceapply/cv.go b/lib/resourceapply/cv.go index 7b45b329c6..f671698ddb 100644 --- a/lib/resourceapply/cv.go +++ b/lib/resourceapply/cv.go @@ -19,6 +19,10 @@ func ApplyClusterVersion(client configclientv1.ClusterVersionsGetter, required * if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureClusterVersion(modified, existing, *required) @@ -39,6 +43,10 @@ func ApplyClusterVersionFromCache(lister configlistersv1.ClusterVersionLister, c if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } // Don't want to mutate cache. existing := obj.DeepCopy() diff --git a/lib/resourceapply/interface.go b/lib/resourceapply/interface.go new file mode 100644 index 0000000000..2f648602f2 --- /dev/null +++ b/lib/resourceapply/interface.go @@ -0,0 +1,18 @@ +package resourceapply + +import ( + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// CreateOnlyAnnotation means that this resource should be created if it does not exist, but should not be updated +// if it already exists. It is uniformly respected across all resources, but the first known use-cases are for +// empty config.openshift.io and initial low-level operator resources. +// Set .metadata.annotations["release.openshift.io/create-only"]="true" to have a create-only resource. +const CreateOnlyAnnotation = "release.openshift.io/create-only" + +// IsCreateOnly takes metadata and returns true if the resource should only be created, not updated. +func IsCreateOnly(metadata metav1.Object) bool { + return strings.EqualFold(metadata.GetAnnotations()[CreateOnlyAnnotation], "true") +} diff --git a/lib/resourceapply/rbac.go b/lib/resourceapply/rbac.go index 26a534e12e..d7b47a0033 100644 --- a/lib/resourceapply/rbac.go +++ b/lib/resourceapply/rbac.go @@ -19,6 +19,10 @@ func ApplyClusterRoleBinding(client rbacclientv1.ClusterRoleBindingsGetter, requ if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureClusterRoleBinding(modified, existing, *required) @@ -40,6 +44,10 @@ func ApplyClusterRole(client rbacclientv1.ClusterRolesGetter, required *rbacv1.C if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureClusterRole(modified, existing, *required) @@ -61,6 +69,10 @@ func ApplyRoleBinding(client rbacclientv1.RoleBindingsGetter, required *rbacv1.R if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureRoleBinding(modified, existing, *required) @@ -82,6 +94,10 @@ func ApplyRole(client rbacclientv1.RolesGetter, required *rbacv1.Role) (*rbacv1. if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureRole(modified, existing, *required) diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index a82e0a7fa4..39c574bf4c 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -19,6 +19,10 @@ func ApplySecurityContextConstraints(client securityclientv1.SecurityContextCons if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if IsCreateOnly(required) { + return nil, false, nil + } modified := pointer.BoolPtr(false) resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) diff --git a/pkg/cvo/internal/generic.go b/pkg/cvo/internal/generic.go index ebc79761db..20a2556504 100644 --- a/pkg/cvo/internal/generic.go +++ b/pkg/cvo/internal/generic.go @@ -5,6 +5,8 @@ import ( "encoding/json" "fmt" + "github.com/openshift/cluster-version-operator/lib/resourceapply" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -39,6 +41,10 @@ func applyUnstructured(client dynamic.ResourceInterface, required *unstructured. if err != nil { return nil, false, err } + // if we only create this resource, we have no need to continue further + if resourceapply.IsCreateOnly(required) { + return nil, false, nil + } existing.SetAnnotations(required.GetAnnotations()) existing.SetLabels(required.GetLabels()) diff --git a/pkg/cvo/internal/generic_test.go b/pkg/cvo/internal/generic_test.go new file mode 100644 index 0000000000..88edd07117 --- /dev/null +++ b/pkg/cvo/internal/generic_test.go @@ -0,0 +1,84 @@ +package internal + +import ( + "testing" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "k8s.io/apimachinery/pkg/runtime" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "k8s.io/client-go/dynamic/fake" +) + +func TestCreateOnlyCreate(t *testing.T) { + feature := `{ + "kind": "FeatureGate", + "apiVersion": "config.openshift.io/v1", + "metadata": { + "name": "cluster", + "annotations": { + "release.openshift.io/create-only": "true" + } + } +}` + obj, err := runtime.Decode(unstructured.UnstructuredJSONScheme, []byte(feature)) + if err != nil { + t.Fatal(err) + } + + fakeClient := fake.NewSimpleDynamicClient(runtime.NewScheme()) + _, modified, err := applyUnstructured( + fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), + obj.(*unstructured.Unstructured)) + if err != nil { + t.Fatal(err) + } + if !modified { + t.Error("should have created") + } +} + +func TestCreateOnlyUpdate(t *testing.T) { + feature := `{ + "kind": "FeatureGate", + "apiVersion": "config.openshift.io/v1", + "metadata": { + "name": "cluster", + "annotations": { + "release.openshift.io/create-only": "true", + "change": "here" + } + } +}` + existing := `{ + "kind": "FeatureGate", + "apiVersion": "config.openshift.io/v1", + "metadata": { + "name": "cluster", + "annotations": { + "release.openshift.io/create-only": "true" + } + } +}` + obj, err := runtime.Decode(unstructured.UnstructuredJSONScheme, []byte(feature)) + if err != nil { + t.Fatal(err) + } + existingObj, err := runtime.Decode(unstructured.UnstructuredJSONScheme, []byte(existing)) + if err != nil { + t.Fatal(err) + } + + fakeClient := fake.NewSimpleDynamicClient(runtime.NewScheme(), existingObj) + _, modified, err := applyUnstructured( + fakeClient.Resource(schema.GroupVersionResource{Group: "config.openshift.io", Version: "v1", Resource: "featuregates"}), + obj.(*unstructured.Unstructured)) + if err != nil { + t.Fatal(err) + } + if modified { + t.Error("should not have updated") + } +}