Make v2 YAML more forgiving when checking GVK for namespaced-ness #3186

blampe · 2024-08-22T21:15:31Z

This changes our error handling in Normalize to degrade gracefully in the case when we can't determine namespaced-ness, probably due to the CRD not existing yet. Instead of failing, we assume the resource is namespaced to allow the preview to succeed.

This is consistent with our error handling for this case elsewhere:

Check

pulumi-kubernetes/provider/pkg/provider/provider.go

Lines 1481 to 1488 in 0f834c8

    
           if clients.IsNoNamespaceInfoErr(err) { 
        
           	// This is probably a CustomResource without a registered CustomResourceDefinition. 
        
           	// Since we can't tell for sure at this point, assume it is namespaced, and correct if 
        
           	// required during the Create step. 
        
           	namespacedKind = true 
        
           } else { 
        
           	return nil, err 
        
           }

Diff

pulumi-kubernetes/provider/pkg/provider/provider.go

Lines 1666 to 1674 in 0f834c8

    
           if clients.IsNoNamespaceInfoErr(err) { 
        
           	// This is probably a CustomResource without a registered CustomResourceDefinition. 
        
           	// Since we can't tell for sure at this point, assume it is namespaced, and correct if 
        
           	// required during the Create step. 
        
           	namespacedKind = true 
        
           } else { 
        
           	return nil, fmt.Errorf( 
        
           		"API server returned error when asked if resource type %s is namespaced: %w", gvk, err) 
        
           }

Invoke:

pulumi-kubernetes/provider/pkg/provider/invoke_decode_yaml.go

Lines 49 to 56 in 0f834c8

    
           if err != nil { 
        
           	if clients.IsNoNamespaceInfoErr(err) { 
        
           		// Assume resource is namespaced. 
        
           		namespaced = true 
        
           	} else { 
        
           		return nil, err 
        
           	} 
        
           }

Added a failing unit test.

Fixes #3176

This changes our error handling in `Normalize` to degrade gracefully in the case when we can't determine namespaced-ness, probably due to the CRD not existing yet. Instead of failing, we assume the resource is namespaced to allow the preview to succeed. This is consistent with our error handling for this case elsewhere: * Check https://github.com/pulumi/pulumi-kubernetes/blob/0f834c8b0d89e0003f0dc2d527d4ca8e2cde26e9/provider/pkg/provider/provider.go#L1481-L1488 * Invoke: https://github.com/pulumi/pulumi-kubernetes/blob/0f834c8b0d89e0003f0dc2d527d4ca8e2cde26e9/provider/pkg/provider/invoke_decode_yaml.go#L49-L56 Fixes #3176

github-actions · 2024-08-22T21:20:41Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

codecov · 2024-08-22T21:25:34Z

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 39.41%. Comparing base (0f834c8) to head (0f429e4).

Files	Patch %	Lines
provider/pkg/provider/yaml/v2/yaml.go	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3186      +/-   ##
==========================================
+ Coverage   39.34%   39.41%   +0.07%     
==========================================
  Files          82       82              
  Lines        9631     9633       +2     
==========================================
+ Hits         3789     3797       +8     
+ Misses       5481     5475       -6     
  Partials      361      361

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EronWright

I considered this approach during the development of the MLCs. Here were my concerns:

the resource URN would be volatile depending on the actual cluster state, since it includes the namespace.
a wrong guess about the resource scope (namespaced-ness) would cause noise during preview, depicting a delete+create of the child.
transformation logic would see unexpected resource names.
having a correct resource urn is the basis for correct processing of the config.kubernetes.io/depends-on annotation (see docs).

Note that the logic in Check doesn't affect the resource name, so it is safer with respect to these concerns.

What else can we do? My proposal is that Release resource add its CRDs to the cache that's within the dynamic client, similarly to what happens to a CRD resource during Check:

pulumi-kubernetes/provider/pkg/provider/provider.go

Lines 1517 to 1524 in 2cc176d

    
           if clients.IsCRD(newInputs) { 
        
           	// add the CRD to the cache such that it contains all the CRDs that the program intends to create. 
        
           	// Do it now instead of later because update is called only if there's a non-empty diff, 
        
           	// and we want to ensure that the CRD is in the cache to support lookups by the component resources. 
        
           	if err := k.clientSet.CRDCache.AddCRD(newInputs); err != nil { 
        
           		return nil, err 
        
           	} 
        
           }

Note that this problem happens for Release but not for Chart because Chart creates child resources such as CustomResourceDefinition, even during preview, for which Check is called (thus populating the cache).

A Helm chart actually has two ways of providing a CRD, and the Check method of Release would need to understand both to be a comprehensive solution. One is described here, and would be about reading the crds/ directory within the chart. The other way is the "crd-install" way where the CRDs are templated as ordinary resources, and this means that Check would need to do helm template to obtain any CRD(s) (see #2568). Unfortunately, the venerable cert-manager chart is in the latter category.

blampe requested a review from EronWright August 22, 2024 21:15

EronWright reviewed Aug 22, 2024

View reviewed changes

mjeffryes assigned blampe Aug 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make v2 YAML more forgiving when checking GVK for namespaced-ness #3186

Make v2 YAML more forgiving when checking GVK for namespaced-ness #3186

blampe commented Aug 22, 2024

github-actions bot commented Aug 22, 2024

codecov bot commented Aug 22, 2024

EronWright left a comment •

edited

Loading

	if clients.IsNoNamespaceInfoErr(err) {
	// This is probably a CustomResource without a registered CustomResourceDefinition.
	// Since we can't tell for sure at this point, assume it is namespaced, and correct if
	// required during the Create step.
	namespacedKind = true
	} else {
	return nil, err
	}

	if err != nil {
	if clients.IsNoNamespaceInfoErr(err) {
	// Assume resource is namespaced.
	namespaced = true
	} else {
	return nil, err
	}
	}

	if clients.IsCRD(newInputs) {
	// add the CRD to the cache such that it contains all the CRDs that the program intends to create.
	// Do it now instead of later because update is called only if there's a non-empty diff,
	// and we want to ensure that the CRD is in the cache to support lookups by the component resources.
	if err := k.clientSet.CRDCache.AddCRD(newInputs); err != nil {
	return nil, err
	}
	}

Make v2 YAML more forgiving when checking GVK for namespaced-ness #3186

Are you sure you want to change the base?

Make v2 YAML more forgiving when checking GVK for namespaced-ness #3186

Conversation

blampe commented Aug 22, 2024

github-actions bot commented Aug 22, 2024

Does the PR have any schema changes?

codecov bot commented Aug 22, 2024

Codecov Report

EronWright left a comment • edited Loading

Choose a reason for hiding this comment

EronWright left a comment •

edited

Loading