fix istioctl deployment #371

eguzki · 2023-12-11T14:05:34Z

When running with ISTIO_INSTALL_SAIL=false make local-setup two ingress gateways are present, one with type NodePort, the other created for the GWAPI Gateway as a LoadBalancer service (which never gets accepted as there is no load balancer provisioner).

We need to install metallb for istioctl installs as well as remove the old gateway.

codecov · 2023-12-11T14:10:40Z

Codecov Report

Merging #371 (371fd4f) into main (13c75de) will decrease coverage by 0.83%.
Report is 10 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
- Coverage   66.14%   65.32%   -0.83%     
==========================================
  Files          35       35              
  Lines        3843     3795      -48     
==========================================
- Hits         2542     2479      -63     
- Misses       1113     1124      +11     
- Partials      188      192       +4

Flag	Coverage Δ
integration	`70.92% <ø> (-1.14%)`	⬇️
unit	`58.90% <ø> (-0.65%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	`∅ <ø> (∅)`
pkg/common (u)	`76.92% <ø> (ø)`
pkg/istio (u)	`37.11% <ø> (ø)`
pkg/log (u)	`31.81% <ø> (ø)`
pkg/reconcilers (u)	`33.21% <ø> (ø)`
pkg/rlptools (u)	`56.46% <ø> (ø)`
controllers (i)	`70.92% <ø> (-1.14%)`	⬇️

see 7 files with indirect coverage changes

KevFan · 2023-12-12T10:42:17Z

doc/user-guides/authenticated-rl-for-app-developers.md

Not sure is it just me, but the curl command curl -H 'Host: api.toystore.com' http://$GATEWAY_URL/toy -i is not resolving when running it 🤔

curl: (28) Failed to connect to 172.18.200.0 port 80 after 75004 ms: Couldn't connect to server

Seems to be happening on main also for me. Forwarding the port gives:

Error from server (NotFound): services "istio-ingressgateway" not found

@KevFan What's the status of the gateway?

kubectl get gtw -n istio-system istio-ingressgateway -o yaml

This is the status of the gateway:

addresses: - type: IPAddress value: 172.18.200.0 conditions: - lastTransitionTime: "2023-12-12T10:30:27Z" message: Resource accepted observedGeneration: 1 reason: Accepted status: "True" type: Accepted - lastTransitionTime: "2023-12-12T10:30:35Z" message: Resource programmed, assigned to service(s) istio-ingressgateway-istio.istio-system.svc.cluster.local:80 observedGeneration: 1 reason: Programmed status: "True" type: Programmed listeners: - attachedRoutes: 1 conditions: - lastTransitionTime: "2023-12-12T10:30:27Z" message: No errors found observedGeneration: 1 reason: Accepted status: "True" type: Accepted - lastTransitionTime: "2023-12-12T10:30:27Z" message: No errors found observedGeneration: 1 reason: NoConflicts status: "False" type: Conflicted - lastTransitionTime: "2023-12-12T10:30:27Z" message: No errors found observedGeneration: 1 reason: Programmed status: "True" type: Programmed - lastTransitionTime: "2023-12-12T10:30:27Z" message: No errors found observedGeneration: 1 reason: ResolvedRefs status: "True" type: ResolvedRefs name: http supportedKinds: - group: gateway.networking.k8s.io kind: HTTPRoute - group: gateway.networking.k8s.io kind: GRPCRoute

yeah, that is the issue when deployed with ISTIO_INSTALL_SAIL=false, for some unknown reason, the gateway status does not export the same info. Therefore, the path walking specified is not working as expected,

export INGRESS_HOST=$(kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.status.addresses[0].value}') export INGRESS_PORT=$(kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.spec.listeners[?(@.name=="http")].port}')

The info is there for me for the changes in this branch:

ISTIO_INSTALL_SAIL=false make local-setup kubectl get gtw -n istio-system istio-ingressgateway -o yaml

spec: gatewayClassName: istio listeners: - allowedRoutes: namespaces: from: All name: http port: 80 protocol: HTTP status: addresses: - type: IPAddress value: 172.19.200.0

export INGRESS_HOST=$(kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.status.addresses[0].value}') export INGRESS_PORT=$(kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.spec.listeners[?(@.name=="http")].port}') export GATEWAY_URL=$INGRESS_HOST:$INGRESS_PORT echo $GATEWAY_URL 172.19.200.0:80

After you added the metallb LB, the LoadBalancer type service got the required address and the gateway resource has status.address field. When the LB service does not get the required address, the status does not have the addresses field

@KevFan I might be adding unneeded noise here. istio-ingressgateway is no longer the name of the service. Now it is istio-ingressgateway-istio.

working for me now

Following up with this, Adam found that aside from the service name is now istio-ingressgateway-istio, port forwarding is necessary for macOS systems as docker cant access loadbalancer ip (which is explained by https://github.com/chipmk/docker-mac-net-connect and as an alternative to port forwarding the service).

We're going with keeping the existing note for users might need to port forward instead of suggesting macOS users to have https://github.com/chipmk/docker-mac-net-connect installed as a prerequiste since the port forward approach is more generic

Makefile

doc/user-guides/simple-rl-for-app-developers.md

doc/user-guides/authenticated-rl-for-app-developers.md

doc/user-guides/authenticated-rl-with-jwt-and-k8s-authnz.md

doc/user-guides/simple-rl-for-app-developers.md

eguzki · 2023-12-12T23:26:10Z

doc/user-guides/authenticated-rl-for-app-developers.md

+> ```sh
+> kubectl port-forward -n istio-system service/istio-ingressgateway 9080:80 2>&1 >/dev/null &
+> export GATEWAY_URL=localhost:9080
+> curl -H 'Host: api.toystore.com' http://$GATEWAY_URL/toys -i


nitpick: I like this one more

curl --resolve api.toystore.com:9080:127.0.0.1 -v "http://api.toystore.com:9080/toys -i

Hmm I kinda thought this would be nice to set the GATEWAY_URL for the rest of the guide where they can copy paste the commands. I can update this single occurrence though if you think it makes sense?

It was not about the GATEWAY_URL. This one also works for me

curl --resolve api.toystore.com:$INGRESS_PORT:$INGRESS_HOST -v "http://api.toystore.com:$INGRESS_PORT/toys -i

I'm sorry I don't think I follow; if we're removing the use of the GATEWAY_URL var in this one curl command doesn't that obfuscate the usage for the others. Shouldn't we make them all match? is that the suggestion, remove the GATEWAY_URL and make all of the curl commands follow this above? and then set INGRESS_HOST and _PORT for both port-forward and non-port-forward requests?

it's a nitpick. I just wanted to point out that using --resolve flag of curl is better than setting the Host header. If it makes more sense as you say, go with it.

KevFan

Something minor. Seems some guides use /toys endpoint, while others use /toy

doc/user-guides/authenticated-rl-for-app-developers.md

doc/user-guides/authenticated-rl-with-jwt-and-k8s-authnz.md

KevFan

Looks good to me !

eguzki · 2023-12-13T17:03:39Z

I get an error when reading the port

❯ kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.spec.listeners[?(@.name==http)].port}'
error: error executing jsonpath "{.spec.listeners[?(@.name==http)].port}": Error executing template: unrecognized identifier http. Printing more information for debugging the template:
	template was:
		{.spec.listeners[?(@.name==http)].port}
	object given to jsonpath engine was:
....

I think it can be fixed with

export INGRESS_PORT=$(kubectl get gtw istio-ingressgateway -n istio-system -o jsonpath='{.spec.listeners[?(@.name=="http")].port}')

adam-cattermole · 2023-12-13T17:18:30Z

Thanks @eguzki, good spot - fixed it now

eguzki · 2023-12-13T20:15:56Z

LGTM (I am the original author, so I cannot approve it)

fix istioctl deployment

272af35

eguzki requested a review from adam-cattermole December 11, 2023 14:05

adam-cattermole added the priority/must Required for the release to happen label Dec 11, 2023

adam-cattermole self-assigned this Dec 11, 2023

alexsnaps assigned eguzki Dec 11, 2023

Install metallb for istioctl installs

54b05a5

adam-cattermole force-pushed the fix-istioctl-deployment branch from 037fc52 to 54b05a5 Compare December 11, 2023 14:51

adam-cattermole marked this pull request as ready for review December 11, 2023 15:16

adam-cattermole requested a review from a team as a code owner December 11, 2023 15:16

KevFan reviewed Dec 12, 2023

View reviewed changes

alexsnaps added this to the v0.6.0 milestone Dec 12, 2023

KevFan reviewed Dec 12, 2023

View reviewed changes

Makefile Outdated Show resolved Hide resolved

KevFan reviewed Dec 12, 2023

View reviewed changes

doc/user-guides/simple-rl-for-app-developers.md Outdated Show resolved Hide resolved

eguzki commented Dec 12, 2023

View reviewed changes

KevFan mentioned this pull request Dec 13, 2023

feat: accepted policy status condition #347

Merged

adam-cattermole force-pushed the fix-istioctl-deployment branch from d5acef6 to 89604c0 Compare December 13, 2023 13:58

KevFan requested changes Dec 13, 2023

View reviewed changes

doc/user-guides/authenticated-rl-for-app-developers.md Outdated Show resolved Hide resolved

doc/user-guides/authenticated-rl-with-jwt-and-k8s-authnz.md Outdated Show resolved Hide resolved

adam-cattermole force-pushed the fix-istioctl-deployment branch from 89604c0 to 99a0c28 Compare December 13, 2023 15:38

KevFan approved these changes Dec 13, 2023

View reviewed changes

Update docs for non-linux install

371fd4f

adam-cattermole force-pushed the fix-istioctl-deployment branch from 99a0c28 to 371fd4f Compare December 13, 2023 17:18

adam-cattermole merged commit 78b3b9e into main Dec 14, 2023
22 checks passed

adam-cattermole deleted the fix-istioctl-deployment branch December 14, 2023 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix istioctl deployment #371

fix istioctl deployment #371

eguzki commented Dec 11, 2023 •

edited by adam-cattermole

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

KevFan Dec 12, 2023

adam-cattermole Dec 12, 2023

KevFan Dec 12, 2023

eguzki Dec 12, 2023

adam-cattermole Dec 12, 2023 •

edited

Loading

eguzki Dec 12, 2023

eguzki Dec 12, 2023

eguzki Dec 12, 2023

KevFan Dec 12, 2023

eguzki Dec 12, 2023

adam-cattermole Dec 13, 2023

eguzki Dec 13, 2023

adam-cattermole Dec 13, 2023 •

edited

Loading

eguzki Dec 13, 2023

KevFan left a comment

KevFan left a comment

eguzki commented Dec 13, 2023

adam-cattermole commented Dec 13, 2023

eguzki commented Dec 13, 2023

fix istioctl deployment #371

fix istioctl deployment #371

Conversation

eguzki commented Dec 11, 2023 • edited by adam-cattermole Loading

codecov bot commented Dec 11, 2023 • edited Loading

Codecov Report

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

adam-cattermole Dec 12, 2023 • edited Loading

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

adam-cattermole Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

eguzki commented Dec 13, 2023

adam-cattermole commented Dec 13, 2023

eguzki commented Dec 13, 2023

eguzki commented Dec 11, 2023 •

edited by adam-cattermole

Loading

codecov bot commented Dec 11, 2023 •

edited

Loading

adam-cattermole Dec 12, 2023 •

edited

Loading

adam-cattermole Dec 13, 2023 •

edited

Loading