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

Commands add connect-serviceFabricCluster commands, add optional properties defaultValue and description #855

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jagilber
Copy link
Contributor

@jagilber jagilber commented May 23, 2023

Commands add connect-serviceFabricCluster commands
add optional property defaultValue

image

image

Copy link
Collaborator

@chensation chensation left a comment

Choose a reason for hiding this comment

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

This command is complex enough that an accompany cypress e2e test should be written for it.

let nodeTypes: INodeTypeInfo[] = []
try {
let XMLnodeTypes = manifest.getElementsByTagName("NodeTypes")[0].getElementsByTagName("NodeType");
private getNodeCertificatesProperty(xmlNode: Element) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use the values from the NodeType section, instead, use the thumbprints found inside the Security section and the common names found inside the Security/AdminClientX509Names section, the Security/ServerX509Names section and the Security/ClientX509Names

Copy link
Collaborator

@chensation chensation Jun 1, 2023

Choose a reason for hiding this comment

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

There could be multiple certificates. For thumbprints, they would be in the format of a comma separated list, for common names, there would be multiple parameters inside each corresponding section

});
const findValueThumbprint = new PowershellCommandParameter("FindValue", CommandParamTypes.enum, {
required: true,
options: [...new Set(clientThumbprints.concat(clusterThumbprints))],
Copy link
Collaborator

Choose a reason for hiding this comment

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

for findValue, use client certificate and admin certificate, not cluster certificate


const serverThumbprint = new PowershellCommandParameter("ServerCertThumbprint", CommandParamTypes.enum, {
required: true,
options: [...new Set(serverThumbprints)],
Copy link
Collaborator

Choose a reason for hiding this comment

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

There could be a secondary Server certificates listed, but only one of them would work. Since the ServerCertificate parameters accept a string array, put all of the certificates in as one value

defaultValue: clientCommonNames[0] ?? clusterCommonNames[0]
});

if (clientCommonNames.length > 0 || serverCommonNames.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For Server Certificate, it is either common name or thumbprint. However, for Client Certificates, both common name and thumbprint could exist at the same time. So maybe instead of two separate commands, it's better to have one command that already has the server certificate parameter defined, then allow the user to choose what kind of client certificate to use.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unsecure clusters that don't have any certificates should also be supported.

const findValueCommonName = new PowershellCommandParameter("FindValue", CommandParamTypes.enum, {
required: true,
options: [...new Set(clientCommonNames.concat(clusterCommonNames))],
defaultValue: clientCommonNames[0] ?? clusterCommonNames[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

for findValue, enable allowCustomValues so users can input their own certificate information.

'Creates a connection to a Service Fabric cluster using certificate thumbprint.',
connectHelp,
CommandSafetyLevel.safe,
`Connect-ServiceFabricCluster ${authType}-StoreName My -FindType FindByThumbprint`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

When AAD is enabled, both FindType and FindValue are not needed, only Server Certificate is needed

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