Update the publicDNSQueryURI in DNS Check to https://quay.io #4477

vyasgun · 2024-11-22T09:36:57Z

Relates to: Issue #4218 (error code 301 was seen in this issue)

Solution/Idea

Change the publicDNSQueryURI to https to prevent 301 Moved Permanently during DNS check.

The alternate is to add -sSL flags to the curl command during the check for a quiet follow to any redirects.

openshift-ci · 2024-11-22T09:37:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

gbraad · 2024-11-22T11:23:47Z

pkg/crc/services/dns/dns.go

@@ -119,7 +119,7 @@ func CheckCRCPublicDNSReachable(serviceConfig services.ServicePostStartConfig) (
 		// try without using proxy
 		proxyConfig = &httpproxy.ProxyConfig{}
 	}
-	curlArgs := []string{"--head", publicDNSQueryURI}
+	curlArgs := []string{"--head", publicDNSQueryURI, "-L"}


Follow is good, but I think it is better to do

curl -sSL

-S: This option is used in conjunction with -s (silent). It stands for "show error." When used with -s, it will still suppress the progress meter but will show error messages if the request fails. This could be beneficial for logging and debugging purposes since it reduces noise. It might even be possible to return the actual error as user-facing information. WDYT?

Thank you for the review - I updated the PR to use the https://quay.io as I feel it's simpler to just update the URL.

gbraad

97ee5fc#r1853762839 => Perhaps the error message can be helpful to the user.

kaovilai

It's a DNS check, not a what's showing at the website check. Why is this needed?

kaovilai · 2024-11-25T16:30:33Z

@vyasgun

DEBU SSH command results: err: , output: HTTP/1.1 301 Moved Permanently

is not an error.

err: <nil>

vyasgun · 2024-11-25T16:33:51Z

@kaovilai yes but checking at https://quay.io instead of http://quay.io returns 200 which is a clearer way to interpret that the check succeeded and also serves the same purpose of querying quay.io.

kaovilai

I would prefer the test actually be a dns query as the name implies...

❯ dig quay.io +short
54.243.17.128
54.85.81.91
3.219.175.113
54.210.148.48
52.54.136.228
52.55.96.46

~
❯ dig non-exist.quay.io +short

and check for non-empty output.

but this is a step in the right direction. just saying it's not solving an error.

openshift-ci · 2024-11-25T16:36:47Z

@kaovilai: changing LGTM is restricted to collaborators

In response to this:

I would prefer the test actually be a dns query as the name implies...
❯ dig quay.io +short
54.243.17.128
54.85.81.91
3.219.175.113
54.210.148.48
52.54.136.228
52.55.96.46

~
❯ dig non-exist.quay.io +short
and check for non-empty output.

but this is a step in the right direction. just saying it's not solving an error.

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-sigs/prow repository.

openshift-ci · 2024-11-25T18:32:18Z

@vyasgun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration-crc	`ae2a8f8`	link	true	`/test integration-crc`
ci/prow/e2e-crc	`ae2a8f8`	link	true	`/test e2e-crc`

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

praveenkumar · 2024-11-26T06:39:19Z

Relates to: Issue #4218 (error code 301 was seen in this issue)

@vyasgun This doesn't look like related to this issue, or I am missing something here.

vyasgun · 2024-11-26T07:02:41Z

@praveenkumar you are right -- it is not directly related to the issue except that as mentioned in the description, it was seen in the logs of this issue and prompted some discussion and I was looking into it. This is not even an error but just adding https to the query URI improves clarity of the debug message as well as performing the same task of querying quay.io. Should I update the description to something else to prevent confusion?

cfergeau · 2024-11-26T10:01:16Z

I would prefer the test actually be a dns query as the name implies...
❯ dig quay.io +short
54.243.17.128
54.85.81.91
3.219.175.113
54.210.148.48
52.54.136.228
52.55.96.46

~
❯ dig non-exist.quay.io +short
and check for non-empty output.

but this is a step in the right direction. just saying it's not solving an error.

This used to be tested this way, but this does not work when an http proxy is involved. This was changed in 52279b3eaf7c7b , details can be found in the commit log.

We could also have two different code paths for proxy/non-proxy, not sure this was discussed/considered at the time of this PR.

praveenkumar · 2024-12-10T09:24:47Z

@vyasgun Can you also update the commit log which have details around why we are changing it?

Querying at https://quay.io instead of http://quay.io returns 200 which is a clearer way to interpret that the check succeeded. This also serves the purpose of querying quay.io without any confusing debug message.

vyasgun · 2024-12-10T10:06:58Z

@praveenkumar Just updated -- is this commit message okay?

openshift-ci · 2024-12-10T10:43:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gbraad, kaovilai

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:

~~OWNERS~~ [gbraad]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gbraad · 2024-12-10T10:48:17Z

This test does too much to be honest; hence my comments. This is supposed to be a DNS test, but has become proxy aware (as expected); but we do not actually error correctly to test if the proxy failed (to perform a remote lookup). The idea to move away to a clean 200 was to avoid involving more HTTP traffic; the redirect is a result of enforced HTTPS; eg HSTS. Which means that in the future, a possible certificate issue also can cause issues. Let's just leave it at this for now.

gbraad · 2024-12-10T10:50:46Z

We could also have two different code paths for proxy/non-proxy, not sure this was discussed/considered at the time of this PR.

We have discussed this, but we most likely need to be more strategic about what to test (especially dependencies), and how to report this back. If this test fails due to the plethora of aforementioned issues, we do not properly indicate that to the user (and Podman Desktop!).

openshift-ci bot added the do-not-merge/work-in-progress label Nov 22, 2024

gbraad reviewed Nov 22, 2024

View reviewed changes

kaovilai reviewed Nov 22, 2024

View reviewed changes

vyasgun force-pushed the pr/curl-fix branch from 97ee5fc to ae2a8f8 Compare November 25, 2024 14:23

vyasgun changed the title ~~Add redirect to curl command in DNS check~~ Update the publicDNSQueryURI in DNS Check to https://quay.io Nov 25, 2024

vyasgun marked this pull request as ready for review November 25, 2024 16:22

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 25, 2024

openshift-ci bot requested review from anjannath and cfergeau November 25, 2024 16:22

This comment was marked as off-topic.

Sign in to view

kaovilai approved these changes Nov 25, 2024

View reviewed changes

dnsCheck: update the publicDNSQueryURI to https://quay.io

3c1c67d

Querying at https://quay.io instead of http://quay.io returns 200 which is a clearer way to interpret that the check succeeded. This also serves the purpose of querying quay.io without any confusing debug message.

vyasgun force-pushed the pr/curl-fix branch from ae2a8f8 to 3c1c67d Compare December 10, 2024 10:06

gbraad approved these changes Dec 10, 2024

View reviewed changes

openshift-ci bot assigned gbraad Dec 10, 2024

openshift-ci bot added the lgtm label Dec 10, 2024

openshift-ci bot added the approved label Dec 10, 2024

gbraad merged commit c86d75d into crc-org:main Dec 10, 2024
22 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the publicDNSQueryURI in DNS Check to https://quay.io #4477

Update the publicDNSQueryURI in DNS Check to https://quay.io #4477

vyasgun commented Nov 22, 2024 •

edited

Loading

openshift-ci bot commented Nov 22, 2024

gbraad Nov 22, 2024 •

edited

Loading

vyasgun Nov 25, 2024

gbraad left a comment •

edited

Loading

kaovilai left a comment

kaovilai commented Nov 25, 2024

This comment was marked as off-topic.

vyasgun commented Nov 25, 2024 •

edited

Loading

kaovilai left a comment

openshift-ci bot commented Nov 25, 2024

openshift-ci bot commented Nov 25, 2024

praveenkumar commented Nov 26, 2024

vyasgun commented Nov 26, 2024

cfergeau commented Nov 26, 2024

praveenkumar commented Dec 10, 2024

vyasgun commented Dec 10, 2024

openshift-ci bot commented Dec 10, 2024

gbraad commented Dec 10, 2024 •

edited

Loading

gbraad commented Dec 10, 2024

Update the publicDNSQueryURI in DNS Check to https://quay.io #4477

Update the publicDNSQueryURI in DNS Check to https://quay.io #4477

Conversation

vyasgun commented Nov 22, 2024 • edited Loading

Solution/Idea

openshift-ci bot commented Nov 22, 2024

gbraad Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

vyasgun Nov 25, 2024

Choose a reason for hiding this comment

gbraad left a comment • edited Loading

Choose a reason for hiding this comment

kaovilai left a comment

Choose a reason for hiding this comment

kaovilai commented Nov 25, 2024

This comment was marked as off-topic.

vyasgun commented Nov 25, 2024 • edited Loading

kaovilai left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 25, 2024

openshift-ci bot commented Nov 25, 2024

praveenkumar commented Nov 26, 2024

vyasgun commented Nov 26, 2024

cfergeau commented Nov 26, 2024

praveenkumar commented Dec 10, 2024

vyasgun commented Dec 10, 2024

openshift-ci bot commented Dec 10, 2024

gbraad commented Dec 10, 2024 • edited Loading

gbraad commented Dec 10, 2024

vyasgun commented Nov 22, 2024 •

edited

Loading

gbraad Nov 22, 2024 •

edited

Loading

gbraad left a comment •

edited

Loading

vyasgun commented Nov 25, 2024 •

edited

Loading

gbraad commented Dec 10, 2024 •

edited

Loading