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

Allow API Gateway controller to get Namespaces #1092

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Mar 10, 2022

Changes proposed in this PR:
Allow the Consul API Gateway controller to get, list and watch namespaces. This enables the controller to validate HTTPRoutes before attaching them to a Gateway that uses a namespace selector for allowedRoutes.

In this example, the Gateway only allows routes from a K8s Namespace with labels matching amAllowed: sure:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: example-gateway
  namespace: consul
spec:
  gatewayClassName: consul-api-gateway
  listeners:
    - allowedRoutes:
        namespaces:
          from: Selector
          selector:
            matchLabels:
              amAllowed: sure
      name: http
      port: 80
      protocol: HTTP

Before the controller attaches a route to this gateway, it needs to retrieve the namespace and see if the label selector matches, thus these new permissions.

How I've tested this PR:
Tested as part of analogous PR, hashicorp/consul-api-gateway#121.

How I expect reviewers to test this PR:
Follow testing described in hashicorp/consul-api-gateway#121.

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@nathancoleman nathancoleman added the area/chart-only Related to changes that simply require yaml Helm chart changes, e.g. exposing a new field label Mar 10, 2022
Copy link
Contributor

@mikemorris mikemorris left a comment

Choose a reason for hiding this comment

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

LGTM from the API Gateway side, not sure if there's a test we should add to cover this.

@nathancoleman nathancoleman requested review from hc-github-team-consul-ecosystem and a team and removed request for hc-github-team-consul-ecosystem and a team March 10, 2022 18:37
@nathancoleman nathancoleman marked this pull request as ready for review March 10, 2022 18:39
@ishustava ishustava requested review from a team, ndhanushkodi and kschoche and removed request for hc-github-team-consul-ecosystem and a team March 10, 2022 18:42
@kschoche
Copy link
Contributor

kschoche commented Mar 10, 2022

LGTM from the API Gateway side, not sure if there's a test we should add to cover this.

@mikemorris Agree! Since nothing is being added that is configurable/templated I think its fine to use the existing bats test which asserts that the clusterrole gets added when apigw is enabled.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

👍🏽

Co-authored-by: Kyle Schochenmaier <[email protected]>
@nathancoleman nathancoleman merged commit abe9b40 into main Mar 10, 2022
@nathancoleman nathancoleman deleted the apigw-namespaces-rbac branch March 10, 2022 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/chart-only Related to changes that simply require yaml Helm chart changes, e.g. exposing a new field
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants