Repository: kubernetes-sigs/gateway-api

Issue:
Validate conformance profiles suite flag values
<!-- Please only use this template for submitting enhancement requests -->

<!--
WARNING: depending on the scope of the enhancement you may be asked to use the
GEP process to build consensus around a proposed change. Please carefully read
through the GEP overview and include a link to the prerequisite GEP discussion
when submitting this issue:
https://gateway-api.sigs.k8s.io/geps/overview/
-->

**What would you like to be added**:

https://github.com/kubernetes-sigs/gateway-api/pull/2066 added a new CLI to run conformance profiles. This CLI accepts 
the following flags related to the implementation running the test suite:
- organization
- project
- url
- version
- contact

We need to validate that:
- the fields are not empty
- url is a valid url



Discussion:
@mlavacca: /area conformance@shaneutt: Seems like some good smaller improvements.

/triage accepted

I hesitate to consider it a `good-first-issue` because it requires someone work on the experimental CLI, and have an implementation they're attaching to conformance, e.t.c. but I do think it's relatively low-hanging fruit for anyone already in that boat.

Not required for GA, but should be worked on not too long after that.

/priority important-longterm@sunjayBhatia: I would argue `version` could maybe be a commit SHA or something similar not necessarily only a semver@shaneutt: > I would argue `version` could maybe be a commit SHA or something similar not necessarily only a semver

That within itself may be a good reason to do a small GEP update _only_ doing that so we can work through it and discuss it.@tssurya: /cc@k8s-triage-robot: This issue has not been updated in over 1 year, and should be re-triaged.

You can:
- Confirm that this issue is still relevant with `/triage accepted` (org members only)
- Close this issue with `/close`

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted@k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@k8s-triage-robot: The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle rotten`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle rotten

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@mlavacca: This is still relevant.

/reopen
/assign@mlavacca: /remove-lifecycle rotten

Solution patch:
diff --git a/conformance/conformance.go b/conformance/conformance.go
index 12e58849..8ea5799e 100644
--- a/conformance/conformance.go
+++ b/conformance/conformance.go
@@ -72,13 +72,14 @@ func DefaultOptions(t *testing.T) suite.ConformanceOptions {
 	namespaceAnnotations := suite.ParseKeyValuePairs(*flags.NamespaceAnnotations)
 	conformanceProfiles := suite.ParseConformanceProfiles(*flags.ConformanceProfiles)
 
-	implementation := suite.ParseImplementation(
+	implementation, err := suite.ParseImplementation(
 		*flags.ImplementationOrganization,
 		*flags.ImplementationProject,
 		*flags.ImplementationURL,
 		*flags.ImplementationVersion,
 		*flags.ImplementationContact,
 	)
+	require.NoError(t, err, "error parsing implementation details")
 
 	return suite.ConformanceOptions{
 		AllowCRDsMismatch:          *flags.AllowCRDsMismatch,
@@ -92,7 +93,7 @@ func DefaultOptions(t *testing.T) suite.ConformanceOptions {
 		ExemptFeatures:             exemptFeatures,
 		ManifestFS:                 []fs.FS{&Manifests},
 		GatewayClassName:           *flags.GatewayClassName,
-		Implementation:             implementation,
+		Implementation:             *implementation,
 		Mode:                       *flags.Mode,
 		NamespaceAnnotations:       namespaceAnnotations,
 		NamespaceLabels:            namespaceLabels,
diff --git a/conformance/utils/suite/suite.go b/conformance/utils/suite/suite.go
index f1164fa2..2402821e 100644
--- a/conformance/utils/suite/suite.go
+++ b/conformance/utils/suite/suite.go
@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	neturl "net/url"
 	"slices"
 	"sort"
 	"strings"
@@ -558,14 +559,33 @@ func (suite *ConformanceTestSuite) Report() (*confv1.ConformanceReport, error) {
 
 // ParseImplementation parses implementation-specific flag arguments and
 // creates a *confv1a1.Implementation.
-func ParseImplementation(org, project, url, version, contact string) confv1.Implementation {
-	return confv1.Implementation{
+func ParseImplementation(org, project, url, version, contact string) (*confv1.Implementation, error) {
+	if org == "" {
+		return nil, errors.New("organization must be set")
+	}
+	if project == "" {
+		return nil, errors.New("project must be set")
+	}
+	if url == "" {
+		return nil, errors.New("url must be set")
+	}
+	if version == "" {
+		return nil, errors.New("version must be set")
+	}
+	if contact == "" {
+		return nil, errors.New("contact must be set")
+	}
+	if _, err := neturl.ParseRequestURI(url); err != nil {
+		return nil, errors.New("url is malformed")
+	}
+
+	return &confv1.Implementation{
 		Organization: org,
 		Project:      project,
 		URL:          url,
 		Version:      version,
 		Contact:      strings.Split(contact, ","),
-	}
+	}, nil
 }
 
 // ParseConformanceProfiles parses flag arguments and converts the string to



Test patch:
diff --git a/conformance/utils/suite/suite_test.go b/conformance/utils/suite/suite_test.go
index ccb259ad..69b7d3d9 100644
--- a/conformance/utils/suite/suite_test.go
+++ b/conformance/utils/suite/suite_test.go
@@ -538,3 +538,89 @@ func namesToFeatureSet(names []string) FeaturesSet {
 	}
 	return featureSet
 }
+
+func TestParseImplementation(t *testing.T) {
+	testCases := []struct {
+		name        string
+		org         string
+		project     string
+		url         string
+		version     string
+		contact     string
+		expected    *confv1.Implementation
+		expectedErr error
+	}{
+		{
+			name:        "missing organization",
+			project:     "test-project",
+			url:         "https://example.com",
+			version:     "v1.0.0",
+			contact:     "test@example.com",
+			expectedErr: errors.New("organization must be set"),
+		},
+		{
+			name:        "missing project",
+			org:         "test-org",
+			url:         "https://example.com",
+			version:     "v1.0.0",
+			contact:     "test@example.com",
+			expectedErr: errors.New("project must be set"),
+		},
+		{
+			name:        "missing url",
+			org:         "test-org",
+			project:     "test-project",
+			version:     "v1.0.0",
+			contact:     "test@example.com",
+			expectedErr: errors.New("url must be set"),
+		},
+		{
+			name:        "missing version",
+			org:         "test-org",
+			project:     "test-project",
+			url:         "https://example.com",
+			contact:     "test@example.com",
+			expectedErr: errors.New("version must be set"),
+		},
+		{
+			name:        "missing contact",
+			org:         "test-org",
+			project:     "test-project",
+			url:         "https://example.com",
+			version:     "v1.0.0",
+			expectedErr: errors.New("contact must be set"),
+		},
+		{
+			name:        "malformed url",
+			org:         "test-org",
+			project:     "test-project",
+			url:         "invalid-url",
+			version:     "v1.0.0",
+			contact:     "test@example.com",
+			expectedErr: errors.New("url is malformed"),
+		},
+		{
+			name:    "valid input",
+			org:     "test-org",
+			project: "test-project",
+			url:     "https://example.com",
+			version: "v1.0.0",
+			contact: "test@example.com,test2@example.com",
+			expected: &confv1.Implementation{
+				Organization: "test-org",
+				Project:      "test-project",
+				URL:          "https://example.com",
+				Version:      "v1.0.0",
+				Contact:      []string{"test@example.com", "test2@example.com"},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := ParseImplementation(tc.org, tc.project, tc.url, tc.version, tc.contact)
+			assert.Equal(t, tc.expected, result)
+			assert.Equal(t, tc.expectedErr, err)
+		})
+	}
+}
diff --git a/pkg/test/cel/grpcroute_test.go b/pkg/test/cel/grpcroute_test.go
index 628de66a..2940ce32 100644
--- a/pkg/test/cel/grpcroute_test.go
+++ b/pkg/test/cel/grpcroute_test.go
@@ -341,7 +341,8 @@ func TestGRPCRouteRule(t *testing.T) {
 				}
 				return rules
 			}(),
-		}}
+		},
+	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {