-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Move to go 1.17 #12868
Move to go 1.17 #12868
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Stripping leading zeroes still isn't quite right, the new 1.17 behaviour is to error when a leading zero is present in a non-zero component. So 0.0.0.0 is fine, but 1.01.1.1 isn't. |
if err != nil { | ||
return nil, fmt.Errorf("error decoding SecretIDBoundCIDRs field of role storage entry: %w", err) | ||
} | ||
role.SecretIDBoundCIDRs[i] = ipn.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the case but still asking this for confirmation. I assume that you intentionally left out setting needsUpgrade
, with the rationale that we don't necessarily need to fix this in storage, as we fix things with every read? (same case with ssh engine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, though I'm open to discussing it. This is me being a bit cautious, since atm even if we migrated all the bad data, we'd still have to keep this code around indefinitely unless we started mandating stepwise upgrades. Given that, I didn't see any reason to take the extra risk of re-writing the records; if there's a bug somewhere, at least we won't damage the data, and once we release a fixed version they can move forward without any manual remediation.
# Conflicts: # go.mod # go.sum
…eeing on "github.com/docker/docker/pkg/archive"
I did some manual testing by writing an approle role using 1.8.3:
Then upgraded the cluster and:
|
# Conflicts: # go.mod # go.sum
…attempt at this PR in ent, and some of those changes were causing problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
at some point we might want to run tooling to upgrade all the +build to go:build https://golang.org/doc/go1.17
In addition to the obvious changes, address some fallout from a breaking change (see https://golang.org/doc/go1.17#net):
ssh roles store CIDRList and ExcludeCIDRList as-is based on provided input in role update requests; they are subsequently parsed during creds generation via validateIP, which will return an error if the original CIDRs contain multiple leading zeroes.
similarly with approle and the secretID fields cidr_list, token_bound_cidrs