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

promote sourceRange into service spec #25826

Merged
merged 2 commits into from
May 28, 2016
Merged

promote sourceRange into service spec #25826

merged 2 commits into from
May 28, 2016

Conversation

freehan
Copy link
Contributor

@freehan freehan commented May 18, 2016 •
edited
Loading

@thockin one more for your pile

I will add docs at http://releases.k8s.io/HEAD/docs/user-guide/services-firewalls.md

cc: @justinsb

Fixes: #20392

@freehan freehan added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 18, 2016
@freehan freehan added this to the v1.3 milestone May 18, 2016
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2016
@freehan freehan force-pushed the svcsourcerange branch 4 times, most recently from 4a46a69 to 6427407 Compare May 19, 2016 20:40
thockin
thockin reviewed May 19, 2016
View reviewed changes
pkg/api/types.go
SessionAffinity ServiceAffinity `json:"sessionAffinity,omitempty"`

// Optional: Supports "LoadBalancer" type on GCE and AWS
// Once specified, it will create corresponding firewall rules for the loadbalancer
Copy link
Member

Choose a reason for hiding this comment

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

The comment should be less implementation-specific.

Maybe

If specified and supported by the platform, this will restrict traffic through the cloud-provider load-balancer will be restricted to the specified client IPs. This field will be ignored if the cloud-provider does not support the feature."

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@thockin thockin added cla: yes and removed cla: no labels May 20, 2016
@googlebot
Copy link

CLAs look good, thanks!

thockin
thockin reviewed May 20, 2016
View reviewed changes
pkg/kubectl/cmd/create.go

_, ok := obj.Annotations[service.AnnotationLoadBalancerSourceRangesKey]
if ok && len(obj.Spec.LoadBalancerSourceRanges) > 0 {
msg := fmt.Sprintf(`You have specified both LoadBalancerSourceRanges field and annotation [service.beta.kubernetes.io/load-balancer-source-ranges].
Copy link
Member

Choose a reason for hiding this comment

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

if we're going to do this sort of message, why not just print for the annotation, regardless of whether they have both? You're using a deprecated annotation, please use the field instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@thockin
Copy link
Member

thockin commented May 20, 2016

@k8s-bot test this: github issue #IGNORE

I have no idea which flake it is - the logs are utterly useless

@thockin
Copy link
Member

thockin commented May 20, 2016

@freehan test fail in pkg/client/cache please fix and reapply LGTM

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@freehan
Copy link
Contributor Author

freehan commented May 20, 2016 •
edited
Loading

@k8s-bot test this issue: #IGNORE

It looks to me that all unit/integration tests passed. Not sure why it complains

@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 20, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 21, 2016
@thockin
Copy link
Member

thockin commented May 23, 2016

Needs rebase :(

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2016
@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2016
@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 26, 2016
@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016
@freehan freehan added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2016
@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 May 28, 2016

GCE e2e build/test passed for commit 466bc38.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a550cf1 into kubernetes:master May 28, 2016
dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018
Merge pull request kubernetes#25826 from freehan/svcsourcerange
ba2df87 
Automatic merge from submit-queue

promote sourceRange into service spec

@thockin  one more for your pile

I will add docs at `http://releases.k8s.io/HEAD/docs/user-guide/services-firewalls.md`

cc: @justinsb 

Fixes: kubernetes#20392
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@thockin thockin

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
5 participants
@freehan @googlebot @thockin @k8s-github-robot @k8s-bot