-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 FIPS mode secret and vendor in libpod/pkg/secret #1537
Conversation
LGTM |
072f733
to
39cd875
Compare
I don't understand why the mounts file is not being overridden in the tests by the |
Look to see if this is the only time CRI_BINARY is called. |
/test all |
/test kata-containers |
@mrunalp Kata containers seems to not be working. Who do we ping to see what is going on? |
/retest |
cmd/crio/main.go
Outdated
Usage: "add one or more default mount paths in the form host:container", | ||
cli.StringFlag{ | ||
Name: "default-mounts-file", | ||
Usage: "path to default mounts file", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking api change, you should actually add a new option and we need to support the old one at least for 2 to 3 releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to have the deprecated flag still there.
|
||
# default_mounts_file is the file path holding the default mounts to be mounted for the | ||
# container when created. | ||
# default_mounts_file = "{{ .DefaultMountsFile }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change as well, we need to support old and new and deprecate the old one so people have time to migrate. Basically, when they use the old one, we print a log so that they can later do the switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
test/default_mounts.bats
Outdated
run crictl create "$pod_id" "$TESTDATA"/container_redis.json "$TESTDATA"/sandbox_config.json | ||
echo "$output" | ||
[ "$status" -eq 0 ] | ||
ctr_id="$output" | ||
run crictl exec --sync "$ctr_id" cat /proc/mounts | ||
run crictl exec --sync "$ctr_id" ls /run/secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather add a new test than changing an old one just to verify a new condition. Please add a new one for all these cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Update cri-o to use the secrets package from libpod/pkg/secret, which also brings in support for FIPS mode secret. If the host is in FIPS mode (i.e /etc/system-fips exists) /run/secrets/system-fips is created in the container so that the container can also run in FIPS mode. Signed-off-by: umohnani8 <[email protected]>
/test all |
Not sure why kata CI is failing. @mrunalp who should I talk to? |
@sboeuf Can you take a look at why kata containers tests are failing? |
@mrunalp nothing related to the PR. I have restarted the job since there is no reason for failure here. |
/test kata-containers |
1 similar comment
/test kata-containers |
LGTM |
Ok kata build fixed. Let's retest one more time ! |
/test kata-containers |
1 similar comment
/test kata-containers |
Tests pass! |
;) |
Ok lets merge. |
Update cri-o to use the secrets package from libpod/pkg/secret,
which also brings in support for FIPS mode secret.
If the host is in FIPS mode (i.e /etc/system-fips exists) /run/secrets/system-fips
is created in the container so that the container can also run in FIPS mode.
Signed-off-by: umohnani8 [email protected]