Skip to content
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

[BREAKING CHANGE] root level kuadrant extensions #78

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

eguzki
Copy link
Collaborator

@eguzki eguzki commented May 15, 2024

What

[BREAKING CHANGE] Openapi info level kuadrant extensions moved to root level

Requires #77 first

Added some happy path tests

Verification Steps

  • OpenAPI doc with kuadrant extensions at the root level
cat << EOF > openapi.yaml
---
openapi: "3.0.3"
info:
  title: "Pet Store API"
  version: "1.0.0"
x-kuadrant:
  route:
    name: "petstore"
    namespace: "petstore-ns"
    hostnames:
      - example.com
    parentRefs:
      - name: gw
        namespace: gw-ns
servers:
  - url: https://example.io/v1
paths:
  /cat:
    x-kuadrant:  ## Path level Kuadrant Extension
      backendRefs:
        - name: petstore
          port: 80
          namespace: petstore
    get:  # Added to the route and rate limited
      operationId: "getCat"
      responses:
        405:
          description: "invalid input"
  /dog:
    get:  # Added to the route and rate limited
      x-kuadrant:  ## Operation level Kuadrant Extension
        backendRefs:
          - name: petstore
            port: 80
            namespace: petstore
      operationId: "getDog"
      responses:
        405:
          description: "invalid input"
EOF
  • Compile kuadrantctl
make install
  • Generate HTTPRotue object
bin/kuadrantctl generate gatewayapi httproute --oas openapi.yaml

The result should be

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  creationTimestamp: null
  name: petstore
  namespace: petstore-ns
spec:
  hostnames:
  - example.com
  parentRefs:
  - name: gw
    namespace: gw-ns
  rules:
  - backendRefs:
    - name: petstore
      namespace: petstore
      port: 80
    matches:
    - method: GET
      path:
        type: Exact
        value: /v1/dog
  - backendRefs:
    - name: petstore
      namespace: petstore
      port: 80
    matches:
    - method: GET
      path:
        type: Exact
        value: /v1/cat
status:
  parents: null

@eguzki eguzki requested review from jasonmadigan and a team May 15, 2024 16:39
@eguzki eguzki marked this pull request as ready for review May 15, 2024 16:52
@eguzki eguzki force-pushed the openapi-kuadrant-extensions-at-root branch from 4def3f7 to fc93631 Compare May 15, 2024 16:56
@jasonmadigan
Copy link
Member

cheers for verification steps, looks good:

cat << EOF > openapi.yaml            git:(openapi-kuadrant-extensions-at-root|⚑3)
---
openapi: "3.0.3"
info:
  title: "Pet Store API"
  version: "1.0.0"
x-kuadrant:
  route:
    name: "petstore"
    namespace: "petstore-ns"
    hostnames:
      - example.com
    parentRefs:
      - name: gw
        namespace: gw-ns
servers:
  - url: https://example.io/v1
paths:
  /cat:
    x-kuadrant:  ## Path level Kuadrant Extension
      backendRefs:
        - name: petstore
          port: 80
          namespace: petstore
    get:  # Added to the route and rate limited
      operationId: "getCat"
      responses:
        405:
          description: "invalid input"
  /dog:
    get:  # Added to the route and rate limited
      x-kuadrant:  ## Operation level Kuadrant Extension
        backendRefs:
          - name: petstore
            port: 80
            namespace: petstore
      operationId: "getDog"
      responses:
        405:
          description: "invalid input"
EOF


➜  kuadrantctl git:(openapi-kuadrant-extensions-at-root -> upstream/openapi-kuadrant-extensions-at-root) ✗ cat openapi.yaml                  git:(openapi-kuadrant-extensions-at-root|…1⚑3
---
openapi: "3.0.3"
info:
  title: "Pet Store API"
  version: "1.0.0"
x-kuadrant:
  route:
    name: "petstore"
    namespace: "petstore-ns"
    hostnames:
      - example.com
    parentRefs:
      - name: gw
        namespace: gw-ns
servers:
  - url: https://example.io/v1
paths:
  /cat:
    x-kuadrant:  ## Path level Kuadrant Extension
      backendRefs:
        - name: petstore
          port: 80
          namespace: petstore
    get:  # Added to the route and rate limited
      operationId: "getCat"
      responses:
        405:
          description: "invalid input"
  /dog:
    get:  # Added to the route and rate limited
      x-kuadrant:  ## Operation level Kuadrant Extension
        backendRefs:
          - name: petstore
            port: 80
            namespace: petstore
      operationId: "getDog"
      responses:
        405:
          description: "invalid input"
➜  kuadrantctl git:(openapi-kuadrant-extensions-at-root -> upstream/openapi-kuadrant-extensions-at-root) ✗ make install                      git:(openapi-kuadrant-extensions-at-root|…1⚑3
go fmt ./...
go vet ./...
GOBIN=/Users/jmadigan/Work/kuadrantctl/bin go install

➜  kuadrantctl git:(openapi-kuadrant-extensions-at-root -> upstream/openapi-kuadrant-extensions-at-root) ✗ bin/kuadrantctl generate gatewayapi httproute --oas openapi.yaml

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  creationTimestamp: null
  name: petstore
  namespace: petstore-ns
spec:
  hostnames:
  - example.com
  parentRefs:
  - name: gw
    namespace: gw-ns
  rules:
  - backendRefs:
    - name: petstore
      namespace: petstore
      port: 80
    matches:
    - method: GET
      path:
        type: Exact
        value: /v1/cat
  - backendRefs:
    - name: petstore
      namespace: petstore
      port: 80
    matches:
    - method: GET
      path:
        type: Exact
        value: /v1/dog
status:
  parents: null

Copy link
Member

@jasonmadigan jasonmadigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on #77

Given the backwards compat change, we'll tag as 0.3.0?

@eguzki
Copy link
Collaborator Author

eguzki commented May 16, 2024

Given the backwards compat change, we'll tag as 0.3.0?

Following semantic versioning rules, we can't. But it is not ready yet for v1.0.0, is it?

I believe that v0.X.Y means alpha and braking changes can occur. So, semantic versioning should start on v1.X.Y. But whatever is fine with me

securitySchemes:
securedDog:
type: openIdConnect
openIdConnectUrl: https://example.com/.well-known/openid-configuration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's for the test data, but as an example the value used here can be dangerous. Please refer to #76 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be clear, you are suggesting openIdConnectUrl in this example to be something like https://example.com and authorino (using go.oidc lib) will append automatically /.well-known/openid-configuration right?

Then, we need to document betterl that openIdConnectUrl and what value are expected. Out of scope of this PR, btw.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like @azgabur will land a fix for this example in https://github.com/Kuadrant/kuadrantctl/pull/76/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been addressed in #76

@@ -1,22 +1,21 @@
## OpenAPI 3.0.X Kuadrant Extensions

### Info level kuadrant extension
### Root level kuadrant extension
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a very welcome change. I always found it weird, having it under the info field.
Well done!

Base automatically changed from general-enhancements to main June 4, 2024 11:17
@jasonmadigan
Copy link
Member

I believe that v0.X.Y means alpha and braking changes can occur. So, semantic versioning should start on v1.X.Y. But whatever is fine with me

Yeah, you're right - I'd forgotten that everything in v0 in semver is considered unstable. Patch version bump probably fine.

@eguzki eguzki force-pushed the openapi-kuadrant-extensions-at-root branch from fc93631 to 58539d6 Compare June 4, 2024 14:37
@eguzki eguzki merged commit 50dc3c4 into main Jun 4, 2024
5 checks passed
@eguzki eguzki deleted the openapi-kuadrant-extensions-at-root branch June 4, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants