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

Remove purell package usage from kubeadm #103801

Merged
merged 1 commit into from Aug 5, 2021
Merged

Remove purell package usage from kubeadm #103801

merged 1 commit into from Aug 5, 2021

Conversation

gkarthiks
Copy link
Contributor

@gkarthiks gkarthiks commented Jul 20, 2021 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes the usage of the purell module from the kubeadm and introduces the localized function for normalizing the URL.

Which issue(s) this PR fixes:

Fixes #103068

Special notes for your reviewer:

This PR is to move away from the non-maintained module purell and create a localized function that does the normalization of the URL string.
Ref: PuerkitoBio/purell#33

Does this PR introduce a user-facing change?

kubeadm: external etcd endpoints passed in the ClusterConfiguration that have Unicode characters are no longer IDNA encoded (converted to Punycode). They are now just URL encoded as per Go's implementation of RFC-3986, have duplicate "/" removed from the URL paths, and passed like that directly to the kube-apiserver --etcd-servers flag. If you have etcd endpoints that have Unicode characters, it is advisable to encode them in advance with tooling that is fully IDNA compliant. If you don't do that, the Go standard library (used in k8s and etcd) would do it for you when making requests to the endpoints.

/cc

/sig cluster-lifecycle
/area kubeadm
/area code-organization
@neolit123

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 20, 2021
@k8s-ci-robot
Copy link
Contributor

@gkarthiks: GitHub didn't allow me to request PR reviews from the following users: gkarthiks.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR removes the usage of the purell module from the kubeadm and introduces the localized function for normalizing the URL.

Which issue(s) this PR fixes:

Fixes #103068

Special notes for your reviewer:

This PR is to move away from the non-maintained module purell and create a localized function that does the normalization of the URL string.
Ref: PuerkitoBio/purell#33

Does this PR introduce a user-facing change?

NONE

But,

kubeadm: external etcd endpoints passed in the ClusterConfiguration that have Unicode characters are no longer IDNA encoded (converted to Punycode). They are now just URL encoded as per Go's implementation of RFC-3986, have duplicate "/" removed from the URL paths, and passed like that directly to the kube-apiserver --etcd-servers flag. If you have etcd endpoints that have Unicode characters, it is advisable to encode them in advance with tooling that is fully IDNA compliant. If you don't do that, the Go standard library (used in k8s and etcd) would do it for you when making requests to the endpoints.

/cc

/sig cluster-lifecycle
/area kubeadm
/area code-organization
@neolit123

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. area/kubeadm area/code-organization Issues or PRs related to kubernetes code organization cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 20, 2021
@gkarthiks
Copy link
Contributor Author

/test pull-kubernetes-dependencies

@k8s-ci-robot k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Jul 20, 2021
@gkarthiks gkarthiks mentioned this pull request Jul 20, 2021
Closed
@neolit123
Copy link
Member

neolit123 commented Jul 20, 2021 •
edited

under "Does this PR introduce a user-facing change?" notice how the original template had the release note formed with release-note type:
https://raw.githubusercontent.com/kubernetes/kubernetes/master/.github/PULL_REQUEST_TEMPLATE.md

try copy pasting it like that.
we don't need this part:

NONE

But,

also please keep the commits squashed to one.

neolit123
neolit123 reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/approve
/milestone v1.23

thanks, the change looks good.
it can be merged on august 4th (code freeze end).

@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Jul 20, 2021
@neolit123
Copy link
Member

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 20, 2021
@gkarthiks
refactor: normalizing URL string locally and move out from purell pac…
019e8f7 
…kage

The purell package at github.com/PuerkitoBio/purell is no longer maintained and in k/k repo under kubeadm package its been used for normalizing the URL. This commit removes the dependency on this package and creates a local function for normalizing the URL within the preflight package under cmd/kubeadm.

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

chore: add new line at end of the file

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>

fix: remove unused mod from vendor modules file

Signed-off-by: gkarthiks <github.gkarthiks@gmail.com>
@gkarthiks gkarthiks requested a review from neolit123 July 20, 2021 17:35
@neolit123
Copy link
Member

Does this PR introduce a user-facing change?

kubeadm: external etcd endpoints passed in the ClusterConfiguration that have Unicode characters are no longer IDNA encoded (converted to Punycode). They are now just URL encoded as per Go's implementation of RFC-3986, have duplicate "/" removed from the URL paths, and passed like that directly to the kube-apiserver --etcd-servers flag. If you have etcd endpoints that have Unicode characters, it is advisable to encode them in advance with tooling that is fully IDNA compliant. If you don't do that, the Go standard library (used in k8s and etcd) would do it for you when making requests to the endpoints.

as mentioned above, the code block must be formatted with leading "```release-note" instead of "````".
the bots here indicated the PR to have a release-note, but the release note tooling might not be able to pick it up.

neolit123
neolit123 reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm
on the code change, thanks.

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

apologies @neolit123 . On the release-note section if you mean like in the below screenshot, that's how I have had it from the beginning except I had NONE initially.

image

Is there something I am misunderstanding?

@neolit123
Copy link
Member

neolit123 commented Jul 21, 2021 via email

@neolit123
Copy link
Member

The go mod changes will likely get approved after code freeze.

@neolit123
Copy link
Member

/assign @thockin @liggitt
PTAL for approval of the github.com/PuerkitoBio/purell dependency removal.

@liggitt
Copy link
Member

liggitt commented Aug 5, 2021

/lgtm
/approve

@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 5, 2021
thockin
thockin reviewed Aug 5, 2021
View reviewed changes
vendor/modules.txt
@@ -81,7 +81,6 @@ github.com/Microsoft/hcsshim/osversion
# github.com/NYTimes/gziphandler v1.1.1 => github.com/NYTimes/gziphandler v1.1.1
github.com/NYTimes/gziphandler
# github.com/PuerkitoBio/purell v1.1.1 => github.com/PuerkitoBio/purell v1.1.1
## explicit
github.com/PuerkitoBio/purell
Copy link
Member

Choose a reason for hiding this comment

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

We can't actually get rid of it yet, right? I still see one dep on it (vendor/github.com/go-openapi/jsonreference/reference.go). Should we port this same change to go-openapi ?

Copy link
Member

Choose a reason for hiding this comment

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

looks like it asks for a more demanding normalization from the library:
https://github.com/go-openapi/jsonreference/blob/master/reference.go#L117

https://github.com/PuerkitoBio/purell/blob/master/README.md

FlagsSafe NormalizationFlags = FlagLowercaseHost | FlagLowercaseScheme | FlagUppercaseEscapes | FlagDecodeUnnecessaryEscapes | FlagEncodeNecessaryEscapes | FlagRemoveDefaultPort | FlagRemoveEmptyQuerySeparator

so i don't think the replacement function in this PR is sufficient, as it only handles the duplicate slashes.

thockin
thockin reviewed Aug 5, 2021
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Thanks!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gkarthiks, liggitt, neolit123, thockin

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

@thockin
Copy link
Member

thockin commented Aug 5, 2021 via email

@neolit123
Copy link
Member

Still, if Purell is abandonware, they should consider in-sourcing that logic

that is a good idea. @gkarthiks would you like to log an issue for https://github.com/go-openapi/jsonreference so that they can adapt as well (and remove our dep on the package, since we are vendoring go-openapi/jsonreference)?

@k8s-ci-robot k8s-ci-robot merged commit 8df554b into kubernetes:master Aug 5, 2021
@gkarthiks gkarthiks mentioned this pull request Aug 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@thockin thockin thockin left review comments

Assignees

@neolit123 neolit123

@liggitt liggitt

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-organization Issues or PRs related to kubernetes code organization area/dependency Issues or PRs related to dependency changes area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

purell module is no longer maintained
5 participants
@gkarthiks @k8s-ci-robot @neolit123 @liggitt @thockin