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

REQUEST: Create github.com/kubernetes-sigs/randfill #5414

Open
thockin opened this issue Feb 19, 2025 · 18 comments
Open

REQUEST: Create github.com/kubernetes-sigs/randfill #5414

thockin opened this issue Feb 19, 2025 · 18 comments
Labels
area/github-repo Creating, migrating or deleting a Kubernetes GitHub Repository

Comments

@thockin
Copy link
Member

thockin commented Feb 19, 2025

New repo, staging repo, or migrate existing

fork from github.com/google/gofuzz

Is it a staging repo?

no

Requested name for new repository

randfill

Which Organization should it reside

kubernetes-sigs

Who should have admin access?

No response

Who should have write access?

No response

Who should be listed as approvers in OWNERS?

sig-testing-approvers, thockin

Who should be listed in SECURITY_CONTACTS?

sig-testing-leads, thockin

What should the repo description be?

Utility functions for filling types with randomized data (for kubernetes testing use).

What SIG and subproject does this fall under?

sig-testing

Please provide references to appropriate approval for this new repo

Discussed in code-organization slack: https://kubernetes.slack.com/archives/CHGFYJVAN/p1739924804148699

Additional context for request

The original repo is archived. Some of the code (but not all) was written by Kubernetes people in the first place. It's unclear if there are special considerations in this case. I read https://github.com/kubernetes/community/blob/master/github-management/kubernetes-repositories.md#rules-for-donated-repositories which says things like "projects that a SIG adopts may also be donated" - this is not a "donation" but a fork of an abandoned project.

It's already Apache2 licensed, so we should be allowed?

/cc @dims @BenTheElder @pohly @liggitt

@thockin thockin added the area/github-repo Creating, migrating or deleting a Kubernetes GitHub Repository label Feb 19, 2025
@thockin
Copy link
Member Author

thockin commented Feb 19, 2025

FTR: the name "randfill" is my proposal, but am open to better names.

We could keep it named "gofuzz", but it's very confusing with the somewhat popular "go-fuzz" project and with Go's own fuzzing. Worse, the module name is "fuzz", which is different from the last directory of the actual import in code ("gofuzz").

@dims
Copy link
Member

dims commented Feb 19, 2025

It's already Apache2 licensed
fork of an abandoned project.

yes, we have done this before.

@dims
Copy link
Member

dims commented Feb 19, 2025

+1 from me.

@thockin
Copy link
Member Author

thockin commented Feb 19, 2025

What's the usual process WRT forking? Does someone create a new repo as empty and then I force-push into it? Or does someone create the repo as a fork of the last commit in the origin?

@dims
Copy link
Member

dims commented Feb 19, 2025

@thockin we can get populate a repo in a personal github org and work with github admins to move it to a specific github org (via transfer to an intermediary org)

@thockin
Copy link
Member Author

thockin commented Feb 19, 2025 via email

@pohly
Copy link
Contributor

pohly commented Feb 19, 2025

What SIG and subproject does this fall under?

sig-testing

LGTM (with my SIG Testing Tech Lead hat on). Might be good to get a thumbs up from @BenTheElder and @aojea. Ultimately we will have to maintain this, even if Tim does the initial work.

cc @michelle192837 (SIG Testing Chair)

@pohly
Copy link
Contributor

pohly commented Feb 19, 2025

FTR: the name "randfill" is my proposal, but am open to better names.

I keep reading it as "landfill"... 😅

I suggested "kfuzz" on Slack to carry on with the original theme, while making it clear that this is now a Kubernetes component maintained for Kubernetes.

Tim suggested that we should discourage external usage. We can't enforce that technically, only by naming and documentation.

What is the planned versioning? If it's really only a test dependency, then we don't need to tag (no risk of API conflicts between different dependencies when each dependency also imports the new module). But I think we have some non-test code importing it?

@aojea
Copy link
Member

aojea commented Feb 19, 2025

/lgtm

@thockin
Copy link
Member Author

thockin commented Feb 20, 2025

keep reading it as "landfill"...

That's the joke! You are filling your objects with garbage data.

I dislike the word "fuzz" for this because it's not REALLY fuzzing.

What is the planned versioning? If it's really only a test dependency, then we don't need to tag (no risk of API conflicts between different dependencies when each dependency also imports the new module). But I think we have some non-test code importing it?

I don't see any non-test code, and if there is some we should weed it out. This is not really appropriate for prod use - it panics at the drop of a hat. I was assuming no tagging required, or at most just a simple 0.X.Y until/unless we find a need to do a v2.

Prepping a repo, I got a little carried away and cleaned up some of the API and made it so the small number of places we use fuzz.Interface can implement self-fuzzing without take a link-time dep on this library (e.g. staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/time_fuzz.go)

@pohly
Copy link
Contributor

pohly commented Feb 20, 2025

That's the joke! You are filling your objects with garbage data.

I noticed that, yeah me! I just wasn't sure whether it was intentional. Definitely needs to go into the docs. 😁

I don't see any non-test code, and if there is some we should weed it out. This is not really appropriate for prod use - it panics at the drop of a hat.

pkg/apis/*/fuzzer is technically part of our API implementation and depends on go-fuzz. The methods might only get called in tests, but it's still a dependency of the kube-apiserver binary. Other components might have something similar.

@thockin
Copy link
Member Author

thockin commented Feb 20, 2025

Hmm, I will have to understand why it's linked. I don't think it should be.

I know metav1 takes a dep on gofuzz via Time, but I thought that was all (and I think we can break that dep)

@pohly
Copy link
Contributor

pohly commented Feb 20, 2025

Perhaps it's really not linked. But my point about API stability and non-test code still stands, I think.

@thockin
Copy link
Member Author

thockin commented Feb 20, 2025

Maybe we should define what "test code" is. In my mind that means code that is only used during tests. Ideally it's not linked into prod binaries, but it's not necessarily in a something_test.go file. A "testing" or "fuzzer" sub-pkg is almost certainly "test code'.

That said, I will look at those packages and make sure that my assertion is correct. I skipped over them because I assumed that was obviously test code. Maybe I'm wrong

@pohly
Copy link
Contributor

pohly commented Feb 20, 2025

One more thought on "separate repo" vs. "k8s.io/utils/randfill": if we go the separate repo route, we will have to set up test jobs. Perhaps "k8s.io/utils" is the simpler approach after all for code with no external dependencies?

I can prepare a commit which preserves the full history under that path before merging it and updating the content to fit the new location and API changes.

@dims
Copy link
Member

dims commented Feb 20, 2025

@pohly i'd prefer a standalone repo for this one. we can do some simple github action based test jobs for this repo

@thockin
Copy link
Member Author

thockin commented Feb 20, 2025

FWIW the PR to k/k is large but it's just renames. No semantic deltas and 99% was a sed script (didn't debug the last 4 or 5 escapes). Tests in all modified dirs pass.

I did not modify k/k types and funcs which use "fuzz" for this, so you'll see thigns like:

func fuzzInitConfiguration(obj *kubeadm.InitConfiguration, c fuzz.Continue) {
-       c.FuzzNoCustom(obj)
+func fuzzInitConfiguration(obj *kubeadm.InitConfiguration, c randfill.Continue) {
+       c.FillNoCustom(obj)

or

 
 // copied from staging/src/k8s.io/apiserver/pkg/endpoints/discovery/v2/handler_test.go
 func fuzzAPIGroups(atLeastNumGroups, maxNumGroups int, seed int64) apidiscoveryv2.APIGroupDiscoveryList {
-       fuzzer := fuzz.NewWithSeed(seed)
+       fuzzer := randfill.NewWithSeed(seed)
        fuzzer.NumElements(atLeastNumGroups, maxNumGroups)
        fuzzer.NilChance(0)
-       fuzzer.Funcs(func(o *apidiscoveryv2.APIGroupDiscovery, c fuzz.Continue) {
-               c.FuzzNoCustom(o)
+       fuzzer.Funcs(func(o *apidiscoveryv2.APIGroupDiscovery, c randfill.Continue) {
+               c.FillNoCustom(o)

I could do more, but I thought it was enough to show that this works end-to-end.

@thockin
Copy link
Member Author

thockin commented Feb 20, 2025

Also, I added non-compiling code to randfill and built kube-apiserver, kubelet, kube-proxy, kube-controller-manager, and kubectl without errors. Running tests exploded as planned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/github-repo Creating, migrating or deleting a Kubernetes GitHub Repository
Projects
None yet
Development

No branches or pull requests

4 participants