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

GKE Autopilot support #2952

Merged
merged 6 commits into from
Sep 15, 2023
Merged

GKE Autopilot support #2952

merged 6 commits into from
Sep 15, 2023

Conversation

curtbushko
Copy link
Contributor

@curtbushko curtbushko commented Sep 13, 2023

Changes proposed in this PR:

  • This adds a first pass of support for running on GKE Autopilot.
  • We won't know how things fully work until we are certified with 1.2.2 (next patch release)
  • I tried to do this without any helm chart values changes but it would have been considerably more messy than it is now.

How I've tested this PR:

  • went through an initial acceptance pass with the Autopilot team
  • installed consul-k8s using the following helm values:
global:
  name: consul
  datacenter: dc1
  logLevel: "debug"
  tls:
    enabled: false
ingress:
  enabled: false
server:
  replicas: 1
  resources:
    requests:
      memory: "200Mi"
      cpu: "500m"
    limits:
      memory: "200Mi"
      cpu: "500m"
connectInject:
  enabled: true
  default: true
  replicas: 1
  transparentProxy:
    defaultEnabled: true
  apiGateway:
    manageExternalCRDs: false
    manageCustomCRDs: true
dns:
  enabled: false
ui:
  enabled: true
  service:
    enabled: true
controller:
  enabled: true

How I expect reviewers to test this PR:

Checklist:

@curtbushko curtbushko force-pushed the curtbushko/gke-autopilot-acceptance branch from f8e1e6e to f3231ce Compare September 13, 2023 14:58
@curtbushko curtbushko self-assigned this Sep 13, 2023
@curtbushko curtbushko added the backport/1.2.x This release branch is no longer active. label Sep 13, 2023
@curtbushko curtbushko marked this pull request as ready for review September 13, 2023 15:17
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.

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

acceptance/framework/config/config.go Outdated Show resolved Hide resolved
charts/consul/values.yaml Outdated Show resolved Hide resolved
# 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
Copy link
Member

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 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks correct.

@nathancoleman
Copy link
Member

I'm fairly certain @Jeff-Apple will have thoughts on the UX here

@curtbushko
Copy link
Contributor Author

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](https://github.com/kubernetes-sigs/gateway-api/tree/main/config/crd/standard) (`Gateway`, `HTTPRoute`, etc.)

* "Extended" CRDs defined by Gateway API [here](https://github.com/kubernetes-sigs/gateway-api/tree/main/config/crd/experimental) (`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 include "remove TCPRoute if..."
manageIndividualCRDs:
- TCPRoute
- GatewayClassConfig
- MeshService

I am fine with manageNonStandardCRD, but not the list. I do not want to get into that game for something that you will probably change in the future...

curtbushko and others added 2 commits September 13, 2023 14:55
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
@david-yu
Copy link
Contributor

I'm ok with manageNonStandardCRD as well seems to be appropriate.

charts/consul/values.yaml Outdated Show resolved Hide resolved
Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>
@curtbushko curtbushko enabled auto-merge (squash) September 15, 2023 14:01
@curtbushko curtbushko merged commit ca870a1 into main Sep 15, 2023
21 of 31 checks passed
@curtbushko curtbushko deleted the curtbushko/gke-autopilot-acceptance branch September 15, 2023 14:29
curtbushko added a commit that referenced this pull request Sep 15, 2023
* GKE Autopilot support
@missylbytes missylbytes added backport/1.4.x and removed backport/1.2.x This release branch is no longer active. backport/1.4.x labels Jul 29, 2024
@missylbytes missylbytes added the backport/1.2.x This release branch is no longer active. label Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2.x This release branch is no longer active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants