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

fix: variadic methods build errors #144

Conversation

muskanlalit18
Copy link
Contributor

@muskanlalit18 muskanlalit18 commented Nov 15, 2024

Issue #, if available:

Description of changes:
[Opensource Ubuntu mode]

  1. Remove all invocations of variadic methods using variadic params
  2. Fix the logger function that used the variadic param to use a const char*
  3. Remove the usage of format string from logging
  4. Update logger method invokes to create C++ style string and pass as a param directly to the logger method

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the CONTRIBUTING doc
  • I used the commit message format described in CONTRIBUTING
  • I have updated any necessary documentation, including READMEs and comments (where appropriate)

Tests

  1. Logs viewed on the console:
Credentials-fetcher daemon has started running
on request failures check logs located at /var/credentials-fetcher/logging
krb_files_dir = /var/credentials-fetcher/krbdir
logging_dir = /var/credentials-fetcher/logging
unix_socket_dir = /var/credentials-fetcher/socket
Thread 0: top of stack near 0x7f6cb51ffbe0; argv_string=grpc_thread
Thread 0: top of stack near 0x7f6cafb7fb28; argv_string=krb_ticket_refresh_thread
2024-11-20 19:24:51     INFO: Server listening on unix:/var/credentials-fetcher/socket/credentials_fetcher.sock
2024-11-20 19:24:51     INFO: CallDataCreateKerberosLease 0x7f6cb000d770status: 0
2024-11-20 19:24:51     INFO: AddNonDomainJoinedKerberosLease 0x7f6cb000e1a0status: 0
2024-11-20 19:24:51     INFO: RenewNonDomainJoinedKerberosLease 0x7f6cb000ebe0status: 0
2024-11-20 19:24:51     INFO: CallDataDeleteKerberosLease 0x7f6cb000f600status: 0
2024-11-20 19:24:51     INFO: CallDataHealthCheck 0x7f6cb0010090 status: 0
  1. Logs in /var/logging/credentials-fetcher.log
2024-11-20 19:24:51: Credentials-fetcher daemon has started running 
2024-11-20 19:24:51: grpc pthread is at 0x58816b1a21f0 
2024-11-20 19:24:51: krb refresh pthread is at 0x58816b1a22a0 

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

common/daemon.h Outdated Show resolved Hide resolved
@muskanlalit18 muskanlalit18 force-pushed the fixes_for_DNS_and_distinguishedName branch from 0fcf4e5 to 95cea2b Compare November 20, 2024 01:19
}
sd_journal_print( level, logFmt.c_str(), logs... );
write_log( logFmt.c_str(), logs... );
// std::string logFmt = fmt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line 133 and 134

@@ -219,24 +224,22 @@ int main( int argc, const char* argv[] )
cf_daemon.gmsa_account_name = CF_TEST_GMSA_ACCOUNT;

std::cerr << "krb_files_dir = " << cf_daemon.krb_files_dir << std::endl;
//std::cerr << "cred_file = " << cf_daemon.cred_file << " (lease id: " << cred_file_lease_id
// << ")" << std::endl;
// std::cerr << "cred_file = " << cf_daemon.cred_file << " (lease id: " << cred_file_lease_id
Copy link
Contributor

Choose a reason for hiding this comment

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

good to remove @smhmhmd?

Copy link
Contributor

Choose a reason for hiding this comment

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

The std:cerr can be added to write_log as well, also, those prints can be brought back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I add all of these cerr logs to write_log?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please go ahead

Copy link
Contributor

@bhallasaksham bhallasaksham left a comment

Choose a reason for hiding this comment

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

Added two minor comments to remove commented out code, but otherwise LGTM

@smhmhmd smhmhmd merged commit b5c8829 into aws:fixes_for_DNS_and_distinguishedName Nov 20, 2024
@bhallasaksham bhallasaksham mentioned this pull request Jan 15, 2025
5 tasks
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.

3 participants