fix ignition config creation (propagation of proxy settings) #18772
Conversation
|
@Luap99 there were comments from you Sorry, but I broke that branch and had to reopen PR There are fixes here, that you asked about |
|
@baude Could you please take a look at this PR |
esendjer
force-pushed
the
main
branch
3 times, most recently
from
34b6cc4 to
ad2249a
Compare
pkg/machine/e2e/proxy_test.go
Outdated
| if err := os.Unsetenv("HTTP_PROXY"); err != nil { | ||
| Fail("unable to unset HTTP_PROXY") | ||
| } | ||
| if err := os.Unsetenv("HTTPS_PROXY"); err != nil { | ||
| Fail("unable to unset HTTPS_PROXY") | ||
| } |
There was a problem hiding this comment.
Thanks, please use defer os.Unsetenv(...) instead directly after the setenv call.
The thing with ginkgo is that if a match above fails it stops executing this function and returns early, so if this test fails you will leak the env into all following test causing likely more errors.
There was a problem hiding this comment.
Done
For some unclear for me reason, a simple definition of defer os.Unsetenv("...") didn't work and there were leaks of the env into all following tests
Now, there is a slightly bulky defer in the test, but it works, it's simple and it's clear.
esendjer
force-pushed
the
main
branch
4 times, most recently
from
f8ef1ff to
306dbe6
Compare
pkg/machine/e2e/proxy_test.go
Outdated
| sshSession, err := mb.setName(name).setCmd(sshProxy.withSSHCommand([]string{"printenv", "HTTPS_PROXY"})).run() | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(sshSession).To(Exit(0)) | ||
| Expect(sshSession.outputToString()).To(ContainSubstring(proxyURL)) | ||
|
|
||
| sshSession, err = mb.setName(name).setCmd(sshProxy.withSSHCommand([]string{"printenv", "HTTPS_PROXY"})).run() |
There was a problem hiding this comment.
both are checking HTTPS_PROXY, I assume one of them should check HTTP_PROXY
| @@ -224,7 +226,17 @@ endif | |||
| # at reference-time (due to `=` and not `=:`). | |||
| _HLP_TGTS_RX = '^[[:print:]]+:.*?\#\# .*$$' | |||
| _HLP_TGTS_CMD = grep -E $(_HLP_TGTS_RX) $(MAKEFILE_LIST) | |||
| _HLP_TGTS_LEN = $(shell $(call err_if_empty,_HLP_TGTS_CMD) | cut -d : -f 1 | wc -L) | |||
| _HLP_TGTS_LEN = $(shell $(call err_if_empty,_HLP_TGTS_CMD) | cut -d : -f 1 | wc -L 2>/dev/null || echo "PARSING_ERROR") | |||
There was a problem hiding this comment.
I think it may be better to keep this in a separate commit as this is not related to the actual fix.
Makefile
Outdated
| $(warning On Darwin (MacOS) installed coreutils is necessary) | ||
| $(warning Use 'brew install coreutils' command to install coreutils on your system) | ||
| endif | ||
| else |
There was a problem hiding this comment.
I think this else can be dropped?
Signed-off-by: esendjer <[email protected]>
* the sequence of Ignition config creation was broken, so that the part responsible for propagation of proxy settings has been out of the final ignConfig * e2e test for proxy settings propagation Signed-off-by: esendjer <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: esendjer, Luap99 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 |
openshift-ci
bot
added
the
approved
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Reopened of #18759
In the v4.5.1 release, the propagation of proxy settings for QEMU VMs was broken.
The issue #13168 that was solved in the PR #13209 has come back.
@amilanoski mentioned the issue there
I could repeat the issue on my side as well, and the researching led me to the next:
The cause was in a broken sequence of Ignition config creation, such that, the part, responsible for propagation of proxy settings, was not included in the final ignConfig
This PR contains:
Does this PR introduce a user-facing change?
None