Skip to content
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.

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

Fixes in how the provider handles CRDs and CRs #271

Merged
merged 3 commits into from
Nov 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 38 additions & 13 deletions pkg/await/await.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ func Creation(
ctx context.Context, host *provider.HostClient, pool dynamic.ClientPool,
disco discovery.ServerResourcesInterface, urn resource.URN, inputs *unstructured.Unstructured,
) (*unstructured.Unstructured, error) {
clientForResource, err := client.FromResource(pool, disco, inputs)
if err != nil {
return nil, err
}

// Issue create request. We retry the create REST request on failure, so that we can tolerate
// some amount of misordering (e.g., creating a `Pod` before the `Namespace` it goes in;
// creating a custom resource before the CRD is registered; etc.), which is common among Helm
Expand All @@ -69,11 +64,24 @@ func Creation(
// nolint
// https://github.com/kubernetes/kubernetes/blob/54889d581a35acf940d52a8a384cccaa0b597ddc/pkg/kubectl/cmd/apply/apply.go#L94
var outputs *unstructured.Unstructured
err = sleepingRetry(
var clientForResource dynamic.ResourceInterface
err := sleepingRetry(
func(i uint) error {
if i > 0 {
_ = host.LogStatus(ctx, diag.Info, urn, fmt.Sprintf("Creation failed, retrying (%d)", i))
}

// Recreate the client for resource, in case the client's cache of the server API was
// invalidated. For example, when a CRD is created, it will invalidate the client cache;
// this allows CRs that we tried (and failed) to create before to re-try with the new
// server API, at which point they should hopefully succeed.
var err error
if clientForResource == nil {
clientForResource, err = client.FromResource(pool, disco, inputs)
if err != nil {
return err
}
}
outputs, err = clientForResource.Create(inputs)
return err
}).
Expand Down Expand Up @@ -293,36 +301,53 @@ func Deletion(
disco discovery.DiscoveryInterface, urn resource.URN, gvk schema.GroupVersionKind, namespace,
name string,
) error {
// nilIfGVKDeleted takes an error and returns nil if `errors.IsNotFound`; otherwise, it returns
// the error argument unchanged.
//
// Rationale: If we have gotten to this point, this resource was successfully created and is now
// being deleted. This implies that the G/V/K once did exist, but now does not. This, in turn,
// implies that it has been successfully deleted. For example: the resource was likely a CR, but
// the CRD has since been removed. Otherwise, the resource was deleted out-of-band.
//
// This is necessary for CRs, which are often deleted after the relevant CRDs (especially in
// Helm charts), and it is acceptable for other resources because it is semantically like
// running `refresh` before deletion.
nilIfGVKDeleted := func(err error) error {
hausdorff marked this conversation as resolved.
Show resolved Hide resolved
if errors.IsNotFound(err) {
return nil
}
return err
}

// Make delete options based on the version of the client.
version, err := client.FetchVersion(disco)
if err != nil {
version = client.DefaultVersion()
}

// Manually set delete propagation for Kubernetes versions < 1.6 to avoid bugs.
deleteOpts := metav1.DeleteOptions{}
if version.Compare(1, 6) < 0 {
// 1.5.x option.
boolFalse := false
// nolint
deleteOpts.OrphanDependents = &boolFalse
} else {
// 1.6.x option. (NOTE: Background delete propagation is broken in k8s v1.6, and maybe later.)
} else if version.Compare(1, 7) < 0 {
// 1.6.x option. Background delete propagation is broken in k8s v1.6.
fg := metav1.DeletePropagationForeground
deleteOpts.PropagationPolicy = &fg
}

// Obtain client for the resource being deleted.
clientForResource, err := client.FromGVK(pool, disco, gvk, namespace)
if err != nil {
return err
return nilIfGVKDeleted(err)
}

// Issue deletion request.
err = clientForResource.Delete(name, &deleteOpts)
if err != nil && !errors.IsNotFound(err) {
return fmt.Errorf("Could not find resource '%s/%s' for deletion: %s", namespace, name, err)
} else if err != nil {
return err
if err != nil {
return nilIfGVKDeleted(err)
}

// Wait until delete resolves as success or error. Note that the conditional is set up to log only
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func serverResourceForGVK(
) (*metav1.APIResource, error) {
resources, err := disco.ServerResourcesForGroupVersion(gvk.GroupVersion().String())
if err != nil {
return nil, fmt.Errorf("unable to fetch resource description for %s: %v", gvk.GroupVersion(), err)
return nil, err
}

for _, r := range resources.APIResources {
Expand Down
15 changes: 14 additions & 1 deletion pkg/provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,13 @@ func (k *kubeProvider) Create(
return nil, initializationError(client.FqObjName(initialized), awaitErr, inputsAndComputed)
}

// Invalidate the client cache if this was a CRD. This will require subsequent CR creations to
// refresh the cache, at which point the CRD definition will be present, so that it doesn't fail
// with an `errors.IsNotFound`.
if isCRD(newInputs) {
k.client.Invalidate()
}

return &pulumirpc.CreateResponse{
Id: client.FqObjName(initialized), Properties: inputsAndComputed,
}, nil
Expand Down Expand Up @@ -597,7 +604,7 @@ func (k *kubeProvider) Update(
// So the roadmap is:
//
// - [x] Implement `Update` using the three-way JSON merge strategy.
// - [ ] Cause `Update` to default to the three-way JSON merge patch strategy. (This will require
// - [x] Cause `Update` to default to the three-way JSON merge patch strategy. (This will require
// plumbing, because it expects nominal types representing the API schema, but the
// discovery client is completely dynamic.)
// - [ ] Support server-side apply, when it comes out.
Expand Down Expand Up @@ -813,3 +820,9 @@ func canonicalNamespace(ns string) string {
}
return ns
}

// isCRD returns true if a Kubernetes resource is a CRD.
func isCRD(obj *unstructured.Unstructured) bool {
return obj.GetKind() == "CustomResourceDefinition" &&
strings.HasPrefix(obj.GetAPIVersion(), "apiextensions.k8s.io/")
}