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

cpu: re-organize security features #833

Merged

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Jun 28, 2022

Move existing security/trusted-execution related features (i.e. SGX and
SE) under the same security feature, deprecating the old features. The
motivation for the change is to keep the source code and user interface
more organized as we experience a constant inflow of similar security
related features. This change will affect the user interface so it is
less painful to do it early on.

New feature labels will be:

  feature.node.kubernetes.io/cpu-security.se.enabled
  feature.node.kubernetes.io/cpu-security.sgx.enabled

and correspondingly new cpu.security feature with se.enabled and
sgx.enabled elements will be available for custom rules, for example:

      - name: "sample sgx rule"
        labels:
          sgx.sample.feature: "true"
        matchFeatures:
          - feature: cpu.security
            matchExpressions:
              "sgx.enabled": {op: IsTrue}

At the same time deprecate old labels cpu-sgx.enabled and
cpu-se.enabled feature labels and the corresponding features for
custom rules. These will be removed in the future causing an effective
change in NFDs user interface.

Move existing security/trusted-execution related features (i.e. SGX and
SE) under the same "security" feature, deprecating the old features. The
motivation for the change is to keep the source code and user interface
more organized as we experience a constant inflow of similar security
related features. This change will affect the user interface so it is
less painful to do it early on.

New feature labels will be:

  feature.node.kubernetes.io/cpu-security.se.enabled
  feature.node.kubernetes.io/cpu-security.sgx.enabled

and correspondingly new "cpu.security" feature with "se.enabled" and
"sgx.enabled" elements will be available for custom rules, for example:

      - name: "sample sgx rule"
        labels:
          sgx.sample.feature: "true"
        matchFeatures:
          - feature: cpu.security
            matchExpressions:
              "sgx.enabled": {op: IsTrue}

At the same time deprecate old labels "cpu-sgx.enabled" and
"cpu-se.enabled" feature labels and the corresponding features for
custom rules. These will be removed in the future causing an effective
change in NFDs user interface.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 28, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Jun 28, 2022

RFC
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2022
@zvonkok
Copy link
Contributor

zvonkok commented Jun 29, 2022

@marquiz Where are we with the topic of having shorter feature labels? I think this is also still open. I think we should start thinking of doing that as well as soon as possible before we are adding features that are used by other projects and it creates to much confusion when switching.

@marquiz marquiz added this to the v.0.12.0 milestone Jun 30, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Jun 30, 2022

@marquiz Where are we with the topic of having shorter feature labels? I think this is also still open. I think we should start thinking of doing that as well as soon as possible before we are adding features that are used

Yeah, agree on this. I added both (#832 and #778) into the v0.12 milestone.

What do you think about this PR vs. a totally separate security feature source?

@marquiz marquiz removed this from the v.0.12.0 milestone Jun 30, 2022
@marquiz
Copy link
Contributor Author

marquiz commented Aug 30, 2022

Any thoughts on this? @mythi @zvonkok @ArangoGutierrez ?

@mythi
Copy link
Contributor

mythi commented Aug 30, 2022

Any thoughts on this? @mythi @zvonkok @ArangoGutierrez ?

I added my thoughts in #832 (comment) earlier

@marquiz
Copy link
Contributor Author

marquiz commented Aug 31, 2022

I added my thoughts in #832 (comment) earlier

Ach yes, sorry I already forgot that comment 😊 Suggestions (or PRs) how to change the feature descriptions in docs are welcome.

Removing the RFC status of this PR
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2022
Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, marquiz

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 81da164 into kubernetes-sigs:master Sep 1, 2022
@marquiz marquiz deleted the devel/security-refactor branch September 1, 2022 13:00
@marquiz marquiz mentioned this pull request Sep 1, 2022
@marquiz marquiz mentioned this pull request Dec 20, 2022
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants