Upgrade operator sdk + a few fixes #8

plaffitt · 2022-05-25T15:08:49Z

This PR contains work from various PR from our fork at Enix. Here is a changelog:

Use lpass status to validate re-login and disable logout command 🛠️
De-hardcode namespace in helm chart 🛠️
Upgrade operator-sdk from 0.8.1 to 1.5.2 🎉
Upgrade golang from <1.13 (before go modules are the default) to golang 1.17 🎉
Upgrade crdVersion from v1alpha1 to v1 🎉
Start building the operator binary inside the Dockerfile with multi-stage build instead of doing it outside before copying it 🎉
Fix missing required namespace in ClusterRoleBinding 🛠️
Add flag (and helm value) to define a template for secrets names 🎉

Namespace is handled by helm and should not be hardcoded

Use of lpass status to validate re-login and disabling logout command

Remove unneeded id from secretName

echo | docker login -u paullaffitte Error: Cannot perform an interactive login from a non TTY device

Upgrade/operator sdk

niqdev · 2022-05-25T17:28:06Z

pkg/lastpass/cli.go

-	if err != nil || "" == string(out) {
-		// sometimes returns error: "Error: HTTP response code said error" even if the credentials are valid
-		return fmt.Errorf("verify credentials, unable to login: %s", err)
+	_, err := sh.Command("lpass", "status").Output()


out of curiosity, any performance issues for doing this i.e. skipping the logout?

Hi, i didn't spot any performance issues, but in our use-case, we need to sync hundreds of secrets, with the logout activated, we get banned from Lastpass.
We didn't try to Log in at each secret synchronisation, but we'll probably get banned too from Lastpass.

Fair enough!

niqdev · 2022-05-25T17:30:41Z

pkg/lastpass/cli.go

 	}
 	log.Printf("Succesfully logged in")
 	return nil
 }

 // Logout using lastpass-cli
 func Logout() {
-	// lpass logout --force


i would probably keep the implementation and comment lastpass.Logout() in here - No point in keeping the method around otherwise

I think it will be ok 👍 Thanks

I'll look into it 😉

niqdev · 2022-05-25T17:32:17Z

main.go

+		MetricsBindAddress: metricsAddr,
+		Port:               9443,
+		LeaderElection:     enableLeaderElection,
+		LeaderElectionID:   "e9330328.edgelevel.com",


what is e9330328 for?

This part of the code was generated by Operator SDK, it's just a random ID to ensure it's unique I guess.

niqdev · 2022-05-25T17:33:23Z

controllers/suite_test.go

+		[]Reporter{printer.NewlineReporter{}})
+}
+
+// var _ = BeforeSuite(func(done Done) {


were these tests auto-generated?

Yes, they were

niqdev · 2022-05-25T17:36:05Z

controllers/lastpass_controller.go

 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:        cr.Name + "-" + secret.ID,
+			Name:        secretName.String(),


i haven't used it in a while, just wondering about potential regressions, do you have an example of how it was looking before cr.Name + "-" + secret.ID compared to now?

I think I remembered why I did this, would there be any potential issues with secret conflicts? See example in the readme example-lastpass-8190226423897406876

The secretName variable comes from a go template. The default template used to generate the secret name is the following one {{.LastPass.ObjectMeta.Name}}-{{.LastPassSecret.ID}}, so it replicate the old behavior and should not introduce any breaking change. After this change I get by default names like the following lp-cred1-7726733720672245904.

Thanks for clarifying this!

.travis/install_operator_sdk.sh

niqdev · 2022-05-25T17:41:32Z

chart/templates/deployment.yaml

          imagePullPolicy: Always
+          {{- if ne .Values.secretNameTemplate "" }}
+          args:
+            - --secret-name-template={{ .Values.secretNameTemplate }}


what is this exactly for?

This is the go template I talked about above. The default one is {{.LastPass.ObjectMeta.Name}}-{{.LastPassSecret.ID}} in order to avoid breaking changes. Setting the secretNameTemplate value in helm chart allows to override this value.

niqdev · 2022-05-25T17:51:00Z

@paullaffitte I'm really grateful for the help here! Amazing work, thanks!

A couple of things

would there be any issue if I upgrade the chart to helm 3? are you actually pointing to the chart currently in the repo?
I'll definitely update the release process to use GH actions
do you mind if I ping you directly or involve in reviewing PRs in the future?
Please consider creating a separate PR adding yourself or Enix as contributors in the README!

Thanks again for contributing to the project! I'll wait for your feedback in the comments and then I'll merge the PR

plaffitt · 2022-05-30T12:23:52Z

You're welcome, it's always a pleasure to help open-source projects 🙂

We tried our best to answer your above questions.

And to answer your last message:

We already use helm 3 so it's totally fine to upgrade the chart.
Nice!
Sure, If I can be of any help.
We will do it, thanks 🙂

niqdev · 2022-05-30T20:42:38Z

Hi, I haven't installed go and operator-sdk in a while, I'm updating the doc and I was trying to build your repo/branch first and I realized that there were some simple issues to fix in the makefile e.g. replace go get with go install, which led me to look into the dependency versions...

Was there any concern in bumping to the latest operator-sdk version, i.e. 1.21.0 cos of too many breaking changes, or it just happened that you did this a while back and the version went forward? Don't get me wrong, I'm just trying to understand your changes 😅

The main reason for asking this is because I need to fix the CI and install the right version of the operator etc., that's all!

plaffitt · 2022-06-02T08:39:53Z

Hi, the makefile was generated by operator-sdk, what is the issue with go get ?

Concerning operator-sdk version, we upgraded it for some reasons, and we just had no reason to upgrade to the latest version, so to save time I stopped at the first version meeting our requirements. But I think it's completely fine to bump the version until the latest one. Our reasons were the following:

We want to build everything from inside the docker image (instead of building locally and then copy the file into the image).
We want to upgrade at least to the first "stable" version (1.0.0 or above) see below why.
We want to use crds version v1 instead of v1beta1 (since operator-sdk 1.0.0).
v1.5.0 is the first version where PROJECT config version 3-alpha is deprecated in favor of version 3. Some files in version 3-alpha seem to not exist anymore so it seems to be impossible to install and use operator-sdk between 1.0.0 and 1.5.0.
We want to use go modules by default (since golang 1.13) and use it's latest available version if possible (1.17).

Why we wanted to upgrade to at least v1.0.0

One of our goals where to be able to upgrade easily the operator in the future.

When upgrading your version of Operator-sdk, it is intended that post-1.0.0 minor versions (i.e. 1.y) are backwards compatible and strictly additive. Therefore, you only need to re-scaffold your operator with a newer version of Operator-SDK if you wish to take advantage of new features. If you do not wish to use new features, all that should be required is bumping the operator image dependency (if a Helm or Ansible operator) and rebuilding your operator image.
https://sdk.operatorframework.io/docs/upgrading-sdk-version/

Now that all of this is done, the remaining upgrades should be easier and faster than the previous ones.

niqdev · 2022-06-03T07:45:57Z

Thanks for all the details, that was very helpful! I'll let you know when I release the image with your changes 🚀

donch and others added 21 commits April 7, 2022 17:46

Use of lpass status to validate login and disabling logout command

7781b00

Namespace are handed by helm and should not be hardcoded

df26e27

Merge pull request #2 from enix/refactor_helm

ddd66bd

Namespace is handled by helm and should not be hardcoded

Use a correct log message in logout function.

2961b53

Merge pull request #1 from enix/verify_login

30df22e

Use of lpass status to validate re-login and disabling logout command

Remove uneeded id from secretName

8bf1569

Merge pull request #3 from enix/remove_id_from_secret

a4392e6

Remove unneeded id from secretName

upgrade to operator-sdk 0.8.2 and golang 1.17

872a46b

upgrade to operator-sdk 0.17.2

497cc22

fix missing required namespace in ClusterRoleBinding

d4ce590

remove docker login before push in makefile

1ddca41

echo | docker login -u paullaffitte Error: Cannot perform an interactive login from a non TTY device

upgrade to operator-sdk 0.18.2

f29229f

upgrade to operator-sdk 0.19.4

310c2cf

upgrade to operator-sdk 1.0.1 and crdVersion v1

9b83307

upgrade to operator-sdk 1.5.2

86088e9

remove Apache 2.0 references since the project is under MIT

7cebf8c

fix syncPolicy is optional

7282e74

remove duplicate setups of lastpass controller in main.go

046a781

Merge pull request #4 from enix/upgrade/operator-sdk

97279ce

Upgrade/operator sdk

add flag (and helm value) to define a template for secrets names

8e2147f

restore repository default value to edgelevel instead of enix

eab5d19

niqdev reviewed May 25, 2022

View reviewed changes

.travis/install_operator_sdk.sh Show resolved Hide resolved

niqdev reviewed May 25, 2022

View reviewed changes

restore logout function but stop calling it

7ac3614

niqdev merged commit aa5913b into edgelevel:master Jun 3, 2022

niqdev mentioned this pull request Jun 3, 2022

TODOs after #8 is merged #9

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade operator sdk + a few fixes #8

Upgrade operator sdk + a few fixes #8

plaffitt commented May 25, 2022

niqdev May 25, 2022

donch May 30, 2022

niqdev May 30, 2022

niqdev May 25, 2022

donch May 30, 2022

niqdev May 30, 2022

niqdev May 25, 2022

plaffitt May 30, 2022

niqdev May 25, 2022

plaffitt May 30, 2022

niqdev May 25, 2022

niqdev May 25, 2022

plaffitt May 30, 2022 •

edited

Loading

niqdev May 30, 2022

niqdev May 25, 2022

plaffitt May 30, 2022 •

edited

Loading

niqdev commented May 25, 2022

plaffitt commented May 30, 2022

niqdev commented May 30, 2022 •

edited

Loading

plaffitt commented Jun 2, 2022

niqdev commented Jun 3, 2022

Upgrade operator sdk + a few fixes #8

Upgrade operator sdk + a few fixes #8

Conversation

plaffitt commented May 25, 2022

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

plaffitt May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plaffitt May 30, 2022 • edited Loading

Choose a reason for hiding this comment

niqdev commented May 25, 2022

plaffitt commented May 30, 2022

niqdev commented May 30, 2022 • edited Loading

plaffitt commented Jun 2, 2022

niqdev commented Jun 3, 2022

plaffitt May 30, 2022 •

edited

Loading

plaffitt May 30, 2022 •

edited

Loading

niqdev commented May 30, 2022 •

edited

Loading