Revert "remove feature flags for se selecting pods (#37374)" #40716

GregHanson · 2022-08-30T18:13:23Z

This reverts commit 2ffbf67.

Please provide a description of this PR:

feature flags PILOT_ENABLE_SERVICEENTRY_SELECT_PODS and PILOT_ENABLE_K8S_SELECT_WORKLOAD_ENTRIES were removed in #37374. PILOT_ENABLE_SERVICEENTRY_SELECT_PODS was re-added in #37407. This PR re-adds PILOT_ENABLE_K8S_SELECT_WORKLOAD_ENTRIES

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

GregHanson · 2022-08-30T18:13:51Z

/hold for discussion from #37374 (comment)

GregHanson · 2022-08-30T18:14:28Z

/cherry-pick release-1.15

istio-testing · 2022-08-30T18:14:29Z

@GregHanson: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

GregHanson · 2022-08-30T18:14:52Z

/cherry-pick release-1.14

istio-testing · 2022-08-30T18:14:53Z

@GregHanson: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This reverts commit 2ffbf67.

linsun

based on today's WG mtg, okay to restore this for now before we come up with a plan to support this officially. Notes:

OK to restore this function for now
Need a design doc to capture user case and have further discussion

@ramaraochavali can you approve?

linsun · 2022-08-31T18:26:52Z

should we add a small release note for this? @GregHanson

howardjohn · 2022-08-31T18:33:38Z

Ca nyou make sure you don't depend on other flags as well?

…

On Wed, Aug 31, 2022 at 11:27 AM Lin Sun ***@***.***> wrote: should we add a small release note for this? @GregHanson <https://github.com/GregHanson> — Reply to this email directly, view it on GitHub <#40716 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXIBDF6C2UVXO7AQ62LV36PXRANCNFSM6AAAAAAQAUZG34> . You are receiving this because your review was requested.Message ID: ***@***.***>

GregHanson · 2022-08-31T22:14:14Z

test for verifying the fix:

cat <<\EOF > ./my-config.yaml
apiVersion: install.istio.io/v1alpha1
kind: IstioOperator
spec:
  hub: $HUB
  tag: $TAG
  components:
    pilot:
      k8s:
        env:
        - name: PILOT_ENABLE_K8S_SELECT_WORKLOAD_ENTRIES
          value: "false"
EOF
istioctl install -f my-config.yaml

# deploy helloworld-v1 inside of mesh and helloworld-v2 outside mesh
kubectl label namespace default istio-injection=enabled
kubectl apply -f samples/sleep/sleep.yaml
kubectl apply -f samples/helloworld/helloworld.yaml
kubectl create ns external
kubectl apply -f samples/helloworld/helloworld.yaml -n external
kubectl delete deploy helloworld-v2
kubectl delete deploy helloworld-v1 -n external

# add the v2 version
kubectl apply -f - <<EOF
apiVersion: networking.istio.io/v1alpha3
kind: WorkloadEntry
metadata:
  name: hello-svc
spec:
  address: 10.244.1.6 # pod IP of helloworld-v2 in external ns
  labels:
    app: helloworld
    instance-id: vm1
---
apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: hello-svc
spec:
  hosts:
  - gihanson.hello.com
  location: MESH_INTERNAL
  ports:
  - number: 5000
    name: http
    protocol: HTTP
  resolution: STATIC
  workloadSelector:
    labels:
      app: helloworld
EOF

Performing curls from the sleep pod to gihanson.hello.com:5000/hello loadbalances between v1 and v2 versions of the service. Calling helloworld.default.svc.cluster.local:5000/hello goes to only v1 when PILOT_ENABLE_K8S_SELECT_WORKLOAD_ENTRIES=false and both versions when PILOT_ENABLE_K8S_SELECT_WORKLOAD_ENTRIES=true

ericvn · 2022-09-01T14:17:40Z

Does this PR need to be cherry-picked to 1.15? I see there is a version of this for 1.14.

GregHanson · 2022-09-01T14:21:07Z

Should be set to cherry-pick to 1.15 due to comments above. A 1.14 cherry-pick exists already here, but I will delete that one if the auto cherrypick created from this PR merges OK

istio-testing · 2022-09-01T14:33:22Z

In response to a cherrypick label: #40716 failed to apply on top of branch "release-1.15":

Applying: Revert "remove feature flags for se selecting pods (#37374)"
Using index info to reconstruct a base tree...
M	pilot/pkg/features/pilot.go
M	pilot/pkg/serviceregistry/kube/controller/controller.go
M	pilot/pkg/serviceregistry/kube/controller/controller_test.go
M	pilot/pkg/serviceregistry/kube/controller/multicluster.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/multicluster.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/controller_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/controller.go
Auto-merging pilot/pkg/features/pilot.go
CONFLICT (content): Merge conflict in pilot/pkg/features/pilot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Revert "remove feature flags for se selecting pods (#37374)"
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

istio-testing · 2022-09-01T14:33:23Z

In response to a cherrypick label: new issue created for failed cherrypick: #40761

istio-testing · 2022-09-01T14:34:00Z

@GregHanson: #40716 failed to apply on top of branch "release-1.14":

Applying: Revert "remove feature flags for se selecting pods (#37374)"
Using index info to reconstruct a base tree...
M	pilot/pkg/features/pilot.go
M	pilot/pkg/serviceregistry/kube/controller/controller.go
M	pilot/pkg/serviceregistry/kube/controller/controller_test.go
M	pilot/pkg/serviceregistry/kube/controller/endpointcontroller.go
M	pilot/pkg/serviceregistry/kube/controller/multicluster.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/multicluster.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/multicluster.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/endpointcontroller.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/controller_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/controller.go
Auto-merging pilot/pkg/features/pilot.go
CONFLICT (content): Merge conflict in pilot/pkg/features/pilot.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Revert "remove feature flags for se selecting pods (#37374)"
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

istio-testing · 2022-09-01T14:34:02Z

@GregHanson: new issue created for failed cherrypick: #40762

In response to this:

/cherry-pick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…tio#40716) * Revert "remove feature flags for se selecting pods (istio#37374)" This reverts commit 2ffbf67. * add release note for re-added feature flag

…#40766) * Revert "remove feature flags for se selecting pods (#37374)" This reverts commit 2ffbf67. * add release note for re-added feature flag

…#40764) * Revert "remove feature flags for se selecting pods (#37374)" This reverts commit 2ffbf67. * add release note for re-added feature flag

GregHanson requested review from a team as code owners August 30, 2022 18:13

istio-policy-bot added the area/networking label Aug 30, 2022

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

GregHanson added the do-not-merge Block automatic merging of a PR. label Aug 30, 2022

Revert "remove feature flags for se selecting pods (istio#37374)"

4783266

This reverts commit 2ffbf67.

GregHanson force-pushed the revert_37374 branch from a26b234 to 4783266 Compare August 30, 2022 18:25

linsun approved these changes Aug 31, 2022

View reviewed changes

add release note for re-added feature flag

566ec88

GregHanson requested review from a team as code owners August 31, 2022 22:13

ramaraochavali approved these changes Sep 1, 2022

View reviewed changes

lei-tang approved these changes Sep 1, 2022

View reviewed changes

linsun approved these changes Sep 1, 2022

View reviewed changes

GregHanson removed the do-not-merge Block automatic merging of a PR. label Sep 1, 2022

ericvn added the cherrypick/release-1.15 label Sep 1, 2022

istio-testing merged commit dd0d5e4 into istio:master Sep 1, 2022

istio-testing mentioned this pull request Sep 1, 2022

[release-1.15] Revert "remove feature flags for se selecting pods (#37374)" #40761

Closed

istio-testing mentioned this pull request Sep 1, 2022

[release-1.14] Revert "remove feature flags for se selecting pods (#37374)" #40762

Closed

GregHanson deleted the revert_37374 branch September 1, 2022 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "remove feature flags for se selecting pods (#37374)" #40716

Revert "remove feature flags for se selecting pods (#37374)" #40716

GregHanson commented Aug 30, 2022 •

edited by istio-policy-bot

Loading

GregHanson commented Aug 30, 2022

GregHanson commented Aug 30, 2022

istio-testing commented Aug 30, 2022

GregHanson commented Aug 30, 2022

istio-testing commented Aug 30, 2022

linsun left a comment

linsun commented Aug 31, 2022

howardjohn commented Aug 31, 2022 via email

GregHanson commented Aug 31, 2022 •

edited

Loading

ericvn commented Sep 1, 2022 •

edited

Loading

GregHanson commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

Revert "remove feature flags for se selecting pods (#37374)" #40716

Revert "remove feature flags for se selecting pods (#37374)" #40716

Conversation

GregHanson commented Aug 30, 2022 • edited by istio-policy-bot Loading

GregHanson commented Aug 30, 2022

GregHanson commented Aug 30, 2022

istio-testing commented Aug 30, 2022

GregHanson commented Aug 30, 2022

istio-testing commented Aug 30, 2022

linsun left a comment

Choose a reason for hiding this comment

linsun commented Aug 31, 2022

howardjohn commented Aug 31, 2022 via email

GregHanson commented Aug 31, 2022 • edited Loading

ericvn commented Sep 1, 2022 • edited Loading

GregHanson commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

istio-testing commented Sep 1, 2022

GregHanson commented Aug 30, 2022 •

edited by istio-policy-bot

Loading

GregHanson commented Aug 31, 2022 •

edited

Loading

ericvn commented Sep 1, 2022 •

edited

Loading