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

Put EnableInjectionOrDie back on the main path #1772

Merged
merged 9 commits into from
Oct 6, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions injection/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
Copyright 2020 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package injection

import (
"errors"
"flag"
"log"
"os"
"os/user"
"path/filepath"

"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/klog"
)

// ParseAndGetRestConfigOrDie parses the rest config flags and creates a client or
// dies by calling log.Fatalf.
func ParseAndGetRestConfigOrDie() *rest.Config {
n3wscott marked this conversation as resolved.
Show resolved Hide resolved
var (
serverURL = flag.String("server", "",
"The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
kubeconfig = flag.String("kubeconfig", "",
"Path to a kubeconfig. Only required if out-of-cluster.")
)
klog.InitFlags(flag.CommandLine)
flag.Parse()

cfg, err := GetRestConfig(*serverURL, *kubeconfig)
if err != nil {
log.Fatal("Error building kubeconfig: ", err)
}

return cfg
}

// GetConfig returns a rest.Config to be used for kubernetes client creation.
// It does so in the following order:
// 1. Use the passed kubeconfig/serverURL.
// 2. Fallback to the KUBECONFIG environment variable.
// 3. Fallback to in-cluster config.
// 4. Fallback to the ~/.kube/config.
func GetRestConfig(serverURL, kubeconfig string) (*rest.Config, error) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: REST

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ick, how about GetKubeconfig

Copy link
Member

Choose a reason for hiding this comment

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

it is getting a rest.Config...

if kubeconfig == "" {
kubeconfig = os.Getenv("KUBECONFIG")
}

// We produce configs a bunch of ways, this gives us a single place
// to "decorate" them with common useful things (e.g. for debugging)
decorate := func(cfg *rest.Config) *rest.Config {
return cfg
}
n3wscott marked this conversation as resolved.
Show resolved Hide resolved

// If we have an explicit indication of where the kubernetes config lives, read that.
if kubeconfig != "" {
c, err := clientcmd.BuildConfigFromFlags(serverURL, kubeconfig)
if err != nil {
return nil, err
}
return decorate(c), nil
}
// If not, try the in-cluster config.
if c, err := rest.InClusterConfig(); err == nil {
return decorate(c), nil
}
// If no in-cluster config, try the default location in the user's home directory.
if usr, err := user.Current(); err == nil {
if c, err := clientcmd.BuildConfigFromFlags("", filepath.Join(usr.HomeDir, ".kube", "config")); err == nil {
return decorate(c), nil
}
}

return nil, errors.New("could not create a valid kubeconfig")
}
66 changes: 66 additions & 0 deletions injection/injection.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
Copyright 2019 The Knative Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package injection

import (
"context"

"go.uber.org/zap"
"k8s.io/client-go/rest"

"knative.dev/pkg/controller"
"knative.dev/pkg/logging"
"knative.dev/pkg/signals"
)

// EnableInjectionOrDie enables Knative Client Injection, and provides a
// callback to start the informers. Both Context and Config are optional.
// Returns context with rest config set and a callback to start the informers
// after watches have been set.
//
// Typical integration:
// ```go
// ctx, startInformers := injection.EnableInjectionOrDie(signals.NewContext(), nil)
// ... start watches with informers, if required ...
// startInformers()
// ```
func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) (context.Context, func()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me, though the name is a little bit confusing as the original EnableInjectionOrDie is exported too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that is intended, the original one is in a different package and marked deprecated.

if ctx == nil {
ctx = signals.NewContext()
}
if cfg == nil {
cfg = ParseAndGetRestConfigOrDie()
}

// Respect user provided settings, but if omitted customize the default behavior.
if cfg.QPS == 0 {
cfg.QPS = rest.DefaultQPS
}
if cfg.Burst == 0 {
cfg.Burst = rest.DefaultBurst
}
ctx = WithConfig(ctx, cfg)

ctx, informers := Default.SetupInformers(ctx, cfg)

return ctx, func() {
logging.FromContext(ctx).Info("Starting informers...")
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
}
}
39 changes: 15 additions & 24 deletions injection/sharedmain/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ import (
// 2. Fallback to the KUBECONFIG environment variable.
// 3. Fallback to in-cluster config.
// 4. Fallback to the ~/.kube/config.
// Deprecated: use injection.GetRestConfig
func GetConfig(serverURL, kubeconfig string) (*rest.Config, error) {
if kubeconfig == "" {
kubeconfig = os.Getenv("KUBECONFIG")
Expand Down Expand Up @@ -129,12 +130,13 @@ func GetLeaderElectionConfig(ctx context.Context) (*leaderelection.Config, error

// EnableInjectionOrDie enables Knative Injection and starts the informers.
// Both Context and Config are optional.
// Deprecated: use injection.EnableInjectionOrDie
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this method to:

   ctx, si := injection.EnableInjectionOrDie(ctx, cfg)
   go si()
   return ctx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good idea!

func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context {
if ctx == nil {
ctx = signals.NewContext()
}
if cfg == nil {
cfg = ParseAndGetConfigOrDie()
cfg = injection.ParseAndGetRestConfigOrDie()
}

// Respect user provided settings, but if omitted customize the default behavior.
Expand All @@ -148,21 +150,19 @@ func EnableInjectionOrDie(ctx context.Context, cfg *rest.Config) context.Context

ctx, informers := injection.Default.SetupInformers(ctx, cfg)

// Start the injection clients and informers.
logging.FromContext(ctx).Info("Starting informers...")
go func(ctx context.Context) {
go func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this go function? I think it is safer to let it finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be deleted asap after knative rolls to the new one, shared main now calls down to the new version. this method is not changed and is legacy.

Copy link
Contributor

Choose a reason for hiding this comment

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

My original question is about using go func or not in the old function, but I guess the whole function will be removed anyway.

logging.FromContext(ctx).Info("Starting informers...")
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
<-ctx.Done()
}(ctx)
}()

return ctx
}

// Main runs the generic main flow with a new context.
// If any of the contructed controllers are AdmissionControllers or Conversion webhooks,
// then a webhook is started to serve them.
// If any of the constructed controllers are AdmissionControllers or Conversion
// webhooks, then a webhook is started to serve them.
func Main(component string, ctors ...injection.ControllerConstructor) {
// Set up signals so we handle the first shutdown signal gracefully.
MainWithContext(signals.NewContext(), component, ctors...)
Expand All @@ -185,7 +185,7 @@ func MainWithContext(ctx context.Context, component string, ctors ...injection.C
"issue upstream!")

// HACK: This parses flags, so the above should be set once this runs.
cfg := ParseAndGetConfigOrDie()
cfg := injection.ParseAndGetRestConfigOrDie()

if *disableHighAvailability {
ctx = WithHADisabled(ctx)
Expand Down Expand Up @@ -225,16 +225,7 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
cfg.Burst = len(ctors) * rest.DefaultBurst
}

// Respect user provided settings, but if omitted customize the default behavior.
if cfg.QPS == 0 {
cfg.QPS = rest.DefaultQPS
}
if cfg.Burst == 0 {
cfg.Burst = rest.DefaultBurst
}
ctx = injection.WithConfig(ctx, cfg)

ctx, informers := injection.Default.SetupInformers(ctx, cfg)
ctx, startInformers := injection.EnableInjectionOrDie(ctx, cfg)

logger, atomicLevel := SetupLoggerOrDie(ctx, component)
defer flush(logger)
Expand Down Expand Up @@ -287,11 +278,10 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
return wh.Run(ctx.Done())
})
}

// Start the injection clients and informers.
logging.FromContext(ctx).Info("Starting informers...")
if err := controller.StartInformers(ctx.Done(), informers...); err != nil {
logging.FromContext(ctx).Fatalw("Failed to start informers", zap.Error(err))
}
startInformers()

// Wait for webhook informers to sync.
if wh != nil {
wh.InformersHaveSynced()
Expand All @@ -317,6 +307,7 @@ func flush(logger *zap.SugaredLogger) {

// ParseAndGetConfigOrDie parses the rest config flags and creates a client or
// dies by calling log.Fatalf.
// Deprecated: use injeciton.ParseAndGetRestConfigOrDie
func ParseAndGetConfigOrDie() *rest.Config {
var (
serverURL = flag.String("server", "",
Expand All @@ -327,7 +318,7 @@ func ParseAndGetConfigOrDie() *rest.Config {
klog.InitFlags(flag.CommandLine)
flag.Parse()

cfg, err := GetConfig(*serverURL, *kubeconfig)
cfg, err := injection.GetRestConfig(*serverURL, *kubeconfig)
if err != nil {
log.Fatal("Error building kubeconfig: ", err)
}
Expand Down
6 changes: 3 additions & 3 deletions leaderelection/chaosduck/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,14 @@ import (
"strings"
"time"

"knative.dev/pkg/injection"

"golang.org/x/sync/errgroup"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/client-go/kubernetes"
kubeclient "knative.dev/pkg/client/injection/kube/client"
"knative.dev/pkg/injection/sharedmain"
"knative.dev/pkg/kflag"
"knative.dev/pkg/signals"
"knative.dev/pkg/system"
Expand Down Expand Up @@ -116,8 +117,7 @@ func quack(ctx context.Context, kc kubernetes.Interface, component string, leade
}

func main() {
ctx := signals.NewContext()
ctx = sharedmain.EnableInjectionOrDie(ctx, nil)
ctx, _ := injection.EnableInjectionOrDie(signals.NewContext(), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have to do this, that means we are really trying to avoid calling the original function. I would recommend to have a roadmap to deprecate the original function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is marked deprecated and documented redirected to the new one version.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's awesome. Is this on purpose not to call the start function here? Note that the original logic will have informers started here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might skip your attention. Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked and the duck does not use informers so it is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking.


kc := kubeclient.Get(ctx)

Expand Down