WIP: feat(generate-config): create config with existing service account #1588

rkpattnaik780 · 2022-06-05T07:05:45Z

rhoas generate-config should not create service-account.

Verification Steps

Generate configuration by specifying --type to be configmap

rhoas generate-config --type=configmap

It should create a configmap object.
Unlike previous version, rhoas generate-config shouldn't create a service account.

ToDo:

Add validation for flag combinations
Add validation to check if the service account credentials provided are valid
Add a message for the breaking change

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify)

wtrocki · 2022-06-05T10:52:27Z

Service accounts (generally any credentials) cannot be passed thru flags due to them being saved in plain form to bash history. We had initial design for that case in docs. Idea was to use json file provided from service account command. WDYT?

rkpattnaik780 · 2022-06-06T04:32:14Z

We had initial design for that case in docs. Idea was to use json file provided from service account command. WDYT?

I was planning to go with it in the beginning, but it occurred to me that credntial can be in some other format too, for which user wll have to reconstruct it, that could be bit time taking, hence dropped the idea to have it as main method to get the credentials.

Service accounts (generally any credentials) cannot be passed thru flags due to them being saved in plain form to bash history.

I will agree, hence there are twoflags - --client-secret-stdin that will take up the secret from standard input and --client-secret to pass as argument for CI/CD environment. WDYT?

wtrocki · 2022-06-06T07:49:54Z

We can use json file as input and env variables presence(user running .env file) Do we need more options?

wtrocki · 2022-06-06T07:58:33Z

some other format too

I would not be worried about it because:

Json is default
User will generate service account specifically for this purpose
We will add validation and description
guide/docs will reflect it

wtrocki · 2022-06-06T08:10:26Z

Since we designed that command many things changed:

we now have secret in service accounts create as output

I'm wondering if we actually approached problem right:

Do we really want to supply users service account?
We would have that already in user desired format. Maybe we just want to stop creating service account as part of generate command?

That will have numerous benefits:

Generate can be called many times without side effects and paired with service account file
We can better reflect openshift story:
Credentials would be in secret but urls can be configmap

wtrocki · 2022-06-10T10:11:21Z

@rkpattnaik780 we need to put some research on this as there are different opinions how we should do it.

Mainly there is big trend in current users to disable sa creation in context generate command.
Another option is to provide flag for that use case etc. We might need to drive some proper pros and cons and see how this affects our future roadmap (helm etc.)

wtrocki · 2022-06-10T10:12:37Z

FYI (low priority) @jackdelahunt @dimakis

rkpattnaik780 · 2022-07-21T13:38:17Z

pkg/cmd/generate/templates.go

 	`)

-	templateSecret = heredoc.Doc(`
+	templateConfigMap = heredoc.Doc(`


@wtrocki
This PR changes generate-config command to skip creating a service account. And replaces secret with configmap.

wtrocki · 2022-07-21T13:41:29Z

Is that good to be verified?

rkpattnaik780 · 2022-07-21T13:48:19Z

Is that good to be verified?

Yes, I will be updating help texts and examples in a while.

wtrocki · 2022-07-21T13:52:08Z

pkg/cmd/generate/configurations.go

+	envConfig             = template.Must(template.New(envFormat).Parse(templateEnv))
+	jsonConfig            = template.Must(template.New(jsonFormat).Parse(templateJSON))
+	propertiesConfig      = template.Must(template.New(propertiesFormat).Parse(templateProperties))
+	congMapTemplateConfig = template.Must(template.New(configmapFormat).Parse(templateConfigMap))


;)

Suggested change

congMapTemplateConfig = template.Must(template.New(configmapFormat).Parse(templateConfigMap))

kongMapTemplateConfig = template.Must(template.New(configmapFormat).Parse(templateConfigMap))

wtrocki · 2022-07-21T13:54:52Z

I think we might need to add some documentation/msg. Something like:

Generate config finished successfully. You can now create new service accounts in order to connect to the service

That will smoothen backwards compatibility issue

wtrocki · 2022-07-22T01:23:06Z

pkg/core/localize/locales/en/cmd/generate_config.en.toml

+Configurations successfully saved to "{{.FilePath}}"
+
+You can now create new service accounts or use existing ones to connect to the service(s)


As user I'm wondering how.
Can we be more specific?.

Run rhoas service-account create etc.

jackdelahunt · 2022-07-22T11:05:07Z

Verification steps work 👍

…ce account (#1588)" This reverts commit 3a558a9.

…ce account (#1588)" This reverts commit 3a558a9. Signed-off-by: Ramakrishna Pattnaik <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress label Jun 5, 2022

wtrocki closed this Jun 6, 2022

wtrocki reopened this Jun 6, 2022

rkpattnaik780 added 2 commits July 21, 2022 15:37

feat(generate-config): create config with existing service account

295e705

docs: generate docs

200f22f

rkpattnaik780 force-pushed the default_generate branch from 4e94580 to 1d47e67 Compare July 21, 2022 13:29

feat(generate-config): replace secret with configmap

c7ea1ac

rkpattnaik780 force-pushed the default_generate branch from 1d47e67 to c7ea1ac Compare July 21, 2022 13:31

rkpattnaik780 commented Jul 21, 2022

View reviewed changes

wtrocki reviewed Jul 21, 2022

View reviewed changes

wtrocki approved these changes Jul 21, 2022

View reviewed changes

fix: rename variables and update docs

86b1f34

wtrocki reviewed Jul 22, 2022

View reviewed changes

wtrocki approved these changes Jul 22, 2022

View reviewed changes

wtrocki marked this pull request as ready for review July 22, 2022 10:34

rkpattnaik780 requested a review from jackdelahunt July 22, 2022 10:38

wtrocki merged commit 3a558a9 into main Jul 22, 2022

wtrocki deleted the default_generate branch July 22, 2022 11:21

rkpattnaik780 mentioned this pull request Jul 25, 2022

fix(generate-config): update success message and default path for env #1676

Merged

5 tasks

rkpattnaik780 added a commit that referenced this pull request Jul 26, 2022

Revert "WIP: feat(generate-config): create config with existing servi…

28dec32

…ce account (#1588)" This reverts commit 3a558a9.

rkpattnaik780 added a commit that referenced this pull request Jul 26, 2022

Revert "WIP: feat(generate-config): create config with existing servi…

3446c44

…ce account (#1588)" This reverts commit 3a558a9. Signed-off-by: Ramakrishna Pattnaik <[email protected]>

rkpattnaik780 mentioned this pull request Aug 1, 2022

chore: revert the context for revert #1691

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat(generate-config): create config with existing service account #1588

WIP: feat(generate-config): create config with existing service account #1588

rkpattnaik780 commented Jun 5, 2022 •

edited

Loading

wtrocki commented Jun 5, 2022 •

edited

Loading

rkpattnaik780 commented Jun 6, 2022

wtrocki commented Jun 6, 2022

wtrocki commented Jun 6, 2022

wtrocki commented Jun 6, 2022 •

edited

Loading

wtrocki commented Jun 10, 2022

wtrocki commented Jun 10, 2022

rkpattnaik780 Jul 21, 2022

wtrocki commented Jul 21, 2022

rkpattnaik780 commented Jul 21, 2022

wtrocki Jul 21, 2022

wtrocki commented Jul 21, 2022 •

edited

Loading

wtrocki Jul 22, 2022 •

edited

Loading

jackdelahunt commented Jul 22, 2022

	congMapTemplateConfig = template.Must(template.New(configmapFormat).Parse(templateConfigMap))
	kongMapTemplateConfig = template.Must(template.New(configmapFormat).Parse(templateConfigMap))

		Configurations successfully saved to "{{.FilePath}}"

		You can now create new service accounts or use existing ones to connect to the service(s)

WIP: feat(generate-config): create config with existing service account #1588

WIP: feat(generate-config): create config with existing service account #1588

Conversation

rkpattnaik780 commented Jun 5, 2022 • edited Loading

Verification Steps

ToDo:

Type of change

wtrocki commented Jun 5, 2022 • edited Loading

rkpattnaik780 commented Jun 6, 2022

wtrocki commented Jun 6, 2022

wtrocki commented Jun 6, 2022

wtrocki commented Jun 6, 2022 • edited Loading

wtrocki commented Jun 10, 2022

wtrocki commented Jun 10, 2022

rkpattnaik780 Jul 21, 2022

Choose a reason for hiding this comment

wtrocki commented Jul 21, 2022

rkpattnaik780 commented Jul 21, 2022

wtrocki Jul 21, 2022

Choose a reason for hiding this comment

wtrocki commented Jul 21, 2022 • edited Loading

wtrocki Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

jackdelahunt commented Jul 22, 2022

rkpattnaik780 commented Jun 5, 2022 •

edited

Loading

wtrocki commented Jun 5, 2022 •

edited

Loading

wtrocki commented Jun 6, 2022 •

edited

Loading

wtrocki commented Jul 21, 2022 •

edited

Loading

wtrocki Jul 22, 2022 •

edited

Loading