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

crd: Expose agent{Https,No}Proxy #355

Conversation

fidencio
Copy link
Member

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.

Copy link
Member

@ChengyuZhu6 ChengyuZhu6 left a comment

Choose a reason for hiding this comment

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

LGTM thanks @fidencio !

// when performing the image pull inside the guest (either using nydus snapshotter with containerd
// or CRI-O)
AgentNoProxy string `json:"agentNoProxy,omitempty"`

Copy link
Member

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 ?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member

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.

@fidencio fidencio force-pushed the topic/crd-expose-agent-https-no-proxy branch from 7254204 to 3072630 Compare June 9, 2024 17:08
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 <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/crd-expose-agent-https-no-proxy branch from 3072630 to 0f7430c Compare August 2, 2024 08:24
@fidencio
Copy link
Member Author

fidencio commented Aug 2, 2024

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.

Copy link
Contributor

@mythi mythi left a 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"

Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

@fidencio fidencio merged commit e3bdedc into confidential-containers:main Aug 4, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants