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

Add status subresource to Ingress #15491

Merged
merged 1 commit into from
Oct 15, 2015
Merged

Add status subresource to Ingress #15491

merged 1 commit into from
Oct 15, 2015
+303 −13

Conversation

derekwaynecarr
Copy link
Member

Fixes #15480

@derekwaynecarr
Copy link
Member Author

Marking WIP at the moment...

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 12, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-bot
Copy link

k8s-bot commented Oct 12, 2015

GCE e2e build/test failed for commit 52ec2acf9b53f6bafea65ecbc8c095e3d42be111.

@derekwaynecarr
Copy link
Member Author

@bprashanth - I am having trouble finding anyone that is calling Ingress to update the Status section. Am I missing something obvious?

@thockin - I am having trouble finding anything that validates Service.Status which has a similar structure to Ingress.Status. How should we validate LoadBalancerIngress struct? Do we require IP or Hostname? Right now, I have no special validation on ``Ingress.Statusaside from normalvalidateObjectMetaUpdate`

@bprashanth
Copy link
Contributor

There's nothing using Ingress in the main repo, the controllers are going into contrib (kubernetes-retired/contrib#132 and https://github.com/kubernetes/contrib/tree/master/service-loadbalancer). Can you please update the godep in contrib so it picks up this change when it goes in?

Re LoadBalancerIngress, suggest checking that IP is an ip and Hostname is a DNS subdomain (eg: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/extensions/validation/validation.go#L459) so we don't update it to random garbage (though an ip is also a subdomain). Going back to the original pr looks like we just forgot (3884d5f) so I think we can punt on anything more elaborate, for now.

Do we require IP or Hostname?

I don't see why a controller shouldn't be allowed to blank it out though leaving it as is and doing this will likely be more useful #8992

@bgrant0607
Copy link
Member

The change so far LGTM.

@derekwaynecarr
Copy link
Member Author

Thanks for info, will make validation updates tomorrow and poke PR for
final review at that time.

On Tuesday, October 13, 2015, Brian Grant notifications@github.com wrote:

The change so far LGTM.

—
Reply to this email directly or view it on GitHub
#15491 (comment)
.

@k8s-bot
Copy link

k8s-bot commented Oct 14, 2015

GCE e2e test build/test passed for commit ebd83bb575deae75083e9b3c5b24af08d116ea24.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 14, 2015
@derekwaynecarr derekwaynecarr changed the title WIP: Add status subresource to Ingress Add status subresource to Ingress Oct 14, 2015
@derekwaynecarr
Copy link
Member Author

@bgrant0607 - this should be ready for review now.

@bgrant0607 bgrant0607 added this to the v1.1 milestone Oct 14, 2015
@bgrant0607
Copy link
Member

@derekwaynecarr Thanks much. WTAL.
cc @thockin

@k8s-bot
Copy link

k8s-bot commented Oct 14, 2015

GCE e2e build/test failed for commit de12a62680b525acd4f39c25b75973c89e0b2a84.

@derekwaynecarr
Copy link
Member Author

@k8s-bot - test this again

bprashanth
bprashanth reviewed Oct 14, 2015
View reviewed changes
pkg/apis/extensions/validation/validation.go

status := ingress.Status
for _, lbIngress := range status.LoadBalancer.Ingress {
if len(lbIngress.IP) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest splitting this out into ValidateLoadBalancerStatus (and using ingress as a prefix in the caller) so we can reuse for Service.Status.

@bprashanth
Copy link
Contributor

nits, defer to others for review

bprashanth
bprashanth reviewed Oct 14, 2015
View reviewed changes
pkg/registry/ingress/strategy.go

var StatusStrategy = ingressStatusStrategy{Strategy}

func (ingressStatusStrategy) PrepareForUpdate(obj, old runtime.Object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Godoc for public functions please

@thockin
Copy link
Member

thockin commented Oct 14, 2015

A couple nits, otherwise LGTM

@derekwaynecarr
Copy link
Member Author

@bprashanth @thockin - thanks for review, will update nits shortly.

@derekwaynecarr
Copy link
Member Author

@bgrant0607 @bprashanth @thockin - nits addressed.

@k8s-bot
Copy link

k8s-bot commented Oct 14, 2015

GCE e2e test build/test passed for commit eae56c3.

@bgrant0607
Copy link
Member

LGTM

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 14, 2015
thockin
thockin reviewed Oct 14, 2015
View reviewed changes
pkg/api/validation/validation.go
allErrs = append(allErrs, errs.NewFieldInvalid("ingress.hostname", ingress.Hostname, errMsg))
}
if isIP := (net.ParseIP(ingress.Hostname) != nil); isIP {
allErrs = append(allErrs, errs.NewFieldInvalid("ingress.hostname", ingress.Hostname, "must be a DNS name, not ip address"))
Copy link
Member

Choose a reason for hiding this comment

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

s/ip IP/

@thockin
Copy link
Member

thockin commented Oct 14, 2015

LGTM. One nit. If you get there before the bot, fix the nit. If the bot gets there first, send a followup :)

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 15, 2015

GCE e2e test build/test passed for commit eae56c3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 15, 2015
Merge pull request #15491 from derekwaynecarr/ingress_status
351cc20 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 351cc20 into kubernetes:master Oct 15, 2015
@derekwaynecarr derekwaynecarr mentioned this pull request Oct 15, 2015
Merged
@bprashanth
Copy link
Contributor

@derekwaynecarr are you ok updating godeps in contrib to pick this up (see kubernetes-retired/contrib#142) or should I? My GCE controller can wait for the new godeps so you don't have to rebase.

@derekwaynecarr
Copy link
Member Author

@bprasanth - I will look to open a PR today and cc you

On Thu, Oct 15, 2015 at 3:27 PM, Prashanth B notifications@github.com
wrote:

@derekwaynecarr https://github.com/derekwaynecarr are you ok updating
godeps in contrib to pick this up (see kubernetes-retired/contrib#142
kubernetes-retired/contrib#142) or should I? My GCE
controller can wait for the new godeps so you don't have to rebase.

—
Reply to this email directly or view it on GitHub
#15491 (comment)
.

@bprashanth
Copy link
Contributor

@derekwaynecarr Nvm it's ok, I'll do it. Thanks for this change!

@derekwaynecarr
Copy link
Member Author

thanks

On Thu, Oct 15, 2015 at 4:31 PM, Prashanth B notifications@github.com
wrote:

@derekwaynecarr https://github.com/derekwaynecarr Nvm it's ok, I'll do
it. Thanks for this change!

—
Reply to this email directly or view it on GitHub
#15491 (comment)
.

@bprashanth bprashanth mentioned this pull request Oct 16, 2015
Merged
bprashanth added a commit that referenced this pull request Oct 20, 2015
@bprashanth
Merge pull request #15783 from bprashanth/automated-cherry-pick-of-#1…
9f6c453 
…5491-upstream-release-1.1

Automated cherry pick of #15491
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@bprashanth
Merge pull request kubernetes#15783 from bprashanth/automated-cherry-…
c2c065f 
…pick-of-#15491-upstream-release-1.1

Automated cherry pick of kubernetes#15491
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@bprashanth
Merge pull request kubernetes#15783 from bprashanth/automated-cherry-…
c39981f 
…pick-of-#15491-upstream-release-1.1

Automated cherry pick of kubernetes#15491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.
9 participants
@derekwaynecarr @k8s-github-robot @k8s-bot @bprashanth @bgrant0607 @thockin @k8s-teamcity-mesosphere @googlebot @eparis