Add support for project sail by adam-cattermole · Pull Request #323 · Kuadrant/kuadrant-operator

adam-cattermole · 2023-11-21T14:19:19Z

Changes

There's an Istio CR that provides the meshConfig instead of an IstioOperator CR for the sail operator
- Added new SailWrapper for updating the meshConfig stored in the Istio CR
- We check for IstioOperator presence first, then Istio, then fall back to OSSM 2.x

TODO:

To use the Istio type I've included the maistra operator as a go dep but I think I'm leaning toward duplicating the types as done prior in our https://github.com/Kuadrant/kuadrant-operator/tree/main/api/external/maistra folder rather than including all of these dep changes in the go.mod - Edit: changed my mind here and I think we should just include the types and deal with it for now, in case the API or interaction with it changes we can keep pulling this and potentially do this down the line.
I might investigate the option of using sail to install istio for local development rather than using istioctl
Some more experimental testing + document the entire process (will be here in verification steps)
Test updates

Install Steps:

① Setup OpenShift cluster

Create an OpenShift cluster and follow the first two steps to install Project Sail and Istio using the guide here https://github.com/maistra/istio-operator/blob/maistra-3.0/bundle/README.md - note you should name your Istio CR istiocontrolplane.

② Install Gateway API CRDs

kubectl get crd gateways.gateway.networking.k8s.io &> /dev/null || \
  { kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd?ref=v1.0.0" | kubectl apply -f -; }

③ Install Kuadrant Operator from this branch

Create a new namespace called kuadrant-system:

kubectl create namespace kuadrant-system

Create a CatalogSource:

kubectl apply -f - <<EOF
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: kuadrant-operator-catalog
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/acatterm/kuadrant-operator-catalog:sail
EOF

Navigate to OperatorHub on the openshift-marketplace project and select Kuadrant Operator from this catalog source (kuadrant-operator-catalog). Install with selected namespace set to kuadrant-system and other settings left as default.

Request an instance of Kuadrant in the kuadrant-system namespace:

kubectl -n kuadrant-system apply -f - <<EOF
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
spec: {}
EOF

④ Check loadbalancer quota

You need space for a loadbalancer for istio to create. Within the administration settings in the Openshift Developer Portal for All Projects, navigate to Administration -> ResourceQuotas -> loadbalancer-quota.

Update spec.quota.hard.services.loadbalancers if you have insufficient space.

⑤ Deploy the toystore application

This creates the toystore Deployment, Service as well as a Gateway API Gateway and HTTPRoute:

kubectl apply -f examples/toystore/toystore.yaml

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: toystore-gateway
  labels:
    app: toystore
spec:
  gatewayClassName: istio
  listeners:
  - name: http
    port: 80
    protocol: HTTP
    allowedRoutes:
      namespaces:
        from: All
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: toystore
spec:
  parentRefs:
  - name: toystore-gateway
  hostnames:
  - api.toystore.com
  rules:
  - matches:
    - method: GET
      path:
        type: PathPrefix
        value: "/cars"
    - method: GET
      path:
        type: PathPrefix
        value: "/dolls"
    backendRefs:
    - name: toystore
      port: 80
  - matches:
    - path:
        type: PathPrefix
        value: "/admin"
    backendRefs:
    - name: toystore
      port: 80
EOF

Ensure that the toystore-gateway is Accepted and Programmed. You can then test access to the service.

Set the environment variables:

export INGRESS_HOST=$(kubectl get gtw toystore-gateway -o jsonpath='{.status.addresses[0].value}')
export INGRESS_PORT=$(kubectl get gtw toystore-gateway -o jsonpath='{.spec.listeners[?(@.name=="http")].port}')
export GATEWAY_URL=$INGRESS_HOST:$INGRESS_PORT

Curl the toystore endpoints:

curl -H 'Host: api.toystore.com' "http://$GATEWAY_URL/cars" -i
# HTTP/1.1 200 OK

curl -H 'Host: api.toystore.com' "http://$GATEWAY_URL/dolls" -i
# HTTP/1.1 200 OK

curl -H 'Host: api.toystore.com' "http://$GATEWAY_URL/admin" -i
# HTTP/1.1 200 OK

⑥ Create a simple AuthPolicy

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: AuthPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  rules:
    authentication:
      "api-key-authn":
        apiKey:
          selector: {}
        credentials:
          authorizationHeader:
            prefix: APIKEY
    authorization:
      "only-admins":
        opa:
          rego: |
            groups := split(object.get(input.auth.identity.metadata.annotations, "kuadrant.io/groups", ""), ",")
            allow { groups[_] == "admins" }
        routeSelectors:
        - matches:
          - path:
              type: PathPrefix
              value: "/admin"
EOF

Create secrets to use for this authentication:

kubectl apply -f -<<EOF
apiVersion: v1
kind: Secret
metadata:
  name: api-key-regular-user
  labels:
    authorino.kuadrant.io/managed-by: authorino
stringData:
  api_key: iamaregularuser
type: Opaque
---
apiVersion: v1
kind: Secret
metadata:
  name: api-key-admin-user
  labels:
    authorino.kuadrant.io/managed-by: authorino
  annotations:
    kuadrant.io/groups: admins
stringData:
  api_key: iamanadmin
type: Opaque
EOF

Send requests to the protected gateway:

curl -H 'Host: api.toystore.com' "http://$GATEWAY_URL/cars" -i
# HTTP/1.1 401 Unauthorized

curl -H 'Host: api.toystore.com' -H 'Authorization: APIKEY iamaregularuser' "http://$GATEWAY_URL/cars" -i
# HTTP/1.1 200 OK

curl -H 'Host: api.toystore.com' -H 'Authorization: APIKEY iamaregularuser' "http://$GATEWAY_URL/admin" -i
# HTTP/1.1 403 Forbidden

curl -H 'Host: api.toystore.com' -H 'Authorization: APIKEY iamanadmin' "http://$GATEWAY_URL/admin" -i
# HTTP/1.1 200 OK

⑦ Create a simple RateLimitPolicy

Lets apply a rate limit to our /cars endpoint:

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta2
kind: RateLimitPolicy
metadata:
  name: toystore
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: toystore
  limits:
    "get-cars":
      rates:
      - limit: 5
        duration: 10
        unit: second
      routeSelectors:
      - matches:
        - path:
            type: PathPrefix
            value: "/cars"
EOF

Test the endpoint:

while :; do curl --write-out '%{http_code}\n' --silent --output /dev/null -H 'Host: api.toystore.com' -H 'Authorization: APIKEY iamaregularuser' "http://$GATEWAY_URL/cars" | grep -E --color "\b(429)\b|$"; sleep 1; done

codecov · 2023-11-21T14:22:20Z

Codecov Report

Merging #323 (57ed8b9) into main (b736a7b) will decrease coverage by 0.57%.
The diff coverage is 30.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
- Coverage   65.42%   64.85%   -0.57%     
==========================================
  Files          35       35              
  Lines        3800     3807       +7     
==========================================
- Hits         2486     2469      -17     
- Misses       1122     1138      +16     
- Partials      192      200       +8

Flag	Coverage Δ
integration	`69.64% <30.00%> (-1.09%)`	⬇️

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)	`30.24% <ø> (ø)`
pkg/log (u)	`31.81% <ø> (ø)`
pkg/reconcilers (u)	`33.33% <ø> (ø)`
pkg/rlptools (u)	`56.46% <ø> (ø)`
controllers (i)	`69.64% <30.00%> (-1.09%)`	⬇️

Files	Coverage Δ
pkg/istio/mesh_config.go	`51.57% <ø> (ø)`
controllers/kuadrant_controller.go	`49.45% <30.00%> (-3.50%)`	⬇️

... and 2 files with indirect coverage changes

alexsnaps · 2023-11-22T14:59:40Z

@adam-cattermole is that an attempt to fix #299 ?

adam-cattermole · 2023-11-22T15:03:34Z

@adam-cattermole is that an attempt to fix #299 ?

Yes it is, still a WIP though

adam-cattermole · 2023-11-29T10:39:46Z

I'm still looking at the other points in the to do section but given they could be follow ups I've marked ready for review

maleck13 · 2023-11-29T12:35:40Z

@adam-cattermole I am a fan of using project sail in regular development but I think we need to be careful to ensure we are still staying compatible with "regular" istio installs

adam-cattermole · 2023-11-29T14:08:09Z

@adam-cattermole I am a fan of using project sail in regular development but I think we need to be careful to ensure we are still staying compatible with "regular" istio installs

I've left the original install options in the Makefile but added a commit moving to installing project sail locally. Will need an additional change to the docs as there is a minor difference in how we access the deployed application.

I'll make a change to have the integration tests run on each istio install type - that might be enough to make sure we aren't breaking anything on each PR.

maleck13 · 2023-12-01T09:10:23Z

 spec:
  gatewayClassName: istio
  listeners:
-  - name: default


I don't mind the name change, but any specific reason to change it?

This was just to match the naming when retrieving the port from the gateway as used in the istio docs - no real reason though we can leave it as default and I can update the exports in each of the guides

maleck13 · 2023-12-01T09:11:04Z

    allowedRoutes:
      namespaces:
        from: All
-  addresses:


glad we can remove this,but does it cause any issues with other user-guides?

On a sail install this part of the spec gets populated when it creates the loadbalancer service so it should be present anyway. I'll double check that I didn't miss any user guides though.

maleck13 · 2023-12-01T09:13:37Z

+  rawValues:
+    gateways:
+      istio-ingressgateway:
+        autoscaleEnabled: false


be good to get a couple of comments as to what it does or why its needed?

This was just to match our original local deploy configuration but isn't necessary and could be removed https://github.com/Kuadrant/kuadrant-operator/blob/main/config/dependencies/istio/istio-operator.yaml#L48-L54

maleck13 · 2023-12-01T09:45:41Z

+	assert.Equal(t, wrapper.GetConfigObject(), ist)
+}
+
+func TestSailWrapper_GetMeshConfig(t *testing.T) {


do we want to cover the error cases?

maleck13

Couple of small comments but the changes look reasonable to me. I don't want to approve this as I don't work in the kuadrant-operator enough. Will leave that to @eguzki @guicassolato or @alexsnaps

I will try out the verification steps next

Note I will try it on kubernetes via https://github.com/maistra/istio-operator/tree/maistra-3.0

Boomatang

I only have one comment

Boomatang · 2023-12-05T10:02:25Z

+.PHONY: sail-install
+sail-install: kustomize
+	$(MAKE) install-metallb
+	$(KUSTOMIZE) build $(ISTIO_INSTALL_DIR)/sail | kubectl apply -f -


This could be replaced with kubectl apply -k $(ISTIO_INSTALL_DIR)/sail I believe. The default version ov kustomize shipped with kubectl might be missing features required, but I have yet to hit this issue.

This does raise a second question. We are controlling the version of kustomize but not kubectl. Should we be controlling the kubectl version?

I see your suggestion, but given we use this for all of our kustomize uses elsewhere I think it would be strange to have this single occurrence use kubectl apply -k. Perhaps we should have a follow up issue to replace all $(KUSTOMIZE) build .. | kubectl apply -f if we think that makes sense? Perhaps we want more fine-grained control over the kustomize version so we're not tied to a specific version of kubectl.

maleck13 · 2023-12-07T10:18:10Z

I was able to work with this in kubernetes. https://gist.github.com/maleck13/eb5860181404f055ac5ec64d81945a4f @alexsnaps @adam-cattermole this ready to merge?

adam-cattermole · 2023-12-07T11:03:21Z

@maleck13 I'm happy with the changes but I think we could use an approving review from service protection

didierofrivia · 2023-12-08T10:37:05Z

+		return nil, err
+	} else {
+		// Error is NoMatchError so check for Istio CR instead
+		ist := &istiov1alpha1.Istio{}


Is this CR watched by the istio operator too?

Yep it is (the project sail install / not istioctl install which still uses IstioOperator CR)

didierofrivia

The way the sail project is added through a wrapper follows the same pattern as the alternatives, however this clearly exposes that we need to find a more declarative way of choosing what mesh provider we are installing. I'd say it's OK now, but we need to consider kickstarting a RFC, say part of the Kuadrant CR spec definitions efforts going like Kuadrant/architecture#5, Kuadrant/architecture#18 and Kuadrant/architecture#6

alexsnaps assigned adam-cattermole Nov 22, 2023

adam-cattermole force-pushed the project-sail branch from d422ea8 to 8073ad3 Compare November 28, 2023 14:20

adam-cattermole changed the title ~~Add support for meshConfig from Istio CR~~ Add support for project sail Nov 28, 2023

adam-cattermole force-pushed the project-sail branch 2 times, most recently from a160ad3 to 3b76423 Compare November 28, 2023 14:50

adam-cattermole marked this pull request as ready for review November 29, 2023 10:39

adam-cattermole requested a review from a team as a code owner November 29, 2023 10:39

adam-cattermole force-pushed the project-sail branch 3 times, most recently from 0231512 to ad4678b Compare November 30, 2023 13:49

maleck13 reviewed Dec 1, 2023

View reviewed changes

adam-cattermole force-pushed the project-sail branch from ad4678b to bd080f1 Compare December 1, 2023 12:41

adam-cattermole commented Dec 1, 2023

View reviewed changes

Comment thread pkg/istio/mesh_config_test.go Outdated

adam-cattermole added 6 commits December 1, 2023 13:10

Add support for meshConfig from Istio CR

19577a6

Update rbac for Istio CR

64405b8

Update go dependencies

d285fc2

Install sail operator locally

650d156

Run integration tests on both sail and istioctl install

1570b12

Doc updates using loadbalancer with sail

57ed8b9

adam-cattermole force-pushed the project-sail branch from bd080f1 to 57ed8b9 Compare December 1, 2023 13:10

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

Boomatang reviewed Dec 5, 2023

View reviewed changes

didierofrivia reviewed Dec 8, 2023

View reviewed changes

didierofrivia approved these changes Dec 8, 2023

View reviewed changes

maleck13 mentioned this pull request Dec 8, 2023

Gh 361 single cluster walk through #362

Merged

adam-cattermole merged commit 6e985e3 into Kuadrant:main Dec 8, 2023

adam-cattermole deleted the project-sail branch December 8, 2023 12:46

alexsnaps mentioned this pull request Dec 8, 2023

Look into OSSM 3.x #299

Closed

adam-cattermole linked an issue Dec 11, 2023 that may be closed by this pull request

Look into OSSM 3.x #299

Closed

maleck13 pushed a commit to maleck13/kuadrant-operator that referenced this pull request Dec 12, 2023

Add support for project sail (Kuadrant#323)

909fd6f

Uh oh!

Conversation

adam-cattermole commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

TODO:

Install Steps:

① Setup OpenShift cluster

② Install Gateway API CRDs

③ Install Kuadrant Operator from this branch

④ Check loadbalancer quota

⑤ Deploy the toystore application

⑥ Create a simple AuthPolicy

⑦ Create a simple RateLimitPolicy

Uh oh!

codecov Bot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexsnaps commented Nov 22, 2023

Uh oh!

adam-cattermole commented Nov 22, 2023

Uh oh!

adam-cattermole commented Nov 29, 2023

Uh oh!

maleck13 commented Nov 29, 2023

Uh oh!

adam-cattermole commented Nov 29, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maleck13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Boomatang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adam-cattermole Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maleck13 commented Dec 7, 2023

Uh oh!

adam-cattermole commented Dec 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adam-cattermole Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didierofrivia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adam-cattermole commented Nov 21, 2023 •

edited

Loading

codecov Bot commented Nov 21, 2023 •

edited

Loading

maleck13 left a comment •

edited

Loading

adam-cattermole Dec 5, 2023 •

edited

Loading

adam-cattermole Dec 8, 2023 •

edited

Loading