write HostAliases to hosts file #45148
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
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. |
k8s-ci-robot
added
the
needs-ok-to-test
|
This change is |
k8s-github-robot
added
size/M
| 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.
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.
And shouldn't we validate for proper IPv4 IPs? :-)
There was a problem hiding this comment.
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.
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.
I think dups is fine - it's your own fault.
|
@k8s-bot ok to test |
k8s-ci-robot
removed
the
needs-ok-to-test
k8s-github-robot
added
the
approved
|
LGTM /lgtm |
k8s-ci-robot
added
the
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 [email protected] and [email protected] 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:
HostAliasesfield in PodSpec along with validationsRelease note:
@thockin @yujuhong
Thanks for reviewing!