diff --git a/changelog/v0.17.2/fix-equality-segfault.yaml b/changelog/v0.17.2/fix-equality-segfault.yaml new file mode 100644 index 000000000..fbe75b9e0 --- /dev/null +++ b/changelog/v0.17.2/fix-equality-segfault.yaml @@ -0,0 +1,6 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/skv2/issues/207 + description: >- + Fix equality for protos, since we were using the `proto.Equals` on k8s resources that implemented the interface + but should have been using reflection. diff --git a/changelog/v0.17.2/kubeconfig-loader-check-recommended.yaml b/changelog/v0.17.2/kubeconfig-loader-check-recommended.yaml new file mode 100644 index 000000000..d876fa6fd --- /dev/null +++ b/changelog/v0.17.2/kubeconfig-loader-check-recommended.yaml @@ -0,0 +1,4 @@ +changelog: + - type: FIX + issueLink: https://github.com/solo-io/skv2/issues/205 + description: Add recommended kubeconfig directory path to kubeconfig loader loading rules. diff --git a/pkg/controllerutils/equality_test.go b/pkg/controllerutils/equality_test.go index 8f8c59f0a..58d04aa12 100644 --- a/pkg/controllerutils/equality_test.go +++ b/pkg/controllerutils/equality_test.go @@ -5,6 +5,7 @@ import ( . "github.com/onsi/gomega" things_test_io_v1 "github.com/solo-io/skv2/codegen/test/api/things.test.io/v1" v1 "k8s.io/api/core/v1" + rbac_authorization_k8s_io_v1 "k8s.io/api/rbac/v1" k8s_meta "k8s.io/apimachinery/pkg/apis/meta/v1" . "github.com/solo-io/skv2/pkg/controllerutils" @@ -81,8 +82,7 @@ var _ = Describe("ObjectsEqual", func() { equal := ObjectsEqual(obj1, obj2) Expect(equal).To(BeFalse()) }) - - It("asserts equality on two proto.Message objects even if they are passed in by value", func() { + It("asserts equality on two proto.Message objects (protov2, with unknown fields) even if they are passed in by reference", func() { obj1 := &things_test_io_v1.Paint{ Spec: things_test_io_v1.PaintSpec{ Color: &things_test_io_v1.PaintColor{ @@ -107,6 +107,46 @@ var _ = Describe("ObjectsEqual", func() { equal := ObjectsEqual(obj1, obj2) Expect(equal).To(BeTrue()) }) + It("asserts equality on two proto.Message objects even if they are 'fake' (i.e., k8s protos implementing github protov1, not google protov2 protos)", func() { + obj1 := &rbac_authorization_k8s_io_v1.RoleBinding{ + TypeMeta: k8s_meta.TypeMeta{}, + ObjectMeta: k8s_meta.ObjectMeta{ + Name: "sample-role-binding", + Namespace: "default", + Labels: map[string]string{"k": "v"}, + }, + Subjects: []rbac_authorization_k8s_io_v1.Subject{{ + Kind: "ServiceAccount", + Name: "sample-target", + Namespace: "default", + }}, + RoleRef: rbac_authorization_k8s_io_v1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "sample-controller", + }, + } + obj2 := &rbac_authorization_k8s_io_v1.RoleBinding{ + TypeMeta: k8s_meta.TypeMeta{}, + ObjectMeta: k8s_meta.ObjectMeta{ + Name: "sample-role-binding", + Namespace: "default", + Labels: map[string]string{"k": "v"}, + }, + Subjects: []rbac_authorization_k8s_io_v1.Subject{{ + Kind: "ServiceAccount", + Name: "sample-target", + Namespace: "default", + }}, + RoleRef: rbac_authorization_k8s_io_v1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: "sample-controller", + }, + } + equal := ObjectsEqual(obj1, obj2) + Expect(equal).To(BeTrue()) + }) }) var _ = Describe("ObjectStatusesEqual", func() { diff --git a/pkg/equalityutils/deep_equal.go b/pkg/equalityutils/deep_equal.go index ab925e633..fc4584278 100644 --- a/pkg/equalityutils/deep_equal.go +++ b/pkg/equalityutils/deep_equal.go @@ -3,19 +3,19 @@ package equalityutils import ( "reflect" - "github.com/golang/protobuf/proto" + protoV2 "google.golang.org/protobuf/proto" ) // DeepEqual should be used in place of reflect.DeepEqual when the type of an object is unknown and may be a proto message. // see https://github.com/golang/protobuf/issues/1173 for details on why reflect.DeepEqual no longer works for proto messages func DeepEqual(val1, val2 interface{}) bool { - protoVal1, isProto := val1.(proto.Message) + protoVal1, isProto := val1.(protoV2.Message) if isProto { - protoVal2, isProto := val2.(proto.Message) + protoVal2, isProto := val2.(protoV2.Message) if !isProto { return false // different types } - return proto.Equal(protoVal1, protoVal2) + return protoV2.Equal(protoVal1, protoVal2) } return reflect.DeepEqual(val1, val2) } diff --git a/pkg/multicluster/kubeconfig/loader.go b/pkg/multicluster/kubeconfig/loader.go index c9d13eec6..9168f20ac 100644 --- a/pkg/multicluster/kubeconfig/loader.go +++ b/pkg/multicluster/kubeconfig/loader.go @@ -1,11 +1,6 @@ package kubeconfig import ( - "fmt" - "os" - "os/user" - "path" - "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" @@ -13,21 +8,16 @@ import ( // Fetch ClientConfig. If kubeConfigPath is not specified, retrieve the kubeconfig from environment in which this is invoked. // Override the API Server URL and current context if specified. -// Copied and modified from https://github.com/kubernetes-sigs/controller-runtime/blob/cb7f85860a8cde7259b35bb84af1fdcb02c098f2/pkg/client/config/config.go#L135 func GetClientConfigWithContext(kubeConfigPath, kubeContext, apiServerUrl string) (clientcmd.ClientConfig, error) { + + // default loading rules checks for KUBECONFIG env var loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() + // also check recommended default kubeconfig file locations + loadingRules.Precedence = append(loadingRules.Precedence, clientcmd.RecommendedHomeFile) + // explicit path overrides all loading rules, will error if not found if kubeConfigPath != "" { loadingRules.ExplicitPath = kubeConfigPath - } else { - // Fetch kubeconfig from environment in which this is invoked - if _, ok := os.LookupEnv("HOME"); !ok { - u, err := user.Current() - if err != nil { - return nil, fmt.Errorf("could not get current user: %v", err) - } - loadingRules.Precedence = append(loadingRules.Precedence, path.Join(u.HomeDir, clientcmd.RecommendedHomeDir, clientcmd.RecommendedFileName)) - } } overrides := &clientcmd.ConfigOverrides{}