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

Modified OS detection logic when updating http proxy settings. #3587

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Modified OS detection logic when updating http proxy settings. #3587

merged 1 commit into from
Oct 13, 2017

Conversation

KashifSaadat
Copy link
Contributor

Reduce duplication in configuring http proxy settings by writing it to the system-wide /etc/environment file and sourcing this in accordingly for the different services (docker, package management).

I have tested and this now correctly covers CoreOS by resolving the following bugs (I don't believe any issues were raised for these):

  • Docker config file was located in /etc/sysconfig/docker as opposed to /etc/default/docker
  • The /etc/lsb-release file exists for CoreOS, so the bootstrap script was incorrectly attempting to write proxy settings into the apt proxy config file

These changes should cover CoreOS, Debian, Ubuntu, RedHat distributions including CentOS and Fedora.

NOTE: A nodeup image will need to be built for these changes to work as expected, as now we rely on nodeup to update the proxy settings within the docker config (by sourcing in the env vars set within /etc/environment.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2017
@KashifSaadat
Copy link
Contributor Author

/assign @justinsb
/assign @chrislovecnm
/assign @gambol99

@KashifSaadat
Copy link
Contributor Author

KashifSaadat commented Oct 10, 2017 •
edited
Loading

There was a comment mentioning moving all this logic to nodeup. I don't think this should be the case, as you may require the proxy settings to be set to pull down the nodeup binary. So I've only moved the docker config part into nodeup which cleans up the bootstrap logic a little.

gambol99
gambol99 suggested changes Oct 10, 2017
View reviewed changes
pkg/model/bootstrapscript.go Outdated
buffer.WriteString("echo DefaultEnvironment=\\\"http_proxy=${http_proxy}\\\" \\\"https_proxy=${http_proxy}\\\"")
buffer.WriteString(" \\\"NO_PROXY=${no_proxy}\\\" \\\"no_proxy=${no_proxy}\\\"")
buffer.WriteString(" >> /etc/systemd/system.conf\n")
buffer.WriteString("echo \"export http_proxy=" + httpProxyURL + "\" >> /etc/environment\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps using `` so you don't need to blackslash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I did think initially about doing this, all the backslashes make it difficult to read. I'll fix up and test..

@gambol99
Copy link
Contributor

other than the one comment ... looks good

@KashifSaadat
Copy link
Contributor Author

Applied the suggested fix, no changes to the generated bootstrap config and seems to work. Cheers @gambol99 👍

@KashifSaadat KashifSaadat force-pushed the proxy-updates branch 3 times, most recently from d249c0a to 0b356b7 Compare October 11, 2017 20:44
@KashifSaadat
Copy link
Contributor Author

/retest

@chrislovecnm
Copy link
Contributor

which OS's have you tested this on?

@KashifSaadat
Copy link
Contributor Author

Hi @chrislovecnm, I have tested this successfully with the following images:

  • kope.io/k8s-1.7-debian-jessie-amd64-hvm-ebs-2017-07-28
  • coreos.com/CoreOS-stable-1409.7.0-hvm

@chrislovecnm
Copy link
Contributor

Can we get RHEL or fedora tested :) We have RHEL customers using this feature.

@KashifSaadat
Copy link
Contributor Author

Sure, I've given this a go with the following images:

  • CentOS-7 (1708)
  • Fedora-Atomic-26-20171003.0.x86_64

CentOS worked fine. Fedora failed, however this is for an unrelated reason. The above Fedora image falls into the following code block, however it is unsuccessfully detected and so nodeup fails: https://github.com/kubernetes/kops/blob/master/nodeup/pkg/distros/identify.go#L92

I can add support for this, but I think it makes sense to do as a separate PR. What do you think?

@chrislovecnm
Copy link
Contributor

Separate pr ;)

@chrislovecnm
Copy link
Contributor

You may not want to run atomic, just vanilla

@KashifSaadat
Copy link
Contributor Author

Ah ok, just tried with the following Fedora image and same issue:

  • Fedora-Cloud-Base-26-20170614.n.1.x86_64-eu-west-1-HVM-standard-0

I'll get another PR raised for Fedora support.

@KashifSaadat
Copy link
Contributor Author

KashifSaadat commented Oct 13, 2017 •
edited
Loading

@chrislovecnm so it turns out the egressProxy functionality never worked successfully with RHEL (both pre and post-PR), because for some reason it didn't load the no_proxy env var and so all AWS metadata web calls were being routed via the proxy by mistake.

I've added a fix for this (source the environment file in the kops-configuration service) and tested successfully with the following image:

  • Red Hat Enterprise Linux 7.4 (HVM)

@KashifSaadat
Copy link
Contributor Author

/test pull-kops-e2e-kubernetes-aws

Not sure why that failed, the last update was an inclusion of EnvironmentFile within kops-configuration.service. Will run again and see, hopefully was just an odd flake.

chrislovecnm
chrislovecnm approved these changes Oct 13, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

As always awesome work!!

@chrislovecnm
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 Oct 13, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

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 Oct 13, 2017
@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.

@k8s-github-robot k8s-github-robot merged commit 184db9a into kubernetes:master Oct 13, 2017
@KashifSaadat
Copy link
Contributor Author

Thanks! :)

@KashifSaadat KashifSaadat deleted the proxy-updates branch October 14, 2017 00:47
@while1eq1 while1eq1 mentioned this pull request Aug 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm approved these changes

@gambol99 gambol99 gambol99 approved these changes

Assignees

@justinsb justinsb

@chrislovecnm chrislovecnm

@gambol99 gambol99

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@KashifSaadat @gambol99 @chrislovecnm @k8s-github-robot @justinsb @k8s-ci-robot