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

warning on network phase #3022

Closed

Conversation

chrislovecnm
Copy link
Contributor

This will allow for further network configurations such as ignoring tags, not having IG, not having NAT gateways. These are multiple issues that we can close with this. This would also allow for the use of private networks in regions that do not support nat gateways. A user couple build the network themselves and build there own nat boxes.

Fixes: #780, #1530, #971

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017
@chrislovecnm
Copy link
Contributor Author

/assign @justinsb

@justinsb
Copy link
Member

The default should be enforcement. We can discuss a feature flag or something like that for "just warn", but I would strongly prefer to add features where they are missing, not just turn off our verification. It's too much of a time sink otherwise to find out what went wrong - both for the user & for us.

private networks in regions that do not support NAT gateways

Let's add support for this

@chrislovecnm
Copy link
Contributor Author

@justinsb we can disagree on this one. There are just too many different crazy network configs out there, and we cannot hope to support them all. I agree that we should implicitly support no gateway, and I have a PR inbound. Let's bring this up on next developer office hours.

@andrewsykim @blakebarnett @sethpollack any opinion on this?

You guys may not be familiar with the new phases, but in a nutshell, @justinsb has broken up iam, network, cluster into different phases. Each phase can be run separately. During the cluster phase, it is checking that networking phase has been completed as specified, but I would like to relax that check to just a warning, not requiring the network that we recommend/expected.

@andrewsykim
Copy link
Member

Skipping network config verifications sounds like it could lead to more issues down the road. Exposing and letting users change the lifecycle for each phase definitely seems reasonable though.

@chrislovecnm
Copy link
Contributor Author

Sounds like we need a lifecycle API section then ;)

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

Associated issue: 780

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2017
@justinsb
Copy link
Member

justinsb commented Aug 6, 2017

My instinct would be that we don't want an API section, we want flags on kops update instead.

@chrislovecnm
Copy link
Contributor Author

@justinsb we can do flags, but I am wondering if this needs to be in the API, because of kops server.

@justinsb
Copy link
Member

I would love to get it so that network is a separate object (see #3191), which is how I see it being in the API.

But I'm not sure how putting a field into the API would really work, because you would typically want to run both lifecycles at different times on the same Cluster object. I don't think it makes sense to change the object so that the different phases run (?) Am I missing a trick?

@k8s-github-robot
Copy link

@chrislovecnm PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2017
@k8s-github-robot k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2017
@justinsb justinsb added this to the backlog milestone Sep 22, 2017
@justinsb justinsb added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2017
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

Prevent issues from auto-closing with an /lifecycle frozen comment.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or @fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 6, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten
/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 9, 2018
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-changes needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lack of Internet Gateway is Fatal Error on Existing Private VPC
6 participants