MGMT-16721: handle hosts with no luks #5940

paul-maidment · 2024-02-01T07:46:07Z

In line 41 of internal/host/hostcommands/tang_connectivity_check_cmd.go
Both luks and luks.Clevis are pointers which can be nil, there was no checking in place to ensure that we avoid a nil pointer dereference

This PR fixes that by making appropriate checks to ensure that neither of these pointers are nil.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2024-02-01T07:46:11Z

gamli75 · 2024-02-01T07:50:31Z

@rccrdpccl please review

paul-maidment · 2024-02-01T07:56:51Z

/retest

omertuc · 2024-02-01T11:28:02Z

What does it mean for Luks to be present in an ignition file but for Clevis to be nil? Is that a valid situation? What is the expected behavior from the service in that situation? To ignore it? To report an error?

Please elaborate

rccrdpccl · 2024-02-01T18:39:18Z

What does it mean for Luks to be present in an ignition file but for Clevis to be nil? Is that a valid situation? What is the expected behavior from the service in that situation? To ignore it? To report an error?

Please elaborate

Could you please also change description of the issue and commit according to the answer for the question above :)?

rccrdpccl · 2024-02-01T18:40:20Z

/retest

codecov · 2024-02-01T18:53:07Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (df15ca0) 68.26% compared to head (9a82bd4) 68.32%.
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5940      +/-   ##
==========================================
+ Coverage   68.26%   68.32%   +0.05%     
==========================================
  Files         236      236              
  Lines       34913    34922       +9     
==========================================
+ Hits        23833    23859      +26     
+ Misses       9006     8995      -11     
+ Partials     2074     2068       -6

Files	Coverage Δ
internal/host/validator.go	`81.77% <ø> (ø)`
...l/host/hostcommands/tang_connectivity_check_cmd.go	`76.19% <40.00%> (+4.03%)`	⬆️

... and 6 files with indirect coverage changes

paul-maidment · 2024-02-04T14:01:40Z

What does it mean for Luks to be present in an ignition file but for Clevis to be nil? Is that a valid situation? What is the expected behavior from the service in that situation? To ignore it? To report an error?

Please elaborate

I just observed that both luks and clevis are pointers and can have a value of nil.
Simply based on the fact that pointers can be nil, (whether or not this is valid) is reasonable grounds to check for nil I believe.

omertuc · 2024-02-04T14:09:58Z

What does it mean for Luks to be present in an ignition file but for Clevis to be nil? Is that a valid situation? What is the expected behavior from the service in that situation? To ignore it? To report an error?
Please elaborate

I just observed that both luks and clevis are pointers and can have a value of nil. Simply based on the fact that pointers can be nil, (whether or not this is valid) is reasonable grounds to check for nil I believe.

I'm in favor of checking, but we perform a check so we can branch out our behavior.

In case of A, do X. In case of B, do Y.

In this PR it's not clear to me how you decided on X and Y, as I don't understand the circumstances that lead to A and B and the reasoning behind what is the correct behavior for the service in each of these circumstances

Or in other words, why return nil, nil ? (not to say that it's the wrong thing to do, just that it's not clear to me, it doesn't seem obvious, and so it should be thought out and pointed out in a code comment)

omertuc · 2024-02-04T14:13:34Z

If you're not sure, even something like // TODO: not sure how this could happen or whether its possibly valid, for now pretend encryption is disabled just so we avoid a nil pointer dereference is also completely valid, as it's obviously improving the previous situation by avoiding a crash, but at least we can recognize that this needs to be thought out more thoroughly and needs looking into

paul-maidment · 2024-02-04T14:19:05Z

Even something like // TODO: not sure how this could happen or whether its possibly valid, for now pretend encryption is disabled just so we avoid a nil pointer dereference is also completely valid, as it's obviously improving the previous situation by avoiding a crash, but at least we can recognize that this needs to be thought out more thoroughly and needs looking into

I'll go with this suggestion and will also add test cases for luks == nil and luks.clevis == nil, just to ensure we have the best coverage.

Seems to me that just because a cluster is imported does not necessarily imply that there needs to be disk encryption, maybe we are simply covering the case where an imported cluster does not have disk encryption?

Certainly the case of missing luks we can imagine this to be the case

Missing luks.Clevis I do agree that it's odd this should be optional.

So for now, I will go with the comment you suggest.

I'll also add a warning to the log about this scenario.

omertuc · 2024-02-04T14:41:14Z

internal/host/hostcommands/tang_connectivity_check_cmd.go

@@ -38,6 +38,11 @@ func (c *tangConnectivityCheckCmd) getTangServersFromHostIgnition(host *models.H
 		if err != nil {
 			return nil, err
 		}
+		if luks == nil || luks.Clevis == nil {
+			// TODO: not sure how this could happen or whether its possibly valid, for now pretend encryption is disabled just so we avoid a nil pointer dereference
+			c.log.Warn("luks configuration is missing or incomplete in the host ignition, disk encryption will be assumed to be disabled.")


luks being nil is pretty common, it doesn't warrant a warning. We'll get a warning every time a cluster doesn't have encryption (at-least, that's what it I think would happen, not sure, need to look at ignitions from actual clusters)

OK, removed the log entry.

My comment was about only luks being nil, which doesn't warrant a warning.

But luks being non-nil and Clevis nil does warrant a warning and a code comment, that's the case I've been discussing in all my comments so far, the one I don't understand and I'm not sure how we should react to

OK, updated to deal with that scenario.

openshift-ci-robot · 2024-02-04T14:48:22Z

rccrdpccl · 2024-02-07T10:20:56Z

/unhold
/lgtm
/approve

omertuc · 2024-02-07T11:22:29Z

So in the case of us receiving a nil in this field, I suspect that we should raise an error because this means that we wont be able to boot the LUKS volume as we lack the key.

Does this make sense?

Not really, we're discussing validations for adding hosts to an existing cluster here.

This entire ordeal of looking at the ignition to get tang details is just so we can perform a validation that the tang config specified in the target cluster actually works on this new day-2 host - we know how to recognize and validate tang, so we look into the clevis struct to get tang servers (when those are present), so we can send those details to our agent so it can attempt to connect to the tang servers, followed by the agent reporting about what it experienced doing that, so that our validation can ensure tang works.

If you see the cluster has encryption (i.e. has the luks struct) but it's not using using the clevis struct, and you throw an error because you don't recognize it, then you are implicitly making an assumption of how that encryption works (in particular, you're assuming that it wouldn't work and it deserves and error just because you have no way to validate that it works), but perhaps it just means that our validation should be skipped (both in the tang_connectivity_check_cmd.go and the corresponding validation code that awaits the results from the agent).

The reasoning of skipping instead of giving an error is that the validation was best-effort in the first place - we're doing it as a "favor" to the user. We can validate tang so we do. If the user uses some other scheme we don't recognize, we should still let them add the host to the cluster, and if they run into issues, that's unfortunate, but it's none of our business, because it just might work. We can't assume it won't because we don't know what it is

Also regardless of whether you simply return nil as you did, or return an error - a user's day-2 host installation will still hang - as you never modified the corresponding validation code that is waiting on those tang results from the agent

rccrdpccl · 2024-02-07T11:36:34Z

/hold

Holding waiting for clarification on this comment:
Also regardless of whether you simply return nil as you did, or return an error - a user's day-2 host installation will still hang - as you never modified the corresponding validation code that is waiting on those tang results from the agent

Is this changing current behaviour, other than fixing the nil pointer case when luks not present?
Just want to make sure we do not add more issues

paul-maidment · 2024-02-07T13:09:24Z

tang_connectivity_check_cmd

Regarding the validation of disk encryption requirements.

It seems that firstly we fetch host ignition and validate that luks is not nil and contains at least one entry.
https://github.com/openshift/assisted-service/blob/master/internal/host/hostutil/host_utils.go#L326
(I also call the above method from my code, so that's common)

https://github.com/openshift/assisted-service/blob/master/internal/host/validator.go#L422-L425
Then we check to see if the clevis setting is present, bypassing validation and marking it as successful if these settings are not present

if luks == nil || luks.Clevis == nil {
	// Disk encryption is disabled for workers on day1 cluster
	return ValidationSuccessSuppressOutput, ""
}

the comment // Disk encryption is disabled for workers on day1 cluster is a little confusing as a few lines above, we have already determined that we are looking at a day 2 host.

With this aside, we can see how a Day2 host without Clevis is treated. This is treated as though there is no disk encryption.
Which to me at least, looks like the same logic I implemented in this PR?

omertuc · 2024-02-07T13:16:22Z

the comment // Disk encryption is disabled for workers on day1 cluster is a little confusing as a few lines above, we have already determined that we are looking at a day 2 host.

By "day 1 cluster" this comment actually means "target cluster"

paul-maidment · 2024-02-07T13:19:06Z

the comment // Disk encryption is disabled for workers on day1 cluster is a little confusing as a few lines above, we have already determined that we are looking at a day 2 host.

By "day 1 cluster" this comment actually means "target cluster"

Should we change the above comment to make this a lot clearer?

omertuc · 2024-02-07T13:20:14Z

With this aside, we can see how a Day2 host without Clevis is treated. This is treated as though there is no disk encryption.
Which to me at least, looks like the same logic I implemented in this PR?

Yes, it's the same.

The comment however is making the false statement that because clavis is nil then there's no encryption. Which is a wrong thing to say as it's too general, there's no tang servers to validate, doesn't mean there's no encryption. But its behavior is still correct since its goal is to validate tang, and clavis being nil does mean there's no tang servers to validate

omertuc · 2024-02-07T13:20:28Z

Should we change the above comment to make this a lot clearer?

Sure

internal/host/hostcommands/tang_connectivity_check_cmd.go

paul-maidment · 2024-02-07T13:36:00Z

/hold

Holding waiting for clarification on this comment: Also regardless of whether you simply return nil as you did, or return an error - a user's day-2 host installation will still hang - as you never modified the corresponding validation code that is waiting on those tang results from the agent

Is this changing current behavior, other than fixing the nil pointer case when luks not present? Just want to make sure we do not add more issues

In short...zero behavioral change. Our existing validations expect these fields to be set otherwise we assume that disk encryption is disabled.

openshift-ci-robot · 2024-02-07T13:55:49Z

In line 41 of internal/host/hostcommands/tang_connectivity_check_cmd.go Both `luks` and `luks.Clevis` are pointers which can be nil, there was no checking in place to ensure that we avoid a nil pointer dereference This PR fixes that by making appropriate checks to ensure that neither of these pointers are nil.

rccrdpccl · 2024-02-07T13:58:47Z

Thanks for clarifying @paul-maidment
/unhold
/lgtm
/approve

openshift-ci · 2024-02-07T14:00:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: paul-maidment, rccrdpccl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [paul-maidment,rccrdpccl]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-02-07T14:57:49Z

/retest-required

Remaining retests: 0 against base HEAD eb3ed40 and 2 for PR HEAD 9a82bd4 in total

paul-maidment · 2024-02-07T17:50:28Z

/retest

openshift-ci · 2024-02-07T19:20:39Z

@paul-maidment: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2024-02-07T22:29:30Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-agent-installer-api-server-container-v4.16.0-202402072112.p0.gbc922ba.assembly.stream.el8 for distgit ose-agent-installer-api-server.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 1, 2024

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 1, 2024

openshift-ci bot requested review from danmanor and omertuc February 1, 2024 07:48

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2024

paul-maidment force-pushed the MGMT-16721 branch from 4328699 to 206bffe Compare February 1, 2024 07:55

paul-maidment force-pushed the MGMT-16721 branch from 206bffe to 9d31b35 Compare February 1, 2024 07:58

paul-maidment force-pushed the MGMT-16721 branch from 9d31b35 to 5921d6f Compare February 4, 2024 14:26

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 4, 2024

paul-maidment force-pushed the MGMT-16721 branch 2 times, most recently from 40efb88 to f8a2ff2 Compare February 4, 2024 14:34

omertuc reviewed Feb 4, 2024

View reviewed changes

paul-maidment force-pushed the MGMT-16721 branch 2 times, most recently from ea7a108 to 6291ed4 Compare February 4, 2024 14:44

paul-maidment force-pushed the MGMT-16721 branch 3 times, most recently from 0a6659d to c4b2913 Compare February 4, 2024 16:54

paul-maidment force-pushed the MGMT-16721 branch from 3e2431f to 9880f3e Compare February 7, 2024 10:14

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2024

openshift-ci bot assigned rccrdpccl Feb 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2024

paul-maidment force-pushed the MGMT-16721 branch from 9880f3e to 0a87210 Compare February 7, 2024 13:29

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

omertuc reviewed Feb 7, 2024

View reviewed changes

internal/host/hostcommands/tang_connectivity_check_cmd.go Outdated Show resolved Hide resolved

paul-maidment force-pushed the MGMT-16721 branch from 0a87210 to 6b6be5e Compare February 7, 2024 13:37

paul-maidment force-pushed the MGMT-16721 branch from 6b6be5e to 9a82bd4 Compare February 7, 2024 13:56

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 7, 2024

openshift-merge-bot bot merged commit bc922ba into openshift:master Feb 7, 2024
14 checks passed

MGMT-16721: handle hosts with no luks #5940

MGMT-16721: handle hosts with no luks #5940

Conversation

paul-maidment commented Feb 1, 2024 • edited

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Feb 1, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

gamli75 commented Feb 1, 2024

paul-maidment commented Feb 1, 2024

omertuc commented Feb 1, 2024

rccrdpccl commented Feb 1, 2024

rccrdpccl commented Feb 1, 2024

codecov bot commented Feb 1, 2024 • edited

Codecov Report

paul-maidment commented Feb 4, 2024

omertuc commented Feb 4, 2024 • edited

omertuc commented Feb 4, 2024 • edited

paul-maidment commented Feb 4, 2024 • edited

omertuc Feb 4, 2024 • edited

Choose a reason for hiding this comment

paul-maidment Feb 4, 2024

Choose a reason for hiding this comment

omertuc Feb 4, 2024 • edited

Choose a reason for hiding this comment

paul-maidment Feb 4, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 4, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl commented Feb 7, 2024

omertuc commented Feb 7, 2024

rccrdpccl commented Feb 7, 2024

paul-maidment commented Feb 7, 2024 • edited

omertuc commented Feb 7, 2024

paul-maidment commented Feb 7, 2024

omertuc commented Feb 7, 2024 • edited

omertuc commented Feb 7, 2024

paul-maidment commented Feb 7, 2024 • edited

openshift-ci-robot commented Feb 7, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

rccrdpccl commented Feb 7, 2024

openshift-ci bot commented Feb 7, 2024

openshift-ci-robot commented Feb 7, 2024

paul-maidment commented Feb 7, 2024

openshift-ci bot commented Feb 7, 2024

openshift-bot commented Feb 7, 2024

paul-maidment commented Feb 1, 2024 •

edited

openshift-ci-robot commented Feb 1, 2024 •

edited by openshift-ci bot

codecov bot commented Feb 1, 2024 •

edited

omertuc commented Feb 4, 2024 •

edited

omertuc commented Feb 4, 2024 •

edited

paul-maidment commented Feb 4, 2024 •

edited

omertuc Feb 4, 2024 •

edited

omertuc Feb 4, 2024 •

edited

openshift-ci-robot commented Feb 4, 2024 •

edited by openshift-ci bot

paul-maidment commented Feb 7, 2024 •

edited

omertuc commented Feb 7, 2024 •

edited

paul-maidment commented Feb 7, 2024 •

edited

openshift-ci-robot commented Feb 7, 2024 •

edited by openshift-ci bot