feat: add v1 DomainMapping API (promote from beta to GA)#16565
feat: add v1 DomainMapping API (promote from beta to GA)#16565Ankitsinghsisodya wants to merge 1 commit intoknative:mainfrom
Conversation
Adds v1 DomainMapping API types, promotes the reconciler and webhook to use v1 as the primary version, and keeps v1beta1 served (but no longer the storage version) for backward compatibility. Changes: - Add v1 DomainMapping types, lifecycle, defaults, validation, conversion - Add v1beta1 → v1 conversion (v1 is hub) - Update generated clientset, informers, listers, injection for v1 - Update domainmapping reconciler/controller to use v1 - Update CRD to serve both v1 (storage) and v1beta1 (non-storage) - Register v1 DomainMapping in webhook - Update e2e/conformance tests to use v1 Partially addresses knative#13967
knative-prow
Bot
added
the
size/XXL
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
knative-prow
Bot
added
the
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Promotes DomainMapping API to GA by introducing serving.knative.dev/v1 DomainMapping types and moving controllers/reconcilers/tests to use v1 as the primary version while keeping v1beta1 served via conversion for backward compatibility.
Changes:
- Add
v1DomainMapping API surface (types/defaulting/validation/lifecycle) plusv1beta1↔v1conversion. - Update domainmapping reconciler/controller/webhook and related resources/helpers to use
v1DomainMapping. - Add/update generated (or manually-aligned) clientset/informers/listers/injection reconciler for
serving/v1DomainMapping and update CRD storage version tov1.
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/e2e/externaldomaintls/domain_mapping_test.go | Switch e2e external domain TLS DomainMapping usage to v1. |
| test/e2e/domainmapping/domain_mapping_websocket_test.go | Switch websocket e2e DomainMapping usage to v1. |
| test/e2e/domainmapping/domain_mapping_test.go | Switch domainmapping e2e test DomainMapping usage to v1 (incl. TLS SecretTLS type). |
| test/conformance/api/v1beta1/domain_mapping_test.go | Update conformance test to create/read v1 DomainMapping while keeping test location. |
| test/clients.go | Route test DomainMappings client to ServingV1() typed client. |
| pkg/reconciler/testing/v1/listers.go | Update reconciler testing listers to use v1 DomainMapping lister. |
| pkg/reconciler/domainmapping/table_test.go | Update domainmapping reconciler table tests to use v1 DomainMapping types and reconciler injection path. |
| pkg/reconciler/domainmapping/resources/ingress_test.go | Update ingress resource unit tests to v1 DomainMapping types. |
| pkg/reconciler/domainmapping/resources/ingress.go | Update MakeIngress to accept v1 DomainMapping. |
| pkg/reconciler/domainmapping/resources/domainclaim_test.go | Update domainclaim resource unit test to v1 DomainMapping type. |
| pkg/reconciler/domainmapping/resources/domainclaim.go | Update MakeDomainClaim to accept v1 DomainMapping. |
| pkg/reconciler/domainmapping/resources/certificate_test.go | Update certificate resource unit tests to v1 DomainMapping types. |
| pkg/reconciler/domainmapping/resources/certificate.go | Update MakeCertificate to accept v1 DomainMapping. |
| pkg/reconciler/domainmapping/reconciler.go | Switch reconciler interfaces/methods from v1beta1 DomainMapping to v1 DomainMapping. |
| pkg/reconciler/domainmapping/controller.go | Switch controller wiring/filtering to v1 DomainMapping informer/reconciler. |
| pkg/client/listers/serving/v1/expansion_generated.go | Add DomainMapping lister expansion hooks for v1. |
| pkg/client/listers/serving/v1/domainmapping.go | Add v1 DomainMapping lister. |
| pkg/client/injection/reconciler/serving/v1/domainmapping/state.go | Add generated typed injection reconciler state for v1 DomainMapping. |
| pkg/client/injection/reconciler/serving/v1/domainmapping/reconciler.go | Add generated typed injection reconciler implementation for v1 DomainMapping. |
| pkg/client/injection/reconciler/serving/v1/domainmapping/controller.go | Add generated typed injection reconciler controller wiring for v1 DomainMapping. |
| pkg/client/injection/informers/serving/v1/domainmapping/filtered/fake/fake.go | Add filtered fake informer injection for v1 DomainMapping. |
| pkg/client/injection/informers/serving/v1/domainmapping/filtered/domainmapping.go | Add filtered informer injection plumbing for v1 DomainMapping. |
| pkg/client/injection/informers/serving/v1/domainmapping/fake/fake.go | Add fake informer injection for v1 DomainMapping. |
| pkg/client/injection/informers/serving/v1/domainmapping/domainmapping.go | Add informer injection for v1 DomainMapping. |
| pkg/client/informers/externalversions/serving/v1/interface.go | Add DomainMappings informer accessor to serving/v1 informer interface. |
| pkg/client/informers/externalversions/serving/v1/domainmapping.go | Add externalversions informer for v1 DomainMapping. |
| pkg/client/informers/externalversions/generic.go | Register v1 DomainMappings in generic informer factory routing. |
| pkg/client/clientset/versioned/typed/serving/v1/serving_client.go | Add DomainMappings getter to the typed ServingV1 client. |
| pkg/client/clientset/versioned/typed/serving/v1/generated_expansion.go | Add DomainMappingExpansion hook. |
| pkg/client/clientset/versioned/typed/serving/v1/fake/fake_serving_client.go | Add fake DomainMappings getter in fake ServingV1 client. |
| pkg/client/clientset/versioned/typed/serving/v1/fake/fake_domainmapping.go | Add fake typed client implementation for v1 DomainMappings. |
| pkg/client/clientset/versioned/typed/serving/v1/domainmapping.go | Add typed REST client for v1 DomainMappings. |
| pkg/apis/serving/v1beta1/domainmapping_conversion.go | Add v1beta1↔v1 conversion methods for DomainMapping. |
| pkg/apis/serving/v1/zz_generated.deepcopy.go | Add deepcopy support for v1 DomainMapping types. |
| pkg/apis/serving/v1/register.go | Register v1 DomainMapping types in scheme. |
| pkg/apis/serving/v1/domainmapping_validation_test.go | Add validation tests for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_validation.go | Add v1 DomainMapping validation logic. |
| pkg/apis/serving/v1/domainmapping_types_test.go | Add basic type behavior test (GetStatus) for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_types.go | Add v1 DomainMapping API types/constants. |
| pkg/apis/serving/v1/domainmapping_lifecycle_test.go | Add lifecycle/conditions tests for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_lifecycle.go | Add lifecycle/conditions implementation for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_defaults_test.go | Add defaulting tests for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_defaults.go | Add defaulting implementation for v1 DomainMapping. |
| pkg/apis/serving/v1/domainmapping_conversion.go | Add v1 conversion stubs consistent with other v1 hub types. |
| config/core/300-resources/domain-mapping.yaml | Update CRD to serve v1 (storage) and keep v1beta1 served (non-storage) with v1 schema. |
| cmd/webhook/main.go | Register v1 DomainMapping in webhook type map for defaulting/validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "knative.dev/pkg/reconciler" | ||
| v1 "knative.dev/serving/pkg/apis/serving/v1" | ||
| "knative.dev/serving/pkg/apis/serving/v1beta1" | ||
| servingv1 "knative.dev/serving/pkg/apis/serving/v1" | ||
| "knative.dev/serving/test" |
There was a problem hiding this comment.
This import block brings in the same package path twice (knative.dev/serving/pkg/apis/serving/v1) under two different aliases (v1 and servingv1). Go disallows duplicate imports of the same path, so this will not compile. Please use a single import alias and reference both Service and DomainMapping types through that alias (or split the usage so only one alias is needed).
| ctx = apis.WithinUpdate(ctx, test.prev) | ||
| if diff := cmp.Diff(test.want.Error(), test.this.Validate(ctx).Error()); diff != "" { | ||
| t.Error("Validate (-want, +got) =", diff) |
There was a problem hiding this comment.
test.want is nil for the "update lastModifier annotation with spec changes" case, so calling test.want.Error() will panic. Please compare errors in a nil-safe way (e.g., compare *apis.FieldError directly, or convert both to strings with a helper that returns "" for nil).
| Verb: "delete", | ||
| Resource: v1beta1.SchemeGroupVersion.WithResource("clusterdomainclaims"), | ||
| Resource: servingv1.SchemeGroupVersion.WithResource("clusterdomainclaims"), | ||
| }, |
There was a problem hiding this comment.
This expected delete action is for NetworkingV1alpha1().ClusterDomainClaims().Delete(...), so the Resource GVR should use the networking API group/version (e.g. networking/v1alpha1), not serving.knative.dev/v1. As written, the test's WantDeletes resource won't match the action produced by the reconciler.
| // ServingBetaClients holds instances of interfaces for making requests to knative serving clients. | ||
| type ServingBetaClients struct { | ||
| DomainMappings servingv1beta1.DomainMappingInterface | ||
| DomainMappings servingv1.DomainMappingInterface | ||
| } |
There was a problem hiding this comment.
ServingBetaClients now exposes a serving/v1 DomainMappingInterface, so the type name and its doc comment are misleading (these are no longer v1beta1/beta clients for DomainMapping). Consider renaming this struct/field (or at least updating the comment) to reflect that DomainMappings are accessed via v1 even when using ServingBetaClient in tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16565 +/- ##
==========================================
+ Coverage 80.16% 80.21% +0.04%
==========================================
Files 217 223 +6
Lines 13542 17449 +3907
==========================================
+ Hits 10856 13996 +3140
- Misses 2320 3089 +769
+ Partials 366 364 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
v1DomainMapping API types (types, lifecycle, defaults, validation, conversion) to promote DomainMapping from Beta to GAv1as the primary DomainMapping versionv1beta1served (non-storage) for backward compatibility, with a v1beta1↔v1 conversionserving:v1v1the storage version andv1beta1still servedv1DomainMappingFixes #13967
Notes
v1is the conversion hub;v1beta1.ConvertTo/ConvertFromperforms the field mappinglifecycle/frozendue to unresolved protocol issues (Websockets do not work with domainmapings by default #13083). This PR implements the API promotion work and can be evaluated once those blockers are resolved.hack/update-codegen.shscript would re-generate the injection/clientset files; the generated files here were written manually following existing patterns to maintain consistencyTest plan
go build ./...passesgo test ./pkg/...passes (all 46 modified/created files build and tests pass)