add unmanged crd detection to remove duplicated openapi path #348
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| package crdcoexistence | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
|
|
||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| apiextensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/client-go/rest" | ||
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| var conflictGroups = map[string]bool{ | ||
|
Member
There was a problem hiding this comment. a map of string to empty struct is better here. an empty struct is 0 bytes. |
||
| "wgpolicyk8s.io": true, | ||
| "openreports.io": true, | ||
|
Member
There was a problem hiding this comment. why do we only have these and not the other crds the reports server serves ? |
||
| } | ||
|
|
||
| // DetectConflictingCRDs checks the cluster for CRDs in API groups that | ||
| // reports-server also serves via APIService. When both a CRD and an APIService | ||
| // exist for the same group/version, the kube-apiserver's OpenAPI aggregator | ||
| // produces duplicate-path errors (K8s 1.28+). For each conflicting group found, | ||
| // the corresponding OpenAPI path prefix is returned so it can be added to | ||
| // IgnorePrefixes — suppressing reports-server's OpenAPI paths for that group | ||
| // while keeping the APIService request routing fully functional. | ||
| func DetectConflictingCRDs(restConfig *rest.Config) []string { | ||
|
Member
There was a problem hiding this comment. its better if this function returns an error and the error is ignored at the caller level. Also it doesn't have to be public |
||
| if restConfig == nil { | ||
| return nil | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) | ||
|
Member
There was a problem hiding this comment. why not pass the context given to |
||
| defer cancel() | ||
|
|
||
| client, err := apiextensionsclient.NewForConfig(restConfig) | ||
| if err != nil { | ||
| klog.V(2).InfoS("unable to create apiextensions client for CRD conflict detection, skipping", "error", err) | ||
| return nil | ||
| } | ||
|
|
||
| crdList, err := client.ApiextensionsV1().CustomResourceDefinitions().List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| klog.V(2).InfoS("unable to list CRDs for conflict detection, skipping", "error", err) | ||
| return nil | ||
| } | ||
|
|
||
| return detectFromCRDList(crdList.Items) | ||
| } | ||
|
|
||
| func detectFromCRDList(crds []apiextensionsv1.CustomResourceDefinition) []string { | ||
|
Member
There was a problem hiding this comment. i think its best to drop this helper entirely and move its logic to |
||
| seen := make(map[string]bool) | ||
| var prefixes []string | ||
|
|
||
| for _, crd := range crds { | ||
| group := crd.Spec.Group | ||
| if !conflictGroups[group] || seen[group] { | ||
|
Member
There was a problem hiding this comment. can you make this a variable that gets supplied to the function rather than a global ? it's fine if the caller references the global. but its ok for the caller to pass the global variable. it makes it easier to guess about the function's intent and what it does. |
||
| continue | ||
| } | ||
| seen[group] = true | ||
| klog.InfoS("detected third-party CRD for API group served by reports-server, suppressing OpenAPI paths to avoid duplicate-path errors", | ||
| "group", group) | ||
| prefixes = append(prefixes, "/apis/"+group+"/") | ||
| } | ||
| return prefixes | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package crdcoexistence | ||
|
|
||
| import ( | ||
| "sort" | ||
| "testing" | ||
|
|
||
| apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestDetectConflictingCRDs_NilConfig(t *testing.T) { | ||
| if result := DetectConflictingCRDs(nil); result != nil { | ||
| t.Errorf("expected nil, got %v", result) | ||
| } | ||
| } | ||
|
|
||
| func TestDetectFromCRDList(t *testing.T) { | ||
| makeCRD := func(name, group string) apiextensionsv1.CustomResourceDefinition { | ||
| return apiextensionsv1.CustomResourceDefinition{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: name}, | ||
| Spec: apiextensionsv1.CustomResourceDefinitionSpec{Group: group}, | ||
| } | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| crds []apiextensionsv1.CustomResourceDefinition | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "no CRDs", | ||
| }, | ||
| { | ||
| name: "unrelated CRD", | ||
| crds: []apiextensionsv1.CustomResourceDefinition{makeCRD("foos.example.com", "example.com")}, | ||
| }, | ||
| { | ||
| name: "wgpolicyk8s CRD", | ||
| crds: []apiextensionsv1.CustomResourceDefinition{makeCRD("policyreports.wgpolicyk8s.io", "wgpolicyk8s.io")}, | ||
| expected: []string{"/apis/wgpolicyk8s.io/"}, | ||
| }, | ||
| { | ||
| name: "openreports CRD", | ||
| crds: []apiextensionsv1.CustomResourceDefinition{makeCRD("reports.openreports.io", "openreports.io")}, | ||
| expected: []string{"/apis/openreports.io/"}, | ||
| }, | ||
| { | ||
| name: "both groups", | ||
| crds: []apiextensionsv1.CustomResourceDefinition{ | ||
| makeCRD("policyreports.wgpolicyk8s.io", "wgpolicyk8s.io"), | ||
| makeCRD("reports.openreports.io", "openreports.io"), | ||
| }, | ||
| expected: []string{"/apis/openreports.io/", "/apis/wgpolicyk8s.io/"}, | ||
| }, | ||
| { | ||
| name: "duplicate CRDs in same group", | ||
| crds: []apiextensionsv1.CustomResourceDefinition{ | ||
| makeCRD("policyreports.wgpolicyk8s.io", "wgpolicyk8s.io"), | ||
| makeCRD("clusterpolicyreports.wgpolicyk8s.io", "wgpolicyk8s.io"), | ||
| }, | ||
| expected: []string{"/apis/wgpolicyk8s.io/"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := detectFromCRDList(tt.crds) | ||
| sort.Strings(result) | ||
| sort.Strings(tt.expected) | ||
|
|
||
| if len(result) != len(tt.expected) { | ||
| t.Errorf("expected %v, got %v", tt.expected, result) | ||
| return | ||
| } | ||
| for i := range result { | ||
| if result[i] != tt.expected[i] { | ||
| t.Errorf("expected %v, got %v", tt.expected, result) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package crdcoexistence | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
|
|
||
| "k8s.io/client-go/rest" | ||
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| // PeriodicallyCheckForNewConflicts re-runs CRD conflict detection on a timer. | ||
| // If a new conflicting CRD appears that was not present at startup, the cancel | ||
| // function is called to trigger a graceful restart. | ||
| func PeriodicallyCheckForNewConflicts(ctx context.Context, cancel context.CancelFunc, restConfig *rest.Config, initialPrefixes []string) { | ||
| known := make(map[string]bool, len(initialPrefixes)) | ||
|
|
||
| for _, p := range initialPrefixes { | ||
| known[p] = true | ||
| } | ||
|
|
||
| ticker := time.NewTicker(2 * time.Minute) | ||
| defer ticker.Stop() | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| current := DetectConflictingCRDs(restConfig) | ||
| for _, p := range current { | ||
| if !known[p] { | ||
|
Member
There was a problem hiding this comment. here, you are checking if we detected new conflicting CRDs other than the ones in |
||
| klog.InfoS("new conflicting CRD detected since startup, restarting to apply OpenAPI suppression", "prefix", p) | ||
| cancel() | ||
| return | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things i wanna comment on in this bit:
First, if the
PeriodicallyCheckForNewConflictsdetect a conflict, it will cancel the context. andRunUntilwill exit because the done channel became readable. This means that cleanups we are running on events received through thestopChwon't run.Second, I think its added complexity to create a context that you cancel when you detect a conflict, run a goroutine that checks if
stopChbecame readable (something else triggered a server shutdown signal) and cancel the periodic check goroutine.Why not just pass the
stopChas an argument to the function, run a for select on it becoming readable (something else cancelled) and when a conflict is detected you close this channel yourself ?Lastly, i am a bit against making this whole thing periodic in the first place. I prefer if its only once on startup. If the admin installed a conflicting CRD, it should be on them to restart the server (to make sure they are doing it in line with keeping their services available).
We can help them out by logging a message that nudges them to do so. Something like
there's a CRD installed that conflicts with the CRDs the reports server serves, please ensure to restart the server once after CRD installation to avoid serving duplicate prefixes. Or something less verbose?