Skip to content

fix(cluster connect): display service account credentials#1237

Merged
rkpattnaik780 merged 2 commits intomainfrom
cluster_connect_log_svc
Oct 19, 2021
Merged

fix(cluster connect): display service account credentials#1237
rkpattnaik780 merged 2 commits intomainfrom
cluster_connect_log_svc

Conversation

@rkpattnaik780
Copy link
Contributor

  • Output client id and client secret of generated service account.
  • Missing i18n key error

Verification Steps

  1. Execute cluster connect command
./rhoas cluster connect --service-type kafka --service-name rama-test-2 
  1. It should display the command required to grant permissions to the generated service account.

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)

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

Copy link
Contributor

@craicoverflow craicoverflow left a comment

Choose a reason for hiding this comment

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

All good, but we should not print client secret IMO.

cliOpts.Logger.Info(cliOpts.Localizer.MustLocalize("cluster.kubernetes.createSASecret.log.info.createSuccess",
localize.NewEntry("Name", createdSecret.Name),
localize.NewEntry("ClientID", serviceAcct.GetClientId()),
localize.NewEntry("ClientSecret", serviceAcct.GetClientSecret()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should print the client secret for any reason. The CLI user only needs the client ID for ACL 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.

Though it may be irrelevant for cluster context, I think it will be useful if user needs a svc-account created like this for other purposes. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's risky, it is printing a password that gives access to data. Default to safe, if people need it we can propose another solution. This is why the service-account commands do not print anything and you must specify a file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Taking it down.

Service Account Secret "{{.Name}}" created successfully

Client ID: {{.ClientID}}
Client Secret: {{.ClientSecret}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can omit that


Execute the following command to grant access to the service-account using rhoas cli

rhoas kafka acl grant-access --producer --consumer --service-account {{.ClientID}} --topic "*" --group "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we externalize this as a variable, we could highlight it in colour to make it easier to identify and keep separate from text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or simply indent it to differentiate.

@rkpattnaik780 rkpattnaik780 merged commit 6a90fd8 into main Oct 19, 2021
@rkpattnaik780 rkpattnaik780 deleted the cluster_connect_log_svc branch October 19, 2021 15:24
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