-
Notifications
You must be signed in to change notification settings - Fork 459
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
Switch to embedded console for MinIO Tenant #739
Conversation
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.
No CONSOLE_ envs are honored everything should be MINIO_
Also update the version to latest release done last night
Addressed all the comments. |
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.
Rest of the changes look good - needs testing
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.
on an upgrade situation, since we don't have a validation against the spec of the service for Console, we don't update it, which we should, at least the selector, else tenants console service will end up in a bad state.
Same, we should add an upgrade situation where we remove the console deployment
Existing console fields are now deprecated and will be removed moving forward. Also upgrade to latest MinIO release.
@@ -232,10 +232,10 @@ const LogAuditTokenKey = "LOGSEARCH_AUDIT_AUTH_TOKEN" | |||
|
|||
// LogQueryTokenKey is the k8s secret/environment variable key name referring to | |||
// the token used to perform search query on audit logs persisted. | |||
const LogQueryTokenKey = "LOGSEARCH_QUERY_AUTH_TOKEN" | |||
const LogQueryTokenKey = "MINIO_QUERY_AUTH_TOKEN" |
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.
Do we really need to rename these? now we will need to also plan an upgrade path for secrets @nitisht @harshavardhana
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.
We do not need to rename these, but we need to make sure to set the right environment variables for MinIO deployment when its considered an "upgrade"
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.
Also the ENV is wrong
~ git grep MINIO_LOG
cmd/common-main.go: if value := env.Get("MINIO_LOG_QUERY_URL", ""); value != "" {
cmd/common-main.go: if value := env.Get("MINIO_LOG_QUERY_AUTH_TOKEN", ""); value != "" {
@nitisht did you test this change because it wouldn't work with logsearch API when auth token is configured.
@@ -719,16 +719,16 @@ func (t *Tenant) CreateUsers(madmClnt *madmin.AdminClient, userCredentialSecrets | |||
ctx, cancel := context.WithTimeout(context.Background(), time.Second*20) | |||
defer cancel() | |||
for _, secret := range userCredentialSecrets { | |||
consoleAccessKey, ok := secret.Data["CONSOLE_ACCESS_KEY"] | |||
consoleAccessKey, ok := secret.Data["MINIO_ACCESS_KEY"] |
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.
if we are doing this change, perhaps we can drop the MINIO_
prefix for these two two avoid it getting confused with the root credentials? what do you think @harshavardhana
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.
Yes we should drop it and not use the older MINIO_ instead it should be generic like
ACCESS_KEY
@harshavardhana @nitisht 751 included these commits |
Existing console fields are now deprecated and will be removed
moving forward.