-
Notifications
You must be signed in to change notification settings - Fork 62
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
base: main
Are you sure you want to change the base?
Changes from 12 commits
3a974cb
a9c683f
6d85851
8dedc1e
704bc78
ee9dc06
d392287
0c62564
1e1b980
3d884da
f31a08e
c355ba1
8158551
78e1e40
7d81b27
a8c3016
80a785b
3c3511d
bf8e2cc
f76db50
86b26a2
35fc902
e4c78de
700a841
5830b22
d1ed81a
f0754f7
3ab4fd3
c8ac946
e02a5d2
f67a91c
dbf56c2
d029f62
570ca30
2fe31a3
e4831cd
1cd1773
70e3d37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For gitea when using dev-mode, we should still use as user: giteaAdmin and password = developer There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
g, developer, role:developer |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ var ( | |
|
||
type Build struct { | ||
name string | ||
devMode bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: 35fc902 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still set as |
||
cfg v1alpha1.BuildCustomizationSpec | ||
kindConfigPath string | ||
kubeConfigPath string | ||
|
@@ -278,5 +279,6 @@ func isBuildCustomizationSpecEqual(s1, s2 v1alpha1.BuildCustomizationSpec) bool | |
s1.IngressHost == s2.IngressHost && | ||
s1.Port == s2.Port && | ||
s1.UsePathRouting == s2.UsePathRouting && | ||
s1.SelfSignedCert == s2.SelfSignedCert | ||
s1.SelfSignedCert == s2.SelfSignedCert && | ||
s1.DevMode == s2.DevMode | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this is the case, then we should install gitea and argocd using a non generated password ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What about using the the parameter: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am ok with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finally we aligned our paths => I will then use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be clear, I meant to have two flags. ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair is the The only argument I could maybe gather is that since it is no longer than admin credentials and it is developer credentials that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I still think this needs to be the case. |
||
kubeVersionUsage = "Version of the kind kubernetes cluster to create." | ||
extraPortsMappingUsage = "List of extra ports to expose on the docker container and kubernetes cluster as nodePort " + | ||
"(e.g. \"22:32222,9090:39090,etc\")." | ||
|
@@ -40,6 +41,7 @@ var ( | |
// Flags | ||
recreateCluster bool | ||
buildName string | ||
devMode bool | ||
kubeVersion string | ||
extraPortsMapping string | ||
kindConfigPath string | ||
|
@@ -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", false, devModeUsage) | ||
CreateCmd.PersistentFlags().StringVar(&kubeVersion, "kube-version", "v1.30.3", kubeVersionUsage) | ||
CreateCmd.PersistentFlags().StringVar(&extraPortsMapping, "extra-ports", "", extraPortsMappingUsage) | ||
CreateCmd.PersistentFlags().StringVar(&kindConfigPath, "kind-config", "", kindConfigPathUsage) | ||
|
@@ -143,6 +146,7 @@ func create(cmd *cobra.Command, args []string) error { | |
IngressHost: ingressHost, | ||
Port: port, | ||
UsePathRouting: pathRouting, | ||
DevMode: devMode, | ||
}, | ||
|
||
CustomPackageDirs: absDirPaths, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,6 +3,13 @@ package localbuild | |||||||||||||||||||||||||||||
import ( | ||||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||||
"embed" | ||||||||||||||||||||||||||||||
"encoding/json" | ||||||||||||||||||||||||||||||
"fmt" | ||||||||||||||||||||||||||||||
"golang.org/x/crypto/bcrypt" | ||||||||||||||||||||||||||||||
v1 "k8s.io/api/core/v1" | ||||||||||||||||||||||||||||||
"k8s.io/apimachinery/pkg/types" | ||||||||||||||||||||||||||||||
"sigs.k8s.io/controller-runtime/pkg/client" | ||||||||||||||||||||||||||||||
"time" | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
"github.com/cnoe-io/idpbuilder/api/v1alpha1" | ||||||||||||||||||||||||||||||
"github.com/cnoe-io/idpbuilder/globals" | ||||||||||||||||||||||||||||||
|
@@ -15,6 +22,14 @@ import ( | |||||||||||||||||||||||||||||
//go:embed resources/argo/* | ||||||||||||||||||||||||||||||
var installArgoFS embed.FS | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
const ( | ||||||||||||||||||||||||||||||
argocdDevModePassword = "developer" | ||||||||||||||||||||||||||||||
argocdAdminSecretName = "argocd-secret" | ||||||||||||||||||||||||||||||
argocdInitialAdminSecretName = "argocd-initial-admin-secret" | ||||||||||||||||||||||||||||||
argocdInitialAdminPasswordKey = "argocd-initial-admin-secret" | ||||||||||||||||||||||||||||||
argocdNamespace = "argocd" | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I will review There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
func RawArgocdInstallResources(templateData any, config v1alpha1.PackageCustomization, scheme *runtime.Scheme) ([][]byte, error) { | ||||||||||||||||||||||||||||||
return k8s.BuildCustomizedManifests(config.FilePath, "resources/argo", installArgoFS, scheme, templateData) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
@@ -53,7 +68,74 @@ func (r *LocalbuildReconciler) ReconcileArgo(ctx context.Context, req ctrl.Reque | |||||||||||||||||||||||||||||
if result, err := argocd.Install(ctx, resource, r.Client, r.Scheme, r.Config); err != nil { | ||||||||||||||||||||||||||||||
return result, err | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
resource.Status.ArgoCD.Available = true | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Let's patch the existing argocd admin secret if devmode is enabled to set the default password | ||||||||||||||||||||||||||||||
if r.Config.DevMode { | ||||||||||||||||||||||||||||||
// Hash password using bcrypt | ||||||||||||||||||||||||||||||
password := argocdDevModePassword | ||||||||||||||||||||||||||||||
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(password), 0) | ||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
return ctrl.Result{}, fmt.Errorf("Error hashing password: %w", err) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Prepare the patch for the Secret's `stringData` field | ||||||||||||||||||||||||||||||
patchData := map[string]interface{}{ | ||||||||||||||||||||||||||||||
"stringData": map[string]string{ | ||||||||||||||||||||||||||||||
"accounts.developer.password": string(hashedPassword), | ||||||||||||||||||||||||||||||
"accounts.developer.passwordMtime": time.Now().Format(time.RFC3339), | ||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
// Convert patch data to JSON | ||||||||||||||||||||||||||||||
patchBytes, err := json.Marshal(patchData) | ||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
return ctrl.Result{}, fmt.Errorf("Error marshalling patch data: %w", err) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
kubeClient, err := k8s.GetKubeClient() | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 A second job could also patch existing resources if by example we pass a different DNS domain = etc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. flag renamed from "dev" to "dev-mode". See: a8c3016 |
||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
return ctrl.Result{}, fmt.Errorf("getting kube client: %w", err) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Getting the argocd-secret | ||||||||||||||||||||||||||||||
s := v1.Secret{} | ||||||||||||||||||||||||||||||
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) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use server side apply instead here to simplify this process. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I will review There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: idpbuilder/pkg/controllers/localbuild/gitea.go Lines 163 to 166 in f31a08e
Here's another example: Lines 78 to 86 in d14289a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. See: 80a785b |
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/* | ||||||||||||||||||||||||||||||
This is not needed as we will not generate a new admin password | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
adminSecret := v1.Secret{ | ||||||||||||||||||||||||||||||
TypeMeta: metav1.TypeMeta{ | ||||||||||||||||||||||||||||||
Kind: "Secret", | ||||||||||||||||||||||||||||||
APIVersion: "v1", | ||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||
ObjectMeta: metav1.ObjectMeta{ | ||||||||||||||||||||||||||||||
Name: argocdInitialAdminSecretName, | ||||||||||||||||||||||||||||||
Namespace: argocdNamespace, | ||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||
StringData: map[string]string{ | ||||||||||||||||||||||||||||||
argocdInitialAdminPasswordKey: argocdDevModePassword, | ||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
// Re-creating the initial admin password secret: argocd-initial-admin-secret as used with "idpbuilder get secrets -p argocd" | ||||||||||||||||||||||||||||||
err = kubeClient.Create(ctx, &adminSecret) | ||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||
return ctrl.Result{}, fmt.Errorf("Error creating the initial admin secret: %w", err) | ||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||
return ctrl.Result{}, nil | ||||||||||||||||||||||||||||||
}*/ | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return ctrl.Result{}, nil | ||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,22 @@ package k8s | |
|
||
import ( | ||
"embed" | ||
"k8s.io/client-go/rest" | ||
"k8s.io/client-go/tools/clientcmd" | ||
"k8s.io/client-go/util/homedir" | ||
"os" | ||
"path/filepath" | ||
ctrl "sigs.k8s.io/controller-runtime" | ||
|
||
"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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed with: 8158551 |
||
) | ||
|
||
func BuildCustomizedManifests(filePath, fsPath string, resourceFS embed.FS, scheme *runtime.Scheme, templateData any) ([][]byte, error) { | ||
rawResources, err := util.ConvertFSToBytes(resourceFS, fsPath, templateData) | ||
if err != nil { | ||
|
@@ -58,3 +67,33 @@ func applyOverrides(filePath string, originalFiles [][]byte, scheme *runtime.Sch | |
|
||
return ConvertYamlToObjectsWithOverride(scheme, originalFiles, rendered) | ||
} | ||
|
||
func GetKubeConfig(kubeConfigPath ...string) (*rest.Config, error) { | ||
// Set default path if no path is provided | ||
path := filepath.Join(homedir.HomeDir(), ".kube", "config") | ||
|
||
if len(kubeConfigPath) > 0 { | ||
path = kubeConfigPath[0] | ||
} | ||
|
||
kubeConfig, err := clientcmd.BuildConfigFromFlags("", path) | ||
if err != nil { | ||
setupLog.Error(err, "Error building kubeconfig from kind cluster") | ||
return nil, err | ||
} | ||
return kubeConfig, nil | ||
} | ||
|
||
func GetKubeClient(kubeConfigPath ...string) (client.Client, error) { | ||
kubeCfg, err := GetKubeConfig(kubeConfigPath...) | ||
if err != nil { | ||
setupLog.Error(err, "Error getting kubeconfig") | ||
return nil, err | ||
} | ||
kubeClient, err := client.New(kubeCfg, client.Options{Scheme: GetScheme()}) | ||
if err != nil { | ||
setupLog.Error(err, "Error creating kubernetes client") | ||
return nil, err | ||
} | ||
return kubeClient, nil | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 35fc902