Clean constants #50170

hzxuzhonghu · 2024-04-01T06:49:01Z

Please provide a description of this PR:

remove

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

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

Please check any characteristics that apply to this pull request.

Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

hanxiaop · 2024-04-01T07:32:34Z

pkg/config/constants/constants.go

@@ -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"


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

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

pilot/pkg/bootstrap/config_compare.go

howardjohn · 2024-04-01T18:18:59Z

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

-	}
-
-	// The old label exists on the deployment; use the old label
-	ti.GatewayNameLabel = constants.DeprecatedGatewayNameLabel


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

The intention is to remove in 1.22

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

But it is marked to be removed in 1.22

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

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

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

…p push when ingress spec not change

howardjohn

This seems to break a lot of ingress stuff, where we don't have a lot of test coverage.

howardjohn · 2024-04-10T17:52:05Z

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

 		shouldProcess := c.shouldProcessIngressUpdate(ing)
 		if !shouldProcess {
 			return nil
 		}
+		// only trigger when real changes were found


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

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?

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

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

howardjohn · 2024-04-11T21:46:00Z

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

 	}
 	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.


How is this fixed? I don't get it

@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 }

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

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

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

This reverts commit bef1dd4.

howardjohn · 2024-04-12T14:11:02Z

pilot/pkg/bootstrap/config_compare.go

@@ -35,27 +36,11 @@ func needsPush(prev config.Config, curr config.Config) bool {
 	if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") {
 		return true
 	}
-	// If current/previous metadata has "*istio.io" label/annotation, just push


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

why should make dryrun push?

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

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

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

..... any example?

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

Line 162 in c5463a0

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

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.

hzxuzhonghu · 2024-04-19T01:48:11Z

@howardjohn another look please

costinm · 2024-04-19T13:49:00Z

On Mon, Apr 15, 2024 at 7:54 PM hzxuzhonghu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pilot/pkg/bootstrap/config_compare.go <#50170 (comment)>: > @@ -35,27 +36,11 @@ func needsPush(prev config.Config, curr config.Config) bool { if !strings.HasSuffix(prev.GroupVersionKind.Group, "istio.io") { return true } - // If current/previous metadata has "*istio.io" label/annotation, just push 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.

We should move EVERYTHING under a spec - as either 'supported for users', 'internal' or 'deprecated', and add some validation that only 'supported' values can be set by users. Together with the env variables it creates extreme danger for anyone who attempts to use OPA or other validations to restrict users from unsupported features.

…

Message ID: ***@***.***>

hzxuzhonghu · 2024-04-22T11:30:27Z

Yes, agree. The path to moving a env/label/annotation to API seems super harder in istio, we can see the number of those stuff is becoming bigger and bigger.

howardjohn

As mentioned in the other comments, removing this label is too risky and harmful to users; it costs us nothing to keep it longer.

hzxuzhonghu · 2024-04-23T01:16:26Z

I update the description, only remove DeprecatedGatewayNameLabel = "istio.io/gateway-name"

howardjohn · 2024-04-23T01:22:01Z

That's the label I don't want to remove. The motivation for the change seems to be a TODO comment that both the author and approved agree was a mistake.

istio-policy-bot · 2024-05-23T05:03:56Z

🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2024-04-23. It will be closed on 2024-06-07 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

hzxuzhonghu requested review from a team as code owners April 1, 2024 06:49

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 1, 2024

hanxiaop reviewed Apr 1, 2024

View reviewed changes

howardjohn requested changes Apr 1, 2024

View reviewed changes

hzxuzhonghu added 3 commits April 7, 2024 15:51

remove always push labels

bef1dd4

Remove deprecated gateway name support

4866367

update

92a9480

hzxuzhonghu force-pushed the clean-constants branch from e1b4812 to 9e46a77 Compare April 7, 2024 08:55

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 7, 2024

hzxuzhonghu added the release-notes-none Indicates a PR that does not require release notes. label Apr 7, 2024

howardjohn reviewed Apr 8, 2024

View reviewed changes

pilot/pkg/config/kube/ingress/controller.go Outdated Show resolved Hide resolved

add internal.istio.io/route-semantics=ingress annotation and also ski…

d5fb49d

…p push when ingress spec not change

hzxuzhonghu force-pushed the clean-constants branch from 9e46a77 to d5fb49d Compare April 10, 2024 09:29

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2024

howardjohn requested changes Apr 10, 2024

View reviewed changes

restore ingress real change check

49cc16c

howardjohn reviewed Apr 11, 2024

View reviewed changes

hzxuzhonghu added 2 commits April 12, 2024 14:55

Revert "remove always push labels"

6c380d4

This reverts commit bef1dd4.

update

02fe586

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 12, 2024

howardjohn requested changes Apr 12, 2024

View reviewed changes

revert config compare

0807711

istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 16, 2024

howardjohn requested changes Apr 22, 2024

View reviewed changes

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean constants #50170

Clean constants #50170

hzxuzhonghu commented Apr 1, 2024 •

edited

hanxiaop Apr 1, 2024

hzxuzhonghu Apr 7, 2024

howardjohn Apr 1, 2024

hzxuzhonghu Apr 2, 2024

keithmattix Apr 10, 2024

hzxuzhonghu Apr 11, 2024

keithmattix Apr 11, 2024

howardjohn Apr 19, 2024

howardjohn left a comment

howardjohn Apr 10, 2024

howardjohn Apr 10, 2024

hzxuzhonghu Apr 11, 2024

hzxuzhonghu Apr 11, 2024

howardjohn Apr 11, 2024

hzxuzhonghu Apr 12, 2024

howardjohn Apr 12, 2024

hzxuzhonghu Apr 12, 2024

hzxuzhonghu Apr 12, 2024

howardjohn Apr 12, 2024

hzxuzhonghu Apr 15, 2024

howardjohn Apr 15, 2024

hzxuzhonghu Apr 16, 2024

howardjohn Apr 16, 2024

hzxuzhonghu Apr 16, 2024

howardjohn Apr 16, 2024

hzxuzhonghu Apr 16, 2024

hzxuzhonghu commented Apr 19, 2024

costinm commented Apr 19, 2024 via email

hzxuzhonghu commented Apr 22, 2024

howardjohn left a comment

hzxuzhonghu commented Apr 23, 2024

howardjohn commented Apr 23, 2024

istio-policy-bot commented May 23, 2024

Clean constants #50170

Are you sure you want to change the base?

Clean constants #50170

Conversation

hzxuzhonghu commented Apr 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Apr 19, 2024

costinm commented Apr 19, 2024 via email

hzxuzhonghu commented Apr 22, 2024

howardjohn left a comment

Choose a reason for hiding this comment

hzxuzhonghu commented Apr 23, 2024

howardjohn commented Apr 23, 2024

istio-policy-bot commented May 23, 2024

hzxuzhonghu commented Apr 1, 2024 •

edited