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

Add proxy env to /etc/environment #687

Merged
merged 1 commit into from
Sep 26, 2019
Merged

Add proxy env to /etc/environment #687

merged 1 commit into from
Sep 26, 2019

Conversation

thz
Copy link
Contributor

@thz thz commented Sep 25, 2019

Add proxy env to /etc/environment

#kubeone markers are used to allow reset and idempotency
What this PR does / why we need it:

NONE

`#kubeone` markers are used to allow reset and idempotency
@kubermatic-bot kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 25, 2019
@kron4eg
Copy link
Member

kron4eg commented Sep 25, 2019

So what’s the rationale behind this?

@thz
Copy link
Contributor Author

thz commented Sep 25, 2019

It makes sure that all shell-sessions on the system will have the specified proxy environment. Kubeadm especially needs to pass that through to supply kube-controller-manager and others with the environment variables. I would argue that it is the right thing to do to supply proxy environment to the whole machine.

@kron4eg
Copy link
Member

kron4eg commented Sep 25, 2019

I mean, /etc/environment is just a convention and is voluntary to source, so what’s the big difference compared to existing kubeone specific file?

Remember that control plane machines are not foe general unix usage, they have single responsibility to host CP components.

P.S.
I wonder what machine-controller doing on this regard.

@thz
Copy link
Contributor Author

thz commented Sep 25, 2019

/etc/environment is more than just a convention. it is not even meant for being sourced. that is why there is no export statement for the variables, which would be needed to make it useful for sourcing.

/etc/environment is the default key-value file of the pam module pam_env. that mechanism makes sure that pam injects the variables to all "sessions".

@thz
Copy link
Contributor Author

thz commented Sep 25, 2019

P.S.
I wonder what machine-controller doing on this regard.

machine-controller is writing /etc/environment by cloud-init directives.

@kron4eg
Copy link
Member

kron4eg commented Sep 25, 2019

Maybe then let’s replace kubeone specific env with /etc/environment?

@thz
Copy link
Contributor Author

thz commented Sep 25, 2019

Maybe then let’s replace kubeone specific env with /etc/environment?

I thought about that too. I think it is good to have /etc/kubeone/proxy-env as a "transport". Also /etc/environment needs you to re-open your shell session to get the environment. For now I would say keeping both is the better choice.

Copy link
Member

@xmudrii xmudrii left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
/hold
@kron4eg Do you have some more comments?

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2019
@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d0d1f98ad4bdebd8199480f83951839e244862ab

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thz, xmudrii

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2019
@kron4eg
Copy link
Member

kron4eg commented Sep 26, 2019

/hold cancel

No, it good.

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2019
@kubermatic-bot kubermatic-bot merged commit dd6ff10 into master Sep 26, 2019
@kubermatic-bot kubermatic-bot added this to the v0.10 milestone Sep 26, 2019
@kubermatic-bot kubermatic-bot deleted the thz/proxy-env branch September 26, 2019 12:08
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. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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