New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure webhook service routing resolves kubernetes.default.svc correctly #62649

Merged
merged 5 commits into from Apr 20, 2018

Conversation

@liggitt
Member

liggitt commented Apr 16, 2018

Going through the normal endpoint resolve path isn't correct in multi-master scenarios

The auth wrapper is pulling from LoopbackClientConfig, the service resolver should do the same

Fixes the kubernetes.default.svc loopback service resolution to use a loopback configuration.
@liggitt

This comment has been minimized.

Member

liggitt commented Apr 16, 2018

/assign @deads2k

@deads2k

This comment has been minimized.

Contributor

deads2k commented Apr 16, 2018

/lgtm

@lavalamp

This comment has been minimized.

Member

lavalamp commented Apr 16, 2018

Please test these things if you want them to keep working:

/lgtm cancel

I think this is an awful hack to work around not having an identity for apiserver. e.g. clearly this can't work for an aggregated apiserver. Or am I misunderstanding something?

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 16, 2018

I think this is an awful hack to work around not having an identity for apiserver

the only service the apiserver can make authentication assumptions about is the kubernetes/default service, and only because it is serving it and controls authentication for it.

e.g. clearly this can't work for an aggregated apiserver

that's why this is only used in kube-apiserver. aggregated api servers running on top of the kube control plane use default dns resolution and in-cluster auth without incident.

@lavalamp

This comment has been minimized.

Member

lavalamp commented Apr 16, 2018

the only service the apiserver can make authentication assumptions about is the kubernetes/default service, and only because it is serving it and controls authentication for it.

A concrete example request that exercises the path would help a lot.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Apr 17, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 17, 2018

test added (verified it failed on master with an unresolveable address timeout), three additional bugs fixed. most integration tests do not actually use the real apiserver construction methods, so wouldn't have exercised the code that was fixed.

The closest integration test to reality I found was this one:

kubeAPIServerOptions := options.NewServerRunOptions()
kubeAPIServerOptions.SecureServing.Listener = listener
kubeAPIServerOptions.SecureServing.BindAddress = net.ParseIP("127.0.0.1")
kubeAPIServerOptions.SecureServing.ServerCert.CertDirectory = certDir
kubeAPIServerOptions.InsecureServing.BindPort = 0
kubeAPIServerOptions.Etcd.StorageConfig.ServerList = []string{framework.GetEtcdURL()}
kubeAPIServerOptions.ServiceClusterIPRange = *defaultServiceClusterIPRange
kubeAPIServerOptions.Authentication.RequestHeader.UsernameHeaders = []string{"X-Remote-User"}
kubeAPIServerOptions.Authentication.RequestHeader.GroupHeaders = []string{"X-Remote-Group"}
kubeAPIServerOptions.Authentication.RequestHeader.ExtraHeaderPrefixes = []string{"X-Remote-Extra-"}
kubeAPIServerOptions.Authentication.RequestHeader.AllowedNames = []string{"kube-aggregator"}
kubeAPIServerOptions.Authentication.RequestHeader.ClientCAFile = proxyCACertFile.Name()
kubeAPIServerOptions.Authentication.ClientCert.ClientCA = clientCACertFile.Name()
kubeAPIServerOptions.Authorization.Modes = []string{"RBAC"}
completedOptions, err := app.Complete(kubeAPIServerOptions)
if err != nil {
t.Fatal(err)
}
tunneler, proxyTransport, err := app.CreateNodeDialer(completedOptions)
if err != nil {
t.Fatal(err)
}
kubeAPIServerConfig, sharedInformers, versionedInformers, _, _, _, err := app.CreateKubeAPIServerConfig(completedOptions, tunneler, proxyTransport)
if err != nil {
t.Fatal(err)
}
// Adjust the loopback config for external use (external server name and CA)
kubeAPIServerClientConfig := rest.CopyConfig(kubeAPIServerConfig.GenericConfig.LoopbackClientConfig)
kubeAPIServerClientConfig.CAFile = path.Join(certDir, "apiserver.crt")
kubeAPIServerClientConfig.CAData = nil
kubeAPIServerClientConfig.ServerName = ""
kubeClientConfigValue.Store(kubeAPIServerClientConfig)
kubeAPIServer, err := app.CreateKubeAPIServer(kubeAPIServerConfig, genericapiserver.NewEmptyDelegate(), sharedInformers, versionedInformers)
if err != nil {
t.Fatal(err)
}
if err := kubeAPIServer.GenericAPIServer.PrepareRun().Run(wait.NeverStop); err != nil {
t.Fatal(err)
}

In the test commit in this PR, I started to extract the test setup to something that:

  1. gives you a one-call invocation to get a running apiserver
  2. lets you tweak run options or config prior to starting
  3. makes use of the actual config and server-building functions that kube-apiserver uses

client, _ := startTestServer(t, stopCh, TestServerSetup{
ModifyServerRunOptions: func(opts *options.ServerRunOptions) {
},
ModifyServerConfig: func(config *master.Config) {
// Avoid resolveable kubernetes service
config.ExtraConfig.EndpointReconcilerType = reconcilers.NoneEndpointReconcilerType
// Hook into audit to watch requests
config.GenericConfig.AuditBackend = auditSinkFunc(func(events ...*auditinternal.Event) {})
config.GenericConfig.AuditPolicyChecker = auditChecker(func(attrs authorizer.Attributes) (auditinternal.Level, []auditinternal.Stage) {
if attrs.GetPath() == webhookPath {
if attrs.GetUser().GetName() != "system:apiserver" {
t.Errorf("expected user %q, got %q", "system:apiserver", attrs.GetUser().GetName())
}
atomic.AddInt32(&called, 1)
}
return auditinternal.LevelNone, nil
})
},
})

@lavalamp

This comment has been minimized.

Member

lavalamp commented Apr 17, 2018

Thank you!

Will look tomorrow if no one beats me to it.

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 17, 2018

/retest

@@ -52,7 +52,7 @@ func New(config *Config) (http.RoundTripper, error) {
// TLSConfigFor returns a tls.Config that will provide the transport level security defined
// by the provided Config. Will return nil if no transport level security is requested.
func TLSConfigFor(c *Config) (*tls.Config, error) {
if !(c.HasCA() || c.HasCertAuth() || c.TLS.Insecure) {
if !(c.HasCA() || c.HasCertAuth() || c.TLS.Insecure || len(c.TLS.ServerName) > 0) {

This comment has been minimized.

@deads2k

deads2k Apr 17, 2018

Contributor

if tls.Insecure is true, why don't we just return nil, nil?

This comment has been minimized.

@liggitt

liggitt Apr 17, 2018

Member

anything that deviates from the defaults must return a custom tls.Config

setting Insecure=true or a custom ServerName requires a non-default tls.Config

@deads2k

This comment has been minimized.

Contributor

deads2k commented Apr 17, 2018

Question on the TLS bit. Integration test looks like it is testing the wiring more than the code since kube integration doesn't run real servers, but sure.

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 20, 2018

any other comments?

@deads2k

This comment has been minimized.

Contributor

deads2k commented Apr 20, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 20, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

liggitt added some commits Apr 17, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 20, 2018

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 20, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 20, 2018

rebased

@liggitt liggitt added the lgtm label Apr 20, 2018

@fejta-bot

This comment has been minimized.

fejta-bot commented Apr 20, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 20, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 20, 2018

Automatic merge from submit-queue (batch tested with PRs 50899, 62649). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 9c25da6 into kubernetes:master Apr 20, 2018

7 of 15 checks passed

pull-kubernetes-e2e-gce Job failed.
Details
pull-kubernetes-integration Job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-local-e2e Skipped
pull-kubernetes-typecheck Job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 20, 2018

@liggitt: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce d421aff link /test pull-kubernetes-e2e-gce
pull-kubernetes-integration d421aff link /test pull-kubernetes-integration
pull-kubernetes-e2e-kops-aws d421aff link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-merge-robot added a commit that referenced this pull request Apr 24, 2018

Merge pull request #62654 from liggitt/automated-cherry-pick-of-#62649-…
…upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #62649: Ensure service routing resolves kubernetes.default.svc

Cherry pick of #62649 on release-1.10.

#62649: Ensure service routing resolves kubernetes.default.svc
@munnerz

This comment has been minimized.

Member

munnerz commented Apr 25, 2018

Is this likely to be also accepted into release-1.9 if cherry picked? Not sure what the usual convention is for which releases something should be backported to.

@liggitt liggitt deleted the liggitt:loopback-routing branch Apr 25, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 27, 2018

Is this likely to be also accepted into release-1.9 if cherry picked? Not sure what the usual convention is for which releases something should be backported to.

picked to 1.9 in #63219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment