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

Add GMSA support for V2 process isolated containers #797

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

dcantah
Copy link
Contributor

@dcantah dcantah commented Apr 8, 2020

Support for V2 HCS Schema process isolated containers

  • Add generated V2 schema files for Container Credential Guard
  • Add new hcs calls that are necessary to setup container credential guard
    instances.
  • Add new resource type CCGState that implements ResourceCloser so a containers
    ccg instance will be cleaned up on container close.
  • Add tests to validate gmsa

Signed-off-by: Daniel Canter dcanter@microsoft.com

@dcantah dcantah requested a review from a team as a code owner April 8, 2020 18:02
@dcantah dcantah force-pushed the gmsa branch 2 times, most recently from 00b4b82 to 57809ee Compare April 10, 2020 02:45
@dcantah dcantah changed the title [WIP] Add GMSA support for V2 process isolated containers Add GMSA support for V2 process isolated containers Apr 10, 2020
@dcantah
Copy link
Contributor Author

dcantah commented Apr 15, 2020

@kevpar @katiewasnothere @ambarve if one of you has time today could you take a gander at this :)

@marosset
Copy link
Member

Linking to #347 for tracking :)

@dcantah dcantah force-pushed the gmsa branch 4 times, most recently from c6d1de7 to d7cf697 Compare April 22, 2020 10:45
internal/hcs/system.go Outdated Show resolved Hide resolved
internal/hcs/system.go Outdated Show resolved Hide resolved
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

Left some feedback

test/cri-containerd/gmsa.go Outdated Show resolved Hide resolved
test/cri-containerd/gmsa.go Outdated Show resolved Hide resolved
test/cri-containerd/gmsa.go Outdated Show resolved Hide resolved
@dcantah dcantah force-pushed the gmsa branch 3 times, most recently from a6b3546 to 4768792 Compare April 24, 2020 08:49
@kevpar
Copy link
Member

kevpar commented Apr 24, 2020

Can you update the title so we don't use the term "V2 process isolated containers", please?

@dcantah
Copy link
Contributor Author

dcantah commented Apr 24, 2020

Can you update the title so we don't use the term "V2 process isolated containers", please?

What is the concern with the title/what would you rather it be? If it's with the V2 removing that wouldn't be accurate as we already do support GMSA for V1 schema containers.

@kevpar
Copy link
Member

kevpar commented Apr 24, 2020

Realistically not a big deal I suppose. I'm trying to cut down on us using "v2" as an shorthand since I think it's vague (are you talking about v2 hcs schema, v2 hcs APIs, v2 containerd shim protocol, something else?). If you clarify in the description I think the title is okay.

@dcantah
Copy link
Contributor Author

dcantah commented Apr 24, 2020

@kevpar Good point, all the V1-V2 and codenames was very confusing at the beginning so I understand haha. Added a clarification to the description.

* Add generated V2 schema files for Container Credential Guard
* Add new hcs calls that are necessary to setup container credential guard
instances.
* Add new resource type CCGInstance that implements ResourceCloser so a containers
ccg instance will be cleaned up on container close.
* Add tests to validate gmsa
* Remove logging from resource Release methods and just return an error.
Forego returning immediately on an error in ReleaseResources and return
afterwards if any of the releases failed.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@dcantah dcantah merged commit d3a8d6f into microsoft:master Apr 28, 2020
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.

5 participants