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

OCPBUGS-1666: Expose flag to enable/disable PodSecurity #6062

Merged
merged 7 commits into from
Oct 13, 2022

Conversation

jmrodri
Copy link
Member

@jmrodri jmrodri commented Oct 7, 2022

Description of the change:

Added --security-context-config flag to enable seccompprofile. It defaults to enabled to support k8s 1.25. You can disable it with --security-context-config=legacy

Signed-off-by: jesus m. rodriguez jesusr@redhat.com

Motivation for the change:
In k8s 1.25 PodSecurityAdmission is enabled. https://kubernetes.io/blog/2022/08/04/upcoming-changes-in-kubernetes-1-25/#podsecuritypolicy-removal

Fixes a few issues found downstream but affect upstream as well:

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:37 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:42 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:42 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:46 Inactive
@jmrodri
Copy link
Member Author

jmrodri commented Oct 7, 2022

Fixes a few issues upstream and downstream:

@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:47 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:47 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 7, 2022 02:49 Inactive
@jmrodri
Copy link
Member Author

jmrodri commented Oct 7, 2022

If you pass in an unsupported option you get the help message:

$ operator-sdk run bundle quay.io/olmqe/upgradeindex-bundle:v0.1 --index-image quay.io/olmqe/largefbcindexwithupgradefbc:v4.11 --timeout 5m --security-context-config=fake -n jesusr
Error: invalid argument "fake" for "--security-context-config" flag: must be one of "legacy", or "restricted"
Usage:
  operator-sdk run bundle <bundle-image> [flags]

Flags:
      --ca-secret-name string                     Name of a generic secret containing a PEM root certificate file required to pull bundle images. This secret *must* be in the namespace that this command is configured to run in, and the file *must* be encoded under the key "cert.pem"
  -h, --help                                      help for bundle
      --index-image string                        index image in which to inject bundle (default "quay.io/operator-framework/opm:latest")
      --install-mode InstallModeValue             install mode
      --kubeconfig string                         Path to the kubeconfig file to use for CLI requests.
  -n, --namespace string                          If present, namespace scope for this CLI request
      --pull-secret-name string                   Name of image pull secret ("type: kubernetes.io/dockerconfigjson") required to pull bundle images. This secret *must* be both in the namespace and an imagePullSecret of the service account that this command is configured to run in
      --security-context-config SecurityContext   specifies the security context to use for the catalog pod
      --service-account string                    Service account name to bind registry objects to. If unset, the default service account is used. This value does not override the operator's service account
      --skip-tls                                  skip authentication of image registry TLS certificate when pulling a bundle image in-cluster
      --skip-tls-verify                           skip TLS certificate verification for container image registries while pulling bundles
      --timeout duration                          Duration to wait for the command to complete before failing (default 2m0s)
      --use-http                                  use plain HTTP for container image registries while pulling bundles

Global Flags:
      --plugins strings   plugin keys to be used for this subcommand execution
      --verbose           Enable verbose logging

FATA[0000] invalid argument "fake" for "--security-context-config" flag: must be one of "legacy", or "restricted" 

@jmrodri
Copy link
Member Author

jmrodri commented Oct 7, 2022

When restricted is enabled, the pod will have the seccompProfile set on it.

$ operator-sdk run bundle quay.io/olmqe/upgradeindex-bundle:v0.1 --index-image quay.io/olmqe/largefbcindexwithupgradefbc:v4.11 --timeout 5m --security-context-config=restricted -n jesusr

$ k get pod quay-io-olmqe-upgradeindex-bundle-v0-1 -o yaml -n jesusr | grep -i seccompProfile -A5 -B1
  securityContext:
    seccompProfile:
      type: RuntimeDefault
  serviceAccount: default
  serviceAccountName: default
  terminationGracePeriodSeconds: 30
  tolerations:

@jmrodri
Copy link
Member Author

jmrodri commented Oct 7, 2022

if legacy is enabled then no seccompprofile is added to the Pod.

$ operator-sdk run bundle quay.io/olmqe/upgradeindex-bundle:v0.1 --index-image quay.io/olmqe/largefbcindexwithupgradefbc:v4.11 --timeout 5m --security-context-config=legacy -n jesusr

$ k get pod quay-io-olmqe-upgradeindex-bundle-v0-1 -o yaml -n jesusr | grep -i seccompProfile -A5 -B1

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2022
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Just a teeny nit on the changelog. Also need to run make generate to update the CLI docs with the latest changes.

changelog/fragments/enable-seccompprofile-run-bundle.yaml Outdated Show resolved Hide resolved
.cncf-maintainers Outdated Show resolved Hide resolved
jmrodri and others added 2 commits October 7, 2022 21:56
Added --security-context-config flag to enable seccompprofile.
It defaults to enabled to support k8s 1.25. You can disable it
with --security-context-config=legacy

Signed-off-by: jesus m. rodriguez <jesusr@redhat.com>
* Ignoring error from Set call in test
* Update .cncf maintainers
* Update run bundle(-upgrade) CLI docs

Signed-off-by: jesus m. rodriguez <jmrodri@gmail.com>
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 13:45 Inactive
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022
This was being duplicated because we had it in the text but were not
setting the value to a default value. Once we set the value to the
default cobra realized this and would output "(default: restricted)".

So removing the manually entered text fixes the duplicate.

Signed-off-by: jesus m. rodriguez <jesusr@redhat.com>
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
@jmrodri jmrodri temporarily deployed to deploy October 12, 2022 14:16 Inactive
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2022
@jmrodri jmrodri merged commit ead5efa into operator-framework:master Oct 13, 2022
@jmrodri
Copy link
Member Author

jmrodri commented Oct 13, 2022

/cherry-pick v1.24.x

@openshift-cherrypick-robot

@jmrodri: new pull request created: #6080

In response to this:

/cherry-pick v1.24.x

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.

tiraboschi added a commit to tiraboschi/release that referenced this pull request Jul 11, 2023
…text

Add a parameter to let the users specify a
value for --security-context-config (legacy/restricted, default
restricted) to by used by the operator-sdk for its catalog pod.

See: operator-framework/operator-sdk#6062

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
openshift-merge-robot pushed a commit to openshift/release that referenced this pull request Jul 11, 2023
…text (#41134)

Add a parameter to let the users specify a
value for --security-context-config (legacy/restricted, default
restricted) to by used by the operator-sdk for its catalog pod.

See: operator-framework/operator-sdk#6062

Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants