Skip to content
This repository has been archived by the owner on Apr 13, 2020. It is now read-only.

[HOUSEKEEPING] Remove/Comment Out resources related to key vault for … #458

Merged
merged 5 commits into from
Mar 26, 2020
Merged

[HOUSEKEEPING] Remove/Comment Out resources related to key vault for … #458

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d651a5b
[HOUSEKEEPING] Remove/Comment Out resources related to key vault for …
dennisseah Mar 26, 2020
3c945f1
remove commented out code
dennisseah Mar 26, 2020
674601e
Update onboard.test.ts
dennisseah Mar 26, 2020
7bf8e5c
Merge branch 'master' into issue1243
dennisseah Mar 26, 2020
0336ab5
Merge branch 'master' into issue1243
andrebriggs Mar 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions guides/service-introspection.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,11 @@ introspection:
table_name: "table-name"
partition_key: "partition-key"
key: "storage-access-key"
service_principal_id: "service-principal-id"
service_principal_secret: "service-principal-secret"
subscription_id: "subscription-id"
tenant_id: "tenant-id"
resource-group: "resource-group-name"
```

## Prerequisites

1. Service principal with owner access.
[Create a service principal with owner access.](#service-principal)
2. Optionally, Azure Key Vault can be used to securely store and tightly control
access to tokens, passwords, API keys, and other secrets
[How to create key vault](https://docs.microsoft.com/en-us/azure/key-vault/quick-create-cli).
3. Give the service principal get and list access. Follow step 2 from
[these instructions](https://docs.microsoft.com/en-us/azure/devops/pipelines/library/variable-groups?view=azure-devops&tabs=yaml#link-secrets-from-an-azure-key-vault).
To create storage-account and table, use the `spk deployment onboard` command to
create them where subscription Id, resource group name, service principal Id,
password and tenant Id are required.

### Service Principal

Expand Down
4 changes: 0 additions & 4 deletions src/commands/deployment/onboard.decorator.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
"description": "Name of the resource group to create new storage account when it does not exist",
"required": true
},
{
"arg": "-k, --key-vault-name <key-vault-name>",
"description": "Name of the Azure key vault"
},
{
"arg": "--service-principal-id <service-principal-id>",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are service-principal-id, service-principal-password, tenant-id and subscription-id still being used elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the onboard command these are used to create account service and table. Isn't this the case?

Copy link
Collaborator

@samiyaakhtar samiyaakhtar Mar 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The presence of these in config file makes them look mandatory for other commands as well. Perhaps in future we should revisit whether or not the config file should have four of these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are not in config file, these are option values for onboard command only

Copy link
Collaborator

Choose a reason for hiding this comment

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

they're also in config file: https://github.com/CatalystCode/spk/blob/master/spk-config.yaml#L32

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i missed these, should I remove them to avoid confusion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for now we can leave them since I believe the onboard command takes them either through config file or through the command itself. Although since it's a one time command, it shouldn't rely on the config file. I'll open a separate discussion for that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

The service principal related options are used to authenticate with Azure to create storage account and table if they don't exist. Deleting these options make this command not consistent with other commands to pass all required values without depending on config.yaml.

"description": "Azure service principal id with `contributor` role in Azure Resource Group",
Expand Down
45 changes: 2 additions & 43 deletions src/commands/deployment/onboard.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import yaml from "js-yaml";
import * as path from "path";
import { Config, loadConfiguration } from "../../config";
import * as config from "../../config";
import * as keyvault from "../../lib/azure/keyvault";
import * as storage from "../../lib/azure/storage";
import { createTempDir } from "../../lib/ioUtil";
import { deepClone } from "../../lib/util";
Expand All @@ -15,7 +14,6 @@ import {
} from "../../logger";
import { AzureAccessOpts, ConfigYaml } from "../../types";
import {
createKeyVault,
execute,
getStorageAccessKey,
CommandOptions,
Expand All @@ -37,7 +35,6 @@ afterAll(() => {
});

const MOCKED_VALUES: CommandOptions = {
keyVaultName: "testKeyVault",
servicePrincipalId: "servicePrincipalId",
servicePrincipalPassword: "servicePrincipalPassword",
storageAccountName: "testaccount",
Expand All @@ -49,7 +46,6 @@ const MOCKED_VALUES: CommandOptions = {
};

const MOCKED_CONFIG: OnBoardConfig = {
keyVaultName: "testKeyVault",
servicePrincipalId: "servicePrincipalId",
servicePrincipalPassword: "servicePrincipalPassword",
storageAccountName: "testaccount",
Expand Down Expand Up @@ -108,7 +104,6 @@ const testPopulatedVal = (
describe("test populateValues", () => {
it("all values are set", () => {
const values = populateValues(getMockedValues());
expect(values.keyVaultName).toBe(MOCKED_VALUES.keyVaultName);
expect(values.servicePrincipalId).toBe(MOCKED_VALUES.servicePrincipalId);
expect(values.servicePrincipalPassword).toBe(
MOCKED_VALUES.servicePrincipalPassword
Expand Down Expand Up @@ -152,19 +147,6 @@ describe("test populateValues", () => {
}
);
});
it("keyVaultName default to config.introspection.azure.key_vault_name", () => {
testPopulatedVal(
(configYaml: ConfigYaml) => {
configYaml.key_vault_name = "KeyVaultName";
},
(values: CommandOptions) => {
values.keyVaultName = undefined;
},
(values) => {
expect(values.keyVaultName).toBe("KeyVaultName");
}
);
});
it("servicePrincipalId default to config.introspection.azure.service_principal_id", () => {
testPopulatedVal(
(configYaml: ConfigYaml) => {
Expand Down Expand Up @@ -316,37 +298,17 @@ describe("test validateAndCreateStorageAccount function", () => {

describe("test getStorageAccessKey function", () => {
it("already exist", async () => {
jest
.spyOn(storage, "getStorageAccountKey")
.mockReturnValueOnce(Promise.resolve("key"));
jest.spyOn(storage, "getStorageAccountKey").mockResolvedValueOnce("key");
const values = getMockedConfig();
const accessOpts = getMockedAccessOpts(values);
const storageKey = await getStorageAccessKey(values, accessOpts);
expect(storageKey).toBe("key");
});
});

describe("test createKeyVault function", () => {
it("[+ve] not key vault value", async () => {
const values = getMockedConfig();
values.keyVaultName = undefined;
const accessOpts = getMockedAccessOpts(values);
// nothing is done
await createKeyVault(values, accessOpts, "accessString");
});
it("[+ve] with key vault value", async () => {
jest.spyOn(keyvault, "setSecret").mockReturnValueOnce(Promise.resolve());
const values = getMockedConfig();
const accessOpts = getMockedAccessOpts(values);
await createKeyVault(values, accessOpts, "accessString");
});
});

describe("onboard", () => {
test("empty location", async () => {
jest
.spyOn(storage, "isStorageAccountExist")
.mockReturnValueOnce(Promise.resolve(false));
jest.spyOn(storage, "isStorageAccountExist").mockResolvedValueOnce(false);

try {
const values = getMockedConfig();
Expand Down Expand Up @@ -392,9 +354,6 @@ describe("onboard", () => {
jest
.spyOn(storage, "createStorageAccount")
.mockReturnValueOnce(Promise.resolve({ location: "test" }));
jest
.spyOn(onboardImpl, "createKeyVault")
.mockReturnValueOnce(Promise.resolve());
jest.spyOn(onboardImpl, "setConfiguration").mockReturnValueOnce(true);

const data = {
Expand Down
47 changes: 2 additions & 45 deletions src/commands/deployment/onboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import commander from "commander";
import fs from "fs";
import yaml from "js-yaml";
import { Config, defaultConfigFile, readYaml } from "../../config";
import { setSecret } from "../../lib/azure/keyvault";
import {
createStorageAccount,
createTableIfNotExists,
Expand All @@ -29,7 +28,6 @@ export interface CommandOptions {
storageTableName: string | undefined;
storageLocation: string | undefined;
storageResourceGroupName: string | undefined;
keyVaultName: string | undefined;
servicePrincipalId: string | undefined;
servicePrincipalPassword: string | undefined;
tenantId: string | undefined;
Expand All @@ -44,7 +42,6 @@ export interface OnBoardConfig {
servicePrincipalPassword: string;
subscriptionId: string;
tenantId: string;
keyVaultName?: string;
storageLocation?: string;
}

Expand All @@ -61,7 +58,6 @@ export const populateValues = (opts: CommandOptions): CommandOptions => {
opts.storageAccountName || azure?.account_name || undefined;
opts.storageTableName =
opts.storageTableName || azure?.table_name || undefined;
opts.keyVaultName = opts.keyVaultName || config.key_vault_name || undefined;
opts.servicePrincipalId =
opts.servicePrincipalId || azure?.service_principal_id || undefined;
opts.servicePrincipalPassword =
Expand Down Expand Up @@ -111,7 +107,6 @@ export const validateValues = (opts: CommandOptions): OnBoardConfig => {
servicePrincipalPassword: opts.servicePrincipalPassword || "",
subscriptionId: opts.subscriptionId || "",
tenantId: opts.tenantId || "",
keyVaultName: opts.keyVaultName,
storageLocation: opts.storageLocation,
};
};
Expand Down Expand Up @@ -207,45 +202,9 @@ export const getStorageAccessKey = async (
return accessKey;
};

/**
* Creates Key Vault if value from commander has value for `keyVaultName`
*
* @param values values from commander
* @param accessOpts Azure Access Opts
* @param accessKey Access Key
*/
export const createKeyVault = async (
values: OnBoardConfig,
accessOpts: AzureAccessOpts,
accessKey: string
): Promise<void> => {
// if key vault is not specified, exit without reading storage account
// key and setting it in the key vault
if (values.keyVaultName) {
logger.debug(
`Calling setSecret with storage account primary key ***
and ${values.keyVaultName}`
);
await setSecret(
values.keyVaultName,
`${values.storageAccountName}Key`,
accessKey,
accessOpts
);
} else {
// notify the user to set the environment variable with storage access key
logger.info(
`Please set the storage account access key in environment variable
INTROSPECTION_STORAGE_ACCESS_KEY before issuing any deployment commands.`
);
logger.info(`Storage account ${values.storageAccountName} access key: ***`);
}
};

/**
* Creates the Storage account `accountName` in resource group `resourceGroup`,
* sets storage account access key in keyvalut, and updates pipelines
* (acr-hld, hld->manifests)
* and updates pipelines (acr-hld, hld->manifests)
*
* @param values Values from commander.
*/
Expand All @@ -254,7 +213,7 @@ export const onboard = async (
): Promise<StorageAccount | undefined> => {
logger.debug(
`onboard called with ${values.storageAccountName}, ${values.storageTableName},
${values.storageResourceGroupName}, ${values.storageLocation}, and ${values.keyVaultName}`
${values.storageResourceGroupName} and ${values.storageLocation}`
);

const accessOpts: AzureAccessOpts = {
Expand Down Expand Up @@ -288,8 +247,6 @@ export const onboard = async (
table ${values.storageTableName} exist.`);
}

await createKeyVault(values, accessOpts, accessKey);

// save storage account and table names in configuration
setConfiguration(values.storageAccountName, values.storageTableName);
return storageAccount;
Expand Down