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

Bump golang support to 1.10 - Fixing "cannot use nil as type _Ctype_CFDataRef in assignment" problem #2869

Closed
wants to merge 2 commits into from

Conversation

stantonxu
Copy link

'make test' failed on the HEAD of Jun 4, 2018, due to issue #2860

Fixed the problem here by removing "nil" from the assignment to CFDataRef

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 4, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stantonxu
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aaron-prindle

Assign the PR to them by writing /assign @aaron-prindle in a comment when ready.

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

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@dlorenc
Copy link
Contributor

dlorenc commented Jun 4, 2018

We can't make changes to the vendor directory directly like this. Can you try updating the dep instead of doing this?

@stantonxu
Copy link
Author

@dlorenc , thanks for quick review. Could you please give me more hints about how to update the dep? Where to check it?

@stantonxu
Copy link
Author

@dlorenc I spent whole afternoon, trying to figure out how to update dep for minikube. It looks that the code structure of google/certificate-transparency-go is different than the folder in minikube. How to update the dep, could you give some hint?

@dlorenc
Copy link
Contributor

dlorenc commented Jun 4, 2018

The flow to update a dependency is basically the same as the one specified here:
https://github.com/kubernetes/minikube/blob/master/docs/contributors/adding_a_dependency.md

This one is a bit tricky since as you point out, certificate-transparency has changed quite a bit. You'd have to find whatever is calling the old version of it, and update that (recursively) until nothing is using that old package anymore.

@stantonxu
Copy link
Author

stantonxu commented Jun 5, 2018

@dlorenc I see the difference about the "github.com/google/certificate-transparency..." vendor dep content in the Godeps.json files of the Kubernetes project and minikube project.

In Kubernetes, it already uses the newest version (certificate-transparency-go), which handle the initialization of the CFDataRef type (which caused the original issue of this PR).

{ "ImportPath": "github.com/google/certificate-transparency-go", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/asn1", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/client", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/client/configpb", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/jsonclient", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/tls", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/x509", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" }, { "ImportPath": "github.com/google/certificate-transparency-go/x509/pkix", "Rev": "1bec4527572c443752ad4f2830bef88be0533236" },

In minikube project, it still uses the old version (certificate-transparency),

{ "ImportPath": "github.com/google/certificate-transparency/go", "Rev": "af98904302724c29aa6659ca372d41c9687de2b7" }, { "ImportPath": "github.com/google/certificate-transparency/go/asn1", "Rev": "af98904302724c29aa6659ca372d41c9687de2b7" }, { "ImportPath": "github.com/google/certificate-transparency/go/client", "Rev": "af98904302724c29aa6659ca372d41c9687de2b7" }, { "ImportPath": "github.com/google/certificate-transparency/go/x509", "Rev": "af98904302724c29aa6659ca372d41c9687de2b7" }, { "ImportPath": "github.com/google/certificate-transparency/go/x509/pkix", "Rev": "af98904302724c29aa6659ca372d41c9687de2b7" },

Can I just simply replace the minikube vendor about it with the newest one of Kubernetes? Or maybe do a dep update in minikube for all the google/certificate-transparency deps?

@dlorenc
Copy link
Contributor

dlorenc commented Jun 5, 2018

Can I just simply replace the minikube vendor about it with the newest one of Kubernetes? Or maybe do a dep update in minikube for all the google/certificate-transparency deps?

Yeah, you'll basically need to update all the deps in minikube that rely on google/certificate-transparency, then update google/certificate-transparency itself.

@stantonxu
Copy link
Author

@dlorenc is there a way (a command or so) to check if there are other deps rely on google/certificate-transparency? Or do I need to look into the Godeps.json files for each of the vendor dep and recursively to find it out? That will be huge amount of work.

@stantonxu
Copy link
Author

I just simply did search in whole minikube project, found out that only another dep "github.com/cloudflare/cfssl" is using "google/certificate-transparency/go". In the newest version of "github.com/cloudflare/cfssl", it is already using "google/certificate-transparency-go" instead. So I guess the task turns to update vendor dep "github.com/cloudflare/cfssl".

Working on it now.

@stantonxu
Copy link
Author

@dlorenc , so basically it is a compatibility problem, due to google/certificate-transparency-go#131

I see that Kubernetes has already bump cfssl to be compatible with Go 1.10, kubernetes/kubernetes#60373

Do we have any plan when we will make minikube compatible with Go 1.10?

I tried to update dep cfssl, but ran into couple problems. I have some questions here,

  1. cfssl at HEAD shows build failed. However go get -u and then godep update doesn't allow me to update to specific version or tag. Do I have to use dep for specific version?

  2. dep needs to init project. Will that generate extra code which I don't want to check in?

  3. I tried to go get -u cfssl, it has its own vendor folder, which including google/certificate-transparency-go and some other deps. The vendor folder of cfssl shows up in my minikube project. Is it safe to manually move those cfssl vendor deps from minikube/vendor/github.com/cloudflare/cfssl/vendor to the root vendor folder - minikube/vendor, and overwrite the existing ones?

General question, is it safe just update all vendor deps of minikube, instead of investigating each of them one by one? That's huge amount of work.

@dlorenc
Copy link
Contributor

dlorenc commented Jun 6, 2018

It looks like that PR to kubernetes was merged after 1.10, so we'll need to wait until Minikube either stops vendor kubernetes (#2870) or until we update to k8s 1.11.

@stantonxu stantonxu changed the title Fixing "cannot use nil as type _Ctype_CFDataRef in assignment" problem [WIP] Bump golang support to 1.10 - Fixing "cannot use nil as type _Ctype_CFDataRef in assignment" problem Jun 26, 2018
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2018
@stantonxu
Copy link
Author

@dlorenc although we are wait until Minikube either stops vendor kubernetes (#2870) or until we update to k8s 1.11 per your suggestion, I am trying to update the github.com/cloudflare/cfssl's dependency - github.com/google/certificate-transparency/go, which should be updated to github.com/google/certificate-transparency-go

However godep restore ./... under minikube folder always runs into some problems. I find that there are some hack scripts under minikube/hack/godeps, but when I tried to run them, they also ran into problems like below,

!!! Error in ./hack/godep-restore.sh:34
  Error in ./hack/godep-restore.sh:34. 'GOPATH="${GOPATH}:${KUBE_ROOT}/staging" godep restore "$@"' exited with status 1
Call stack:
  1: ./hack/godep-restore.sh:34 main(...)
Exiting with status 1

Could you please help give me some hints?

My godep is at v79, and go is at 1.9.6

@stantonxu
Copy link
Author

@dlorenc , do you have some time to look at my latest comment above? I could use some help here to move forward. Thanks.

@dlorenc
Copy link
Contributor

dlorenc commented Jul 12, 2018

@dlorenc , do you have some time to look at my latest comment above? I could use some help here to move forward. Thanks.

Hey,

I think I mentioned this earlier somewhere, but I don't think you're going to be able to update the cfssl dependency. Minikube currently vendors k8s 1.10, which is pinned to the older version of the library. We'd need to patch k8s 1.10, updated to 1.11 or just delete the vendored code.

@stantonxu
Copy link
Author

Thanks, @dlorenc . When we have the plan to upgrade to 1.11 or delete the vendored code, please let me know. I would like to continue and follow upon this PR, either commit corresponding changes or close it.

@stantonxu stantonxu changed the title [WIP] Bump golang support to 1.10 - Fixing "cannot use nil as type _Ctype_CFDataRef in assignment" problem Bump golang support to 1.10 - Fixing "cannot use nil as type _Ctype_CFDataRef in assignment" problem Jul 24, 2018
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2018
@stantonxu
Copy link
Author

Golang version bump up being addressed by #2777, closing this PR.

@stantonxu stantonxu closed this Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants