Skip to content

Conversation

@kiashok
Copy link
Contributor

@kiashok kiashok commented Apr 28, 2025

This commit makes the high level changes needed for gcs-sidecar

  • Starts sidecar as service
  • Dereferences the various valid rpc requests
  • Adds code to invoke refs formatter

Note: This commit does not add invokers to the code for new
ResourceTypes like SecurityPolicy, CWCOWBlockCIMs,
Container scratch formatting etc. This will come in along
with functional tests in later PRs.
There are some TODO comments in the code which will be
addressed in upcoming PRs as well.

To make this initialization of the gcs-sidecar flow complete,
certain high level code for the policy enforcement have been
brought into this commit from @MahatiC 's changes.
Example: internal/gcs-sidecar/policy.go, internal/gcs-sidecar/host.go
and helper functions in internal/gcs-sidecar/host.go.
Hence adding her as co-author in this commit.
The rest of the policy framework code will be brought in by @MahatiC
as follow up PRs.

Commit 1: Rearranges some bridge code to make them reusable for gcs-sidecar framework as well
Commit 2: This is a cherry pick of Amit's PR here : #2421 . Will rebase this PR once his PR is merged.
Commit 3: This is the main commit that brings in changes for gcs-sidecar + minimal changes from policy framework in order to make the initialization flow complete. Rest of the policy enforcement changes will be brought in by @MahatiC

@kiashok kiashok requested a review from a team as a code owner April 28, 2025 17:35
@kiashok
Copy link
Contributor Author

kiashok commented Apr 28, 2025

@kiashok kiashok force-pushed the gcs-sidecar-framework branch 3 times, most recently from 973753d to 588627a Compare April 28, 2025 19:40
Copy link
Member

@MahatiC MahatiC left a comment

Choose a reason for hiding this comment

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

I did a quick first pass review, will take another closer look.

@kiashok kiashok force-pushed the gcs-sidecar-framework branch 6 times, most recently from a1d186f to a800f04 Compare May 8, 2025 17:11
@kiashok kiashok force-pushed the gcs-sidecar-framework branch from a800f04 to a2da501 Compare May 27, 2025 19:28
@kiashok
Copy link
Contributor Author

kiashok commented Jun 3, 2025

@MahatiC @helsaawy could you please take a look when you have some time? Thanks!

Copy link
Member

@MahatiC MahatiC left a comment

Choose a reason for hiding this comment

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

Other than the minor changes/nits in my latest review, it looks good to me.

@kiashok kiashok force-pushed the gcs-sidecar-framework branch from a2da501 to 3b7df1c Compare June 5, 2025 22:40
@kiashok
Copy link
Contributor Author

kiashok commented Jun 5, 2025

Other than the minor changes/nits in my latest review, it looks good to me.

Addressed them in the latest push. Thanks for reviewing! :)

@KenGordon KenGordon self-assigned this Jun 9, 2025
Copy link
Collaborator

@KenGordon KenGordon left a comment

Choose a reason for hiding this comment

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

This is a blocker.

}
sidecarErrChan <- recverr
}()
// Send response to hcsshim
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that this is work in progress and Mahati is addressing it somewhat, but not all messages to the inbox GCS will directly result in a message to the host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate on what you mean by "all messages to inbox GCs will not directly result in a message to the host" ?

Send response to hcsshim is a goroutine which waits on a channel. Please refer to handlers.go.

  • Used when there is a premature response like policy enforcement not allowing a request or ResourceTypeWCOWBlockCims for example. OR,
  • We got a response from inbox GCS and need to forward response to hcsshim:
    image

This go routine does not mean every request coming to the gcs-sidecar needs to have a response. Not sure what the concern is or if I am missing smth.

cc @MahatiC what is it you are addressing?

@kiashok kiashok force-pushed the gcs-sidecar-framework branch from 3b7df1c to 3f07f1e Compare June 11, 2025 17:06
@kiashok kiashok force-pushed the gcs-sidecar-framework branch from 3f07f1e to 36c0d2b Compare July 1, 2025 16:11
- Move common bridge protocol definitions to subpackage
under internal/gcs
- Move helper functions to internal/bridgeutils pkg
so that they can be used by gcs-sidecar as well

Signed-off-by: Kirtana Ashok <[email protected]>
@kiashok kiashok force-pushed the gcs-sidecar-framework branch from 36c0d2b to e64ed40 Compare July 9, 2025 20:51
This commit makes the high level changes needed for gcs-sidecar
- Starts sidecar as service
- Dereferences the various valid rpc requests
- Adds code to invoke refs formatter

Note: This commit does not add invokers to the code for new
ResourceTypes like SecurityPolicy, CWCOWBlockCIMs,
Container scratch formatting etc. This will come in along
with functional tests in later PRs.
There are some TODO comments in the code which will be
addressed in upcoming PRs as well.

To make this initialization of the gcs-sidecar flow complete,
certain high level code for the policy enforcement have been
brought into this commit from Mahati's changes.
Example: internal/gcs-sidecar/policy.go, internal/gcs-sidecar/host.go
and helper functions in internal/gcs-sidecar/host.go.
Hence adding her as co-author in this commit.
The rest of the policy framework code will be brought in by Mahati
as follow up PRs.

Co-authored-by: <[email protected]>

Signed-off-by: Kirtana Ashok <[email protected]>
@kiashok kiashok force-pushed the gcs-sidecar-framework branch from e64ed40 to c3dcf03 Compare July 16, 2025 23:07
Copy link
Contributor

@anmaxvl anmaxvl left a comment

Choose a reason for hiding this comment

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

lgtm

@kiashok kiashok merged commit b8f90a0 into microsoft:main Jul 17, 2025
84 of 89 checks passed
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.

6 participants