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.

Sign up for GitHub

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

Jump to bottom

Clean constants #50170

Closed
wants to merge 8 commits into from
Closed

Clean constants #50170

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bef1dd4
remove always push labels
hzxuzhonghu Apr 1, 2024
4866367
Remove deprecated gateway name support
hzxuzhonghu Apr 1, 2024
92a9480
update
hzxuzhonghu Apr 1, 2024
d5fb49d
add internal.istio.io/route-semantics=ingress annotation and also ski…
hzxuzhonghu Apr 10, 2024
49cc16c
restore ingress real change check
hzxuzhonghu Apr 11, 2024
6c380d4
Revert "remove always push labels"
hzxuzhonghu Apr 12, 2024
02fe586
update
hzxuzhonghu Apr 12, 2024
0807711
revert config compare
hzxuzhonghu Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions pilot/pkg/bootstrap/config_compare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,8 @@ import (
// to be triggered. This is to avoid unnecessary pushes only when labels have changed
// for example.
func needsPush(prev config.Config, curr config.Config) bool {
if prev.GroupVersionKind != curr.GroupVersionKind {
hzxuzhonghu marked this conversation as resolved.
Show resolved Hide resolved
// This should never happen.
return true
}
// If the config is not Istio, let us just push.
// Used for gateway api reconcile.
if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
return true
}
Expand Down
25 changes: 0 additions & 25 deletions pilot/pkg/bootstrap/config_compare_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (

networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pkg/config"
"istio.io/istio/pkg/config/constants"
"istio.io/istio/pkg/config/schema/gvk"
)

Expand All @@ -30,26 +29,6 @@ func TestNeedsPush(t *testing.T) {
curr config.Config
expected bool
}{
{
name: "different gvk",
prev: config.Config{
Meta: config.Meta{
GroupVersionKind: gvk.VirtualService,
Name: "acme2-v1",
Namespace: "not-default",
},
Spec: &networking.VirtualService{},
},
curr: config.Config{
Meta: config.Meta{
GroupVersionKind: gvk.DestinationRule,
Name: "acme2-v1",
Namespace: "not-default",
},
Spec: &networking.VirtualService{},
},
expected: true,
},
{
name: "same gvk label change",
prev: config.Config{
Expand Down Expand Up @@ -108,7 +87,6 @@ func TestNeedsPush(t *testing.T) {
GroupVersionKind: gvk.Ingress,
Name: "acme2-v1",
Namespace: "not-default",
Labels: map[string]string{constants.AlwaysPushLabel: "true"},
},
},
expected: true,
Expand All @@ -120,7 +98,6 @@ func TestNeedsPush(t *testing.T) {
GroupVersionKind: gvk.Ingress,
Name: "acme2-v1",
Namespace: "not-default",
Labels: map[string]string{constants.AlwaysPushLabel: "true"},
},
},
curr: config.Config{
Expand All @@ -146,7 +123,6 @@ func TestNeedsPush(t *testing.T) {
GroupVersionKind: gvk.Ingress,
Name: "acme2-v1",
Namespace: "not-default",
Annotations: map[string]string{constants.AlwaysPushLabel: "true"},
},
},
expected: true,
Expand All @@ -158,7 +134,6 @@ func TestNeedsPush(t *testing.T) {
GroupVersionKind: gvk.Ingress,
Name: "acme2-v1",
Namespace: "not-default",
Annotations: map[string]string{constants.AlwaysPushLabel: "true"},
},
},
curr: config.Config{
Expand Down
12 changes: 1 addition & 11 deletions pilot/pkg/config/kube/gateway/deploymentcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ func (d *DeploymentController) render(templateName string, mi TemplateInput) ([]
return nil, fmt.Errorf("no %q template defined", templateName)
}

labelToMatch := map[string]string{constants.GatewayNameLabel: mi.Name, constants.DeprecatedGatewayNameLabel: mi.Name}
labelToMatch := map[string]string{constants.GatewayNameLabel: mi.Name}
proxyConfig := d.env.GetProxyConfigOrDefault(mi.Namespace, labelToMatch, nil, cfg.MeshConfig)
input := derivedInput{
TemplateInput: mi,
Expand Down Expand Up @@ -618,16 +618,6 @@ func (d *DeploymentController) setGatewayNameLabel(ti *TemplateInput) {
log.Debugf("deployment %s/%s not found in store; using to the new gateway name label", ti.DeploymentName, ti.Namespace)
return
}

// Base label choice on the deployment's selector
_, exists := dep.(*appsv1.Deployment).Spec.Selector.MatchLabels[constants.DeprecatedGatewayNameLabel]
if !exists {
// The old label doesn't already exist on the deployment; use the new label
return
}

// The old label exists on the deployment; use the old label
ti.GatewayNameLabel = constants.DeprecatedGatewayNameLabel
Copy link
Member

Choose a reason for hiding this comment

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

This is for backwards compatibility from a pretty recent change, not sure we are ready to remove it? @keithmattix

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention is to remove in 1.22

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this is still a release or 2 too early; it just got merged in 1.20 IIRC

Copy link
Member Author

Choose a reason for hiding this comment

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

But it is marked to be removed in 1.22

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was probably too aggressive; 1 more release should be ok with an upgrade note

Copy link
Member

Choose a reason for hiding this comment

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

agree with Keith (at the time of the comment being merged, I had worries about 1.22 being too early). Note Keith added the comment.

This code costs us nothing but changing now will break users

}

type TemplateInput struct {
Expand Down
13 changes: 8 additions & 5 deletions pilot/pkg/config/kube/ingress/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package ingress
import (
"errors"
"fmt"
"reflect"
"sort"
"sync"

Expand Down Expand Up @@ -186,29 +187,31 @@ func (c *controller) onEvent(item types.NamespacedName) error {
// we should check need process only when event is not delete,
// if it is delete event, and previously processed, we need to process too.
if event != model.EventDelete {
oldIngress := c.ingresses[config.NamespacedName(ing)]
shouldProcess := c.shouldProcessIngressUpdate(ing)
if !shouldProcess {
return nil
}
// only trigger when real changes were found
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem quite right because it doesn't take into account other dependencies? For example, when Service changes, we need to do a push. Even if Ingress doesn't change

Copy link
Member

Choose a reason for hiding this comment

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

I also don't get why we can remove the AlwaysPush. The VS and GW we are pushing below have no spec, so won't they just always be skipped?

Copy link
Member Author

Choose a reason for hiding this comment

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

as to AlwaysPush, we have annotation with istio.io

As to This doesn't seem quite right because it doesn't take into account other dependencies?

Thanks for remind, need to revert it. Seems we have no way to distinguish unless updating the input. Let's keep the original

Copy link
Member Author

Choose a reason for hiding this comment

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

	for annotation := range curr.Meta.Annotations {
		if strings.Contains(annotation, "istio.io") {
			return true
		}
	}
``

if oldIngress != nil && reflect.DeepEqual(oldIngress.Spec, ing.Spec) {
return nil
}
}

vsmetadata := config.Meta{
Name: item.Name + "-" + "virtualservice",
Namespace: item.Namespace,
GroupVersionKind: gvk.VirtualService,
// Set this label so that we do not compare configs and just push.
Labels: map[string]string{constants.AlwaysPushLabel: "true"},
hzxuzhonghu marked this conversation as resolved.
Show resolved Hide resolved
Annotations: map[string]string{constants.InternalRouteSemantics: constants.RouteSemanticsIngress},
}
gatewaymetadata := config.Meta{
Name: item.Name + "-" + "gateway",
Namespace: item.Namespace,
GroupVersionKind: gvk.Gateway,
// Set this label so that we do not compare configs and just push.
Copy link
Member

Choose a reason for hiding this comment

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

How is this fixed? I don't get it

Copy link
Member Author

Choose a reason for hiding this comment

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

@howardjohn Take a look at the below logic, not only ingress but also gateway api go through this comparison

func needsPush(prev config.Config, curr config.Config) bool {

    ...
	// If the config is not Istio, let us just push.
	if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
		return true
	}
	// If current/previous metadata has "*istio.io" label/annotation, just push
	for label := range curr.Meta.Labels {
		if strings.Contains(label, "istio.io") {   
			return true
		}
	}
	for annotation := range curr.Meta.Annotations {
		if strings.Contains(annotation, "istio.io") {    // Here if we checked an annotation, we will always push
			return true
		}
	}
	for label := range prev.Meta.Labels {
		if strings.Contains(label, "istio.io") {
			return true
		}
	}
	for annotation := range prev.Meta.Annotations {
		if strings.Contains(annotation, "istio.io") {
			return true
		}
	}
	....
	return true
}

Copy link
Member

Choose a reason for hiding this comment

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

wow that's extremely subtle and likely to break in the future. I really prefer the explicit AlwaysPush...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, you know this stull of inner constant looks very hardcoding. I would like to remove all those by updating configHandler with an explicit arg, so we donot need to compare by needPush for some resources converted from ingress/gateway-api

Copy link
Member Author

Choose a reason for hiding this comment

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

But currently we cannot gert rid of InternalRouteSemantics annotation easily, cause it is used across the code

Labels: map[string]string{constants.AlwaysPushLabel: "true"},
Annotations: map[string]string{constants.InternalRouteSemantics: constants.RouteSemanticsIngress},
}

// Trigger updates for Gateway and VirtualService
// TODO: we could be smarter here and only trigger when real changes were found
for _, f := range c.virtualServiceHandlers {
f(config.Config{Meta: vsmetadata}, config.Config{Meta: vsmetadata}, event)
}
Expand Down
5 changes: 0 additions & 5 deletions pilot/pkg/model/policyattachment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,6 @@ const (

func KubernetesGatewayNameAndExists(l labels.Instance) (string, bool) {
gwName, exists := l[constants.GatewayNameLabel]
if !exists {
// TODO: Remove deprecated gateway name label (1.22 or 1.23)
gwName, exists = l[constants.DeprecatedGatewayNameLabel]
}

return gwName, exists
}

Expand Down
5 changes: 0 additions & 5 deletions pkg/config/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,6 @@ const (

TestVMVersionLabel = "istio.io/test-vm-version"

// Label to skip config comparison.
AlwaysPushLabel = "internal.istio.io/always-push"

// InternalParentNames declares the original resources of an internally-generated config.
// This is used by k8s gateway-api.
// It is a comma separated list. For example, "HTTPRoute/foo.default,HTTPRoute/bar.default"
Expand Down Expand Up @@ -175,8 +172,6 @@ const (
RemoteGatewayClassName = "istio-remote"
WaypointGatewayClassName = "istio-waypoint"

// DeprecatedGatewayNameLabel indicates the gateway managing a particular proxy instances. Only populated for Gateway API gateways
DeprecatedGatewayNameLabel = "istio.io/gateway-name"
Copy link
Member

Choose a reason for hiding this comment

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

How about the labels of k8s gateways in manifests/charts/istio-control/istio-discovery/files?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we can remove too, as gateway.networking.k8s.io/gateway-name is also added

// GatewayNameLabel indicates the gateway managing a particular proxy instances. Only populated for Gateway API gateways
GatewayNameLabel = "gateway.networking.k8s.io/gateway-name"

Expand Down