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
Step 3 – generic controlplane: split server #120202
Step 3 – generic controlplane: split server #120202
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
48eb016
to
e4e3524
Compare
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
ccb2c07
to
31a8223
Compare
return fmt.Errorf("error in registering group versions: %w", err) | ||
} | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier review:
// InstallAPIs will install the APIs for the restStorageProviders if they are enabled.
-func (m *Instance) InstallAPIs(apiResourceConfigSource serverstorage.APIResourceConfigSource, restOptionsGetter generic.RESTOptionsGetter, restStorageProviders ...RESTStorageProvider) error {
+func (s *Server) InstallAPIs(restStorageProviders ...RESTStorageProvider) error {
nonLegacy := []*genericapiserver.APIGroupInfo{}
// used later in the loop to filter the served resource by those that have expired.
- resourceExpirationEvaluator, err := genericapiserver.NewResourceExpirationEvaluator(*m.GenericAPIServer.Version)
+ resourceExpirationEvaluator, err := genericapiserver.NewResourceExpirationEvaluator(*s.GenericAPIServer.Version)
if err != nil {
return err
}
for _, restStorageBuilder := range restStorageProviders {
groupName := restStorageBuilder.GroupName()
- apiGroupInfo, err := restStorageBuilder.NewRESTStorage(apiResourceConfigSource, restOptionsGetter)
+ apiGroupInfo, err := restStorageBuilder.NewRESTStorage(s.APIResourceConfigSource, s.RESTOptionsGetter)
if err != nil {
- return fmt.Errorf("problem initializing API group %q : %v", groupName, err)
+ return fmt.Errorf("problem initializing API group %q: %w", groupName, err)
}
if len(apiGroupInfo.VersionedResourcesStorageMap) == 0 {
// If we have no storage for any resource configured, this API group is effectively disabled.
@@ -35,14 +35,14 @@
if postHookProvider, ok := restStorageBuilder.(genericapiserver.PostStartHookProvider); ok {
name, hook, err := postHookProvider.PostStartHook()
if err != nil {
- klog.Fatalf("Error building PostStartHook: %v", err)
+ return fmt.Errorf("error building PostStartHook: %w", err)
}
- m.GenericAPIServer.AddPostStartHookOrDie(name, hook)
+ s.GenericAPIServer.AddPostStartHookOrDie(name, hook)
}
if len(groupName) == 0 {
// the legacy group for core APIs is special that it is installed into /api via this special install method.
- if err := m.GenericAPIServer.InstallLegacyAPIGroup(genericapiserver.DefaultLegacyAPIPrefix, &apiGroupInfo); err != nil {
+ if err := s.GenericAPIServer.InstallLegacyAPIGroup(genericapiserver.DefaultLegacyAPIPrefix, &apiGroupInfo); err != nil {
return fmt.Errorf("error in registering legacy API: %w", err)
}
} else {
@@ -51,8 +51,8 @@
}
}
- if err := m.GenericAPIServer.InstallAPIGroups(nonLegacy...); err != nil {
- return fmt.Errorf("error in registering group versions: %v", err)
+ if err := s.GenericAPIServer.InstallAPIGroups(nonLegacy...); err != nil {
+ return fmt.Errorf("error in registering group versions: %w", err)
}
return nil
}
}) | ||
} | ||
|
||
if utilfeature.DefaultFeatureGate.Enabled(features.UnknownVersionInteroperabilityProxy) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier review:
+ _, publicServicePort, err := c.Generic.SecureServing.HostPort()
+ if err != nil {
+ return nil, fmt.Errorf("failed to get listener address: %w", err)
+ }
+
if utilfeature.DefaultFeatureGate.Enabled(features.UnknownVersionInteroperabilityProxy) {
- peeraddress := getPeerAddress(c.ControlPlane.Extra.PeerAdvertiseAddress, c.ControlPlane.Generic.PublicAddress, publicServicePort)
+ peeraddress := getPeerAddress(c.Extra.PeerAdvertiseAddress, c.Generic.PublicAddress, publicServicePort)
peerEndpointCtrl := peerreconcilers.New(
- c.ControlPlane.Generic.APIServerID,
+ c.Generic.APIServerID,
peeraddress,
- c.ControlPlane.Extra.PeerEndpointLeaseReconciler,
- c.Extra.EndpointReconcilerConfig.Interval,
+ c.Extra.PeerEndpointLeaseReconciler,
+ c.Extra.PeerEndpointReconcileInterval,
client)
if err != nil {
return nil, fmt.Errorf("failed to create peer endpoint lease controller: %w", err)
}
- m.GenericAPIServer.AddPostStartHookOrDie("peer-endpoint-reconciler-controller",
+ s.GenericAPIServer.AddPostStartHookOrDie("peer-endpoint-reconciler-controller",
func(hookContext genericapiserver.PostStartHookContext) error {
peerEndpointCtrl.Start(hookContext.StopCh)
return nil
})
- m.GenericAPIServer.AddPreShutdownHookOrDie("peer-endpoint-reconciler-controller",
+ s.GenericAPIServer.AddPreShutdownHookOrDie("peer-endpoint-reconciler-controller",
func() error {
peerEndpointCtrl.Stop()
return nil
})
- // Add PostStartHooks for Unknown Version Proxy filter.
- if c.ControlPlane.Extra.PeerProxy != nil {
- m.GenericAPIServer.AddPostStartHookOrDie("unknown-version-proxy-filter", func(context genericapiserver.PostStartHookContext) error {
- err := c.ControlPlane.Extra.PeerProxy.WaitForCacheSync(context.StopCh)
+ if c.Extra.PeerProxy != nil {
+ s.GenericAPIServer.AddPostStartHookOrDie("unknown-version-proxy-filter", func(context genericapiserver.PostStartHookContext) error {
+ err := c.Extra.PeerProxy.WaitForCacheSync(context.StopCh)
return err
})
}
}
- m.GenericAPIServer.AddPostStartHookOrDie("start-cluster-authentication-info-controller", func(hookContext genericapiserver.PostStartHookContext) error {
- controller := clusterauthenticationtrust.NewClusterAuthenticationTrustController(m.ClusterAuthenticationInfo, client)
+ s.GenericAPIServer.AddPostStartHookOrDie("start-cluster-authentication-info-controller", func(hookContext genericapiserver.PostStartHookContext) error {
+ controller := clusterauthenticationtrust.NewClusterAuthenticationTrustController(s.ClusterAuthenticationInfo, client)
// generate a context from stopCh. This is to avoid modifying files which are relying on apiserver
// TODO: See if we can pass ctx to the current method
ctx := wait.ContextForChannel(hookContext.StopCh)
// prime values and start listeners
- if m.ClusterAuthenticationInfo.ClientCA != nil {
- m.ClusterAuthenticationInfo.ClientCA.AddListener(controller)
- if controller, ok := m.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.ControllerRunner); ok {
+ if s.ClusterAuthenticationInfo.ClientCA != nil {
+ s.ClusterAuthenticationInfo.ClientCA.AddListener(controller)
+ if controller, ok := s.ClusterAuthenticationInfo.ClientCA.(dynamiccertificates.ControllerRunner); ok {
// runonce to be sure that we have a value.
if err := controller.RunOnce(ctx); err != nil {
runtime.HandleError(err)
@@ -46,9 +50,9 @@
go controller.Run(ctx, 1)
}
}
- if m.ClusterAuthenticationInfo.RequestHeaderCA != nil {
- m.ClusterAuthenticationInfo.RequestHeaderCA.AddListener(controller)
- if controller, ok := m.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.ControllerRunner); ok {
+ if s.ClusterAuthenticationInfo.RequestHeaderCA != nil {
+ s.ClusterAuthenticationInfo.RequestHeaderCA.AddListener(controller)
+ if controller, ok := s.ClusterAuthenticationInfo.RequestHeaderCA.(dynamiccertificates.ControllerRunner); ok {
// runonce to be sure that we have a value.
if err := controller.RunOnce(ctx); err != nil {
runtime.HandleError(err)
@@ -62,15 +66,15 @@
})
if utilfeature.DefaultFeatureGate.Enabled(apiserverfeatures.APIServerIdentity) {
- m.GenericAPIServer.AddPostStartHookOrDie("start-kube-apiserver-identity-lease-controller", func(hookContext genericapiserver.PostStartHookContext) error {
+ s.GenericAPIServer.AddPostStartHookOrDie("start-kube-apiserver-identity-lease-controller", func(hookContext genericapiserver.PostStartHookContext) error {
// generate a context from stopCh. This is to avoid modifying files which are relying on apiserver
// TODO: See if we can pass ctx to the current method
ctx := wait.ContextForChannel(hookContext.StopCh)
- leaseName := m.GenericAPIServer.APIServerID
- holderIdentity := m.GenericAPIServer.APIServerID + "_" + string(uuid.NewUUID())
+ leaseName := s.GenericAPIServer.APIServerID
+ holderIdentity := s.GenericAPIServer.APIServerID + "_" + string(uuid.NewUUID())
- peeraddress := getPeerAddress(c.ControlPlane.Extra.PeerAdvertiseAddress, c.ControlPlane.Generic.PublicAddress, publicServicePort)
+ peeraddress := getPeerAddress(c.Extra.PeerAdvertiseAddress, c.Generic.PublicAddress, publicServicePort)
// must replace ':,[]' in [ip:port] to be able to store this as a valid label value
controller := lease.NewController(
clock.RealClock{},
@@ -82,28 +86,28 @@
leaseName,
metav1.NamespaceSystem,
// TODO: receive identity label value as a parameter when post start hook is moved to generic apiserver.
- labelAPIServerHeartbeatFunc(KubeAPIServer, peeraddress))
+ labelAPIServerHeartbeatFunc(name, peeraddress))
go controller.Run(ctx)
return nil
})
// TODO: move this into generic apiserver and make the lease identity value configurable
- m.GenericAPIServer.AddPostStartHookOrDie("start-kube-apiserver-identity-lease-garbage-collector", func(hookContext genericapiserver.PostStartHookContext) error {
+ s.GenericAPIServer.AddPostStartHookOrDie("start-kube-apiserver-identity-lease-garbage-collector", func(hookContext genericapiserver.PostStartHookContext) error {
go apiserverleasegc.NewAPIServerLeaseGC(
client,
IdentityLeaseGCPeriod,
metav1.NamespaceSystem,
- KubeAPIServerIdentityLeaseLabelSelector,
+ IdentityLeaseComponentLabelKey+"="+name,
).Run(hookContext.StopCh)
return nil
})
}
- m.GenericAPIServer.AddPostStartHookOrDie("start-legacy-token-tracking-controller", func(hookContext genericapiserver.PostStartHookContext) error {
+ s.GenericAPIServer.AddPostStartHookOrDie("start-legacy-token-tracking-controller", func(hookContext genericapiserver.PostStartHookContext) error {
go legacytokentracking.NewController(client).Run(hookContext.StopCh)
return nil
})
- return m, nil
+ return s, nil
}
func labelAPIServerHeartbeatFunc(identity string, peeraddress string) lease.ProcessLeaseFunc {
} else { | ||
routes.NewOpenIDMetadataServer(md.ConfigJSON, md.PublicKeysetJSON). | ||
Install(generic.Handler.GoRestfulContainer) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier review:
- if c.ControlPlane.Extra.EnableLogsSupport {
- routes.Logs{}.Install(s.Handler.GoRestfulContainer)
+ if c.EnableLogsSupport {
+ routes.Logs{}.Install(generic.Handler.GoRestfulContainer)
}
// Metadata and keys are expected to only change across restarts at present,
// so we just marshal immediately and serve the cached JSON bytes.
md, err := serviceaccount.NewOpenIDMetadata(
- c.ControlPlane.Extra.ServiceAccountIssuerURL,
- c.ControlPlane.Extra.ServiceAccountJWKSURI,
- c.ControlPlane.Generic.ExternalAddress,
- c.ControlPlane.Extra.ServiceAccountPublicKeys,
+ c.ServiceAccountIssuerURL,
+ c.ServiceAccountJWKSURI,
+ c.Generic.ExternalAddress,
+ c.ServiceAccountPublicKeys,
)
if err != nil {
// If there was an error, skip installing the endpoints and log the
@@ -18,7 +18,7 @@
msg := fmt.Sprintf("Could not construct pre-rendered responses for"+
" ServiceAccountIssuerDiscovery endpoints. Endpoints will not be"+
" enabled. Error: %v", err)
- if c.ControlPlane.Extra.ServiceAccountIssuerURL != "" {
+ if c.ServiceAccountIssuerURL != "" {
// The user likely expects this feature to be enabled if issuer URL is
// set and the feature gate is enabled. In the future, if there is no
// longer a feature gate and issuer URL is not set, the user may not
@@ -30,5 +30,5 @@
}
} else {
routes.NewOpenIDMetadataServer(md.ConfigJSON, md.PublicKeysetJSON).
- Install(s.Handler.GoRestfulContainer)
+ Install(generic.Handler.GoRestfulContainer)
}
} else { | ||
return net.JoinHostPort(publicAddress.String(), strconv.Itoa(publicServicePort)) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For easier review:
// utility function to get the apiserver address that is used by peer apiservers to proxy
// requests to this apiserver in case the peer is incapable of serving the request
-func getPeerAddress(peerAdvertiseAddress peerreconcilers.PeerAdvertiseAddress, publicAddress net.IP, publicServicePort int) string {
+func getPeerAddress(peerAdvertiseAddress reconcilers.PeerAdvertiseAddress, publicAddress net.IP, publicServicePort int) string {
if peerAdvertiseAddress.PeerAdvertiseIP != "" && peerAdvertiseAddress.PeerAdvertisePort != "" {
return net.JoinHostPort(peerAdvertiseAddress.PeerAdvertiseIP, peerAdvertiseAddress.PeerAdvertisePort)
} else {
67d34d1
to
81dd49f
Compare
/retest |
2d50493
to
46f5ce6
Compare
looks straightforward move. waiting on #124576 |
46f5ce6
to
ff305aa
Compare
/retest |
Signed-off-by: Dr. Stefan Schimanski <stefan.schimanski@gmail.com>
ff305aa
to
3b6d2a6
Compare
#124576 merged. @mjudeikis if you can take another look |
@@ -84,7 +85,7 @@ func TestCreateLeaseOnStart(t *testing.T) { | |||
leases, err := kubeclient. | |||
CoordinationV1(). | |||
Leases(metav1.NamespaceSystem). | |||
List(context.TODO(), metav1.ListOptions{LabelSelector: controlplane.KubeAPIServerIdentityLeaseLabelSelector}) | |||
List(context.TODO(), metav1.ListOptions{LabelSelector: controlplaneapiserver.IdentityLeaseComponentLabelKey + "=" + controlplane.KubeAPIServer}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for learning, why this changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KubeAPIServerIdentityLeaseLabelSelector
was a strange constant, although the components were defined just next to it. Also we want other component names than kube-apiserver
for the generic case.
/lgtm |
LGTM label has been added. Git tree hash: 36bf9d664a9b54918075a070ff23446de540d779
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @sttts!
Minor comments, mostly for the future
Indeed LGTM as well 👍
"k8s.io/kubernetes/pkg/serviceaccount" | ||
) | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was var
before as well, but couldn't it be const
?
VersionedInformers clientgoinformers.SharedInformerFactory | ||
} | ||
|
||
// New returns a new instance of Master from the given config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new instance of Server...
// New returns a new instance of Master from the given config. | ||
// Certain config fields will be set to a default value if unset. | ||
// Certain config fields must be specified, including: | ||
// KubeletClientConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is not relevant anymore; either we remove this last comment, or specify the fields which are actually required.
IdentityLeaseRenewIntervalPeriod, | ||
leaseName, | ||
metav1.NamespaceSystem, | ||
// TODO: receive identity label value as a parameter when post start hook is moved to generic apiserver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fixed now? 😄
go controller.Run(ctx) | ||
return nil | ||
}) | ||
// TODO: move this into generic apiserver and make the lease identity value configurable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want to propagate the name variable into the hook ID below, instead of having "kube-apiserver" in the name, or if the IDs do not need to be stable, I guess we can just change "start-kube-apiserver-identity-lease-garbage-collector" to "start-apiserver-identity-lease-garbage-collector"
} | ||
} | ||
|
||
go controller.Run(ctx, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's just me, but it seems scary to have the same variable name here as for the "inner" controllers, that define "local" variables with the same name
I know the code was like this, but why not just (later) move this line up where controller was defined, and use non-overlapping names?
peerEndpointCtrl := peerreconcilers.New( | ||
c.Generic.APIServerID, | ||
peeraddress, | ||
c.Extra.PeerEndpointLeaseReconciler, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could remove the Extra
field reference now if you did not want that to be visible in the field path
}) | ||
} | ||
|
||
s.GenericAPIServer.AddPostStartHookOrDie("start-legacy-token-tracking-controller", func(hookContext genericapiserver.PostStartHookContext) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this in a new generic API server, if it's "legacy"?
return err | ||
} | ||
|
||
for _, restStorageBuilder := range restStorageProviders { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: (probably was before as well) unexpected to call it restStorageBuilder
when it is in the restStorageProviders
list, restStorageProvider
would be more expected
client := kubernetes.NewForConfigOrDie(s.GenericAPIServer.LoopbackClientConfig) | ||
if len(c.SystemNamespaces) > 0 { | ||
s.GenericAPIServer.AddPostStartHookOrDie("start-system-namespaces-controller", func(hookContext genericapiserver.PostStartHookContext) error { | ||
go systemnamespaces.NewController(c.SystemNamespaces, client, s.VersionedInformers.Core().V1().Namespaces()).Run(hookContext.StopCh) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably this is the pattern we want to follow, to have "local" properties off commonly-used fields in the structs they are used, but just noting that we could just use c.VersionedInformers
here, because it's not used anywhere else (for now at least)
similar case for ClusterAuthenticationInfo, it's only used in this setup function, not later, so technically it would not need to be in the struct
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR splits the kube-apiserver server struct
Instance
(pkg/controlplane/instance.go
) into the kube-apiserver-specific and the generic onepkg/controlplane/apiserver.Server
.Part of #124530.
Which issue(s) this PR fixes:
Towards kubernetes/enhancements#4080
Special notes for your reviewer:
pkg/controlplane
is a misnamer. It's actually highly kube-apiserver specific and will eventually be renamed to make the difference topkg/controlplane/apiserver
clear. But that's beyond this PR.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: