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.

Sign up for GitHub

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

Jump to bottom

charts: add ReferenceGrant permissions to Consul API Gateway ClusterRole #1299

Merged
merged 2 commits into from
Aug 9, 2022
Merged

charts: add ReferenceGrant permissions to Consul API Gateway ClusterRole #1299

merged 2 commits into from
Aug 9, 2022

Conversation

mikemorris
Copy link
Contributor

@mikemorris mikemorris commented Jun 21, 2022 •
edited
Loading

Changes proposed in this PR:

How I've tested this PR:

  • Deployed Consul API Gateway from Helm chart from this branch and ran Gateway API conformance tests.
[INFO]  k8s/logger.go:30: consul-api-gateway-server.controller-runtime: Starting EventSource: controller=tcproute controllerGroup=gateway.networking.k8s.io controllerKind=TCPRoute info="Starting EventSource" source="kind source: *v1alpha2.ReferenceGrant"

How I expect reviewers to test this PR:

[ERROR] k8s/logger.go:35: consul-api-gateway-server.kubernetes-client: pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:167: Failed to watch *v1alpha2.ReferenceGrant: failed to list *v1alpha2.ReferenceGrant: referencegrants.gateway.networking.k8s.io is forbidden: User "system:serviceaccount:consul:consul-consul-api-gateway-controller" cannot list resource "referencegrants" in API group "gateway.networking.k8s.io" at the cluster scope

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 requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@nathancoleman nathancoleman left a comment

Choose a reason for hiding this comment

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

We'll definitely want a release note for this. If I'm not mistaken, anyone having apiGateway.enabled=true will have to update their gateway CRDs prior to upgrading to the release of consul-k8s containing this change.

@nathancoleman nathancoleman mentioned this pull request Jun 27, 2022
Merged
13 tasks
@mikemorris mikemorris force-pushed the charts/capigw-controller-clusterrole-referencegrants branch from 00920a4 to 4875b7d Compare June 29, 2022 21:00
nathancoleman
nathancoleman reviewed Jun 30, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -10,6 +10,7 @@ IMPROVEMENTS:
* Added annotations `consul.hashicorp.com/prometheus-ca-file`, `consul.hashicorp.com/prometheus-ca-path`, `consul.hashicorp.com/prometheus-cert-file`, and `consul.hashicorp.com/prometheus-key-file` for configuring TLS scraping on Prometheus metrics endpoints for Envoy sidecars. To enable, set the cert and key file annotations along with one of the ca file/path annotations. [[GH-1303](https://github.com/hashicorp/consul-k8s/pull/1303)]
* Helm
* Added `connectInject.annotations` and `syncCatalog.annotations` values for setting annotations on connect inject and sync catalog deployments. [[GH-775](https://github.com/hashicorp/consul-k8s/pull/775)]
* Added support for Consul API Gateway to read ReferenceGrant custom resources. This will require updating installed CRDs to include ReferenceGrant from the Gateway API v0.5 [Experimental Channel](https://gateway-api.sigs.k8s.io/concepts/versioning/#release-channels-eg-experimental-standard) if setting `apiGateway.enabled=true` [[GH-1299](https://github.com/hashicorp/consul-k8s/pull/1299)]
Copy link
Member

@nathancoleman nathancoleman Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should we so far as to include the command? If I wasn't a core dev on API Gateway, I feel like it'd be hard for me to determine from the release note here what exactly I need to do.

$ kubectl apply --kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.5.0-rc1"

Copy link
Contributor Author

@mikemorris mikemorris Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

🤔 I think not, because the preferred way to install would be to install our CRDs, which include the necessary upstream ones (and installing directly like ^ will skip installing the deprecated ReferencePolicy CRD by default). However, it may be weird to land this before we have a tagged release that installs upstream CRDs including ReferenceGrant?

kubectl apply --kustomize "github.com/hashicorp/consul-api-gateway/config/crd?ref=v0.4.0"

Copy link
Member

Choose a reason for hiding this comment

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

Our CRDs don't currently include the necessary ReferenceGrant CRD though and won't for a few weeks until we release API Gateway v0.4.0. There's a pretty good chance consul-k8s will cut a release before v0.4.0 exists. I'm mainly thinking of what consumers of the next version of consul-k8s will do in that interim period.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, it's possible we just shouldn't merge this until the release cycle directly before our v0.4.0 release? Not quite sure the best way to handle this or communicate in support matrices.

Copy link
Member

Choose a reason for hiding this comment

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

I was previously thinking it best to merge soon so we wouldn't have to ask for a special release of consul-k8s, but I agree with you. Releasing early seems like it will make the practitioner's life more difficult.

@mikemorris mikemorris force-pushed the charts/capigw-controller-clusterrole-referencegrants branch from 4875b7d to 7a4cc74 Compare August 1, 2022 17:38
nathancoleman
nathancoleman approved these changes Aug 9, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@mikemorris mikemorris force-pushed the charts/capigw-controller-clusterrole-referencegrants branch from 7a4cc74 to 383bbd4 Compare August 9, 2022 15:40
@mikemorris mikemorris force-pushed the charts/capigw-controller-clusterrole-referencegrants branch from 383bbd4 to d404ada Compare August 9, 2022 15:53
@mikemorris mikemorris merged commit 1abda49 into main Aug 9, 2022
@mikemorris mikemorris deleted the charts/capigw-controller-clusterrole-referencegrants branch August 9, 2022 16:11
@mikemorris mikemorris mentioned this pull request Aug 10, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishustava ishustava ishustava approved these changes

@nathancoleman nathancoleman nathancoleman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikemorris @nathancoleman @ishustava