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

Rework proxy support #7095

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Rework proxy support #7095

merged 1 commit into from
Jan 11, 2021

Conversation

champtar
Copy link
Contributor

@champtar champtar commented Jan 5, 2021

What type of PR is this?
/kind feature

What this PR does / why we need it:
no_proxy is a pain to get right, and having proxy variables present can have side effect
It's better to only configure what require proxy:

  • the runtime (containerd/docker/crio)
  • the package manager
  • the download tasks

Which issue(s) this PR fixes:
Fixes #7100

Special notes for your reviewer:
Only tested on CentOS 8 for now (cluster and upgrade)

Does this PR introduce a user-facing change?:

Rework proxy support

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 5, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @champtar. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 5, 2021
@champtar champtar marked this pull request as ready for review January 5, 2021 17:06
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 5, 2021
@floryut
Copy link
Member

floryut commented Jan 5, 2021

Given all the proxy issues we had, maybe hold this one and merge after 2.15 ?
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2021
@champtar
Copy link
Contributor Author

champtar commented Jan 5, 2021

Right now your cluster breaks on upgrade (#7100), so not sure we want to hold, but need some testing on Ubuntu/Debian for sure.
@floryut if you know some proxy user can you ping them ?

@champtar
Copy link
Contributor Author

champtar commented Jan 5, 2021

To be clear I don't want to delay 2.15, but I'll want to backport this once it's tested by someone else.
Setting proxy env variables everywhere is the wrong approach IMO as we have some tricky behavior changes, and in the end we don't use the proxy in that much places.

@champtar
Copy link
Contributor Author

champtar commented Jan 5, 2021

I can split the first 2 commits in an other PR

@floryut
Copy link
Member

floryut commented Jan 6, 2021

Right now your cluster breaks on upgrade (#7100), so not sure we want to hold, but need some testing on Ubuntu/Debian for sure.
@floryut if you know some proxy user can you ping them ?

Ok indeed if there is some issue then we don't need to think too much.
I can try but not sure to find anybody with proxy setup and time to test

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2021
no_proxy is a pain to get right, and having proxy variables present causes issues
(k8s components get proxy configuration after upgrade, see kubernetes-sigs#7100)

It's better to only configure what require proxy:
- the runtime (containerd/docker/crio)
- the package manager + apt_key
- the download tasks

Tested with the following clusters
- 4 CentOS 8 nodes
- 1 Ubuntu 20.04 node

Signed-off-by: Etienne Champetier <[email protected]>
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

/approve
Looks fine to me, WDYT @EppO @oomichi @LuckySB ?

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2021
@LuckySB
Copy link
Contributor

LuckySB commented Jan 11, 2021

/approve

I don't have a proxy, but it looks ok

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: champtar, floryut, LuckySB

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

@champtar
Copy link
Contributor Author

champtar commented Jan 11, 2021

There is a small risk of breakage in the contrib as I have not tested them, but the fix would be to add environment: "{{ proxy_env }}" or even better to use kubespray download_file.yml. I did grep for get_url/uri. In any cases it would be clean breakage pretty easy to troubleshoot and fix.

@EppO
Copy link
Contributor

EppO commented Jan 11, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit a790935 into kubernetes-sigs:master Jan 11, 2021
@champtar champtar deleted the proxy branch January 11, 2021 15:30
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Feb 1, 2021
no_proxy is a pain to get right, and having proxy variables present causes issues
(k8s components get proxy configuration after upgrade, see kubernetes-sigs#7100)

It's better to only configure what require proxy:
- the runtime (containerd/docker/crio)
- the package manager + apt_key
- the download tasks

Tested with the following clusters
- 4 CentOS 8 nodes
- 1 Ubuntu 20.04 node

Signed-off-by: Etienne Champetier <[email protected]>
@floryut floryut mentioned this pull request May 11, 2021
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxy is configured for K8S components after upgrade-cluster.yml
5 participants