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

gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup #63696

Merged
merged 1 commit into from
May 16, 2018

Conversation

grosskur
Copy link
Contributor

@grosskur grosskur commented May 11, 2018

MASTER_ADVERTISE_ADDRESS is used to set the --advertise-address flag
for the apiserver. It's useful for running the apiserver behind a load
balancer.

However, if PROJECT_ID, TOKEN_URL, TOKEN_BODY, and NODE_NETWORK are
all set, the GCE VM's external IP address will be fetched and used
instead and MASTER_ADVERTISE_ADDRESS will be ignored.

Change this behavior so that MASTER_ADVERTISE_ADDRESS takes precedence
because it's more specific. We still fall back to using the VM's
external IP address if the other variables are set.

Also: Move the setting of --ssh-user and --ssh-keyfile based on
PROXY_SSH_USER) to a top-level block because this is common to all
codepaths.

GCE: Fix to make the built-in `kubernetes` service properly point to the master's load balancer address in clusters that use multiple master VMs.

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 11, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 11, 2018
@grosskur
Copy link
Contributor Author

I've now signed the CNCF CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 11, 2018
@bowei
Copy link
Member

bowei commented May 13, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 13, 2018
@bowei
Copy link
Member

bowei commented May 13, 2018

/assign

@bowei
Copy link
Member

bowei commented May 13, 2018

/assign @dnardo

params+=" --advertise-address=${MASTER_ADVERTISE_ADDRESS}"
elif [[ -n "${PROJECT_ID:-}" && -n "${TOKEN_URL:-}" && -n "${TOKEN_BODY:-}" && -n "${NODE_NETWORK:-}" ]]; then
local -r vm_external_ip=$(curl --retry 5 --retry-delay 3 ${CURL_RETRY_CONNREFUSED} --fail --silent -H 'Metadata-Flavor: Google' "http://metadata/computeMetadata/v1/instance/network-interfaces/0/access-configs/0/external-ip")
params+=" --advertise-address=${vm_external_ip}"
Copy link
Member

@bowei bowei May 13, 2018

Choose a reason for hiding this comment

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

Seems like this was set only if PROXY_SSH_USER is set in the previous code. Was that wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. I looked at the git history but couldn't figure out why --advertise-address was only set if PROXY_SSH_USER was set. To be safe, I've updated the diff to leave that entire block as-is.

fi
elif [ -n "${MASTER_ADVERTISE_ADDRESS:-}" ]; then
params="${params} --advertise-address=${MASTER_ADVERTISE_ADDRESS}"
if [ -n "${MASTER_ADVERTISE_ADDRESS:-}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

use the double bracket test to be consistent (e.g. "[[" and "]]")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if [ -n "${MASTER_ADVERTISE_ADDRESS:-}" ]; then
params+=" --advertise-address=${MASTER_ADVERTISE_ADDRESS}"
elif [[ -n "${PROJECT_ID:-}" && -n "${TOKEN_URL:-}" && -n "${TOKEN_BODY:-}" && -n "${NODE_NETWORK:-}" ]]; then
local -r vm_external_ip=$(curl --retry 5 --retry-delay 3 ${CURL_RETRY_CONNREFUSED} --fail --silent -H 'Metadata-Flavor: Google' "http://metadata/computeMetadata/v1/instance/network-interfaces/0/access-configs/0/external-ip")
Copy link
Member

Choose a reason for hiding this comment

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

why not use get-metadata-value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. I initially developed this patch on an older branch that didn't have get-metadata-value. Switched to use get-metadata-value now.

@dnardo
Copy link
Contributor

dnardo commented May 14, 2018

/lgtm other than Bowei's comments.

Was kubeclt logs/exec tested with this change (in all cases) ?

@grosskur
Copy link
Contributor Author

Thanks for the review!

@bowei: I've pushed a new diff that addresses your comments.

@dnardo: I've tested both kubectl exec and kubectl logs in all cases now, and verified they still work.

@grosskur
Copy link
Contributor Author

/retest

MASTER_ADVERTISE_ADDRESS is used to set the --advertise-address flag
for the apiserver. It's useful for running the apiserver behind a load
balancer.

However, if PROJECT_ID, TOKEN_URL, TOKEN_BODY, and NODE_NETWORK are
all set, the GCE VM's external IP address will be fetched and used
instead and MASTER_ADVERTISE_ADDRESS will be ignored.

Change this behavior so that MASTER_ADVERTISE_ADDRESS takes precedence
because it's more specific. We still fall back to using the VM's
external IP address if the other variables are set.

Also: Pass --ssh-user and --ssh-keyfile flags if both PROXY_SSH_USER
and MASTER_ADVERTISE_ADDRESS is set.
@grosskur
Copy link
Contributor Author

/test pull-kubernetes-integration

@bowei
Copy link
Member

bowei commented May 16, 2018

/approve no-issue

@bowei
Copy link
Member

bowei commented May 16, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, dnardo, grosskur

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 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit e392f5b into kubernetes:master May 16, 2018
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels May 18, 2018
k8s-github-robot pushed a commit that referenced this pull request May 22, 2018
…96-upstream-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

Cherry pick of #63696 on release-1.8.

#63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

```release-note
NONE
```
k8s-github-robot pushed a commit that referenced this pull request May 25, 2018
…96-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

Cherry pick of #63696 on release-1.10.

#63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

```release-note
NONE
```
k8s-github-robot pushed a commit that referenced this pull request May 30, 2018
…96-upstream-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

Cherry pick of #63696 on release-1.9.

#63696: gce: Prefer MASTER_ADVERTISE_ADDRESS in apiserver setup

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants