PeeringAcceptor controller #1225

ndhanushkodi · 2022-05-17T06:33:54Z

Changes proposed in this PR:

PeeringAcceptor CR + controller + unit tests
was NOT planning to unit test getExistingSecret or createK8sPeeringTokenSecretWithOwner because their cases are covered in the ReconcileCreateUpdate test. LMK if you see gaps or prefer the helper function tests.
was NOT planning to unit test the generateToken, deletePeering, or createSecret helper functions as they are heavily used throughout other tests.

Future PRs (tasks are in Cluster Peering Asana):

PeeringDialer CR + controller
upstream changes for CRDs and upstream annotation
Acceptance test for service mesh
Version annotation (and storing that as peering metadata)
Adding Finalizer
Add peering state/status to the CR status

How I've tested this PR:
helm install nitya ../../consul-k8s/charts/consul -f consul-values.yml

consul-values.yml:

global:
  domain: consul
  datacenter: dc1
  imageK8S: ndhanushkodi/consul-k8s-dev:peering22
  image: ndhanushkodi/consul-dev:peering4
server:
  replicas: 1
  bootstrapExpect: 1
client:
  enabled: true
  grpc: true
ui:
  enabled: true
connectInject:
  enabled: true
controller:
  enabled: true

k apply -f peering-acceptor.yaml

apiVersion: consul.hashicorp.com/v1alpha1
kind: PeeringAcceptor
metadata:
  name: peeraaaaa
spec:
  peer:
    secret:
      name: "foo"
      key: "bar"
      backend: "kubernetes"

k delete -f peering-acceptor.yaml

Look at the logs and make sure it's as expected.

How I expect reviewers to test this PR:
Code review

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...)

thisisnotashwin

I like what im seeing so far

charts/consul/templates/connect-inject-clusterrole.yaml

control-plane/api/v1alpha1/peeringacceptor_types.go

control-plane/connect-inject/peering_acceptor_controller.go

ndhanushkodi · 2022-05-23T16:54:32Z

.github/workflows/test.yml

@@ -142,14 +142,22 @@ jobs:
      - run:  mkdir -p ${{env.TEST_RESULTS}}


The changes in this file would be removed when merging in the feature branch. This is because the most recently released version of consul does not have peering.

Will we have the right binaries by the time we merge? I'm thinking that maybe we should keep it on the feature branch as well until we have released consul binaries.

Yup agreed, we'd not want to cut a release with all this unless there's something for Consul also plus we'll want to address TODOs and unit tests (for peering dialer) on the feature branch peering before merging to main.

control-plane/connect-inject/peering_acceptor_controller.go

t-eckert · 2022-05-24T01:47:30Z

I don't feel I have much to add given my lack of context. I perused to really just gain a better understanding of what you are working on here. I think you've expressed it very clearly.

ishustava

Great work, Nitya! As usual, love all the comments and the thought you put into this ❤️

I'm leaving my comments so far (I got to reviewing most of the reconcile logic, except for some private functions and tests).

I will review some more today, but wanted to leave my comments so far. Some of them are very minor and I marked them as non-blockers. Others are more questions or thoughts about edge cases, so feel free to punt on those too if need be!

ishustava · 2022-05-23T22:16:09Z

.github/workflows/test.yml

@@ -142,14 +142,22 @@ jobs:
      - run:  mkdir -p ${{env.TEST_RESULTS}}


Will we have the right binaries by the time we merge? I'm thinking that maybe we should keep it on the feature branch as well until we have released consul binaries.

ishustava · 2022-05-23T22:19:24Z

charts/consul/templates/crd-peeringacceptors.yaml

@@ -0,0 +1,99 @@
+{{- if .Values.connectInject.enabled }}


Just thinking out loud. I think it makes sense to make this part of connect injector. I think at some point we wanted to make the CRD controller part of the injector deployment as well, so I think making this controller be part of connect inject makes sense.

Yeah that sounds good-- there is technically a service discovery case that can be enabled without service mesh, which could mean Consul DNS for imported services could work without enabling mesh functionality. Do you think it's worth enabling these controllers by default, not gated on any values?

yeah I think we will in the future, but probably not for this feature.

control-plane/api/v1alpha1/peeringacceptor_types.go

control-plane/config/crd/patches/cainjection_in_peeringacceptors.yaml

control-plane/config/crd/patches/webhook_in_peeringacceptors.yaml

control-plane/connect-inject/peering_acceptor_controller.go

ishustava · 2022-05-24T16:47:37Z

control-plane/connect-inject/peering_acceptor_controller.go

+
+	// Look up existing secret.
+	var existingSecret *corev1.Secret
+	if statusSecretSet {


could there be an edge case when the status is not set but secret exists?

Any time we create a secret, we do then set the status. However, if the secret was successfully created but the status didn't properly get updated, then the scenario you described could happen. However, that would return a reconcile error since updateStatus would return an error. Upon the next reconcile, statusSecretSet would be false, but the peering would exist in Consul, so it'll go into the case in L140 and try to generate a token again, so I think it'd be fine.

control-plane/connect-inject/peering_acceptor_controller.go

ishustava · 2022-05-24T17:58:23Z

control-plane/connect-inject/peering_acceptor_controller.go

+	// Compare the existing secret hash.
+	// Get the secret specified by the status, make sure it matches the status' secret.latestHash.
+	if existingSecret != nil {
+		existingSecretHashBytes := sha256.Sum256(existingSecret.Data[peeringAcceptor.Status.Secret.Key])


could we use secret's resourceVersion instead of computing a hash?

I really like this idea!!

Yup, great idea! update: still working on this

updated to use resource version! this was a great idea!

control-plane/connect-inject/peering_acceptor_controller.go

ishustava

Looks good! Leaving my remaining comments. I think this is good to go once we decide what to do about some questions I left.

control-plane/connect-inject/peering_acceptor_controller.go

ishustava · 2022-05-24T22:30:47Z

control-plane/connect-inject/peering_acceptor_controller.go

+}
+
+// updateStatusError updates the peeringAcceptor's ReconcileError in the status.
+func (r *PeeringAcceptorController) updateStatusError(ctx context.Context, peeringAcceptor *consulv1alpha1.PeeringAcceptor, reconcileErr error) error {


We always ignore the error that's returned by this function. I'm assuming we shouldn't, so maybe it could be a TODO?

I don't think there's anything we can do to handle the error from this function. If we fail to update the status with the error message, we don't want the status update failure to be what's returned from the reconcile, instead we want the real error. I suppose we could have this function just not return an error, and document that we are ignoring any errors in this case? The only error case is if the K8s api is down when trying to make an update to the status, and instead the real error would be returned as a reconcile error, wdyt?

control-plane/connect-inject/peering_acceptor_controller.go

ishustava · 2022-05-24T22:44:54Z

charts/consul/templates/crd-peeringacceptors.yaml

+    listKind: PeeringAcceptorList
+    plural: peeringacceptors
+    singular: peeringacceptor
+  scope: Namespaced


should it be a cluster resource?

this was something that i had suggested. This to me felt similar to our config entries of proxy_defaults and mesh ie it is ok for these to be namespaced because there are users that prefer to save all of their config in a single configuration namespace and this feels like something that falls into the same category. additionally, given it is responsible for creating and managing a secret, it is easy to assume that the secret needs to be created in the same namespace as the resource itself instead of us having to determine which namespace the secret has to be written to.

Ahh ok! I was thinking that you would only want one of these per cluster, and so that's why it might make sense from rbac perspective to make it cluster-wide. For secret, i think it's ok to manage a secret in some namespace (e.g. ClusterRoleBinding refers to a service account and service accounts are namespaced).

Yeah I also think it might be weird for PeeringAcceptors to be cluster wide and for their managed secrets to be namespaced, it might be nicer to always create the secret in whatever ns the PeeringAcceptor is in. And yup there can be multiple PeeringAcceptors in a single cluster

control-plane/config/rbac/peeringacceptor_editor_role.yaml

ndhanushkodi · 2022-05-26T07:58:08Z

Updates:

Hey @ishustava re-requesting review, while still addressing:

secret's resourceVersion instead of computing a hash (need to fix tests for this case, and make sure it all works)

@thisisnotashwin I'm still addressing:

using the object reference
refactor to have getters on the CRD fields

After updating control-plane/PROJECT L6 to reference the control plane folder, ran the command: operator-sdk create api --group consul --version v1alpha1 --kind Peering --controller --namespaced=true --make=false --resource=true

…reate case work

ndhanushkodi · 2022-05-27T05:53:05Z

Made a cleanup Asana to refactor to have getters on the CRD fields

Currently thinking to not using the object reference, and using the local type instead due to https://github.com/kubernetes/api/blob/be84346886a48ff00fc676f9cb37f10e79dfd0da/core/v1/types.go#L5624

thisisnotashwin

This looks good. i haven't tested the behavior on a cluster myself but this great!

Adds new PeeringAcceptor CRD and controller with unit tests. This controller determines whether to generate a new peering token in Consul by reconciling the PeeringAcceptor resource. It currently supports writing the peering token to a K8s secret. Generating the new CRD and API: After updating control-plane/PROJECT L6 to reference the control plane folder, ran the command: `operator-sdk create api --group consul --version v1alpha1 --kind Peering --controller --namespaced=true --make=false --resource=true`

ndhanushkodi changed the base branch from main to peering May 17, 2022 18:23

ndhanushkodi changed the title ~~Peering controllers~~ PeeringAcceptor controller May 17, 2022

thisisnotashwin reviewed May 17, 2022

View reviewed changes

ndhanushkodi force-pushed the initial-controller branch 3 times, most recently from dfe0cc6 to ba63d22 Compare May 20, 2022 04:53

ndhanushkodi marked this pull request as ready for review May 23, 2022 16:36

ndhanushkodi requested review from thisisnotashwin, a team, t-eckert and ishustava and removed request for a team May 23, 2022 16:41

ndhanushkodi commented May 23, 2022

View reviewed changes

t-eckert reviewed May 24, 2022

View reviewed changes

control-plane/connect-inject/peering_acceptor_controller.go Outdated Show resolved Hide resolved

t-eckert reviewed May 24, 2022

View reviewed changes

control-plane/connect-inject/peering_acceptor_controller.go Outdated Show resolved Hide resolved

ishustava reviewed May 24, 2022

View reviewed changes

ndhanushkodi commented May 25, 2022

View reviewed changes

control-plane/config/rbac/peeringacceptor_editor_role.yaml Outdated Show resolved Hide resolved

ndhanushkodi force-pushed the initial-controller branch from b47572b to a0faab9 Compare May 26, 2022 06:50

ndhanushkodi requested a review from ishustava May 26, 2022 07:55

ndhanushkodi added 8 commits May 26, 2022 22:34

generate new peering CRD

ea5ca7d

After updating control-plane/PROJECT L6 to reference the control plane folder, ran the command: operator-sdk create api --group consul --version v1alpha1 --kind Peering --controller --namespaced=true --make=false --resource=true

add fields to Peering CR, and run make ctrl-generate

dc8d3d9

add plumbing for controller

47477d9

fixup! add fields to Peering CR, and run make ctrl-generate

875d599

run make ctrl-manifests

aaaa8b3

add more plumbing to allow controller to work-- it now sees peerings

d62596a

use new consul api package

f4e98de

change spec to be closer to peering acceptor, make peering acceptor c…

a107442

…reate case work

ndhanushkodi added 17 commits May 26, 2022 22:39

gh actions consul ent

123a6aa

gh actions upload binary to gh

6cde267

gh action

11467eb

gh actions use latest consul binary

27ee4d2

streamline controller cases

cb56930

add delete perms for secrets

7d77ffc

add shouldGenerateToken test

b118069

add all remaining test cases

a0286e2

add more comments

f90c006

remove unused field and run make ctrl-generate and ctrl-manifests

6f57525

address review comments, add docs for exported fields in CR

27a74a1

run make ctrl-generate and make ctrl-manifests

7cc8da6

delete some unnecessary files

d287a77

address more review comments

6e3ce5f

WIP change shasum to resource version

e128b0d

use resource version instead of shasum

966aca1

renames and cleanup

99fa4b6

ndhanushkodi force-pushed the initial-controller branch from 12ec371 to 99fa4b6 Compare May 27, 2022 05:42

ndhanushkodi added 2 commits May 26, 2022 22:43

cleanup

f184d2f

rename secretstatus to secretrefstatus

db42916

thisisnotashwin approved these changes May 27, 2022

View reviewed changes

ndhanushkodi merged commit 590ce14 into peering May 27, 2022

ndhanushkodi deleted the initial-controller branch May 27, 2022 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PeeringAcceptor controller #1225

PeeringAcceptor controller #1225

ndhanushkodi commented May 17, 2022 •

edited

Loading

thisisnotashwin left a comment

ndhanushkodi May 23, 2022

ishustava May 23, 2022

ndhanushkodi May 25, 2022

t-eckert commented May 24, 2022

ishustava left a comment

ishustava May 23, 2022

ishustava May 23, 2022

ndhanushkodi May 25, 2022

ishustava May 31, 2022

ishustava May 24, 2022

ndhanushkodi May 26, 2022

ishustava May 24, 2022 •

edited

Loading

thisisnotashwin May 25, 2022

ndhanushkodi May 25, 2022 •

edited

Loading

ndhanushkodi May 27, 2022

ishustava left a comment

ishustava May 24, 2022

ndhanushkodi May 26, 2022

ishustava May 24, 2022

thisisnotashwin May 25, 2022

ishustava May 25, 2022

ndhanushkodi May 25, 2022

ndhanushkodi commented May 26, 2022

ndhanushkodi commented May 27, 2022

thisisnotashwin left a comment

		@@ -142,14 +142,22 @@ jobs:
		- run: mkdir -p ${{env.TEST_RESULTS}}

PeeringAcceptor controller #1225

PeeringAcceptor controller #1225

Conversation

ndhanushkodi commented May 17, 2022 • edited Loading

thisisnotashwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-eckert commented May 24, 2022

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava May 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndhanushkodi commented May 26, 2022

ndhanushkodi commented May 27, 2022

thisisnotashwin left a comment

Choose a reason for hiding this comment

ndhanushkodi commented May 17, 2022 •

edited

Loading

ishustava May 24, 2022 •

edited

Loading

ndhanushkodi May 25, 2022 •

edited

Loading