-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
write HostAliases to hosts file #45148
write HostAliases to hosts file #45148
Conversation
Hi @rickypai. 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 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. I understand the commands that are listed here. |
123.45.67.89 baz | ||
456.78.90.123 park | ||
456.78.90.123 doo | ||
456.78.90.123 boo |
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.
should we add validation that there are not duplicate hostnames in case someone adds:
[]v1.HostAlias{
{IP: "123.45.67.89", Hostnames: []string{"foo", "bar"}},
{IP: "456.78.90.123", Hostnames: []string{"foo", "bar"}},
}
which results in
123.45.67.89 foo
123.45.67.89 bar
456.78.90.123 foo
456.78.90.123 bar
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.
And shouldn't we validate for proper IPv4 IPs? :-)
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.
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 dups is fine - it's your own fault.
// write each IP/hostname pair as an entry into hosts file | ||
for _, hostAlias := range hostAliases { | ||
for _, hostname := range hostAlias.Hostnames { | ||
buffer.WriteString(fmt.Sprintf("%s\t%s\n", hostAlias.IP, hostname)) |
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.
Comment that this was previously validated to be a valid IP address and hostname?
123.45.67.89 baz | ||
456.78.90.123 park | ||
456.78.90.123 doo | ||
456.78.90.123 boo |
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 dups is fine - it's your own fault.
@k8s-bot ok to test |
LGTM /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, rickypai, thockin
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
@rickypai: The following test(s) failed:
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 45110, 45148) |
@rickypai can you also add a node e2e test for this feature? Thanks. |
Thanks a lot for the fast review! There's discussion in #43632 about
whether e2e is actually needed. I can write an e2e or integration test when
the dust settles on which one is preferred
Also, Do you think this is a valid candidate for backport to 1.6? It
addresses a lot of user issues around /etc/hosts management.
Thanks a lot again!
…On Mon, May 1, 2017 at 9:16 AM Yu-Ju Hong ***@***.***> wrote:
@rickypai <https://github.com/rickypai> can you also add a node e2e test
for this feature? Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45148 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZsi5b1q6kaK1wBIJ_dtF3H_cV2jveeks5r1gVKgaJpZM4NMV9C>
.
|
@rickypai, we usually patch the release branches for bugs or security issues. Your PRs added a new feature (w/ API changes), so I'd advise against back porting this. /cc @enisoc, the 1.6 patch manager(?) |
No problem. I can understand the release complexity because it involves an API addition, although users won't be affected unless they actually take advantage of the new field. If they do, it's likely because they need to address #43632 #44473 or the like. I can also just follow the build guide into doing an internal release for ourselves until 1.7 lands. I'll write a node e2e test per recommendation in #43632 |
@rickypai we need tomake sure we get docs for this for 1.7, too. @kubernetes/sig-docs-maintainers |
re: documentation, i'm thinking either adding notes about HostAliases in https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/ or as a separate page under "Services, Load Balancing, and Networking" |
Defer to +chenopis@google.com and +stevepe@google.com on placement.
…On Wed, Jun 7, 2017 at 10:42 AM, Ricky Pai ***@***.***> wrote:
re: documentation, i'm thinking either adding notes about HostAliases in
https://kubernetes.io/docs/concepts/services-networking/dns-pod-service/
or as a separate page under "Services, Load Balancing, and Networking"
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#45148 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARmNwM9CXirizJPSilySvSKNpT-jHMPNks5sBuEcgaJpZM4NMV9C>
.
|
Automatic merge from submit-queue (batch tested with PRs 43005, 46660, 46385, 46991, 47103) add e2e node test for Pod hostAliases feature **What this PR does / why we need it**: adds node e2e test for #45148 tests requested in #43632 (comment) **Release note**: ```release-note NONE ``` @yujuhong @thockin
i went with a separate page to give room for more details. kubernetes/website#4080 |
@rickypai That works for me. |
What this PR does / why we need it: using the PodSpec's
HostAliases
, we write entries into the Kubernetes-managed hosts file.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #43632Special notes for your reviewer:
Previous PRs in this series:
HostAliases
field in PodSpec along with validationsRelease note:
@thockin @yujuhong
Thanks for reviewing!