-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Changes from 3 commits
831d8fb
9efd4f2
36a6c5d
30f41e8
6c35dfa
c113257
3e03d54
9c9d4ae
014a93e
c0b2caf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For an example test case: That said, it is a race condition, and may not be easy to trigger. Better to just fail open. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
|
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.
+1 to filtering kube-system out.