Create Admin Partitions when enabled #696

thisisnotashwin · 2021-08-30T18:56:45Z

Changes proposed in this PR:

Create a partition when Admin Partitions are enabled.
The Kubernetes cluster that has servers (server cluster) will have clients installed in the default partition.
Workload clusters will have clients deployed in the partition whose name is provided in the values file.

How I've tested this PR:

Deployed the servers using this values file

global:
  tls:
    enabled: true
  image: ashwinvenkatesh/consul@sha256:08b61b39c2c45679f328ff618626540981261d37abb0e96246fa965f4c60e163
  imageK8S: ashwinvenkatesh/consul-k8s@sha256:fd9f407059514cc986369228f65d6ff3e127b9e37af0dd2513a494e12be3b5e3
  adminPartitions:
    enabled: true
server:
  exposeGossipAndRPCPorts: true
  enterpriseLicense:
    secretName: license
    secretKey: key
connectInject:
  enabled: true
controller:
  enabled: true
ui:
  service:
    type: NodePort
    nodePort:
      https: 30100

Things of note are this is a Consul Enterprise image and the license was provided as a secret. Additionally, an explicit nodePort has been assigned to the UI service. This is to expose Consul's https port.

Now, copy the TLS secrets and the Ent License to the workload-cluster.

global:
  enabled: false
  image: ashwinvenkatesh/consul@sha256:08b61b39c2c45679f328ff618626540981261d37abb0e96246fa965f4c60e163
  imageK8S: ashwinvenkatesh/consul-k8s@sha256:fd9f407059514cc986369228f65d6ff3e127b9e37af0dd2513a494e12be3b5e3
  adminPartitions:
    enabled: true
    name: "alpha"
  tls:
    enabled: true
    caCert:
      secretName: consul-consul-ca-cert
      secretKey: tls.crt
    caKey:
      secretName: consul-consul-ca-key
      secretKey: tls.key
server:
  enterpriseLicense:
    secretName: license
    secretKey: key
externalServers:
  enabled: true
  hosts: ["10.128.0.50"]
  httpsPort: 30100
  tlsServerName: server.dc1.consul
client:
  enabled: true
  exposeGossipPorts: true
  join:
  - "10.128.0.50"
  - "10.128.0.52"
  - "10.128.15.192"
connectInject:
  enabled: true
controller:
  enabled: true

The name of the partition here is "alpha". The ips specified under hosts and join are internal IPs for the nodes on GKE. the node IPs can be found using the kubectl get nodes -o wide command. Additionally, the firewalls rules on GKE needed to be updated to ensure that the node network and pod network from the server cluster and workload cluster could communicate all the nodes.

The clients logs in the server client state that they are in the default partition and the logs in the workload cluster state that they are in the alpha partition.
Validated the Connect works on the default cluster. Connect needs to make some changes in order for it to be supported on workload clusters and that is being worked on.

How I expect reviewers to test this PR:
Follow the above steps. DM me if you would like to pair on the above. Re-do the above steps and deploy "connect" apps in the default cluster.

Checklist:

Tests added
CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

- This removes any requirement on a post-install cleanup job.

lkysow

🎉 looking good

charts/consul/test/unit/partition-init-podsecuritypolicy.bats

charts/consul/values.yaml

control-plane/go.mod

control-plane/subcommand/partition-init/command.go

control-plane/subcommand/partition-init/command_ent_test.go

control-plane/subcommand/partition-init/command.go

lkysow · 2021-09-02T21:07:16Z

charts/consul/values.yaml

+    enabled: false
+    # The name of the Admin Partition. Must be "default" in the server cluster ie the Kubernetes cluster that
+    # the Consul server pods are deployed onto.
+    name: "default"


Do you do any validation that if servers are enabled that this == default?

Not right now. I wanted to do this in a follow up task as it is not a must have. Have a task for it: https://app.asana.com/0/1200449541311255/1200910791029388/f

control-plane/subcommand/common/common.go

control-plane/subcommand/partition-init/command.go

control-plane/subcommand/server-acl-init/command.go

ndhanushkodi

Looking awesome so far! I've just gotten through reviewing the Helm templates and some of the control-plane code and will continue tomorrow! Barely had comments because it's looking really great!

Other qs that popped up:
Do we explicitly require TLS to enable partitions or just highly recommend?

charts/consul/values.yaml

thisisnotashwin · 2021-09-09T14:18:00Z

Setting up external servers explicitly requires TLS. This is more a safety consideration as compared to Consul not working without it. So in a certain sense, we do not support non-TLS on k8s with externalServers. Are you thinking about adding that as an additional check?

ndhanushkodi

we do not support non-TLS on k8s with externalServers. Are you thinking about adding that as an additional check?
It could be worth the additional check, ok with that being a follow on task though!

Awesome work Ashwin!!!

ndhanushkodi · 2021-09-09T16:55:24Z

control-plane/subcommand/common/common.go

+
+// GetResolvedServerAddresses resolves the Consul server address if it has been provided a provider else it returns the server addresses that were input to it.
+func GetResolvedServerAddresses(serverAddresses []string, providers map[string]discover.Provider, logger hclog.Logger) ([]string, error) {
+	if len(serverAddresses) != 1 || !strings.Contains(serverAddresses[0], "provider=") {


I was a little confused why it checking len != 1 and why serverAddresses could have a string "provider=" in it until I saw the test-- Maybe the comment could describe what the serverAddresses argument and providers argument could look like?

Could there be a case where there is exactly one server address that's not a provider?

Sure thing :)
There could be a case where we have a single address which is not a provider. For instance, with the Partition service, the single address would be the address of the partition service LB. Ill improve the text here.

Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>

This is because these tests are quite similar except for the additional assertion in the health checks tests that asserts that traffic is not routed when static-server probe fails. This will save us quite a bit of run-time for both regular and *namespace tests because the bulk of the test time is taken up by the installs. This change essentially optimizes and re-uses existing installs that we already have to do to test connect.

* Create Admin Partitions when enabled

Create Admin Partitions when enabled

5344fe1

thisisnotashwin requested review from a team, lkysow and sadjamz and removed request for a team August 30, 2021 18:56

Migrate partition-init to pre-install hook.

854c25a

- This removes any requirement on a post-install cleanup job.

thisisnotashwin force-pushed the create-partition branch from 376d849 to 854c25a Compare August 30, 2021 21:26

Remove reference to ACLs as it cannot be tested presently

7d058e1

lkysow reviewed Sep 1, 2021

View reviewed changes

thisisnotashwin requested a review from lkysow September 2, 2021 20:35

lkysow approved these changes Sep 2, 2021

View reviewed changes

Address Code Review comments

3f95619

thisisnotashwin force-pushed the create-partition branch from 9a6854f to 3f95619 Compare September 2, 2021 22:00

thisisnotashwin requested review from ndhanushkodi and removed request for sadjamz September 8, 2021 16:44

ndhanushkodi reviewed Sep 9, 2021

View reviewed changes

charts/consul/values.yaml Outdated Show resolved Hide resolved

ndhanushkodi approved these changes Sep 9, 2021

View reviewed changes

Apply suggestions from code review

51c4292

Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>

thisisnotashwin force-pushed the create-partition branch from d2f80b1 to 51c4292 Compare September 9, 2021 18:13

thisisnotashwin merged commit 8d0bcf1 into partitions Sep 9, 2021

thisisnotashwin deleted the create-partition branch September 9, 2021 18:14

thisisnotashwin added a commit that referenced this pull request Sep 14, 2021

Create Admin Partitions when enabled (#696)

8872a54

* Create Admin Partitions when enabled

thisisnotashwin added a commit that referenced this pull request Sep 14, 2021

Create Admin Partitions when enabled (#696)

82b9e11

* Create Admin Partitions when enabled

thisisnotashwin added a commit that referenced this pull request Sep 16, 2021

Create Admin Partitions when enabled (#696)

5666658

* Create Admin Partitions when enabled

thisisnotashwin added a commit that referenced this pull request Sep 17, 2021

Create Admin Partitions when enabled (#696)

4e9cf3a

* Create Admin Partitions when enabled

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Admin Partitions when enabled #696

Create Admin Partitions when enabled #696

thisisnotashwin commented Aug 30, 2021

lkysow left a comment

lkysow Sep 2, 2021

thisisnotashwin Sep 2, 2021

ndhanushkodi left a comment

thisisnotashwin commented Sep 9, 2021

ndhanushkodi left a comment

ndhanushkodi Sep 9, 2021

thisisnotashwin Sep 9, 2021

Create Admin Partitions when enabled #696

Create Admin Partitions when enabled #696

Conversation

thisisnotashwin commented Aug 30, 2021

lkysow left a comment

Choose a reason for hiding this comment

lkysow Sep 2, 2021

Choose a reason for hiding this comment

thisisnotashwin Sep 2, 2021

Choose a reason for hiding this comment

ndhanushkodi left a comment

Choose a reason for hiding this comment

thisisnotashwin commented Sep 9, 2021

ndhanushkodi left a comment

Choose a reason for hiding this comment

ndhanushkodi Sep 9, 2021

Choose a reason for hiding this comment

thisisnotashwin Sep 9, 2021

Choose a reason for hiding this comment