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

Integration tests for windows service support #3733

Merged
merged 11 commits into from Jan 19, 2023

Conversation

guilhermocc
Copy link
Contributor

@guilhermocc guilhermocc commented Jan 5, 2023

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Running spire as a windows service inside windows containers.

Description of change

  • Adds integration tests for windows service support.
  • Fix windows service support not working when running spire in windows containers.

Which issue this PR fixes
Complement for #3490

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
…tainers

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
@guilhermocc guilhermocc changed the title Windows service integration tests Windows service support integration tests Jan 5, 2023
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
@guilhermocc guilhermocc changed the title Windows service support integration tests Integration tests for windows service support Jan 5, 2023
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
@@ -20,7 +25,10 @@ type systemCall struct {
}

func (s *systemCall) IsWindowsService() (bool, error) {
return svc.IsWindowsService()
// We are using a custom function because the svc.IsWindowsService() one still has an open issue in which it states
// that it is not working properly in Windows containers: https://github.com/golang/go/issues/56335. Soon as we have
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth submitting a PR? That issue has a help wanted tag and it seems like you have a fix below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A PR is already opened for this, in fact, the custom function with the workaround is based on the opened solution proposal: golang/sys#141

@@ -0,0 +1,6 @@
#!/bin/bash

pwd
Copy link
Collaborator

Choose a reason for hiding this comment

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

why pwd is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it is actually required, I took it from another test sample (windows workload attestor). I'm going to check if it is really necessary.

Comment on lines 6 to 7
create-service spire-server C:\\spire\\bin\\spire-server.exe
start-service spire-server run -config C:/spire/conf/server/server.conf
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to be consistent and use \\ or / in all places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! nice suggestion

Comment on lines 8 to 9
docker-compose exec -T -u ContainerUser spire-base \
c:/spire/bin/spire-agent api fetch jwt -audience mydb fail-now "failed to fetch x509"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
docker-compose exec -T -u ContainerUser spire-base \
c:/spire/bin/spire-agent api fetch jwt -audience mydb fail-now "failed to fetch x509"
docker-compose exec -T -u ContainerUser spire-base \
c:/spire/bin/spire-agent api fetch jwt -audience mydb || fail-now "failed to fetch JWT"


FROM spire-server-windows:latest-local as spire-base

COPY --from=spire-agent-windows C:/spire/bin/spire-agent.exe C:/spire/bin/spire-agent.exe
Copy link
Collaborator

Choose a reason for hiding this comment

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

why you choose to use the same container to start both services?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, in this initial version, I tried to reproduce a test that I ran manually on my local machine (spire server and agent on the same host). Still, I see that a more realistic scenario would be running them in different hosts, right? I can update it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that is 'generally' the case

@MarcosDY MarcosDY added this to the 1.6.0 milestone Jan 17, 2023
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
@guilhermocc guilhermocc requested review from MarcosDY and faisal-memon and removed request for MarcosDY and faisal-memon January 18, 2023 17:31
Copy link
Member

@amartinezfayo amartinezfayo left a comment

Choose a reason for hiding this comment

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

Thank you @guilhermocc, LGTM!

@MarcosDY MarcosDY merged commit 968a198 into spiffe:main Jan 19, 2023
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
* Create integration tests for windows service support

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
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.

None yet

4 participants