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.

Sign up for GitHub

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

support k8s < v1.19 & watch-ingress-without-class #12794

Merged
merged 2 commits into from
Oct 28, 2021
Merged

support k8s < v1.19 & watch-ingress-without-class #12794

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76c1e79
support k8s < v1.19 & watch-ingress-without-class
prezha Oct 27, 2021
ea3aa4f
support ingress-dns addon, add tests
prezha Oct 28, 2021
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
4 changes: 2 additions & 2 deletions deploy/addons/ingress/ingress-deploy.yaml.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,12 +365,12 @@ spec:
- --election-id=ingress-controller-leader
{{- if eq .IngressAPIVersion "v1"}}
- --controller-class=k8s.io/ingress-nginx
- --watch-ingress-without-class=true
{{- end}}
{{- if eq .IngressAPIVersion "v1beta1"}}
- --ingress-class=nginx
- --watch-ingress-without-class=true
- --publish-status-address=localhost
{{- end}}
- --publish-status-address=localhost
- --configmap=$(POD_NAMESPACE)/ingress-nginx-controller
- --report-node-internal-ip-address
- --tcp-services-configmap=$(POD_NAMESPACE)/tcp-services
Expand Down
25 changes: 10 additions & 15 deletions pkg/addons/addons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,13 @@ func EnableOrDisableAddon(cc *config.ClusterConfig, name string, val string) err
exit.Error(reason.GuestCpConfig, "Error getting primary control plane", err)
}

// maintain backwards compatibility for ingress with k8s < v1.19
if strings.HasPrefix(name, "ingress") && enable {
if err := supportLegacyIngress(addon, *cc); err != nil {
return err
}
}

// Persist images even if the machine is running so starting gets the correct images.
images, customRegistries, err := assets.SelectAndPersistImages(addon, cc)
if err != nil {
Expand Down Expand Up @@ -229,9 +236,6 @@ func addonSpecificChecks(cc *config.ClusterConfig, name string, enable bool, run
out.Styled(style.Tip, `After the addon is enabled, please run "minikube tunnel" and your ingress resources would be available at "127.0.0.1"`)
}
}
if err := supportLegacyIngress(cc); err != nil {
return false, err
}
}

if strings.HasPrefix(name, "istio") && enable {
Expand Down Expand Up @@ -290,29 +294,20 @@ func isAddonAlreadySet(cc *config.ClusterConfig, addon *assets.Addon, enable boo
return false
}

// maintain backwards compatibility with k8s < v1.19
// by replacing images with old versions if custom ones are not already provided
func supportLegacyIngress(cc *config.ClusterConfig) error {
// maintain backwards compatibility for ingress with k8s < v1.19 by replacing default addon images with older versions
func supportLegacyIngress(addon *assets.Addon, cc config.ClusterConfig) error {
v, err := util.ParseKubernetesVersion(cc.KubernetesConfig.KubernetesVersion)
if err != nil {
return errors.Wrap(err, "parsing Kubernetes version")
}
if semver.MustParseRange("<1.19.0")(v) {
imgs := map[string]string{
addon.Images = map[string]string{
// /~https://github.com/kubernetes/ingress-nginx/blob/0a2ec01eb4ec0e1b29c4b96eb838a2e7bfe0e9f6/deploy/static/provider/kind/deploy.yaml#L328
"IngressController": "ingress-nginx/controller:v0.49.3@sha256:35fe394c82164efa8f47f3ed0be981b3f23da77175bbb8268a9ae438851c8324",
// issues: /~https://github.com/kubernetes/ingress-nginx/issues/7418 and /~https://github.com/jet/kube-webhook-certgen/issues/30
"KubeWebhookCertgenCreate": "docker.io/jettech/kube-webhook-certgen:v1.5.1@sha256:950833e19ade18cd389d647efb88992a7cc077abedef343fa59e012d376d79b7",
"KubeWebhookCertgenPatch": "docker.io/jettech/kube-webhook-certgen:v1.5.1@sha256:950833e19ade18cd389d647efb88992a7cc077abedef343fa59e012d376d79b7",
}
if cc.CustomAddonImages == nil {
Copy link
Member

@medyagh medyagh Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I could be wrong but @prezha does this mean the cutsom addon image wont work for ingress ? why we are removing this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@medyagh in my first attempt to maintain backward compatibility (pr #12325), i used cc.CustomAddonImages to pass compatible images but only in case that those were not already set by the user

that would not work because of the logic in assets.SelectAndPersistImages overrides it - expecting to read custom images from the viper/flags (not from the cc):

minikube/pkg/minikube/assets/addons.go

Lines 735 to 747 in 418a9a1

newImages := parseMapString(viper.GetString(config.AddonImages))
for name, image := range newImages {
if image == "" {
out.WarningT("Ignoring empty custom image {{.name}}", out.V{"name": name})
delete(newImages, name)
continue
}
if _, ok := addonDefaultImages[name]; !ok {
out.WarningT("Ignoring unknown custom image {{.name}}", out.V{"name": name})
}
}
// Use newly configured custom images.
images = overrideDefaults(addonDefaultImages, newImages)

hence, here i've amended the addons.supportLegacyIngress to replace instead the default images to those compatible if the older k8s was used, ie, without using or affecting in any way the cc - the custom images user could have provided via flags, so that should continue to work as before

on a side note, i think the current logic in assets.SelectAndPersistImages mentioned above could be problematic as it could override the custom images that were defined in the first run with the default ones in subsequent runs if those that follow would not use the same flag providing initial custom images, same could be for the custom registries, but if an issue at all, it would be another story :)

cc.CustomAddonImages = map[string]string{}
}
for name, path := range imgs {
if _, exists := cc.CustomAddonImages[name]; !exists {
cc.CustomAddonImages[name] = path
}
}
}
return nil
}
Expand Down