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

CORS-3917: Add install config validation for service endpoints #9517

Open

barbacbd wants to merge 1 commit into openshift:main from barbacbd:CORS-3917

+52 −0

Contributor

barbacbd commented Feb 25, 2025

** check whether the user provided endpoints are reachable or not.

openshift-ci-robot added the jira/valid-reference label

Contributor

openshift-ci-robot commented Feb 25, 2025 •

edited by openshift-ci bot

Loading

@barbacbd: This pull request references CORS-3917 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

** check whether the user provided endpoints are reachable or not.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci bot requested review from patrickdillon and sadasu

February 25, 2025 15:08


          CORS-3917: Add install config validation for service endpoints

2c3dfc9

** check whether the user provided endpoints are reachable or not.

barbacbd force-pushed the CORS-3917 branch from 1cb4ca5 to 2c3dfc9 Compare

February 25, 2025 18:53

Contributor

openshift-ci bot commented Feb 25, 2025

@barbacbd: 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/e2e-azure-ovn-resourcegroup	`2c3dfc9`	link	false	`/test e2e-azure-ovn-resourcegroup`
ci/prow/e2e-aws-ovn	`2c3dfc9`	link	true	`/test e2e-aws-ovn`
ci/prow/okd-scos-e2e-aws-ovn	`2c3dfc9`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-vsphere-externallb-ovn	`2c3dfc9`	link	false	`/test e2e-vsphere-externallb-ovn`
ci/prow/okd-scos-images	`2c3dfc9`	link	true	`/test okd-scos-images`

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.

tthvo reviewed

View reviewed changes

Contributor

tthvo left a comment

LGTM, just some comments. Sorry for too many nits 😅

pkg/asset/installconfig/gcp/validation.go

+              			continue
+              		}
+              		if _, err := url.Parse(serviceEndpoint.URL); err != nil {
+              			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))

Contributor

tthvo Feb 26, 2025

Suggested change

      
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))
          
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoints").Index(id), serviceEndpoint.URL, err.Error()))

nit: The field name is serviceEndpoints, right?

pkg/asset/installconfig/gcp/validation.go

+              		if _, err := url.Parse(serviceEndpoint.URL); err != nil {
+              			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))
+              		} else if _, err := http.Head(serviceEndpoint.URL); err != nil {
+              			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))

Contributor

tthvo Feb 26, 2025

Suggested change

      
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))
          
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoints").Index(id), serviceEndpoint.URL, err.Error()))

nit: The field name is serviceEndpoints?

pkg/asset/installconfig/gcp/validation_test.go Show resolved Hide resolved

pkg/asset/installconfig/gcp/validation.go Show resolved Hide resolved

patrickdillon reviewed

View reviewed changes

Contributor

patrickdillon left a comment

/approve

By and large this looks good, just a few minor changes.

pkg/asset/installconfig/gcp/validation.go

Comment on lines +474 to +478

+              		// Ignore e2e.local for unit tests.
+              		if serviceEndpoint.URL == "e2e.local" {
+              			logrus.Warnf("Skipping service endpoint validation for %s", serviceEndpoint.Name)
+              			continue
+              		}

Contributor

patrickdillon Feb 26, 2025

I won't block on this, particularly as it is the same approach used for aws, but as a matter of principal it's not a good idea to adjust code specifically for unit tests. Instead, what we should have done for aws is essentially inject a mock http client.

pkg/asset/installconfig/gcp/validation.go

+              			logrus.Warnf("Skipping service endpoint validation for %s", serviceEndpoint.Name)
+              			continue
+              		}
+              		if _, err := url.Parse(serviceEndpoint.URL); err != nil {

Contributor

patrickdillon Feb 26, 2025

technically this should probably go in pkg/types as static validation, but I'm fine leaving it here.

pkg/asset/installconfig/gcp/validation.go

+              		if _, err := url.Parse(serviceEndpoint.URL); err != nil {
+              			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))
+              		} else if _, err := http.Head(serviceEndpoint.URL); err != nil {
+              			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))

Contributor

patrickdillon Feb 26, 2025

I think we probably want to wrap this error message with something like "error when connecting to endpoint:"

Suggested change

      
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, err.Error()))
          
            			allErrs = append(allErrs, field.Invalid(fieldPath.Child("serviceEndpoint").Index(id), serviceEndpoint.URL, fmt.Errorf("error when connecting to endpoint: %w", err.Error())))

Contributor

openshift-ci bot commented Feb 26, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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:

~~pkg/asset/installconfig/gcp/OWNERS~~ [patrickdillon]

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

openshift-ci bot added the approved label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved jira/valid-reference