Skip to content

Commit

Permalink
fix: ensure namespaces do not collide when generating canonicalGroup
Browse files Browse the repository at this point in the history
names

This commit ensures that we do not have package collisions when storing
canonical group names. Additional consideration is required to ensure
that there are no final schema changes so that we do not break v4 users.
It is not expected to have any package collisions in the p-k provider,
but is possible in the downstream crd2pulumi consure of our schemagen.
Here, users can supply any number of CRDs to typegen, so we need to be
specific when it comes to handling group names.
  • Loading branch information
rquitales committed Sep 30, 2024
1 parent 45fc9c5 commit a5873e0
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 52 deletions.
11 changes: 11 additions & 0 deletions provider/pkg/gen/additionalComments.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package gen
import (
"fmt"
"strconv"
"strings"

"github.com/pulumi/pulumi-kubernetes/provider/v4/pkg/await"
"github.com/pulumi/pulumi-kubernetes/provider/v4/pkg/kinds"
Expand Down Expand Up @@ -167,6 +168,16 @@ func PulumiComment(kind string) string {
func APIVersionComment(gvk schema.GroupVersionKind) string {
const deprecatedTemplate = `%s is deprecated by %s`
const notSupportedTemplate = ` and not supported by Kubernetes v%v+ clusters.`

// Get only the last segment of the group.
t := strings.Split(gvk.Group, ".")
groupBackwardsCompatible := t[len(t)-1]
gvk = schema.GroupVersionKind{
Group: groupBackwardsCompatible,
Version: gvk.Version,
Kind: gvk.Kind,
}

gvkStr := gvk.GroupVersion().String() + "/" + gvk.Kind
removedIn := kinds.RemovedInVersion(gvk)

Expand Down
8 changes: 4 additions & 4 deletions provider/pkg/gen/additionalComments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,19 @@ func TestAPIVersionComment(t *testing.T) {
expected string
}{
{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1beta1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1beta1", Kind: "Deployment"},
expected: "apps/v1beta1/Deployment is deprecated by apps/v1/Deployment and not supported by Kubernetes v1.16+ clusters.",
},
{
gvk: schema.GroupVersionKind{Group: "extensions", Version: "v1beta1", Kind: "Ingress"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.extensions", Version: "v1beta1", Kind: "Ingress"},
expected: "extensions/v1beta1/Ingress is deprecated by networking.k8s.io/v1beta1/Ingress and not supported by Kubernetes v1.20+ clusters.",
},
{
gvk: schema.GroupVersionKind{Group: "batch", Version: "v1beta1", Kind: "CronJob"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.batch", Version: "v1beta1", Kind: "CronJob"},
expected: "batch/v1beta1/CronJob is deprecated by batch/v1beta1/CronJob.",
},
{
gvk: schema.GroupVersionKind{Group: "core", Version: "v1", Kind: "Pod"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.core", Version: "v1", Kind: "Pod"},
expected: "core/v1/Pod is deprecated by core/v1/Pod.",
},
}
Expand Down
8 changes: 4 additions & 4 deletions provider/pkg/gen/testdata/identify-list-kinds/list-kind
Original file line number Diff line number Diff line change
Expand Up @@ -145,9 +145,9 @@
}

-- kinds --
- k8sversion.test.TestResource
- k8sversion.test.TestResourceNotRealList
- com.pulumi.k8sversion.test.TestResource
- com.pulumi.k8sversion.test.TestResourceNotRealList

-- listKinds --
- k8sversion.test.TestResourceList
- k8sversion.test.TestResourceNotRealListList
- com.pulumi.k8sversion.test.TestResourceList
- com.pulumi.k8sversion.test.TestResourceNotRealListList
67 changes: 40 additions & 27 deletions provider/pkg/gen/typegen.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,28 +205,28 @@ func (d definition) isTopLevel() bool {
switch fmt.Sprintf("%s/%s", d.gvk.GroupVersion().String(), d.gvk.Kind) {
case
"v1/Status",
"apps/v1beta1/Scale",
"apps/v1beta2/Scale",
"authentication/v1/TokenRequest",
"authentication/v1/TokenReview",
"authentication/v1alpha1/SelfSubjectReview",
"authentication/v1beta1/SelfSubjectReview",
"authentication/v1/SelfSubjectReview",
"authentication/v1beta1/TokenReview",
"authorization/v1/LocalSubjectAccessReview",
"authorization/v1/SelfSubjectAccessReview",
"authorization/v1/SelfSubjectRulesReview",
"authorization/v1/SubjectAccessReview",
"authorization/v1beta1/LocalSubjectAccessReview",
"authorization/v1beta1/SelfSubjectAccessReview",
"authorization/v1beta1/SelfSubjectRulesReview",
"authorization/v1beta1/SubjectAccessReview",
"autoscaling/v1/Scale",
"core/v1/ComponentStatus",
"core/v1/ComponentStatusList",
"extensions/v1beta1/Scale",
"policy/v1beta1/Eviction",
"policy/v1/Eviction":
"io.k8s.api.apps/v1beta1/Scale",
"io.k8s.api.apps/v1beta2/Scale",
"io.k8s.api.authentication/v1/TokenRequest",
"io.k8s.api.authentication/v1/TokenReview",
"io.k8s.api.authentication/v1alpha1/SelfSubjectReview",
"io.k8s.api.authentication/v1beta1/SelfSubjectReview",
"io.k8s.api.authentication/v1/SelfSubjectReview",
"io.k8s.api.authentication/v1beta1/TokenReview",
"io.k8s.api.authorization/v1/LocalSubjectAccessReview",
"io.k8s.api.authorization/v1/SelfSubjectAccessReview",
"io.k8s.api.authorization/v1/SelfSubjectRulesReview",
"io.k8s.api.authorization/v1/SubjectAccessReview",
"io.k8s.api.authorization/v1beta1/LocalSubjectAccessReview",
"io.k8s.api.authorization/v1beta1/SelfSubjectAccessReview",
"io.k8s.api.authorization/v1beta1/SelfSubjectRulesReview",
"io.k8s.api.authorization/v1beta1/SubjectAccessReview",
"io.k8s.api.autoscaling/v1/Scale",
"io.k8s.api.core/v1/ComponentStatus",
"io.k8s.api.core/v1/ComponentStatusList",
"io.k8s.api.extensions/v1beta1/Scale",
"io.k8s.api.policy/v1beta1/Eviction",
"io.k8s.api.policy/v1/Eviction":
return false
}

Expand Down Expand Up @@ -260,7 +260,7 @@ func GVKFromRef(ref string) schema.GroupVersionKind {
gvk := schema.GroupVersionKind{
Kind: split[len(split)-1],
Version: split[len(split)-2],
Group: split[len(split)-3],
Group: strings.Join(split[:len(split)-2], "."),
}
return gvk
}
Expand Down Expand Up @@ -478,7 +478,8 @@ func createGroups(definitionsJSON map[string]any, allowHyphens bool) []GroupConf
func createCanonicalGroups(definitionsJSON map[string]any) map[string]string {
// Hard-code some canonical groups as they don't contain the `x-kubernetes-group-version-kind` field.
canonicalGroups := map[string]string{
"meta": "meta",
"io.k8s.apimachinery.pkg.apis.meta": "meta",
"io.k8s.apimachinery.pkg": "pkg",
}

for defName, defData := range definitionsJSON {
Expand All @@ -493,7 +494,7 @@ func createCanonicalGroups(definitionsJSON map[string]any) map[string]string {
gvk := gvks[0].(map[string]any)
group := gvk["group"].(string)
// The "core" group shows up as "" in the OpenAPI spec.
if group == "" && def.gvk.Group == "core" {
if group == "" && def.gvk.Group == "io.k8s.api.core" {
group = "core"
}
def.canonicalGroup = group
Expand Down Expand Up @@ -687,7 +688,7 @@ func createKinds(definitions []definition, canonicalGroups map[string]string, al
}

// These resources are hard-coded as lists as they do not adhere to the normal conventions.
if d.gvk.Group == "meta" &&
if d.gvk.Group == "io.k8s.apimachinery.pkg.apis.meta" &&
d.gvk.Version == "v1" &&
(d.gvk.Kind == "APIResourceList" || d.gvk.Kind == "APIGroupList") {
isList = true
Expand Down Expand Up @@ -751,10 +752,22 @@ func createVersions(kinds []KindConfig) []VersionConfig {
}

// createGroupsFromVersions creates a `GroupConfig` for each group of versions.
// Note: we have always stored the last segment of a Swagger definition as the group name,
// but this resulted in collisions between packages that may have the same last segment.
// For example, `io.k8s.testpackage` and `com.testpackage` would both be stored as `testpackage`.
// To fix this, we now store the full path as the group name when keying the canonical group map.
// However, to ensure we don't break users, we MUST ensure that the types are still generated in
// the `testpackage` package, and not individual `io.k8s.testpackage` and `com.testpackage` packages.
// It is here that we ensure that the group name is still just the last segment.
func createGroupsFromVersions(versions []VersionConfig) []GroupConfig {
groupMap := make(map[string][]VersionConfig)
for _, version := range versions {
groupMap[version.gv.Group] = append(groupMap[version.gv.Group], version)
// Get the last segment of the group name so we don't break users.
groupBackwardsCompatible := version.gv.Group
s := strings.Split(groupBackwardsCompatible, ".")
groupBackwardsCompatible = s[len(s)-1]

groupMap[groupBackwardsCompatible] = append(groupMap[groupBackwardsCompatible], version)
}

var groups []GroupConfig
Expand Down
82 changes: 65 additions & 17 deletions provider/pkg/gen/typegen_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -430,13 +430,13 @@ func TestCreateDefinitions(t *testing.T) {
}

canonicalGroups := map[string]string{
"apps": "apps",
"core": "core",
"io.k8s.api.apps": "apps",
"io.k8s.api.core": "core",
}

expected := []definition{
{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "Deployment"},
name: "io.k8s.api.apps.v1.Deployment",
data: map[string]any{
"properties": map[string]any{
Expand All @@ -451,7 +451,7 @@ func TestCreateDefinitions(t *testing.T) {
canonicalGroup: "apps",
},
{
gvk: schema.GroupVersionKind{Group: "core", Version: "v1", Kind: "Pod"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.core", Version: "v1", Kind: "Pod"},
name: "io.k8s.api.core.v1.Pod",
data: map[string]any{
"properties": map[string]any{
Expand All @@ -470,6 +470,7 @@ func TestCreateDefinitions(t *testing.T) {
actual := createDefinitions(definitionsJSON, canonicalGroups)
assert.Equal(t, expected, actual)
}

func TestCreateCanonicalGroups(t *testing.T) {
tests := []struct {
name string
Expand All @@ -491,9 +492,9 @@ func TestCreateCanonicalGroups(t *testing.T) {
},
},
expectedGroups: map[string]string{
"core": "core",
"apps": "apps",
"meta": "meta",
"io.k8s.api.core": "core",
"io.k8s.api.apps": "apps",
"io.k8s.apimachinery.pkg.apis.meta": "meta",
},
},
{
Expand All @@ -506,7 +507,7 @@ func TestCreateCanonicalGroups(t *testing.T) {
},
},
expectedGroups: map[string]string{
"meta": "meta",
"io.k8s.apimachinery.pkg.apis.meta": "meta",
},
},
}
Expand Down Expand Up @@ -603,7 +604,7 @@ func TestMakeSchemaTypeSpec(t *testing.T) {
"$ref": "#/definitions/io.k8s.api.apps.v1.Deployment",
},
canonicalGroups: map[string]string{
"apps": "apps",
"io.k8s.api.apps": "apps",
},
expectedTypeSpec: pschema.TypeSpec{
Ref: "#/types/kubernetes:apps/v1:Deployment",
Expand All @@ -628,7 +629,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "top-level resource with ObjectMeta",
definition: definition{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "Deployment"},
data: map[string]any{
"properties": map[string]any{
"metadata": map[string]any{
Expand All @@ -645,7 +646,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "top-level resource with ListMeta",
definition: definition{
gvk: schema.GroupVersionKind{Group: "core", Version: "v1", Kind: "PodList"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.core", Version: "v1", Kind: "PodList"},
data: map[string]any{
"properties": map[string]any{
"metadata": map[string]any{
Expand All @@ -662,7 +663,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "non-top-level resource",
definition: definition{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "ReplicaSet"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "ReplicaSet"},
data: map[string]any{
"properties": map[string]any{
"spec": map[string]any{
Expand All @@ -679,10 +680,10 @@ func TestIsTopLevel(t *testing.T) {
{
name: "imperative resource type",
definition: definition{
gvk: schema.GroupVersionKind{Group: "core", Version: "v1", Kind: "Status"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.authentication", Version: "v1", Kind: "TokenRequest"},
data: map[string]any{
"x-kubernetes-group-version-kind": []any{
map[string]any{"group": "core", "version": "v1", "kind": "Status"},
map[string]any{"group": "authentication", "version": "v1", "kind": "TokenRequest"},
},
},
},
Expand All @@ -691,7 +692,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "missing properties",
definition: definition{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "Deployment"},
data: map[string]any{
"x-kubernetes-group-version-kind": []any{
map[string]any{"group": "apps", "version": "v1", "kind": "Deployment"},
Expand All @@ -703,7 +704,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "missing metadata",
definition: definition{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "Deployment"},
data: map[string]any{
"properties": map[string]any{
"spec": map[string]any{
Expand All @@ -720,7 +721,7 @@ func TestIsTopLevel(t *testing.T) {
{
name: "missing $ref in metadata",
definition: definition{
gvk: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"},
gvk: schema.GroupVersionKind{Group: "io.k8s.api.apps", Version: "v1", Kind: "Deployment"},
data: map[string]any{
"properties": map[string]any{
"metadata": map[string]any{
Expand All @@ -743,3 +744,50 @@ func TestIsTopLevel(t *testing.T) {
})
}
}

func TestGVKFromRef(t *testing.T) {
tests := []struct {
ref string
expected schema.GroupVersionKind
}{
{
ref: "io.k8s.api.apps.v1.Deployment",
expected: schema.GroupVersionKind{
Group: "io.k8s.api.apps",
Version: "v1",
Kind: "Deployment",
},
},
{
ref: "io.k8s.api.core.v1.Pod",
expected: schema.GroupVersionKind{
Group: "io.k8s.api.core",
Version: "v1",
Kind: "Pod",
},
},
{
ref: "io.k8s.api.extensions.v1beta1.Ingress",
expected: schema.GroupVersionKind{
Group: "io.k8s.api.extensions",
Version: "v1beta1",
Kind: "Ingress",
},
},
{
ref: "io.k8s.api.networking.v1.NetworkPolicy",
expected: schema.GroupVersionKind{
Group: "io.k8s.api.networking",
Version: "v1",
Kind: "NetworkPolicy",
},
},
}

for _, tt := range tests {
t.Run(tt.ref, func(t *testing.T) {
actual := GVKFromRef(tt.ref)
assert.Equal(t, tt.expected, actual)
})
}
}

0 comments on commit a5873e0

Please sign in to comment.