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

Gateway API: Recommend the config approach rather than extraArgs #1517

Merged
merged 3 commits into from
Aug 8, 2024

Conversation

maelvls
Copy link
Member

@maelvls maelvls commented Jul 16, 2024

The config approach is much more convenient than to have to concatenate flags in the extraFlags field. The config approach isn't immediately obvious to users, as seen in cert-manager/cert-manager#7121 where we found that there was no need for adding an extra helm value: one could simply use config.enableGatewayAPI.

@cert-manager-prow cert-manager-prow bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 16, 2024
@maelvls
Copy link
Member Author

maelvls commented Jul 16, 2024

@wallrj Can you take a look? Thanks!

@maelvls maelvls requested a review from wallrj July 16, 2024 12:34
Copy link

netlify bot commented Jul 16, 2024

Deploy Preview for cert-manager ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ff1e41a
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager/deploys/66b4899ac4bfdb00082d8699
😎 Deploy Preview https://deploy-preview-1517--cert-manager.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @maelvls

This looks like a nicer way to configure that feature, but it won't work until the next cert-manager release, right?
It depends on cert-manager/cert-manager@1b9c02e which has not been backported or released yet.

And since this documentation relates to the next version of cert-manager, it should be merged into release-next, along with the release note for the change which makes this possible.

And the DCO check seems to be failing.

@hongbo-miao
Copy link

hongbo-miao commented Aug 3, 2024

Thanks @maelvls @wallrj for the info. It would be great to document the difference. Right now I noticed there are 3 methods in Helm chart:

featureGates: ExperimentalGatewayAPISupport=true
extraArgs:
  - --feature-gates=ExperimentalGatewayAPISupport=true
  1. This method current lists in Helm values.yaml
config:
  featureGates:
    ExperimentalGatewayAPISupport: true

@maelvls
Copy link
Member Author

maelvls commented Aug 5, 2024

I'll clarify further why and what the recommended approaches are depending on the cert-manager version. Thanks for the feedback!

@maelvls
Copy link
Member Author

maelvls commented Aug 6, 2024

Here would be my recommendation depending on whether you are using 1.14 or below, 1.15, or 1.16 and above:

If you are using 1.15 and above, you no longer have to set a feature flag, but you still need to enable the feature. The recommended way to turn on the Gateway API support in 1.15 is to use the file-based configuration using the config Helm value:

config:
  apiVersion: controller.config.cert-manager.io/v1alpha1
  kind: ControllerConfiguration
  enableGatewayAPI: true

If you are using 1.16 and above, it is even simpler:

config:
  enableGatewayAPI: true

Another less practical way is to rely on the command line flag --enable-gateway-api:

extraArgs:
  - --enable-gateway-api

If you are using 1.14 and below, you will have to use the feature flag to turn on the Gateway API support. The recommended way is to use the featureGates Helm value:

Another way is to use the config Helm value:

config:
  apiVersion: controller.config.cert-manager.io/v1alpha1
  kind: ControllerConfiguration
  featureGates:
    ExperimentalGatewayAPISupport: true

A less readable way (as it requires a comma-separated list of feature flags) is to use the featureGates Helm value:

featureGates: ExperimentalGatewayAPISupport=true

which is equivalent to:

extraArgs:
  - --feature-gates=ExperimentalGatewayAPISupport=true

I'm not sure how to put that into the documentation. Should I only talk about the latest version, or can I explain how to set up Gateway API depending on the version? @wallrj

@cert-manager-prow cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Aug 6, 2024
@maelvls
Copy link
Member Author

maelvls commented Aug 6, 2024

Discussed in this morning's open standup:

  • I will only keep the 1.15 instructions, and only show the config approach since it's the most sensible one (we think).
  • I will add a note that explains that the feature flag that existed in 1.14 no longer exists and was replaced with a configuration option enableGatewayAPI.

@cert-manager-prow cert-manager-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 6, 2024
The "config" approach is much more convenient than
to have to concatenate flags in the extraFlags field.

Signed-off-by: Maël Valais <[email protected]>
@maelvls maelvls requested a review from wallrj August 6, 2024 13:12
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

I spotted a minor typo which you can ignore if you like, by unholding.
I didn't test this, but I expect you have.

/approve
/lgtm
/hold

content/docs/usage/gateway.md Outdated Show resolved Hide resolved
@cert-manager-prow cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 6, 2024
Co-authored-by: Richard Wall <[email protected]>
Signed-off-by: Maël Valais <[email protected]>
@cert-manager-prow cert-manager-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2024
content/docs/usage/gateway.md Outdated Show resolved Hide resolved
Signed-off-by: Maël Valais <[email protected]>
@maelvls
Copy link
Member Author

maelvls commented Aug 8, 2024

I didn't test this, but I expect you have.

I had forgotten to test it... and found multiple typos in the commands. 🤦

I fixed them, it should be good to go now.

@maelvls
Copy link
Member Author

maelvls commented Aug 8, 2024

/unhold

@cert-manager-prow cert-manager-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2024
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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

@cert-manager-prow cert-manager-prow bot merged commit 6ceceee into cert-manager:master Aug 8, 2024
7 checks passed
@maelvls maelvls deleted the gateway-api-helm branch October 10, 2024 05:44
@maelvls
Copy link
Member Author

maelvls commented Oct 10, 2024

Now that cert-manager 1.16 has been released, --set config.enableGatewayAPI=true is now the recommended approach for projects that show instructions on how to enable cert-manager's gateway API support, especially on visible projects like Cilium: https://docs.cilium.io/en/latest/network/servicemesh/tls-termination/

I opened #1586 to track this issue.

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. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants