Skip to content

Commit f0ee51b

Browse files
committed
Cleanup deadcode in preperation for k8s gogo changes
See kubernetes/enhancements#5590 (comment); we use a lot of very questionable gogoproto on k8s type operations. The good news is, as far as I can every single one of these is on code paths that never actually touch k8s types (anymore), or are on entirely dead code paths. My plan is to split this into two PRs to make things simpler/safer. This PR removes strictly dead code that is in this area. There is a surprising amount here! I think this is as we moved more and more stuff away from these legacy `config.Config` paths towards kclient/krt the cruft just stayed around. In the next PRs I will remove the gogo proto handling (TBD how, if we want to fail or fallback). I've analyzed the remaining usages: ``` DeepCopy UpdateHealth, DeleteHealthCondition (WE only) mergeVirtualServicesIfNeeded (on VS spec) mergeDestinationRule mergeHTTPRoutes (its a VirtualService though, not a HTTPRoute) resolveVirtualServiceShortnames (on VS only) convertToWasmPluginWrapper (On WasmPlugin) In a bunch of tests ApplyJSON FromJSON ConvertObject webhook.validate (only called for istio types) ParseInputs (should be called for everything) kube/file.NewController Only allows Istio objects but I think its going to read all of them, so we couldn't panic or something? ApplyJSONStrict fromSchemaAndJSONMap validateResource (in istioctl, only used on Istio types) Equals() We already explicitly do not use gogoproto.Equals EXCEPT in `needsPush` which only operates on istio.io types ToProto(): PilotConfigToResource (I think this will only have Istio types) ToJSON(): ToRaw() ConvertConfig used in configz handler, which should only have Istio types ToPrettyJSON(): Not even used on protos, bug in xds/debug.go ```
1 parent 70491be commit f0ee51b

File tree

24 files changed

+12
-432
lines changed

24 files changed

+12
-432
lines changed

pilot/pkg/autoregistration/internal/state/store.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ func (s *Store) UpdateHealth(proxyID, entryName, entryNs string, condition *v1al
7676
}
7777

7878
// replace the updated status
79-
wle := status.UpdateConfigCondition(*cfg, condition)
79+
wle := status.UpdateIstioConfigCondition(*cfg, condition)
8080
// update the status
8181
_, err := s.store.UpdateStatus(wle)
8282
if err != nil {
@@ -89,7 +89,7 @@ func (s *Store) UpdateHealth(proxyID, entryName, entryNs string, condition *v1al
8989
// DeleteHealthCondition updates WorkloadEntry of a workload that is not using auto-registration
9090
// to remove information about the health status (since we can no longer be certain about it).
9191
func (s *Store) DeleteHealthCondition(wle config.Config) error {
92-
wle = status.DeleteConfigCondition(wle, status.ConditionHealthy)
92+
wle = status.DeleteIstioConfigCondition(wle, status.ConditionHealthy)
9393
// update the status
9494
_, err := s.store.UpdateStatus(wle)
9595
if err != nil && !errors.IsNotFound(err) {

pilot/pkg/config/aggregate/config.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -166,13 +166,6 @@ func (cr *store) UpdateStatus(c config.Config) (string, error) {
166166
return cr.writer.UpdateStatus(c)
167167
}
168168

169-
func (cr *store) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
170-
if cr.writer == nil {
171-
return "", errorUnsupported
172-
}
173-
return cr.writer.Patch(orig, patchFn)
174-
}
175-
176169
type storeCache struct {
177170
model.ConfigStore
178171
caches []model.ConfigStoreController

pilot/pkg/config/file/store.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -101,10 +101,6 @@ func (s *KubeSource) UpdateStatus(config config.Config) (newRevision string, err
101101
return s.inner.UpdateStatus(config)
102102
}
103103

104-
func (s *KubeSource) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
105-
return s.inner.Patch(orig, patchFn)
106-
}
107-
108104
func (s *KubeSource) Delete(typ config.GroupVersionKind, name, namespace string, resourceVersion *string) error {
109105
return s.inner.Delete(typ, name, namespace, resourceVersion)
110106
}

pilot/pkg/config/kube/crd/conversion.go

Lines changed: 3 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,13 @@ import (
2121
"io"
2222
"reflect"
2323

24-
"github.com/hashicorp/go-multierror"
25-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26-
"k8s.io/apimachinery/pkg/types"
27-
kubeyaml "k8s.io/apimachinery/pkg/util/yaml"
28-
"sigs.k8s.io/yaml"
29-
3024
"istio.io/istio/pkg/config"
3125
"istio.io/istio/pkg/config/schema/collections"
3226
"istio.io/istio/pkg/config/schema/resource"
3327
"istio.io/istio/pkg/log"
28+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29+
"k8s.io/apimachinery/pkg/types"
30+
kubeyaml "k8s.io/apimachinery/pkg/util/yaml"
3431
)
3532

3633
// FromJSON converts a canonical JSON to a proto message
@@ -45,17 +42,6 @@ func FromJSON(s resource.Schema, js string) (config.Spec, error) {
4542
return c, nil
4643
}
4744

48-
func FromJSONStrict(s resource.Schema, js string) (config.Spec, error) {
49-
c, err := s.NewInstance()
50-
if err != nil {
51-
return nil, err
52-
}
53-
if err = config.ApplyJSONStrict(c, js); err != nil {
54-
return nil, err
55-
}
56-
return c, nil
57-
}
58-
5945
func StatusJSONFromMap(schema resource.Schema, jsonMap *json.RawMessage) (config.Status, error) {
6046
if jsonMap == nil {
6147
return nil, nil
@@ -75,33 +61,6 @@ func StatusJSONFromMap(schema resource.Schema, jsonMap *json.RawMessage) (config
7561
return status, nil
7662
}
7763

78-
// FromYAML converts a canonical YAML to a proto message
79-
func FromYAML(s resource.Schema, yml string) (config.Spec, error) {
80-
c, err := s.NewInstance()
81-
if err != nil {
82-
return nil, err
83-
}
84-
if err = config.ApplyYAML(c, yml); err != nil {
85-
return nil, err
86-
}
87-
return c, nil
88-
}
89-
90-
// FromJSONMap converts from a generic map to a proto message using canonical JSON encoding
91-
// JSON encoding is specified here: https://developers.google.com/protocol-buffers/docs/proto3#json
92-
func FromJSONMap(s resource.Schema, data any) (config.Spec, error) {
93-
// Marshal to YAML bytes
94-
str, err := yaml.Marshal(data)
95-
if err != nil {
96-
return nil, err
97-
}
98-
out, err := FromYAML(s, string(str))
99-
if err != nil {
100-
return nil, multierror.Prefix(err, fmt.Sprintf("YAML decoding error: %v", string(str)))
101-
}
102-
return out, nil
103-
}
104-
10564
type ConversionFunc = func(s resource.Schema, js string) (config.Spec, error)
10665

10766
func ConvertObjectInternal(schema resource.Schema, object IstioObject, domain string, convert ConversionFunc) (*config.Config, error) {

pilot/pkg/config/kube/crdclient/client.go

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -272,18 +272,6 @@ func (cl *Client) UpdateStatus(cfg config.Config) (string, error) {
272272
return meta.GetResourceVersion(), nil
273273
}
274274

275-
// Patch applies only the modifications made in the PatchFunc rather than doing a full replace. Useful to avoid
276-
// read-modify-write conflicts when there are many concurrent-writers to the same resource.
277-
func (cl *Client) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
278-
modified, patchType := patchFn(orig.DeepCopy())
279-
280-
meta, err := patch(cl.client, orig, getObjectMetadata(orig), modified, getObjectMetadata(modified), patchType)
281-
if err != nil {
282-
return "", err
283-
}
284-
return meta.GetResourceVersion(), nil
285-
}
286-
287275
// Delete implements store interface
288276
// `resourceVersion` must be matched before deletion is carried out. If not possible, a 409 Conflict status will be
289277
func (cl *Client) Delete(typ config.GroupVersionKind, name, namespace string, resourceVersion *string) error {

pilot/pkg/config/kube/crdclient/client_test.go

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323

2424
"go.uber.org/atomic"
2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26-
"k8s.io/apimachinery/pkg/types"
2726
"sigs.k8s.io/gateway-api/pkg/consts"
2827

2928
"istio.io/api/meta/v1alpha1"
@@ -239,24 +238,6 @@ func TestClient(t *testing.T) {
239238
return nil
240239
})
241240

242-
// check we can patch items
243-
var patchedCfg config.Config
244-
if _, err := store.(*Client).Patch(*cfg, func(cfg config.Config) (config.Config, types.PatchType) {
245-
cfg.Annotations["fizz"] = "buzz"
246-
patchedCfg = cfg
247-
return cfg, types.JSONPatchType
248-
}); err != nil {
249-
t.Errorf("unexpected err in Patch: %v", err)
250-
}
251-
// validate it is updated
252-
retry.UntilSuccessOrFail(t, func() error {
253-
cfg := store.Get(r.GroupVersionKind(), configName, configMeta.Namespace)
254-
if cfg == nil || !reflect.DeepEqual(cfg.Meta, patchedCfg.Meta) {
255-
return fmt.Errorf("get(%v) => got unexpected object %v", name, cfg)
256-
}
257-
return nil
258-
})
259-
260241
// Check we can remove items
261242
if err := store.Delete(r.GroupVersionKind(), configName, configNamespace, nil); err != nil {
262243
t.Fatalf("failed to delete: %v", err)

pilot/pkg/config/kube/file/controller.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -144,10 +144,6 @@ func (c *Controller) UpdateStatus(config config.Config) (newRevision string, err
144144
return "", errUnsupportedOp
145145
}
146146

147-
func (c *Controller) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
148-
return "", errUnsupportedOp
149-
}
150-
151147
func (c *Controller) Delete(typ config.GroupVersionKind, name, namespace string, _ *string) error {
152148
return errUnsupportedOp
153149
}

pilot/pkg/config/kube/gateway/controller.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -541,10 +541,6 @@ func (c *Controller) UpdateStatus(config config.Config) (newRevision string, err
541541
return "", errUnsupportedOp
542542
}
543543

544-
func (c *Controller) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
545-
return "", errUnsupportedOp
546-
}
547-
548544
func (c *Controller) Delete(typ config.GroupVersionKind, name, namespace string, _ *string) error {
549545
return errUnsupportedOp
550546
}

pilot/pkg/config/memory/controller.go

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@ package memory
1717
import (
1818
"fmt"
1919

20-
"k8s.io/apimachinery/pkg/types"
21-
2220
"istio.io/istio/pilot/pkg/model"
2321
"istio.io/istio/pkg/config"
2422
"istio.io/istio/pkg/config/schema/collection"
@@ -123,24 +121,6 @@ func (c *Controller) UpdateStatus(config config.Config) (newRevision string, err
123121
return newRevision, err
124122
}
125123

126-
func (c *Controller) Patch(orig config.Config, patchFn config.PatchFunc) (newRevision string, err error) {
127-
cfg, typ := patchFn(orig.DeepCopy())
128-
switch typ {
129-
case types.MergePatchType:
130-
case types.JSONPatchType:
131-
default:
132-
return "", fmt.Errorf("unsupported merge type: %s", typ)
133-
}
134-
if newRevision, err = c.configStore.Patch(cfg, patchFn); err == nil {
135-
c.monitor.ScheduleProcessEvent(ConfigEvent{
136-
old: orig,
137-
config: cfg,
138-
event: model.EventUpdate,
139-
})
140-
}
141-
return newRevision, err
142-
}
143-
144124
func (c *Controller) Delete(kind config.GroupVersionKind, key, namespace string, resourceVersion *string) error {
145125
if config := c.Get(kind, key, namespace); config != nil {
146126
if err := c.configStore.Delete(kind, key, namespace, resourceVersion); err != nil {

pilot/pkg/config/memory/store.go

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -227,39 +227,6 @@ func (cr *store) UpdateStatus(cfg config.Config) (string, error) {
227227
return cr.Update(cfg)
228228
}
229229

230-
func (cr *store) Patch(orig config.Config, patchFn config.PatchFunc) (string, error) {
231-
cr.mutex.Lock()
232-
defer cr.mutex.Unlock()
233-
234-
gvk := orig.GroupVersionKind
235-
s, ok := cr.schemas.FindByGroupVersionKind(gvk)
236-
if !ok {
237-
return "", fmt.Errorf("unknown type %v", gvk)
238-
}
239-
240-
cfg, _ := patchFn(orig)
241-
if !cr.skipValidation {
242-
if _, err := s.ValidateConfig(cfg); err != nil {
243-
return "", err
244-
}
245-
}
246-
247-
_, ok = cr.data[gvk]
248-
if !ok {
249-
return "", errNotFound
250-
}
251-
ns, exists := cr.data[gvk][orig.Namespace]
252-
if !exists {
253-
return "", errNotFound
254-
}
255-
256-
rev := time.Now().String()
257-
cfg.ResourceVersion = rev
258-
ns[cfg.Name] = cfg
259-
260-
return rev, nil
261-
}
262-
263230
// hasConflict checks if the two resources have a conflict, which will block Update calls
264231
func hasConflict(existing, replacement config.Config) bool {
265232
if replacement.ResourceVersion == "" {

0 commit comments

Comments
 (0)