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 38 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 cmoulliard requested a review from nabuskey November 8, 2024 17:32
@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be fair is the --dev- prefix needed? I would imagine we could just have a --static-creds parameter or --static-credentials to set the password statically rather then dynamically? Unless there's a reason why we feel --dev- prefix is valuable?

The only argument I could maybe gather is that since it is no longer than admin credentials and it is developer credentials that --dev- prefix gives clarity to that but I think a good description here could explain that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--dev-mode is a pretty common parameter used by many developer's tools and CLI which could also be used to support different additional features in the future. On the opposite --static-creds or --static-credentials parameters are probably too restrictive, don't help the users as they have no idea about what static word means

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.

I still think this needs to be the case.

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

@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

cmoulliard commented Dec 4, 2024

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.

I did a test using the update password command and even if we can change the password the initial-admin-secret is not changed and then you will continue to get as value the original value except if we update it in our code

❯ argocd account update-password --account admin
2024/12/04 09:10:49 maxprocs: Leaving GOMAXPROCS=12: CPU quota undefined
*** Enter password of currently logged in user (admin):
*** Enter new password for user admin:
*** Confirm new password for user admin:
Password updated
Context 'argocd.cnoe.localtest.me:9443' updated

~/code/cnoe/fork-idpbuilder on dev-mode •
❯ idp  get secrets -p argocd
---------------------------
Name: argocd-initial-admin-secret
Namespace: argocd
Data:
  password : aXoAmYg2y3dNvFi4
  username : admin

@nabuskey
Copy link
Collaborator

nabuskey commented Dec 4, 2024

It is a rest API reference page. There's an instruction here too: https://argo-cd.readthedocs.io/en/latest/developer-guide/api-docs/

I tested it and it does work.

# get initial token
curl -ik https://argocd.cnoe.localtest.me:8443/api/v1/session -d $'{"username":"admin","password":"dMQW00CssVXZWz7t"}' -H "Content-Type:application/json"

# update password
curl -X PUT -ik https://argocd.cnoe.localtest.me:8443/api/v1/account/password -d $'{"currentPassword":"dMQW00CssVXZWz7t","newPassword":"password", "name":"admin"}' -H "Content-Type:application/json" -H "Authorization: Bearer $ARGOCD_TOKEN"

# verify
curl -ik https://argocd.cnoe.localtest.me:8443/api/v1/session -d $'{"username":"admin","password":"password"}' -H "Content-Type:application/json"

The get secrets command looks at the initial admin secret generated by ArgoCD. If we are are to change passwords, we should make our own secrets and reference that instead.

@cmoulliard
Copy link
Contributor Author

The get secrets command looks at the initial admin secret generated by ArgoCD. If we are are to change passwords, we should make our own secrets and reference that instead.

We should then align argocd with gitea to name the secret using the same convention

secret name
gitea-credential
argocd-credential

@cmoulliard
Copy link
Contributor Author

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)

Is it the right place to put such post-installation step here as the code is part of a loop which is executed several times ....

@nabuskey
Copy link
Collaborator

nabuskey commented Dec 6, 2024

Probably here:

logger.V(1).Info("done installing core packages. passing control to argocd")

There's a possibility that ArgoCD is not yet reachable through Ingress-nginx. In that case, we need to retry a few times.

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.

Thanks for this. Looks like there are issues with build too. Please take a look.

return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil
}

logger.Info("Initial argocd admin secret found ...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a debug message. This gets printed every loop.

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: 86b26a2

@@ -89,6 +104,48 @@ func (r *LocalbuildReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
}

if r.Config.DevMode {
logger.Info("DevMode is enabled")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a debug message. This gets printed every loop.

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: 86b26a2

@@ -53,6 +53,7 @@ type BuildCustomizationSpec struct {
Port string `json:"port,omitempty"`
UsePathRouting bool `json:"usePathRouting,omitempty"`
SelfSignedCert string `json:"selfSignedCert,omitempty"`
DevMode bool `json:"devMode,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This field should be StaticPasswords. We need to make an effort to make what these fields do.

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: 35fc902

@@ -28,6 +28,7 @@ var (

type Build struct {
name string
devMode bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be staticPasswords

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: 35fc902

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still set as devMode

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

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.

I still think this needs to be the case.

Comment on lines 748 to 752
req, err := http.NewRequest("PUT", argocdEndpoint+"/account/password", bytes.NewBuffer(payloadBytes))
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", argocdSession.Token))
req.Header.Set("Content-Type", "application/json")

resp, err := client.Do(req)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Variable names shadow. I really do not like shadowing in longer blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better or do you expect something else 5830b22 ?

if resp.StatusCode == 200 {
// Password verification succeeded !
return nil, "succeeded"
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This else is not needed.

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: e4c78de

} else {
return fmt.Errorf("HTTP Error: %d", resp.StatusCode), "failed"
}
return nil, "failed"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the meaning of returning nil (indicating no error) but failed?

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: 700a841

@@ -43,7 +45,7 @@ func RawGiteaInstallResources(templateData any, config v1alpha1.PackageCustomiza
return k8s.BuildCustomizedManifests(config.FilePath, "resources/gitea/k8s", installGiteaFS, scheme, templateData)
}

func giteaAdminSecretObject() corev1.Secret {
func (r *LocalbuildReconciler) GiteaAdminSecretObject() corev1.Secret {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need to be a method. Better to move to the utils package because it's probably needed in the get command.

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 think about this f0754f7 ?

@@ -212,7 +215,7 @@ func getGiteaToken(ctx context.Context, baseUrl, username, password string) (str
return token.Token, nil
}

func giteaBaseUrl(config v1alpha1.BuildCustomizationSpec) string {
func (r *LocalbuildReconciler) GiteaBaseUrl(config v1alpha1.BuildCustomizationSpec) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't need to be a method and doesn't need to be exported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See: 3ab4fd3

… issues with log format of the messages

Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
…ssword which is only used internally

Signed-off-by: cmoulliard <[email protected]>
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.

It's fine to have the dev-mode and dev-password flags. It needs to be represented with clear intent internally in code. The flag names are user friendly but it isn't really descriptive of what it does when referring to the option internally.

When we think about the name dev-password or dev-mode, it's not clear what that means internally. Does dev-password mean we are relaxing password rules? e.g. no special char requirements. Does dev-mode mean we disable TLS?
What we want is to set a static password for all core packages. So we should make this intent clear in code.

@@ -28,6 +28,7 @@ var (

type Build struct {
name string
devMode bool
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still set as devMode

@@ -67,6 +69,7 @@ func init() {
CreateCmd.PersistentFlags().StringVar(&buildName, "build-name", "localdev", buildNameUsage)
CreateCmd.PersistentFlags().MarkDeprecated("build-name", "use --name instead.")
CreateCmd.PersistentFlags().StringVar(&buildName, "name", "localdev", buildNameUsage)
CreateCmd.PersistentFlags().BoolVar(&devMode, "dev-mode", false, devModeUsage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

add another flag here with the name dev-password.

Copy link
Contributor Author

@cmoulliard cmoulliard Dec 13, 2024

Choose a reason for hiding this comment

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

ok but then what will be the purpose of the flag dev-mode if now we have 2 flags: dev-mode and dev-password ? @nabuskey

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed devMode from build.go as non used: 1cd1773

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -143,6 +143,10 @@ func (r *RepositoryReconciler) reconcileGitRepo(ctx context.Context, repo *v1alp
return ctrl.Result{}, fmt.Errorf("getting git provider credentials: %w", err)
}

if r.Config.StaticPassword {
creds.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.

We use the string developer in different places. We should define it somewhere and reference it from everywhere it's used.

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: 570ca30

@@ -57,3 +66,20 @@ func (r *LocalbuildReconciler) ReconcileArgo(ctx context.Context, req ctrl.Reque
resource.Status.ArgoCD.Available = true
return ctrl.Result{}, nil
}

func (r *LocalbuildReconciler) ArgocdInitialAdminSecretObject() corev1.Secret {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be moved to utils.

return nil, "succeeded"
}

func (r *LocalbuildReconciler) updateArgocdDevPassword(ctx context.Context, adminPassword string) (error, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change the signature to (ctx context.Context, adminPassword string) error. If it tails for some reason, return an error. If there's an error while updating the password, we should not continue.

return nil, "succeeded"
}

func (r *LocalbuildReconciler) updateArgocdDevPassword(ctx context.Context, adminPassword string) (error, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change the name to updateArgocdPassword. The word dev doesn't mean much internally.

return string(sec.Data["password"]), nil
}

func (r *LocalbuildReconciler) updateGiteaDevPassword(ctx context.Context, adminPassword string) (error, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (r *LocalbuildReconciler) updateGiteaDevPassword(ctx context.Context, adminPassword string) (error, string) {
func (r *LocalbuildReconciler) updateGiteaPassword(ctx context.Context, adminPassword string) error {

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: e4831cd

// Gitea admin secret is not yet available ...
return ctrl.Result{RequeueAfter: defaultRequeueTime}, nil
}
logger.Info("Gitea admin secret found ...")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this a debug message. It's not useful for normal operations.

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: e4831cd

@@ -89,6 +104,48 @@ func (r *LocalbuildReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
}

if r.Config.StaticPassword {
logger.V(1).Info("Dev mode is enabled")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.V(1).Info("Dev mode is enabled")
logger.V(1).Info("static passwords enabled")

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: e4831cd

)

const (
ArgocdDevModePassword = "developer"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ArgocdDevModePassword = "developer"
ArgocdStaticPassword = "developer"

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

…e of the functions using it. Convert some messages to log debug messages

Signed-off-by: cmoulliard <[email protected]>
Signed-off-by: cmoulliard <[email protected]>
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.

3 participants