Skip to content

test/e2e: Update testcases for HTTPS KBS#2285

Merged
mkulke merged 1 commit intoconfidential-containers:mainfrom
chathuryaadapa:https-kbs-e2e
Mar 3, 2025
Merged

test/e2e: Update testcases for HTTPS KBS#2285
mkulke merged 1 commit intoconfidential-containers:mainfrom
chathuryaadapa:https-kbs-e2e

Conversation

@chathuryaadapa
Copy link
Contributor

@chathuryaadapa chathuryaadapa commented Feb 7, 2025 •
edited
Loading

Support certificates in KBS and aa/cdh

  • Update the kbs e2e test cases to Https kbs e2e test cases
  • Update certificate and key creation for Https kbs
  • Update the set policy func with certificate

fixes: #2300

@chathuryaadapa chathuryaadapa requested a review from a team as a code owner February 7, 2025 08:36
@chathuryaadapa chathuryaadapa marked this pull request as draft February 7, 2025 08:37
stevenhorsman
stevenhorsman reviewed Feb 7, 2025
View reviewed changes
@chathuryaadapa chathuryaadapa changed the title update testcases for https kbs test/e2e: Update testcases for HTTPS KBS Feb 7, 2025
@stevenhorsman stevenhorsman added the test_e2e_libvirt Run Libvirt e2e tests label Feb 7, 2025
@chathuryaadapa chathuryaadapa force-pushed the https-kbs-e2e branch 3 times, most recently from e1f08ea to 7cbad6f Compare February 13, 2025 11:37
@chathuryaadapa chathuryaadapa force-pushed the https-kbs-e2e branch 5 times, most recently from ec76f50 to b45c04f Compare February 18, 2025 05:55
@chathuryaadapa chathuryaadapa marked this pull request as ready for review February 18, 2025 06:42
stevenhorsman
stevenhorsman reviewed Feb 24, 2025
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

It generally is looking fine. I had a few questions/comments of some of the code choices and we also need to have a commit body explaining the changes in commit message.

src/cloud-api-adaptor/test/provisioner/trustee_kbs.go Outdated

var trusteeRepoPath string
var certPath string
var workerNodeIP string
Copy link
Member

@stevenhorsman stevenhorsman Feb 24, 2025

Choose a reason for hiding this comment

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

What is the reason for this being made a package level variable?

Copy link
Contributor Author

@chathuryaadapa chathuryaadapa Feb 25, 2025

Choose a reason for hiding this comment

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

Hello steve,
certificate path is being used while setting the resource policy and attestation policy.

And worker node Ip is being used by NewHTTPSKbsInstallOverlay to generate certificate and key.

Copy link
Member

@stevenhorsman stevenhorsman Feb 25, 2025

Choose a reason for hiding this comment

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

So can we just use workerNodeIP, _, _ = getFirstWorkerNodeIPAndName(cfg) (or refactor getFirstWorkerNodeIPAndName to have a verison that just returns the ip) when we need it rather than making it package level and then having to do if workerNodeIP != "" {?

@chathuryaadapa chathuryaadapa force-pushed the https-kbs-e2e branch 2 times, most recently from 5089cc4 to c3eaf07 Compare February 25, 2025 04:07
@chathuryaadapa chathuryaadapa force-pushed the https-kbs-e2e branch 2 times, most recently from 34c8377 to 13ec5b7 Compare February 28, 2025 13:12
@chathuryaadapa
test/e2e : Updated kbs e2e test cases to support https kbs
228ea92 
- Switch the KBS tests, using initdata to use https rather
than http, as we want to ensure this flow works in order to
avoid exposure of secrets
- Update `checkout_kbs.sh` to configure certificates
in the kbs deployment

Signed-off-by: Adapa Chathurya <[email protected]>
stevenhorsman
stevenhorsman approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

The code looks reasonable and the tests all pass. Thanks @chathuryaadapa!

@mkulke mkulke merged commit 2cfd7e5 into confidential-containers:main Mar 3, 2025
45 checks passed
@chathuryaadapa chathuryaadapa deleted the https-kbs-e2e branch March 3, 2025 09:40
@bpradipt bpradipt mentioned this pull request Mar 14, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkulke mkulke mkulke approved these changes

@stevenhorsman stevenhorsman stevenhorsman approved these changes

Assignees

No one assigned

Labels

test_e2e_libvirt Run Libvirt e2e tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

test/e2e: Updating the kbs e2e test cases Https Kbs

3 participants

@chathuryaadapa @mkulke @stevenhorsman