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 NodeInternalIP as a fallback to federation api-server nodeport service #46960

Merged
merged 1 commit into from
Jun 14, 2017
Merged

Add NodeInternalIP as a fallback to federation api-server nodeport service #46960

merged 1 commit into from
Jun 14, 2017

Conversation

lukaszo
Copy link
Contributor

@lukaszo lukaszo commented Jun 5, 2017 •
edited
Loading

Previously NodeLegacyHostIP was used as a fallback (see #41243) but in 1.7 it was removed (#44830)
Now clusters where nodes have not set ExternalIP can not be used by kubefed to setup federation.

cc @shashidharatd

kubefed will now configure NodeInternalIP as the federation API server endpoint when NodeExternalIP is unavailable for federation API servers exposed as NodePort services

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 5, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 5, 2017
@shashidharatd
Copy link

LGTM. In some non-cloud environment, nodes might not have NodeExternalIP and in such cases kubefed might not work.
But i want to hear from @madhusudancs, because he was opposed to fall-back to NodeInternalIP initially.

/cc @madhusudancs @kubernetes/sig-federation-pr-reviews

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 8, 2017

@madhusudancs ping

I consider it as a bugfix for 1.7.

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 12, 2017 •
edited
Loading

/assign @madhusudancs

@irfanurrehman
Copy link
Contributor

looks good to me too.
/assign @madhusudancs
/unassign

@marun marun added this to the v1.7 milestone Jun 12, 2017
@marun marun added the kind/bug Categorizes issue or PR as related to a bug. label Jun 12, 2017
@marun
Copy link
Contributor

marun commented Jun 12, 2017

Given the trivial nature of the fix I don't think an issue should be required.

@madhusudancs Bump, if this is going to make 1.7 it will need to be merged asap.

@madhusudancs
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lukaszo, madhusudancs

Associated issue: 41243

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 13, 2017
@marun
Copy link
Contributor

marun commented Jun 13, 2017

@lukaszo I think a release note is suggested here so that users know about the change in behavior.

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 13, 2017
@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 13, 2017

@marun done

@madhusudancs
Copy link
Contributor

@lukaszo I think the release note needs to be paraphrased a little bit. It sounds like a commit log targeted to developers. The target audience for this release note are kubefed users. Can you please phrase it in a way you would like your users to think about this change? Also see - #42092 (comment).

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 14, 2017

@madhusudancs what about now?

@madhusudancs
Copy link
Contributor

@lukaszo How about: kubefed will now configure NodeInternalIP as the federation API server endpoint when NodeExternalIP is unavailable for federation API servers exposed as NodePort services?

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 14, 2017

@madhusudancs perfect, updated :)

@madhusudancs madhusudancs removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jun 14, 2017
@madhusudancs
Copy link
Contributor

@lukaszo thanks for updating the release note. I have removed the do-not-merge label. PR should be good to go now.

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 14, 2017

@madhusudancs thank you

@lukaszo
Copy link
Contributor Author

lukaszo commented Jun 14, 2017

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit ce76bab into kubernetes:master Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@madhusudancs madhusudancs

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
7 participants
@lukaszo @shashidharatd @irfanurrehman @marun @madhusudancs @k8s-github-robot @k8s-ci-robot