conformance: allow reading BaseManifests from non-embedded local filesystem

[mikemorris]

What would you like to be added:

Update getContentsFromPathOrURL to support reading files from the local filesystem rather than only the embedded filesystem.

Why this is needed:
We're currently working on switching our implementation of Gateway API conformance tests from a local git checkout to re-implementing conformance_test.go directly (in order to specify SupportedFeatures, which IIUC is a recommended pattern), but we're currently blocked due to our need to make some slight tweaks to the base manifests (lowering the number of replicas to work around resource constraints in CI and adding an annotation to all Deployments), which we have previously been doing with a kustomize patch.

Specifying a string path for BaseManifests is currently supported as an option when initializing a ConformanceTestSuite struct, but unless the string is a remote path beginning with https://, it is hardcoded to only read from the read-only embedded filesystem.

I've managed to read the base manifests from the embedded filesystem, patch them with krusty and write them to a local temp file with kyaml/filesys, but would need to add support for reading from the local filesystem to be able to use the patched manifests in this way - is this something we could consider supporting, or is there a different way I should be approaching this problem?

[youngnick]

I can't see a reason not to support the local filesystem, subject to the usual input-validation checks to prevent directory traversal.

[mikemorris]

Any thoughts on implementation UX? The most lightweight explicit config I can think of would be something like introducing another string prefix check (either local:// or embed://, although an embedded-specific prefix would require updating all the conformance test Manifests config paths, e.g.

gateway-api/conformance/tests/httproute-cross-namespace.go

Line 36 in 20683af

Manifests: []string{"tests/httproute-cross-namespace.yaml"},

)

Adding an additional arg of some sort would be a bit more problematic, as this function isn't called directly by an implementer, just as a util to parse a config string, so may require something like switching the plain string config to a struct, typed strings with generics or an interface and reflection.

Is there an example somewhere of the input-validation checks you're recommending @youngnick, and would those potentially prevent reading from /tmp paths?

[youngnick]

I can't find any good examples in a quick scan, but even just doing the obvious stuff is an improvement (no more than one consecutive dot allowed in paths and setting a maximum path length to prevent overflows).

Total path length varies depending on OS, but 4096 chars is reasonably common, and probably a good default. Filename length is 255 chars on most Unix-y OSes, so that will do there too.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs 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 or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages 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:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

[k8s-ci-robot]

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages 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:

• Reopen this issue with /reopen
• Mark this issue as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close not-planned

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/test-infra repository.