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

Add support for end-users to create their own conformance profiles #2864

Merged
merged 5 commits into from
Apr 2, 2024

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Mar 12, 2024

What type of PR is this?

/kind test
/kind cleanup
/area conformance

What this PR does / why we need it:

Advanced end users would like to create their own conformance profiles out of tree. This makes it easier to validate an implementation supports a certain set of features without relying on manually reading conformance reports on the gateway API website.

With this change writing your own profile to run is as simple as creating a single _test.go file

package gatewayapi

import (
	"testing"

	"sigs.k8s.io/gateway-api/conformance"
	"sigs.k8s.io/gateway-api/conformance/utils/suite"

	"k8s.io/apimachinery/pkg/util/sets"
)

// KnativeConformanceProfile is a ConformanceProfile that covers testing Gateway API features
// that Knative require
var (
	KnativeConformanceProfile = suite.ConformanceProfile{
		Name: "KNATIVE",
		CoreFeatures: sets.New(
			// Core HTTPRoute Features
			suite.SupportGateway,
			suite.SupportReferenceGrant,
			suite.SupportHTTPRoute,

			// HTTPRoute Extended Features
			suite.SupportHTTPRouteHostRewrite,
			// suite.SupportHTTPRouteBackendRequestHeaderModification, // https://github.com/kubernetes-sigs/gateway-api/pull/2820

			// HTTPRoute Experimental Features
			suite.SupportHTTPRouteBackendProtocolH2C,
			suite.SupportHTTPRouteBackendProtocolWebSocket,

			// HTTPRoute - Optional Features Knative might care about
			suite.SupportHTTPRouteRequestTimeout,
			suite.SupportHTTPRouteSchemeRedirect,

			// suite.SupportGatewayInfrastructureMetadata, // https://github.com/kubernetes-sigs/gateway-api/pull/2845
		),
	}
)

func TestConformance(t *testing.T) {
	suite.RegisterConformanceProfile(KnativeConformanceProfile)
	conformance.RunConformance(t, nil)
}

Which issue(s) this PR fixes:

N/A

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/test kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2024
@dprotaso
Copy link
Contributor Author

Note I also refactored a bunch so the tests can be run downstream a bit more easily without a whole lot of boilerplate

/assign @shaneutt @mlavacca

@dprotaso
Copy link
Contributor Author

Also if we don't like this direction then I think it would be good to keep some of the refactor then it would be possible for downstream to run the test as follows

func TestConformance(t *testing.T) {
	opts := conformance.DefaultOptions()
        opts.SupportedFeatures.Union(.... /* features I care about */)
	conformance.RunConformance(t, opts)
}

@shaneutt
Copy link
Member

/cc @youngnick @robscott @sunjayBhatia

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for adding this. A few comments, all minor.

conformance/conformance.go Outdated Show resolved Hide resolved
conformance/conformance.go Outdated Show resolved Hide resolved
conformance/conformance.go Outdated Show resolved Hide resolved
conformance/conformance.go Outdated Show resolved Hide resolved
conformance/conformance.go Show resolved Hide resolved
conformance/legacy.go Outdated Show resolved Hide resolved
conformance/utils/flags/experimental_flags.go Outdated Show resolved Hide resolved
conformance/utils/suite/experimental_profiles.go Outdated Show resolved Hide resolved
@dprotaso
Copy link
Contributor Author

Few of the comments mentioned dropping the legacy support in this PR. I suggest we do this in a follow up PR. It'll be easier to understand the changes that took place.

Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/approve
/cc @youngnick @robscott @mlavacca for lgtm

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2024
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

I've synced on Slack with @dprotaso, and we agreed to wait for the conformance suite promotion (#2851) to land before merging this one.

@shaneutt shaneutt requested a review from mlavacca March 12, 2024 18:56
@mlavacca
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2024
@mlavacca
Copy link
Member

/unhold

@dprotaso #2851 has been merged, hence we can move this one forward

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 29, 2024
@dprotaso
Copy link
Contributor Author

@mlavacca take a look now

This lets downstreams to have their own tests with embedded yaml files
conformance/conformance.go Show resolved Hide resolved
conformance/utils/suite/profiles.go Show resolved Hide resolved
conformance/utils/suite/suite.go Show resolved Hide resolved
Copy link
Member

@mlavacca mlavacca left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, mlavacca, shaneutt

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

@k8s-ci-robot k8s-ci-robot merged commit b150af2 into kubernetes-sigs:main Apr 2, 2024
8 checks passed
@@ -199,10 +199,6 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,
mode = options.Mode
}

if options.FS == nil {
options.FS = &conformance.Manifests
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't know why you removed this part, but it's a breaking change for downstream projects.

If the downstream project uses gwapi's conformance test as a library, it will cause panic after this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@dprotaso I know, but if downstream project not use DefaultOptions, it will cause panic. like this one https://github.com/Kong/kubernetes-ingress-controller/actions/runs/8661628250/job/23752106390?pr=5863#step:5:290

--- FAIL: TestGatewayConformance (10.89s)
    --- FAIL: TestGatewayConformance/GatewayInvalidRouteKind (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x12be5d0]

goroutine 1479 [running]:
testing.tRunner.func1.2({0x452c460, 0x64d1600})
	/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1545 +0x3f7
testing.tRunner.func1()
	/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1548 +0x716
panic({0x452c460?, 0x64d1600?})
	/opt/hostedtoolcache/go/1.21.6/x64/src/runtime/panic.go:920 +0x270
bytes.(*Buffer).Read(0x0, {0xc002d5a000, 0x1000, 0x7538b00?})
	/opt/hostedtoolcache/go/1.21.6/x64/src/bytes/buffer.go:319 +0x30
bufio.(*Reader).fill(0xc001565ec0)
	/opt/hostedtoolcache/go/1.21.6/x64/src/bufio/bufio.go:113 +0x29a
bufio.(*Reader).Peek(0xc001565ec0, 0x1000)
	/opt/hostedtoolcache/go/1.21.6/x64/src/bufio/bufio.go:151 +0xc7
k8s.io/apimachinery/pkg/util/yaml.GuessJSONStream({0x50173a0?, 0x0}, 0x1000)
	/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/yaml/decoder.go:374 +0x1cb
k8s.io/apimachinery/pkg/util/yaml.(*YAMLOrJSONDecoder).Decode(0xc002d63b10, {0x48a9ca0, 0xc002ba6cc0})
	/home/runner/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/yaml/decoder.go:278 +0xae
sigs.k8s.io/gateway-api/conformance/utils/kubernetes.Applier.prepareResources({0x0, 0x0, {0xc0014b6570, 0x24}, {0xc000bb5080, 0x21}, {0x0, 0x0, 0x0}, {0x0, ...}, ...}, ...)
	/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/kubernetes/apply.go:190 +0xc5
sigs.k8s.io/gateway-api/conformance/utils/kubernetes.Applier.MustApplyWithCleanup({0x0, 0x0, {0xc0014b6570, 0x24}, {0xc000bb5080, 0x21}, {0x0, 0x0, 0x0}, {0x0, ...}, ...}, ...)
	/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/kubernetes/apply.go:254 +0x2a8
sigs.k8s.io/gateway-api/conformance/utils/suite.(*ConformanceTest).Run(0xc001565a40, 0xc001c4fd40, 0xc000eae000)
	/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/suite/conformance.go:64 +0x706
sigs.k8s.io/gateway-api/conformance/utils/suite.(*ConformanceTestSuite).Run.func1(0x0?)
	/home/runner/go/pkg/mod/sigs.k8s.io/gateway-api@v1.0.1-0.20240409212030-c57c131b49f8/conformance/utils/suite/suite.go:390 +0x45
testing.tRunner(0xc001c4fd40, 0xc001404ee8)
	/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1595 +0x262
created by testing.(*T).Run in goroutine 372
	/opt/hostedtoolcache/go/1.21.6/x64/src/testing/testing.go:1648 +0x846
FAIL	github.com/kong/kubernetes-ingress-controller/v3/test/conformance	86.039s

This is the configuration it uses.

Kong/kubernetes-ingress-controller@150b1d4#diff-cd95a80b6232fe201246d9c47d7595ff74f315ba2ccd0f54eb0da46d0b9839c7

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @tao12345666333 on this one. We cannot expect that implementation always use the DefaultOptions method, as the suite is often used as a library. And in case FS is not set I think we should provide the default instead of expecting implementations to always set the default.

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. area/conformance cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/test lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.

None yet

5 participants