dependencies: Upgrade all k8s client-go dependent sources to v1.18.X #1627

josephglanville · 2020-06-12T10:57:22Z

This requires pinning grpc for now as istio client-go otherwise pulls in
breaking changes.

Required to make progress on #1366

k8s-ci-robot · 2020-06-12T10:57:25Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-06-12T10:57:29Z

Welcome @josephglanville!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

josephglanville · 2020-06-12T11:00:15Z

Signed the CLA.

tariq1890 · 2020-06-12T16:37:39Z

go.mod

@@ -70,5 +75,6 @@ replace (
 	github.com/Azure/go-autorest/autorest/adal => github.com/Azure/go-autorest/autorest/adal v0.6.0
 	github.com/Azure/go-autorest/autorest/azure/auth => github.com/Azure/go-autorest/autorest/azure/auth v0.3.0
 	github.com/golang/glog => github.com/kubermatic/glog-logrus v0.0.0-20180829085450-3fa5b9870d1d
+	google.golang.org/grpc => google.golang.org/grpc v1.26.0


We are trying to avoid these. Do you know where the conflict arises?

Problem stems from backwards incompatible changes to gRPC that most dependencies have not upgraded past yet, resulting in errors such as these:

# github.com/coreos/etcd/clientv3/balancer/resolver/endpoint ../../go/pkg/mod/github.com/coreos/etcd@v3.3.22+incompatible/clientv3/balancer/resolver/endpoint/endpoint.go:114:78: undefined: resolver.BuildOption ../../go/pkg/mod/github.com/coreos/etcd@v3.3.22+incompatible/clientv3/balancer/resolver/endpoint/endpoint.go:182:31: undefined: resolver.ResolveNowOption github.com/coreos/etcd/pkg/types github.com/coreos/etcd/clientv3/balancer/connectivity github.com/coreos/etcd/clientv3/balancer/picker github.com/coreos/etcd/version # github.com/coreos/etcd/clientv3/balancer/picker ../../go/pkg/mod/github.com/coreos/etcd@v3.3.22+incompatible/clientv3/balancer/picker/err.go:37:44: undefined: balancer.PickOptions ../../go/pkg/mod/github.com/coreos/etcd@v3.3.22+incompatible/clientv3/balancer/picker/roundrobin_balanced.go:55:54: undefined: balancer.PickOptions

Unfortunately the very same commit of the Istio client that upgrades to v1.18.x also bumps gRPC to v1.28 causing misery unless pinned back so the rest of the code continues to function.

Upstream issue in etcd regarding the API change: etcd-io/etcd#11563

It might be possible to pin the etcd client instead but I haven't tried that as it has a larger surface area and imported via more than one package path due to change from coreos org. Regardless at least one pin will be required to get Contour, Istio and Linode dependencies to play nice it's just a matter of which one.

Got it. Can we add a TODO comment, just so that we can get back to it at a later time?

tariq1890 · 2020-06-12T16:39:20Z

@josephglanville Just a note regarding context.TODO(). We use TODO only in places where it's actually a TODO item to insert context. In all other places, we just stick to context.Background()

josephglanville · 2020-06-12T17:52:26Z

Switched context.TODO() to context.Background().

josephglanville · 2020-06-17T04:35:08Z

@tariq1890 any chance you could take another look at this? If the gRPC version pin isn't desirable could you suggest other options that would be amenable?

njuettner · 2020-06-17T07:13:22Z

source/gateway.go

@@ -117,7 +117,7 @@ func NewIstioGatewaySource(
 // Endpoints returns endpoint objects for each host-target combination that should be processed.
 // Retrieves all gateway resources in the source's namespace(s).
 func (sc *gatewaySource) Endpoints() ([]*endpoint.Endpoint, error) {
-	gwList, err := sc.istioClient.NetworkingV1alpha3().Gateways(sc.namespace).List(metav1.ListOptions{})
+	gwList, err := sc.istioClient.NetworkingV1alpha3().Gateways(sc.namespace).List(context.Background(), metav1.ListOptions{})


what about adding the context in the gatewaySource?

I can look at that sure.

Thinking about this more I think there is a larger task to plumb down the context from main -> store -> sources. I think that makes more sense as a follow up to this but I could do it in this PR if that is desired.

@njuettner this was done.

josephglanville · 2020-06-20T09:14:23Z

I added context.Context to the Endpoints() function on the Source interface. This allows sources to make use of cancelation on shutdown similar to providers.

I will rebase #1628 on this so it can also make use of it.

josephglanville · 2020-06-20T09:40:26Z

/assign @Raffo

josephglanville · 2020-06-24T19:12:13Z

@tariq1890 @Raffo @njuettner could you please review these changes?

tariq1890 · 2020-06-27T00:32:42Z

I've had an in-depth look into this. LGTM expect for that one minor comment @josephglanville

Sorry for the delay. Also, note that I am just a contributor to the project. It's ultimately upto @njuettner and @Raffo to approve the changes.

Raffo · 2020-06-28T12:08:47Z

@njuettner marking this to review next week.

Raffo · 2020-07-01T07:55:24Z

We're not going to have this merged as this, as it is today, includes a very recent version of client-go (0.18) which is not even listed in the compatibility matrix from https://github.com/kubernetes/client-go . We will always keep compatibility with officially supported kubernetes version (AFAIK 1.15 is the latest supported as of now) and also with what the major cloud providers will do. EKS, for example, supports 1.14 (hopefully not for long 😄 ). This means that we will have to postpone this change.
We can think of changing this to a version of client go, but it doesn't fix the problem with contour.

josephglanville · 2020-07-01T14:22:36Z

As I understand it client-go 1.18.X support 1.15.X, in-fact I am using this code with a 1.15.X cluster right now as that is what we are on for the foreseeable future. I have yet to experience any problems with compatibility.

Is this just a prudence measure or do you actually think there is a compatibility problem with 1.18.X and 1.15.X/1.14.X that needs looking into?

Without this upgrade work on the Contour HTTPProxy source can't reasonably continue without falling back to specifying the load balancer IPs directly as is done in the IngressRoute source (which won't work for our use-case which has multiple Contour deployments with multiple LBs).

josephglanville · 2020-07-01T22:49:28Z

Created an issue on client-go to update the compatibility matrix: kubernetes/client-go#828

Raffo · 2020-07-02T16:37:28Z

Thanks @josephglanville , I'll wait for client-go to have clear info published and re-assess this later.

tariq1890 · 2020-07-02T16:51:41Z

I agree with @josephglanville here. I wouldn't be too worried about compatibility issues with importing client-go v1.18.x

This is a definitely case of the maintainers not updating their documentation

This requires pinning grpc for now as istio client-go otherwise pulls in breaking changes.

josephglanville · 2020-07-03T04:33:59Z

PR to update compat matrix in client-go: kubernetes/client-go#821

josephglanville · 2020-07-03T07:43:28Z

@Raffo client-go matrix is now updated.

nikhita · 2020-07-03T07:44:25Z

I wouldn't be too worried about compatibility issues with importing client-go v1.18.x

This is a definitely case of the maintainers not updating their documentation

Fwiw confirming from kubernetes/client-go#821 that it was an indeed an oversight to miss adding the support for 1.18.x in the compatibility matrix in client-go. Apologies!

The README now reflects 1.18 as well. 👍

Raffo · 2020-07-03T17:45:53Z

Thanks for the update, I'll check this again next week and give an update.

Raffo · 2020-07-08T08:03:03Z

/lgtm
/approve

k8s-ci-robot · 2020-07-08T08:03:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: josephglanville, Raffo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Raffo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 12, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2020

k8s-ci-robot requested review from hjacobs and linki June 12, 2020 10:57

josephglanville force-pushed the jpg/upgrade-versions branch from 3f25400 to d649932 Compare June 12, 2020 12:07

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 12, 2020

josephglanville force-pushed the jpg/upgrade-versions branch from d649932 to 8be87fc Compare June 12, 2020 12:17

tariq1890 reviewed Jun 12, 2020

View reviewed changes

josephglanville force-pushed the jpg/upgrade-versions branch from 8be87fc to 6b5ce44 Compare June 12, 2020 17:52

josephglanville force-pushed the jpg/upgrade-versions branch from 6b5ce44 to 628ed59 Compare June 12, 2020 17:53

josephglanville mentioned this pull request Jun 15, 2020

Upgrade k8s client API to 1.18+ SeldonIO/seldon-core#1949

Closed

njuettner reviewed Jun 17, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2020

josephglanville force-pushed the jpg/upgrade-versions branch from 628ed59 to e0c2c35 Compare June 20, 2020 09:08

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2020

k8s-ci-robot assigned Raffo Jun 20, 2020

josephglanville force-pushed the jpg/upgrade-versions branch from e0c2c35 to 6d84913 Compare June 23, 2020 10:36

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020

josephglanville force-pushed the jpg/upgrade-versions branch from 6d84913 to f154118 Compare June 24, 2020 19:05

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020

tariq1890 approved these changes Jun 27, 2020

View reviewed changes

josephglanville force-pushed the jpg/upgrade-versions branch from f154118 to 1cd1bd3 Compare June 27, 2020 14:02

josephglanville force-pushed the jpg/upgrade-versions branch from 1cd1bd3 to 03e3840 Compare July 3, 2020 04:14

dependencies: Upgrade all k8s client-go dependent sources to v1.18.X

6efca13

This requires pinning grpc for now as istio client-go otherwise pulls in breaking changes.

josephglanville force-pushed the jpg/upgrade-versions branch from 03e3840 to 6efca13 Compare July 3, 2020 04:30

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020

k8s-ci-robot merged commit b1a2f0b into kubernetes-sigs:master Jul 8, 2020

josephglanville mentioned this pull request Jul 8, 2020

Contour HTTPProxy support #1628

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dependencies: Upgrade all k8s client-go dependent sources to v1.18.X #1627

dependencies: Upgrade all k8s client-go dependent sources to v1.18.X #1627

josephglanville commented Jun 12, 2020

k8s-ci-robot commented Jun 12, 2020

k8s-ci-robot commented Jun 12, 2020

josephglanville commented Jun 12, 2020

tariq1890 Jun 12, 2020

josephglanville Jun 12, 2020 •

edited

Loading

josephglanville Jun 12, 2020

josephglanville Jun 13, 2020

tariq1890 Jun 27, 2020

tariq1890 commented Jun 12, 2020

josephglanville commented Jun 12, 2020 •

edited

Loading

josephglanville commented Jun 17, 2020

njuettner Jun 17, 2020

josephglanville Jun 17, 2020

josephglanville Jun 17, 2020

josephglanville Jun 24, 2020

josephglanville commented Jun 20, 2020

josephglanville commented Jun 20, 2020

josephglanville commented Jun 24, 2020

tariq1890 commented Jun 27, 2020 •

edited

Loading

Raffo commented Jun 28, 2020

Raffo commented Jul 1, 2020

josephglanville commented Jul 1, 2020

josephglanville commented Jul 1, 2020

Raffo commented Jul 2, 2020

tariq1890 commented Jul 2, 2020

josephglanville commented Jul 3, 2020

josephglanville commented Jul 3, 2020

nikhita commented Jul 3, 2020

Raffo commented Jul 3, 2020

Raffo commented Jul 8, 2020

k8s-ci-robot commented Jul 8, 2020

dependencies: Upgrade all k8s client-go dependent sources to v1.18.X #1627

dependencies: Upgrade all k8s client-go dependent sources to v1.18.X #1627

Conversation

josephglanville commented Jun 12, 2020

k8s-ci-robot commented Jun 12, 2020

k8s-ci-robot commented Jun 12, 2020

josephglanville commented Jun 12, 2020

Choose a reason for hiding this comment

josephglanville Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq1890 commented Jun 12, 2020

josephglanville commented Jun 12, 2020 • edited Loading

josephglanville commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephglanville commented Jun 20, 2020

josephglanville commented Jun 20, 2020

josephglanville commented Jun 24, 2020

tariq1890 commented Jun 27, 2020 • edited Loading

Raffo commented Jun 28, 2020

Raffo commented Jul 1, 2020

josephglanville commented Jul 1, 2020

josephglanville commented Jul 1, 2020

Raffo commented Jul 2, 2020

tariq1890 commented Jul 2, 2020

josephglanville commented Jul 3, 2020

josephglanville commented Jul 3, 2020

nikhita commented Jul 3, 2020

Raffo commented Jul 3, 2020

Raffo commented Jul 8, 2020

k8s-ci-robot commented Jul 8, 2020

josephglanville Jun 12, 2020 •

edited

Loading

josephglanville commented Jun 12, 2020 •

edited

Loading

tariq1890 commented Jun 27, 2020 •

edited

Loading