Skip to content
New issue

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

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

apiserver identity: use persistent names for lease objects #113307

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/controlplane/instance.go
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"net"
"net/http"
"os"
"reflect"
"strconv"
"time"
Expand Down Expand Up @@ -60,6 +61,7 @@ import (
"k8s.io/apimachinery/pkg/runtime/schema"
utilnet "k8s.io/apimachinery/pkg/util/net"
"k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/uuid"
"k8s.io/apiserver/pkg/endpoints/discovery"
apiserverfeatures "k8s.io/apiserver/pkg/features"
"k8s.io/apiserver/pkg/registry/generic"
Expand Down Expand Up @@ -470,13 +472,18 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
if err != nil {
return err
}

leaseName := m.GenericAPIServer.APIServerID
holderIdentity := m.GenericAPIServer.APIServerID + "_" + string(uuid.NewUUID())

controller := lease.NewController(
clock.RealClock{},
kubeClient,
m.GenericAPIServer.APIServerID,
holderIdentity,
int32(c.ExtraConfig.IdentityLeaseDurationSeconds),
nil,
time.Duration(c.ExtraConfig.IdentityLeaseRenewIntervalSeconds)*time.Second,
leaseName,
metav1.NamespaceSystem,
labelAPIServerHeartbeat)
go controller.Run(hookContext.StopCh)
Expand Down Expand Up @@ -515,6 +522,14 @@ func labelAPIServerHeartbeat(lease *coordinationapiv1.Lease) error {
}
// This label indicates that kube-apiserver owns this identity lease object
lease.Labels[IdentityLeaseComponentLabelKey] = KubeAPIServer

hostname, err := os.Hostname()
if err != nil {
return err
}

// convenience label to easily map a lease object to a specific apiserver
lease.Labels[apiv1.LabelHostname] = hostname
return nil
}

Expand Down
1 change: 1 addition & 0 deletions pkg/kubelet/kubelet.go
Expand Up @@ -833,6 +833,7 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
kubeCfg.NodeLeaseDurationSeconds,
klet.onRepeatedHeartbeatFailure,
renewInterval,
string(klet.nodeName),
v1.NamespaceNodeLease,
util.SetNodeOwnerFunc(klet.heartbeatClient, string(klet.nodeName)))

Expand Down
11 changes: 10 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/server/config.go
Expand Up @@ -19,8 +19,10 @@ package server
import (
"context"
"fmt"
"hash/fnv"
"net"
"net/http"
"os"
goruntime "runtime"
"runtime/debug"
"sort"
Expand Down Expand Up @@ -328,7 +330,14 @@ func NewConfig(codecs serializer.CodecFactory) *Config {
defaultHealthChecks := []healthz.HealthChecker{healthz.PingHealthz, healthz.LogHealthz}
var id string
if utilfeature.DefaultFeatureGate.Enabled(genericfeatures.APIServerIdentity) {
id = "kube-apiserver-" + uuid.New().String()
hostname, err := os.Hostname()
if err != nil {
klog.Fatalf("error getting hostname for apiserver identity: %v", err)
}

h := fnv.New32a()
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a real hash function, this is only called at startup, there's no performance problem. Can you switch to SHA256? It is OK to truncate the result a bit, but 4 bytes is too small.

If we assume there are 1M k8s clusters, each with 3 apiservers, there is a 1000000*(1-e^(-(3^2)/(2^32))) = .2% one of them will have a collision. That's far too high, I would like a chance less than 1e-9. (google "birthday paradox estimator" if you want to check my math)

6 bytes is enough to pass that test, so I would go to 8 bytes selected from SHA256, where there is no question that it is very random.

Copy link
Member

Choose a reason for hiding this comment

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

(and sorry I wasn't able to review this until now)

Copy link
Member

Choose a reason for hiding this comment

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

(oh and if you want you can use the base58 encoder to save some characters)

Copy link
Member

Choose a reason for hiding this comment

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

Why bother truncating at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #113649 -- tests seem fine without needing to truncate, but still might be worthwhile just to avoid lease names that are really long. I don't feel too strongly with the lease name being that long but myabe some people do?

h.Write([]byte(hostname))
id = "kube-apiserver-" + fmt.Sprint(h.Sum32())
}
lifecycleSignals := newLifecycleSignals()

Expand Down
Expand Up @@ -54,6 +54,7 @@ type controller struct {
client clientset.Interface
leaseClient coordclientset.LeaseInterface
holderIdentity string
leaseName string
leaseNamespace string
leaseDurationSeconds int32
renewInterval time.Duration
Expand All @@ -71,7 +72,7 @@ type controller struct {
}

// NewController constructs and returns a controller
func NewController(clock clock.Clock, client clientset.Interface, holderIdentity string, leaseDurationSeconds int32, onRepeatedHeartbeatFailure func(), renewInterval time.Duration, leaseNamespace string, newLeasePostProcessFunc ProcessLeaseFunc) Controller {
func NewController(clock clock.Clock, client clientset.Interface, holderIdentity string, leaseDurationSeconds int32, onRepeatedHeartbeatFailure func(), renewInterval time.Duration, leaseName, leaseNamespace string, newLeasePostProcessFunc ProcessLeaseFunc) Controller {
var leaseClient coordclientset.LeaseInterface
if client != nil {
leaseClient = client.CoordinationV1().Leases(leaseNamespace)
Expand All @@ -80,6 +81,7 @@ func NewController(clock clock.Clock, client clientset.Interface, holderIdentity
client: client,
leaseClient: leaseClient,
holderIdentity: holderIdentity,
leaseName: leaseName,
leaseNamespace: leaseNamespace,
leaseDurationSeconds: leaseDurationSeconds,
renewInterval: renewInterval,
Expand Down Expand Up @@ -151,7 +153,7 @@ func (c *controller) backoffEnsureLease() (*coordinationv1.Lease, bool) {
// ensureLease creates the lease if it does not exist. Returns the lease and
// a bool (true if this call created the lease), or any error that occurs.
func (c *controller) ensureLease() (*coordinationv1.Lease, bool, error) {
lease, err := c.leaseClient.Get(context.TODO(), c.holderIdentity, metav1.GetOptions{})
lease, err := c.leaseClient.Get(context.TODO(), c.leaseName, metav1.GetOptions{})
if apierrors.IsNotFound(err) {
// lease does not exist, create it.
leaseToCreate, err := c.newLease(nil)
Expand Down Expand Up @@ -208,7 +210,7 @@ func (c *controller) newLease(base *coordinationv1.Lease) (*coordinationv1.Lease
if base == nil {
lease = &coordinationv1.Lease{
ObjectMeta: metav1.ObjectMeta{
Name: c.holderIdentity,
Name: c.leaseName,
Namespace: c.leaseNamespace,
},
Spec: coordinationv1.LeaseSpec{
Expand Down
Expand Up @@ -59,6 +59,7 @@ func TestNewNodeLease(t *testing.T) {
desc: "nil base without node",
controller: &controller{
client: fake.NewSimpleClientset(),
leaseName: node.Name,
holderIdentity: node.Name,
leaseDurationSeconds: 10,
clock: fakeClock,
Expand All @@ -80,6 +81,7 @@ func TestNewNodeLease(t *testing.T) {
desc: "nil base with node",
controller: &controller{
client: fake.NewSimpleClientset(node),
leaseName: node.Name,
holderIdentity: node.Name,
leaseDurationSeconds: 10,
clock: fakeClock,
Expand Down
37 changes: 33 additions & 4 deletions test/integration/controlplane/apiserver_identity_test.go
Expand Up @@ -18,11 +18,14 @@ package controlplane

import (
"context"
"strings"
"fmt"
"hash/fnv"
"os"
"testing"
"time"

coordinationv1 "k8s.io/api/coordination/v1"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/wait"
Expand All @@ -40,6 +43,12 @@ const (
testLeaseName = "apiserver-lease-test"
)

func expectedAPIServerIdentity(hostname string) string {
h := fnv.New32a()
h.Write([]byte(hostname))
return "kube-apiserver-" + fmt.Sprint(h.Sum32())
}

func TestCreateLeaseOnStart(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.APIServerIdentity, true)()
result := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd())
Expand All @@ -50,6 +59,11 @@ func TestCreateLeaseOnStart(t *testing.T) {
t.Fatalf("Unexpected error: %v", err)
}

hostname, err := os.Hostname()
if err != nil {
t.Fatalf("Unexpected error getting apiserver hostname: %v", err)
}

t.Logf(`Waiting the kube-apiserver Lease to be created`)
if err := wait.PollImmediate(500*time.Millisecond, 10*time.Second, func() (bool, error) {
leases, err := kubeclient.
Expand All @@ -59,10 +73,25 @@ func TestCreateLeaseOnStart(t *testing.T) {
if err != nil {
return false, err
}
if leases != nil && len(leases.Items) == 1 && strings.HasPrefix(leases.Items[0].Name, "kube-apiserver-") {
return true, nil

if leases == nil {
return false, nil
}
return false, nil

if len(leases.Items) != 1 {
return false, nil
}

lease := leases.Items[0]
if lease.Name != expectedAPIServerIdentity(hostname) {
return false, fmt.Errorf("unexpected apiserver identity, got: %v, expected: %v", lease.Name, expectedAPIServerIdentity(hostname))
}

if lease.Labels[corev1.LabelHostname] != hostname {
return false, fmt.Errorf("unexpected hostname label, got: %v, expected: %v", lease.Labels[corev1.LabelHostname], hostname)
}

return true, nil
}); err != nil {
t.Fatalf("Failed to see the kube-apiserver lease: %v", err)
}
Expand Down