-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
add kube-apiserver wiring #20578
add kube-apiserver wiring #20578
Conversation
/retest |
Commented on the follow-up PR. |
@@ -164,6 +164,10 @@ func CreateServerChain(completedOptions completedServerRunOptions, stopCh <-chan | |||
return nil, err | |||
} | |||
|
|||
if err := PatchKubeAPIServerConfig(kubeAPIServerConfig, sharedInformers, versionedInformers, &pluginInitializer); err != 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.
👍
35dcbe2
to
f9b0ce8
Compare
@liggitt I plan to kill the |
no objection |
470fdac
to
db4e9a2
Compare
db4e9a2
to
d9dde2e
Compare
func BuildHandlerChain(genericConfig *genericapiserver.Config, kubeInformers informers.SharedInformerFactory, kubeAPIServerConfig *configapi.MasterConfig, stopCh <-chan struct{}) (func(apiHandler http.Handler, kc *genericapiserver.Config) http.Handler, map[string]genericapiserver.PostStartHookFunc, error) { | ||
webconsoleProxyHandler, err := newWebConsoleProxy(genericConfig, kubeInformers, kubeAPIServerConfig) | ||
func BuildHandlerChain(genericConfig *genericapiserver.Config, kubeInformers informers.SharedInformerFactory, kubeAPIServerConfig *configapi.MasterConfig) (func(apiHandler http.Handler, kc *genericapiserver.Config) http.Handler, map[string]genericapiserver.PostStartHookFunc, error) { | ||
extraPostStarkHooks := map[string]genericapiserver.PostStartHookFunc{} |
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.
typo: start
d9dde2e
to
107c2d1
Compare
2f21baf
to
4480027
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
28b9c41
to
28b885a
Compare
@sttts this is ready for a real review. We can bring in the old SCC as a later step for cluster up |
@smarterclayton in 4.0 are we ready to drop SCC in |
28b885a
to
1d2c1a7
Compare
return nil, err | ||
} | ||
|
||
// these flags are overridden by a patch |
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.
who maintains this list? not sure of the value
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.
who maintains this list? not sure of the value
We need this list so that we can create a struct to represent it in order to manage it. We'll need to write a test of some sort to make sure we don't lose them.
setIfUnset(args, "secure-port", portString) | ||
|
||
var keys []string | ||
for key := range args { |
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.
keys := append([]string(nil), args...)
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.
keys := append([]string(nil), args...)
args is a map
return nil, err | ||
} | ||
// if the config isn't a DefaultAdmissionConfig, then assume we're enabled (we were called after all) | ||
// if the config *is* a DefaultAdmissionConfig and it explicitly said |
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.
"it explicitly said" ?
|
||
} else if defaultConfig.Disable { | ||
forceOff = append(forceOff, pluginName) | ||
continue |
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.
no need for the continues if "else" is used.
tempFile.Close() | ||
|
||
setIfUnset(args, "admission-control-config-file", tempFile.Name()) | ||
setIfUnset(args, "disable-admission-plugins", forceOff...) |
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.
these are additive to the default values. Maybe we should append?
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.
these are additive to the default values. Maybe we should append?
We've always stomped admission with our choices. I only want one way to set via config.
@@ -1,23 +1,27 @@ | |||
package openshiftkubeapiserver | |||
|
|||
import ( | |||
"github.com/openshift/origin/pkg/admission/namespaceconditions" |
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.
Goland still hasn't learned our import order.
if err != nil { | ||
return nil, err | ||
} | ||
clusterQuotaMappingController := openshiftapiserver.NewClusterQuotaMappingController(kubeAPIServerInformers.InternalKubernetesInformers.Core().InternalVersion().Namespaces(), kubeAPIServerInformers.InternalOpenshiftQuotaInformers.Quota().InternalVersion().ClusterResourceQuotas()) | ||
kubeClient, err := kubernetes.NewForConfig(config.GenericConfig.LoopbackClientConfig) | ||
patchContext.postStartHooks["quota.openshift.io-clusterquotamapping"] = func(context genericapiserver.PostStartHookContext) error { | ||
go clusterQuotaMappingController.Run(5, context.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.
any deeper reason why this is now a post start hook and not a plugin initializer?
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.
any deeper reason why this is now a post start hook and not a plugin initializer?
There's a controller piece that neesd to run
@@ -137,15 +171,6 @@ func (c *KubeAPIServerServerPatchContext) PatchServer(server *master.Master) err | |||
} | |||
return nil | |||
}) | |||
server.GenericAPIServer.AddPostStartHookOrDie("openshift.io-restmapperupdater", func(context 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.
where is this gone? Why don't we need that anymore?
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.
where is this gone? Why don't we need that anymore?
we use the upstream restmapper now.
|
||
return func(apiHandler http.Handler, genericConfig *genericapiserver.Config) http.Handler { | ||
// Machinery that let's use discover the Web Console Public URL | ||
accessor := newWebConsolePublicURLAccessor(genericConfig.LoopbackClientConfig) | ||
go accessor.Run(stopCh) | ||
extraPostStartHooks["openshift.io-webconsolepublicurl"] = func(context 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.
this is (and was) dirty and deserves a big, bold explanation why we do this here.
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.
this is (and was) dirty and deserves a big, bold explanation why we do this here.
No worse than before, but I can comment it.
comments addressed in a separate commit. |
bootstrappolicy.ClusterRoles = bootstrappolicy.OpenshiftClusterRoles | ||
bootstrappolicy.ClusterRoleBindings = bootstrappolicy.OpenshiftClusterRoleBindings | ||
|
||
options.AllOrderedPlugins = originadmission.CombinedAdmissionControlPlugins |
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.
ugly to override global slice vars meant as constants. But I guess we have no choice.
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.
a carry patch might be more explicit.
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.
a carry patch might be more explicit.
doing so doesn't allow running in both modes and exposes us to greater conflicts
@@ -78,7 +79,8 @@ func createAggregatorConfig( | |||
// copy the etcd options so we don't mutate originals. | |||
etcdOptions := *commandOptions.Etcd | |||
etcdOptions.StorageConfig.Codec = aggregatorscheme.Codecs.LegacyCodec(v1beta1.SchemeGroupVersion, v1.SchemeGroupVersion) | |||
genericConfig.RESTOptionsGetter = &genericoptions.SimpleRestOptionsFactory{Options: etcdOptions} | |||
aggregatorinstall.Install(legacyscheme.Scheme) | |||
//genericConfig.RESTOptionsGetter = &genericoptions.SimpleRestOptionsFactory{Options: etcdOptions} |
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? This needs a comment
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? This needs a comment
added
@@ -30,6 +30,10 @@ import ( | |||
"k8s.io/kubernetes/cmd/kube-apiserver/app/options" | |||
) | |||
|
|||
func createAPIExtensionsServer(apiextensionsConfig *apiextensionsapiserver.Config, delegateAPIServer genericapiserver.DelegationTarget) (*apiextensionsapiserver.CustomResourceDefinitions, error) { | |||
return apiextensionsConfig.Complete().New(delegateAPIServer) |
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.
If I trust my eyes, this is only an (unnecessary) move?
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.
If I trust my eyes, this is only an (unnecessary) move?
fixed
@@ -179,6 +179,9 @@ func CreateServerChain(completedOptions completedServerRunOptions, stopCh <-chan | |||
return nil, err | |||
} | |||
|
|||
if err := PatchKubeAPIServerServer(kubeAPIServer); err != 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.
drop if or handle 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.
drop if or handle error
fixed
@@ -634,6 +634,8 @@ func BuildStorageFactory(s *options.ServerRunOptions, apiResourceConfig *servers | |||
return nil, fmt.Errorf("error in initializing storage factory: %s", err) | |||
} | |||
|
|||
storageFactory.SetResourceEtcdPrefix(schema.GroupResource{Group: "apiregistration.k8s.io", Resource: "apiservices"}, "apiservices") |
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.
what's the history of this? Did we ship an earlier version with a wrong etcd path?
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.
what's the history of this? Did we ship an earlier version with a wrong etcd path?
Yes, in 3.6
f10076b
to
5e13f18
Compare
Still quite some ugly hacks. But it's a good step into the right direction 👍 |
/retest |
1 similar comment
/retest |
@sttts I've come around. This is where my head's at. Not pretty, but I think we can fit most everything into those two spots.