-
Notifications
You must be signed in to change notification settings - Fork 65
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
crd: Expose agent{Https,No}Proxy #355
crd: Expose agent{Https,No}Proxy #355
Conversation
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 thanks @fidencio !
api/v1beta1/ccruntime_types.go
Outdated
// when performing the image pull inside the guest (either using nydus snapshotter with containerd | ||
// or CRI-O) | ||
AgentNoProxy string `json:"agentNoProxy,omitempty"` | ||
|
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.
Since this is kata runtime specific (and I'm assuming doesn't apply to enclave-cc), how about putting these in a separate Kata agent specific struct so that the code is easier to read and update if new agent config params gets added ?
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.
That's a very good point, @bpradipt, let me revisit this one soon.
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.
Is the same not possible with EnvironmentVariables
? Do you have the code somewhere that takes these into the kata agent?
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.
Based on ^^^ I added a do-not-merge label.
7254204
to
3072630
Compare
Let's just add a basic example of what the admin can set as environment variables in order to deal with proxy shenanigans, in case they're unluck enough to have to deal with it. :-) Signed-off-by: Fabiano Fidêncio <[email protected]>
3072630
to
0f7430c
Compare
I've taken @mythi's suggestion and instead of exposing it, we're just leaving a hint to the user, in the default kustomization, of what they can uncomment in order to have proxies working as expected. |
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. I prefer this approach over adding proxies to the "API"
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
Depending on the environment where we deploy Confidential Containers, setting up the proxy is required for the agent to be able to connect with the external world.
With that in mind, mainly considering this is needed for the basic TDX CI, let's ensure we expose to the users a way to set it up.