Implement autonaming configuration protocol #3377
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3377 +/- ##
==========================================
+ Coverage 41.17% 41.18% +0.01%
==========================================
Files 85 85
Lines 12764 12773 +9
==========================================
+ Hits 5255 5261 +6
- Misses 7120 7122 +2
- Partials 389 390 +1 ☔ View full report in Codecov by Sentry. |
CHANGELOG.md
Outdated
| - Respect autonaming configuration specified by the engine. | ||
| (https://github.com/pulumi/pulumi-kubernetes/issues/3363) |
There was a problem hiding this comment.
| - Respect autonaming configuration specified by the engine. | |
| (https://github.com/pulumi/pulumi-kubernetes/issues/3363) | |
| ### Added | |
| - Added support for autonaming configuration specified by the engine. | |
| (https://github.com/pulumi/pulumi-kubernetes/issues/3363) |
provider/pkg/metadata/naming_test.go
Outdated
| // o7 has no name, but autonaming is disabled by the engine, so autonaming fails. | ||
| o7 := &unstructured.Unstructured{} | ||
| autonamingDisabled := &pulumirpc.CheckRequest_AutonamingOptions{ | ||
| Mode: pulumirpc.CheckRequest_AutonamingOptions_DISABLE, | ||
| } | ||
| AssignNameIfAutonamable(nil, autonamingDisabled, o7, pm1, resource.NewURN(tokens.QName("teststack"), tokens.PackageName("testproj"), | ||
| tokens.Type(""), tokens.Type("bang:boom/fizzle:MajorResource"), "foo")) | ||
| assert.False(t, IsAutonamed(o7)) | ||
| assert.Equal(t, "", o7.GetGenerateName()) | ||
| assert.Equal(t, "", o7.GetName()) |
There was a problem hiding this comment.
In this case autonaming doesn't fail, the object just doesn't get a name. That will ultimately become an error when we try to apply the object to the cluster, since the name is required.
If we encounter this situation we should return an error to the user instead of letting them apply the invalid object. Something like "Autonaming is disabled so XYZ requires a .metadata.name". It would make sense to modify AssignNameIfAutonamable to return error for this situation.
There was a problem hiding this comment.
Is it always an error if a name is not specified, for all resource types? I assumed that some objects may allow that? But it sounds like that's not true? If name/generateName are always required, an error makes sense to me.
There was a problem hiding this comment.
Added an error on Disabled
provider/pkg/metadata/naming.go
Outdated
| case pulumirpc.CheckRequest_AutonamingOptions_ENFORCE, pulumirpc.CheckRequest_AutonamingOptions_PROPOSE: | ||
| autoname = engineAutonaming.ProposedName | ||
| } |
There was a problem hiding this comment.
We should also probably validate the proposed name, since the user could specify a template that isn't valid for k8s.
I think that would be IsDNS1123Subdomain but @rquitales should double check this!
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names
There was a problem hiding this comment.
Do we validate urn.Name() anywhere?
There was a problem hiding this comment.
Yes, IsDNS1123Subdomain is the correct validator.
There was a problem hiding this comment.
Do we validate
urn.Name()anywhere?
That's a good question, I don't think we do! I guess we offload all of that to the cluster, so maybe we don't need to validate anything here.
I'm curious what the user experience looks like in that situation? If it's pretty clear how to resolve the issue then this is probably fine.
There was a problem hiding this comment.
This is the error that one gets in both cases:
error: resource "urn:pulumi:dev::kubernetes-ts::kubernetes:apps/v1:Deployment::nginx!!!!" was not successfully created by the Kubernetes API server: Deployment.apps "nginx!!!!-6626ce94" is invalid: metadata.name: Invalid value: "nginx!!!!-6626ce94": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is 'a-z0-9?(.a-z0-9?)*')
|
Rebasing should fix your build failure. |
mikhailshilkov
force-pushed
the
mikhailshilkov/autonaming
branch
from
d6c5b8b to
87f84ae
Compare
@blampe Could you point me to an existing example that I could copy? |
This simplest/fasted would probably be to copy on of the yaml tests that live (confusingly) under tests/java https://github.com/pulumi/pulumi-kubernetes/blob/master/tests/sdk/java/yamlv2_test.go |
Proposed changes
This PR implement the Kubernetes part of pulumi/pulumi#1518. See pulumi/pulumi#17592 for the full design.
In short, pulumi/pulumi#17810 introduced the protobuf changes required for the provider-side implementation. With those, a provider can:
This PR applies those parameters to auto-name calculation if they are specified:
Related issues (optional)
Resolves #3363