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

Set default value of report-node-internal-ip-address as true. #2072

Closed
wants to merge 1 commit into from
Closed

Set default value of report-node-internal-ip-address as true. #2072

wants to merge 1 commit into from

Conversation

gyliu513
Copy link
Contributor

Using default value of report-node-internal-ip-address as true can
make sure the ingress can always return ADDRESS, this also keep
backward compatibility with ingress 0.9.0.beta.12 and its previous
releases.

/cc @aledbf

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Using default value of report-node-internal-ip-address as true can
make sure the ingress can always return ADDRESS, this also keep
backward compatibility with ingress 0.9.0.beta.12 and its previous
releases.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 12, 2018
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gyliu513
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

Assign the PR to them by writing /assign @aledbf in a comment when ready.

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

The pull request process is described here

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 12, 2018
@gyliu513 gyliu513 mentioned this pull request Feb 12, 2018
@gyliu513
Copy link
Contributor Author

Set default value of report-node-internal-ip-address as true.

@aledbf
Copy link
Member

aledbf commented Feb 12, 2018

@gyliu513 we cannot change this default. If the user wants to use the internal IP it must be done explicitly

@gyliu513
Copy link
Contributor Author

gyliu513 commented Feb 12, 2018

Why not let the ingress itself to decide return internal or external ip? I think that we should keep default behavior as before: Using external ip as priority, but if no external ip, use internal ip. /cc @aledbf

@gyliu513
Copy link
Contributor Author

The ingress 0.9.0.beta-12 behavior is Using external ip as priority, but if no external ip, use internal ip. We can let end user configure report-node-internal-ip-address as false if they do not want to use internal ip.

@aledbf
Copy link
Member

aledbf commented Feb 12, 2018

Why not let the ingress itself to decide return internal or external ip?

Because internal IP only makes sense in minikube or baremetal where you already need to change the defaults like the use of nodePort

@gyliu513
Copy link
Contributor Author

But this changed the behavior since 0.9.0-beta.12, when I upgrade, I have to manually add this parameter, I think this is not good behavior for upgrade, comments? @aledbf

@gyliu513
Copy link
Contributor Author

Also I can see many customer using Kubernetes distribution in their own data center, and they usually do not have external ip, so that's why I want to set the default value as true.

@aledbf
Copy link
Member

aledbf commented Feb 12, 2018

But this changed the behavior since 0.9.0-beta.12, when I upgrade, I have to manually add this parameter, I think this is not good behavior for upgrade, comments? @aledbf

The version 0.9.0-beta.12 was released on Aug 30, 2017, and the change of the defaults was requested by other users having the same problem with the opposite case than you.

@aledbf
Copy link
Member

aledbf commented Feb 12, 2018

Also I can see many customer using Kubernetes distribution in their own data center, and they usually do not have external ip, so that's why I want to set the default value as true.

I understand that but most of the users are using a cloud provider, not baremetal. As I said, if you are using Kubernetes in baremetal you already have to change lots of configurations, like get used to deal with nodePorts or using something like metallb.

@gyliu513
Copy link
Contributor Author

Thanks @aledbf , let me upgrade my ingress by setting the report-node-internal-ip-address as true by default for now.

@gyliu513 gyliu513 closed this Feb 12, 2018
@gyliu513 gyliu513 deleted the internal-ip-true branch February 12, 2018 02:26
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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants