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

Create default passwords when dev mode is set. #441 #442

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

cmoulliard
Copy link
Contributor

@cmoulliard
Copy link
Contributor Author

Question: Is it the best place to add the code able to generate a new password (= developer) when devMode is enabled -

if result, err := argocd.Install(ctx, resource, r.Client, r.Scheme, r.Config); err != nil {
? @nabuskey

@nabuskey
Copy link
Collaborator

nabuskey commented Nov 8, 2024

Yeah after install() comes back with no errors.

@cmoulliard

This comment was marked as resolved.

@cmoulliard cmoulliard marked this pull request as ready for review November 8, 2024 17:21
@cmoulliard
Copy link
Contributor Author

If what I did cannot work, we have 2 options:

  • Create an Argocd PR to propose to use password instead of generating one usng a new parameter --dev or --insecure-password or --dev-password`
    OR
  • Add to our project the 2 Kube secrets (argocd-secret and argocd-initial-admin-secret) with content populated and where we set the property adminAccount.Enabled to false

@nabuskey
Copy link
Collaborator

nabuskey commented Nov 9, 2024

I am still not sure if the flag name is what we want to go with.

Is proving static password the only thing needed for dev mode? Does it describe what it does? If we add more config changes for dev mode in the future, do we want to provide ways to control each change?

If the names I proposed in the issue are too long, can we get away with a short flag?

I do like the idea of having a static default passwords for sure.

@cmoulliard
Copy link
Contributor Author

Is proving static password the only thing needed for dev mode? Does it describe what it does? If we add more config changes for dev mode in the future, do we want to provide ways to control each change?

If we continue to develop idpbuilder, having a devMode help a lot as we do for quarkus devMode able to launch testcontainers: backstage, DB, argocd, gitea, etc and where developers can access many information using dev UI - https://quarkus.io/guides/dev-mode-differences#dev-mode-features :-)

@cmoulliard
Copy link
Contributor Author

If the names I proposed in the issue are too long, can we get away with a short flag?

Let's review that later when we have a PR which is working as --dev is short, enough to test the PR

@cmoulliard
Copy link
Contributor Author

I do like the idea of having a static default passwords for sure.

I will make a try to see if that works

@cmoulliard

This comment was marked as resolved.

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Nov 12, 2024

Apparently, we should be (to be investigated of course) to set the accounts directly within the argocd secret as discussed here: argoproj/argo-cd#4096 (comment)

I was able to play with a user developer where I installed or customized some resources.

RBAC

apiVersion: v1
kind: ConfigMap
metadata:
  labels:
    app.kubernetes.io/name: argocd-rbac-cm
    app.kubernetes.io/part-of: argocd
  name: argocd-rbac-cm
  namespace: argocd
data:
  policy.csv: |
    p, role:developer, applications, *, *, allow
    g, developer, role:developer

ConfigMap

apiVersion: v1
kind: ConfigMap
metadata:
  labels:
    app.kubernetes.io/name: argocd-cm
    app.kubernetes.io/part-of: argocd
  name: argocd-cm
  namespace: argocd
data:
  accounts.developer: apiKey, login
  application.resourceTrackingMethod: annotation
  resource.exclusions: |
    - kinds:
        - ProviderConfigUsage
      apiGroups:
        - "*"
  timeout.reconciliation: 60s

As the following secret is created on the flight by argocd when admin.enabled and generate an admin password using bcrypt, we should find a way to patch it with the developer password

Argocd Secret

apiVersion: v1
kind: Secret
metadata:
  labels:
    app.kubernetes.io/name: argocd-secret
    app.kubernetes.io/part-of: argocd
  name: argocd-secret
  namespace: argocd
type: Opaque
data:
  accounts.developer.password: JDJhJDEwJEYwakhFSmJMYTEvREQzWUZsOWtxN2U0TUZXUXdDclhZWXZNTXhYVXEwdGhVWWUuLi9WWUZL
  accounts.developer.passwordMtime: MjAyNC0xMS0xMlQwODo1NDoyNVo=
...

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Nov 12, 2024

The PR supports now to log on to the UI of Argocd using (developer/developer) with RBAC Applications - Admin or gitea (developer/developer)

Can you please review it ?
@nabuskey

@cmoulliard
Copy link
Contributor Author

Apparently we cannot use for gitea developer/developer as we got such an error reported when we create a cluster

Nov 12 11:52:39 ERROR Reconciler error controller=gitrepository controllerGroup=idpbuilder.cnoe.io controllerKind=GitRepository name=argocd namespace=idpbuilder-dabou namespace=idpbuilder-dabou name=argocd reconcileID=1f80e800-276a-4b74-a868-b54ff4a26de5 err=creating repository: failed to create git repository: The repository with the same name already exists. 
Nov 12 11:52:39 ERROR Reconciler error controller=gitrepository controllerGroup=idpbuilder.cnoe.io controllerKind=GitRepository name=nginx namespace=idpbuilder-dabou namespace=idpbuilder-dabou name=nginx reconcileID=798effc7-600a-4035-8e39-eb8bcdcb79b3 err=creating repository: failed to create git repository: The repository with the same name already exists. 
Nov 12 11:52:39 ERROR Reconciler error controller=gitrepository controllerGroup=idpbuilder.cnoe.io controllerKind=GitRepository name=gitea namespace=idpbuilder-dabou namespace=idpbuilder-dabou name=gitea reconcileID=bbb8fbdd-2803-4904-b0c5-c680e2a9c0b9 err=creating repository: failed to create git repository: The repository with the same name already exists. 

I will then revert to the user giteaAdmin in the meantime

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

If you are having problems setting passwords on install, we can change them through APIs after install.

namespace: argocd
data:
policy.csv: |
p, role:developer, applications, *, *, allow
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of having a separate account that has a very similar permissions as admin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an account for the developers and it only allows to handle applications

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you tell me if this is inline with what you are thinking?

  1. Use a random password for the developer and admin account if the dev password flag is unset.
  2. Use a static, known password for the developer and admin account if the dev password flag is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Option 2 => Use a known password for the developer and admin account if the dev password flag is set

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok that sounds good to me. Looks like Gitea static password isn't working for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok that sounds good to me. Looks like Gitea static password isn't working for some reason?

For gitea when using dev-mode, we should still use as user: giteaAdmin and password = developer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you are adding a user called developer to argocd, we may as well add the developer user in Gitea.

return ctrl.Result{}, fmt.Errorf("Error marshalling patch data: %w", err)
}

kubeClient, err := k8s.GetKubeClient()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The reconciler has client already. You can just use that instead. e.g. r.Client.. You can also change passwords through ArgoCD api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can also change passwords through ArgoCD api.

That will require to be done in a 2nd step while here the idea is to have the account developer/developer available when packages are installed too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

r.Client fixed. See: 7d81b27

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also change passwords through ArgoCD api.

That will require to be done in a 2nd step while here the idea is to have the account developer/developer available when packages are installed too

Yeah I am starting to think we should set them through API for both Gitea and ArgoCD in every invocation of idpbuilder. Instead of relying on manifests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should then (maybe) create some job(s) which are applied post installation of the core package like gitea or argocd and able to execute some additional steps based on parameters passed.

Until now, the first action could be to create a new user: developer able to log on using as user/pwd => developer/developer or idpbuilder/developer if the parameter passed is --dev-mode.

A second job could also patch existing resources if by example we pass a different DNS domain = --host

etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flag renamed from "dev" to "dev-mode". See: a8c3016

Comment on lines 102 to 111
err = kubeClient.Get(ctx, client.ObjectKey{Name: argocdAdminSecretName, Namespace: argocdNamespace}, &s)
if err != nil {
return ctrl.Result{}, fmt.Errorf("getting argocd secret: %w", err)
}

// Patching the argocd-secret with the user's hashed password
err = kubeClient.Patch(ctx, &s, client.RawPatch(types.StrategicMergePatchType, patchBytes))
if err != nil {
return ctrl.Result{}, fmt.Errorf("Error patching the Secret: %w", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use server side apply instead here to simplify this process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "server side" ? My code is similar to what we do here with the Gitea -

return r.Client.Patch(ctx, &u, client.Apply, client.ForceOwnership, client.FieldOwner(v1alpha1.FieldManager))
...

Copy link
Collaborator

@nabuskey nabuskey Nov 21, 2024

Choose a reason for hiding this comment

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

The way it's done in Gitea is server side apply. If you look further up, you'll see it's working with unstructured object to avoid having ownership of fields we do not care about. Sometimes we see errors like "object has been updated and version doesn't match". Server side apply avoids that.

This talks about it: https://eng.d2iq.com/blog/conflict-resolution-kubernetes-server-side-apply/

Here's where unstructured object is used:

u := unstructured.Unstructured{}
u.SetName(giteaAdminSecret)
u.SetNamespace(giteaNamespace)
u.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("Secret"))

Here's another example:

func ApplyAnnotation(ctx context.Context, kubeClient client.Client, obj client.Object, annotations map[string]string, opts ...client.PatchOption) error {
// MUST be unstructured to avoid managing fields we do not care about.
u := unstructured.Unstructured{}
u.SetAnnotations(annotations)
u.SetName(obj.GetName())
u.SetNamespace(obj.GetNamespace())
u.SetGroupVersionKind(obj.GetObjectKind().GroupVersionKind())
return kubeClient.Patch(ctx, &u, client.Apply, opts...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. See: 80a785b

Comment on lines 26 to 30
argocdDevModePassword = "developer"
argocdAdminSecretName = "argocd-secret"
argocdInitialAdminSecretName = "argocd-initial-admin-secret"
argocdInitialAdminPasswordKey = "argocd-initial-admin-secret"
argocdNamespace = "argocd"
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are already defined in other packages I think. Either export them or move them to globals or APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed 2 non used constants and others are non declared in another go file or package. See: 78e1e40

@@ -20,6 +20,7 @@ import (
const (
recreateClusterUsage = "Delete cluster first if it already exists."
buildNameUsage = "Name for build (Prefix for kind cluster name, pod names, etc)."
devModeUsage = "When enabled, the platform will run the core packages with developer password."
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. I am just not sold on the name dev mode. idpbuilder is primary used for development and testing purposes already. I think we should use more explicit name with a short flag.
  2. Even if we accept dev mode, we shouldn't limit the flag name to just passwords. dev mode could mean a lot of different things. This should be a meta flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. idpbuilder is primary used for development and testing purposes already. I think we should use more explicit name with a short flag.

If this is the case, then we should install gitea and argocd using a non generated password ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2. Even if we accept dev mode, we shouldn't limit the flag name to just passwords. dev mode could mean a lot of different things. This should be a meta flag.

What about using the the parameter: --devPwd or --devPassword as boolean @nabuskey

Copy link
Collaborator

@nabuskey nabuskey Nov 21, 2024

Choose a reason for hiding this comment

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

I am ok with dev-pwd or dev-password since we use snake case for our flags. We can also have --dev-mode flag that is essentially a convenient flag that enables dev passwords and any other QOL stuff for dev purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally we aligned our paths => I will then use --dev-mode

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, I meant to have two flags. (--dev-password OR --dev-pwd ) AND --dev-mode because we could enable other QOL features under --dev-mode other than static passwords.

pkg/k8s/util.go Outdated

"github.com/cnoe-io/idpbuilder/pkg/util"
"k8s.io/apimachinery/pkg/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
)

var (
setupLog = ctrl.Log.WithName("k8s")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather not produce log messages in utility functions. Let's wrap the error instead. e.g. fmt.Errorf("Error creating kubernetes client: %w"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with: 8158551

@cmoulliard
Copy link
Contributor Author

I pushed a commit: 3c3511d to show the developer username/password @nabuskey

Remark: This code should be reviewed and improved as the secret containing the developer username/password is argocd-secret where the password has been bcrypted and by consequence we cannot get and decode it from the secret
This is why it has been included as such and will be displayed every time no matter if --dev-mode has been used or not

Comment on lines +211 to +221
// TODO: The following code should be reviewed and improved as the secret containing the developer username/password is argocd-secret
// where the password has been bcrypted and by consequence we cannot get and decode it from the secret
// This is why we are going to add it here BUT it will be displayed every time no matter if --dev-mode has been used or not
if strings.Contains(s.Name, "gitea") {
data.Data["username-developer"] = "giteAdmin"
data.Data["password-developer"] = "developer"
} else if strings.Contains(s.Name, "argocd") {
data.Data["username-developer"] = "developer"
data.Data["password-developer"] = "developer"
}

Copy link
Collaborator

@nabuskey nabuskey Nov 22, 2024

Choose a reason for hiding this comment

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

We can get the localbuild object from K8s since it's essentially our way of storing configurations. You added the DevMode field so you can just check the value. We should change that field name btw. devPassword makes sense. There's a typo: giteAdmin -> giteaAdmin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can get the localbuild object from K8s since it's essentially our way of storing configurations.

Yes but the flag --dev-mode or --dev-password is not passed at all to the command: idp get secrets and users will not like to perform such a command idp get secrets --dev-mode.

@nabuskey
Copy link
Collaborator

The more I think about it, I think it's cleaner to change passwords and add users post install. This enables flows like:

  1. Create a cluster without specifying the static password flag. e.g. idpbuilder create
  2. Run the same command except we specify the static password flag. idpbuilder create --dev-password

@cmoulliard
Copy link
Contributor Author

The more I think about it, I think it's cleaner to change passwords and add users post install

I'm also in favor to go down that road as I mentioned also in a different comments where we could use job or something else able to perform additional config steps or let's say post install steps.

2 remarks:

  • Such approach will nevertheless don't allow the user to watch directly using developr password the information avaialble from the web application using UI except if they use the generated password
  • We should avoid to request to the user to re-apply the command idpbuilder create with additional flags as that will create confusion.
    A better approach would be to have a different command to install the packages and their config, execute some "actions" on a core package or package using a job, etc.
    Idea about such commands could be:
  • idp package add <package_name> or <package1_name,package2_name2,etc>
  • idp package execute <job1> // for the post-installation tasks:

@cmoulliard
Copy link
Contributor Author

As the result about what I did and also what I discovered imply that finally we hack the existing code to support to generate a developer password, should we close it and come with a better solution as proposed here: #442 (comment)

WDYT ? @nabuskey

@nabuskey
Copy link
Collaborator

2 remarks:
Such approach will nevertheless don't allow the user to watch directly using developr password the information avaialble from the web application using UI except if they use the generated password
We should avoid to request to the user to re-apply the command idpbuilder create with additional flags as that will create confusion.
A better approach would be to have a different command to install the packages and their config, execute some "actions" on a core package or package using a job, etc.
Idea about such commands could be:
idp package add <package_name> or <package1_name,package2_name2,etc>
idp package execute // for the post-installation tasks:

What I meant was to do this in post-install (e.g. the core packages are ready) during the reconciliation loop. Essentially around here:

go r.installCorePackages(instCtx, req, &localBuild, errChan)

Instead of using manifests to set initial passwords, change them to a known value if the dev password flag is set.

We should avoid to request to the user to re-apply the command idpbuilder create with additional flags as that will create confusion.

I agree with this. We should have a separate command to configure / reconfigure things.

package execute
This implies we need to create a configuration file for each package. I have been resisting the idea of creating yet another configuration file. Maybe we end up needing to create one because current approach of using argocd resource hooks is not too user friendly. BUT for this particular issue of setting static passwords, it's not necessary imo.

@cmoulliard
Copy link
Contributor Author

Instead of using manifests to set initial passwords, change them to a known value if the dev password flag is set.

I dont follow you

Instead of using manifests to set initial passwords, change them to a known value if the dev password flag is set.

I don't set initial password using manifests. Can you then elaborate a bit here ? @nabuskey

@nabuskey
Copy link
Collaborator

nabuskey commented Dec 3, 2024

So here's the flow I am thinking.

  1. We do the things we already do bootstrap the cluster. e.g. stand up kind, install argocd, gitea, etc.
  2. During the installation process, we do not set static passwords. We do it the way it's done as-is.
  3. Once these services are up and available, we grab the initial passwords or token, then use that to change the passwords.

@cmoulliard
Copy link
Contributor Author

cmoulliard commented Dec 3, 2024

3. Once these services are up and available, we grab the initial passwords or token, then use that to change the passwords.

This is perhaps doable for gitea but not for argocd as there is no argocd Go Lib that we could use for that purpose, the Argocd CLI don't offer such an option to create new account except to update an existing account argocd account update-password --account developer. Reason why I updated the argocd secret to include the account developer !

The gitea go client allows to create a user (which is perhaps an admin user - to be verified)

// From user_test.go - https://gitea.com/gitea/go-sdk/src/branch/main/gitea/user_test.go#L205
client.AdminCreateUser(CreateUserOption{Username: username, Password: username + "!1234", Email: username + "@gitea.io", MustChangePassword: OptionalBool(false), SendNotify: false})

I confirm that we can create a gitea user using such a code

func NewGiteaUser(ctx context.Context) {
	baseUrl := "http://localhost:3000"
	username := "developer"
	password := "developer"
	client, err := gitea.NewClient(baseUrl, gitea.SetHTTPClient(util.GetHttpClient()),
		gitea.SetBasicAuth(username, password), gitea.SetContext(ctx),
	)
	if err != nil {
		fmt.Errorf("cannot create gitea client: %w", err)
	}

	opts := gitea.CreateUserOption{
		Username: "user1",
		Password: "user451!",
		Email:    "[email protected]",
	}

	user, resp, err := client.AdminCreateUser(opts)
	if err != nil {
		fmt.Errorf("cannot create gitea user: %w", resp.Status, err)
	}
	fmt.Printf("User created: %s", user.UserName)
	os.Exit(0)
}

@nabuskey

@nabuskey
Copy link
Collaborator

nabuskey commented Dec 4, 2024

https://cd.apps.argoproj.io/swagger-ui#operation/AccountService_UpdatePassword

It's available through the rest api. We should be able to make a request for this I think.

@cmoulliard
Copy link
Contributor Author

It's available through the rest api. We should be able to make a request for this I think.

This is the request used by the argocd CLI but there is no command or REST API to create a new user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants