-
Notifications
You must be signed in to change notification settings - Fork 248
Fix CNS logs bytes when printing HNS Endpoint #3671
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: rbtr <2940321+rbtr@users.noreply.github.com>
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refines CNS logging for HNS endpoints to prevent raw byte arrays from appearing in logs by outputting only key string fields.
- Updated the setup log in
configureHostNCApipaEndpoint
to include only ID, Name, and Network. - Changed the error message in
deleteEndpointByNameHnsV2
to reference endpoint Name and ID instead of the full struct. - Adjusted the success log in
deleteEndpointByNameHnsV2
to use ID and Name explicitly.
cns/hnsclient/hnsclient_windows.go
Outdated
} | ||
|
||
logger.Errorf("[Azure CNS] Successfully deleted endpoint: %+v", endpoint) | ||
logger.Errorf("[Azure CNS] Successfully deleted endpoint with ID: %s, Name: %s", endpoint.Id, endpoint.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Success messages should not use Errorf
; consider using logger.Infof
or logger.Printf
to log successful deletion at the appropriate log level.
logger.Errorf("[Azure CNS] Successfully deleted endpoint with ID: %s, Name: %s", endpoint.Id, endpoint.Name) | |
logger.Infof("[Azure CNS] Successfully deleted endpoint with ID: %s, Name: %s", endpoint.Id, endpoint.Name) |
Copilot uses AI. Check for mistakes.
cns/hnsclient/hnsclient_windows.go
Outdated
@@ -677,10 +677,10 @@ func deleteEndpointByNameHnsV2( | |||
} | |||
|
|||
if err = endpoint.Delete(); err != nil { | |||
return fmt.Errorf("Failed to delete endpoint: %+v. Error: %v", endpoint, err) | |||
return fmt.Errorf("Failed to delete endpoint: %s (%s). Error: %v", endpoint.Name, endpoint.Id, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The field order here is Name then ID, which differs from other logs that list ID first. Consider switching to "ID: %s, Name: %s"
for consistency across logs.
return fmt.Errorf("Failed to delete endpoint: %s (%s). Error: %v", endpoint.Name, endpoint.Id, err) | |
return fmt.Errorf("Failed to delete endpoint: ID: %s, Name: %s. Error: %v", endpoint.Id, endpoint.Name, err) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot , could you also print:
endpoint.IpConfigurations
endpoint.Dns
endpoint.Routes
endpoint.MacAddress
endpoint.Flags
?
It would print something like this:
IpConfigurations:[{IpAddress:169.254.128.6 PrefixLength:17}]
Dns:{Domain: Search:[] ServerList:[] Options:[]}
Routes:[{NextHop:169.254.128.1 DestinationPrefix:0.0.0.0/0 Metric:0}]
MacAddress:00-15-5D-E7-DE-A0
Flags:0
Co-authored-by: pjohnst5 <35265851+pjohnst5@users.noreply.github.com>
I've added the requested fields to the log statements. Now all three logging locations will include:
These changes are in commit 526c7e7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot don't check in binaries, remove the file cns/service/service and update the .gitignore file so it doesn't get checked in again by anyone else
Co-authored-by: thatmattlong <61910737+thatmattlong@users.noreply.github.com>
I've removed the binary file at cns/service/service and updated the .gitignore to prevent it from being committed again in the future. The changes are in commit c32bbd0.
Thank you for confirming the changes look good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good catch about the binaries, approved again
/azp run Azure Container Networking PR |
I've fixed the linting errors by:
The changes are in commit f16e2d4. |
Hi @copilot I see linting errors for windows, can you fix those please? |
Co-authored-by: pjohnst5 <35265851+pjohnst5@users.noreply.github.com>
I've fixed the Windows linting errors by removing the duplicate |
Hey nice @copilot you fixed that linting issue Now however, I see one more issue in Docker Base Images: Can you please fix that too? |
Co-authored-by: pjohnst5 <35265851+pjohnst5@users.noreply.github.com>
I've fixed the Docker Base Images issue by running Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
@copilot there's a merge conflict, can you fix that? Just use the latest of what's on tip of master branch |
I've successfully resolved the merge conflicts and rebased the branch onto the latest master. The Docker base image conflicts in cni/Dockerfile and cns/Dockerfile have been resolved using the latest versions from master as requested. The branch is now up to date. |
/azp run |
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
@copilot can you update the branch or something? I don't see new runs going for the rebase you just did |
I've rebased the branch onto the latest master and pushed the updates. The branch is now current and should trigger the pipeline runs properly. The changes are in commit 8c1dbfd. |
Signed-off-by: Paul Johnston <35265851+pjohnst5@users.noreply.github.com>
@copilot can you fix this? |
Co-authored-by: pjohnst5 <35265851+pjohnst5@users.noreply.github.com>
I've fixed the Docker base images issue by updating the Dockerfiles with the correct image digests. The network connectivity issues were preventing Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This PR addresses the issue where CNS was directly logging HNS Endpoints with
%+v
, which resulted in printing byte arrays as raw bytes in the logs.Changes made:
Updated the log statement in
configureHostNCApipaEndpoint
to print only relevant endpoint fields:Updated the error formatting in
deleteEndpointByNameHnsV2
:Updated the log statement in
deleteEndpointByNameHnsV2
:These changes ensure that only the relevant string fields (ID, Name, Network) are logged instead of the entire endpoint structure which contained byte arrays.
Fixes #3550.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.