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.

Sign up for GitHub

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

Jump to bottom

Connect CA Roots #332

Merged
merged 9 commits into from
Jun 13, 2024
Merged

Connect CA Roots #332

merged 9 commits into from
Jun 13, 2024

Conversation

IvanKolchanov
Copy link
Contributor

Changes made

  1. Added GET /v1/agent/connect/ca/roots endpoint
  2. Included new endpoint into ICatalogEndpoint.cs
  3. Added possible test to AgentTest.cs
  4. Edited test_config.json for the test

Issue ticket number and link

@IvanKolchanov
Added connect/ca/roots endpoint, added the endpoint to IAgentEndpoint…
0d6cb04 
….cs, added possible test, changed test_config for the test
@IvanKolchanov
Copy link
Contributor Author

The only issue I have is with another test case that the Consul in Golang has: https://github.com/hashicorp/consul/blob/v1.9.17/api/connect_ca_test.go#L12 .
They use the testutil to create another client for testing and do not bootstrap connect in that test case, which is, however, not really possible with the current testing setup that consuldotnet has. Because that would require to create another test_config.json and run both the new and the old one when testing. Please correct me if I am wrong!

@marcin-krystianc
Copy link
Contributor

The only issue I have is with another test case that the Consul in Golang has: https://github.com/hashicorp/consul/blob/v1.9.17/api/connect_ca_test.go#L12 . They use the testutil to create another client for testing and do not bootstrap connect in that test case, which is, however, not really possible with the current testing setup that consuldotnet has. Because that would require to create another test_config.json and run both the new and the old one when testing. Please correct me if I am wrong!

You are right, but we don't have to replicate that test. The test you've already added is sufficient. It tests that the endpoint works and returns desired data.

@marcin-krystianc marcin-krystianc marked this pull request as ready for review June 12, 2024 07:37
marcin-krystianc
marcin-krystianc reviewed Jun 12, 2024
View reviewed changes
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Generally, it looks good. Please add more test assertions so we know that all the fields are named correctly.

Consul.Test/AgentTest.cs Outdated
Comment on lines 1030 to 1034
{
var caRoots = await _client.Agent.GetCARoots();
Assert.NotEqual((ulong)0, caRoots.LastIndex);
Assert.Single(caRoots.Response.Roots);
Assert.Equal("11111111-2222-3333-4444-555555555555.consul", caRoots.Response.TrustDomain);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could test all fields in the response (at least for fields being non-null). That would assure us that all fields in the newly added structures are named correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added the test assertions, ready for further review!

@IvanKolchanov
Copy link
Contributor Author

The only issue I have is with another test case that the Consul in Golang has: https://github.com/hashicorp/consul/blob/v1.9.17/api/connect_ca_test.go#L12 . They use the testutil to create another client for testing and do not bootstrap connect in that test case, which is, however, not really possible with the current testing setup that consuldotnet has. Because that would require to create another test_config.json and run both the new and the old one when testing. Please correct me if I am wrong!

You are right, but we don't have to replicate that test. The test you've already added is sufficient. It tests that the endpoint works and returns desired data.

That's what I thought, just had to make sure! :)

@IvanKolchanov
Copy link
Contributor Author

@marcin-krystianc I see that a few tests had failed because of an assertion error, expected that a bit, because I wasn't sure which kind of values can SerialNumber, CreateIndex and ModifyIndex take depending on conditions. PrivateKeyBits shouldn't be 0 though. Don't think that there is a way for me to initialize those values in config, except for PrivateKeyBits (https://developer.hashicorp.com/consul/docs/connect/ca/consul). Should I just delete the assert tests for SerialNumber, CreateIndex and ModifyIndex?
Thank you in advance!

@marcin-krystianc
Copy link
Contributor

@marcin-krystianc I see that a few tests had failed because of an assertion error, expected that a bit, because I wasn't sure which kind of values can SerialNumber, CreateIndex and ModifyIndex take depending on conditions. PrivateKeyBits shouldn't be 0 though. Don't think that there is a way for me to initialize those values in config, except for PrivateKeyBits (https://developer.hashicorp.com/consul/docs/connect/ca/consul). Should I just delete the assert tests for SerialNumber, CreateIndex and ModifyIndex? Thank you in advance!

It seem that the problem only occurs for version 1.6.10. So perhaps we can use AgentVersion variable (

consuldotnet/Consul.Test/BaseFixture.cs

Line 15 in 219ecd4

protected static SemanticVersion AgentVersion;
) and assert those problematic fields only when the version is equal or greater than 1.7.0 ?

IvanKolchanov and others added 2 commits June 12, 2024 06:53
@IvanKolchanov
Copy link
Contributor Author

@marcin-krystianc I see that a few tests had failed because of an assertion error, expected that a bit, because I wasn't sure which kind of values can SerialNumber, CreateIndex and ModifyIndex take depending on conditions. PrivateKeyBits shouldn't be 0 though. Don't think that there is a way for me to initialize those values in config, except for PrivateKeyBits (https://developer.hashicorp.com/consul/docs/connect/ca/consul). Should I just delete the assert tests for SerialNumber, CreateIndex and ModifyIndex? Thank you in advance!

It seem that the problem only occurs for version 1.6.10. So perhaps we can use AgentVersion variable (

consuldotnet/Consul.Test/BaseFixture.cs

Line 15 in 219ecd4

protected static SemanticVersion AgentVersion;

) and assert those problematic fields only when the version is equal or greater than 1.7.0 ?

Makes sense, just added that AgentVersion check, can you run the tests @marcin-krystianc ?

marcin-krystianc
marcin-krystianc reviewed Jun 12, 2024
View reviewed changes
Consul.Test/AgentTest.cs Outdated
Assert.Null(root.IntermediateCerts);
Assert.True(root.Active);
Assert.NotNull(root.PrivateKeyType);
Assert.NotEqual(0, root.PrivateKeyBits);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one needs to go inside the if (AgentVersion >= SemanticVersion.Parse("1.7.0")) block.

Copy link
Contributor Author

@IvanKolchanov IvanKolchanov Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, a bit surprising, but should be working now, fixed it!

marcin-krystianc
marcin-krystianc approved these changes Jun 13, 2024
View reviewed changes
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

I think now it is perfect, thank you!

@marcin-krystianc marcin-krystianc merged commit 51d9847 into G-Research:master Jun 13, 2024
242 of 243 checks passed
@IvanKolchanov IvanKolchanov mentioned this pull request Jun 14, 2024
Closed
@IvanKolchanov IvanKolchanov deleted the add-connect-ca-roots branch June 17, 2024 09:59
@IvanKolchanov IvanKolchanov mentioned this pull request Jun 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcin-krystianc marcin-krystianc marcin-krystianc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IvanKolchanov @marcin-krystianc @octocat