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.

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

Fix load-balancer firewall messages #11254

Merged
merged 1 commit into from
Jul 15, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Jul 14, 2015

No description provided.

@thockin
Copy link
Member Author

thockin commented Jul 14, 2015

@bgrant0607 @bprashanth

cluster. If you want to expose this service to the external internet, you may
need to set up firewall rules for the service port(s) (%s) to serve traffic.

See http://releases.k8s.io/HEAD/docs/services-firewalls.md for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: since this is kubectl, it feels like we can confirm and tell them one way or another (not expecting it in this pr).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ai gree we could, but we pretty much need that to come from the
cloud-provider, which means from the master.

#11259

On Tue, Jul 14, 2015 at 1:12 PM, Prashanth B notifications@github.com
wrote:

In pkg/kubectl/cmd/create.go
#11254 (comment)
:

        out.Write([]byte(msg))
    }
    if obj.Spec.Type == api.ServiceTypeNodePort {
  •       msg := fmt.Sprintf(`
    
  •           You have exposed your service on an external port on all nodes in your cluster.
    
  •           If you want to expose this service to the external internet, you may need to set up
    

- firewall rules for the service port(s) (%s) to serve traffic.

  •           See https://github.com/GoogleCloudPlatform/kubernetes/tree/master/docs/services-firewalls.md for more details.
    
  •           `, makePortsString(obj.Spec.Ports, true))
    
  •       msg := fmt.Sprintf(
    
  •           `You have exposed your service on an external port on all nodes in your
    
    +cluster. If you want to expose this service to the external internet, you may
    +need to set up firewall rules for the service port(s) (%s) to serve traffic.
    +
    +See http://releases.k8s.io/HEAD/docs/services-firewalls.md for more details.

Nit: since this is kubectl, it feels like we can confirm and tell them one
way or another (not expecting it in this pr).


Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11254/files#r34613478
.

@bprashanth
Copy link
Contributor

LGTM

@k8s-bot
Copy link

k8s-bot commented Jul 14, 2015

GCE e2e build/test passed for commit 8da6aef5acf9933e93890810aa304c033f30737a.

@thockin thockin force-pushed the dont-print-lb-firewall-warning branch from 8da6aef to 670141b Compare July 14, 2015 22:29
@k8s-bot
Copy link

k8s-bot commented Jul 14, 2015

GCE e2e build/test passed for commit 670141b1973dedc64123b4fb2dcf090527420522.

@thockin thockin force-pushed the dont-print-lb-firewall-warning branch from 670141b to 85a56ce Compare July 14, 2015 23:08
@thockin thockin closed this Jul 14, 2015
@thockin thockin reopened this Jul 14, 2015
@k8s-bot
Copy link

k8s-bot commented Jul 14, 2015

GCE e2e build/test passed for commit 85a56ce6d3efa9c42c253c9968125630263e4316.

@thockin thockin force-pushed the dont-print-lb-firewall-warning branch from 85a56ce to 9005c5b Compare July 15, 2015 00:52
@k8s-bot
Copy link

k8s-bot commented Jul 15, 2015

GCE e2e build/test passed for commit 9005c5b4d89ba2218b8afc1ef230728f84971304.

@bgrant0607
Copy link
Member

I'd prefer to eliminate the message than to just fix formatting. We don't have warnings about privileged mode, use of PD, or anything else that requires external action by the user.

If we're not going to eliminate it, how about we reduce it to 1 line:

See the [Services and Firewalls doc](docs/services-firewalls.md) regarding opening of firewalls for the service.

docs/services-firewalls.md is out of date, though. It still instructs GCE users to open the firewall with gcloud.

@thockin thockin force-pushed the dont-print-lb-firewall-warning branch 2 times, most recently from 182b0a9 to 2d4df16 Compare July 15, 2015 03:46
@thockin
Copy link
Member Author

thockin commented Jul 15, 2015

OK, message removed, docs updated.

On Tue, Jul 14, 2015 at 8:35 PM, Brian Grant notifications@github.com
wrote:

I'd prefer to eliminate the message than to just fix formatting. We don't
have warnings about privileged mode, use of PD, or anything else that
requires external action by the user.

If we're not going to eliminate it, how about we reduce it to 1 line:

See the Services and Firewalls doc regarding opening of firewalls for the service.

docs/services-firewalls.md is out of date, though. It still instructs GCE
users to open the firewall with gcloud.


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

@bgrant0607
Copy link
Member

LGTM
Thanks.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2015
@bgrant0607
Copy link
Member

cc @davidopp, who is planning to move services-firewalls.md

@k8s-bot
Copy link

k8s-bot commented Jul 15, 2015

GCE e2e build/test failed for commit 182b0a98e57b4f7e5a35afdb76a69f98927568f2.

@k8s-bot
Copy link

k8s-bot commented Jul 15, 2015

GCE e2e build/test passed for commit 2d4df16215e77e27e22142074df2b6961a93d7d9.

@thockin thockin force-pushed the dont-print-lb-firewall-warning branch from 2d4df16 to fe89298 Compare July 15, 2015 04:08
@thockin thockin closed this Jul 15, 2015
@thockin thockin reopened this Jul 15, 2015
@k8s-bot
Copy link

k8s-bot commented Jul 15, 2015

GCE e2e build/test passed for commit fe89298.

@bgrant0607
Copy link
Member

all green

let's merge now before @davidopp re-creates his PR to move services-firewalls.md.

thockin added a commit that referenced this pull request Jul 15, 2015
@thockin thockin merged commit cdc74dd into kubernetes:master Jul 15, 2015
@davidopp
Copy link
Member

It seems weird to have services-firewall.md at the top-level of the docs tree, but I couldn't decide where to put it, so I left it there in my PR. It seems like it belongs in user-guide?

@davidopp
Copy link
Member

(My PR actually didn't move services-firewall.md, because I couldn't figure out which subdir to put it in.)

zmerlynn added a commit that referenced this pull request Jul 15, 2015
…4-upstream-release-1.0

Automated cherry pick of #11254 upstream release 1.0
@thockin
Copy link
Member Author

thockin commented Jul 15, 2015

I'd say user-guide

On Wed, Jul 15, 2015 at 2:23 AM, David Oppenheimer <notifications@github.com

wrote:

(My PR actually didn't move services-firewall.md, because I couldn't
figure out which subdir to put it in.)


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

@zmerlynn zmerlynn mentioned this pull request Jul 17, 2015
@thockin thockin deleted the dont-print-lb-firewall-warning branch July 25, 2015 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants