-
Notifications
You must be signed in to change notification settings - Fork 321
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
GKE Autopilot support #2952
GKE Autopilot support #2952
Conversation
f8e1e6e
to
f3231ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to avoid having "custom" refer to both our own CRDs -- which are "custom" for Consul -- and some of the CRDs defined by the Kubernetes SIG that have not graduated to "standard" yet but most likely will in the future.
These are the groups as I see them
- "Standard" CRDs defined by Gateway API here (
Gateway
,HTTPRoute
, etc.) - "Experimental" CRDs defined by Gateway API here (
TCPRoute
in this case) - "Consul" CRDs defined by us (
GatewayClassConfig
,MeshService
)
Here are a couple of slightly different config structures that I think make more sense given that grouping. Let me know what you think.
manageExternalCRDs: false # Let GKE manage the "standard" CRDs
manageNonStandardCRDs: true # Install TCPRoute, GatewayClassConfig + MeshService
manageExternalCRDs: false # Let GKE manage the "standard" CRDs
# This is a higher bar for the config since this set *must* be installed right now; however, it
# avoids grouping things together and acknowledges that TCPRoute, for example, might one
# day be considered "standard". Our upgrade instructions would need to include
# - "remove TCPRoute if..."
# - "add SomeNewConsulCRD if..."
manageIndividualCRDs:
- TCPRoute
- GatewayClassConfig
- MeshService
charts/consul/values.yaml
Outdated
# Enables Consul on Kubernets to manage only the custom CRDs used for Gateway API. If manageExternalCRDs is true | ||
# then all CRDs will be installed. If manageCustomerCRDs is true then only TCPRoute, gatewayclassconfigs and meshservice | ||
# will be installed. | ||
manageCustomCRDs: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly, the combinations that matter are
# manageCustomCRDs doesn't matter here and could be true or false, all CRDs will be installed
manageExternalCRDs: true
# manageCustomCRDs: false
# This only installs TCPRoute, GatewayClassConfig and MeshService
manageExternalCRDs: false
manageCustomCRDs: true
# No CRDs at all will be installed
manageExternalCRDs: false
manageCustomCRDs: false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks correct.
I'm fairly certain @Jeff-Apple will have thoughts on the UX here |
I am fine with |
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
I'm ok with |
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: