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

Bring back applying labels to services deployed with helm #2568

Merged
merged 4 commits into from
Jul 30, 2019

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Jul 30, 2019

Fixes #2379

In #965, we turned off labelling services due to kubernetes/kubernetes#68087
This bug is in the kube-controller-manager which runs on the cluster master side.
I tested this PR on my cluster with 1.11.7-gke.4 and port-forwarding for LoadBalancer type services works.

Impacted versions for this bug

GKE cluster Version State Notes
v1.12.0 and onwards Never Occurred As per the Changelog for v1.12.0 here, this bug never appeared in 1.12.0 . The very first version lists kubernetes/kubernetes#68087 being fixed in notes. See L1243
v1.11.x Occurred in v1.11.3 and before This fix was patched in v1.11.4 as per the Changelog for for v1.11.0 here. As per release notes on Jan 2019, all cluster masters running 1.11.2 through 1.11.4 will be upgraded to 1.11.5-gke.5. which should contain the fix.

As per GKE version skew policy, June 2019 release notes mention GKE will only support the three newest minor versions.
So in june end, for all node pools older than v1.11.X Google will enable node auto-upgrade and these nodes will be updated to 1.11.10-gke.5. ( we are v1.14 now)

In conclusion, i feel this is safe way to enable port-forwarding for services deployed by helm (helm2) to get in.

@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #2568 into master will increase coverage by 0.89%.
The diff coverage is n/a.

Impacted Files Coverage Δ
pkg/skaffold/deploy/labels.go 16.88% <ø> (+0.83%) ⬆️
pkg/skaffold/runner/build_deploy.go 65.9% <0%> (-2.35%) ⬇️
pkg/skaffold/util/util.go 85.91% <0%> (-0.76%) ⬇️
pkg/skaffold/docker/image_util.go 0% <0%> (ø) ⬆️
pkg/skaffold/jib/jib_init.go 89.39% <0%> (ø)
pkg/skaffold/sync/kubectl/kubectl.go 9.67% <0%> (ø)
pkg/skaffold/runner/context/context.go 62.5% <0%> (ø)
pkg/skaffold/debug/debug.go 45% <0%> (+1.66%) ⬆️
pkg/skaffold/docker/client.go 76.29% <0%> (+2.55%) ⬆️
pkg/skaffold/build/local/types.go 71.42% <0%> (+2.85%) ⬆️
... and 8 more

Copy link
Contributor

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

LGTM -- should we log a warning if the kubernetes version is lower than 1.11.3 and we are deploying with helm? Since we do that for kubectl version already.

@balopat
Copy link
Contributor

balopat commented Jul 30, 2019

I wouldn't add a warning - if someone sees the issue, we should just ask them to upgrade to 1.11.4+

@balopat balopat merged commit 54d1a23 into master Jul 30, 2019
@balopat balopat deleted the gke_load_balancer_svc branch August 2, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port forwarding broken for helm releases
4 participants