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 all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 3 additions & 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,15 +27,13 @@ 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.
if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
return true
}

// If current/previous metadata has "*istio.io" label/annotation, 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.

This was not only for "AlwaysPush". There are annotations that impact XDS generation. Below we only compare spec, so if a config changes just the annotation nothing happens.

Things like istio.io/dry-run, etc

Copy link
Member Author

Choose a reason for hiding this comment

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

why should make dryrun push?

Copy link
Member

Choose a reason for hiding this comment

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

Any change to inputs that produces a change in outputs should result in a push so things are recomputed. Otherwise the update would be ignored and generate invalid information.

Its not special about "dry-run", just anything where annotation change leads to XDS changes. I recall dry-run was one motivation use

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more explicit, i donot think of a case we should annotate a config to make istiod push.

Originally this function is to mitigate pushed when labels change to a DR. Simutabeously introduce alwaysPush label, but not sure i know why "*istio.io" should be checked if we checked alwaysPush labels explicitly

Copy link
Member

Choose a reason for hiding this comment

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

We check istio.io only because we know other random annotations are not used in xds generation. It's a hack to improve performance and ignore irrelevant annotations

Copy link
Member Author

Choose a reason for hiding this comment

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

..... any example?

Copy link
Member

Choose a reason for hiding this comment

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

istio/pilot/pkg/security/authz/builder/builder.go

Line 162 in c5463a0

if val, ok := policy.Annotations[annotation.IoIstioDryRun.Name]; ok {

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 just learned another annotation, hha. This makes me hard to do any simplify.

BTW, if we need to keep the feature or this is a useful feature, i think so. we should move it under spec.

// Currently applies to label "internal.istio.io/always-push", annotation "istio.io/dry-run"
for label := range curr.Meta.Labels {
if strings.Contains(label, "istio.io") {
return true
Expand All @@ -56,6 +54,7 @@ func needsPush(prev config.Config, curr config.Config) bool {
return true
}
}

prevspecProto, okProtoP := prev.Spec.(proto.Message)
currspecProto, okProtoC := curr.Spec.(proto.Message)
if okProtoP && okProtoC {
Expand Down
20 changes: 0 additions & 20 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 @@ -30,26 +30,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
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
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
2 changes: 0 additions & 2 deletions pkg/config/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,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