-
Notifications
You must be signed in to change notification settings - Fork 34
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.
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
Update tutorials to reference 'Getting Started' as prereq #1129
Conversation
… to remove the use of guide to tutorial Signed-off-by: R-Lawton <[email protected]>
Signed-off-by: R-Lawton <[email protected]>
Signed-off-by: David Martin <[email protected]>
Signed-off-by: David Martin <[email protected]>
👀 |
|
||
Follow this [setup doc](https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/install/install-make.md) to set up your environment before continuing with this doc. | ||
- Kubernetes cluster with Kuadrant operator installed. See our [Getting Started](/getting-started) guide for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just because this will 404 on GH, maybe we include an absolute link? https://docs.kuadrant.io/latest/getting-started
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I realise this will 404 too right now until we do our next stable release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonmadigan summarising some options discussed offline:
- Your suggestion above to change link to absolute /latest
- Similar to above, but use a /dev link
- Leaving as is, knowing it will 404 on github, but be OK on docs site
- Move those user guides into the docs repo, so relative links work in github and on docs site
The links 404ing are a symptom of how these docs are being used for the docs site by mkdocs.
The dual target is not ideal, given this limitation around relative links if referencing a doc from another repo.
I'm not keen on having the absolute url as that gets treated as an external url by the mkdocs tooling and potentially bypasses some CI checks (like detecting 404s)
If you're in agreement, how about we go with 3 (as that's already the case potentially for other docs),
with an agreement to investigate 4 as a long term option, at least for these docs in the kuadrant operator site.
I wonder if there's a need to have these docs in this repo as well, if we have them in the docs repo and site, and link to the site from the main README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like the most pragmatic option to me. can work on 4 longer term.
doc/user-guides/auth/auth-for-app-devs-and-platform-engineers.md
Outdated
Show resolved
Hide resolved
``` | ||
|
||
Create the Toy Store HTTPRoute | ||
```bash | ||
|
||
kubectl apply -f - <<EOF | ||
apiVersion: gateway.networking.k8s.io/v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
w/ helm guide, GW API v1.1 is installed
kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: toystore
namespace: ${KUADRANT_DEVELOPER_NS}
labels:
app: toystore
spec:
parentRefs:
- name: ${KUADRANT_GATEWAY_NAME}
namespace: ${KUADRANT_GATEWAY_NS}
hostnames:
- api.toystore.com
rules:
- name: rule-1
matches:
- method: GET
path:
type: PathPrefix
value: "/cars"
- method: GET
path:
type: PathPrefix
value: "/dolls"
backendRefs:
- name: toystore
port: 80
- name: rule-2
matches:
- path:
type: PathPrefix
value: "/admin"
backendRefs: <....
Error from server (BadRequest): error when creating "STDIN": HTTPRoute in version "v1" cannot be handled as a HTTPRoute: strict decoding error: unknown field "spec.rules[0].name", unknown field "spec.rules[1].name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will continue tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-martin did you install your stack with helm (via the guide on https://artifacthub.io/packages/helm/kuadrant/kuadrant-operator) + cloud-provider-kind
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, named rules are in 1.2[1], I guess this won't work as the helm guide currently installs the v1.1 CRDs
[1] https://kubernetes.io/blog/2024/11/21/gateway-api-v1-2/#new-additions-to-experimental-channel
went without to push forward:
kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: toystore
namespace: ${KUADRANT_DEVELOPER_NS}
labels:
app: toystore
spec:
parentRefs:
- name: ${KUADRANT_GATEWAY_NAME}
namespace: ${KUADRANT_GATEWAY_NS}
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried the tutorials where I made the updates to add rule names.
I added those as I thought it was an oversight in those guides, given the reference to sectionNames in the Policy examples.
We might have to back out those sectionName references if they won't work with the latest kuadrant release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, just looks bad ootb as the helm guide steers you towards 1.1
if we're not highlighting sectionNames, perhaps we don't need right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the helm install and olm install reference 1.1.0 of the gateway api CRDs.
I've traced back the original author of the guide to look for advice on what to do here.
Turns out this wasn't an oversight and it was written as intended.
Currently in Kuadrant, there are implied rule names when referencing them from a policy
- https://github.com/Kuadrant/policy-machinery/blob/ca213ee94f9b9f35d59d2d8a54a70902d91c8537/machinery/gateway_api_topology.go#L391
For example,rule-1
means the first rule, and so on.
The reason for this was to support sectionNames in a policy targetRef if gateway api 1.1 is used (which is the case with OSSM support at present on OpenShift).
The plan is to remove this and leverage the experimental feature in gateway api 1.2.
I'll back out the changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
👀 - continuing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple of small things that I think we should fixup
doc/user-guides/auth/auth-for-app-devs-and-platform-engineers.md
Outdated
Show resolved
Hide resolved
doc/user-guides/ratelimiting/authenticated-rl-for-app-developers.md
Outdated
Show resolved
Hide resolved
doc/user-guides/ratelimiting/authenticated-rl-with-jwt-and-k8s-authnz.md
Outdated
Show resolved
Hide resolved
2da5b75
to
59e1f97
Compare
Signed-off-by: David Martin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
Replaces #1126
Closes #1093
Other changes: