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.

Sign up for GitHub

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

Jump to bottom

HPCC-27830 Azure LogAccess use Secrets #16245

Merged
merged 1 commit into from Jun 28, 2022
Merged

HPCC-27830 Azure LogAccess use Secrets #16245

merged 1 commit into from Jun 28, 2022

Conversation

rpastrana
Copy link
Member

@rpastrana rpastrana commented Jun 22, 2022 •
edited

  • Adds esp jsecret category
  • Eliminates use of Env vars
  • Stablishes azure_logaccess secret
  • Updates documentation
  • Provides secrets template

Signed-off-by: Rodrigo Pastrana rodrigo.pastrana@lexisnexisrisk.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

@github-actions
Copy link

https://track.hpccsystems.com/browse/HPCC-27830
Jira updated

@rpastrana rpastrana requested a review from afishbeck June 23, 2022 13:08
afishbeck
afishbeck reviewed Jun 23, 2022
View reviewed changes
system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp Outdated

if (m_tenantID.isEmpty())
throw makeStringException(-1, "Could not determine Azure Tenant ID, set 'AZURE_TENANT_ID' env var, or connection/@tenantID in AzureClient LogAccess configuration!");
getSecretValue(m_aadTenantID.clear(), azureLogAccessSecretCategory, azureLogAccessSecretName, azureLogAccessSecretAADTenantID, true);
Copy link
Member

@afishbeck afishbeck Jun 23, 2022 •
edited

Choose a reason for hiding this comment

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

Since you are calling this several times to get values from the same secret (azureLogAccessSecretName) it would be more efficient to get the secret first (as a ptree from the secret cache) and use that to get each value.

Owned<IPropertyTree> secretTree = getSecret(azureLogAccessSecretCategory, azureLogAccessSecretName);

getSecretkeyValue(m_aadTenantID.clear(), secretTree, azureLogAccessSecretAADTenantID);
getSecretKeyValue(m_aadClientID.clear(), secretTree, azureLogAccessSecretAADClientID);

etc.

@rpastrana rpastrana force-pushed the HPCC-27790-logicregressionAzureLA branch 3 times, most recently from d300295 to d2d8161 Compare June 24, 2022 14:56
rpastrana
rpastrana commented Jun 24, 2022
View reviewed changes
helm/examples/azure/log-analytics/README.md Outdated

Example use:
```console
helm install myhpcc hpcc/hpcc -f HPCC-Platform/helm/examples/azure/log-analytics/loganalytics-hpcc-logaccess.yaml
helm install myhpcc hpcc/hpcc -f HPCC-Platform/helm/examples/azure/log-analytics/loganalytics-hpcc-logaccess.yaml -f HPCC-Platform/helm/examples/azure/log-analytics/loganalytics-logccess-secrets.yaml
Copy link
Member Author

Choose a reason for hiding this comment

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

@afishbeck can you confirm this is the intended approach for declaring jsecret secrets?
I'm concerned the secrets/vaults structure might be lead to inadvertent overwrites of a given secret/vault category (in this case 'esp')

@rpastrana rpastrana requested a review from afishbeck June 24, 2022 15:02
@rpastrana rpastrana force-pushed the HPCC-27790-logicregressionAzureLA branch 4 times, most recently from 6e41de3 to 8d472b4 Compare June 24, 2022 15:55
@rpastrana
Copy link
Member Author

@afishbeck please review changes from your review.
Note, I added a helper script to create the required secret.

afishbeck
afishbeck reviewed Jun 25, 2022
View reviewed changes
system/logaccess/Azure/LogAnalytics/CurlClient/AzureLogAnalyticsCurlClient.cpp Outdated
if (m_clientID.isEmpty())
throw makeStringException(-1, "Could not find Azure AD client ID, set 'AZURE_CLIENT_ID' env var, or connection/@clientID in AzureClient LogAccess configuration - format is '00000000-0000-0000-0000-000000000000'!");
if (m_aadClientID.isEmpty())
throw makeStringExceptionV(-1, "%s: Could not find AAD Client ID, provide it as part of %s.%s secret, or connection/@clientID in AzureClient LogAccess configuration!", COMPONENT_NAME, azureLogAccessSecretName, azureLogAccessSecretAADClientID);
Copy link
Member

Choose a reason for hiding this comment

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

Some of the indentations in this file look a bit off.

afishbeck
afishbeck reviewed Jun 25, 2022
View reviewed changes
helm/examples/azure/log-analytics/create-azure-logaccess-secret.sh Outdated
else
echo "Target secret '${secretname}' successfully created!"
${k8scommand} get secret ${secretname}
fi
Copy link
Member

Choose a reason for hiding this comment

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

Git complains about these files without newlines at the end. For bash and yaml I would add newlines. For the secret content files you can't.

afishbeck
afishbeck reviewed Jun 25, 2022
View reviewed changes
helm/examples/azure/log-analytics/loganalytics-logaccess-secrets.yaml Outdated
azure-logaccess: "azure-logaccess"
vaults:
esp:
- name: azure-logaccess
Copy link
Member

Choose a reason for hiding this comment

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

Very trivial, but this makes it look like this vault is just for the logaccess secrets. A name like "myvault" would show how it could be a common vault, or whatever they want/need.

afishbeck
afishbeck approved these changes Jun 25, 2022
View reviewed changes
Copy link
Member

@afishbeck afishbeck left a comment

Choose a reason for hiding this comment

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

@rpastrana a few trivial comment, but looks good.

@rpastrana rpastrana force-pushed the HPCC-27790-logicregressionAzureLA branch from c1367f2 to 9e5140b Compare June 27, 2022 16:04
@rpastrana
HPCC-27830 Azure LogAccess use Secrets
c4ebc36 
- Adds esp jsecret category
- Eliminates use of Env vars
- Stablishes azure_logaccess secret
- Updates documentation
- Provides secrets template
- Provides logAccess secret creator

Signed-off-by: Rodrigo Pastrana <rodrigo.pastrana@lexisnexisrisk.com>
@rpastrana rpastrana force-pushed the HPCC-27790-logicregressionAzureLA branch from 9e5140b to c4ebc36 Compare June 27, 2022 18:42
@rpastrana
Copy link
Member Author

@afishbeck integrated the minor changes you suggested

@rpastrana
Copy link
Member Author

@ghalliday @richardkchapman ready for merge

@ghalliday ghalliday merged commit ce93207 into hpcc-systems:candidate-8.8.x Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghalliday ghalliday ghalliday approved these changes

@afishbeck afishbeck afishbeck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@rpastrana @ghalliday @afishbeck