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.

Sign up for GitHub

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

Add KptFileGVK to expose typed GVK information for Kptfile #3584

Merged
merged 1 commit into from
Oct 14, 2022
Merged

Add KptFileGVK to expose typed GVK information for Kptfile #3584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion commands/live/migrate/migratecmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ func (mr *Runner) retrieveConfigMapInv(reader io.Reader, args []string) (invento

// cli-utils treats any resource that contains the inventory-id label as an inventory object. We should
// ignore any inventories that are stored as ResourceGroup resources since they do not need migration.
if cmInvObj.GetKind() == rgfilev1alpha1.RGFileKind {
if cmInvObj.GetKind() == rgfilev1alpha1.ResourceGroupGVK().Kind {
// No ConfigMap inventory means the migration has already run before.
fmt.Fprintln(mr.ioStreams.Out, "no ConfigMap inventory...completed")
return nil, &inventory.NoInventoryObjError{}
Expand Down
10 changes: 6 additions & 4 deletions commands/pkg/get/cmdget_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,9 @@ func TestCmd_execute(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind,
},
},
Upstream: &kptfilev1.Upstream{
Type: kptfilev1.GitOrigin,
Expand Down Expand Up @@ -126,8 +127,9 @@ func TestCmdMainBranch_execute(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind,
},
},
Upstream: &kptfilev1.Upstream{
Type: kptfilev1.GitOrigin,
Expand Down
2 changes: 1 addition & 1 deletion internal/builtins/pkg_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const (

var (
configMapGVK = resid.NewGvk("", "v1", "ConfigMap")
kptfileGVK = resid.NewGvk(kptfilev1.KptFileGroup, kptfilev1.KptFileVersion, kptfilev1.KptFileKind)
kptfileGVK = resid.NewGvk(kptfilev1.KptFileGVK().Group, kptfilev1.KptFileGVK().Version, kptfilev1.KptFileGVK().Kind)
)

// PackageContextGenerator is a built-in KRM function that generates
Expand Down
4 changes: 2 additions & 2 deletions internal/errors/resolver/pkg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func resolveNestedErr(err error, tmplArgs map[string]interface{}) (ResolvedResul

var deprecatedv1alpha1KptfileError *pkg.DeprecatedKptfileError
if errors.As(err, &deprecatedv1alpha1KptfileError) &&
deprecatedv1alpha1KptfileError.Version == pkg.DeprecatedKptfileVersions[0] {
deprecatedv1alpha1KptfileError.Version == "v1alpha1" {
tmplArgs["version"] = deprecatedv1alpha1KptfileError.Version
errMsg := deprecatedv1Alpha1KptfileMsg
return ResolvedResult{
Expand All @@ -109,7 +109,7 @@ func resolveNestedErr(err error, tmplArgs map[string]interface{}) (ResolvedResul

var deprecatedv1alpha2KptfileError *pkg.DeprecatedKptfileError
if errors.As(err, &deprecatedv1alpha2KptfileError) &&
deprecatedv1alpha1KptfileError.Version == pkg.DeprecatedKptfileVersions[1] {
deprecatedv1alpha1KptfileError.Version == "v1alpha2" {
tmplArgs["version"] = deprecatedv1alpha2KptfileError.Version
errMsg := deprecatedv1Alpha2KptfileMsg
return ResolvedResult{
Expand Down
40 changes: 23 additions & 17 deletions internal/pkg/pkg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
kptfilev1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
rgfilev1alpha1 "github.com/GoogleContainerTools/kpt/pkg/api/resourcegroup/v1alpha1"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/kubectl/pkg/util/slice"
"sigs.k8s.io/kustomize/kyaml/filesys"
"sigs.k8s.io/kustomize/kyaml/kio"
"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
Expand All @@ -45,17 +44,17 @@ const (
pkgPathAnnotation = "internal.config.kubernetes.io/package-path"
)

var DeprecatedKptfileVersions = []string{
"v1alpha1",
"v1alpha2",
var DeprecatedKptfileVersions = []schema.GroupVersionKind{
kptfilev1.KptFileGVK().GroupKind().WithVersion("v1alpha1"),
kptfilev1.KptFileGVK().GroupKind().WithVersion("v1alpha2"),
}

// MatchAllKRM represents set of glob pattern to match all KRM
// resources including Kptfile.
var MatchAllKRM = append([]string{kptfilev1.KptFileName}, kio.MatchAll...)

var SupportedKptfileVersions = []string{
kptfilev1.KptFileVersion,
var SupportedKptfileVersions = []schema.GroupVersionKind{
kptfilev1.KptFileGVK(),
}

// KptfileError records errors regarding reading or parsing of a Kptfile.
Expand Down Expand Up @@ -286,18 +285,15 @@ func CheckKptfileVersion(content []byte) error {
if err != nil {
return err
}
gvk := gv.WithKind(kind)

switch {
// If the resource type matches what we are looking for, just return nil.
case gv.Group == kptfilev1.KptFileGroup &&
kind == kptfilev1.KptFileKind &&
isSupportedKptfileVersion(gv.Version):
case isSupportedKptfileVersion(gvk):
return nil
// If the kind and group is correct and the version is a known deprecated
// schema for the Kptfile, return DeprecatedKptfileError.
case gv.Group == kptfilev1.KptFileGroup &&
kind == kptfilev1.KptFileKind &&
isDeprecatedKptfileVersion(gv.Version):
case isDeprecatedKptfileVersion(gvk):
return &DeprecatedKptfileError{
Version: gv.Version,
}
Expand All @@ -310,12 +306,22 @@ func CheckKptfileVersion(content []byte) error {
}
}

func isDeprecatedKptfileVersion(version string) bool {
return slice.ContainsString(DeprecatedKptfileVersions, version, nil)
func isDeprecatedKptfileVersion(gvk schema.GroupVersionKind) bool {
for _, v := range DeprecatedKptfileVersions {
if v == gvk {
return true
}
}
return false
}

func isSupportedKptfileVersion(version string) bool {
return slice.ContainsString(SupportedKptfileVersions, version, nil)
func isSupportedKptfileVersion(gvk schema.GroupVersionKind) bool {
for _, v := range SupportedKptfileVersions {
if v == gvk {
return true
}
}
return false
}

// Pipeline returns the Pipeline section of the pkg's Kptfile.
Expand Down Expand Up @@ -777,7 +783,7 @@ func filterResourceGroups(input []*yaml.RNode) (output []*yaml.RNode, err error)
return nil, fmt.Errorf("failed to read metadata for resource %w", err)
}
// Filter out any non-ResourceGroup files.
if !(meta.APIVersion == rgfilev1alpha1.RGFileAPIVersion && meta.Kind == rgfilev1alpha1.RGFileKind) {
if !(meta.APIVersion == rgfilev1alpha1.ResourceGroupGVK().GroupVersion().String() && meta.Kind == rgfilev1alpha1.ResourceGroupGVK().Kind) {
continue
}

Expand Down
19 changes: 11 additions & 8 deletions internal/util/fetch/fetch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,9 @@ func TestCommand_Run(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind,
},
},
Upstream: &kptfilev1.Upstream{
Type: "git",
Expand Down Expand Up @@ -258,8 +259,8 @@ func TestCommand_Run_subdir(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind},
},
Upstream: &kptfilev1.Upstream{
Type: kptfilev1.GitOrigin,
Expand Down Expand Up @@ -334,8 +335,9 @@ func TestCommand_Run_branch(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind,
},
},
Upstream: &kptfilev1.Upstream{
Type: kptfilev1.GitOrigin,
Expand Down Expand Up @@ -415,8 +417,9 @@ func TestCommand_Run_tag(t *testing.T) {
},
},
TypeMeta: yaml.TypeMeta{
APIVersion: kptfilev1.KptFileAPIVersion,
Kind: kptfilev1.KptFileKind},
APIVersion: kptfilev1.KptFileGVK().GroupVersion().String(),
Kind: kptfilev1.KptFileGVK().Kind,
},
},
Upstream: &kptfilev1.Upstream{
Type: "git",
Expand Down
21 changes: 17 additions & 4 deletions pkg/api/fnresult/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package v1

import (
kptfilev1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/kustomize/kyaml/fn/framework"
"sigs.k8s.io/kustomize/kyaml/yaml"
)
Expand All @@ -42,12 +42,25 @@ type Result struct {
}

const (
ResultListKind = "FunctionResultList"
ResultListGroup = kptfilev1.KptFileGroup
ResultListVersion = kptfilev1.KptFileVersion
// Deprecated: prefer ResultListGVK
ResultListKind = "FunctionResultList"
// Deprecated: prefer ResultListGVK
ResultListGroup = "kpt.dev"
// Deprecated: prefer ResultListGVK
ResultListVersion = "v1"
// Deprecated: prefer ResultListGVK
ResultListAPIVersion = ResultListGroup + "/" + ResultListVersion
)

// KptFileGVK is the GroupVersionKind of FunctionResultList objects
func ResultListGVK() schema.GroupVersionKind {
return schema.GroupVersionKind{
Group: "kpt.dev",
Version: "v1",
Kind: "FunctionResultList",
}
}

// ResultList contains aggregated results from multiple functions
type ResultList struct {
yaml.ResourceMeta `yaml:",inline"`
Expand Down
26 changes: 22 additions & 4 deletions pkg/api/kptfile/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,35 @@ package v1
import (
Copy link
Contributor

@yuwenma yuwenma Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's our long term plan about the Kptfile schema @mortent @droot? I think kpt should import the Kptfile schema from kpt-functions-sdk, so as users and other tools which use the Kptfile schema do not need to import it from Kpt, and this can avoid expected Kpt exposure and dependent loop problems. For example, the go SDK also needs to use the Kptfile schema and it imports from the kpt-functions-sdk/go/api. This change could potentially make the go sdk incompatible with the kpt fn.

This is not a blocker for this PR. I can pull the change to the kpt-functions-sdk this time. But I'm a little bit worried that the go sdk and kpt could be out of sync if we only make the change on the kpt side and not converge the Kptfile schema.

Copy link
Contributor

@mortent mortent Oct 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuwenma Why does the kpt-functions-sdk have a copy of the Kptfile type? It seems like the kpt repo would be the natural place have it and the sdk would import it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the exact legacy reasons, but I think it makes sense for 2 reasons

  1. Users who need to use the Kptfile type (i.e. to write their own kptfile relevant functions), they can import from go-sdk and do not need to import the entire kpt pkg.
  2. kpt, as a CLI, is not designed for importing purpose so it should not worry too much about supporting its dependants.

It seems like the kpt repo would be the natural place have it and the sdk would import it.

I see kpt/porch has imported go/fn in several places. e.g. porch function wrapper main.go, porch builtin function runtime, which is aligned with the library dependencies I described above. And because of that, I don't think sdk can then import kpt/porch.

"fmt"

"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/kustomize/kyaml/yaml"
)

const (
KptFileName = "Kptfile"
KptFileKind = "Kptfile"
KptFileGroup = "kpt.dev"
KptFileVersion = "v1"
KptFileName = "Kptfile"

// Deprecated: prefer KptFileGVK
KptFileKind = "Kptfile"

// Deprecated: prefer KptFileGVK
KptFileGroup = "kpt.dev"

// Deprecated: prefer KptFileGVK
KptFileVersion = "v1"

// Deprecated: prefer KptFileGVK
KptFileAPIVersion = KptFileGroup + "/" + KptFileVersion
)

// KptFileGVK is the GroupVersionKind of Kptfile objects
func KptFileGVK() schema.GroupVersionKind {
return schema.GroupVersionKind{
Group: "kpt.dev",
Version: "v1",
Kind: "Kptfile",
}
}

// TypeMeta is the TypeMeta for KptFile instances.
var TypeMeta = yaml.ResourceMeta{
TypeMeta: yaml.TypeMeta{
Expand Down
25 changes: 20 additions & 5 deletions pkg/api/resourcegroup/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,35 @@
package v1alpha1

import (
"k8s.io/apimachinery/pkg/runtime/schema"
"sigs.k8s.io/cli-utils/pkg/common"
"sigs.k8s.io/kustomize/kyaml/yaml"
)

const (
RGFileName = "resourcegroup.yaml"
RGFileKind = "ResourceGroup"
RGFileGroup = "kpt.dev"
RGFileVersion = "v1alpha1"
RGFileAPIVersion = RGFileGroup + "/" + RGFileVersion
RGFileName = "resourcegroup.yaml"
// RGInventoryIDLabel is the label name used for storing an inventory ID.
RGInventoryIDLabel = common.InventoryLabel

// Deprecated: prefer ResourceGroupGVK
RGFileKind = "ResourceGroup"
// Deprecated: prefer ResourceGroupGVK
RGFileGroup = "kpt.dev"
// Deprecated: prefer ResourceGroupGVK
RGFileVersion = "v1alpha1"
// Deprecated: prefer ResourceGroupGVK
RGFileAPIVersion = RGFileGroup + "/" + RGFileVersion
)

// ResourceGroupGVK is the GroupVersionKind of ResourceGroup objects
func ResourceGroupGVK() schema.GroupVersionKind {
return schema.GroupVersionKind{
Group: "kpt.dev",
Version: "v1alpha1",
Kind: "ResourceGroup",
}
}

// DefaultMeta is the ResourceMeta for ResourceGroup instances.
var DefaultMeta = yaml.ResourceMeta{
TypeMeta: yaml.TypeMeta{
Expand Down
18 changes: 14 additions & 4 deletions pkg/live/load.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
kptfilev1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
rgfilev1alpha1 "github.com/GoogleContainerTools/kpt/pkg/api/resourcegroup/v1alpha1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/klog/v2"
"k8s.io/kubectl/pkg/cmd/util"
"sigs.k8s.io/cli-utils/pkg/common"
"sigs.k8s.io/cli-utils/pkg/inventory"
Expand Down Expand Up @@ -195,8 +197,7 @@ type InventoryFilter struct {
}

func (i *InventoryFilter) Filter(object *yaml.RNode) (*yaml.RNode, error) {
if object.GetApiVersion() != kptfilev1.KptFileAPIVersion ||
object.GetKind() != kptfilev1.KptFileKind {
if GroupVersionKindForObject(object) != kptfilev1.KptFileGVK() {
return object, nil
}

Expand All @@ -220,9 +221,18 @@ type RGFilter struct {
Inventories []*rgfilev1alpha1.ResourceGroup
}

// GroupVersionKindForObject extracts the group/version/kind from an RNode holding a kubernetes object.
func GroupVersionKindForObject(object *yaml.RNode) schema.GroupVersionKind {
apiVersion := object.GetApiVersion()
gv, err := schema.ParseGroupVersion(apiVersion)
if err != nil {
klog.Warningf("error parsing apiVersion=%q", apiVersion)
}
return gv.WithKind(object.GetKind())
}

func (r *RGFilter) Filter(object *yaml.RNode) (*yaml.RNode, error) {
if object.GetApiVersion() != rgfilev1alpha1.RGFileAPIVersion ||
object.GetKind() != rgfilev1alpha1.RGFileKind {
if GroupVersionKindForObject(object) != rgfilev1alpha1.ResourceGroupGVK() {
return object, nil
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/live/rgpath.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (r *ResourceGroupPathManifestReader) Read() ([]*unstructured.Unstructured,
// Skip if current file is a ResourceGroup resource. We do not want to apply/delete any ResourceGroup CRs when we
// run any `kpt live` commands on a package. Instead, we have specific logic in place for handling ResourceGroups in
// the live cluster.
if u.GetKind() == rgfilev1alpha1.RGFileKind && u.GetAPIVersion() == rgfilev1alpha1.DefaultMeta.APIVersion {
if u.GroupVersionKind() == rgfilev1alpha1.ResourceGroupGVK() {
continue
}
objs = append(objs, u)
Expand Down
10 changes: 2 additions & 8 deletions pkg/live/rgstream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,8 @@ import (

var (
excludedGKs = []schema.GroupKind{
{
Group: kptfilev1.KptFileGroup,
Kind: kptfilev1.KptFileKind,
},
{
Group: rgfilev1alpha1.RGFileGroup,
Kind: rgfilev1alpha1.RGFileKind,
},
kptfilev1.KptFileGVK().GroupKind(),
rgfilev1alpha1.ResourceGroupGVK().GroupKind(),
}
)

Expand Down