Send Application Sync request to Argo CD Server #44

DiogoMCampos · 2021-03-16T09:09:07Z

Issue link: #22

Opted to use the code behind the CLI, authenticating against the server based on secrets read from a local file. For the initial implementation, we opted to use Insecure.

What this does

Reads configuration from two files.
Creates a client to the argocd server.
Sends a sync request for every app that's scheduled to do so.
Sets up secrets to run controller locally as part of the set up scripts.

What this doesn't do

Doesn't handle certificates to connect securely to the argocd server;
Doesn't do anything on sync failure;

There's a couple of things missing that I intend to work on still, namely understanding what I need to change to the definitions/Makefile to use and mount the secrets as files.

DiogoMCampos · 2021-03-19T16:36:18Z

controllers/progressiverollout_controller.go

+
+			if err != nil {
+				if strings.Contains(err.Error(), "another operation is already in progress") {
+					log.Info("another operation is already in progress", "app", s)


I assume this means we proceed as normal - it's not an error per se - so I don't return here.

I'm not really even sure we want to log this to be honest. This mean we tried to re sync the app while a sync was already in progress, correct? So what's the big deal? :)
I might miss a valid use case.

DiogoMCampos · 2021-03-19T16:36:44Z

controllers/progressiverollout_controller.go

+					log.Info("another operation is already in progress", "app", s)
+				} else {
+					log.Error(err, "unable to sync app", "app", s)
+					return ctrl.Result{}, err


Is this all we want to do? Do we want to somehow signal the stage as failed? Or is that future work?

Do we want to somehow signal the stage as failed

For future work in #30 - as we don't do it at the moment

What @maruina said... happy path only :-)

controllers/progressiverollout_controller_test.go

DiogoMCampos · 2021-03-19T16:38:58Z

hack/dev-functions.sh

@@ -87,6 +87,13 @@ function local_argocd_login() {
 		echo "ARGOCD_LOCAL_ADDRESS=https://localhost:$argoserver_nodeport"
 	} >.env.local

+	if [ ! -d .prcconfig ]; then


This sets up a config folder locally so we can make run and use it to connect to the argocd-server in kind.

DiogoMCampos · 2021-03-19T16:40:23Z

controllers/progressiverollout_controller.go

 			r.Log.Info("syncing app", "app", s)
+
+			_, err := r.syncApp(s.Name)


The left-hand side variable is the synced *argov1alpha.Application, when there's no error. Is there anything we want to do with it?

I guess it's to have a lambda style approach where you can do things like r.syncApps(s.Name).Status() or something like this in a single command.
Don't think we need to do something in particular right now.

We're actually doing something funny with that one. the sync from Argo takes an ApplicationRequest that we get from the Name. I guess the left hand side there is meant to tell you whether the selector worked. It could be useful down the line for us.

controllers/progressiverollout_controller.go

maruina

Just a couple of comment to answer some of your questions, I'll do a deeper review later.

controllers/progressiverollout_controller.go

maruina · 2021-03-19T17:16:08Z

controllers/progressiverollout_controller.go

+
+			if err != nil {
+				if strings.Contains(err.Error(), "another operation is already in progress") {
+					log.Info("another operation is already in progress", "app", s)


I'm not really even sure we want to log this to be honest. This mean we tried to re sync the app while a sync was already in progress, correct? So what's the big deal? :)
I might miss a valid use case.

maruina · 2021-03-19T17:17:52Z

controllers/progressiverollout_controller.go

+					log.Info("another operation is already in progress", "app", s)
+				} else {
+					log.Error(err, "unable to sync app", "app", s)
+					return ctrl.Result{}, err


Do we want to somehow signal the stage as failed

For future work in #30 - as we don't do it at the moment

sledigabel · 2021-03-23T22:19:43Z

config/crd/bases/deployment.skyscanner.net_progressiverollouts.yaml

@@ -1,10 +1,10 @@

 ---
-apiVersion: apiextensions.k8s.io/v1beta1
+apiVersion: apiextensions.k8s.io/v1


Is this the official release for v1?
Should we keep it a separate commit and release/package a specific version?

Not at all. This was auto-generated by kubebuilder - I can revert the change and try to see where/why it was generated, but judging by https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/ v1beta1 seems to have been deprecated in v1.16?

that confused the hell out of me :-)
my bad

If we use apiextensions.k8s.io/v1 it will not run in our current control cluster, so please let's skip this for a little bit.

controllers/progressiverollout_controller_test.go

sledigabel · 2021-03-23T22:35:32Z

internal/utils/config.go

+ReadConfiguration returns a Configuration, reading it from environment variables if present, or from files in
+the configuration directory otherwise
+*/
+func ReadConfiguration() (*Configuration, error) {


why using pointer semantics here?
we're using value semantics everywhere else?

it's a minor thing, tbh, the logic of using pointer semantics makes sense so we can use nil. It's just against the decision made in #33.

@maruina what do you think?

Uhm - we can go with value semantics and return an empty config. As long as it also returns an error, we're able to handle it.

Depending how we make things complicated, but it would be good to follow the same convention.

Co-authored-by: Sebastien Le Digabel <sledigabel@gmail.com>

maruina · 2021-03-24T08:15:21Z

README.md

+make deploy
+```
+
+In order to so, a `.env.cluster` file needs to exist in the `config/manager` folder, containing


I am not sure about adding a file into the kustomize directory. Is that the only way to make kustomize work?

Not very fond of it either, to be honest. But from https://kubectl.docs.kubernetes.io/faq/kustomize/: v2.0 added a security check that prevents kustomizations from reading files outside their own directory root.

This meant I can't find any file that's not inside that directory - I'm open to alternatives.

Ah, so if I had not stopped reading at the first line, it says this:

Resources (including configmap and secret generators) can still be shared via the recommended best practice of placing them in a directory with their own kustomization file, and referring to this directory as a base from any kustomization that wants to use it. This encourages modularity and relocatability.

Let me investigate that.

maruina · 2021-03-24T08:15:25Z

README.md

+2. Files in the Config Directory.
+```
+.
+├── .prcconfig


Should this be ~/.prcconfig?

maruina · 2021-03-24T08:16:31Z

config/crd/bases/deployment.skyscanner.net_progressiverollouts.yaml

@@ -1,10 +1,10 @@

 ---
-apiVersion: apiextensions.k8s.io/v1beta1
+apiVersion: apiextensions.k8s.io/v1


If we use apiextensions.k8s.io/v1 it will not run in our current control cluster, so please let's skip this for a little bit.

maruina · 2021-03-24T08:18:01Z

config/manager/manager.yaml

@@ -36,4 +41,8 @@ spec:
          requests:
            cpu: 100m
            memory: 20Mi
+        volumeMounts:
+            - mountPath: /.prcconfig


I'm not sure why we should use a .prcconfig in the root filesystem. Maybe /etc/prcconfig, or /opt/prcconfig or ~/.prcconfig?

Good point, we should not. Which one do you prefer? I'm not unhappy with ~, but maybe /etc is a better place for it?

👍 for /etc/

maruina · 2021-03-24T08:32:38Z

go.mod

 	k8s.io/api v0.20.1
 	k8s.io/apimachinery v0.20.1
 	k8s.io/client-go v11.0.1-0.20190816222228-6d55c1b1f1ca+incompatible
+	k8s.io/kubernetes v1.20.1 // indirect


Do you know where this is coming from?

Tbh, no - it sure is weird because I've not installed nor updated any K8s-specific libraries. I'll get back to you on this.

maruina · 2021-03-24T08:33:50Z

internal/utils/config.go

+ReadConfiguration returns a Configuration, reading it from environment variables if present, or from files in
+the configuration directory otherwise
+*/
+func ReadConfiguration() (*Configuration, error) {


Depending how we make things complicated, but it would be good to follow the same convention.

sledigabel · 2021-03-24T10:05:56Z

internal/utils/argocd_client.go

+func GetArgoCDAppClient(c *Configuration) ArgoCDAppClient {
+	acdClientOpts := argocdclient.ClientOptions{
+		ServerAddr: c.ArgoCDServerAddr,
+		Insecure:   true,


mmmmh I'm not sure about this. could we parameterise this in the Configuration object instead of leaving this insecure by default?

maruina · 2021-03-25T08:26:23Z

controllers/progressiverollout_controller_test.go

@@ -22,6 +25,26 @@ const (
 	interval = time.Millisecond * 10
 )

+type mockArgoCDAppClientCounter struct {


I think there is something I don't understand here: why is this called Counter? Is that because we're counting how many apps we sync?

The idea of this mock is to mimic what in Java or Python would be assertCalledWith() or similar - I want to make sure that Sync() is called with a specific app name.

I think I called it Counter because initially I was just counting number of calls - but that's wrong, we want to know that a specific app was synced, rather than how many times apps where synced. So perhaps there's a better name for it?

The idea of this mock is to mimic what in Java or Python would be assertCalledWith() or similar - I want to make sure that Sync() is called with a specific app name.

Thanks, now it make more sense to me. Maybe mockArgoCDAppClientWithApp?

maruina · 2021-03-25T08:41:27Z

controllers/progressiverollout_controller_unit_test.go

+	"testing"
+)
+
+type mockArgoCDAppClientSyncOK struct{}


I think there is something that I'm missing here: why we don't have a mockArgoCDAppClient package with its own methods and tests?
The approach would be similar in https://github.com/Skyscanner/kms-issuer/tree/main/pkg/kmsmock

For why the mocks are in the file and not a separate package, see the other comment - I'm happy to move them if we prefer that.

There's also the fact that I want different mocks to do different things - in fact, the one in suite_test is more like a stub than a mock per se, it's there just to make sure the other tests pass by default.

maruina · 2021-03-25T08:42:01Z

controllers/suite_test.go

@@ -44,6 +47,12 @@ var k8sClient client.Client
 var testEnv *envtest.Environment
 var reconciler *ProgressiveRolloutReconciler

+type mockArgoCDAppClient struct{}
+
+func (*mockArgoCDAppClient) Sync(ctx context.Context, in *applicationpkg.ApplicationSyncRequest, opts ...grpc.CallOption) (*argov1alpha1.Application, error) {


I'm not sure I understand we we need to redefine the mockArgoCDAppClient and the sync method here. Can't we import it from a package?

From what I saw, defining mocks in the test files they're used seems to be the most common practice (see https://github.com/kubernetes/kubernetes/blob/cea1d4e20b4a7886d8ff65f34c6d4f95efcb4742/staging/src/k8s.io/apiserver/pkg/authentication/token/union/unionauth_test.go#L30, for an example).

But if we prefer to have a package for the mocks, I'm happy to move it.

I don't think I have good enough knowledge on the ecosystem and golang on this to be opinionated :)

I guess my point it was to be able to reuse the mock instead of redefining it multiple times? I guess when I see mockArgoCDAppClientCounter, mockArgoCDAppClient and mockArgoCDAppClientSyncOK they feel the same thing to me with different methods? But maybe not.

@sledigabel, @danmx, @Smirl: what do you think about this?

They serve different purposes in different tests, but are all implementing the same interface:

mockArgoCDAppClient is not a mock, it really is a generic stub so that we can run the other tests when we don't care about what happens in the calls to Argo CD.

mockArgoCDAppClientCounter is really just a way to assertCalledWith the Sync() method - we could use this mock as the 'general' one (in 1.) if we so wished, I opted to make the other entirely useless to make it clear we don't care about its functionality.

mockArgoCDAppClientSyncOK is used in unit testing to mock that the Argo CD app client returned OK, and conversely, SyncNotOK returns an error.

There's good things with this approach, imo: it's clear that each mock does something different, for instance. There's also bad things: if we ever opt to use one more method from the Argo CD App Client in the code, we have to update all these mocks, which adds toil to future maintainers.

Thoughts from those with more knowldege of Golang are most welcome here 🙂

why bother creating your own mocks in the first place? Go can generate mocks for you (https://github.com/golang/mock) based on your interfaces.

Don't see any reason not to use it, but I've never used any way of mocking in Golang, so I'm not opinionated on it.

Taking a step back then: what is the approach we want to take w.r.t. mocking in this project? If it's using gomock, then let's change these mocks :). Again, I'm not opinionated either way, but would rather decide on one approach and have consistency going forward.

yeah i was going to suggest having a /mock package would be convenient, in case we need to reuse this for other tests moving forward.
Not sure about using mock to do that, but why not if that helps in the future.

Alright - so I did some playground stuff with gomock and I think that might be useful in the long term. Bearing in mind that the Sync is a blocker to other tasks, I've opted to simply move the mocks to a mocks package for the time being. We should still agree on how we're doing these things long-term.

DiogoMCampos · 2021-03-26T16:51:57Z

Alright, I think I've addressed all comments - let me know if I haven't. A few notes:

I opted not to generate the secret with kustomize, as I feel Matteo is right on not wanting the .env file in those directories. Instead, I assume it already exists in the target cluster - for the purposes of development, the setup script generates said secret. If we are okay with this, I'll create a ticket for us to manage this secret in the future.
I think gomock is actually a very good shout - although, I did run into an issue with the Eventually/async tests and with gomock expecting a set number of calls in my experimentation. For the purposes of this PR, I'm suggesting we don't use it, but I think we should give it serious consideration going forward (and revisit the mocks in here).
As the reviews came in and the PR evolved, the configuration got a bit more complex than it was initially - at some point we should consider viper (or similar) instead of maintaining our own configuration manager.

maruina · 2021-03-29T12:36:07Z

controllers/progressiverollout_controller_unit_test.go

@@ -0,0 +1,35 @@
+package controllers


Question for the wider audience: do we want to have a separate test file for the unit tests, or can we merge those tests with progressiverollout_controller_test.go?

I would prefer a single test file for simplicity, but I'm open to suggestions

There's an argument to separate integration tests vs unit tests as they look and are different.
How is it done on other operators?

I have never seen anything other controller.go and controller_test.go but I am happy if anyone can findaa different example.

In other languages (such as Python or Java), tests would be in their own folder, and I feel like I'd separate unit and other types of tests in different files too. So I felt this makes sense.

But Golang seems to be different in that tests generally live in the same package as the code under test; so perhaps a 1:1 relation between code-under-test and test-code files is ideal? If so, I can move them to the same file. I'm not opinionated on this.

internal/utils/config.go

Co-authored-by: Matteo Ruina <matteo.ruina@gmail.com>

maruina · 2021-03-29T14:43:15Z

@DiogoMCampos I think we should also change the helm chart to exposes

ArgoCDAuthToken
ArgoCDServerAddr
ArgoCDInsecure

in the values.yaml and create the secret. It is then up to the end-user how to pass to token to the chart.

internal/utils/config.go

sledigabel · 2021-03-30T08:56:12Z

controllers/progressiverollout_controller.go

+	syncReq := applicationpkg.ApplicationSyncRequest{
+		Name: &appName,
+	}
+


I think we have an opportunity here to validate that the app being synced is the app we passed as a parameter.
i'll raise a PR separately, it's not blocking that one.

maruina

LGTM

sledigabel

LGTM, adds a sync capability to the progressive rollout operator

sledigabel · 2021-04-05T11:27:58Z

Are we gonna merge this?

DiogoMCampos added 12 commits March 11, 2021 17:34

Add initial skeleton of app syncing code

c906c0c

Create Argo CD clients on startup

b40c1f4

Formatting and variable naming updates

a293e7a

Merge branch 'main' into app-sync

be67420

Regen go.sum

d417182

Restructure code to mock Argo CD client

3c98af7

Return synced applications

73049dd

Fix configuration loading bug

c68675d

Add tests related to app sync

417d162

Add mutex to mocked argocd app client

0266d6c

Automatically create .prcconfig directory to run locally

94b25f0

Add .prcconfig to .gitignore

ee55d75

DiogoMCampos changed the title ~~WIP: Application Sync~~ Send Application Sync request to Argo CD Server Mar 19, 2021

Adopt shellcheck recommendations

f44facd

DiogoMCampos force-pushed the app-sync branch from f10ca70 to f44facd Compare March 19, 2021 16:29

DiogoMCampos commented Mar 19, 2021

View reviewed changes

controllers/progressiverollout_controller_test.go Outdated Show resolved Hide resolved

DiogoMCampos commented Mar 19, 2021

View reviewed changes

controllers/progressiverollout_controller.go Outdated Show resolved Hide resolved

Use mutex in mocked argocd app client

1c643be

maruina reviewed Mar 19, 2021

View reviewed changes

DiogoMCampos added 4 commits March 23, 2021 18:29

Set up ability to run locally with env variables

c291262

Reenable leader election

0e231ce

Update README with information on configuration

00dc5b9

Merge branch 'main' into app-sync

b01394f

sledigabel reviewed Mar 23, 2021

View reviewed changes

controllers/progressiverollout_controller_test.go Outdated Show resolved Hide resolved

sledigabel reviewed Mar 23, 2021

View reviewed changes

controllers/progressiverollout_controller_test.go Outdated Show resolved Hide resolved

sledigabel reviewed Mar 23, 2021

View reviewed changes

DiogoMCampos and others added 2 commits March 24, 2021 09:19

Update main.go

950822d

Co-authored-by: Sebastien Le Digabel <sledigabel@gmail.com>

Address review comments

d6a7155

maruina reviewed Mar 24, 2021

View reviewed changes

sledigabel reviewed Mar 24, 2021

View reviewed changes

DiogoMCampos added 2 commits March 24, 2021 14:04

Reset prog rollout definition to use v1beta1

b8c728e

Address review comments

e64b6a6

DiogoMCampos force-pushed the app-sync branch from b0cf549 to e64b6a6 Compare March 24, 2021 13:52

maruina reviewed Mar 25, 2021

View reviewed changes

DiogoMCampos added 2 commits March 26, 2021 13:34

Address review comments around configuration

5edcfbc

Move to mocks to mocks package

a78221d

DiogoMCampos added 2 commits March 29, 2021 11:49

Remove leftover generatorOptions

8d8e31a

Update README

c0da044

maruina reviewed Mar 29, 2021

View reviewed changes

Update internal/utils/config.go

a158563

Co-authored-by: Matteo Ruina <matteo.ruina@gmail.com>

sledigabel reviewed Mar 29, 2021

View reviewed changes

internal/utils/config.go Show resolved Hide resolved

sledigabel mentioned this pull request Mar 29, 2021

Add some sensible defaults to the progressive rollout #45

Open

DiogoMCampos added 3 commits March 30, 2021 10:16

Add config secret to helm chart

ce985c9

Base 64 encode secrets

6d3d31f

Bump Helm chart version

e711f9f

sledigabel reviewed Mar 30, 2021

View reviewed changes

Merge controller test files

88573b9

maruina reviewed Mar 31, 2021

View reviewed changes

sledigabel approved these changes Mar 31, 2021

View reviewed changes

DiogoMCampos merged commit 42c9374 into main Apr 5, 2021

sledigabel mentioned this pull request Apr 5, 2021

Add sync Application logic #22

Closed

maruina deleted the app-sync branch May 12, 2021 12:45

		r.Log.Info("syncing app", "app", s)

		_, err := r.syncApp(s.Name)

Send Application Sync request to Argo CD Server #44

Send Application Sync request to Argo CD Server #44

Conversation

DiogoMCampos commented Mar 16, 2021 • edited

What this does

What this doesn't do

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maruina 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiogoMCampos Mar 24, 2021 • edited

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

Choose a reason for hiding this comment

maruina Mar 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DiogoMCampos Mar 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maruina Mar 25, 2021 • edited

Choose a reason for hiding this comment

DiogoMCampos Mar 25, 2021 • edited

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

DiogoMCampos commented Mar 26, 2021 • edited

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

maruina commented Mar 29, 2021

Choose a reason for hiding this comment

maruina left a comment

Choose a reason for hiding this comment

sledigabel left a comment

Choose a reason for hiding this comment

sledigabel commented Apr 5, 2021

DiogoMCampos commented Mar 16, 2021 •

edited

DiogoMCampos Mar 24, 2021 •

edited

maruina Mar 25, 2021 •

edited

DiogoMCampos Mar 25, 2021 •

edited

maruina Mar 25, 2021 •

edited

DiogoMCampos Mar 25, 2021 •

edited

DiogoMCampos commented Mar 26, 2021 •

edited