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

Update google.golang.org/api to v0.46.0 #262

Merged
merged 6 commits into from Aug 19, 2021

Conversation

sdmodi
Copy link
Contributor

@sdmodi sdmodi commented Aug 9, 2021

This is the same version that k/k is pointing to after kubernetes/kubernetes#102913. These changes are required to support IPv6 in cloud-controller-manager.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @sdmodi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 9, 2021
@sdmodi
Copy link
Contributor Author

sdmodi commented Aug 11, 2021

Bowei, Joe, would you please review this. This requires an ok-to-test label as well.

@cheftako
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2021
@sdmodi
Copy link
Contributor Author

sdmodi commented Aug 13, 2021

The cloud-provider-gcp-tests are failing:

E0812 19:07:24.394354   30692 gce_loadbalancer_internal.go:983] forwardingRulesEqual(): failed to parse resource URL from new FR, err - "https://compute.googleapis.com/compute/v1/test-project/regions/us-central1/backendServices/a" is not a valid resource URL
--- FAIL: TestEnsureInternalLoadBalancerClearPreviousResources (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x25b8e06]
goroutine 122 [running]:
testing.tRunner.func1.2(0x29e3fa0, 0x4442ab0)
	GOROOT/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc000031b00)
	GOROOT/src/testing/testing.go:1146 +0x4b6
panic(0x29e3fa0, 0x4442ab0)
	GOROOT/src/runtime/panic.go:965 +0x1b9
k8s.io/cloud-provider-gcp/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud.(*ResourceID).Equal(...)
	vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go:42
k8s.io/cloud-provider-gcp/providers/gce.forwardingRulesEqual(0xc0006ff800, 0xc0006ffe00, 0x29)
	providers/gce/gce_loadbalancer_internal.go:990 +0x2c6
k8s.io/cloud-provider-gcp/providers/gce.(*Cloud).ensureInternalLoadBalancer(0xc0005abb00, 0x2ecc4ba, 0x11, 0x2eca364, 0xf, 0xc00026d180, 0xc0006ff800, 0xc000467cc8, 0x1, 0x1, ...)
	providers/gce/gce_loadbalancer_internal.go:198 +0x1d18
k8s.io/cloud-provider-gcp/providers/gce.createInternalLoadBalancer(0xc0005abb00, 0xc00026d180, 0xc0006ff800, 0xc000655e50, 0x1, 0x1, 0x2ecc4ba, 0x11, 0x2eca364, 0xf, ...)
	providers/gce/gce_loadbalancer_internal_test.go:49 +0x125
k8s.io/cloud-provider-gcp/providers/gce.TestEnsureInternalLoadBalancerClearPreviousResources(0xc000031b00)
	providers/gce/gce_loadbalancer_internal_test.go:333 +0xaf3
testing.tRunner(0xc000031b00, 0x2faf478)
	GOROOT/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
	GOROOT/src/testing/testing.go:1238 +0x2b3
================================================================================

@liggitt
Copy link
Member

liggitt commented Aug 13, 2021

looks like a nil error related to the URL not being able to be parsed... this is likely related to the projects suffix change in the library

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go#L978-L986

E0812 19:07:24.394354   30692 gce_loadbalancer_internal.go:983] forwardingRulesEqual(): failed to parse resource URL from new FR, err - "https://compute.googleapis.com/compute/v1/test-project/regions/us-central1/backendServices/a" is not a valid resource URL
--- FAIL: TestEnsureInternalLoadBalancerClearPreviousResources (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x25b8e06]
goroutine 122 [running]:
testing.tRunner.func1.2(0x29e3fa0, 0x4442ab0)
	GOROOT/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc000031b00)
	GOROOT/src/testing/testing.go:1146 +0x4b6
panic(0x29e3fa0, 0x4442ab0)
	GOROOT/src/runtime/panic.go:965 +0x1b9
k8s.io/cloud-provider-gcp/vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud.(*ResourceID).Equal(...)
	vendor/github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/utils.go:42
k8s.io/cloud-provider-gcp/providers/gce.forwardingRulesEqual(0xc0006ff800, 0xc0006ffe00, 0x29)
	providers/gce/gce_loadbalancer_internal.go:990 +0x2c6
k8s.io/cloud-provider-gcp/providers/gce.(*Cloud).ensureInternalLoadBalancer(0xc0005abb00, 0x2ecc4ba, 0x11, 0x2eca364, 0xf, 0xc00026d180, 0xc0006ff800, 0xc000467cc8, 0x1, 0x1, ...)

@@ -67,7 +64,7 @@ replace (
golang.org/x/lint => golang.org/x/lint v0.0.0-20201208152925-83fdc39ff7b5 // indirect
golang.org/x/oauth2 => golang.org/x/oauth2 v0.0.0-20210112200429-01de73cf58bd
golang.org/x/sync => golang.org/x/sync v0.0.0-20201207232520-09787c993a3a // indirect
google.golang.org/api => google.golang.org/api v0.30.0
google.golang.org/api => google.golang.org/api v0.46.0
google.golang.org/genproto => google.golang.org/genproto v0.0.0-20210111234610-22ae2b108f89 // indirect
google.golang.org/grpc => google.golang.org/grpc v1.27.1 // indirect
gopkg.in/check.v1 => gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b // indirect
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this have to pick up a compatible version of k8s.io/legacy-cloud-providers that works properly with google.golang.org/api v0.46.0?

Copy link
Member

Choose a reason for hiding this comment

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

This also updates
- Various k8s.io modules to v0.22.0
- Update google.golang.org/grpc to v1.34.0
- Update go.uber.org/zap to v1.17.0
@sdmodi
Copy link
Contributor Author

sdmodi commented Aug 18, 2021

Tests finally passed. I had to update gce_loadbalancer_internal_test.go to work with the new API. Joe, Jordan would you please review this. Thanks!

@@ -136,7 +136,7 @@ func processCSR(client clientset.Interface, privateKeyData []byte, hostname stri
apicertificates.UsageClientAuth,
}
name := digestedName(privateKeyData, subject, usages)
reqName, reqUID, err := csr.RequestCertificate(client, csrData, name, apicertificates.KubeAPIServerClientKubeletSignerName, usages, privateKey)
reqName, reqUID, err := csr.RequestCertificate(client, csrData, name, apicertificates.KubeAPIServerClientKubeletSignerName, nil, usages, privateKey)
Copy link
Member

Choose a reason for hiding this comment

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

@mikedanese is a nil request duration the desired param value?

Copy link
Member

Choose a reason for hiding this comment

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

yes

@cheftako
Copy link
Member

/lgtm
I would like to see someone from security check the nil duration. It would also be good if Joe could check the legacy-cloud-provider vs providers version stuff.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 19, 2021
@sdmodi
Copy link
Contributor Author

sdmodi commented Aug 19, 2021

Thanks a lot Walter! Appreciate the review.

@bowei
Copy link
Member

bowei commented Aug 19, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, sdmodi

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0cdff34 into kubernetes:master Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants