-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
Add HostAliases to PodSpec to support adding entires to a Pod's hosts file #44641
Add HostAliases to PodSpec to support adding entires to a Pod's hosts file #44641
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. |
Btw, this should probably inherit the same labels as #43632:
|
86f6494
to
32d63f3
Compare
@thockin hi tim, is it possible for you to help me look at this change? it implements the proposed type for host mapping in |
@k8s-bot ok to test |
seems like I forgot to generate federation docs and also one of the fields do not conform to convention. updating those. |
5cf7e3f
to
3be8711
Compare
3be8711
to
412f3e7
Compare
412f3e7
to
ed31db5
Compare
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'll approve and let the reviewer's LGTM
pkg/api/validation/validation.go
Outdated
allErrors := field.ErrorList{} | ||
if hostNetwork { | ||
if len(hostMappings) > 0 { | ||
allErrors = append(allErrors, field.Invalid(fldPath, hostMappings, "no `hostMappings` allowed when `hostNetwork` is true")) |
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.
field.Forbidden(..."may not be set when hostNetwork
is true")
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.
fixed
/approve |
@yujuhong I'm open to better names, and if you want to discuss this more, that's fair - just hold off on the LGTM. |
Updating the name to |
ed31db5
to
d2cb57e
Compare
e066bcd
to
a76ada8
Compare
@k8s-bot unit test this |
@k8s-bot kubemark e2e test this |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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] |
Automatic merge from submit-queue |
Automatic merge from submit-queue (batch tested with PRs 45110, 45148) write HostAliases to hosts file **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 #43632 **Special notes for your reviewer**: Previous PRs in this series: - #44572 isolates the logic of creating the file and writing the file - #44641 introduces the `HostAliases` field in PodSpec along with validations **Release note**: ```release-note PodSpec's `HostAliases` now write entries into the Kubernetes-managed hosts file. ``` @thockin @yujuhong Thanks for reviewing!
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 Sat, Apr 29, 2017 at 10:40 AM Kubernetes Submit Queue < ***@***.***> wrote:
Merged #44641 <#44641>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#44641 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZsi_jFkmDeE8rIerRFZMyWH93NmgMVks5r03XxgaJpZM4NA_uf>
.
|
Opps the above comment was meant for a different PR.
On Mon, May 1, 2017 at 2:01 PM rickyp999@gmail.com <rickyp999@gmail.com>
wrote:
… 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 Sat, Apr 29, 2017 at 10:40 AM Kubernetes Submit Queue <
***@***.***> wrote:
> Merged #44641 <#44641>
>
—
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#44641 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABZsi_jFkmDeE8rIerRFZMyWH93NmgMVks5r03XxgaJpZM4NA_uf>
> .
>
|
…ias_hostnames Automatic merge from submit-queue (batch tested with PRs 46977, 47005, 47018, 47061, 46809) Fix HostAlias to validate against DNS1123 hostname instead of just labels **What this PR does / why we need it**: the validation for HostAlias was validating the hostnames against DNS labels instead of hostnames. This means hostnames like `foo.bar` would fail. I did not catch this because unit test cases only had hostnames like `foo`. **Which issue this PR fixes**: fixes issue introduced in #44641 **Release note**: ```release-note fixed HostAlias in PodSpec to allow `foo.bar` hostnames instead of just `foo` DNS labels. ```
Automatic merge from submit-queue (batch tested with PRs 42252, 42251, 42249, 47512, 47887) fix HostAliases' json keys to be hostAlias instead of hostMapping to reflect actual feature name **What this PR does / why we need it**: a rename was introduce during the middle of #44641 to change from `hostMappings` to `hostAliases`. the Go structs were updated, but I neglected to update the json keys. They should be in sync. **Special notes for your reviewer**: I messed up. This is an API change. I hope this is still ok to be in the 1.7 release. **Release note**: ```release-note HostAliases is now parsed with `hostAliases` json keys to be in line with the feature's name. ```
@rickypai thanks for your well done job! |
@rickypai Thanks! |
What this PR does / why we need it:
Adds a new field to PodSpec
HostAliases
to support adding entries to a Pod's hosts file. A PR to incorporate this logic intoensureHostsFile
in kubelet will be next in order to isolate the discussion on the API.Which issue this PR fixes:
A step into fixing #43632
Special notes for your reviewer:
hostNetwork: true
Pods are addressed with an validation. Provide a way to add entries to /etc/hosts #43632 (comment)Release note:
Testing done: