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

Jump to bottom

build: Upgrade to go 1.12.10 #41576

Merged
merged 1 commit into from Oct 16, 2019
Merged

build: Upgrade to go 1.12.10 #41576

merged 1 commit into from Oct 16, 2019

Conversation

bobvawter
Copy link
Member

@bobvawter bobvawter commented Oct 14, 2019 •
edited 

build: Upgrade to go 1.12.10

This change updates the golang version to 1.12.10 to pick up a security fix in
the net/http package.  This also picks up a URL parsing security fix in 1.12.8
that eliminates the use of named ports. See the relevant discussion in
https://github.com/golang/go/commit/3226f2d492963d361af9dfc6714ef141ba606713

The change in URL parsing necessitates a test fix. Any customer impact should
be minimal, given the rarity of named ports in common practice, and can be
fixed by simply switching to numeric ports.

Per the [checklist](build/README.md):
* [X] Adjust version in Docker image
* [X] Rebuild the Docker image and bump the version in builder.sh accordingly
* [ ] ~Bump the version in go-version-check.sh~ (Patch release, not necessary)
* [X] Bump the default installed version of Go in bootstrap-debian.sh

Fixes: #40939
X-Ref: #23481

Release note (build change): The golang runtime has been upgraded to 1.12.10.

Release note (general change): The use of named ports in URLs has been
eliminated due to URL parsing ambiguities that were fixed in go 1.12.8. Any
impact can be mitigated with the use of numeric port numbers.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

petermattis
petermattis approved these changes Oct 14, 2019
View reviewed changes
Copy link
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

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

LGTM

@bobvawter bobvawter requested a review from a team as a code owner October 14, 2019 19:45
@bobvawter
Copy link
Member Author

PTAL. A security fix in go 1.12.8 changed how url.Parse handles named ports (which it to say it doesn't anymore).

@knz
Copy link
Contributor

knz commented Oct 14, 2019

huh wait

We document that cockroachdb is able to use ports by name. If url.Parse is not doing it for us any more this simply means we need to do it ourselves instead. I wouldn't go about this by just removing the tests.

@petermattis
Copy link
Collaborator

We document that cockroachdb is able to use ports by name. If url.Parse is not doing it for us any more this simply means we need to do it ourselves instead. I wouldn't go about this by just removing the tests.

Removing support for named ports was part of a security fix in go1.12.8. See https://go-review.googlesource.com/c/go/+/189258/. Trying to re-add support ourselves sounds fraught and not worth the effort (writing our own URL parser?). Do you suspect anyone is using this functionality? I didn't even know it existed until this PR.

@bobvawter
Copy link
Member Author

I can't remember ever actually using named ports in practice. Either something's using the default port, or it's not worth updating /etc/protocols for the perceived convenience. FWIW, the URL parser has never supported named ports with IPV6 literals.

@kenliu
Copy link

kenliu commented Oct 15, 2019

@petermattis @knz unclear how we should proceed here -- who makes the call on whether this "feature" gets removed, and how do we follow up on it?

@benesch
Copy link
Contributor

benesch commented Oct 15, 2019

Probably worth bumping

cockroach/build/bootstrap/bootstrap-debian.sh

Line 41 in 53e1686

curl https://dl.google.com/go/go1.12.7.linux-amd64.tar.gz > /tmp/go.tgz
as well.

@knz
Copy link
Contributor

knz commented Oct 15, 2019

@kenliu I am looking at this now. I'm checking what we have in docs about this.

@knz
Copy link
Contributor

knz commented Oct 15, 2019

Ok I got to the bottom of this from the linked CL by Peter:

Note that net.SplitHostPort and net.Dial explicitly accept service names
in place of port numbers, but this is an URL package, and RFC 3986,
Section 3.2.3, clearly specifies ports as a number in decimal.

RFC 3986 refers to the URL syntax. This tells us that even if CockoachDB supports service names in --listen-addr etc, we are not expecting clients to use a client URL with a non-numeric port number, because the standard URL syntax does not support this.

So you can go ahead with the change, I have no more objection.

@bobvawter
build: Upgrade to go 1.12.10
5d34195 
This change updates the golang version to 1.12.10 to pick up a security fix in
the net/http package.  This also picks up a URL parsing security fix in 1.12.8
that eliminates the use of named ports. See the relevant discussion in
golang/go@3226f2d

The change in URL parsing necessitates a test fix. Any customer impact should
be minimal, given the rarity of named ports in common practice, and can be
fixed by simply switching to numeric ports.

Per the [checklist](build/README.md):
* [X] Adjust version in Docker image
* [X] Rebuild the Docker image and bump the version in builder.sh accordingly
* [ ] ~Bump the version in go-version-check.sh~ (Patch release, not necessary)
* [X] Bump the default installed version of Go in bootstrap-debian.sh

Fixes: #40939
X-Ref: #23481

Release note (build change): The golang runtime has been upgraded to 1.12.10.

Release note (general change): The use of named ports in URLs has been
eliminated due to URL parsing ambiguities that were fixed in go 1.12.8. Any
impact can be mitigated with the use of numeric port numbers.
@bobvawter
Copy link
Member Author

bors r=petermattis

craig bot pushed a commit that referenced this pull request Oct 16, 2019
@nvanbenschoten @bobvawter @tbg
Merge #41509 #41576 #41632 #41636
c0700e5 
41509: client/kv: keep transaction UserPriority in sync across client.Txn and TxnCoordSender r=nvanbenschoten a=nvanbenschoten

Informs #32508.

Before this change, a transaction's UserPriority could diverge between the
transaction's client.Txn and its TxnCoordSender. This resulted in the need for
ugly code
(https://github.com/cockroachdb/cockroach/blob/57b02ddc74004e39a411084e05a20dccd93c3a22/pkg/kv/txn_coord_sender.go#L786),
had bugs where a transaction could be restarted with the wrong user priority,
and resulted in redundant calls to `roachpb.MakePriority`.

This last issue had a runtime cost. We saw the redundant calls to
`roachpb.MakePriority` show up in profiles. For instance, it was responsible for
**0.32%** of a CPU profile when running Sysbench's `oltp_point_select` workload,
mainly due to how slow `rand.ExpFloat64` is on the global rand object.

This commit fixes this by keeping the priority in-sync between the `client.Txn`
and the `kv.TxnCoordSender`. A side-effect of this is that we no longer perform
redundant calls to `roachpb.MakePriority`.

Release note: None

41576: build: Upgrade to go 1.12.10 r=petermattis a=bobvawter

    build: Upgrade to go 1.12.10
    
    This change updates the golang version to 1.12.10 to pick up a security fix in
    the net/http package.  This also picks up a URL parsing security fix in 1.12.8
    that eliminates the use of named ports. See the relevant discussion in
    golang/go@3226f2d
    
    The change in URL parsing necessitates a test fix. Any customer impact should
    be minimal, given the rarity of named ports in common practice, and can be
    fixed by simply switching to numeric ports.
    
    Per the [checklist](build/README.md):
    * [X] Adjust version in Docker image
    * [X] Rebuild the Docker image and bump the version in builder.sh accordingly
    * [ ] ~Bump the version in go-version-check.sh~ (Patch release, not necessary)
    * [X] Bump the default installed version of Go in bootstrap-debian.sh
    
    Fixes: #40939
    X-Ref: #23481
    
    Release note (build change): The golang runtime has been upgraded to 1.12.10.
    
    Release note (general change): The use of named ports in URLs has been
    eliminated due to URL parsing ambiguities that were fixed in go 1.12.8. Any
    impact can be mitigated with the use of numeric port numbers.



41632: builtins: allow full consistency check w/o nuking cluster r=andreimatei a=tbg

Previously, triggering a full consistency check would have potential to
harm the cluster due to triggering checks with full DistSender
parallelism. Limit to one check at a time for a (much slower, but) less
harmful check (that we can potentially emit in roachtests).

Release note: None

41636: roachtest: set acceptance/version-upgrade timeout to 30m r=andreimatei a=tbg

When adding aggressive consistency checks, I noticed that
acceptance/version-upgrade got pushed over the limit.

Release note: None

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Bob Vawter <bob@cockroachlabs.com>
Co-authored-by: Tobias Schottdorf <tobias.schottdorf@gmail.com>
@craig
Copy link
Contributor

craig bot commented Oct 16, 2019

Build succeeded

@craig craig bot merged commit 5d34195 into cockroachdb:master Oct 16, 2019
@bobvawter bobvawter mentioned this pull request Oct 16, 2019
Merged
craig bot pushed a commit that referenced this pull request Oct 16, 2019
@bobvawter
Merge #41647
52a51ed 
41647: release-19.2: build: Upgrade to go 1.12.10 r=petermattis a=bobvawter

Backport 1/1 commits from #41576.

/cc @cockroachdb/release

---

    build: Upgrade to go 1.12.10
    
    This change updates the golang version to 1.12.10 to pick up a security fix in
    the net/http package.  This also picks up a URL parsing security fix in 1.12.8
    that eliminates the use of named ports. See the relevant discussion in
    golang/go@3226f2d
    
    The change in URL parsing necessitates a test fix. Any customer impact should
    be minimal, given the rarity of named ports in common practice, and can be
    fixed by simply switching to numeric ports.
    
    Per the [checklist](build/README.md):
    * [X] Adjust version in Docker image
    * [X] Rebuild the Docker image and bump the version in builder.sh accordingly
    * [ ] ~Bump the version in go-version-check.sh~ (Patch release, not necessary)
    * [X] Bump the default installed version of Go in bootstrap-debian.sh
    
    Fixes: #40939
    X-Ref: #23481
    
    Release note (build change): The golang runtime has been upgraded to 1.12.10.
    
    Release note (general change): The use of named ports in URLs has been
    eliminated due to URL parsing ambiguities that were fixed in go 1.12.8. Any
    impact can be mitigated with the use of numeric port numbers.




Co-authored-by: Bob Vawter <bob@cockroachlabs.com>
@knz knz mentioned this pull request Nov 14, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petermattis petermattis petermattis approved these changes

@vilterp vilterp Awaiting requested review from vilterp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@bobvawter @cockroach-teamcity @knz @petermattis @kenliu @benesch