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

Make sure gcp-auth addon can be enabled on startup #9318

Merged
merged 10 commits into from
Sep 29, 2020
7 changes: 7 additions & 0 deletions deploy/addons/gcp-auth/gcp-auth-webhook.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,17 @@ metadata:
app: gcp-auth
webhooks:
- name: gcp-auth-mutate.k8s.io
failurePolicy: Fail
objectSelector:
matchExpressions:
- key: gcp-auth-skip-secret
operator: DoesNotExist
namespaceSelector:
matchExpressions:
- key: name
operator: NotIn
values:
- kube-system
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to filtering kube-system out.

sideEffects: None
admissionReviewVersions: ["v1","v1beta1"]
clientConfig:
Expand Down
50 changes: 45 additions & 5 deletions pkg/addons/addons.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,18 @@ func verifyAddonStatus(cc *config.ClusterConfig, name string, val string) error
}

func verifyGCPAuthAddon(cc *config.ClusterConfig, name string, val string) error {
return verifyAddonStatusInternal(cc, name, val, "gcp-auth")
enable, err := strconv.ParseBool(val)
if err != nil {
return errors.Wrapf(err, "parsing bool: %s", name)
}
err = verifyAddonStatusInternal(cc, name, val, "gcp-auth")

if enable && err == nil {
out.T(style.Notice, "Your GCP credentials will now be mounted into every pod created in the {{.name}} cluster.", out.V{"name": cc.Name})
out.T(style.Notice, "If you don't want your credentials mounted into a specific pod, add a label with the `gcp-auth-skip-secret` key to your pod configuration.")
}

return err
}

func verifyAddonStatusInternal(cc *config.ClusterConfig, name string, val string, ns string) error {
Expand Down Expand Up @@ -394,24 +405,53 @@ func Start(wg *sync.WaitGroup, cc *config.ClusterConfig, toEnable map[string]boo

var awg sync.WaitGroup

defer func() { // making it show after verifications( not perfect till #7613 is closed)
deferredAddons := []string{}
enabledAddons := []string{}

defer func() { // making it show after verifications (see #7613)
register.Reg.SetStep(register.EnablingAddons)
out.T(style.AddonEnable, "Enabled addons: {{.addons}}", out.V{"addons": strings.Join(toEnableList, ", ")})
out.T(style.AddonEnable, "Enabled addons: {{.addons}}", out.V{"addons": enabledAddons})
}()
var addonErr error
for _, a := range toEnableList {
// Run the gcp-auth addon last to make sure everything else is up
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still a race condition, as addon enablement does not block until the pods are available.

I think you can remove this special case by naming a failurePolicy for the webhook instead, so that it fails open if unavailable:

https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#failure-policy

Copy link
Contributor

Choose a reason for hiding this comment

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

For an example test case: /out/minikube start --addons=dashboard --addons=efk --addons=freshpod --addons=istio --addons=istio-provisioner --addons=olm --memory=16384 --driver=hyperkit

That said, it is a race condition, and may not be easy to trigger. Better to just fail open.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think adding the failurePolicy is a good idea just in general, but this code isn't supposed to eliminate the race, just reduce it as much as possible. In my testing it seems to be very effectively at precisely that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that failurePolicy is in place, can this ordering code be safely removed?

The nice thing about failurePolicy is that it also eliminates the race condition when you use minikube addons enable while another pod is scheduling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so just in my empirical testing, the race condition still happens fairly often without this code, which makes me think I don't quite understand what's happening here.

if a == "gcp-auth" {
deferredAddons = append(deferredAddons, a)
continue
}
awg.Add(1)
go func(name string) {
err := func(name string) error {
err := RunCallbacks(cc, name, "true")
if err != nil {
out.WarningT("Enabling '{{.name}}' returned an error: {{.error}}", out.V{"name": name, "error": err})
}
awg.Done()
return err
}(a)

if err != nil {
addonErr = err
} else {
enabledAddons = append(enabledAddons, a)
}
}

// Wait until all of the addons are enabled before updating the config (not thread safe)
awg.Wait()
for _, a := range toEnableList {

// Don't bother trying more addons if one of the default ones failed
if addonErr == nil {
for _, a := range deferredAddons {
err := RunCallbacks(cc, a, "true")
if err != nil {
out.WarningT("Enabling '{{.name}}' returned an error: {{.error}}", out.V{"name": a, "error": err})
} else {
enabledAddons = append(enabledAddons, a)
}
}
}

for _, a := range enabledAddons {
if err := Set(cc, a, "true"); err != nil {
glog.Errorf("store failed: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/addons/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ var Addons = []*Addon{
{
name: "gcp-auth",
set: SetBool,
callbacks: []setFn{gcpauth.EnableOrDisable, enableOrDisableAddon, verifyGCPAuthAddon, gcpauth.DisplayAddonMessage},
callbacks: []setFn{gcpauth.EnableOrDisable, enableOrDisableAddon, verifyGCPAuthAddon},
},
{
name: "volumesnapshots",
Expand Down
15 changes: 1 addition & 14 deletions pkg/addons/gcpauth/enable.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func enableAddon(cfg *config.ClusterConfig) error {
ctx := context.Background()
creds, err := google.FindDefaultCredentials(ctx)
if err != nil {
exit.Message(reason.InternalCredsNotFound, "Could not find any GCP credentials. Either run `gcloud auth login` or set the GOOGLE_APPLICATION_CREDENTIALS environment variable to the path of your credentials file.")
exit.Message(reason.InternalCredsNotFound, "Could not find any GCP credentials. Either run `gcloud auth application-default login` or set the GOOGLE_APPLICATION_CREDENTIALS environment variable to the path of your credentials file.")
}

f := assets.NewMemoryAssetTarget(creds.JSON, credentialsPath, "0444")
Expand Down Expand Up @@ -116,16 +116,3 @@ func disableAddon(cfg *config.ClusterConfig) error {

return nil
}

// DisplayAddonMessage display an gcp auth addon specific message to the user
func DisplayAddonMessage(cfg *config.ClusterConfig, name string, val string) error {
enable, err := strconv.ParseBool(val)
if err != nil {
return errors.Wrapf(err, "parsing bool: %s", name)
}
if enable {
out.T(style.Notice, "Your GCP credentials will now be mounted into every pod created in the {{.name}} cluster.", out.V{"name": cfg.Name})
out.T(style.Notice, "If you don't want your credentials mounted into a specific pod, add a label with the `gcp-auth-skip-secret` key to your pod configuration.")
}
return nil
}
2 changes: 1 addition & 1 deletion site/content/en/docs/handbook/addons/gcp-auth.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ weight: 1
date: 2020-07-15
---

If you have a containerized GCP app with a Kubernetes yaml, you can automatically add your credentials to all your deployed pods dynamically with this minikube addon. You just need to have a credentials file, which can be generated with `gcloud auth login`. If you already have a json credentials file you want specify, use the GOOGLE_APPLICATION_CREDENTIALS environment variable.
If you have a containerized GCP app with a Kubernetes yaml, you can automatically add your credentials to all your deployed pods dynamically with this minikube addon. You just need to have a credentials file, which can be generated with `gcloud auth application-default login`. If you already have a json credentials file you want specify, use the GOOGLE_APPLICATION_CREDENTIALS environment variable.

- Start a cluster:
```
Expand Down