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

docs: update multiple-ns guide for v1alpha2 #807

Conversation

shaneutt
Copy link
Member

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This updates the multiple-ns.md guide for v1alpha2.

Which issue(s) this PR fixes:

Resolves the 3rd task for #783

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 23, 2021
@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from fb71855 to c365248 Compare August 23, 2021 14:17
@shaneutt shaneutt marked this pull request as ready for review August 23, 2021 14:17
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2021
@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch 3 times, most recently from b18e2eb to b86de6e Compare August 23, 2021 15:45
the least permissive choice which ensures that other Gateways in the cluster
(perhaps created in the future at some point) will not bind with these Routes.
If cluster administrators have full control over how Gateways are deployed in a
cluster then a more permissive binding option could be configured on Routes. The
Copy link
Contributor

Choose a reason for hiding this comment

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

@robscott Are we using "Route Binding" or "Route attachment" from a terminology perspective? If latter, please change within the doc for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

I think we've historically used the terms interchangeably, but especially with the newer v1alpha2 approach, I personally prefer "attachment"

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like we're getting a lot of this mixed around, for instance the site-src/v1alpha2/concepts/api-overview.md recently changed the nomenclature of the section previously known as Route Binding to Attaching Routes to Gateways but then inside of that section continues to use the word binding (interchangeably, as rob suggested).

For the purposes of updating this specific guide, I've removed the binding language and just use "attachment" for consistency with the Attaching Routes to Gateways section in the concepts docs:

48b31b7

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still working on the API overview, but I think we should standardise on "attach" as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

this guide now uses "route attachment"

@@ -63,90 +64,71 @@ administration to ensure that Routes are not over-exposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Route-owners can specify that they will bind with all Gateways in the cluster,
or only Gateways from a specific Namespace, with a specific label selector, or
an individual Gateway.

This is outdated. Route owners now target specific gateways only.

Similarly, Gateways provide the same level of control.

Please expand on this. Gateways trust namespaces where Routes (which bind to it) can live.

Copy link
Member Author

Choose a reason for hiding this comment

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

49cd40e was created to clean up the wording, and expand the wording as requested.

@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from b86de6e to 0ab36db Compare August 23, 2021 17:19
@shaneutt shaneutt requested a review from hbagdi August 23, 2021 17:46
@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from 1d12e51 to 49cd40e Compare August 23, 2021 17:46
@shaneutt shaneutt requested a review from robscott August 23, 2021 18:08
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @shaneutt!

examples/v1alpha2/cross-namespace-routing/gateway.yaml Outdated Show resolved Hide resolved
examples/v1alpha2/cross-namespace-routing/site-route.yaml Outdated Show resolved Hide resolved
examples/v1alpha2/cross-namespace-routing/site-route.yaml Outdated Show resolved Hide resolved
examples/v1alpha2/cross-namespace-routing/store-route.yaml Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/multiple-ns.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/multiple-ns.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/multiple-ns.md Show resolved Hide resolved
site-src/v1alpha2/guides/multiple-ns.md Show resolved Hide resolved
Comment on lines 55 to 62
As a result, Gateways and Routes have independent control to determine which
resources they permit binding with. It is a handshake between the infra owners
and the application owners that allows them to be independent actors.
Route-owners can specify that they will bind with all Gateways in the cluster,
or only Gateways from a specific Namespace, with a specific label selector, or
an individual Gateway. Similarly, Gateways provide the same level of control.
This allows a cluster to be more self-governed, which requires less central
administration to ensure that Routes are not over-exposed.
resources they permit attaching to. It is a handshake between the infra owners
and the application owners that allows them to be independent actors. Routes
can only attach to specified gateways and Gateways provide a similar level of
selection control over which Routes they will send traffic to by trusting
specific namespaces in which those Routes live and operate. This allows a
cluster to be more self-governed, which requires less central administration
to ensure that Routes are not over-exposed.
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be updated more significantly. It all seems a bit too close to the v1alpha1 docs with the mentions of selection. In the new model, Routes directly reference Gateways they want to attach to, and Gateways describe where they trust Routes to attach from. These docs got lost in the recent restructuring, but might be a helpful reference point for rewriting this: https://deploy-preview-762--kubernetes-sigs-gateway-api.netlify.app/v1alpha2/concepts/api-overview/#how-it-works.

Copy link
Member Author

@shaneutt shaneutt Aug 23, 2021

Choose a reason for hiding this comment

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

2ad44ca was an attempt to significantly reword this area of the guide based on feedback, let me know your thoughts and if you feel there's more that needs to be done here.

I made changes such as:

  • using specific label selectors for namespaces, instead of "All"
  • splitting the previous "deployment" section into two sections "multiple ns for gateways" and "attaching routes"

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 what you have is looking pretty good, but I'd probably change this to reference that Gateways allow attachment, and Routes request attachment.

So, this section should be something like (sorry Github won't let me suggest here for some reason):

As a result, Gateways and Routes have independent control to determine which
resources will succeed in attaching. It is a handshake between the infra owners
and the application owners that allows them to be independent actors. Routes
can only attach to specified gateways and Gateways provide a similar level of
selection control over which Routes they allow to attach to by trusting
specific namespaces in which those Routes live and operate. This allows a
cluster to be more self-governed, which requires less central administration
to ensure that Routes are not over-exposed.

I also think we need to be really careful with "forward traffic to" because that implies the existence of an extra routing hop, which won't be the case with most implementations.

I think that for the attachment process, we need to make sure we're talking purely in terms of the Kubernetes objects and how they interact.

Copy link
Member Author

Choose a reason for hiding this comment

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

I made the changes you requested in e5488f6 and verified that the word "forwarding" is being used in the technical sense and is not misrepresented elsewhere in the doc. Let me know if there's anything further you want to see changed.

@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from 1885e38 to 8908617 Compare August 23, 2021 18:49
@shaneutt shaneutt requested a review from robscott August 23, 2021 18:51
@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch 2 times, most recently from d2ff120 to 2ad44ca Compare August 23, 2021 19:03
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

Sorry, I'm being really nitpicky about language I've been inconsistent with too. But I think that the point that we need to have consistent language is one hundred percent fair, and I think that should extend to how we talk about the Route - Gateway attachment process.

Comment on lines 55 to 62
As a result, Gateways and Routes have independent control to determine which
resources they permit binding with. It is a handshake between the infra owners
and the application owners that allows them to be independent actors.
Route-owners can specify that they will bind with all Gateways in the cluster,
or only Gateways from a specific Namespace, with a specific label selector, or
an individual Gateway. Similarly, Gateways provide the same level of control.
This allows a cluster to be more self-governed, which requires less central
administration to ensure that Routes are not over-exposed.
resources they permit attaching to. It is a handshake between the infra owners
and the application owners that allows them to be independent actors. Routes
can only attach to specified gateways and Gateways provide a similar level of
selection control over which Routes they will send traffic to by trusting
specific namespaces in which those Routes live and operate. This allows a
cluster to be more self-governed, which requires less central administration
to ensure that Routes are not over-exposed.
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 what you have is looking pretty good, but I'd probably change this to reference that Gateways allow attachment, and Routes request attachment.

So, this section should be something like (sorry Github won't let me suggest here for some reason):

As a result, Gateways and Routes have independent control to determine which
resources will succeed in attaching. It is a handshake between the infra owners
and the application owners that allows them to be independent actors. Routes
can only attach to specified gateways and Gateways provide a similar level of
selection control over which Routes they allow to attach to by trusting
specific namespaces in which those Routes live and operate. This allows a
cluster to be more self-governed, which requires less central administration
to ensure that Routes are not over-exposed.

I also think we need to be really careful with "forward traffic to" because that implies the existence of an extra routing hop, which won't be the case with most implementations.

I think that for the attachment process, we need to make sure we're talking purely in terms of the Kubernetes objects and how they interact.

site-src/v1alpha2/guides/multiple-ns.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/multiple-ns.md Outdated Show resolved Hide resolved
@shaneutt
Copy link
Member Author

As a note please don't merge thus until we hear back from @mark-church, as he and I were talking in Slack and I've given him write access to my fork: he has some updates he wants to add to this guide as well as a part of this PR.

@hbagdi
Copy link
Contributor

hbagdi commented Aug 24, 2021

As a note please don't merge thus until we hear back from @mark-church, as he and I were talking in Slack and I've given him write access to my fork: he has some updates he wants to add to this guide as well as a part of this PR.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2021
@shaneutt
Copy link
Member Author

shaneutt commented Sep 7, 2021

Just an update: Talked with Mark, we've still got some inbound commits for this PR he's working on 👍 still on hold for a bit longer

@mark-church
Copy link
Contributor

okay finally got my edits in! @shaneutt I accidentally created a PR in your repo, which you can ignore, while trying to figure out how to push to your PR.

@mark-church mark-church force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch 2 times, most recently from 25510e0 to 6801ae3 Compare September 8, 2021 13:36
@mark-church
Copy link
Contributor

Seeing following error in the tests but I am unsure why:

error: error validating "examples/v1alpha2/cross-namespace-routing/gateway.yaml": error validating data: ValidationError(Gateway.spec.listeners[0]): unknown field "routes" in io.k8s.networking.gateway.v1alpha2.Gateway.spec.listeners; if you choose to ignore these errors, turn validation off with --validate=false

@shaneutt
Copy link
Member Author

shaneutt commented Sep 8, 2021

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2021
@robscott
Copy link
Member

robscott commented Sep 8, 2021

@mark-church listener.routes has been renamed to listener.allowedRoutes based on feedback from the sig-network review.

@mark-church mark-church force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from 6801ae3 to 7a7afb2 Compare September 8, 2021 16:01
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks for the work on this @shaneutt and @mark-church! Have a few nits but otherwise LGTM.

access and fault domains.

Gateways and Routes can be deployed into different Namespaces and Routes
attached to Gateways across Namespace boundaries. This allows differing user access and
Copy link
Member

Choose a reason for hiding this comment

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

Not sure on this one, but maybe "different", feel free to ignore.

Suggested change
attached to Gateways across Namespace boundaries. This allows differing user access and
attached to Gateways across Namespace boundaries. This allows different user access and

Comment on lines 14 to 15
is explored in this guide and demonstrates how independent teams can safely
share the same Gateway.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
is explored in this guide and demonstrates how independent teams can safely
share the same Gateway.
is explored in this guide and demonstrates how independent teams can safely
share the same Gateway.

This Gateway also configures HTTPS using the `foo-example-com` Secret
in the `infra-ns` Namespace. This allows the infrastructure team to centrally
manage TLS on behalf of app owners. The `foo-example-com` certificate will
terminate all traffic going to all its attached Routes, without any TLS configuration
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit, but maybe the second "and" can be removed, feel free to ignore.

Suggested change
terminate all traffic going to all its attached Routes, without any TLS configuration
terminate all traffic going to its attached Routes, without any TLS configuration

namespaces:
from: "All"
Copy link
Member

Choose a reason for hiding this comment

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

I think you'd still need a line like this, just from: "SameNamespace"

from: "All"
selector:
matchLabels:
shared-gateway-access: "true"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shared-gateway-access: "true"
shared-gateway-access: "true"

Comment on lines 59 to 62
application owners that enables them to be independent actors. Routes
can only attach to specified Gateways and Gateways can constrain the Namespaces
and type of Routes which are able to attach to it. Clusters which use attachment
constraints to govern Route and Gateway attachment require less central administration
Copy link
Member

Choose a reason for hiding this comment

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

I found this section a bit confusing, but I'm not sure my suggestion here is better, feel free to ignore.

Suggested change
application owners that enables them to be independent actors. Routes
can only attach to specified Gateways and Gateways can constrain the Namespaces
and type of Routes which are able to attach to it. Clusters which use attachment
constraints to govern Route and Gateway attachment require less central administration
application owners that enables them to be independent actors. App owners directly reference
the Gateway(s) they want their Routes to be attached to. Infra owners can constrain the
namespaces and types of Routes that can be attached to their Gateway(s). Clusters which use
these attachment constraints to govern Route and Gateway attachment require less central
administration

precedence](/references/spec/#networking.x-k8s.io/v1alpha1.HTTPRouteRule)
between the flat list of routing rules is determined by most specific match and
After these three Routes are deployed, they will all be attached to the
`shared-gateway` Gateway. The Gateway merges its bound attached into a single flat
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this was the intent?

Suggested change
`shared-gateway` Gateway. The Gateway merges its bound attached into a single flat
`shared-gateway` Gateway. The Gateway merges its attached Routes into a single flat

owner both agree to the relationship. This bi-directional relationship exists
for two reasons:
[Route attachment][attaching] is an important concept that dictates how Routes
and Gateways select each other to apply routing configuration to a Gateway. It
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to remove the idea of selecting each other since that's not really what happens now.

Suggested change
and Gateways select each other to apply routing configuration to a Gateway. It
are attached to Gateways. It

Comment on lines 44 to 45
is especially relevant when there are multiple Gateways and multiple Namespaces
in a cluster. Gateway and Route attachment is bidirectional - attachment can
Copy link
Member

@robscott robscott Sep 9, 2021

Choose a reason for hiding this comment

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

Suggested change
is especially relevant when there are multiple Gateways and multiple Namespaces
in a cluster. Gateway and Route attachment is bidirectional - attachment can
is especially relevant when there are Routes in multiple namespaces that need to be
attached to a shared Gateway. Gateway and Route attachment is bidirectional - attachment can

kind: Namespace
metadata:
name: no-external-access

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Looks like an extra newline here.

@mark-church mark-church force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch 3 times, most recently from 35132c5 to 958cf60 Compare September 10, 2021 17:20
@shaneutt shaneutt force-pushed the shaneutt/update-multiple-ns-guide-for-v1alpha2 branch from 958cf60 to 3900382 Compare September 10, 2021 17:51
@robscott
Copy link
Member

Thanks for the work on this @shaneutt and @mark-church!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, shaneutt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit d540dee into kubernetes-sigs:master Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants