From 93ebd0f94946e7934d441e394091dd8fb08f85b0 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Tue, 26 Jan 2021 17:34:27 -0500 Subject: [PATCH 1/4] internal/plog: add Enabled() Signed-off-by: Andrew Keesler --- internal/plog/klog.go | 34 ++++++++++++++- internal/plog/level.go | 26 +++++------- internal/plog/level_test.go | 85 +++++++++++++++++++++---------------- 3 files changed, 92 insertions(+), 53 deletions(-) diff --git a/internal/plog/klog.go b/internal/plog/klog.go index aac73503..52759814 100644 --- a/internal/plog/klog.go +++ b/internal/plog/klog.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package plog @@ -7,6 +7,7 @@ import ( "sync" "github.com/spf13/pflag" + "k8s.io/klog/v2" ) //nolint: gochecknoglobals @@ -30,3 +31,34 @@ func RemoveKlogGlobalFlags() { panic("unsupported global klog flag set") } } + +func klogLevelForPlogLevel(plogLevel LogLevel) (klogLevel klog.Level) { + switch plogLevel { + case LevelWarning: + klogLevel = klogLevelWarning // unset means minimal logs (Error and Warning) + case LevelInfo: + klogLevel = klogLevelInfo + case LevelDebug: + klogLevel = klogLevelDebug + case LevelTrace: + klogLevel = klogLevelTrace + case LevelAll: + klogLevel = klogLevelAll + 100 // make all really mean all + default: + klogLevel = -1 + } + + return +} + +func getKlogLevel() klog.Level { + // hack around klog not exposing a Get method + for i := klog.Level(0); i < 256; i++ { + if klog.V(i).Enabled() { + continue + } + return i - 1 + } + + return -1 +} diff --git a/internal/plog/level.go b/internal/plog/level.go index b1a0bf11..20083d68 100644 --- a/internal/plog/level.go +++ b/internal/plog/level.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package plog @@ -39,26 +39,20 @@ const ( ) func ValidateAndSetLogLevelGlobally(level LogLevel) error { - var klogLogLevel int - - switch level { - case LevelWarning: - klogLogLevel = klogLevelWarning // unset means minimal logs (Error and Warning) - case LevelInfo: - klogLogLevel = klogLevelInfo - case LevelDebug: - klogLogLevel = klogLevelDebug - case LevelTrace: - klogLogLevel = klogLevelTrace - case LevelAll: - klogLogLevel = klogLevelAll + 100 // make all really mean all - default: + klogLevel := klogLevelForPlogLevel(level) + if klogLevel < 0 { return errInvalidLogLevel } - if _, err := logs.GlogSetter(strconv.Itoa(klogLogLevel)); err != nil { + if _, err := logs.GlogSetter(strconv.Itoa(int(klogLevel))); err != nil { panic(err) // programmer error } return nil } + +// Enabled returns whether the provided plog level is enabled, i.e., whether print statements at the +// provided level will show up. +func Enabled(level LogLevel) bool { + return getKlogLevel() >= klogLevelForPlogLevel(level) +} diff --git a/internal/plog/level_test.go b/internal/plog/level_test.go index 2f3ad9d4..a2fde0b6 100644 --- a/internal/plog/level_test.go +++ b/internal/plog/level_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package plog @@ -13,42 +13,50 @@ import ( ) func TestValidateAndSetLogLevelGlobally(t *testing.T) { - originalLogLevel := getKlogLevel(t) + originalLogLevel := getKlogLevel() + require.GreaterOrEqual(t, int(originalLogLevel), int(klog.Level(0)), "cannot get klog level") tests := []struct { - name string - level LogLevel - wantLevel klog.Level - wantErr string + name string + level LogLevel + wantLevel klog.Level + wantEnabled []LogLevel + wantErr string }{ { - name: "unset", - wantLevel: 0, + name: "unset", + wantLevel: 0, + wantEnabled: []LogLevel{LevelWarning}, }, { - name: "warning", - level: LevelWarning, - wantLevel: 0, + name: "warning", + level: LevelWarning, + wantLevel: 0, + wantEnabled: []LogLevel{LevelWarning}, }, { - name: "info", - level: LevelInfo, - wantLevel: 2, + name: "info", + level: LevelInfo, + wantLevel: 2, + wantEnabled: []LogLevel{LevelWarning, LevelInfo}, }, { - name: "debug", - level: LevelDebug, - wantLevel: 4, + name: "debug", + level: LevelDebug, + wantLevel: 4, + wantEnabled: []LogLevel{LevelWarning, LevelInfo, LevelDebug}, }, { - name: "trace", - level: LevelTrace, - wantLevel: 6, + name: "trace", + level: LevelTrace, + wantLevel: 6, + wantEnabled: []LogLevel{LevelWarning, LevelInfo, LevelDebug, LevelTrace}, }, { - name: "all", - level: LevelAll, - wantLevel: 108, + name: "all", + level: LevelAll, + wantLevel: 108, + wantEnabled: []LogLevel{LevelWarning, LevelInfo, LevelDebug, LevelTrace, LevelAll}, }, { name: "invalid level", @@ -66,26 +74,31 @@ func TestValidateAndSetLogLevelGlobally(t *testing.T) { err := ValidateAndSetLogLevelGlobally(tt.level) require.Equal(t, tt.wantErr, errString(err)) - require.Equal(t, tt.wantLevel, getKlogLevel(t)) + require.Equal(t, tt.wantLevel, getKlogLevel()) + + if tt.wantEnabled != nil { + allLevels := []LogLevel{LevelWarning, LevelInfo, LevelDebug, LevelTrace, LevelAll} + for _, level := range allLevels { + if contains(tt.wantEnabled, level) { + require.Truef(t, Enabled(level), "wanted %q to be enabled", level) + } else { + require.False(t, Enabled(level), "did not want %q to be enabled", level) + } + } + } }) } - require.Equal(t, originalLogLevel, getKlogLevel(t)) + require.Equal(t, originalLogLevel, getKlogLevel()) } -func getKlogLevel(t *testing.T) klog.Level { - t.Helper() - - // hack around klog not exposing a Get method - for i := klog.Level(0); i < 256; i++ { - if klog.V(i).Enabled() { - continue +func contains(haystack []LogLevel, needle LogLevel) bool { + for _, hay := range haystack { + if hay == needle { + return true } - return i - 1 } - - t.Fatal("unknown log level") - return 0 + return false } func errString(err error) string { From 93d25a349f7346e775786057966b4f296a1948ae Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 22 Jan 2021 08:33:24 -0500 Subject: [PATCH 2/4] hack: fix docker most recent tag check I think this stopped working when we starting using a specific registry in e0b94f47. Signed-off-by: Andrew Keesler --- hack/prepare-for-integration-tests.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 8eb07b54..4ac811a2 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -136,7 +136,7 @@ if ! tilt_mode; then tag=$(uuidgen) # always a new tag to force K8s to reload the image on redeploy if [[ "$skip_build" == "yes" ]]; then - most_recent_tag=$(docker images "$repo" --format "{{.Tag}}" | head -1) + most_recent_tag=$(docker images "$registry/$repo" --format "{{.Tag}}" | head -1) if [[ -n "$most_recent_tag" ]]; then tag="$most_recent_tag" do_build=no From efe1fa89feec076bea78596afc21573e1fcbcc06 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 12 Jan 2021 20:27:41 -0500 Subject: [PATCH 3/4] Allow multiple Pinnipeds to work on same cluster Yes, this is a huge commit. The middleware allows you to customize the API groups of all of the *.pinniped.dev API groups. Some notes about other small things in this commit: - We removed the internal/client package in favor of pkg/conciergeclient. The two packages do basically the same thing. I don't think we use the former anymore. - We re-enabled cluster-scoped owner assertions in the integration tests. This code was added in internal/ownerref. See a0546942 for when this assertion was removed. - Note: the middlware code is in charge of restoring the GV of a request object, so we should never need to write mutations that do that. - We updated the supervisor secret generation to no longer manually set an owner reference to the deployment since the middleware code now does this. I think we still need some way to make an initial event for the secret generator controller, which involves knowing the namespace and the name of the generated secret, so I still wired the deployment through. We could use a namespace/name tuple here, but I was lazy. Signed-off-by: Andrew Keesler Co-authored-by: Ryan Richard --- cmd/pinniped-supervisor/main.go | 8 +- cmd/pinniped/cmd/deprecated.go | 5 +- cmd/pinniped/cmd/kubeconfig.go | 17 +- cmd/pinniped/cmd/kubeconfig_test.go | 20 +- cmd/pinniped/cmd/login_oidc_test.go | 16 + cmd/pinniped/cmd/login_static_test.go | 16 + go.mod | 1 + go.sum | 1 + internal/apigroup/apigroup.go | 29 - internal/apigroup/apigroup_test.go | 36 - internal/client/client.go | 93 --- internal/client/client_test.go | 146 ---- internal/concierge/apiserver/apiserver.go | 38 +- internal/concierge/server/server.go | 79 +- internal/config/concierge/config.go | 9 + internal/config/concierge/config_test.go | 18 +- internal/config/supervisor/config.go | 9 + internal/config/supervisor/config_test.go | 10 + .../generator/supervisor_secrets.go | 26 +- .../generator/supervisor_secrets_test.go | 133 +-- .../integration/examplecontroller_test.go | 6 +- .../controllermanager/prepare_controllers.go | 10 +- internal/deploymentref/deploymentref.go | 27 +- internal/deploymentref/deploymentref_test.go | 127 +++ internal/groupsuffix/groupsuffix.go | 142 ++++ internal/groupsuffix/groupsuffix_test.go | 508 ++++++++++++ internal/kubeclient/copied.go | 70 ++ internal/kubeclient/gvk.go | 79 ++ internal/kubeclient/gvk_test.go | 222 +++++ internal/kubeclient/kubeclient.go | 16 +- internal/kubeclient/kubeclient_test.go | 775 ++++++++++++++++++ internal/kubeclient/middleware.go | 147 ++++ internal/kubeclient/middleware_test.go | 74 ++ internal/kubeclient/option.go | 4 + internal/kubeclient/path.go | 75 ++ internal/kubeclient/path_test.go | 231 ++++++ internal/kubeclient/roundtrip.go | 430 ++++++++-- internal/kubeclient/scheme.go | 84 ++ internal/kubeclient/scheme_test.go | 156 ++++ internal/kubeclient/serializer.go | 39 + internal/kubeclient/verb.go | 27 + internal/kubeclient/verb_test.go | 54 ++ internal/kubeclient/watch.go | 163 ++++ internal/mocks/mockroundtripper/generate.go | 6 + .../mockroundtripper/mockroundtripper.go | 53 ++ internal/ownerref/ownerref.go | 76 +- internal/ownerref/ownerref_test.go | 210 +++-- internal/plog/plog.go | 8 +- internal/testutil/doc.go | 7 + internal/testutil/fakekubeapi/fakekubeapi.go | 221 +++++ internal/testutil/round_trip.go | 70 ++ pkg/conciergeclient/conciergeclient.go | 11 +- pkg/conciergeclient/conciergeclient_test.go | 11 +- test/integration/cli_test.go | 10 +- .../concierge_api_serving_certs_test.go | 2 +- .../concierge_availability_test.go | 4 +- .../concierge_credentialrequest_test.go | 7 +- .../concierge_kubecertagent_test.go | 4 +- test/integration/kube_api_discovery_test.go | 21 +- test/integration/kubeclient_test.go | 83 +- test/integration/supervisor_discovery_test.go | 6 +- test/integration/supervisor_secrets_test.go | 2 +- ...ervisor_storage_garbage_collection_test.go | 4 +- test/integration/supervisor_storage_test.go | 4 +- test/library/access.go | 69 +- test/library/client.go | 36 +- test/library/dumplogs.go | 4 +- 67 files changed, 4285 insertions(+), 820 deletions(-) delete mode 100644 internal/apigroup/apigroup.go delete mode 100644 internal/apigroup/apigroup_test.go delete mode 100644 internal/client/client.go delete mode 100644 internal/client/client_test.go create mode 100644 internal/deploymentref/deploymentref_test.go create mode 100644 internal/groupsuffix/groupsuffix.go create mode 100644 internal/groupsuffix/groupsuffix_test.go create mode 100644 internal/kubeclient/copied.go create mode 100644 internal/kubeclient/gvk.go create mode 100644 internal/kubeclient/gvk_test.go create mode 100644 internal/kubeclient/kubeclient_test.go create mode 100644 internal/kubeclient/middleware.go create mode 100644 internal/kubeclient/middleware_test.go create mode 100644 internal/kubeclient/path.go create mode 100644 internal/kubeclient/path_test.go create mode 100644 internal/kubeclient/scheme.go create mode 100644 internal/kubeclient/scheme_test.go create mode 100644 internal/kubeclient/serializer.go create mode 100644 internal/kubeclient/verb.go create mode 100644 internal/kubeclient/verb_test.go create mode 100644 internal/kubeclient/watch.go create mode 100644 internal/mocks/mockroundtripper/generate.go create mode 100644 internal/mocks/mockroundtripper/mockroundtripper.go create mode 100644 internal/testutil/doc.go create mode 100644 internal/testutil/fakekubeapi/fakekubeapi.go create mode 100644 internal/testutil/round_trip.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 950e07d7..94d12817 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -37,6 +37,7 @@ import ( "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/deploymentref" "go.pinniped.dev/internal/downward" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" @@ -258,14 +259,15 @@ func run(podInfo *downward.PodInfo, cfg *supervisor.Config) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - // TODO remove code that relies on supervisorDeployment directly dref, supervisorDeployment, err := deploymentref.New(podInfo) if err != nil { return fmt.Errorf("cannot create deployment ref: %w", err) } - _ = *cfg.APIGroupSuffix // TODO: wire API group into kubeclient. - client, err := kubeclient.New(dref) + client, err := kubeclient.New( + dref, + kubeclient.WithMiddleware(groupsuffix.New(*cfg.APIGroupSuffix)), + ) if err != nil { return fmt.Errorf("cannot create k8s client: %w", err) } diff --git a/cmd/pinniped/cmd/deprecated.go b/cmd/pinniped/cmd/deprecated.go index aa229d4e..eeca0d1a 100644 --- a/cmd/pinniped/cmd/deprecated.go +++ b/cmd/pinniped/cmd/deprecated.go @@ -51,6 +51,7 @@ func legacyGetKubeconfigCommand(deps kubeconfigDeps) *cobra.Command { namespace string authenticatorType string authenticatorName string + apiGroupSuffix string ) cmd.Flags().StringVar(&token, "token", "", "Credential to include in the resulting kubeconfig output (Required)") @@ -59,6 +60,8 @@ func legacyGetKubeconfigCommand(deps kubeconfigDeps) *cobra.Command { cmd.Flags().StringVar(&namespace, "pinniped-namespace", "pinniped-concierge", "Namespace in which Pinniped was installed") cmd.Flags().StringVar(&authenticatorType, "authenticator-type", "", "Authenticator type (e.g., 'webhook', 'jwt')") cmd.Flags().StringVar(&authenticatorName, "authenticator-name", "", "Authenticator name") + cmd.Flags().StringVar(&apiGroupSuffix, "api-group-suffix", "pinniped.dev", "Concierge API group suffix") + mustMarkRequired(cmd, "token") plog.RemoveKlogGlobalFlags() cmd.RunE = func(cmd *cobra.Command, args []string) error { @@ -70,7 +73,7 @@ func legacyGetKubeconfigCommand(deps kubeconfigDeps) *cobra.Command { namespace: namespace, authenticatorName: authenticatorName, authenticatorType: authenticatorType, - apiGroupSuffix: "pinniped.dev", + apiGroupSuffix: apiGroupSuffix, }, }) } diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index 2ccd7eb0..1b108c02 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -26,23 +26,27 @@ import ( conciergev1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/authentication/v1alpha1" conciergeclientset "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" ) type kubeconfigDeps struct { getPathToSelf func() (string, error) - getClientset func(clientcmd.ClientConfig) (conciergeclientset.Interface, error) + getClientset func(clientConfig clientcmd.ClientConfig, apiGroupSuffix string) (conciergeclientset.Interface, error) } func kubeconfigRealDeps() kubeconfigDeps { return kubeconfigDeps{ getPathToSelf: os.Executable, - getClientset: func(clientConfig clientcmd.ClientConfig) (conciergeclientset.Interface, error) { + getClientset: func(clientConfig clientcmd.ClientConfig, apiGroupSuffix string) (conciergeclientset.Interface, error) { restConfig, err := clientConfig.ClientConfig() if err != nil { return nil, err } - client, err := kubeclient.New(kubeclient.WithConfig(restConfig)) + client, err := kubeclient.New( + kubeclient.WithConfig(restConfig), + kubeclient.WithMiddleware(groupsuffix.New(apiGroupSuffix)), + ) if err != nil { return nil, err } @@ -126,6 +130,11 @@ func kubeconfigCommand(deps kubeconfigDeps) *cobra.Command { //nolint:funlen func runGetKubeconfig(out io.Writer, deps kubeconfigDeps, flags getKubeconfigParams) error { + // Validate api group suffix and immediately return an error if it is invalid. + if err := groupsuffix.Validate(flags.concierge.apiGroupSuffix); err != nil { + return fmt.Errorf("invalid api group suffix: %w", err) + } + execConfig := clientcmdapi.ExecConfig{ APIVersion: clientauthenticationv1beta1.SchemeGroupVersion.String(), Args: []string{}, @@ -153,7 +162,7 @@ func runGetKubeconfig(out io.Writer, deps kubeconfigDeps, flags getKubeconfigPar if err != nil { return fmt.Errorf("could not load --kubeconfig/--kubeconfig-context: %w", err) } - clientset, err := deps.getClientset(clientConfig) + clientset, err := deps.getClientset(clientConfig, flags.concierge.apiGroupSuffix) if err != nil { return fmt.Errorf("could not configure Kubernetes client: %w", err) } diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 90d4635d..cbae7146 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -46,6 +46,7 @@ func TestGetKubeconfig(t *testing.T) { wantStdout string wantStderr string wantOptionsCount int + wantAPIGroupSuffix string }{ { name: "help flag passed", @@ -280,6 +281,17 @@ func TestGetKubeconfig(t *testing.T) { Error: only one of --static-token and --static-token-env can be specified `), }, + { + name: "invalid api group suffix", + args: []string{ + "--concierge-api-group-suffix", ".starts.with.dot", + }, + wantError: true, + wantStderr: here.Doc(` + Error: invalid api group suffix: 1 error(s): + - 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]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + `), + }, { name: "valid static token", args: []string{ @@ -492,6 +504,7 @@ func TestGetKubeconfig(t *testing.T) { env: [] provideClusterInfo: true `, base64.StdEncoding.EncodeToString(testCA.Bundle())), + wantAPIGroupSuffix: "tuna.io", }, } for _, tt := range tests { @@ -504,7 +517,12 @@ func TestGetKubeconfig(t *testing.T) { } return ".../path/to/pinniped", nil }, - getClientset: func(clientConfig clientcmd.ClientConfig) (conciergeclientset.Interface, error) { + getClientset: func(clientConfig clientcmd.ClientConfig, apiGroupSuffix string) (conciergeclientset.Interface, error) { + if tt.wantAPIGroupSuffix == "" { + require.Equal(t, "pinniped.dev", apiGroupSuffix) // "pinniped.dev" = api group suffix default + } else { + require.Equal(t, tt.wantAPIGroupSuffix, apiGroupSuffix) + } if tt.getClientsetErr != nil { return nil, tt.getClientsetErr } diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index a12050f0..41607e4e 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -120,6 +120,22 @@ func TestLoginOIDCCommand(t *testing.T) { Error: could not read --ca-bundle-data: illegal base64 data at input byte 7 `), }, + { + name: "invalid api group suffix", + args: []string{ + "--issuer", "test-issuer", + "--enable-concierge", + "--concierge-api-group-suffix", ".starts.with.dot", + "--concierge-authenticator-type", "jwt", + "--concierge-authenticator-name", "test-authenticator", + "--concierge-endpoint", "https://127.0.0.1:1234/", + }, + wantError: true, + wantStderr: here.Doc(` + Error: invalid concierge parameters: invalid api group suffix: 1 error(s): + - 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]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + `), + }, { name: "login error", args: []string{ diff --git a/cmd/pinniped/cmd/login_static_test.go b/cmd/pinniped/cmd/login_static_test.go index f1c30da3..0152dbd0 100644 --- a/cmd/pinniped/cmd/login_static_test.go +++ b/cmd/pinniped/cmd/login_static_test.go @@ -130,6 +130,22 @@ func TestLoginStaticCommand(t *testing.T) { Error: could not complete concierge credential exchange: some concierge error `), }, + { + name: "invalid api group suffix", + args: []string{ + "--token", "test-token", + "--enable-concierge", + "--concierge-api-group-suffix", ".starts.with.dot", + "--concierge-authenticator-type", "jwt", + "--concierge-authenticator-name", "test-authenticator", + "--concierge-endpoint", "https://127.0.0.1:1234/", + }, + wantError: true, + wantStderr: here.Doc(` + Error: invalid concierge parameters: invalid api group suffix: 1 error(s): + - 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]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') + `), + }, { name: "static token success", args: []string{ diff --git a/go.mod b/go.mod index 4f20afed..7c4f536f 100644 --- a/go.mod +++ b/go.mod @@ -41,6 +41,7 @@ require ( k8s.io/apiserver v0.20.1 k8s.io/client-go v0.20.1 k8s.io/component-base v0.20.1 + k8s.io/gengo v0.0.0-20201113003025-83324d819ded k8s.io/klog/v2 v2.4.0 k8s.io/kube-aggregator v0.20.1 k8s.io/utils v0.0.0-20201110183641-67b214c5f920 diff --git a/go.sum b/go.sum index 6dedac6c..77f9c531 100644 --- a/go.sum +++ b/go.sum @@ -1512,6 +1512,7 @@ k8s.io/code-generator v0.20.1/go.mod h1:UsqdF+VX4PU2g46NC2JRs4gc+IfrctnwHb76RNbW k8s.io/component-base v0.20.1 h1:6OQaHr205NSl24t5wOF2IhdrlxZTWEZwuGlLvBgaeIg= k8s.io/component-base v0.20.1/go.mod h1:guxkoJnNoh8LNrbtiQOlyp2Y2XFCZQmrcg2n/DeYNLk= k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0= +k8s.io/gengo v0.0.0-20201113003025-83324d819ded h1:JApXBKYyB7l9xx+DK7/+mFjC7A9Bt5A93FPvFD0HIFE= k8s.io/gengo v0.0.0-20201113003025-83324d819ded/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E= k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE= k8s.io/klog/v2 v2.2.0/go.mod h1:Od+F08eJP+W3HUb4pSrPpgp9DGU4GzlpG/TmITuYh/Y= diff --git a/internal/apigroup/apigroup.go b/internal/apigroup/apigroup.go deleted file mode 100644 index 065812a5..00000000 --- a/internal/apigroup/apigroup.go +++ /dev/null @@ -1,29 +0,0 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package apigroup provides centralized logic around Pinniped's API group parameterization. -package apigroup - -import ( - "fmt" - "strings" -) - -// defaultAPIGroupSuffix is the default suffix of the Concierge API group. Our generated code uses -// this suffix, so we know that we can replace this suffix with the configured API group suffix. -const defaultAPIGroupSuffix = "pinniped.dev" - -// Make constructs an API group from a baseAPIGroup and a parameterized apiGroupSuffix. -// -// We assume that all apiGroup's will end in "pinniped.dev", and therefore we can safely replace the -// reference to "pinniped.dev" with the provided apiGroupSuffix. If the provided baseAPIGroup does -// not end in "pinniped.dev", then this function will return an empty string and false. -// -// See Example_loginv1alpha1 and Example_string for more information on input/output pairs. -func Make(baseAPIGroup, apiGroupSuffix string) (string, bool) { - if !strings.HasSuffix(baseAPIGroup, defaultAPIGroupSuffix) { - return "", false - } - i := strings.LastIndex(baseAPIGroup, defaultAPIGroupSuffix) - return fmt.Sprintf("%s%s", baseAPIGroup[:i], apiGroupSuffix), true -} diff --git a/internal/apigroup/apigroup_test.go b/internal/apigroup/apigroup_test.go deleted file mode 100644 index 1f68c6d9..00000000 --- a/internal/apigroup/apigroup_test.go +++ /dev/null @@ -1,36 +0,0 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package apigroup - -import ( - "fmt" - "testing" - - "github.com/stretchr/testify/require" - - loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" -) - -func TestMakeError(t *testing.T) { - _, ok := Make("bad-suffix", "shouldnt-matter.com") - require.False(t, ok) -} - -func TestMakeSuffix(t *testing.T) { - s, ok := Make("something.pinniped.dev.something-else.pinniped.dev", "tuna.io") - require.Equal(t, "something.pinniped.dev.something-else.tuna.io", s) - require.True(t, ok) -} - -func Example_loginv1alpha1() { - s, _ := Make(loginv1alpha1.GroupName, "tuna.fish.io") - fmt.Println(s) - // Output: login.concierge.tuna.fish.io -} - -func Example_string() { - s, _ := Make("idp.supervisor.pinniped.dev", "marlin.io") - fmt.Println(s) - // Output: idp.supervisor.marlin.io -} diff --git a/internal/client/client.go b/internal/client/client.go deleted file mode 100644 index 78d7d49f..00000000 --- a/internal/client/client.go +++ /dev/null @@ -1,93 +0,0 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package client is a wrapper for interacting with Pinniped's CredentialRequest API. -package client - -import ( - "context" - "errors" - "fmt" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" - "k8s.io/client-go/tools/clientcmd" - clientcmdapi "k8s.io/client-go/tools/clientcmd/api" - - "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" - "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned" - "go.pinniped.dev/internal/kubeclient" -) - -// ErrLoginFailed is returned by ExchangeToken when the server rejects the login request. -var ErrLoginFailed = errors.New("login failed") - -// ExchangeToken exchanges an opaque token using the Pinniped TokenCredentialRequest API, returning a client-go ExecCredential valid on the target cluster. -func ExchangeToken(ctx context.Context, namespace string, authenticator corev1.TypedLocalObjectReference, token string, caBundle string, apiEndpoint string) (*clientauthenticationv1beta1.ExecCredential, error) { - client, err := getClient(apiEndpoint, caBundle) - if err != nil { - return nil, fmt.Errorf("could not get API client: %w", err) - } - - resp, err := client.LoginV1alpha1().TokenCredentialRequests(namespace).Create(ctx, &v1alpha1.TokenCredentialRequest{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: namespace, - }, - Spec: v1alpha1.TokenCredentialRequestSpec{ - Token: token, - Authenticator: authenticator, - }, - }, metav1.CreateOptions{}) - if err != nil { - return nil, fmt.Errorf("could not login: %w", err) - } - if resp.Status.Credential == nil || resp.Status.Message != nil { - if resp.Status.Message != nil { - return nil, fmt.Errorf("%w: %s", ErrLoginFailed, *resp.Status.Message) - } - return nil, fmt.Errorf("%w: unknown", ErrLoginFailed) - } - - return &clientauthenticationv1beta1.ExecCredential{ - TypeMeta: metav1.TypeMeta{ - Kind: "ExecCredential", - APIVersion: "client.authentication.k8s.io/v1beta1", - }, - Status: &clientauthenticationv1beta1.ExecCredentialStatus{ - ExpirationTimestamp: &resp.Status.Credential.ExpirationTimestamp, - ClientCertificateData: resp.Status.Credential.ClientCertificateData, - ClientKeyData: resp.Status.Credential.ClientKeyData, - Token: resp.Status.Credential.Token, - }, - }, nil -} - -// getClient returns an anonymous client for the Pinniped API at the provided endpoint/CA bundle. -func getClient(apiEndpoint string, caBundle string) (versioned.Interface, error) { - cfg, err := clientcmd.NewNonInteractiveClientConfig(clientcmdapi.Config{ - Clusters: map[string]*clientcmdapi.Cluster{ - "cluster": { - Server: apiEndpoint, - CertificateAuthorityData: []byte(caBundle), - }, - }, - Contexts: map[string]*clientcmdapi.Context{ - "current": { - Cluster: "cluster", - AuthInfo: "client", - }, - }, - AuthInfos: map[string]*clientcmdapi.AuthInfo{ - "client": {}, - }, - }, "current", &clientcmd.ConfigOverrides{}, nil).ClientConfig() - if err != nil { - return nil, err - } - client, err := kubeclient.New(kubeclient.WithConfig(cfg)) - if err != nil { - return nil, err - } - return client.PinnipedConcierge, nil -} diff --git a/internal/client/client_test.go b/internal/client/client_test.go deleted file mode 100644 index e91849ef..00000000 --- a/internal/client/client_test.go +++ /dev/null @@ -1,146 +0,0 @@ -// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package client - -import ( - "context" - "encoding/json" - "io/ioutil" - "net/http" - "testing" - "time" - - "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" - - auth1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/authentication/v1alpha1" - loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" - "go.pinniped.dev/internal/testutil" -) - -func TestExchangeToken(t *testing.T) { - t.Parallel() - ctx := context.Background() - - testAuthenticator := corev1.TypedLocalObjectReference{ - APIGroup: &auth1alpha1.SchemeGroupVersion.Group, - Kind: "WebhookAuthenticator", - Name: "test-webhook", - } - - t.Run("invalid configuration", func(t *testing.T) { - t.Parallel() - got, err := ExchangeToken(ctx, "test-namespace", testAuthenticator, "", "", "") - require.EqualError(t, err, "could not get API client: invalid configuration: no configuration has been provided, try setting KUBERNETES_MASTER environment variable") - require.Nil(t, got) - }) - - t.Run("server error", func(t *testing.T) { - t.Parallel() - // Start a test server that returns only 500 errors. - caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte("some server error")) - }) - - got, err := ExchangeToken(ctx, "test-namespace", testAuthenticator, "", caBundle, endpoint) - require.EqualError(t, err, `could not login: an error on the server ("some server error") has prevented the request from succeeding (post tokencredentialrequests.login.concierge.pinniped.dev)`) - require.Nil(t, got) - }) - - t.Run("login failure", func(t *testing.T) { - t.Parallel() - // Start a test server that returns success but with an error message - errorMessage := "some login failure" - caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("content-type", "application/json") - _ = json.NewEncoder(w).Encode(&loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{APIVersion: "login.concierge.pinniped.dev/v1alpha1", Kind: "TokenCredentialRequest"}, - Status: loginv1alpha1.TokenCredentialRequestStatus{Message: &errorMessage}, - }) - }) - - got, err := ExchangeToken(ctx, "test-namespace", testAuthenticator, "", caBundle, endpoint) - require.EqualError(t, err, `login failed: some login failure`) - require.Nil(t, got) - }) - - t.Run("login failure unknown error", func(t *testing.T) { - t.Parallel() - // Start a test server that returns without any error message but also without valid credentials - caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("content-type", "application/json") - _ = json.NewEncoder(w).Encode(&loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{APIVersion: "login.concierge.pinniped.dev/v1alpha1", Kind: "TokenCredentialRequest"}, - }) - }) - - got, err := ExchangeToken(ctx, "test-namespace", testAuthenticator, "", caBundle, endpoint) - require.EqualError(t, err, `login failed: unknown`) - require.Nil(t, got) - }) - - t.Run("success", func(t *testing.T) { - t.Parallel() - expires := metav1.NewTime(time.Now().Truncate(time.Second)) - - // Start a test server that returns successfully and asserts various properties of the request. - caBundle, endpoint := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, http.MethodPost, r.Method) - require.Equal(t, "/apis/login.concierge.pinniped.dev/v1alpha1/namespaces/test-namespace/tokencredentialrequests", r.URL.Path) - require.Equal(t, "application/json", r.Header.Get("content-type")) - - body, err := ioutil.ReadAll(r.Body) - require.NoError(t, err) - require.JSONEq(t, - `{ - "kind": "TokenCredentialRequest", - "apiVersion": "login.concierge.pinniped.dev/v1alpha1", - "metadata": { - "creationTimestamp": null, - "namespace": "test-namespace" - }, - "spec": { - "token": "test-token", - "authenticator": { - "apiGroup": "authentication.concierge.pinniped.dev", - "kind": "WebhookAuthenticator", - "name": "test-webhook" - } - }, - "status": {} - }`, - string(body), - ) - - w.Header().Set("content-type", "application/json") - _ = json.NewEncoder(w).Encode(&loginv1alpha1.TokenCredentialRequest{ - TypeMeta: metav1.TypeMeta{APIVersion: "login.concierge.pinniped.dev/v1alpha1", Kind: "TokenCredentialRequest"}, - Status: loginv1alpha1.TokenCredentialRequestStatus{ - Credential: &loginv1alpha1.ClusterCredential{ - ExpirationTimestamp: expires, - ClientCertificateData: "test-certificate", - ClientKeyData: "test-key", - }, - }, - }) - }) - - got, err := ExchangeToken(ctx, "test-namespace", testAuthenticator, "test-token", caBundle, endpoint) - require.NoError(t, err) - require.Equal(t, &clientauthenticationv1beta1.ExecCredential{ - TypeMeta: metav1.TypeMeta{ - Kind: "ExecCredential", - APIVersion: "client.authentication.k8s.io/v1beta1", - }, - Status: &clientauthenticationv1beta1.ExecCredentialStatus{ - ClientCertificateData: "test-certificate", - ClientKeyData: "test-key", - ExpirationTimestamp: &expires, - }, - }, got) - }) -} diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index f4cb86fe..af794115 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -10,43 +10,14 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/runtime/serializer" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" "k8s.io/client-go/pkg/version" - loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" - loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/credentialrequest" ) -var ( - //nolint: gochecknoglobals - scheme = runtime.NewScheme() - //nolint: gochecknoglobals, golint - Codecs = serializer.NewCodecFactory(scheme) -) - -//nolint: gochecknoinits -func init() { - utilruntime.Must(loginv1alpha1.AddToScheme(scheme)) - utilruntime.Must(loginapi.AddToScheme(scheme)) - - // add the options to empty v1 - metav1.AddToGroupVersion(scheme, schema.GroupVersion{Version: "v1"}) - - unversioned := schema.GroupVersion{Group: "", Version: "v1"} - scheme.AddUnversionedTypes(unversioned, - &metav1.Status{}, - &metav1.APIVersions{}, - &metav1.APIGroupList{}, - &metav1.APIGroup{}, - &metav1.APIResourceList{}, - ) -} - type Config struct { GenericConfig *genericapiserver.RecommendedConfig ExtraConfig ExtraConfig @@ -56,6 +27,9 @@ type ExtraConfig struct { Authenticator credentialrequest.TokenCredentialRequestAuthenticator Issuer credentialrequest.CertIssuer StartControllersPostStartHook func(ctx context.Context) + Scheme *runtime.Scheme + NegotiatedSerializer runtime.NegotiatedSerializer + GroupVersion schema.GroupVersion } type PinnipedServer struct { @@ -96,15 +70,15 @@ func (c completedConfig) New() (*PinnipedServer, error) { GenericAPIServer: genericServer, } - gvr := loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests") + gvr := c.ExtraConfig.GroupVersion.WithResource("tokencredentialrequests") storage := credentialrequest.NewREST(c.ExtraConfig.Authenticator, c.ExtraConfig.Issuer) if err := s.GenericAPIServer.InstallAPIGroup(&genericapiserver.APIGroupInfo{ PrioritizedVersions: []schema.GroupVersion{gvr.GroupVersion()}, VersionedResourcesStorageMap: map[string]map[string]rest.Storage{gvr.Version: {gvr.Resource: storage}}, OptionsExternalVersion: &schema.GroupVersion{Version: "v1"}, - Scheme: scheme, + Scheme: c.ExtraConfig.Scheme, ParameterCodec: metav1.ParameterCodec, - NegotiatedSerializer: Codecs, + NegotiatedSerializer: c.ExtraConfig.NegotiatedSerializer, }); err != nil { return nil, fmt.Errorf("could not install API group %s: %w", gvr.String(), err) } diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index b0529364..9c1a0a40 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -11,11 +11,16 @@ import ( "time" "github.com/spf13/cobra" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" genericapiserver "k8s.io/apiserver/pkg/server" genericoptions "k8s.io/apiserver/pkg/server/options" + loginapi "go.pinniped.dev/generated/1.20/apis/concierge/login" loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" - "go.pinniped.dev/internal/apigroup" "go.pinniped.dev/internal/certauthority/dynamiccertauthority" "go.pinniped.dev/internal/concierge/apiserver" "go.pinniped.dev/internal/config/concierge" @@ -23,6 +28,7 @@ import ( "go.pinniped.dev/internal/controllermanager" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/dynamiccert" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/registry/credentialrequest" @@ -168,24 +174,80 @@ func getAggregatedAPIServerConfig( startControllersPostStartHook func(context.Context), apiGroupSuffix string, ) (*apiserver.Config, error) { - // This is ignored for now because we turn off etcd storage below, but this is - // the right prefix in case we turn it back on. - apiGroup, ok := apigroup.Make(loginv1alpha1.GroupName, apiGroupSuffix) + apiGroup, ok := groupsuffix.Replace(loginv1alpha1.GroupName, apiGroupSuffix) if !ok { return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, apiGroupSuffix) } + + // standard set up of the server side scheme + scheme := runtime.NewScheme() + codecs := serializer.NewCodecFactory(scheme) + + utilruntime.Must(loginv1alpha1.AddToScheme(scheme)) + utilruntime.Must(loginapi.AddToScheme(scheme)) + + // add the options to empty v1 + metav1.AddToGroupVersion(scheme, schema.GroupVersion{Version: "v1"}) + + unversioned := schema.GroupVersion{Group: "", Version: "v1"} + scheme.AddUnversionedTypes(unversioned, + &metav1.Status{}, + &metav1.APIVersions{}, + &metav1.APIGroupList{}, + &metav1.APIGroup{}, + &metav1.APIResourceList{}, + ) + + // use closure to avoid mutating scheme during iteration + var addPinnipedTypeToAPIGroup []func() //nolint: prealloc // expected slice size is unknown + for gvk := range scheme.AllKnownTypes() { + gvk := gvk + + if apiGroup == loginv1alpha1.GroupName { + break // bail out early if using the standard group + } + + if gvk.Group != loginv1alpha1.GroupName { + continue // ignore types that are not in the aggregated API group + } + + // re-register the existing type but with the new group + f := func() { + obj, err := scheme.New(gvk) + if err != nil { + panic(err) // programmer error, scheme internal code is broken + } + newGVK := schema.GroupVersionKind{ + Group: apiGroup, + Version: gvk.Version, + Kind: gvk.Kind, + } + scheme.AddKnownTypeWithName(newGVK, obj) + } + + addPinnipedTypeToAPIGroup = append(addPinnipedTypeToAPIGroup, f) + } + + // run the closures to mutate the scheme to understand the types at the new group + for _, f := range addPinnipedTypeToAPIGroup { + f() + } + defaultEtcdPathPrefix := fmt.Sprintf("/registry/%s", apiGroup) + groupVersion := schema.GroupVersion{ + Group: apiGroup, + Version: loginv1alpha1.SchemeGroupVersion.Version, + } recommendedOptions := genericoptions.NewRecommendedOptions( defaultEtcdPathPrefix, - apiserver.Codecs.LegacyCodec(loginv1alpha1.SchemeGroupVersion), - // TODO we should check to see if all the other default settings are acceptable for us + codecs.LegacyCodec(groupVersion), ) recommendedOptions.Etcd = nil // turn off etcd storage because we don't need it yet recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider recommendedOptions.SecureServing.BindPort = 8443 // Don't run on default 443 because that requires root - serverConfig := genericapiserver.NewRecommendedConfig(apiserver.Codecs) + serverConfig := genericapiserver.NewRecommendedConfig(codecs) // Note that among other things, this ApplyTo() function copies // `recommendedOptions.SecureServing.ServerCert.GeneratedCert` into // `serverConfig.SecureServing.Cert` thus making `dynamicCertProvider` @@ -205,6 +267,9 @@ func getAggregatedAPIServerConfig( Authenticator: authenticator, Issuer: issuer, StartControllersPostStartHook: startControllersPostStartHook, + Scheme: scheme, + NegotiatedSerializer: codecs, + GroupVersion: groupVersion, }, } return apiServerConfig, nil diff --git a/internal/config/concierge/config.go b/internal/config/concierge/config.go index ead188de..1cfa11b6 100644 --- a/internal/config/concierge/config.go +++ b/internal/config/concierge/config.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/yaml" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/plog" ) @@ -47,6 +48,10 @@ func FromPath(path string) (*Config, error) { return nil, fmt.Errorf("validate api: %w", err) } + if err := validateAPIGroupSuffix(*config.APIGroupSuffix); err != nil { + return nil, fmt.Errorf("validate apiGroupSuffix: %w", err) + } + if err := validateNames(&config.NamesConfig); err != nil { return nil, fmt.Errorf("validate names: %w", err) } @@ -121,6 +126,10 @@ func validateAPI(apiConfig *APIConfigSpec) error { return nil } +func validateAPIGroupSuffix(apiGroupSuffix string) error { + return groupsuffix.Validate(apiGroupSuffix) +} + func int64Ptr(i int64) *int64 { return &i } diff --git a/internal/config/concierge/config_test.go b/internal/config/concierge/config_test.go index bbba174d..31b7a356 100644 --- a/internal/config/concierge/config_test.go +++ b/internal/config/concierge/config_test.go @@ -175,7 +175,7 @@ func TestFromPath(t *testing.T) { api: servingCertificate: durationSeconds: 2400 - renewBeforeSeconds: -10 + renewBeforeSeconds: 0 names: servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate credentialIssuer: pinniped-config @@ -183,6 +183,22 @@ func TestFromPath(t *testing.T) { `), wantError: "validate api: renewBefore must be positive", }, + { + name: "InvalidAPIGroupSuffix", + yaml: here.Doc(` + --- + api: + servingCertificate: + durationSeconds: 3600 + renewBeforeSeconds: 2400 + apiGroupSuffix: .starts.with.dot + names: + servingCertificateSecret: pinniped-concierge-api-tls-serving-certificate + credentialIssuer: pinniped-config + apiService: pinniped-api + `), + wantError: "validate apiGroupSuffix: 1 error(s):\n- 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]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + }, } for _, test := range tests { test := test diff --git a/internal/config/supervisor/config.go b/internal/config/supervisor/config.go index 6e20b21f..f6dabb04 100644 --- a/internal/config/supervisor/config.go +++ b/internal/config/supervisor/config.go @@ -13,6 +13,7 @@ import ( "sigs.k8s.io/yaml" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/plog" ) @@ -36,6 +37,10 @@ func FromPath(path string) (*Config, error) { maybeSetAPIGroupSuffixDefault(&config.APIGroupSuffix) + if err := validateAPIGroupSuffix(*config.APIGroupSuffix); err != nil { + return nil, fmt.Errorf("validate apiGroupSuffix: %w", err) + } + if err := validateNames(&config.NamesConfig); err != nil { return nil, fmt.Errorf("validate names: %w", err) } @@ -53,6 +58,10 @@ func maybeSetAPIGroupSuffixDefault(apiGroupSuffix **string) { } } +func validateAPIGroupSuffix(apiGroupSuffix string) error { + return groupsuffix.Validate(apiGroupSuffix) +} + func validateNames(names *NamesConfigSpec) error { missingNames := []string{} if names.DefaultTLSCertificateSecret == "" { diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index a2e5d46f..7576822c 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -64,6 +64,16 @@ func TestFromPath(t *testing.T) { `), wantError: "validate names: missing required names: defaultTLSCertificateSecret", }, + { + name: "apiGroupSuffix is prefixed with '.'", + yaml: here.Doc(` + --- + apiGroupSuffix: .starts.with.dot + names: + defaultTLSCertificateSecret: my-secret-name + `), + wantError: "validate apiGroupSuffix: 1 error(s):\n- 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]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + }, } for _, test := range tests { test := test diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets.go b/internal/controller/supervisorconfig/generator/supervisor_secrets.go index 2a736009..4075d0fc 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets.go @@ -28,7 +28,6 @@ import ( var generateKey = generateSymmetricKey type supervisorSecretsController struct { - owner *appsv1.Deployment labels map[string]string kubeClient kubernetes.Interface secretInformer corev1informers.SecretInformer @@ -46,7 +45,6 @@ func NewSupervisorSecretsController( initialEventFunc pinnipedcontroller.WithInitialEventOptionFunc, ) controllerlib.Controller { c := supervisorSecretsController{ - owner: owner, labels: labels, kubeClient: kubeClient, secretInformer: secretInformer, @@ -64,13 +62,7 @@ func NewSupervisorSecretsController( if secret.Type != SupervisorCSRFSigningKeySecretType { return false } - ownerReferences := secret.GetOwnerReferences() - for i := range secret.GetOwnerReferences() { - if ownerReferences[i].UID == owner.GetUID() { - return true - } - } - return false + return true }, nil), controllerlib.InformerOption{}, ), @@ -96,7 +88,7 @@ func (c *supervisorSecretsController) Sync(ctx controllerlib.Context) error { return nil } - newSecret, err := generateSecret(ctx.Key.Namespace, ctx.Key.Name, c.labels, secretDataFunc, c.owner) + newSecret, err := generateSecret(ctx.Key.Namespace, ctx.Key.Name, c.labels, secretDataFunc) if err != nil { return fmt.Errorf("failed to generate secret: %w", err) } @@ -193,27 +185,17 @@ func secretDataFunc() (map[string][]byte, error) { }, nil } -func generateSecret(namespace, name string, labels map[string]string, secretDataFunc func() (map[string][]byte, error), owner metav1.Object) (*corev1.Secret, error) { +func generateSecret(namespace, name string, labels map[string]string, secretDataFunc func() (map[string][]byte, error)) (*corev1.Secret, error) { secretData, err := secretDataFunc() if err != nil { return nil, err } - deploymentGVK := appsv1.SchemeGroupVersion.WithKind("Deployment") - return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: deploymentGVK.GroupVersion().String(), - Kind: deploymentGVK.Kind, - Name: owner.GetName(), - UID: owner.GetUID(), - }, - }, - Labels: labels, + Labels: labels, }, Type: SupervisorCSRFSigningKeySecretType, Data: secretData, diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go index e027a585..eeeb9930 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go @@ -34,12 +34,6 @@ var ( }, } - ownerGVK = schema.GroupVersionKind{ - Group: appsv1.SchemeGroupVersion.Group, - Version: appsv1.SchemeGroupVersion.Version, - Kind: "Deployment", - } - labels = map[string]string{ "some-label-key-1": "some-label-value-1", "some-label-key-2": "some-label-value-2", @@ -57,89 +51,13 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) { wantDelete bool }{ { - name: "owner reference is missing", + name: "owner reference is missing but Secret type is correct", secret: &corev1.Secret{ Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", ObjectMeta: metav1.ObjectMeta{ Namespace: "some-namespace", }, }, - }, - { - name: "owner reference with incorrect `APIVersion`", - secret: &corev1.Secret{ - Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", - ObjectMeta: metav1.ObjectMeta{ - Namespace: "some-namespace", - OwnerReferences: []metav1.OwnerReference{ - { - Name: owner.GetName(), - Kind: ownerGVK.Kind, - UID: owner.GetUID(), - }, - }, - }, - }, - wantAdd: true, - wantUpdate: true, - wantDelete: true, - }, - { - name: "owner reference with incorrect `Kind`", - secret: &corev1.Secret{ - Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", - ObjectMeta: metav1.ObjectMeta{ - Namespace: "some-namespace", - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: ownerGVK.String(), - Name: owner.GetName(), - Kind: "IncorrectKind", - UID: owner.GetUID(), - }, - }, - }, - }, - wantAdd: true, - wantUpdate: true, - wantDelete: true, - }, - { - name: "expected owner reference with incorrect `UID`", - secret: &corev1.Secret{ - Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", - ObjectMeta: metav1.ObjectMeta{ - Namespace: "some-namespace", - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: ownerGVK.String(), - Name: owner.GetName(), - Kind: ownerGVK.Kind, - UID: "DOES_NOT_MATCH", - }, - }, - }, - }, - }, - { - name: "multiple owner references (expected owner reference, and one more)", - secret: &corev1.Secret{ - Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", - ObjectMeta: metav1.ObjectMeta{ - Namespace: "some-namespace", - OwnerReferences: []metav1.OwnerReference{ - { - Kind: "UnrelatedKind", - }, - { - APIVersion: ownerGVK.String(), - Name: owner.GetName(), - Kind: ownerGVK.Kind, - UID: owner.GetUID(), - }, - }, - }, - }, wantAdd: true, wantUpdate: true, wantDelete: true, @@ -152,10 +70,8 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) { Namespace: "some-namespace", OwnerReferences: []metav1.OwnerReference{ { - APIVersion: ownerGVK.String(), - Name: owner.GetName(), - Kind: ownerGVK.Kind, - UID: owner.GetUID(), + Name: owner.GetName(), + UID: owner.GetUID(), }, }, }, @@ -166,32 +82,15 @@ func TestSupervisorSecretsControllerFilterSecret(t *testing.T) { secret: &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "some-namespace"}}, }, { - name: "owner reference with `Controller`: true", - secret: &corev1.Secret{ - Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", - ObjectMeta: metav1.ObjectMeta{ - Namespace: "some-namespace", - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(owner, ownerGVK), - }, - }, - }, - wantAdd: true, - wantUpdate: true, - wantDelete: true, - }, - { - name: "expected owner reference - where `Controller`: false", + name: "realistic owner reference and correct Secret type", secret: &corev1.Secret{ Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", ObjectMeta: metav1.ObjectMeta{ Namespace: "some-namespace", OwnerReferences: []metav1.OwnerReference{ { - APIVersion: ownerGVK.String(), - Name: owner.GetName(), - Kind: ownerGVK.Kind, - UID: owner.GetUID(), + Name: owner.GetName(), + UID: owner.GetUID(), }, }, }, @@ -272,15 +171,7 @@ func TestSupervisorSecretsControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: generatedSecretName, Namespace: generatedSecretNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: ownerGVK.GroupVersion().String(), - Kind: ownerGVK.Kind, - Name: owner.GetName(), - UID: owner.GetUID(), - }, - }, - Labels: labels, + Labels: labels, }, Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", Data: map[string][]byte{ @@ -292,15 +183,7 @@ func TestSupervisorSecretsControllerSync(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Name: generatedSecretName, Namespace: generatedSecretNamespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: ownerGVK.GroupVersion().String(), - Kind: ownerGVK.Kind, - Name: owner.GetName(), - UID: owner.GetUID(), - }, - }, - Labels: labels, + Labels: labels, }, Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", Data: map[string][]byte{ diff --git a/internal/controllerlib/test/integration/examplecontroller_test.go b/internal/controllerlib/test/integration/examplecontroller_test.go index 6d932eec..610c9bde 100644 --- a/internal/controllerlib/test/integration/examplecontroller_test.go +++ b/internal/controllerlib/test/integration/examplecontroller_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -12,6 +12,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/kubernetes" "go.pinniped.dev/internal/controllerlib/test/integration/examplecontroller/api" examplestart "go.pinniped.dev/internal/controllerlib/test/integration/examplecontroller/starter" @@ -31,7 +32,8 @@ func TestExampleController(t *testing.T) { err := examplestart.StartExampleController(ctx, config, secretData) require.NoError(t, err) - client := library.NewClientset(t) + client, err := kubernetes.NewForConfig(config) + require.NoError(t, err, "unexpected failure from kubernetes.NewForConfig()") namespaces := client.CoreV1().Namespaces() diff --git a/internal/controllermanager/prepare_controllers.go b/internal/controllermanager/prepare_controllers.go index a7f87d6a..cbc3f069 100644 --- a/internal/controllermanager/prepare_controllers.go +++ b/internal/controllermanager/prepare_controllers.go @@ -18,7 +18,6 @@ import ( loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned" pinnipedinformers "go.pinniped.dev/generated/1.20/client/concierge/informers/externalversions" - "go.pinniped.dev/internal/apigroup" "go.pinniped.dev/internal/config/concierge" "go.pinniped.dev/internal/controller/apicerts" "go.pinniped.dev/internal/controller/authenticator/authncache" @@ -31,6 +30,7 @@ import ( "go.pinniped.dev/internal/deploymentref" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/dynamiccert" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" ) @@ -89,8 +89,10 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { return nil, fmt.Errorf("cannot create deployment ref: %w", err) } - _ = c.APIGroupSuffix // TODO: wire API group into kubeclient. - client, err := kubeclient.New(dref) + client, err := kubeclient.New( + dref, + kubeclient.WithMiddleware(groupsuffix.New(c.APIGroupSuffix)), + ) if err != nil { return nil, fmt.Errorf("could not create clients for the controllers: %w", err) } @@ -111,7 +113,7 @@ func PrepareControllers(c *Config) (func(ctx context.Context), error) { Name: c.NamesConfig.CredentialIssuer, } - groupName, ok := apigroup.Make(loginv1alpha1.GroupName, c.APIGroupSuffix) + groupName, ok := groupsuffix.Replace(loginv1alpha1.GroupName, c.APIGroupSuffix) if !ok { return nil, fmt.Errorf("cannot make api group from %s/%s", loginv1alpha1.GroupName, c.APIGroupSuffix) } diff --git a/internal/deploymentref/deploymentref.go b/internal/deploymentref/deploymentref.go index cee32f25..b5fa2c9f 100644 --- a/internal/deploymentref/deploymentref.go +++ b/internal/deploymentref/deploymentref.go @@ -17,24 +17,35 @@ import ( "go.pinniped.dev/internal/ownerref" ) +// getTempClient is stubbed out for testing. +// +// We would normally inject a kubernetes.Interface into New(), but the client we want to create in +// the calling code depends on the return value of New() (i.e., on the kubeclient.Option for the +// OwnerReference). +//nolint: gochecknoglobals +var getTempClient = func() (kubernetes.Interface, error) { + client, err := kubeclient.New() + if err != nil { + return nil, err + } + return client.Kubernetes, nil +} + func New(podInfo *downward.PodInfo) (kubeclient.Option, *appsv1.Deployment, error) { - tempClient, err := kubeclient.New() + tempClient, err := getTempClient() if err != nil { return nil, nil, fmt.Errorf("cannot create temp client: %w", err) } - deployment, err := getDeployment(tempClient.Kubernetes, podInfo) + deployment, err := getDeployment(tempClient, podInfo) if err != nil { return nil, nil, fmt.Errorf("cannot get deployment: %w", err) } - ref := metav1.OwnerReference{ - Name: deployment.Name, - UID: deployment.UID, - } - ref.APIVersion, ref.Kind = appsv1.SchemeGroupVersion.WithKind("Deployment").ToAPIVersionAndKind() + // work around stupid behavior of WithoutVersionDecoder.Decode + deployment.APIVersion, deployment.Kind = appsv1.SchemeGroupVersion.WithKind("Deployment").ToAPIVersionAndKind() - return kubeclient.WithMiddleware(ownerref.New(ref)), deployment, nil + return kubeclient.WithMiddleware(ownerref.New(deployment)), deployment, nil } func getDeployment(kubeClient kubernetes.Interface, podInfo *downward.PodInfo) (*appsv1.Deployment, error) { diff --git a/internal/deploymentref/deploymentref_test.go b/internal/deploymentref/deploymentref_test.go new file mode 100644 index 00000000..8d6e5a80 --- /dev/null +++ b/internal/deploymentref/deploymentref_test.go @@ -0,0 +1,127 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package deploymentref + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes" + kubefake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + "go.pinniped.dev/internal/downward" +) + +func TestNew(t *testing.T) { + troo := true + goodDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + Name: "some-name", + }, + } + tests := []struct { + name string + apiObjects []runtime.Object + client func(*kubefake.Clientset) + createClientErr error + podInfo *downward.PodInfo + wantDeployment *appsv1.Deployment + wantError string + }{ + { + name: "happy", + apiObjects: []runtime.Object{ + goodDeployment, + &appsv1.ReplicaSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + Name: "some-name-rsname", + OwnerReferences: []metav1.OwnerReference{ + { + Controller: &troo, + Name: "some-name", + }, + }, + }, + }, + &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + Name: "some-name-rsname-podhash", + OwnerReferences: []metav1.OwnerReference{ + { + Controller: &troo, + Name: "some-name-rsname", + }, + }, + }, + }, + }, + podInfo: &downward.PodInfo{ + Namespace: "some-namespace", + Name: "some-name-rsname-podhash", + }, + wantDeployment: goodDeployment, + }, + { + name: "failed to create client", + createClientErr: errors.New("some create error"), + podInfo: &downward.PodInfo{ + Namespace: "some-namespace", + Name: "some-name-rsname-podhash", + }, + wantError: "cannot create temp client: some create error", + }, + { + name: "failed to talk to api", + client: func(c *kubefake.Clientset) { + c.PrependReactor( + "get", + "pods", + func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("get failed") + }, + ) + }, + podInfo: &downward.PodInfo{ + Namespace: "some-namespace", + Name: "some-name-rsname-podhash", + }, + wantError: "cannot get deployment: could not get pod: get failed", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + client := kubefake.NewSimpleClientset(test.apiObjects...) + if test.client != nil { + test.client(client) + } + + getTempClient = func() (kubernetes.Interface, error) { + return client, test.createClientErr + } + + _, d, err := New(test.podInfo) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + return + } + + require.NoError(t, err) + require.Equal(t, test.wantDeployment, d) + }) + } +} diff --git a/internal/groupsuffix/groupsuffix.go b/internal/groupsuffix/groupsuffix.go new file mode 100644 index 00000000..24f6f96a --- /dev/null +++ b/internal/groupsuffix/groupsuffix.go @@ -0,0 +1,142 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package groupsuffix + +import ( + "context" + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/validation" + + "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/multierror" +) + +const ( + pinnipedDefaultSuffix = "pinniped.dev" + pinnipedDefaultSuffixWithDot = ".pinniped.dev" +) + +func New(apiGroupSuffix string) kubeclient.Middleware { + // return a no-op middleware by default + if len(apiGroupSuffix) == 0 || apiGroupSuffix == pinnipedDefaultSuffix { + return nil + } + + return kubeclient.Middlewares{ + kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { + group := rt.Resource().Group + newGroup, ok := Replace(group, apiGroupSuffix) + if !ok { + return // ignore APIs that do not have our group + } + + rt.MutateRequest(func(obj kubeclient.Object) { + typeMeta := obj.GetObjectKind() + origGVK := typeMeta.GroupVersionKind() + newGVK := schema.GroupVersionKind{ + Group: newGroup, + Version: origGVK.Version, + Kind: origGVK.Kind, + } + typeMeta.SetGroupVersionKind(newGVK) + }) + }), + + kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { + // we should not mess with owner refs on things we did not create + if rt.Verb() != kubeclient.VerbCreate { + return + } + + // we probably do not want mess with an owner ref on a subresource + if len(rt.Subresource()) != 0 { + return + } + + rt.MutateRequest(mutateOwnerRefs(Replace, apiGroupSuffix)) + }), + + kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { + // always unreplace owner refs with apiGroupSuffix because we can consume those objects across all verbs + rt.MutateResponse(mutateOwnerRefs(unreplace, apiGroupSuffix)) + }), + } +} + +func mutateOwnerRefs(replaceFunc func(baseAPIGroup, apiGroupSuffix string) (string, bool), apiGroupSuffix string) func(kubeclient.Object) { + return func(obj kubeclient.Object) { + // fix up owner refs because they are consumed by external and internal actors + oldRefs := obj.GetOwnerReferences() + if len(oldRefs) == 0 { + return + } + + var changedGroup bool + + newRefs := make([]metav1.OwnerReference, 0, len(oldRefs)) + for _, ref := range oldRefs { + ref := *ref.DeepCopy() + + gv, _ := schema.ParseGroupVersion(ref.APIVersion) // error is safe to ignore, empty gv is fine + + if newGroup, ok := replaceFunc(gv.Group, apiGroupSuffix); ok { + changedGroup = true + gv.Group = newGroup + ref.APIVersion = gv.String() + } + + newRefs = append(newRefs, ref) + } + + if !changedGroup { + return + } + + obj.SetOwnerReferences(newRefs) + } +} + +// Replace constructs an API group from a baseAPIGroup and a parameterized apiGroupSuffix. +// +// We assume that all baseAPIGroup's will end in "pinniped.dev", and therefore we can safely replace +// the reference to "pinniped.dev" with the provided apiGroupSuffix. If the provided baseAPIGroup +// does not end in "pinniped.dev", then this function will return an empty string and false. +// +// See ExampleReplace_loginv1alpha1 and ExampleReplace_string for more information on input/output pairs. +func Replace(baseAPIGroup, apiGroupSuffix string) (string, bool) { + if !strings.HasSuffix(baseAPIGroup, pinnipedDefaultSuffixWithDot) { + return "", false + } + return strings.TrimSuffix(baseAPIGroup, pinnipedDefaultSuffix) + apiGroupSuffix, true +} + +func unreplace(baseAPIGroup, apiGroupSuffix string) (string, bool) { + if !strings.HasSuffix(baseAPIGroup, "."+apiGroupSuffix) { + return "", false + } + return strings.TrimSuffix(baseAPIGroup, apiGroupSuffix) + pinnipedDefaultSuffix, true +} + +// Validate validates the provided apiGroupSuffix is usable as an API group suffix. Specifically, it +// makes sure that the provided apiGroupSuffix is a valid DNS-1123 subdomain with at least one dot, +// to match Kubernetes behavior. +func Validate(apiGroupSuffix string) error { + err := multierror.New() + + if len(strings.Split(apiGroupSuffix, ".")) < 2 { + err.Add(constable.Error("must contain '.'")) + } + + errorStrings := validation.IsDNS1123Subdomain(apiGroupSuffix) + for _, errorString := range errorStrings { + errorString := errorString + err.Add(constable.Error(errorString)) + } + + return err.ErrOrNil() +} diff --git a/internal/groupsuffix/groupsuffix_test.go b/internal/groupsuffix/groupsuffix_test.go new file mode 100644 index 00000000..2b4a58a5 --- /dev/null +++ b/internal/groupsuffix/groupsuffix_test.go @@ -0,0 +1,508 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package groupsuffix + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + + loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + configv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/testutil" +) + +func ExampleReplace_loginv1alpha1() { + s, _ := Replace(loginv1alpha1.GroupName, "tuna.fish.io") + fmt.Println(s) + // Output: login.concierge.tuna.fish.io +} + +func ExampleReplace_string() { + s, _ := Replace("idp.supervisor.pinniped.dev", "marlin.io") + fmt.Println(s) + // Output: idp.supervisor.marlin.io +} +func TestMiddlware(t *testing.T) { + const newSuffix = "some.suffix.com" + + podWithoutOwner := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{}, + }, + } + + nonPinnipedOwner := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-name", + UID: "some-uid", + }, + } + nonPinnipedOwnerGVK := corev1.SchemeGroupVersion.WithKind("Pod") + podWithNonPinnipedOwner := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + + var ok bool + pinnipedOwner := &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-name", + UID: "some-uid", + }, + } + pinnipedOwnerGVK := configv1alpha1.SchemeGroupVersion.WithKind("FederationDomain") + pinnipedOwnerWithNewGroupGVK := configv1alpha1.SchemeGroupVersion.WithKind("FederationDomain") + pinnipedOwnerWithNewGroupGVK.Group, ok = Replace(pinnipedOwnerWithNewGroupGVK.Group, newSuffix) + require.True(t, ok) + podWithPinnipedOwner := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerGVK), + + // make sure we don't update the non-pinniped owner + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + podWithPinnipedOwnerWithNewGroup := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1.SchemeGroupVersion.String(), + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerWithNewGroupGVK), + + // make sure we don't update the non-pinniped owner + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + + federationDomainWithPinnipedOwner := &configv1alpha1.FederationDomain{ + TypeMeta: metav1.TypeMeta{ + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "FederationDomain", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerGVK), + + // make sure we don't update the non-pinniped owner + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + federationDomainWithPinnipedOwnerWithNewGroup := &configv1alpha1.FederationDomain{ + TypeMeta: metav1.TypeMeta{ + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "FederationDomain", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerWithNewGroupGVK), + + // make sure we don't update the non-pinniped owner + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup := &configv1alpha1.FederationDomain{ + TypeMeta: metav1.TypeMeta{ + APIVersion: replaceGV(t, configv1alpha1.SchemeGroupVersion, newSuffix), + Kind: "FederationDomain", + }, + ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(pinnipedOwner, pinnipedOwnerWithNewGroupGVK), + + // make sure we don't update the non-pinniped owner + *metav1.NewControllerRef(nonPinnipedOwner, nonPinnipedOwnerGVK), + }, + }, + } + + tokenCredentialRequest := &loginv1alpha1.TokenCredentialRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: loginv1alpha1.SchemeGroupVersion.String(), + Kind: "TokenCredentialRequest", + }, + } + tokenCredentialRequestWithNewGroup := &loginv1alpha1.TokenCredentialRequest{ + TypeMeta: metav1.TypeMeta{ + APIVersion: replaceGV(t, loginv1alpha1.SchemeGroupVersion, newSuffix), + Kind: "TokenCredentialRequest", + }, + } + + tests := []struct { + name string + apiGroupSuffix string + rt *testutil.RoundTrip + requestObj, responseObj kubeclient.Object + wantNilMiddleware bool + wantMutateRequests, wantMutateResponses int + wantRequestObj, wantResponseObj kubeclient.Object + + skip bool + }{ + { + name: "api group suffix is empty", + apiGroupSuffix: "", + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + wantNilMiddleware: true, + }, + { + name: "api group suffix is default", + apiGroupSuffix: "pinniped.dev", + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + wantNilMiddleware: true, + }, + { + name: "get resource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithoutOwner, + wantMutateResponses: 1, + wantResponseObj: podWithoutOwner, + }, + { + name: "get resource without pinniped.dev with non-pinniped.dev owner ref", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithNonPinnipedOwner, + wantMutateResponses: 1, + wantResponseObj: podWithNonPinnipedOwner, + }, + { + name: "get resource without pinniped.dev with pinniped.dev owner ref", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithPinnipedOwnerWithNewGroup, + wantMutateResponses: 1, + wantResponseObj: podWithPinnipedOwner, + }, + { + name: "get resource with pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbGet). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequest, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroup, + wantResponseObj: tokenCredentialRequest, + }, + { + name: "create resource without pinniped.dev and without owner ref", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + requestObj: podWithoutOwner, + responseObj: podWithoutOwner, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: podWithoutOwner, + wantResponseObj: podWithoutOwner, + }, + { + name: "create resource without pinniped.dev and with owner ref that has no pinniped.dev owner", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + requestObj: podWithNonPinnipedOwner, + responseObj: podWithNonPinnipedOwner, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: podWithNonPinnipedOwner, + wantResponseObj: podWithNonPinnipedOwner, + }, + { + name: "create resource without pinniped.dev and with owner ref that has pinniped.dev owner", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + requestObj: podWithPinnipedOwner, + responseObj: podWithPinnipedOwnerWithNewGroup, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: podWithPinnipedOwnerWithNewGroup, + wantResponseObj: podWithPinnipedOwner, + }, + { + name: "create subresource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")). + WithSubresource("some-subresource"), + responseObj: podWithPinnipedOwner, + wantMutateResponses: 1, + wantResponseObj: podWithPinnipedOwner, + }, + { + // test that both of our middleware request mutations play nicely with each other + name: "create resource with pinniped.dev and with owner ref that has pinniped.dev owner", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbCreate). + WithNamespace("some-namespace"). + WithResource(configv1alpha1.SchemeGroupVersion.WithResource("federationdomains")), + requestObj: federationDomainWithPinnipedOwner, + responseObj: federationDomainWithPinnipedOwnerWithNewGroup, + wantMutateRequests: 2, + wantMutateResponses: 1, + wantRequestObj: federationDomainWithNewGroupAndPinnipedOwnerWithNewGroup, + wantResponseObj: federationDomainWithPinnipedOwner, + }, + { + name: "update resource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbUpdate). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithoutOwner, + wantMutateResponses: 1, + wantResponseObj: podWithoutOwner, + }, + { + name: "update resource with pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbUpdate). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequest, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroup, + wantResponseObj: tokenCredentialRequest, + }, + { + name: "list resource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbList). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithoutOwner, + wantMutateResponses: 1, + wantResponseObj: podWithoutOwner, + }, + { + name: "list resource with pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbList). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequest, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroup, + wantResponseObj: tokenCredentialRequest, + }, + { + name: "watch resource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbWatch). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithoutOwner, + wantMutateResponses: 1, + wantResponseObj: podWithoutOwner, + }, + { + name: "watch resource with pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbList). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequest, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroup, + wantResponseObj: tokenCredentialRequest, + }, + { + name: "patch resource without pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbPatch). + WithNamespace("some-namespace"). + WithResource(corev1.SchemeGroupVersion.WithResource("pods")), + responseObj: podWithoutOwner, + wantMutateResponses: 1, + wantResponseObj: podWithoutOwner, + }, + { + name: "patch resource with pinniped.dev", + apiGroupSuffix: newSuffix, + rt: (&testutil.RoundTrip{}). + WithVerb(kubeclient.VerbList). + WithResource(loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests")), + requestObj: tokenCredentialRequest, + responseObj: tokenCredentialRequest, + wantMutateRequests: 1, + wantMutateResponses: 1, + wantRequestObj: tokenCredentialRequestWithNewGroup, + wantResponseObj: tokenCredentialRequest, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + if test.skip { + t.Skip() + } + + m := New(test.apiGroupSuffix) + if test.wantNilMiddleware { + require.Nil(t, m, "wanted nil middleware") + return + } + + m.Handle(context.Background(), test.rt) + require.Len(t, test.rt.MutateRequests, test.wantMutateRequests) + require.Len(t, test.rt.MutateResponses, test.wantMutateResponses) + + if test.wantMutateRequests != 0 { + require.NotNil(t, test.requestObj, "expected test.requestObj to be set") + objMutated := test.requestObj.DeepCopyObject().(kubeclient.Object) + for _, mutateRequest := range test.rt.MutateRequests { + mutateRequest := mutateRequest + mutateRequest(objMutated) + } + require.Equal(t, test.wantRequestObj, objMutated, "request obj did not match") + } + + if test.wantMutateResponses != 0 { + require.NotNil(t, test.responseObj, "expected test.responseObj to be set") + objMutated := test.responseObj.DeepCopyObject().(kubeclient.Object) + for _, mutateResponse := range test.rt.MutateResponses { + mutateResponse := mutateResponse + mutateResponse(objMutated) + } + require.Equal(t, test.wantResponseObj, objMutated, "response obj did not match") + } + }) + } +} + +func TestReplaceError(t *testing.T) { + _, ok := Replace("bad-suffix-that-doesnt-end-in-pinniped-dot-dev", "shouldnt-matter.com") + require.False(t, ok) + + _, ok = Replace("bad-suffix-that-end-in.prefixed-pinniped.dev", "shouldnt-matter.com") + require.False(t, ok) +} + +func TestReplaceSuffix(t *testing.T) { + s, ok := Replace("something.pinniped.dev.something-else.pinniped.dev", "tuna.io") + require.Equal(t, "something.pinniped.dev.something-else.tuna.io", s) + require.True(t, ok) +} + +func TestValidate(t *testing.T) { + tests := []struct { + apiGroupSuffix string + wantErrorPrefix string + }{ + { + apiGroupSuffix: "happy.suffix.com", + }, + { + apiGroupSuffix: "no-dots", + wantErrorPrefix: "1 error(s):\n- must contain '.'", + }, + { + apiGroupSuffix: ".starts.with.dot", + wantErrorPrefix: "1 error(s):\n- a lowercase RFC 1123 subdomain must consist", + }, + { + apiGroupSuffix: "ends.with.dot.", + wantErrorPrefix: "1 error(s):\n- a lowercase RFC 1123 subdomain must consist", + }, + { + apiGroupSuffix: ".multiple-issues.because-this-string-is-longer-than-the-253-character-limit-of-a-dns-1123-identifier-" + chars(253), + wantErrorPrefix: "2 error(s):\n- must be no more than 253 characters\n- a lowercase RFC 1123 subdomain must consist", + }, + } + for _, test := range tests { + test := test + t.Run(test.apiGroupSuffix, func(t *testing.T) { + err := Validate(test.apiGroupSuffix) + if test.wantErrorPrefix != "" { + require.Error(t, err) + require.Truef( + t, + strings.HasPrefix(err.Error(), test.wantErrorPrefix), + "%q does not start with %q", err.Error(), test.wantErrorPrefix) + } else { + require.NoError(t, err) + } + }) + } +} + +func replaceGV(t *testing.T, baseGV schema.GroupVersion, apiGroupSuffix string) string { + t.Helper() + groupName, ok := Replace(baseGV.Group, apiGroupSuffix) + require.True(t, ok, "expected to be able to replace %q's suffix with %q", baseGV.Group, apiGroupSuffix) + return schema.GroupVersion{Group: groupName, Version: baseGV.Version}.String() +} + +func chars(count int) string { + return strings.Repeat("a", count) +} diff --git a/internal/kubeclient/copied.go b/internal/kubeclient/copied.go new file mode 100644 index 00000000..a822deaa --- /dev/null +++ b/internal/kubeclient/copied.go @@ -0,0 +1,70 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "bytes" + "encoding/hex" + "fmt" + "net/url" + + "k8s.io/apimachinery/pkg/runtime/schema" + restclient "k8s.io/client-go/rest" + + "go.pinniped.dev/internal/plog" +) + +// defaultServerUrlFor was copied from k8s.io/client-go/rest/url_utils.go. +//nolint: golint +func defaultServerUrlFor(config *restclient.Config) (*url.URL, string, error) { + hasCA := len(config.CAFile) != 0 || len(config.CAData) != 0 + hasCert := len(config.CertFile) != 0 || len(config.CertData) != 0 + defaultTLS := hasCA || hasCert || config.Insecure + host := config.Host + if host == "" { + host = "localhost" + } + + if config.GroupVersion != nil { + return restclient.DefaultServerURL(host, config.APIPath, *config.GroupVersion, defaultTLS) + } + return restclient.DefaultServerURL(host, config.APIPath, schema.GroupVersion{}, defaultTLS) +} + +// truncateBody was copied from k8s.io/client-go/rest/request.go +// ...except i changed klog invocations to analogous plog invocations +// +// truncateBody decides if the body should be truncated, based on the glog Verbosity. +func truncateBody(body string) string { + max := 0 + switch { + case plog.Enabled(plog.LevelAll): + return body + case plog.Enabled(plog.LevelTrace): + max = 10240 + case plog.Enabled(plog.LevelDebug): + max = 1024 + } + + if len(body) <= max { + return body + } + + return body[:max] + fmt.Sprintf(" [truncated %d chars]", len(body)-max) +} + +// glogBody logs a body output that could be either JSON or protobuf. It explicitly guards against +// allocating a new string for the body output unless necessary. Uses a simple heuristic to determine +// whether the body is printable. +func glogBody(prefix string, body []byte) { + if plog.Enabled(plog.LevelDebug) { + if bytes.IndexFunc(body, func(r rune) bool { + return r < 0x0a + }) != -1 { + plog.Debug(prefix, "body", truncateBody(hex.Dump(body))) + } else { + plog.Debug(prefix, "body", truncateBody(string(body))) + } + } +} diff --git a/internal/kubeclient/gvk.go b/internal/kubeclient/gvk.go new file mode 100644 index 00000000..31d0c73e --- /dev/null +++ b/internal/kubeclient/gvk.go @@ -0,0 +1,79 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "encoding/json" + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func maybeRestoreGVK(serializer runtime.Serializer, respData []byte, result *mutationResult) ([]byte, error) { + if !result.gvkChanged { + return respData, nil + } + + // the body could be an API status, random trash or the actual object we want + unknown := &runtime.Unknown{} + _ = runtime.DecodeInto(serializer, respData, unknown) // we do not care about the error + + doesNotNeedGVKFix := len(unknown.Raw) == 0 || unknown.GroupVersionKind() != result.newGVK + + if doesNotNeedGVKFix { + return respData, nil + } + + return restoreGVK(serializer, unknown, result.origGVK) +} + +func restoreGVK(encoder runtime.Encoder, unknown *runtime.Unknown, gvk schema.GroupVersionKind) ([]byte, error) { + typeMeta := runtime.TypeMeta{} + typeMeta.APIVersion, typeMeta.Kind = gvk.ToAPIVersionAndKind() + + newUnknown := &runtime.Unknown{} + *newUnknown = *unknown + newUnknown.TypeMeta = typeMeta + + switch newUnknown.ContentType { + case runtime.ContentTypeJSON: + // json is messy if we want to avoid decoding the whole object + keysOnly := map[string]json.RawMessage{} + + // get the keys. this does not preserve order. + if err := json.Unmarshal(newUnknown.Raw, &keysOnly); err != nil { + return nil, fmt.Errorf("failed to unmarshall json keys: %w", err) + } + + // turn the type meta into JSON bytes + typeMetaBytes, err := json.Marshal(typeMeta) + if err != nil { + return nil, fmt.Errorf("failed to marshall type meta: %w", err) + } + + // overwrite the type meta keys with the new data + if err := json.Unmarshal(typeMetaBytes, &keysOnly); err != nil { + return nil, fmt.Errorf("failed to type meta keys: %w", err) + } + + // marshall everything back to bytes + newRaw, err := json.Marshal(keysOnly) + if err != nil { + return nil, fmt.Errorf("failed to marshall new raw: %w", err) + } + + // we could just return the bytes but it feels weird to not use the encoder + newUnknown.Raw = newRaw + + case runtime.ContentTypeProtobuf: + // protobuf is easy because of the unknown wrapper + // newUnknown.Raw already contains the correct data we need + + default: + return nil, fmt.Errorf("unknown content type: %s", newUnknown.ContentType) // this should never happen + } + + return runtime.Encode(encoder, newUnknown) +} diff --git a/internal/kubeclient/gvk_test.go b/internal/kubeclient/gvk_test.go new file mode 100644 index 00000000..cf0f711c --- /dev/null +++ b/internal/kubeclient/gvk_test.go @@ -0,0 +1,222 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "io" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func Test_maybeRestoreGVK(t *testing.T) { + type args struct { + unknown *runtime.Unknown + origGVK, newGVK schema.GroupVersionKind + } + tests := []struct { + name string + args args + want runtime.Object + wantChanged bool + wantErr string + }{ + { + name: "should update gvk via JSON", + args: args{ + unknown: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "new/v1", + Kind: "Tree", + }, + Raw: []byte(`{"apiVersion":"new/v1","kind":"Tree","spec":{"pandas":"love"}}`), + ContentType: runtime.ContentTypeJSON, + }, + origGVK: schema.GroupVersionKind{ + Group: "old", + Version: "v1", + Kind: "Tree", + }, + newGVK: schema.GroupVersionKind{ + Group: "new", + Version: "v1", + Kind: "Tree", + }, + }, + want: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "old/v1", + Kind: "Tree", + }, + Raw: []byte(`{"apiVersion":"old/v1","kind":"Tree","spec":{"pandas":"love"}}`), + ContentType: runtime.ContentTypeJSON, + }, + wantChanged: true, + wantErr: "", + }, + { + name: "should update gvk via protobuf", + args: args{ + unknown: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "new/v1", + Kind: "Tree", + }, + Raw: []byte(`assumed to be valid and does not change`), + ContentType: runtime.ContentTypeProtobuf, + }, + origGVK: schema.GroupVersionKind{ + Group: "original", + Version: "v1", + Kind: "Tree", + }, + newGVK: schema.GroupVersionKind{ + Group: "new", + Version: "v1", + Kind: "Tree", + }, + }, + want: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "original/v1", + Kind: "Tree", + }, + Raw: []byte(`assumed to be valid and does not change`), + ContentType: runtime.ContentTypeProtobuf, + }, + wantChanged: true, + wantErr: "", + }, + { + name: "should ignore because gvk is different", + args: args{ + unknown: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "new/v1", + Kind: "Tree", + }, + }, + newGVK: schema.GroupVersionKind{ + Group: "new", + Version: "v1", + Kind: "Forest", + }, + }, + want: nil, + wantChanged: false, + wantErr: "", + }, + { + name: "empty raw is ignored", + args: args{ + unknown: &runtime.Unknown{}, + }, + want: nil, + wantChanged: false, + wantErr: "", + }, + { + name: "invalid content type errors", + args: args{ + unknown: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "walrus.tld/v1", + Kind: "Seal", + }, + Raw: []byte(`data that should be ignored because we do not used YAML`), + ContentType: runtime.ContentTypeYAML, + }, + origGVK: schema.GroupVersionKind{ + Group: "pinniped.dev", + Version: "v1", + Kind: "Seal", + }, + newGVK: schema.GroupVersionKind{ + Group: "walrus.tld", + Version: "v1", + Kind: "Seal", + }, + }, + want: nil, + wantChanged: false, + wantErr: "unknown content type: application/yaml", + }, + { + name: "invalid JSON should error", + args: args{ + unknown: &runtime.Unknown{ + TypeMeta: runtime.TypeMeta{ + APIVersion: "ocean/v1", + Kind: "Water", + }, + Raw: []byte(`lol not JSON`), + ContentType: runtime.ContentTypeJSON, + }, + origGVK: schema.GroupVersionKind{ + Group: "dirt", + Version: "v1", + Kind: "Land", + }, + newGVK: schema.GroupVersionKind{ + Group: "ocean", + Version: "v1", + Kind: "Water", + }, + }, + want: nil, + wantChanged: false, + wantErr: "failed to unmarshall json keys: invalid character 'l' looking for beginning of value", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + serializer := &testSerializer{unknown: tt.args.unknown} + respData := []byte(`original`) + result := &mutationResult{origGVK: tt.args.origGVK, newGVK: tt.args.newGVK, gvkChanged: tt.args.origGVK != tt.args.newGVK} + + newRespData, err := maybeRestoreGVK(serializer, respData, result) + + if len(tt.wantErr) > 0 { + require.EqualError(t, err, tt.wantErr) + require.Nil(t, newRespData) + require.Nil(t, serializer.obj) + return + } + + require.NoError(t, err) + + if tt.wantChanged { + require.Equal(t, []byte(`changed`), newRespData) + } else { + require.Equal(t, []byte(`original`), newRespData) + } + + require.Equal(t, tt.want, serializer.obj) + }) + } +} + +type testSerializer struct { + unknown *runtime.Unknown + obj runtime.Object +} + +func (s *testSerializer) Encode(obj runtime.Object, w io.Writer) error { + s.obj = obj + _, err := w.Write([]byte(`changed`)) + return err +} + +func (s *testSerializer) Decode(_ []byte, _ *schema.GroupVersionKind, into runtime.Object) (runtime.Object, *schema.GroupVersionKind, error) { + u := into.(*runtime.Unknown) + *u = *s.unknown + return u, nil, nil +} + +func (s *testSerializer) Identifier() runtime.Identifier { + panic("not called") +} diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index 740e99e9..5a04e1f8 100644 --- a/internal/kubeclient/kubeclient.go +++ b/internal/kubeclient/kubeclient.go @@ -6,7 +6,6 @@ package kubeclient import ( "fmt" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes" kubescheme "k8s.io/client-go/kubernetes/scheme" @@ -29,12 +28,6 @@ type Client struct { JSONConfig, ProtoConfig *restclient.Config } -// TODO expand this interface to address more complex use cases. -type Middleware interface { - Handles(httpMethod string) bool - Mutate(obj metav1.Object) (mutated bool) -} - func New(opts ...Option) (*Client, error) { c := &clientConfig{} @@ -58,13 +51,13 @@ func New(opts ...Option) (*Client, error) { protoKubeConfig := createProtoKubeConfig(c.config) // Connect to the core Kubernetes API. - k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Codecs, c.middlewares)) + k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Scheme, kubescheme.Codecs, c.middlewares)) if err != nil { return nil, fmt.Errorf("could not initialize Kubernetes client: %w", err) } // Connect to the Kubernetes aggregation API. - aggregatorClient, err := aggregatorclient.NewForConfig(configWithWrapper(protoKubeConfig, aggregatorclientscheme.Codecs, c.middlewares)) + aggregatorClient, err := aggregatorclient.NewForConfig(configWithWrapper(protoKubeConfig, aggregatorclientscheme.Scheme, aggregatorclientscheme.Codecs, c.middlewares)) if err != nil { return nil, fmt.Errorf("could not initialize aggregation client: %w", err) } @@ -72,8 +65,7 @@ func New(opts ...Option) (*Client, error) { // Connect to the pinniped concierge API. // We cannot use protobuf encoding here because we are using CRDs // (for which protobuf encoding is not yet supported). - // TODO we should try to add protobuf support to TokenCredentialRequests since it is an aggregated API - pinnipedConciergeClient, err := pinnipedconciergeclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedconciergeclientsetscheme.Codecs, c.middlewares)) + pinnipedConciergeClient, err := pinnipedconciergeclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedconciergeclientsetscheme.Scheme, pinnipedconciergeclientsetscheme.Codecs, c.middlewares)) if err != nil { return nil, fmt.Errorf("could not initialize pinniped client: %w", err) } @@ -81,7 +73,7 @@ func New(opts ...Option) (*Client, error) { // Connect to the pinniped supervisor API. // We cannot use protobuf encoding here because we are using CRDs // (for which protobuf encoding is not yet supported). - pinnipedSupervisorClient, err := pinnipedsupervisorclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedsupervisorclientsetscheme.Codecs, c.middlewares)) + pinnipedSupervisorClient, err := pinnipedsupervisorclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedsupervisorclientsetscheme.Scheme, pinnipedsupervisorclientsetscheme.Codecs, c.middlewares)) if err != nil { return nil, fmt.Errorf("could not initialize pinniped client: %w", err) } diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go new file mode 100644 index 00000000..a5269910 --- /dev/null +++ b/internal/kubeclient/kubeclient_test.go @@ -0,0 +1,775 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "context" + "fmt" + "io" + "net" + "net/http" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/rest" + "k8s.io/client-go/transport" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + + loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + configv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/internal/testutil/fakekubeapi" +) + +const ( + someClusterName = "some cluster name" +) + +var ( + podGVK = corev1.SchemeGroupVersion.WithKind("Pod") + goodPod = &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-pod", + Namespace: "good-namespace", + }, + } + + apiServiceGVK = apiregistrationv1.SchemeGroupVersion.WithKind("APIService") + goodAPIService = &apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-api-service", + }, + } + + tokenCredentialRequestGVK = loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequest") + goodTokenCredentialRequest = &loginv1alpha1.TokenCredentialRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-token-credential-request", + Namespace: "good-namespace", + }, + } + + federationDomainGVK = configv1alpha1.SchemeGroupVersion.WithKind("FederationDomain") + goodFederationDomain = &configv1alpha1.FederationDomain{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-federation-domain", + Namespace: "good-namespace", + }, + } + + middlewareAnnotations = map[string]string{"some-annotation": "thing 1"} + middlewareLabels = map[string]string{"some-label": "thing 2"} +) + +// TestKubeclient tests a subset of kubeclient functionality (from the public interface down). We +// intend for the following list of things to be tested with the integration tests: +// list (running in every informer cache) +// watch (running in every informer cache) +// discovery +// api errors +func TestKubeclient(t *testing.T) { + // plog.ValidateAndSetLogLevelGlobally(plog.LevelDebug) // uncomment me to get some more debug logs + + tests := []struct { + name string + editRestConfig func(t *testing.T, restConfig *rest.Config) + middlewares func(t *testing.T) []*spyMiddleware + reallyRunTest func(t *testing.T, c *Client) + wantMiddlewareReqs, wantMiddlewareResps [][]Object + }{ + { + name: "crud core api", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newAnnotationMiddleware(t), newLabelMiddleware(t)} + }, + reallyRunTest: func(t *testing.T, c *Client) { + // create + pod, err := c.Kubernetes. + CoreV1(). + Pods(goodPod.Namespace). + Create(context.Background(), goodPod, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodPod, pod) + + // read + pod, err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Get(context.Background(), pod.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodPod, annotations(), labels()), pod) + + // read when not found + _, err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Get(context.Background(), "this-pod-does-not-exist", metav1.GetOptions{}) + require.EqualError(t, err, "the server could not find the requested resource (get pods this-pod-does-not-exist)") + + // update + goodPodWithAnnotationsAndLabelsAndClusterName := with(goodPod, annotations(), labels(), clusterName()).(*corev1.Pod) + pod, err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Update(context.Background(), goodPodWithAnnotationsAndLabelsAndClusterName, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, goodPodWithAnnotationsAndLabelsAndClusterName, pod) + + // delete + err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Delete(context.Background(), pod.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }, + wantMiddlewareReqs: [][]Object{ + { + with(goodPod, gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + with(goodPod, annotations(), labels(), clusterName(), gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + }, + { + with(goodPod, annotations(), gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + with(goodPod, annotations(), labels(), clusterName(), gvk(podGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(podGVK)), + }, + }, + wantMiddlewareResps: [][]Object{ + { + with(goodPod, annotations(), labels(), gvk(podGVK)), + with(goodPod, annotations(), labels(), gvk(podGVK)), + with(goodPod, annotations(), labels(), clusterName(), gvk(podGVK)), + }, + { + with(goodPod, emptyAnnotations(), labels(), gvk(podGVK)), + with(goodPod, annotations(), labels(), gvk(podGVK)), + with(goodPod, annotations(), labels(), clusterName(), gvk(podGVK)), + }, + }, + }, + { + name: "crud core api without middlewares", + reallyRunTest: func(t *testing.T, c *Client) { + // create + pod, err := c.Kubernetes. + CoreV1(). + Pods(goodPod.Namespace). + Create(context.Background(), goodPod, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodPod, pod) + + // read + pod, err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Get(context.Background(), pod.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodPod), pod) + + // update + pod, err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Update(context.Background(), goodPod, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, goodPod, pod) + + // delete + err = c.Kubernetes. + CoreV1(). + Pods(pod.Namespace). + Delete(context.Background(), pod.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }, + }, + { + name: "crud aggregation api", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newAnnotationMiddleware(t), newLabelMiddleware(t)} + }, + reallyRunTest: func(t *testing.T, c *Client) { + // create + apiService, err := c.Aggregation. + ApiregistrationV1(). + APIServices(). + Create(context.Background(), goodAPIService, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodAPIService, apiService) + + // read + apiService, err = c.Aggregation. + ApiregistrationV1(). + APIServices(). + Get(context.Background(), apiService.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodAPIService, annotations(), labels()), apiService) + + // update + goodAPIServiceWithAnnotationsAndLabelsAndClusterName := with(goodAPIService, annotations(), labels(), clusterName()).(*apiregistrationv1.APIService) + apiService, err = c.Aggregation. + ApiregistrationV1(). + APIServices(). + Update(context.Background(), goodAPIServiceWithAnnotationsAndLabelsAndClusterName, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, goodAPIServiceWithAnnotationsAndLabelsAndClusterName, apiService) + + // delete + err = c.Aggregation. + ApiregistrationV1(). + APIServices(). + Delete(context.Background(), apiService.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }, + wantMiddlewareReqs: [][]Object{ + { + with(goodAPIService, gvk(apiServiceGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), clusterName(), gvk(apiServiceGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(apiServiceGVK)), + }, + { + with(goodAPIService, annotations(), gvk(apiServiceGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), clusterName(), gvk(apiServiceGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(apiServiceGVK)), + }, + }, + wantMiddlewareResps: [][]Object{ + { + with(goodAPIService, annotations(), labels(), gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), clusterName(), gvk(apiServiceGVK)), + }, + { + with(goodAPIService, emptyAnnotations(), labels(), gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), gvk(apiServiceGVK)), + with(goodAPIService, annotations(), labels(), clusterName(), gvk(apiServiceGVK)), + }, + }, + }, + { + name: "crud concierge api", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newAnnotationMiddleware(t), newLabelMiddleware(t)} + }, + reallyRunTest: func(t *testing.T, c *Client) { + // create + tokenCredentialRequest, err := c.PinnipedConcierge. + LoginV1alpha1(). + TokenCredentialRequests(goodTokenCredentialRequest.Namespace). + Create(context.Background(), goodTokenCredentialRequest, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodTokenCredentialRequest, tokenCredentialRequest) + + // read + tokenCredentialRequest, err = c.PinnipedConcierge. + LoginV1alpha1(). + TokenCredentialRequests(tokenCredentialRequest.Namespace). + Get(context.Background(), tokenCredentialRequest.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodTokenCredentialRequest, annotations(), labels()), tokenCredentialRequest) + + // update + goodTokenCredentialRequestWithAnnotationsAndLabelsAndClusterName := with(goodTokenCredentialRequest, annotations(), labels(), clusterName()).(*loginv1alpha1.TokenCredentialRequest) + tokenCredentialRequest, err = c.PinnipedConcierge. + LoginV1alpha1(). + TokenCredentialRequests(tokenCredentialRequest.Namespace). + Update(context.Background(), goodTokenCredentialRequestWithAnnotationsAndLabelsAndClusterName, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, goodTokenCredentialRequestWithAnnotationsAndLabelsAndClusterName, tokenCredentialRequest) + + // delete + err = c.PinnipedConcierge. + LoginV1alpha1(). + TokenCredentialRequests(tokenCredentialRequest.Namespace). + Delete(context.Background(), tokenCredentialRequest.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }, + wantMiddlewareReqs: [][]Object{ + { + with(goodTokenCredentialRequest, gvk(tokenCredentialRequestGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), clusterName(), gvk(tokenCredentialRequestGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(tokenCredentialRequestGVK)), + }, + { + with(goodTokenCredentialRequest, annotations(), gvk(tokenCredentialRequestGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), clusterName(), gvk(tokenCredentialRequestGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(tokenCredentialRequestGVK)), + }, + }, + wantMiddlewareResps: [][]Object{ + { + with(goodTokenCredentialRequest, annotations(), labels(), gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), clusterName(), gvk(tokenCredentialRequestGVK)), + }, + { + with(goodTokenCredentialRequest, emptyAnnotations(), labels(), gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), gvk(tokenCredentialRequestGVK)), + with(goodTokenCredentialRequest, annotations(), labels(), clusterName(), gvk(tokenCredentialRequestGVK)), + }, + }, + }, + { + name: "crud supervisor api", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newAnnotationMiddleware(t), newLabelMiddleware(t)} + }, + reallyRunTest: func(t *testing.T, c *Client) { + // create + federationDomain, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Create(context.Background(), goodFederationDomain, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodFederationDomain, federationDomain) + + // read + federationDomain, err = c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(federationDomain.Namespace). + Get(context.Background(), federationDomain.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodFederationDomain, annotations(), labels()), federationDomain) + + // update + goodFederationDomainWithAnnotationsAndLabelsAndClusterName := with(goodFederationDomain, annotations(), labels(), clusterName()).(*configv1alpha1.FederationDomain) + federationDomain, err = c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(federationDomain.Namespace). + Update(context.Background(), goodFederationDomainWithAnnotationsAndLabelsAndClusterName, metav1.UpdateOptions{}) + require.NoError(t, err) + require.Equal(t, goodFederationDomainWithAnnotationsAndLabelsAndClusterName, federationDomain) + + // delete + err = c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(federationDomain.Namespace). + Delete(context.Background(), federationDomain.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + }, + wantMiddlewareReqs: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), clusterName(), gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, annotations(), gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), clusterName(), gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + }, + wantMiddlewareResps: [][]Object{ + { + with(goodFederationDomain, annotations(), labels(), gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), clusterName(), gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, emptyAnnotations(), labels(), gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), gvk(federationDomainGVK)), + with(goodFederationDomain, annotations(), labels(), clusterName(), gvk(federationDomainGVK)), + }, + }, + }, + { + name: "we don't call any middleware if there are no mutation funcs", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, false, false, false), newSimpleMiddleware(t, false, false, false)} + }, + reallyRunTest: createGetFederationDomainTest, + wantMiddlewareReqs: [][]Object{nil, nil}, + wantMiddlewareResps: [][]Object{nil, nil}, + }, + { + name: "we don't call any resp middleware if there was no req mutations done and there are no resp mutation funcs", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, true, false, false), newSimpleMiddleware(t, true, false, false)} + }, + reallyRunTest: createGetFederationDomainTest, + wantMiddlewareReqs: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + }, + wantMiddlewareResps: [][]Object{nil, nil}, + }, + { + name: "we don't call any resp middleware if there are no resp mutation funcs even if there was req mutations done", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, true, true, false), newSimpleMiddleware(t, true, true, false)} + }, + reallyRunTest: func(t *testing.T, c *Client) { + // create + federationDomain, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Create(context.Background(), goodFederationDomain, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodFederationDomain, clusterName()), federationDomain) + + // read + federationDomain, err = c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(federationDomain.Namespace). + Get(context.Background(), federationDomain.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, with(goodFederationDomain, clusterName()), federationDomain) + }, + wantMiddlewareReqs: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, clusterName(), gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + }, + wantMiddlewareResps: [][]Object{nil, nil}, + }, + { + name: "we still call resp middleware if there is a resp mutation func even if there were req mutation funcs", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, false, false, true), newSimpleMiddleware(t, false, false, true)} + }, + reallyRunTest: createGetFederationDomainTest, + wantMiddlewareReqs: [][]Object{nil, nil}, + wantMiddlewareResps: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(goodFederationDomain, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(goodFederationDomain, gvk(federationDomainGVK)), + }, + }, + }, + { + name: "we still call resp middleware if there is a resp mutation func even if there was no req mutation", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, true, false, true), newSimpleMiddleware(t, true, false, true)} + }, + reallyRunTest: createGetFederationDomainTest, + wantMiddlewareReqs: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK)), + }, + }, + wantMiddlewareResps: [][]Object{ + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(goodFederationDomain, gvk(federationDomainGVK)), + }, + { + with(goodFederationDomain, gvk(federationDomainGVK)), + with(goodFederationDomain, gvk(federationDomainGVK)), + }, + }, + }, + { + name: "mutating object meta on a get request is not allowed since that isn't pertinent to the api request", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{{ + name: "non-pertinent mutater", + t: t, + mutateReq: func(rt RoundTrip, obj Object) { + clusterName()(obj) + }, + }} + }, + reallyRunTest: func(t *testing.T, c *Client) { + _, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Get(context.Background(), goodFederationDomain.Name, metav1.GetOptions{}) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid object meta mutation") + }, + wantMiddlewareReqs: [][]Object{{with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}}, + wantMiddlewareResps: [][]Object{nil}, + }, + { + name: "when the client gets errors from the api server", + middlewares: func(t *testing.T) []*spyMiddleware { + return []*spyMiddleware{newSimpleMiddleware(t, true, false, false)} + }, + editRestConfig: func(t *testing.T, restConfig *rest.Config) { + restConfig.Dial = func(_ context.Context, _, _ string) (net.Conn, error) { + return nil, fmt.Errorf("some fake connection error") + } + }, + reallyRunTest: func(t *testing.T, c *Client) { + _, err := c.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Get(context.Background(), goodFederationDomain.Name, metav1.GetOptions{}) + require.Error(t, err) + require.Contains(t, err.Error(), ": some fake connection error") + }, + wantMiddlewareReqs: [][]Object{{with(&metav1.PartialObjectMetadata{}, gvk(federationDomainGVK))}}, + wantMiddlewareResps: [][]Object{nil}, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + server, restConfig := fakekubeapi.Start(t, nil) + defer server.Close() + + if test.editRestConfig != nil { + test.editRestConfig(t, restConfig) + } + + // our rt chain is: + // kubeclient -> wantCloseResp -> http.DefaultTransport -> wantCloseResp -> kubeclient + restConfig.Wrap(wantCloseRespWrapper(t)) + + var middlewares []*spyMiddleware + if test.middlewares != nil { + middlewares = test.middlewares(t) + } + + opts := []Option{WithConfig(restConfig)} + for _, middleware := range middlewares { + opts = append(opts, WithMiddleware(middleware)) + } + client, err := New(opts...) + require.NoError(t, err) + + test.reallyRunTest(t, client) + + for i, spyMiddleware := range middlewares { + require.Equalf(t, test.wantMiddlewareReqs[i], spyMiddleware.reqObjs, "unexpected req obj in middleware %q (index %d)", spyMiddleware.name, i) + require.Equalf(t, test.wantMiddlewareResps[i], spyMiddleware.respObjs, "unexpected resp obj in middleware %q (index %d)", spyMiddleware.name, i) + } + }) + } +} + +type spyMiddleware struct { + name string + t *testing.T + mutateReq func(RoundTrip, Object) + mutateResp func(RoundTrip, Object) + reqObjs []Object + respObjs []Object +} + +func (s *spyMiddleware) Handle(_ context.Context, rt RoundTrip) { + s.t.Log(s.name, "handling", reqStr(rt, nil)) + + if s.mutateReq != nil { + rt.MutateRequest(func(obj Object) { + s.t.Log(s.name, "mutating request", reqStr(rt, obj)) + s.reqObjs = append(s.reqObjs, obj.DeepCopyObject().(Object)) + s.mutateReq(rt, obj) + }) + } + + if s.mutateResp != nil { + rt.MutateResponse(func(obj Object) { + s.t.Log(s.name, "mutating response", reqStr(rt, obj)) + s.respObjs = append(s.respObjs, obj.DeepCopyObject().(Object)) + s.mutateResp(rt, obj) + }) + } +} + +func reqStr(rt RoundTrip, obj Object) string { + b := strings.Builder{} + fmt.Fprintf(&b, "%s /%s", rt.Verb(), rt.Resource().GroupVersion()) + if rt.NamespaceScoped() { + fmt.Fprintf(&b, "/namespaces/%s", rt.Namespace()) + } + fmt.Fprintf(&b, "/%s", rt.Resource().Resource) + if obj != nil { + fmt.Fprintf(&b, "/%s", obj.GetName()) + } + return b.String() +} + +func newAnnotationMiddleware(t *testing.T) *spyMiddleware { + return &spyMiddleware{ + name: "annotater", + t: t, + mutateReq: func(rt RoundTrip, obj Object) { + if rt.Verb() == VerbCreate { + annotations()(obj) + } + }, + mutateResp: func(rt RoundTrip, obj Object) { + if rt.Verb() == VerbCreate { + for key := range middlewareAnnotations { + delete(obj.GetAnnotations(), key) + } + } + }, + } +} + +func newLabelMiddleware(t *testing.T) *spyMiddleware { + return &spyMiddleware{ + name: "labeler", + t: t, + mutateReq: func(rt RoundTrip, obj Object) { + if rt.Verb() == VerbCreate { + labels()(obj) + } + }, + mutateResp: func(rt RoundTrip, obj Object) { + if rt.Verb() == VerbCreate { + for key := range middlewareLabels { + delete(obj.GetLabels(), key) + } + } + }, + } +} + +func newSimpleMiddleware(t *testing.T, hasMutateReqFunc, mutatedReq, hasMutateRespFunc bool) *spyMiddleware { + m := &spyMiddleware{ + name: "nop", + t: t, + } + if hasMutateReqFunc { + m.mutateReq = func(rt RoundTrip, obj Object) { + if mutatedReq { + if rt.Verb() == VerbCreate { + obj.SetClusterName(someClusterName) + } + } + } + } + if hasMutateRespFunc { + m.mutateResp = func(rt RoundTrip, obj Object) {} + } + return m +} + +type wantCloser struct { + io.ReadCloser + closeCount int + couldReadBytesJustBeforeClosing bool +} + +func (wc *wantCloser) Close() error { + wc.closeCount++ + n, _ := wc.ReadCloser.Read([]byte{0}) + if n > 0 { + // there were still bytes left to be read + wc.couldReadBytesJustBeforeClosing = true + } + return wc.ReadCloser.Close() +} + +// wantCloseRespWrapper returns a transport.WrapperFunc that validates that the http.Response +// returned by the underlying http.RoundTripper is closed properly. +func wantCloseRespWrapper(t *testing.T) transport.WrapperFunc { + _, file, line, ok := runtime.Caller(1) + if !ok { + file = "???" + line = 0 + } + return func(rt http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + resp, err := rt.RoundTrip(req) + if err != nil { + // request failed, so there is no response body to watch for Close() calls on + return false, resp, err + } + wc := &wantCloser{ReadCloser: resp.Body} + t.Cleanup(func() { + require.False(t, wc.couldReadBytesJustBeforeClosing, "did not consume all response body bytes before closing %s:%d", file, line) + require.Equalf(t, wc.closeCount, 1, "did not close resp body at %s:%d", file, line) + }) + resp.Body = wc + return false, resp, err + }) + } +} + +type withFunc func(obj Object) + +func with(obj Object, withFuncs ...withFunc) Object { + obj = obj.DeepCopyObject().(Object) + for _, withFunc := range withFuncs { + withFunc(obj) + } + return obj +} + +func gvk(gvk schema.GroupVersionKind) withFunc { + return func(obj Object) { + obj.GetObjectKind().SetGroupVersionKind(gvk) + } +} + +func annotations() withFunc { + return func(obj Object) { + obj.SetAnnotations(middlewareAnnotations) + } +} + +func emptyAnnotations() withFunc { + return func(obj Object) { + obj.SetAnnotations(make(map[string]string)) + } +} + +func labels() withFunc { + return func(obj Object) { + obj.SetLabels(middlewareLabels) + } +} + +func clusterName() withFunc { + return func(obj Object) { + obj.SetClusterName(someClusterName) + } +} + +func createGetFederationDomainTest(t *testing.T, client *Client) { + t.Helper() + + // create + federationDomain, err := client.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(goodFederationDomain.Namespace). + Create(context.Background(), goodFederationDomain, metav1.CreateOptions{}) + require.NoError(t, err) + require.Equal(t, goodFederationDomain, federationDomain) + + // read + federationDomain, err = client.PinnipedSupervisor. + ConfigV1alpha1(). + FederationDomains(federationDomain.Namespace). + Get(context.Background(), federationDomain.Name, metav1.GetOptions{}) + require.NoError(t, err) + require.Equal(t, goodFederationDomain, federationDomain) +} diff --git a/internal/kubeclient/middleware.go b/internal/kubeclient/middleware.go new file mode 100644 index 00000000..4c4a02be --- /dev/null +++ b/internal/kubeclient/middleware.go @@ -0,0 +1,147 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type Middleware interface { + Handle(ctx context.Context, rt RoundTrip) +} + +var _ Middleware = MiddlewareFunc(nil) + +type MiddlewareFunc func(ctx context.Context, rt RoundTrip) + +func (f MiddlewareFunc) Handle(ctx context.Context, rt RoundTrip) { + f(ctx, rt) +} + +var _ Middleware = Middlewares{} + +type Middlewares []Middleware + +func (m Middlewares) Handle(ctx context.Context, rt RoundTrip) { + for _, middleware := range m { + middleware := middleware + middleware.Handle(ctx, rt) + } +} + +type RoundTrip interface { + Verb() Verb + Namespace() string // this is the only valid way to check namespace, Object.GetNamespace() will almost always be empty + NamespaceScoped() bool + Resource() schema.GroupVersionResource + Subresource() string + MutateRequest(f func(obj Object)) + MutateResponse(f func(obj Object)) +} + +type Object interface { + runtime.Object // generic access to TypeMeta + metav1.Object // generic access to ObjectMeta +} + +var _ RoundTrip = &request{} + +type request struct { + verb Verb + namespace string + resource schema.GroupVersionResource + reqFuncs, respFuncs []func(obj Object) + subresource string +} + +func (r *request) Verb() Verb { + return r.verb +} + +func (r *request) Namespace() string { + return r.namespace +} + +//nolint: gochecknoglobals +var namespaceGVR = corev1.SchemeGroupVersion.WithResource("namespaces") + +func (r *request) NamespaceScoped() bool { + if r.Resource() == namespaceGVR { + return false // always consider namespaces to be cluster scoped + } + + return len(r.Namespace()) != 0 +} + +func (r *request) Resource() schema.GroupVersionResource { + return r.resource +} + +func (r *request) Subresource() string { + return r.subresource +} + +func (r *request) MutateRequest(f func(obj Object)) { + r.reqFuncs = append(r.reqFuncs, f) +} + +func (r *request) MutateResponse(f func(obj Object)) { + r.respFuncs = append(r.respFuncs, f) +} + +type mutationResult struct { + origGVK, newGVK schema.GroupVersionKind + gvkChanged, mutated bool +} + +func (r *request) mutateRequest(obj Object) (*mutationResult, error) { + origGVK := obj.GetObjectKind().GroupVersionKind() + if origGVK.Empty() { + return nil, fmt.Errorf("invalid empty orig GVK for %T: %#v", obj, r) + } + + origObj, ok := obj.DeepCopyObject().(Object) + if !ok { + return nil, fmt.Errorf("invalid deep copy semantics for %T: %#v", obj, r) + } + + for _, reqFunc := range r.reqFuncs { + reqFunc := reqFunc + reqFunc(obj) + } + + newGVK := obj.GetObjectKind().GroupVersionKind() + if newGVK.Empty() { + return nil, fmt.Errorf("invalid empty new GVK for %T: %#v", obj, r) + } + + return &mutationResult{ + origGVK: origGVK, + newGVK: newGVK, + gvkChanged: origGVK != newGVK, + mutated: len(r.respFuncs) != 0 || !apiequality.Semantic.DeepEqual(origObj, obj), + }, nil +} + +func (r *request) mutateResponse(obj Object) (bool, error) { + origObj, ok := obj.DeepCopyObject().(Object) + if !ok { + return false, fmt.Errorf("invalid deep copy semantics for %T: %#v", obj, r) + } + + for _, respFunc := range r.respFuncs { + respFunc := respFunc + respFunc(obj) + } + + mutated := !apiequality.Semantic.DeepEqual(origObj, obj) + return mutated, nil +} diff --git a/internal/kubeclient/middleware_test.go b/internal/kubeclient/middleware_test.go new file mode 100644 index 00000000..4c94e8df --- /dev/null +++ b/internal/kubeclient/middleware_test.go @@ -0,0 +1,74 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func Test_request_mutate(t *testing.T) { + tests := []struct { + name string + reqFuncs []func(Object) + obj Object + want *mutationResult + wantObj Object + wantErr string + }{ + { + name: "mutate config map data", + reqFuncs: []func(Object){ + func(obj Object) { + cm := obj.(*corev1.ConfigMap) + cm.Data = map[string]string{"new": "stuff"} + }, + }, + obj: &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + Data: map[string]string{"old": "things"}, + BinaryData: map[string][]byte{"weee": nil}, + }, + want: &mutationResult{ + origGVK: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"}, + newGVK: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "ConfigMap"}, + gvkChanged: false, + mutated: true, + }, + wantObj: &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + Data: map[string]string{"new": "stuff"}, + BinaryData: map[string][]byte{"weee": nil}, + }, + wantErr: "", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + r := &request{reqFuncs: tt.reqFuncs} + orig := tt.obj.DeepCopyObject() + + got, err := r.mutateRequest(tt.obj) + + if len(tt.wantErr) > 0 { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + + require.Equal(t, tt.want, got) + + if tt.wantObj != nil { + require.Equal(t, tt.wantObj, tt.obj) + } else { + require.Equal(t, orig, tt.obj) + } + }) + } +} diff --git a/internal/kubeclient/option.go b/internal/kubeclient/option.go index ba4ada35..789120fb 100644 --- a/internal/kubeclient/option.go +++ b/internal/kubeclient/option.go @@ -20,6 +20,10 @@ func WithConfig(config *restclient.Config) Option { func WithMiddleware(middleware Middleware) Option { return func(c *clientConfig) { + if middleware == nil { + return // support passing in a nil middleware as a no-op + } + c.middlewares = append(c.middlewares, middleware) } } diff --git a/internal/kubeclient/path.go b/internal/kubeclient/path.go new file mode 100644 index 00000000..ad27cde8 --- /dev/null +++ b/internal/kubeclient/path.go @@ -0,0 +1,75 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "fmt" + "net/http" + "net/url" + "path" + "strings" + + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + restclient "k8s.io/client-go/rest" +) + +func updatePathNewGVK(reqURL *url.URL, result *mutationResult, apiPathPrefix string, reqInfo *genericapirequest.RequestInfo) (*url.URL, error) { + if !result.gvkChanged { + return reqURL, nil + } + + if len(result.origGVK.Group) == 0 { + return nil, fmt.Errorf("invalid attempt to change core group") + } + + newURL := &url.URL{} + *newURL = *reqURL + + // replace old GVK with new GVK + apiRoot := path.Join(apiPathPrefix, reqInfo.APIPrefix) + oldPrefix := restclient.DefaultVersionedAPIPath(apiRoot, result.origGVK.GroupVersion()) + newPrefix := restclient.DefaultVersionedAPIPath(apiRoot, result.newGVK.GroupVersion()) + + newURL.Path = path.Join(newPrefix, strings.TrimPrefix(newURL.Path, oldPrefix)) + + return newURL, nil +} + +func getHostAndAPIPathPrefix(config *restclient.Config) (string, string, error) { + hostURL, _, err := defaultServerUrlFor(config) + if err != nil { + return "", "", fmt.Errorf("failed to parse host URL from rest config: %w", err) + } + + return hostURL.String(), hostURL.Path, nil +} + +func reqWithoutPrefix(req *http.Request, hostURL, apiPathPrefix string) *http.Request { + if len(apiPathPrefix) == 0 { + return req + } + + if !strings.HasSuffix(hostURL, "/") { + hostURL += "/" + } + + if !strings.HasPrefix(req.URL.String(), hostURL) { + return req + } + + if !strings.HasPrefix(apiPathPrefix, "/") { + apiPathPrefix = "/" + apiPathPrefix + } + if !strings.HasSuffix(apiPathPrefix, "/") { + apiPathPrefix += "/" + } + + reqCopy := req.WithContext(req.Context()) + urlCopy := &url.URL{} + *urlCopy = *reqCopy.URL + urlCopy.Path = "/" + strings.TrimPrefix(urlCopy.Path, apiPathPrefix) + reqCopy.URL = urlCopy + + return reqCopy +} diff --git a/internal/kubeclient/path_test.go b/internal/kubeclient/path_test.go new file mode 100644 index 00000000..2ed1ae61 --- /dev/null +++ b/internal/kubeclient/path_test.go @@ -0,0 +1,231 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "bytes" + "context" + "io/ioutil" + "net/http" + "net/url" + "reflect" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + + loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + configv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1" +) + +func Test_updatePathNewGVK(t *testing.T) { + type args struct { + reqURL *url.URL + result *mutationResult + apiPathPrefix string + reqInfo *genericapirequest.RequestInfo + } + tests := []struct { + name string + args args + want *url.URL + wantErr bool + }{ + { + name: "no gvk change", + args: args{ + reqURL: mustParse(t, "https://walrus.tld/api/v1/pods"), + result: &mutationResult{}, + }, + want: mustParse(t, "https://walrus.tld/api/v1/pods"), + }, + { + name: "no original gvk group", + args: args{ + result: &mutationResult{ + origGVK: schema.GroupVersionKind{ + Group: "", + }, + gvkChanged: true, + }, + }, + wantErr: true, + }, + { + name: "cluster-scoped list path", + args: args{ + reqURL: mustParse(t, "https://walrus.tld/apis/"+loginv1alpha1.SchemeGroupVersion.String()+"/tokencredentialrequests"), + result: &mutationResult{ + origGVK: loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequest"), + newGVK: schema.GroupVersionKind{ + Group: "login.concierge.tuna.io", + Version: loginv1alpha1.SchemeGroupVersion.Version, + Kind: "TokenCredentialRequest", + }, + gvkChanged: true, + }, + apiPathPrefix: "/apis", + reqInfo: &genericapirequest.RequestInfo{}, + }, + want: mustParse(t, "https://walrus.tld/apis/login.concierge.tuna.io/v1alpha1/tokencredentialrequests"), + }, + { + name: "cluster-scoped get path", + args: args{ + reqURL: mustParse(t, "https://walrus.tld/apis/"+loginv1alpha1.SchemeGroupVersion.String()+"/tokencredentialrequests/some-name"), + result: &mutationResult{ + origGVK: loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequest"), + newGVK: schema.GroupVersionKind{ + Group: "login.concierge.tuna.io", + Version: loginv1alpha1.SchemeGroupVersion.Version, + Kind: "TokenCredentialRequest", + }, + gvkChanged: true, + }, + apiPathPrefix: "/apis", + reqInfo: &genericapirequest.RequestInfo{}, + }, + want: mustParse(t, "https://walrus.tld/apis/login.concierge.tuna.io/v1alpha1/tokencredentialrequests/some-name"), + }, + { + name: "namespace-scoped list path", + args: args{ + reqURL: mustParse(t, "https://walrus.tld/apis/"+configv1alpha1.SchemeGroupVersion.String()+"/namespaces/default/federationdomains"), + result: &mutationResult{ + origGVK: configv1alpha1.SchemeGroupVersion.WithKind("FederationDomain"), + newGVK: schema.GroupVersionKind{ + Group: "config.supervisor.tuna.io", + Version: configv1alpha1.SchemeGroupVersion.Version, + Kind: "FederationDomain", + }, + gvkChanged: true, + }, + apiPathPrefix: "/apis", + reqInfo: &genericapirequest.RequestInfo{}, + }, + want: mustParse(t, "https://walrus.tld/apis/config.supervisor.tuna.io/v1alpha1/namespaces/default/federationdomains"), + }, + { + name: "namespace-scoped get path", + args: args{ + reqURL: mustParse(t, "https://walrus.tld/apis/"+configv1alpha1.SchemeGroupVersion.String()+"/namespaces/default/federationdomains/some-name"), + result: &mutationResult{ + origGVK: configv1alpha1.SchemeGroupVersion.WithKind("FederationDomain"), + newGVK: schema.GroupVersionKind{ + Group: "config.supervisor.tuna.io", + Version: configv1alpha1.SchemeGroupVersion.Version, + Kind: "FederationDomain", + }, + gvkChanged: true, + }, + apiPathPrefix: "/apis", + reqInfo: &genericapirequest.RequestInfo{}, + }, + want: mustParse(t, "https://walrus.tld/apis/config.supervisor.tuna.io/v1alpha1/namespaces/default/federationdomains/some-name"), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got, err := updatePathNewGVK(tt.args.reqURL, tt.args.result, tt.args.apiPathPrefix, tt.args.reqInfo) + if (err != nil) != tt.wantErr { + t.Errorf("updatePathNewGVK() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("updatePathNewGVK() got = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_reqWithoutPrefix(t *testing.T) { + body := ioutil.NopCloser(bytes.NewBuffer([]byte("some body"))) + newReq := func(rawurl string) *http.Request { + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawurl, body) + require.NoError(t, err) + return req + } + + type args struct { + req *http.Request + hostURL string + apiPathPrefix string + } + tests := []struct { + name string + args args + want *http.Request + }{ + { + name: "happy path", + args: args{ + req: newReq("https://walrus.tld/apis/some/path"), + hostURL: "https://walrus.tld", + apiPathPrefix: "/apis", + }, + want: newReq("https://walrus.tld/some/path"), + }, + { + name: "host url already has slash suffix", + args: args{ + req: newReq("https://walrus.tld/apis/some/path"), + hostURL: "https://walrus.tld/", + apiPathPrefix: "/apis", + }, + want: newReq("https://walrus.tld/some/path"), + }, + { + name: "api prefix already has slash prefix", + args: args{ + req: newReq("https://walrus.tld/apis/some/path"), + hostURL: "https://walrus.tld", + apiPathPrefix: "apis", + }, + want: newReq("https://walrus.tld/some/path"), + }, + { + name: "api prefix already has slash suffix", + args: args{ + req: newReq("https://walrus.tld/apis/some/path"), + hostURL: "https://walrus.tld", + apiPathPrefix: "/apis/", + }, + want: newReq("https://walrus.tld/some/path"), + }, + { + name: "no api path prefix", + args: args{ + req: newReq("https://walrus.tld"), + }, + want: newReq("https://walrus.tld"), + }, + { + name: "hostURL and req URL mismatch", + args: args{ + req: newReq("https://walrus.tld.some-other-url/some/path"), + hostURL: "https://walrus.tld", + apiPathPrefix: "/apis", + }, + want: newReq("https://walrus.tld.some-other-url/some/path"), + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + req := *tt.args.req + if got := reqWithoutPrefix(&req, tt.args.hostURL, tt.args.apiPathPrefix); !reflect.DeepEqual(got, tt.want) { + t.Errorf("reqWithoutPrefix() = %v, want %v", got, tt.want) + } + }) + } +} + +func mustParse(t *testing.T, rawurl string) *url.URL { + t.Helper() + url, err := url.Parse(rawurl) + require.NoError(t, err) + return url +} diff --git a/internal/kubeclient/roundtrip.go b/internal/kubeclient/roundtrip.go index 38240cdd..5caf2a39 100644 --- a/internal/kubeclient/roundtrip.go +++ b/internal/kubeclient/roundtrip.go @@ -9,14 +9,27 @@ import ( "io/ioutil" "net/http" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/server" restclient "k8s.io/client-go/rest" + "k8s.io/client-go/transport" + + "go.pinniped.dev/internal/plog" ) -// TODO unit test +func configWithWrapper(config *restclient.Config, scheme *runtime.Scheme, negotiatedSerializer runtime.NegotiatedSerializer, middlewares []Middleware) *restclient.Config { + hostURL, apiPathPrefix, err := getHostAndAPIPathPrefix(config) + if err != nil { + plog.DebugErr("invalid rest config", err) + return config // invalid input config, will fail existing client-go validation + } -func configWithWrapper(config *restclient.Config, negotiatedSerializer runtime.NegotiatedSerializer, middlewares []Middleware) *restclient.Config { // no need for any wrapping when we have no middleware to inject if len(middlewares) == 0 { return config @@ -26,97 +39,346 @@ func configWithWrapper(config *restclient.Config, negotiatedSerializer runtime.N if !ok { panic(fmt.Errorf("unknown content type: %s ", config.ContentType)) // static input, programmer error } - serializer := info.Serializer // should perform no conversion + regSerializer := info.Serializer // should perform no conversion - f := func(rt http.RoundTripper) http.RoundTripper { - return roundTripperFunc(func(req *http.Request) (*http.Response, error) { - // ignore everything that has an unreadable body - if req.GetBody == nil { - return rt.RoundTrip(req) - } + resolver := server.NewRequestInfoResolver(server.NewConfig(serializer.CodecFactory{})) - var reqMiddlewares []Middleware - for _, middleware := range middlewares { - middleware := middleware - if middleware.Handles(req.Method) { - reqMiddlewares = append(reqMiddlewares, middleware) - } - } + schemeRestMapperFunc := schemeRestMapper(scheme) - // no middleware to handle this request - if len(reqMiddlewares) == 0 { - return rt.RoundTrip(req) - } - - body, err := req.GetBody() - if err != nil { - return nil, fmt.Errorf("get body failed: %w", err) - } - defer body.Close() - data, err := ioutil.ReadAll(body) - if err != nil { - return nil, fmt.Errorf("read body failed: %w", err) - } - - // attempt to decode with no defaults or into specified, i.e. defer to the decoder - // this should result in the a straight decode with no conversion - obj, _, err := serializer.Decode(data, nil, nil) - if err != nil { - return nil, fmt.Errorf("body decode failed: %w", err) - } - - accessor, err := meta.Accessor(obj) - if err != nil { - return rt.RoundTrip(req) // ignore everything that has no object meta for now - } - - // run all the mutating operations - var reqMutated bool - for _, reqMiddleware := range reqMiddlewares { - mutated := reqMiddleware.Mutate(accessor) - reqMutated = mutated || reqMutated - } - - // no mutation occurred, keep the original request - if !reqMutated { - return rt.RoundTrip(req) - } - - // we plan on making a new request so make sure to close the original request's body - _ = req.Body.Close() - - newData, err := runtime.Encode(serializer, obj) - if err != nil { - return nil, fmt.Errorf("new body encode failed: %w", err) - } - - // TODO log newData at high loglevel similar to REST client - - // simplest way to reuse the body creation logic - newReqForBody, err := http.NewRequest(req.Method, req.URL.String(), bytes.NewReader(newData)) - if err != nil { - return nil, fmt.Errorf("failed to create new req for body: %w", err) // this should never happen - } - - // shallow copy because we want to preserve all the headers and such but not mutate the original request - newReq := req.WithContext(req.Context()) - - // replace the body with the new data - newReq.ContentLength = newReqForBody.ContentLength - newReq.Body = newReqForBody.Body - newReq.GetBody = newReqForBody.GetBody - - return rt.RoundTrip(newReq) - }) - } + f := newWrapper(hostURL, apiPathPrefix, config, resolver, regSerializer, negotiatedSerializer, schemeRestMapperFunc, middlewares) cc := restclient.CopyConfig(config) cc.Wrap(f) return cc } -type roundTripperFunc func(req *http.Request) (*http.Response, error) +type roundTripperFunc func(req *http.Request) (bool, *http.Response, error) func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { - return f(req) + // always attempt to close the body, as long as we are the ones that handled the request + // see http.RoundTripper doc: + // "RoundTrip must always close the body, including on errors, ..." + handled, resp, err := f(req) + if handled && req.Body != nil { + _ = req.Body.Close() + } + return resp, err +} + +func newWrapper( + hostURL, apiPathPrefix string, + config *restclient.Config, + resolver genericapirequest.RequestInfoResolver, + regSerializer runtime.Serializer, + negotiatedSerializer runtime.NegotiatedSerializer, + schemeRestMapperFunc func(schema.GroupVersionResource, Verb) (schema.GroupVersionKind, bool), + middlewares []Middleware, +) transport.WrapperFunc { + return func(rt http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + reqInfo, err := resolver.NewRequestInfo(reqWithoutPrefix(req, hostURL, apiPathPrefix)) + if err != nil || !reqInfo.IsResourceRequest { + resp, err := rt.RoundTrip(req) // we only handle kube resource requests + return false, resp, err + } + + middlewareReq := &request{ + verb: verb(reqInfo.Verb), + namespace: reqInfo.Namespace, + resource: schema.GroupVersionResource{ + Group: reqInfo.APIGroup, + Version: reqInfo.APIVersion, + Resource: reqInfo.Resource, + }, + subresource: reqInfo.Subresource, + } + + for _, middleware := range middlewares { + middleware := middleware + middleware.Handle(req.Context(), middlewareReq) + } + + if len(middlewareReq.reqFuncs) == 0 && len(middlewareReq.respFuncs) == 0 { + resp, err := rt.RoundTrip(req) // no middleware wanted to mutate this request + return false, resp, err + } + + switch v := middlewareReq.Verb(); v { + case VerbCreate, VerbUpdate: + return handleCreateOrUpdate(req, middlewareReq, regSerializer, rt, apiPathPrefix, reqInfo, config, negotiatedSerializer) + + case VerbGet, VerbList, VerbDelete, VerbDeleteCollection, VerbPatch, VerbWatch: + return handleOtherVerbs(v, req, middlewareReq, schemeRestMapperFunc, rt, apiPathPrefix, reqInfo, config, negotiatedSerializer) + + case VerbProxy: // for now we do not support proxy interception + fallthrough + + default: + resp, err := rt.RoundTrip(req) // we only handle certain verbs + return false, resp, err + } + }) + } +} + +func handleOtherVerbs( + v Verb, + req *http.Request, + middlewareReq *request, + schemeRestMapperFunc func(schema.GroupVersionResource, Verb) (schema.GroupVersionKind, bool), + rt http.RoundTripper, + apiPathPrefix string, + reqInfo *genericapirequest.RequestInfo, + config *restclient.Config, + negotiatedSerializer runtime.NegotiatedSerializer, +) (bool, *http.Response, error) { + mapperGVK, ok := schemeRestMapperFunc(middlewareReq.Resource(), v) + if !ok { + return true, nil, fmt.Errorf("unable to determine GVK for middleware request %#v", middlewareReq) + } + + // no need to do anything with object meta since we only support GVK changes + obj := &metav1.PartialObjectMetadata{} + obj.APIVersion, obj.Kind = mapperGVK.ToAPIVersionAndKind() + + result, err := middlewareReq.mutateRequest(obj) + if err != nil { + return true, nil, err + } + + if !result.mutated { + resp, err := rt.RoundTrip(req) // no middleware mutated the request + return false, resp, err + } + + // sanity check to make sure mutation is to type meta and/or the response + unexpectedMutation := len(middlewareReq.respFuncs) == 0 && !result.gvkChanged + metaIsZero := apiequality.Semantic.DeepEqual(obj.ObjectMeta, metav1.ObjectMeta{}) + if unexpectedMutation || !metaIsZero { + return true, nil, fmt.Errorf("invalid object meta mutation: %#v", middlewareReq) + } + + reqURL, err := updatePathNewGVK(req.URL, result, apiPathPrefix, reqInfo) + if err != nil { + return true, nil, err + } + + // shallow copy because we want to preserve all the headers and such but not mutate the original request + newReq := req.WithContext(req.Context()) + + // replace the body and path with the new data + newReq.URL = reqURL + + glogBody("mutated request url", []byte(reqURL.String())) + + resp, err := rt.RoundTrip(newReq) + if err != nil { + return true, nil, fmt.Errorf("middleware request for %#v failed: %w", middlewareReq, err) + } + + switch v { + case VerbDelete, VerbDeleteCollection: + return true, resp, nil // we do not need to fix the response on delete + + case VerbWatch: + resp, err := handleWatchResponseNewGVK(config, negotiatedSerializer, resp, middlewareReq, result) + return true, resp, err + + default: // VerbGet, VerbList, VerbPatch + resp, err := handleResponseNewGVK(config, negotiatedSerializer, resp, middlewareReq, result) + return true, resp, err + } +} + +func handleCreateOrUpdate( + req *http.Request, + middlewareReq *request, + regSerializer runtime.Serializer, + rt http.RoundTripper, + apiPathPrefix string, + reqInfo *genericapirequest.RequestInfo, + config *restclient.Config, + negotiatedSerializer runtime.NegotiatedSerializer, +) (bool, *http.Response, error) { + if req.GetBody == nil { + return true, nil, fmt.Errorf("unreadible body for request: %#v", middlewareReq) // this should never happen + } + + body, err := req.GetBody() + if err != nil { + return true, nil, fmt.Errorf("get body failed: %w", err) + } + defer body.Close() + data, err := ioutil.ReadAll(body) + if err != nil { + return true, nil, fmt.Errorf("read body failed: %w", err) + } + + // attempt to decode with no defaults or into specified, i.e. defer to the decoder + // this should result in the a straight decode with no conversion + decodedObj, err := runtime.Decode(regSerializer, data) + if err != nil { + return true, nil, fmt.Errorf("body decode failed: %w", err) + } + + obj, ok := decodedObj.(Object) + if !ok { + return true, nil, fmt.Errorf("middleware request for %#v has invalid object semantics: %T", middlewareReq, decodedObj) + } + + result, err := middlewareReq.mutateRequest(obj) + if err != nil { + return true, nil, err + } + + if !result.mutated { + resp, err := rt.RoundTrip(req) // no middleware mutated the request + return false, resp, err + } + + reqURL, err := updatePathNewGVK(req.URL, result, apiPathPrefix, reqInfo) + if err != nil { + return true, nil, err + } + + newData, err := runtime.Encode(regSerializer, obj) + if err != nil { + return true, nil, fmt.Errorf("new body encode failed: %w", err) + } + + // simplest way to reuse the body creation logic + newReqForBody, err := http.NewRequest(req.Method, reqURL.String(), bytes.NewReader(newData)) + if err != nil { + return true, nil, fmt.Errorf("failed to create new req for body: %w", err) // this should never happen + } + + // shallow copy because we want to preserve all the headers and such but not mutate the original request + newReq := req.WithContext(req.Context()) + + // replace the body and path with the new data + newReq.URL = reqURL + newReq.ContentLength = newReqForBody.ContentLength + newReq.Body = newReqForBody.Body + newReq.GetBody = newReqForBody.GetBody + + glogBody("mutated request", newData) + + resp, err := rt.RoundTrip(newReq) + if err != nil { + return true, nil, fmt.Errorf("middleware request for %#v failed: %w", middlewareReq, err) + } + + if !result.gvkChanged && len(middlewareReq.respFuncs) == 0 { + return true, resp, nil // we did not change the GVK, so we do not need to mess with the incoming data + } + + resp, err = handleResponseNewGVK(config, negotiatedSerializer, resp, middlewareReq, result) + return true, resp, err +} + +func handleResponseNewGVK( + config *restclient.Config, + negotiatedSerializer runtime.NegotiatedSerializer, + resp *http.Response, + middlewareReq *request, + result *mutationResult, +) (*http.Response, error) { + // defer these status codes to client-go + switch { + case resp.StatusCode == http.StatusSwitchingProtocols, + resp.StatusCode < http.StatusOK || resp.StatusCode > http.StatusPartialContent: + return resp, nil + } + + // always make sure we close the body, even if reading from it fails + defer resp.Body.Close() + respData, err := ioutil.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + + serializerInfo, err := getSerializerInfo(config, negotiatedSerializer, resp, middlewareReq) + if err != nil { + return nil, err + } + + fixedRespData, err := maybeRestoreGVK(serializerInfo.Serializer, respData, result) + if err != nil { + return nil, fmt.Errorf("unable to restore GVK for %#v: %w", middlewareReq, err) + } + + fixedRespData, err = maybeMutateResponse(serializerInfo.Serializer, fixedRespData, middlewareReq, result) + if err != nil { + return nil, fmt.Errorf("unable to mutate response for %#v: %w", middlewareReq, err) + } + + newResp := &http.Response{} + *newResp = *resp + + newResp.Body = ioutil.NopCloser(bytes.NewBuffer(fixedRespData)) + return newResp, nil +} + +func maybeMutateResponse(serializer runtime.Serializer, fixedRespData []byte, middlewareReq *request, result *mutationResult) ([]byte, error) { + if len(middlewareReq.respFuncs) == 0 { + return fixedRespData, nil + } + + decodedObj, err := runtime.Decode(serializer, fixedRespData) + if err != nil { + return fixedRespData, nil // if we cannot decode it, it is not for us - let client-go figure out what to do + } + + if decodedObj.GetObjectKind().GroupVersionKind() != result.origGVK { + return fixedRespData, nil + } + + var mutated bool + + switch middlewareReq.Verb() { + case VerbList: + if err := meta.EachListItem(decodedObj, func(listObj runtime.Object) error { + obj, ok := listObj.(Object) + if !ok { + return fmt.Errorf("middleware request for %#v has invalid object semantics: %T", middlewareReq, decodedObj) + } + + singleMutated, err := middlewareReq.mutateResponse(obj) + if err != nil { + return fmt.Errorf("response mutation failed for %#v: %w", middlewareReq, err) + } + + mutated = mutated || singleMutated + + return nil + }); err != nil { + return nil, fmt.Errorf("failed to iterate over list for %#v: %T", middlewareReq, decodedObj) + } + + default: + obj, ok := decodedObj.(Object) + if !ok { + return nil, fmt.Errorf("middleware request for %#v has invalid object semantics: %T", middlewareReq, decodedObj) + } + + mutated, err = middlewareReq.mutateResponse(obj) + if err != nil { + return nil, fmt.Errorf("response mutation failed for %#v: %w", middlewareReq, err) + } + } + + if !mutated { + return fixedRespData, nil + } + + newData, err := runtime.Encode(serializer, decodedObj) + if err != nil { + return nil, fmt.Errorf("new body encode failed: %w", err) + } + + // only log if we mutated the response; we only need to log the unmutated response since client-go + // will log the mutated response for us + glogBody("unmutated response", fixedRespData) + + return newData, nil } diff --git a/internal/kubeclient/scheme.go b/internal/kubeclient/scheme.go new file mode 100644 index 00000000..4c6e8f14 --- /dev/null +++ b/internal/kubeclient/scheme.go @@ -0,0 +1,84 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/gengo/namer" + "k8s.io/gengo/types" +) + +type objectList interface { + runtime.Object // generic access to TypeMeta + metav1.ListInterface // generic access to ListMeta +} + +func schemeRestMapper(scheme *runtime.Scheme) func(schema.GroupVersionResource, Verb) (schema.GroupVersionKind, bool) { + // we are assuming that no code uses the `// +resourceName=CUSTOM_RESOURCE_NAME` directive + // and that no Kube code generator is passed a --plural-exceptions argument + pluralExceptions := map[string]string{"Endpoints": "Endpoints"} // default copied from client-gen + lowercaseNamer := namer.NewAllLowercasePluralNamer(pluralExceptions) + + listVerbMapping := map[schema.GroupVersionResource]schema.GroupVersionKind{} + nonListVerbMapping := map[schema.GroupVersionResource]schema.GroupVersionKind{} + + for gvk := range scheme.AllKnownTypes() { + obj, err := scheme.New(gvk) + if err != nil { + panic(err) // programmer error (internal scheme code is broken) + } + + switch t := obj.(type) { + case interface { + Object + objectList + }: + panic(fmt.Errorf("type is both list and non-list: %T", t)) + + case Object: + resource := lowercaseNamer.Name(types.Ref("ignored", gvk.Kind)) + gvr := gvk.GroupVersion().WithResource(resource) + nonListVerbMapping[gvr] = gvk + + case objectList: + if _, ok := t.(*metav1.Status); ok { + continue // ignore status since it does not have an Items field + } + + itemsPtr, err := meta.GetItemsPtr(obj) + if err != nil { + panic(err) // programmer error (internal scheme code is broken) + } + items, err := conversion.EnforcePtr(itemsPtr) + if err != nil { + panic(err) // programmer error (internal scheme code is broken) + } + nonListKind := items.Type().Elem().Name() + resource := lowercaseNamer.Name(types.Ref("ignored", nonListKind)) + gvr := gvk.GroupVersion().WithResource(resource) + listVerbMapping[gvr] = gvk + + default: + // ignore stuff like ListOptions + } + } + + return func(resource schema.GroupVersionResource, v Verb) (schema.GroupVersionKind, bool) { + switch v { + case VerbList: + gvk, ok := listVerbMapping[resource] + return gvk, ok + + default: + gvk, ok := nonListVerbMapping[resource] + return gvk, ok + } + } +} diff --git a/internal/kubeclient/scheme_test.go b/internal/kubeclient/scheme_test.go new file mode 100644 index 00000000..7094ba59 --- /dev/null +++ b/internal/kubeclient/scheme_test.go @@ -0,0 +1,156 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubescheme "k8s.io/client-go/kubernetes/scheme" + apiregistrationv1 "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1" + aggregatorclientscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" + + loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" + idpv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/idp/v1alpha1" + pinnipedconciergeclientsetscheme "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned/scheme" + pinnipedsupervisorclientsetscheme "go.pinniped.dev/generated/1.20/client/supervisor/clientset/versioned/scheme" +) + +func Test_schemeRestMapper(t *testing.T) { + type args struct { + scheme *runtime.Scheme + gvr schema.GroupVersionResource + v Verb + } + tests := []struct { + name string + args args + want schema.GroupVersionKind + }{ + { + name: "config map get", + args: args{ + scheme: kubescheme.Scheme, + gvr: corev1.SchemeGroupVersion.WithResource("configmaps"), + v: VerbGet, + }, + want: corev1.SchemeGroupVersion.WithKind("ConfigMap"), + }, + { + name: "config map list", + args: args{ + scheme: kubescheme.Scheme, + gvr: corev1.SchemeGroupVersion.WithResource("configmaps"), + v: VerbList, + }, + want: corev1.SchemeGroupVersion.WithKind("ConfigMapList"), + }, + { + name: "endpoints patch", + args: args{ + scheme: kubescheme.Scheme, + gvr: corev1.SchemeGroupVersion.WithResource("endpoints"), + v: VerbPatch, + }, + want: corev1.SchemeGroupVersion.WithKind("Endpoints"), + }, + { + name: "endpoints list", + args: args{ + scheme: kubescheme.Scheme, + gvr: corev1.SchemeGroupVersion.WithResource("endpoints"), + v: VerbList, + }, + want: corev1.SchemeGroupVersion.WithKind("EndpointsList"), + }, + { + name: "api service create", + args: args{ + scheme: aggregatorclientscheme.Scheme, + gvr: apiregistrationv1.SchemeGroupVersion.WithResource("apiservices"), + v: VerbCreate, + }, + want: apiregistrationv1.SchemeGroupVersion.WithKind("APIService"), + }, + { + name: "api service create - wrong scheme", + args: args{ + scheme: kubescheme.Scheme, + gvr: apiregistrationv1.SchemeGroupVersion.WithResource("apiservices"), + v: VerbCreate, + }, + }, + { + name: "api service list", + args: args{ + scheme: aggregatorclientscheme.Scheme, + gvr: apiregistrationv1.SchemeGroupVersion.WithResource("apiservices"), + v: VerbList, + }, + want: apiregistrationv1.SchemeGroupVersion.WithKind("APIServiceList"), + }, + { + name: "token credential delete", + args: args{ + scheme: pinnipedconciergeclientsetscheme.Scheme, + gvr: loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests"), + v: VerbDelete, + }, + want: loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequest"), + }, + { + name: "token credential list", + args: args{ + scheme: pinnipedconciergeclientsetscheme.Scheme, + gvr: loginv1alpha1.SchemeGroupVersion.WithResource("tokencredentialrequests"), + v: VerbList, + }, + want: loginv1alpha1.SchemeGroupVersion.WithKind("TokenCredentialRequestList"), + }, + { + name: "oidc idp update", + args: args{ + scheme: pinnipedsupervisorclientsetscheme.Scheme, + gvr: idpv1alpha1.SchemeGroupVersion.WithResource("oidcidentityproviders"), + v: VerbUpdate, + }, + want: idpv1alpha1.SchemeGroupVersion.WithKind("OIDCIdentityProvider"), + }, + { + name: "oidc idp list", + args: args{ + scheme: pinnipedsupervisorclientsetscheme.Scheme, + gvr: idpv1alpha1.SchemeGroupVersion.WithResource("oidcidentityproviders"), + v: VerbList, + }, + want: idpv1alpha1.SchemeGroupVersion.WithKind("OIDCIdentityProviderList"), + }, + { + name: "oidc idp list - wrong scheme", + args: args{ + scheme: pinnipedconciergeclientsetscheme.Scheme, + gvr: idpv1alpha1.SchemeGroupVersion.WithResource("oidcidentityproviders"), + v: VerbList, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + schemeRestMapperFunc := schemeRestMapper(tt.args.scheme) + gvk, ok := schemeRestMapperFunc(tt.args.gvr, tt.args.v) + + if tt.want.Empty() { + require.True(t, gvk.Empty()) + require.False(t, ok) + } else { + require.Equal(t, tt.want, gvk) + require.True(t, ok) + } + }) + } +} diff --git a/internal/kubeclient/serializer.go b/internal/kubeclient/serializer.go new file mode 100644 index 00000000..c7cf7130 --- /dev/null +++ b/internal/kubeclient/serializer.go @@ -0,0 +1,39 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "fmt" + "mime" + "net/http" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + restclient "k8s.io/client-go/rest" +) + +type passthroughDecoder struct{} + +func (d passthroughDecoder) Decode(data []byte, _ *schema.GroupVersionKind, _ runtime.Object) (runtime.Object, *schema.GroupVersionKind, error) { + return &runtime.Unknown{Raw: data}, &schema.GroupVersionKind{}, nil +} + +func getSerializerInfo(config *restclient.Config, negotiatedSerializer runtime.NegotiatedSerializer, resp *http.Response, middlewareReq *request) (runtime.SerializerInfo, error) { + contentType := resp.Header.Get("Content-Type") + if len(contentType) == 0 { + contentType = config.ContentType + } + + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil { + return runtime.SerializerInfo{}, fmt.Errorf("failed to parse content type for %#v: %w", middlewareReq, err) + } + + respInfo, ok := runtime.SerializerInfoForMediaType(negotiatedSerializer.SupportedMediaTypes(), mediaType) + if !ok || respInfo.Serializer == nil || respInfo.StreamSerializer == nil || respInfo.StreamSerializer.Serializer == nil || respInfo.StreamSerializer.Framer == nil { + return runtime.SerializerInfo{}, fmt.Errorf("unable to find resp serialier for %#v with content-type %s", middlewareReq, mediaType) + } + + return respInfo, nil +} diff --git a/internal/kubeclient/verb.go b/internal/kubeclient/verb.go new file mode 100644 index 00000000..bd95b668 --- /dev/null +++ b/internal/kubeclient/verb.go @@ -0,0 +1,27 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +type Verb interface { + verb() // private method to prevent creation of verbs outside this package +} + +const ( + VerbCreate verb = "create" + VerbUpdate verb = "update" + VerbDelete verb = "delete" + VerbDeleteCollection verb = "deletecollection" + VerbGet verb = "get" + VerbList verb = "list" + VerbWatch verb = "watch" + VerbPatch verb = "patch" + + VerbProxy verb = "proxy" // proxy unsupported for now +) + +var _, _ Verb = VerbGet, verb("") + +type verb string + +func (verb) verb() {} diff --git a/internal/kubeclient/verb_test.go b/internal/kubeclient/verb_test.go new file mode 100644 index 00000000..7b2bf61b --- /dev/null +++ b/internal/kubeclient/verb_test.go @@ -0,0 +1,54 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_verb(t *testing.T) { + tests := []struct { + name string + f func() string + want string + }{ + { + name: "error: string format", + f: func() string { + return fmt.Errorf("%s", VerbGet).Error() + }, + want: "get", + }, + { + name: "error: value format", + f: func() string { + return fmt.Errorf("%v", VerbUpdate).Error() + }, + want: "update", + }, + { + name: "error: go value format", + f: func() string { + return fmt.Errorf("%#v", VerbDelete).Error() + }, + want: `"delete"`, + }, + { + name: "error: go value format in middelware request", + f: func() string { + return fmt.Errorf("%#v", request{verb: VerbPatch}).Error() + }, + want: `kubeclient.request{verb:"patch", namespace:"", resource:schema.GroupVersionResource{Group:"", Version:"", Resource:""}, reqFuncs:[]func(kubeclient.Object)(nil), respFuncs:[]func(kubeclient.Object)(nil), subresource:""}`, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, tt.f()) + }) + } +} diff --git a/internal/kubeclient/watch.go b/internal/kubeclient/watch.go new file mode 100644 index 00000000..a1817582 --- /dev/null +++ b/internal/kubeclient/watch.go @@ -0,0 +1,163 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package kubeclient + +import ( + stderrors "errors" + "fmt" + "io" + "io/ioutil" + "net/http" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer/streaming" + "k8s.io/apimachinery/pkg/util/net" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/watch" + restclient "k8s.io/client-go/rest" + restclientwatch "k8s.io/client-go/rest/watch" + + "go.pinniped.dev/internal/plog" +) + +func handleWatchResponseNewGVK( + config *restclient.Config, + negotiatedSerializer runtime.NegotiatedSerializer, + resp *http.Response, + middlewareReq *request, + result *mutationResult, +) (*http.Response, error) { + // defer non-success cases to client-go + if resp.StatusCode != http.StatusOK { + return resp, nil + } + + var goRoutineStarted bool + defer func() { + if goRoutineStarted { + return + } + // always drain and close the body if we do not get to the point of starting our go routine + drainAndMaybeCloseBody(resp, true) + }() + + serializerInfo, err := getSerializerInfo(config, negotiatedSerializer, resp, middlewareReq) + if err != nil { + return nil, err + } + + newResp := &http.Response{} + *newResp = *resp + + newBodyReader, newBodyWriter := io.Pipe() + + newResp.Body = newBodyReader // client-go is responsible for closing this reader + + goRoutineStarted = true + go func() { + var sourceDecoder watch.Decoder + defer utilruntime.HandleCrash() + defer func() { + // the sourceDecoder will close the resp body. we want to make sure the drain the body before + // we do that + drainAndMaybeCloseBody(resp, false) + if sourceDecoder != nil { + sourceDecoder.Close() + } + }() + defer newBodyWriter.Close() + + frameReader := serializerInfo.StreamSerializer.Framer.NewFrameReader(resp.Body) + watchEventDecoder := streaming.NewDecoder(frameReader, serializerInfo.StreamSerializer.Serializer) + sourceDecoder = restclientwatch.NewDecoder(watchEventDecoder, &passthroughDecoder{}) + defer sourceDecoder.Close() + + frameWriter := serializerInfo.StreamSerializer.Framer.NewFrameWriter(newBodyWriter) + watchEventEncoder := streaming.NewEncoder(frameWriter, serializerInfo.StreamSerializer.Serializer) + + for { + ok, err := sendWatchEvent(sourceDecoder, serializerInfo.Serializer, middlewareReq, result, watchEventEncoder) + if err != nil { + if stderrors.Is(err, io.ErrClosedPipe) { + return // calling newBodyReader.Close() will send this to all newBodyWriter.Write() + } + + // CloseWithError always returns nil + // all newBodyReader.Read() will get this error + _ = newBodyWriter.CloseWithError(err) + + return + } + + if !ok { + return + } + } + }() + + return newResp, nil +} + +func sendWatchEvent(sourceDecoder watch.Decoder, s runtime.Serializer, middlewareReq *request, result *mutationResult, watchEventEncoder streaming.Encoder) (bool, error) { + // partially copied from watch.NewStreamWatcher.receive + eventType, obj, err := sourceDecoder.Decode() + if err != nil { + switch { + case stderrors.Is(err, io.EOF): + // watch closed normally + case stderrors.Is(err, io.ErrUnexpectedEOF): + plog.InfoErr("Unexpected EOF during watch stream event decoding", err) + case net.IsProbableEOF(err), net.IsTimeout(err): + plog.TraceErr("Unable to decode an event from the watch stream", err) + default: + return false, fmt.Errorf("unexpected watch decode error for %#v: %w", middlewareReq, err) + } + return false, nil // all errors end watch + } + + unknown, ok := obj.(*runtime.Unknown) + if !ok || len(unknown.Raw) == 0 { + return false, fmt.Errorf("unexpected decode type: %T", obj) + } + + respData := unknown.Raw + fixedRespData, err := maybeRestoreGVK(s, respData, result) + if err != nil { + return false, fmt.Errorf("unable to restore GVK for %#v: %w", middlewareReq, err) + } + + fixedRespData, err = maybeMutateResponse(s, fixedRespData, middlewareReq, result) + if err != nil { + return false, fmt.Errorf("unable to mutate response for %#v: %w", middlewareReq, err) + } + + event := &metav1.WatchEvent{ + Type: string(eventType), + Object: runtime.RawExtension{Raw: fixedRespData}, + } + + if err := watchEventEncoder.Encode(event); err != nil { + return false, fmt.Errorf("failed to encode watch event for %#v: %w", middlewareReq, err) + } + + return true, nil +} + +// drainAndMaybeCloseBody attempts to drain and optionallt close the provided body. +// +// We want to drain used HTTP response bodies so that the underlying TCP connection can be +// reused. However, if the underlying response body is extremely large or a never-ending stream, +// then we don't want to wait for the read to finish. In these cases, we give up on the TCP +// connection and just close the body. +func drainAndMaybeCloseBody(resp *http.Response, close bool) { + // from k8s.io/client-go/rest/request.go... + const maxBodySlurpSize = 2 << 10 + if resp.ContentLength <= maxBodySlurpSize { + _, _ = io.Copy(ioutil.Discard, &io.LimitedReader{R: resp.Body, N: maxBodySlurpSize}) + } + if close { + resp.Body.Close() + } +} diff --git a/internal/mocks/mockroundtripper/generate.go b/internal/mocks/mockroundtripper/generate.go new file mode 100644 index 00000000..0b59a953 --- /dev/null +++ b/internal/mocks/mockroundtripper/generate.go @@ -0,0 +1,6 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mockroundtripper + +//go:generate go run -v github.com/golang/mock/mockgen -destination=mockroundtripper.go -package=mockroundtripper -copyright_file=../../../hack/header.txt net/http RoundTripper diff --git a/internal/mocks/mockroundtripper/mockroundtripper.go b/internal/mocks/mockroundtripper/mockroundtripper.go new file mode 100644 index 00000000..b259aea2 --- /dev/null +++ b/internal/mocks/mockroundtripper/mockroundtripper.go @@ -0,0 +1,53 @@ +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: net/http (interfaces: RoundTripper) + +// Package mockroundtripper is a generated GoMock package. +package mockroundtripper + +import ( + gomock "github.com/golang/mock/gomock" + http "net/http" + reflect "reflect" +) + +// MockRoundTripper is a mock of RoundTripper interface +type MockRoundTripper struct { + ctrl *gomock.Controller + recorder *MockRoundTripperMockRecorder +} + +// MockRoundTripperMockRecorder is the mock recorder for MockRoundTripper +type MockRoundTripperMockRecorder struct { + mock *MockRoundTripper +} + +// NewMockRoundTripper creates a new mock instance +func NewMockRoundTripper(ctrl *gomock.Controller) *MockRoundTripper { + mock := &MockRoundTripper{ctrl: ctrl} + mock.recorder = &MockRoundTripperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockRoundTripper) EXPECT() *MockRoundTripperMockRecorder { + return m.recorder +} + +// RoundTrip mocks base method +func (m *MockRoundTripper) RoundTrip(arg0 *http.Request) (*http.Response, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RoundTrip", arg0) + ret0, _ := ret[0].(*http.Response) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RoundTrip indicates an expected call of RoundTrip +func (mr *MockRoundTripperMockRecorder) RoundTrip(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RoundTrip", reflect.TypeOf((*MockRoundTripper)(nil).RoundTrip), arg0) +} diff --git a/internal/ownerref/ownerref.go b/internal/ownerref/ownerref.go index 1afac4b7..b036d5d2 100644 --- a/internal/ownerref/ownerref.go +++ b/internal/ownerref/ownerref.go @@ -4,36 +4,68 @@ package ownerref import ( - "net/http" + "context" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/internal/kubeclient" ) -func New(ref metav1.OwnerReference) kubeclient.Middleware { - return ownerRefMiddleware(ref) -} +func New(refObj kubeclient.Object) kubeclient.Middleware { + ref := metav1.OwnerReference{ + Name: refObj.GetName(), + UID: refObj.GetUID(), + } + ref.APIVersion, ref.Kind = refObj.GetObjectKind().GroupVersionKind().ToAPIVersionAndKind() + refNamespace := refObj.GetNamespace() -var _ kubeclient.Middleware = ownerRefMiddleware(metav1.OwnerReference{}) + // if refNamespace is empty, we assume the owner ref is to a cluster scoped object which can own any object + refIsNamespaced := len(refNamespace) != 0 -type ownerRefMiddleware metav1.OwnerReference - -func (o ownerRefMiddleware) Handles(httpMethod string) bool { - return httpMethod == http.MethodPost // only handle create requests -} - -// TODO this func assumes all objects are namespace scoped and are in the same namespace. -// i.e. it assumes all objects are safe to set an owner ref on -// i.e. the owner could be namespace scoped and thus cannot own a cluster scoped object -// this could be fixed by using a rest mapper to confirm the REST scoping -// or we could always use an owner ref to a cluster scoped object -func (o ownerRefMiddleware) Mutate(obj metav1.Object) (mutated bool) { - // we only want to set the owner ref on create and when one is not already present - if len(obj.GetOwnerReferences()) != 0 { - return false + // special handling of namespaces to treat them as namespace scoped to themselves + if isNamespace(refObj) { + refNamespace = refObj.GetName() + refIsNamespaced = true } - obj.SetOwnerReferences([]metav1.OwnerReference{metav1.OwnerReference(o)}) - return true + return kubeclient.MiddlewareFunc(func(_ context.Context, rt kubeclient.RoundTrip) { + // we should not mess with owner refs on things we did not create + if rt.Verb() != kubeclient.VerbCreate { + return + } + + // we probably do not want to set an owner ref on a subresource + if len(rt.Subresource()) != 0 { + return + } + + // when ref is not cluster scoped, we ignore cluster scoped resources + if refIsNamespaced && !rt.NamespaceScoped() { + return + } + + // when ref is not cluster scoped, we require refNamespace to match + // the request namespace since cross namespace ownership is disallowed + if refIsNamespaced && refNamespace != rt.Namespace() { + return + } + + rt.MutateRequest(func(obj kubeclient.Object) { + // we only want to set the owner ref on create and when one is not already present + if len(obj.GetOwnerReferences()) != 0 { + return + } + + obj.SetOwnerReferences([]metav1.OwnerReference{ref}) + }) + }) +} + +//nolint: gochecknoglobals +var namespaceGVK = corev1.SchemeGroupVersion.WithKind("Namespace") + +func isNamespace(obj kubeclient.Object) bool { + _, ok := obj.(*corev1.Namespace) + return ok || obj.GetObjectKind().GroupVersionKind() == namespaceGVK } diff --git a/internal/ownerref/ownerref_test.go b/internal/ownerref/ownerref_test.go index 7a12efa2..0875ea7c 100644 --- a/internal/ownerref/ownerref_test.go +++ b/internal/ownerref/ownerref_test.go @@ -4,46 +4,47 @@ package ownerref import ( + "context" "net/http" "testing" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/meta" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + + "go.pinniped.dev/internal/kubeclient" + "go.pinniped.dev/internal/testutil" ) func TestOwnerReferenceMiddleware(t *testing.T) { - ref1 := metav1.OwnerReference{ - Name: "earth", - UID: "0x11", - } - ref2 := metav1.OwnerReference{ - Name: "mars", - UID: "0x12", - } - ref3 := metav1.OwnerReference{ - Name: "sun", - UID: "0x13", - } + ref1 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "earth", Namespace: "some-namespace", UID: "0x11"}} + ref2 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "mars", Namespace: "some-namespace", UID: "0x12"}} + ref3 := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "sun", Namespace: "some-namespace", UID: "0x13"}} + clusterRef := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "bananas", UID: "0x13"}} - secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers"}} - configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas"}} + secret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-namespace"}} + secretOtherNamespace := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-other-namespace"}} + configMap := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", Namespace: "some-namespace"}} + clusterRole := &rbacv1.ClusterRole{ObjectMeta: metav1.ObjectMeta{Name: "bananas"}} - secretWithOwner := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", OwnerReferences: []metav1.OwnerReference{ref3}}} - configMapWithOwner := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", OwnerReferences: []metav1.OwnerReference{ref3}}} + secretWithOwner := withOwnerRef(t, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "twizzlers", Namespace: "some-namespace"}}, ref3) + configMapWithOwner := withOwnerRef(t, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "pandas", Namespace: "some-namespace"}}, ref3) + + namespaceRef := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "solar-system", UID: "0x42"}} + secretInSameNamespaceAsNamespaceRef := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "venus", Namespace: "solar-system", UID: "0x11"}} type args struct { - ref metav1.OwnerReference - httpMethod string - obj metav1.Object + ref kubeclient.Object + httpMethod string + subresource string + obj kubeclient.Object } tests := []struct { name string args args wantHandles, wantMutates bool - wantObj metav1.Object + wantObj kubeclient.Object }{ { name: "on update", @@ -54,7 +55,36 @@ func TestOwnerReferenceMiddleware(t *testing.T) { }, wantHandles: false, wantMutates: false, - wantObj: nil, + }, + { + name: "on get", + args: args{ + ref: ref1, + httpMethod: http.MethodGet, + obj: secret.DeepCopy(), + }, + wantHandles: false, + wantMutates: false, + }, + { + name: "on delete", + args: args{ + ref: ref1, + httpMethod: http.MethodDelete, + obj: secret.DeepCopy(), + }, + wantHandles: false, + wantMutates: false, + }, + { + name: "on patch", + args: args{ + ref: ref1, + httpMethod: http.MethodPatch, + obj: secret.DeepCopy(), + }, + wantHandles: false, + wantMutates: false, }, { name: "on create", @@ -67,6 +97,17 @@ func TestOwnerReferenceMiddleware(t *testing.T) { wantMutates: true, wantObj: withOwnerRef(t, secret, ref1), }, + { + name: "on create when the ref object is a namespace", + args: args{ + ref: namespaceRef, + httpMethod: http.MethodPost, + obj: secretInSameNamespaceAsNamespaceRef.DeepCopy(), + }, + wantHandles: true, + wantMutates: true, + wantObj: withOwnerRef(t, secretInSameNamespaceAsNamespaceRef, namespaceRef), + }, { name: "on create config map", args: args{ @@ -78,27 +119,78 @@ func TestOwnerReferenceMiddleware(t *testing.T) { wantMutates: true, wantObj: withOwnerRef(t, configMap, ref2), }, + { + name: "on create with cluster-scoped owner", + args: args{ + ref: clusterRef, + httpMethod: http.MethodPost, + obj: secret.DeepCopy(), + }, + wantHandles: true, + wantMutates: true, + wantObj: withOwnerRef(t, secret, clusterRef), + }, + { + name: "on create of cluster-scoped resource with namespace-scoped owner", + args: args{ + ref: ref1, + httpMethod: http.MethodPost, + obj: clusterRole.DeepCopy(), + }, + wantHandles: false, + wantMutates: false, + }, + { + name: "on create of cluster-scoped resource with cluster-scoped owner", + args: args{ + ref: clusterRef, + httpMethod: http.MethodPost, + obj: clusterRole.DeepCopy(), + }, + wantHandles: true, + wantMutates: true, + wantObj: withOwnerRef(t, clusterRole, clusterRef), + }, { name: "on create with pre-existing ref", args: args{ ref: ref1, httpMethod: http.MethodPost, - obj: secretWithOwner.DeepCopy(), + obj: secretWithOwner.DeepCopyObject().(kubeclient.Object), }, wantHandles: true, wantMutates: false, - wantObj: nil, }, { name: "on create with pre-existing ref config map", args: args{ ref: ref2, httpMethod: http.MethodPost, - obj: configMapWithOwner.DeepCopy(), + obj: configMapWithOwner.DeepCopyObject().(kubeclient.Object), }, wantHandles: true, wantMutates: false, - wantObj: nil, + }, + { + name: "on create of subresource", + args: args{ + ref: ref2, + httpMethod: http.MethodPost, + subresource: "some-subresource", + obj: configMapWithOwner.DeepCopyObject().(kubeclient.Object), + }, + wantHandles: false, + wantMutates: false, + }, + { + name: "on create with namespace mismatch", + args: args{ + ref: ref2, + httpMethod: http.MethodPost, + obj: secretOtherNamespace.DeepCopyObject().(kubeclient.Object), + }, + wantHandles: false, + wantMutates: false, }, } for _, tt := range tests { @@ -106,37 +198,63 @@ func TestOwnerReferenceMiddleware(t *testing.T) { t.Run(tt.name, func(t *testing.T) { middleware := New(tt.args.ref) - handles := middleware.Handles(tt.args.httpMethod) - require.Equal(t, tt.wantHandles, handles) - - if !handles { + rt := (&testutil.RoundTrip{}). + WithVerb(verb(t, tt.args.httpMethod)). + WithNamespace(tt.args.obj.GetNamespace()). + WithSubresource(tt.args.subresource) + middleware.Handle(context.Background(), rt) + require.Empty(t, rt.MutateResponses, 1) + if !tt.wantHandles { + require.Empty(t, rt.MutateRequests) return } + require.Len(t, rt.MutateRequests, 1) - orig := tt.args.obj.(runtime.Object).DeepCopyObject() - - mutates := middleware.Mutate(tt.args.obj) - require.Equal(t, tt.wantMutates, mutates) - - if mutates { + orig := tt.args.obj.DeepCopyObject().(kubeclient.Object) + for _, mutateRequest := range rt.MutateRequests { + mutateRequest := mutateRequest + mutateRequest(tt.args.obj) + } + if !tt.wantMutates { + require.Equal(t, orig, tt.args.obj) + } else { require.NotEqual(t, orig, tt.args.obj) require.Equal(t, tt.wantObj, tt.args.obj) - } else { - require.Equal(t, orig, tt.args.obj) } }) } } -func withOwnerRef(t *testing.T, obj runtime.Object, ref metav1.OwnerReference) metav1.Object { +func withOwnerRef(t *testing.T, obj kubeclient.Object, ref kubeclient.Object) kubeclient.Object { t.Helper() - obj = obj.DeepCopyObject() - accessor, err := meta.Accessor(obj) - require.NoError(t, err) + ownerRef := metav1.OwnerReference{ + Name: ref.GetName(), + UID: ref.GetUID(), + } - require.Len(t, accessor.GetOwnerReferences(), 0) - accessor.SetOwnerReferences([]metav1.OwnerReference{ref}) + obj = obj.DeepCopyObject().(kubeclient.Object) + require.Len(t, obj.GetOwnerReferences(), 0) + obj.SetOwnerReferences([]metav1.OwnerReference{ownerRef}) - return accessor + return obj +} + +func verb(t *testing.T, v string) kubeclient.Verb { + t.Helper() + switch v { + case http.MethodGet: + return kubeclient.VerbGet + case http.MethodPut: + return kubeclient.VerbUpdate + case http.MethodPost: + return kubeclient.VerbCreate + case http.MethodDelete: + return kubeclient.VerbDelete + case http.MethodPatch: + return kubeclient.VerbPatch + default: + require.FailNowf(t, "unknown verb", "unknown verb: %q", v) + return kubeclient.VerbGet // shouldn't get here + } } diff --git a/internal/plog/plog.go b/internal/plog/plog.go index ffcefdb8..b38e7179 100644 --- a/internal/plog/plog.go +++ b/internal/plog/plog.go @@ -46,7 +46,7 @@ func Warning(msg string, keysAndValues ...interface{}) { // Use WarningErr to issue a Warning message with an error object as part of the message. func WarningErr(msg string, err error, keysAndValues ...interface{}) { - Warning(msg, append([]interface{}{errorKey, err}, keysAndValues)...) + Warning(msg, append([]interface{}{errorKey, err}, keysAndValues...)...) } func Info(msg string, keysAndValues ...interface{}) { @@ -55,7 +55,7 @@ func Info(msg string, keysAndValues ...interface{}) { // Use InfoErr to log an expected error, e.g. validation failure of an http parameter. func InfoErr(msg string, err error, keysAndValues ...interface{}) { - Info(msg, append([]interface{}{errorKey, err}, keysAndValues)...) + Info(msg, append([]interface{}{errorKey, err}, keysAndValues...)...) } func Debug(msg string, keysAndValues ...interface{}) { @@ -64,7 +64,7 @@ func Debug(msg string, keysAndValues ...interface{}) { // Use DebugErr to issue a Debug message with an error object as part of the message. func DebugErr(msg string, err error, keysAndValues ...interface{}) { - Debug(msg, append([]interface{}{errorKey, err}, keysAndValues)...) + Debug(msg, append([]interface{}{errorKey, err}, keysAndValues...)...) } func Trace(msg string, keysAndValues ...interface{}) { @@ -73,7 +73,7 @@ func Trace(msg string, keysAndValues ...interface{}) { // Use TraceErr to issue a Trace message with an error object as part of the message. func TraceErr(msg string, err error, keysAndValues ...interface{}) { - Trace(msg, append([]interface{}{errorKey, err}, keysAndValues)...) + Trace(msg, append([]interface{}{errorKey, err}, keysAndValues...)...) } func All(msg string, keysAndValues ...interface{}) { diff --git a/internal/testutil/doc.go b/internal/testutil/doc.go new file mode 100644 index 00000000..cc145707 --- /dev/null +++ b/internal/testutil/doc.go @@ -0,0 +1,7 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package testutil contains shared test utilities for the Pinniped project. +// +// As of right now, it is more or less a dumping ground for our test utilities. +package testutil diff --git a/internal/testutil/fakekubeapi/fakekubeapi.go b/internal/testutil/fakekubeapi/fakekubeapi.go new file mode 100644 index 00000000..e3096e5e --- /dev/null +++ b/internal/testutil/fakekubeapi/fakekubeapi.go @@ -0,0 +1,221 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package fakekubeapi contains a *very* simple httptest.Server that can be used to stand in for +// a real Kube API server in tests. +// +// Usage: +// func TestSomething(t *testing.T) { +// resources := map[string]kubeclient.Object{ +// // store preexisting resources here +// "/api/v1/namespaces/default/pods/some-pod-name": &corev1.Pod{...}, +// } +// server, restConfig := fakekubeapi.Start(t, resources) +// defer server.Close() +// client := kubeclient.New(kubeclient.WithConfig(restConfig)) +// // do stuff with client... +// } +package fakekubeapi + +import ( + "encoding/pem" + "io/ioutil" + "mime" + "net/http" + "net/http/httptest" + "path" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + kubescheme "k8s.io/client-go/kubernetes/scheme" + restclient "k8s.io/client-go/rest" + aggregatorclientscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" + + pinnipedconciergeclientsetscheme "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned/scheme" + pinnipedsupervisorclientsetscheme "go.pinniped.dev/generated/1.20/client/supervisor/clientset/versioned/scheme" + "go.pinniped.dev/internal/httputil/httperr" + "go.pinniped.dev/internal/multierror" +) + +// Unlike the standard httperr.New(), this one does not prepend error messages with any prefix. +type plainHTTPErr struct { + code int + msg string +} + +func (e plainHTTPErr) Error() string { + return e.msg +} + +func (e plainHTTPErr) Respond(w http.ResponseWriter) { + http.Error(w, e.msg, e.code) +} + +// Start starts an httptest.Server (with TLS) that pretends to be a Kube API server. +// +// The server uses the provided resources map to store API Object's. The map should be from API path +// to Object (e.g., /api/v1/namespaces/default/pods/some-pod-name => &corev1.Pod{}). +// +// Start returns an already started httptest.Server and a restclient.Config that can be used to talk +// to the server. +// +// Note! Only these following verbs are (partially) supported: create, get, update, delete. +func Start(t *testing.T, resources map[string]metav1.Object) (*httptest.Server, *restclient.Config) { + if resources == nil { + resources = make(map[string]metav1.Object) + } + + server := httptest.NewTLSServer(httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (err error) { + obj, err := decodeObj(r) + if err != nil { + return err + } + + obj, err = handleObj(r, obj, resources) + if err != nil { + return err + } + + if r.Method != http.MethodDelete && obj == nil { + return &plainHTTPErr{ + code: http.StatusNotFound, + // This is representative of a real Kube 404 message body. + msg: `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"not found","reason":"NotFound","details":{"name":"not-found","kind":"pods"},"code":404}`, + } + } + + if err := encodeObj(w, r, obj); err != nil { + return err + } + + return nil + })) + restConfig := &restclient.Config{ + Host: server.URL, + TLSClientConfig: restclient.TLSClientConfig{ + CAData: pem.EncodeToMemory(&pem.Block{Bytes: server.Certificate().Raw, Type: "CERTIFICATE"}), + }, + } + return server, restConfig +} + +func decodeObj(r *http.Request) (metav1.Object, error) { + switch r.Method { + case http.MethodPut, http.MethodPost: + default: + return nil, nil + } + + contentType := r.Header.Get("Content-Type") + if len(contentType) == 0 { + return nil, httperr.New(http.StatusUnsupportedMediaType, "empty content-type header is not allowed") + } + + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil { + return nil, httperr.Wrap(http.StatusUnsupportedMediaType, "could not parse mime type from content-type header", err) + } + + body, err := ioutil.ReadAll(r.Body) + if err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "read body", err) + } + + var obj metav1.Object + multiErr := multierror.New() + codecsThatWeUseInOurCode := []runtime.NegotiatedSerializer{ + kubescheme.Codecs, + aggregatorclientscheme.Codecs, + pinnipedconciergeclientsetscheme.Codecs, + pinnipedsupervisorclientsetscheme.Codecs, + } + for _, codec := range codecsThatWeUseInOurCode { + obj, err = tryDecodeObj(mediaType, body, codec) + if err == nil { + return obj, nil + } + multiErr.Add(err) + } + return nil, multiErr.ErrOrNil() +} + +func tryDecodeObj( + mediaType string, + body []byte, + negotiatedSerializer runtime.NegotiatedSerializer, +) (metav1.Object, error) { + serializerInfo, ok := runtime.SerializerInfoForMediaType(negotiatedSerializer.SupportedMediaTypes(), mediaType) + if !ok { + return nil, httperr.Newf(http.StatusInternalServerError, "unable to find serialier with content-type %s", mediaType) + } + + obj, err := runtime.Decode(serializerInfo.Serializer, body) + if err != nil { + return nil, httperr.Wrap(http.StatusInternalServerError, "decode obj", err) + } + + return obj.(metav1.Object), nil +} + +func handleObj(r *http.Request, obj metav1.Object, resources map[string]metav1.Object) (metav1.Object, error) { + switch r.Method { + case http.MethodGet: + obj = resources[r.URL.Path] + case http.MethodPost, http.MethodPut: + resources[path.Join(r.URL.Path, obj.GetName())] = obj + case http.MethodDelete: + if _, ok := resources[r.URL.Path]; !ok { + return nil, httperr.Newf(http.StatusNotFound, "no resource with path %q", r.URL.Path) + } + delete(resources, r.URL.Path) + default: + return nil, httperr.New(http.StatusMethodNotAllowed, "check source code for methods supported") + } + + return obj, nil +} + +func encodeObj(w http.ResponseWriter, r *http.Request, obj metav1.Object) error { + if r.Method == http.MethodDelete { + return nil + } + + accepts := strings.Split(r.Header.Get("Accept"), ",") + contentType := findGoodContentType(accepts) + if len(contentType) == 0 { + return httperr.Newf(http.StatusUnsupportedMediaType, "can't find good content type in %s", accepts) + } + + mediaType, _, err := mime.ParseMediaType(contentType) + if err != nil { + return httperr.Wrap(http.StatusUnsupportedMediaType, "could not parse mime type from accept header", err) + } + + serializerInfo, ok := runtime.SerializerInfoForMediaType(kubescheme.Codecs.SupportedMediaTypes(), mediaType) + if !ok { + return httperr.Newf(http.StatusInternalServerError, "unable to find serialier with content-type %s", mediaType) + } + + data, err := runtime.Encode(serializerInfo.Serializer, obj.(runtime.Object)) + if err != nil { + return httperr.Wrap(http.StatusInternalServerError, "decode obj", err) + } + + w.Header().Set("Content-Type", contentType) + if _, err := w.Write(data); err != nil { + return httperr.Wrap(http.StatusInternalServerError, "write response", err) + } + + return nil +} + +func findGoodContentType(contentTypes []string) string { + for _, contentType := range contentTypes { + if strings.Contains(contentType, "json") || strings.Contains(contentType, "yaml") || strings.Contains(contentType, "protobuf") { + return contentType + } + } + return "" +} diff --git a/internal/testutil/round_trip.go b/internal/testutil/round_trip.go new file mode 100644 index 00000000..9b12b0e6 --- /dev/null +++ b/internal/testutil/round_trip.go @@ -0,0 +1,70 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package testutil + +import ( + "k8s.io/apimachinery/pkg/runtime/schema" + + "go.pinniped.dev/internal/kubeclient" +) + +// RoundTrip is an implementation of kubeclient.RoundTrip that is easy to use in tests. +type RoundTrip struct { + verb kubeclient.Verb + namespace string + namespaceScoped bool + resource schema.GroupVersionResource + subresource string + + MutateRequests, MutateResponses []func(kubeclient.Object) +} + +func (rt *RoundTrip) WithVerb(verb kubeclient.Verb) *RoundTrip { + rt.verb = verb + return rt +} + +func (rt *RoundTrip) Verb() kubeclient.Verb { + return rt.verb +} + +func (rt *RoundTrip) WithNamespace(namespace string) *RoundTrip { + rt.namespace = namespace + rt.namespaceScoped = len(namespace) != 0 + return rt +} + +func (rt *RoundTrip) Namespace() string { + return rt.namespace +} + +func (rt *RoundTrip) NamespaceScoped() bool { + return rt.namespaceScoped +} + +func (rt *RoundTrip) WithResource(resource schema.GroupVersionResource) *RoundTrip { + rt.resource = resource + return rt +} + +func (rt *RoundTrip) Resource() schema.GroupVersionResource { + return rt.resource +} + +func (rt *RoundTrip) WithSubresource(subresource string) *RoundTrip { + rt.subresource = subresource + return rt +} + +func (rt *RoundTrip) Subresource() string { + return rt.subresource +} + +func (rt *RoundTrip) MutateRequest(fn func(kubeclient.Object)) { + rt.MutateRequests = append(rt.MutateRequests, fn) +} + +func (rt *RoundTrip) MutateResponse(fn func(kubeclient.Object)) { + rt.MutateResponses = append(rt.MutateResponses, fn) +} diff --git a/pkg/conciergeclient/conciergeclient.go b/pkg/conciergeclient/conciergeclient.go index b8163437..0bebd3d2 100644 --- a/pkg/conciergeclient/conciergeclient.go +++ b/pkg/conciergeclient/conciergeclient.go @@ -22,6 +22,7 @@ import ( loginv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/login/v1alpha1" conciergeclientset "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" ) @@ -116,8 +117,8 @@ func WithEndpoint(endpoint string) Option { // WithAPIGroupSuffix configures the concierge's API group suffix (e.g., "pinniped.dev"). func WithAPIGroupSuffix(apiGroupSuffix string) Option { return func(c *Client) error { - if apiGroupSuffix == "" { - return fmt.Errorf("api group suffix must not be empty") + if err := groupsuffix.Validate(apiGroupSuffix); err != nil { + return fmt.Errorf("invalid api group suffix: %w", err) } c.apiGroupSuffix = apiGroupSuffix return nil @@ -163,8 +164,10 @@ func (c *Client) clientset() (conciergeclientset.Interface, error) { if err != nil { return nil, err } - _ = c.apiGroupSuffix // TODO: wire API group into kubeclient. - client, err := kubeclient.New(kubeclient.WithConfig(cfg)) + client, err := kubeclient.New( + kubeclient.WithConfig(cfg), + kubeclient.WithMiddleware(groupsuffix.New(c.apiGroupSuffix)), + ) if err != nil { return nil, err } diff --git a/pkg/conciergeclient/conciergeclient_test.go b/pkg/conciergeclient/conciergeclient_test.go index 79560c71..937db5f0 100644 --- a/pkg/conciergeclient/conciergeclient_test.go +++ b/pkg/conciergeclient/conciergeclient_test.go @@ -111,7 +111,16 @@ func TestNew(t *testing.T) { WithEndpoint("https://example.com"), WithAPIGroupSuffix(""), }, - wantErr: "api group suffix must not be empty", + wantErr: "invalid api group suffix: 2 error(s):\n- must contain '.'\n- 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]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", + }, + { + name: "invalid api group suffix", + opts: []Option{ + WithAuthenticator("jwt", "test-authenticator"), + WithEndpoint("https://example.com"), + WithAPIGroupSuffix(".starts.with.dot"), + }, + wantErr: "invalid api group suffix: 1 error(s):\n- 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]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", }, { name: "valid", diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index cc8a4ac0..d31b817d 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -57,6 +57,7 @@ func TestCLIGetKubeconfigStaticToken(t *testing.T) { "--pinniped-namespace", env.ConciergeNamespace, "--authenticator-type", "webhook", "--authenticator-name", authenticator.Name, + "--api-group-suffix", env.APIGroupSuffix, }, expectStderr: "Command \"get-kubeconfig\" is deprecated, Please use `pinniped get kubeconfig` instead.\n", }, @@ -84,16 +85,15 @@ func TestCLIGetKubeconfigStaticToken(t *testing.T) { // In addition to the client-go based testing below, also try the kubeconfig // with kubectl to validate that it works. - adminClient := library.NewClientset(t) t.Run( "access as user with kubectl", - library.AccessAsUserWithKubectlTest(ctx, adminClient, stdout, env.TestUser.ExpectedUsername, env.ConciergeNamespace), + library.AccessAsUserWithKubectlTest(stdout, env.TestUser.ExpectedUsername, env.ConciergeNamespace), ) for _, group := range env.TestUser.ExpectedGroups { group := group t.Run( "access as group "+group+" with kubectl", - library.AccessAsGroupWithKubectlTest(ctx, adminClient, stdout, group, env.ConciergeNamespace), + library.AccessAsGroupWithKubectlTest(stdout, group, env.ConciergeNamespace), ) } @@ -101,10 +101,10 @@ func TestCLIGetKubeconfigStaticToken(t *testing.T) { kubeClient := library.NewClientsetForKubeConfig(t, stdout) // Validate that we can auth to the API via our user. - t.Run("access as user with client-go", library.AccessAsUserTest(ctx, adminClient, env.TestUser.ExpectedUsername, kubeClient)) + t.Run("access as user with client-go", library.AccessAsUserTest(ctx, env.TestUser.ExpectedUsername, kubeClient)) for _, group := range env.TestUser.ExpectedGroups { group := group - t.Run("access as group "+group+" with client-go", library.AccessAsGroupTest(ctx, adminClient, group, kubeClient)) + t.Run("access as group "+group+" with client-go", library.AccessAsGroupTest(ctx, group, kubeClient)) } }) } diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index e2138457..c7885fb1 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -73,7 +73,7 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - kubeClient := library.NewClientset(t) + kubeClient := library.NewKubernetesClientset(t) aggregatedClient := library.NewAggregatedClientset(t) conciergeClient := library.NewConciergeClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) diff --git a/test/integration/concierge_availability_test.go b/test/integration/concierge_availability_test.go index a4705513..575e4fc2 100644 --- a/test/integration/concierge_availability_test.go +++ b/test/integration/concierge_availability_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -18,7 +18,7 @@ import ( func TestGetDeployment(t *testing.T) { env := library.IntegrationEnv(t) - client := library.NewClientset(t) + client := library.NewKubernetesClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index cb673dcf..8b1b6c45 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -104,9 +104,6 @@ func TestSuccessfulCredentialRequest(t *testing.T) { require.NotNil(t, response.Status.Credential.ExpirationTimestamp) require.InDelta(t, 5*time.Minute, time.Until(response.Status.Credential.ExpirationTimestamp.Time), float64(time.Minute)) - // Create a client using the admin kubeconfig. - adminClient := library.NewClientset(t) - // Create a client using the certificate from the CredentialRequest. clientWithCertFromCredentialRequest := library.NewClientsetWithCertAndKey( t, @@ -116,13 +113,13 @@ func TestSuccessfulCredentialRequest(t *testing.T) { t.Run( "access as user", - library.AccessAsUserTest(ctx, adminClient, username, clientWithCertFromCredentialRequest), + library.AccessAsUserTest(ctx, username, clientWithCertFromCredentialRequest), ) for _, group := range groups { group := group t.Run( "access as group "+group, - library.AccessAsGroupTest(ctx, adminClient, group, clientWithCertFromCredentialRequest), + library.AccessAsGroupTest(ctx, group, clientWithCertFromCredentialRequest), ) } }) diff --git a/test/integration/concierge_kubecertagent_test.go b/test/integration/concierge_kubecertagent_test.go index 7066dfae..8d054dce 100644 --- a/test/integration/concierge_kubecertagent_test.go +++ b/test/integration/concierge_kubecertagent_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -30,7 +30,7 @@ func TestKubeCertAgent(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) defer cancel() - kubeClient := library.NewClientset(t) + kubeClient := library.NewKubernetesClientset(t) // Get the current number of kube-cert-agent pods. // diff --git a/test/integration/kube_api_discovery_test.go b/test/integration/kube_api_discovery_test.go index ed82226d..4ab3bfaa 100644 --- a/test/integration/kube_api_discovery_test.go +++ b/test/integration/kube_api_discovery_test.go @@ -4,6 +4,7 @@ package integration import ( + "errors" "fmt" "strings" "testing" @@ -12,6 +13,7 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" "go.pinniped.dev/test/library" ) @@ -19,9 +21,21 @@ import ( func TestGetAPIResourceList(t *testing.T) { env := library.IntegrationEnv(t) - client := library.NewClientset(t) + client := library.NewKubernetesClientset(t) groups, resources, err := client.Discovery().ServerGroupsAndResources() - require.NoError(t, err) + + // discovery can have partial failures when an API service is unavailable (i.e. because of TestAPIServingCertificateAutoCreationAndRotation) + // we ignore failures for groups that are not relevant to this test + if err != nil { + discoveryFailed := &discovery.ErrGroupDiscoveryFailed{} + isDiscoveryFailed := errors.As(err, &discoveryFailed) + require.True(t, isDiscoveryFailed, err) + for gv, gvErr := range discoveryFailed.Groups { + if strings.HasSuffix(gv.Group, "."+env.APIGroupSuffix) { + require.NoError(t, gvErr) + } + } + } makeGV := func(firstSegment, secondSegment string) schema.GroupVersion { return schema.GroupVersion{ @@ -195,12 +209,15 @@ func TestGetAPIResourceList(t *testing.T) { for _, tt := range tests { testedGroups[tt.group.Name] = true } + foundPinnipedGroups := 0 for _, g := range groups { if !strings.Contains(g.Name, env.APIGroupSuffix) { continue } + foundPinnipedGroups++ assert.Truef(t, testedGroups[g.Name], "expected group %q to have assertions defined", g.Name) } + require.Equal(t, len(testedGroups), foundPinnipedGroups) }) t.Run("every API categorized appropriately", func(t *testing.T) { diff --git a/test/integration/kubeclient_test.go b/test/integration/kubeclient_test.go index f225df36..211287f1 100644 --- a/test/integration/kubeclient_test.go +++ b/test/integration/kubeclient_test.go @@ -17,6 +17,7 @@ import ( conciergeconfigv1alpha1 "go.pinniped.dev/generated/1.20/apis/concierge/config/v1alpha1" supervisorconfigv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/ownerref" "go.pinniped.dev/test/library" @@ -25,7 +26,7 @@ import ( func TestKubeClientOwnerRef(t *testing.T) { env := library.IntegrationEnv(t) - regularClient := library.NewClientset(t) + regularClient := library.NewKubernetesClientset(t) ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() @@ -62,16 +63,20 @@ func TestKubeClientOwnerRef(t *testing.T) { require.NoError(t, err) require.Len(t, parentSecret.OwnerReferences, 0) - // create a client that should set an owner ref back to parent on create + // work around stupid behavior of WithoutVersionDecoder.Decode + parentSecret.APIVersion, parentSecret.Kind = corev1.SchemeGroupVersion.WithKind("Secret").ToAPIVersionAndKind() + ref := metav1.OwnerReference{ - APIVersion: "v1", - Kind: "Secret", + APIVersion: parentSecret.APIVersion, + Kind: parentSecret.Kind, Name: parentSecret.Name, UID: parentSecret.UID, } - _ = env.APIGroupSuffix // TODO: wire API group into kubeclient. + + // create a client that should set an owner ref back to parent on create ownerRefClient, err := kubeclient.New( - kubeclient.WithMiddleware(ownerref.New(ref)), + kubeclient.WithMiddleware(ownerref.New(parentSecret)), + kubeclient.WithMiddleware(groupsuffix.New(env.APIGroupSuffix)), kubeclient.WithConfig(library.NewClientConfig(t)), ) require.NoError(t, err) @@ -137,44 +142,29 @@ func TestKubeClientOwnerRef(t *testing.T) { return err }) - // TODO: update middleware code to not set owner references on cluster-scoped objects. - // - // The Kube 1.20 garbage collector asserts some new behavior in regards to invalid owner - // references (i.e., when you have a namespace-scoped owner references for a cluster-scoped - // dependent, the cluster-scoped dependent is not removed). We also found a bug in the 1.20 - // garbage collector where namespace-scoped dependents are not garbage collected if their owner - // had been used as an invalid owner reference before - this bug causes our test to fallover - // because we are setting a namespace-scoped owner ref on this APIService. - // - // We believe that the best way to get around this problem is to update our kubeclient code to - // never set owner references on cluster-scoped objects. After we do that, we will uncomment this - // part of the test. - if false { - // sanity check API service client - apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create( - ctx, - &apiregistrationv1.APIService{ - ObjectMeta: metav1.ObjectMeta{ - Name: "v1.pandas.dev", - OwnerReferences: nil, // no owner refs set - }, - Spec: apiregistrationv1.APIServiceSpec{ - Version: "v1", - Group: "pandas.dev", - GroupPriorityMinimum: 10_000, - VersionPriority: 500, - }, + // sanity check API service client - the middleware code shouldn't add an owner reference to this + // APIService because the APIService is cluster-scoped and the parent object is namespace-scoped, + // which is invalid in Kubernetes + apiService, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Create( + ctx, + &apiregistrationv1.APIService{ + ObjectMeta: metav1.ObjectMeta{ + Name: "v1.pandas.dev", + OwnerReferences: nil, // no owner refs set }, - metav1.CreateOptions{}, - ) - require.NoError(t, err) - hasOwnerRef(t, apiService, ref) - // this owner ref is invalid for an API service so it should be immediately deleted - isEventuallyDeleted(t, func() error { - _, err := ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Get(ctx, apiService.Name, metav1.GetOptions{}) - return err - }) - } + Spec: apiregistrationv1.APIServiceSpec{ + Version: "v1", + Group: "pandas.dev", + GroupPriorityMinimum: 10_000, + VersionPriority: 500, + }, + }, + metav1.CreateOptions{}, + ) + require.NoError(t, err) + hasNoOwnerRef(t, apiService) + err = ownerRefClient.Aggregation.ApiregistrationV1().APIServices().Delete(ctx, apiService.Name, metav1.DeleteOptions{}) + require.NoError(t, err) // sanity check concierge client credentialIssuer, err := ownerRefClient.PinnipedConcierge.ConfigV1alpha1().CredentialIssuers(namespace.Name).Create( @@ -256,6 +246,13 @@ func hasOwnerRef(t *testing.T, obj metav1.Object, ref metav1.OwnerReference) { require.Equal(t, ref, ownerReferences[0]) } +func hasNoOwnerRef(t *testing.T, obj metav1.Object) { + t.Helper() + + ownerReferences := obj.GetOwnerReferences() + require.Len(t, ownerReferences, 0) +} + func isEventuallyDeleted(t *testing.T, f func() error) { t.Helper() diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index fef641b7..dc2bf971 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -48,7 +48,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) defer cancel() - temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, library.NewClientset(t)) + temporarilyRemoveAllFederationDomainsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, library.NewKubernetesClientset(t)) tests := []struct { Scheme string @@ -146,7 +146,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { func TestSupervisorTLSTerminationWithSNI(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewSupervisorClientset(t) - kubeClient := library.NewClientset(t) + kubeClient := library.NewKubernetesClientset(t) ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) @@ -213,7 +213,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewSupervisorClientset(t) - kubeClient := library.NewClientset(t) + kubeClient := library.NewKubernetesClientset(t) ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go index 797c928a..ebd36693 100644 --- a/test/integration/supervisor_secrets_test.go +++ b/test/integration/supervisor_secrets_test.go @@ -21,7 +21,7 @@ import ( func TestSupervisorSecrets(t *testing.T) { env := library.IntegrationEnv(t) - kubeClient := library.NewClientset(t) + kubeClient := library.NewKubernetesClientset(t) supervisorClient := library.NewSupervisorClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) diff --git a/test/integration/supervisor_storage_garbage_collection_test.go b/test/integration/supervisor_storage_garbage_collection_test.go index e7efa159..c89a66e4 100644 --- a/test/integration/supervisor_storage_garbage_collection_test.go +++ b/test/integration/supervisor_storage_garbage_collection_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -26,7 +26,7 @@ func TestStorageGarbageCollection(t *testing.T) { t.Parallel() env := library.IntegrationEnv(t) - client := library.NewClientset(t) + client := library.NewKubernetesClientset(t) secrets := client.CoreV1().Secrets(env.SupervisorNamespace) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() diff --git a/test/integration/supervisor_storage_test.go b/test/integration/supervisor_storage_test.go index f551d2cd..51d0f70e 100644 --- a/test/integration/supervisor_storage_test.go +++ b/test/integration/supervisor_storage_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -25,7 +25,7 @@ import ( func TestAuthorizeCodeStorage(t *testing.T) { env := library.IntegrationEnv(t) - client := library.NewClientset(t) + client := library.NewKubernetesClientset(t) const ( // randomly generated HMAC authorization code (see below) diff --git a/test/library/access.go b/test/library/access.go index ee96e7c6..7aacf6b1 100644 --- a/test/library/access.go +++ b/test/library/access.go @@ -6,7 +6,6 @@ package library import ( "context" "io/ioutil" - "net/http" "os" "os/exec" "testing" @@ -16,7 +15,6 @@ import ( "github.com/stretchr/testify/require" v1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" ) @@ -33,12 +31,11 @@ const ( // performing a Pinniped credential exchange. func AccessAsUserTest( ctx context.Context, - adminClient kubernetes.Interface, testUsername string, clientUnderTest kubernetes.Interface, ) func(t *testing.T) { return func(t *testing.T) { - addTestClusterUserCanViewEverythingRoleBinding(ctx, t, adminClient, testUsername) + addTestClusterUserCanViewEverythingRoleBinding(t, testUsername) // Use the client which is authenticated as the test user to list namespaces var listNamespaceResponse *v1.NamespaceList @@ -55,14 +52,12 @@ func AccessAsUserTest( } func AccessAsUserWithKubectlTest( - ctx context.Context, - adminClient kubernetes.Interface, testKubeConfigYAML string, testUsername string, expectedNamespace string, ) func(t *testing.T) { return func(t *testing.T) { - addTestClusterUserCanViewEverythingRoleBinding(ctx, t, adminClient, testUsername) + addTestClusterUserCanViewEverythingRoleBinding(t, testUsername) // Use the given kubeconfig with kubectl to list namespaces as the test user var kubectlCommandOutput string @@ -85,12 +80,11 @@ func AccessAsUserWithKubectlTest( // a group membership) after performing a Pinniped credential exchange. func AccessAsGroupTest( ctx context.Context, - adminClient kubernetes.Interface, testGroup string, clientUnderTest kubernetes.Interface, ) func(t *testing.T) { return func(t *testing.T) { - addTestClusterGroupCanViewEverythingRoleBinding(ctx, t, adminClient, testGroup) + addTestClusterGroupCanViewEverythingRoleBinding(t, testGroup) // Use the client which is authenticated as the test user to list namespaces var listNamespaceResponse *v1.NamespaceList @@ -107,14 +101,12 @@ func AccessAsGroupTest( } func AccessAsGroupWithKubectlTest( - ctx context.Context, - adminClient kubernetes.Interface, testKubeConfigYAML string, testGroup string, expectedNamespace string, ) func(t *testing.T) { return func(t *testing.T) { - addTestClusterGroupCanViewEverythingRoleBinding(ctx, t, adminClient, testGroup) + addTestClusterGroupCanViewEverythingRoleBinding(t, testGroup) // Use the given kubeconfig with kubectl to list namespaces as the test user var kubectlCommandOutput string @@ -130,67 +122,38 @@ func AccessAsGroupWithKubectlTest( } } -func addTestClusterUserCanViewEverythingRoleBinding(ctx context.Context, t *testing.T, adminClient kubernetes.Interface, testUsername string) { +func addTestClusterUserCanViewEverythingRoleBinding(t *testing.T, testUsername string) { t.Helper() - addTestClusterRoleBinding(ctx, t, adminClient, &rbacv1.ClusterRoleBinding{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Name: "integration-test-user-readonly-role-binding", - }, - Subjects: []rbacv1.Subject{{ + CreateTestClusterRoleBinding(t, + rbacv1.Subject{ Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: testUsername, - }}, - RoleRef: rbacv1.RoleRef{ + }, + rbacv1.RoleRef{ Kind: "ClusterRole", APIGroup: rbacv1.GroupName, Name: "view", }, - }) + ) } -func addTestClusterGroupCanViewEverythingRoleBinding(ctx context.Context, t *testing.T, adminClient kubernetes.Interface, testGroup string) { +func addTestClusterGroupCanViewEverythingRoleBinding(t *testing.T, testGroup string) { t.Helper() - addTestClusterRoleBinding(ctx, t, adminClient, &rbacv1.ClusterRoleBinding{ - TypeMeta: metav1.TypeMeta{}, - ObjectMeta: metav1.ObjectMeta{ - Name: "integration-test-group-readonly-role-binding", - }, - Subjects: []rbacv1.Subject{{ + CreateTestClusterRoleBinding(t, + rbacv1.Subject{ Kind: rbacv1.GroupKind, APIGroup: rbacv1.GroupName, Name: testGroup, - }}, - RoleRef: rbacv1.RoleRef{ + }, + rbacv1.RoleRef{ Kind: "ClusterRole", APIGroup: rbacv1.GroupName, Name: "view", }, - }) -} - -func addTestClusterRoleBinding(ctx context.Context, t *testing.T, adminClient kubernetes.Interface, binding *rbacv1.ClusterRoleBinding) { - t.Helper() - - _, err := adminClient.RbacV1().ClusterRoleBindings().Get(ctx, binding.Name, metav1.GetOptions{}) - if err != nil { - statusError, isStatus := err.(*errors.StatusError) - require.True(t, isStatus, "Only StatusNotFound error would be acceptable, but error was: ", err.Error()) - require.Equal(t, http.StatusNotFound, int(statusError.Status().Code)) - - _, err = adminClient.RbacV1().ClusterRoleBindings().Create(ctx, binding, metav1.CreateOptions{}) - require.NoError(t, err) - } - - t.Cleanup(func() { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - err = adminClient.RbacV1().ClusterRoleBindings().Delete(ctx, binding.Name, metav1.DeleteOptions{}) - require.NoError(t, err, "Test failed to clean up after itself") - }) + ) } func runKubectlGetNamespaces(t *testing.T, kubeConfigYAML string) (string, error) { diff --git a/test/library/client.go b/test/library/client.go index d432f919..011f2b77 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -31,6 +31,7 @@ import ( idpv1alpha1 "go.pinniped.dev/generated/1.20/apis/supervisor/idp/v1alpha1" conciergeclientset "go.pinniped.dev/generated/1.20/client/concierge/clientset/versioned" supervisorclientset "go.pinniped.dev/generated/1.20/client/supervisor/clientset/versioned" + "go.pinniped.dev/internal/groupsuffix" "go.pinniped.dev/internal/kubeclient" // Import to initialize client auth plugins - the kubeconfig that we use for @@ -44,12 +45,6 @@ func NewClientConfig(t *testing.T) *rest.Config { return newClientConfigWithOverrides(t, &clientcmd.ConfigOverrides{}) } -func NewClientset(t *testing.T) kubernetes.Interface { - t.Helper() - - return newClientsetWithConfig(t, NewClientConfig(t)) -} - func NewClientsetForKubeConfig(t *testing.T, kubeConfig string) kubernetes.Interface { t.Helper() return newClientsetWithConfig(t, NewRestConfigFromKubeconfig(t, kubeConfig)) @@ -74,6 +69,12 @@ func NewClientsetWithCertAndKey(t *testing.T, clientCertificateData, clientKeyDa return newClientsetWithConfig(t, newAnonymousClientRestConfigWithCertAndKeyAdded(t, clientCertificateData, clientKeyData)) } +func NewKubernetesClientset(t *testing.T) kubernetes.Interface { + t.Helper() + + return newKubeclient(t, NewClientConfig(t)).Kubernetes +} + func NewSupervisorClientset(t *testing.T) supervisorclientset.Interface { t.Helper() @@ -135,8 +136,11 @@ func newAnonymousClientRestConfigWithCertAndKeyAdded(t *testing.T, clientCertifi func newKubeclient(t *testing.T, config *rest.Config) *kubeclient.Client { t.Helper() - _ = IntegrationEnv(t).APIGroupSuffix // TODO: wire API group into kubeclient. - client, err := kubeclient.New(kubeclient.WithConfig(config)) + env := IntegrationEnv(t) + client, err := kubeclient.New( + kubeclient.WithConfig(config), + kubeclient.WithMiddleware(groupsuffix.New(env.APIGroupSuffix)), + ) require.NoError(t, err) return client } @@ -163,6 +167,12 @@ func CreateTestWebhookAuthenticator(ctx context.Context, t *testing.T) corev1.Ty t.Cleanup(func() { t.Helper() + + if t.Failed() { + t.Logf("skipping deletion of test WebhookAuthenticator %s/%s", webhook.Namespace, webhook.Name) + return + } + t.Logf("cleaning up test WebhookAuthenticator %s/%s", webhook.Namespace, webhook.Name) deleteCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -227,6 +237,12 @@ func CreateTestJWTAuthenticator(ctx context.Context, t *testing.T, spec auth1alp t.Cleanup(func() { t.Helper() + + if t.Failed() { + t.Logf("skipping deletion of test JWTAuthenticator %s/%s", jwtAuthenticator.Namespace, jwtAuthenticator.Name) + return + } + t.Logf("cleaning up test JWTAuthenticator %s/%s", jwtAuthenticator.Namespace, jwtAuthenticator.Name) deleteCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() @@ -325,7 +341,7 @@ func RandHex(t *testing.T, numBytes int) string { func CreateTestSecret(t *testing.T, namespace string, baseName string, secretType corev1.SecretType, stringData map[string]string) *corev1.Secret { t.Helper() - client := NewClientset(t) + client := NewKubernetesClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() @@ -396,7 +412,7 @@ func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityP func CreateTestClusterRoleBinding(t *testing.T, subject rbacv1.Subject, roleRef rbacv1.RoleRef) *rbacv1.ClusterRoleBinding { t.Helper() - client := NewClientset(t) + client := NewKubernetesClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() diff --git a/test/library/dumplogs.go b/test/library/dumplogs.go index dc727e03..dca0d276 100644 --- a/test/library/dumplogs.go +++ b/test/library/dumplogs.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package library @@ -21,7 +21,7 @@ func DumpLogs(t *testing.T, namespace string, labelSelector string) { return } - kubeClient := NewClientset(t) + kubeClient := NewKubernetesClientset(t) ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) defer cancel() From 62c117421ab042a686fd72e419bf272c9265b814 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 3 Feb 2021 08:19:34 -0500 Subject: [PATCH 4/4] internal/kubeclient: fix not found test and request body closing bug - I realized that the hardcoded fakekubeapi 404 not found response was invalid, so we were getting a default error message. I fixed it so the tests follow a higher fidelity code path. - I caved and added a test for making sure the request body was always closed, and believe it or not, we were double closing a body. I don't *think* this will matter in production, since client-go will pass us ioutil.NopReader()'s, but at least we know now. Signed-off-by: Andrew Keesler --- internal/kubeclient/kubeclient.go | 8 +-- internal/kubeclient/kubeclient_test.go | 70 +++++++++++++++----- internal/kubeclient/option.go | 19 +++++- internal/kubeclient/roundtrip.go | 13 ++-- internal/testutil/fakekubeapi/fakekubeapi.go | 56 +++++++--------- 5 files changed, 104 insertions(+), 62 deletions(-) diff --git a/internal/kubeclient/kubeclient.go b/internal/kubeclient/kubeclient.go index 5a04e1f8..ebf771ab 100644 --- a/internal/kubeclient/kubeclient.go +++ b/internal/kubeclient/kubeclient.go @@ -51,13 +51,13 @@ func New(opts ...Option) (*Client, error) { protoKubeConfig := createProtoKubeConfig(c.config) // Connect to the core Kubernetes API. - k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Scheme, kubescheme.Codecs, c.middlewares)) + k8sClient, err := kubernetes.NewForConfig(configWithWrapper(protoKubeConfig, kubescheme.Scheme, kubescheme.Codecs, c.middlewares, c.transportWrapper)) if err != nil { return nil, fmt.Errorf("could not initialize Kubernetes client: %w", err) } // Connect to the Kubernetes aggregation API. - aggregatorClient, err := aggregatorclient.NewForConfig(configWithWrapper(protoKubeConfig, aggregatorclientscheme.Scheme, aggregatorclientscheme.Codecs, c.middlewares)) + aggregatorClient, err := aggregatorclient.NewForConfig(configWithWrapper(protoKubeConfig, aggregatorclientscheme.Scheme, aggregatorclientscheme.Codecs, c.middlewares, c.transportWrapper)) if err != nil { return nil, fmt.Errorf("could not initialize aggregation client: %w", err) } @@ -65,7 +65,7 @@ func New(opts ...Option) (*Client, error) { // Connect to the pinniped concierge API. // We cannot use protobuf encoding here because we are using CRDs // (for which protobuf encoding is not yet supported). - pinnipedConciergeClient, err := pinnipedconciergeclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedconciergeclientsetscheme.Scheme, pinnipedconciergeclientsetscheme.Codecs, c.middlewares)) + pinnipedConciergeClient, err := pinnipedconciergeclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedconciergeclientsetscheme.Scheme, pinnipedconciergeclientsetscheme.Codecs, c.middlewares, c.transportWrapper)) if err != nil { return nil, fmt.Errorf("could not initialize pinniped client: %w", err) } @@ -73,7 +73,7 @@ func New(opts ...Option) (*Client, error) { // Connect to the pinniped supervisor API. // We cannot use protobuf encoding here because we are using CRDs // (for which protobuf encoding is not yet supported). - pinnipedSupervisorClient, err := pinnipedsupervisorclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedsupervisorclientsetscheme.Scheme, pinnipedsupervisorclientsetscheme.Codecs, c.middlewares)) + pinnipedSupervisorClient, err := pinnipedsupervisorclientset.NewForConfig(configWithWrapper(jsonKubeConfig, pinnipedsupervisorclientsetscheme.Scheme, pinnipedsupervisorclientsetscheme.Codecs, c.middlewares, c.transportWrapper)) if err != nil { return nil, fmt.Errorf("could not initialize pinniped client: %w", err) } diff --git a/internal/kubeclient/kubeclient_test.go b/internal/kubeclient/kubeclient_test.go index a5269910..2bb4aaee 100644 --- a/internal/kubeclient/kubeclient_test.go +++ b/internal/kubeclient/kubeclient_test.go @@ -66,12 +66,6 @@ var ( middlewareLabels = map[string]string{"some-label": "thing 2"} ) -// TestKubeclient tests a subset of kubeclient functionality (from the public interface down). We -// intend for the following list of things to be tested with the integration tests: -// list (running in every informer cache) -// watch (running in every informer cache) -// discovery -// api errors func TestKubeclient(t *testing.T) { // plog.ValidateAndSetLogLevelGlobally(plog.LevelDebug) // uncomment me to get some more debug logs @@ -109,7 +103,7 @@ func TestKubeclient(t *testing.T) { CoreV1(). Pods(pod.Namespace). Get(context.Background(), "this-pod-does-not-exist", metav1.GetOptions{}) - require.EqualError(t, err, "the server could not find the requested resource (get pods this-pod-does-not-exist)") + require.EqualError(t, err, `couldn't find object for path "/api/v1/namespaces/good-namespace/pods/this-pod-does-not-exist"`) // update goodPodWithAnnotationsAndLabelsAndClusterName := with(goodPod, annotations(), labels(), clusterName()).(*corev1.Pod) @@ -546,16 +540,15 @@ func TestKubeclient(t *testing.T) { test.editRestConfig(t, restConfig) } - // our rt chain is: - // kubeclient -> wantCloseResp -> http.DefaultTransport -> wantCloseResp -> kubeclient - restConfig.Wrap(wantCloseRespWrapper(t)) - var middlewares []*spyMiddleware if test.middlewares != nil { middlewares = test.middlewares(t) } - opts := []Option{WithConfig(restConfig)} + // our rt chain is: + // wantCloseReq -> kubeclient -> wantCloseResp -> http.DefaultTransport -> wantCloseResp -> kubeclient -> wantCloseReq + restConfig.Wrap(wantCloseRespWrapper(t)) + opts := []Option{WithConfig(restConfig), WithTransportWrapper(wantCloseReqWrapper(t))} for _, middleware := range middlewares { opts = append(opts, WithMiddleware(middleware)) } @@ -675,11 +668,13 @@ func newSimpleMiddleware(t *testing.T, hasMutateReqFunc, mutatedReq, hasMutateRe type wantCloser struct { io.ReadCloser closeCount int + closeCalls []string couldReadBytesJustBeforeClosing bool } func (wc *wantCloser) Close() error { wc.closeCount++ + wc.closeCalls = append(wc.closeCalls, getCaller()) n, _ := wc.ReadCloser.Read([]byte{0}) if n > 0 { // there were still bytes left to be read @@ -688,14 +683,53 @@ func (wc *wantCloser) Close() error { return wc.ReadCloser.Close() } -// wantCloseRespWrapper returns a transport.WrapperFunc that validates that the http.Response -// returned by the underlying http.RoundTripper is closed properly. -func wantCloseRespWrapper(t *testing.T) transport.WrapperFunc { - _, file, line, ok := runtime.Caller(1) +func getCaller() string { + _, file, line, ok := runtime.Caller(2) if !ok { file = "???" line = 0 } + return fmt.Sprintf("%s:%d", file, line) +} + +// wantCloseReqWrapper returns a transport.WrapperFunc that validates that the http.Request +// passed to the underlying http.RoundTripper is closed properly. +func wantCloseReqWrapper(t *testing.T) transport.WrapperFunc { + caller := getCaller() + return func(rt http.RoundTripper) http.RoundTripper { + return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { + if req.Body != nil { + wc := &wantCloser{ReadCloser: req.Body} + t.Cleanup(func() { + require.Equalf(t, wc.closeCount, 1, "did not close req body expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) + }) + req.Body = wc + } + + if req.GetBody != nil { + originalBodyCopy, originalErr := req.GetBody() + req.GetBody = func() (io.ReadCloser, error) { + if originalErr != nil { + return nil, originalErr + } + wc := &wantCloser{ReadCloser: originalBodyCopy} + t.Cleanup(func() { + require.Equalf(t, wc.closeCount, 1, "did not close req body copy expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) + }) + return wc, nil + } + } + + resp, err := rt.RoundTrip(req) + return false, resp, err + }) + } +} + +// wantCloseRespWrapper returns a transport.WrapperFunc that validates that the http.Response +// returned by the underlying http.RoundTripper is closed properly. +func wantCloseRespWrapper(t *testing.T) transport.WrapperFunc { + caller := getCaller() return func(rt http.RoundTripper) http.RoundTripper { return roundTripperFunc(func(req *http.Request) (bool, *http.Response, error) { resp, err := rt.RoundTrip(req) @@ -705,8 +739,8 @@ func wantCloseRespWrapper(t *testing.T) transport.WrapperFunc { } wc := &wantCloser{ReadCloser: resp.Body} t.Cleanup(func() { - require.False(t, wc.couldReadBytesJustBeforeClosing, "did not consume all response body bytes before closing %s:%d", file, line) - require.Equalf(t, wc.closeCount, 1, "did not close resp body at %s:%d", file, line) + require.False(t, wc.couldReadBytesJustBeforeClosing, "did not consume all response body bytes before closing %s", caller) + require.Equalf(t, wc.closeCount, 1, "did not close resp body expected number of times at %s for req %#v; actual calls = %s", caller, req, wc.closeCalls) }) resp.Body = wc return false, resp, err diff --git a/internal/kubeclient/option.go b/internal/kubeclient/option.go index 789120fb..7c10bda9 100644 --- a/internal/kubeclient/option.go +++ b/internal/kubeclient/option.go @@ -3,13 +3,17 @@ package kubeclient -import restclient "k8s.io/client-go/rest" +import ( + restclient "k8s.io/client-go/rest" + "k8s.io/client-go/transport" +) type Option func(*clientConfig) type clientConfig struct { - config *restclient.Config - middlewares []Middleware + config *restclient.Config + middlewares []Middleware + transportWrapper transport.WrapperFunc } func WithConfig(config *restclient.Config) Option { @@ -27,3 +31,12 @@ func WithMiddleware(middleware Middleware) Option { c.middlewares = append(c.middlewares, middleware) } } + +// WithTransportWrapper will wrap the client-go http.RoundTripper chain *after* the middleware +// wrapper is applied. I.e., this wrapper has the opportunity to supply an http.RoundTripper that +// runs first in the client-go http.RoundTripper chain. +func WithTransportWrapper(wrapper transport.WrapperFunc) Option { + return func(c *clientConfig) { + c.transportWrapper = wrapper + } +} diff --git a/internal/kubeclient/roundtrip.go b/internal/kubeclient/roundtrip.go index 5caf2a39..3d6029dc 100644 --- a/internal/kubeclient/roundtrip.go +++ b/internal/kubeclient/roundtrip.go @@ -23,7 +23,7 @@ import ( "go.pinniped.dev/internal/plog" ) -func configWithWrapper(config *restclient.Config, scheme *runtime.Scheme, negotiatedSerializer runtime.NegotiatedSerializer, middlewares []Middleware) *restclient.Config { +func configWithWrapper(config *restclient.Config, scheme *runtime.Scheme, negotiatedSerializer runtime.NegotiatedSerializer, middlewares []Middleware, wrapper transport.WrapperFunc) *restclient.Config { hostURL, apiPathPrefix, err := getHostAndAPIPathPrefix(config) if err != nil { plog.DebugErr("invalid rest config", err) @@ -49,6 +49,9 @@ func configWithWrapper(config *restclient.Config, scheme *runtime.Scheme, negoti cc := restclient.CopyConfig(config) cc.Wrap(f) + if wrapper != nil { + cc.Wrap(wrapper) + } return cc } @@ -173,20 +176,20 @@ func handleOtherVerbs( resp, err := rt.RoundTrip(newReq) if err != nil { - return true, nil, fmt.Errorf("middleware request for %#v failed: %w", middlewareReq, err) + return false, nil, fmt.Errorf("middleware request for %#v failed: %w", middlewareReq, err) } switch v { case VerbDelete, VerbDeleteCollection: - return true, resp, nil // we do not need to fix the response on delete + return false, resp, nil // we do not need to fix the response on delete case VerbWatch: resp, err := handleWatchResponseNewGVK(config, negotiatedSerializer, resp, middlewareReq, result) - return true, resp, err + return false, resp, err default: // VerbGet, VerbList, VerbPatch resp, err := handleResponseNewGVK(config, negotiatedSerializer, resp, middlewareReq, result) - return true, resp, err + return false, resp, err } } diff --git a/internal/testutil/fakekubeapi/fakekubeapi.go b/internal/testutil/fakekubeapi/fakekubeapi.go index e3096e5e..66450f58 100644 --- a/internal/testutil/fakekubeapi/fakekubeapi.go +++ b/internal/testutil/fakekubeapi/fakekubeapi.go @@ -19,6 +19,7 @@ package fakekubeapi import ( "encoding/pem" + "fmt" "io/ioutil" "mime" "net/http" @@ -39,20 +40,6 @@ import ( "go.pinniped.dev/internal/multierror" ) -// Unlike the standard httperr.New(), this one does not prepend error messages with any prefix. -type plainHTTPErr struct { - code int - msg string -} - -func (e plainHTTPErr) Error() string { - return e.msg -} - -func (e plainHTTPErr) Respond(w http.ResponseWriter) { - http.Error(w, e.msg, e.code) -} - // Start starts an httptest.Server (with TLS) that pretends to be a Kube API server. // // The server uses the provided resources map to store API Object's. The map should be from API path @@ -62,9 +49,9 @@ func (e plainHTTPErr) Respond(w http.ResponseWriter) { // to the server. // // Note! Only these following verbs are (partially) supported: create, get, update, delete. -func Start(t *testing.T, resources map[string]metav1.Object) (*httptest.Server, *restclient.Config) { +func Start(t *testing.T, resources map[string]runtime.Object) (*httptest.Server, *restclient.Config) { if resources == nil { - resources = make(map[string]metav1.Object) + resources = make(map[string]runtime.Object) } server := httptest.NewTLSServer(httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (err error) { @@ -78,12 +65,8 @@ func Start(t *testing.T, resources map[string]metav1.Object) (*httptest.Server, return err } - if r.Method != http.MethodDelete && obj == nil { - return &plainHTTPErr{ - code: http.StatusNotFound, - // This is representative of a real Kube 404 message body. - msg: `{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"not found","reason":"NotFound","details":{"name":"not-found","kind":"pods"},"code":404}`, - } + if obj == nil { + obj = newNotFoundStatus(r.URL.Path) } if err := encodeObj(w, r, obj); err != nil { @@ -101,7 +84,7 @@ func Start(t *testing.T, resources map[string]metav1.Object) (*httptest.Server, return server, restConfig } -func decodeObj(r *http.Request) (metav1.Object, error) { +func decodeObj(r *http.Request) (runtime.Object, error) { switch r.Method { case http.MethodPut, http.MethodPost: default: @@ -123,7 +106,7 @@ func decodeObj(r *http.Request) (metav1.Object, error) { return nil, httperr.Wrap(http.StatusInternalServerError, "read body", err) } - var obj metav1.Object + var obj runtime.Object multiErr := multierror.New() codecsThatWeUseInOurCode := []runtime.NegotiatedSerializer{ kubescheme.Codecs, @@ -145,7 +128,7 @@ func tryDecodeObj( mediaType string, body []byte, negotiatedSerializer runtime.NegotiatedSerializer, -) (metav1.Object, error) { +) (runtime.Object, error) { serializerInfo, ok := runtime.SerializerInfoForMediaType(negotiatedSerializer.SupportedMediaTypes(), mediaType) if !ok { return nil, httperr.Newf(http.StatusInternalServerError, "unable to find serialier with content-type %s", mediaType) @@ -156,19 +139,17 @@ func tryDecodeObj( return nil, httperr.Wrap(http.StatusInternalServerError, "decode obj", err) } - return obj.(metav1.Object), nil + return obj, nil } -func handleObj(r *http.Request, obj metav1.Object, resources map[string]metav1.Object) (metav1.Object, error) { +func handleObj(r *http.Request, obj runtime.Object, resources map[string]runtime.Object) (runtime.Object, error) { switch r.Method { case http.MethodGet: obj = resources[r.URL.Path] case http.MethodPost, http.MethodPut: - resources[path.Join(r.URL.Path, obj.GetName())] = obj + resources[path.Join(r.URL.Path, obj.(metav1.Object).GetName())] = obj case http.MethodDelete: - if _, ok := resources[r.URL.Path]; !ok { - return nil, httperr.Newf(http.StatusNotFound, "no resource with path %q", r.URL.Path) - } + obj = resources[r.URL.Path] delete(resources, r.URL.Path) default: return nil, httperr.New(http.StatusMethodNotAllowed, "check source code for methods supported") @@ -177,7 +158,18 @@ func handleObj(r *http.Request, obj metav1.Object, resources map[string]metav1.O return obj, nil } -func encodeObj(w http.ResponseWriter, r *http.Request, obj metav1.Object) error { +func newNotFoundStatus(path string) runtime.Object { + status := &metav1.Status{ + Status: metav1.StatusFailure, + Message: fmt.Sprintf("couldn't find object for path %q", path), + Reason: metav1.StatusReasonNotFound, + Code: http.StatusNotFound, + } + status.APIVersion, status.Kind = metav1.SchemeGroupVersion.WithKind("Status").ToAPIVersionAndKind() + return status +} + +func encodeObj(w http.ResponseWriter, r *http.Request, obj runtime.Object) error { if r.Method == http.MethodDelete { return nil }