Skip to content
This repository has been archived by the owner on Oct 30, 2024. It is now read-only.

support for new override labels #527

Merged
merged 10 commits into from
Feb 28, 2023
Merged

Conversation

dani-santos-code
Copy link
Contributor

@dani-santos-code dani-santos-code commented Feb 23, 2023

Description

This PR addresses feedback from the kubernetes community on using unregistered annotations.

We have purchased a domain (kubeaudit.io) as suggested for our own purpose: override labels.

Backwards compatibility testing

This should be backwards compatible. I added 3 tests for the 3 old labels that should capture that. I also ran kubeaudit locally using both the old and the new labels and correctly applied the override label. I also tested using a random label (e.g kubeau.io) and it didn't work, as expected. 🎉

Deprecation Warning

Now we print a general deprecation warning to let users know that it will be deprecated.
Screenshot 2023-02-24 at 2 55 05 PM
Screenshot 2023-02-24 at 3 00 53 PM

Actual deprecation

Left comments in the code base with TODOs so we can come back and delete the deprecated annotations when we think it's the right time. When should we do so?

Fixes # #457

Type of change
  • Bug fix 🐛
  • New feature ✨
  • This change requires a documentation update 📖
  • Breaking changes ⚠️
How Has This Been Tested?
  • Test A
  • Test B
Checklist:
  • I have 🎩 my changes (A 🎩 specifically includes pulling down changes, setting them up, and manually testing the changed features and potential side effects to make sure nothing is broken)
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The test coverage did not decrease
  • I have signed the appropriate Contributor License Agreement

@dani-santos-code dani-santos-code marked this pull request as draft February 23, 2023 23:11
@dani-santos-code dani-santos-code force-pushed the ds/remove-tainted-annotations branch 5 times, most recently from 8544ca5 to ea2c44a Compare February 24, 2023 20:19
@dani-santos-code dani-santos-code marked this pull request as ready for review February 24, 2023 20:34
genevieveluyt
genevieveluyt previously approved these changes Feb 24, 2023
Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

Backwards compatible is awesome! I wonder if this would be a good opportunity to clean up the annotation format (for the new annotations). I think we could still keep it backwards compatible since the override logic is contained in a single package.

The syntax for container / pod / namespace currently is:

  • container.audit.kubernetes.io/[container name].[override identifier] (on the pod resource)
  • audit.kubernetes.io/pod.[override identifier] (on the pod resource)
  • audit.kubernetes.io/namespace.[override identifier] (on the namespace resource)

The requirements for label syntax are here

eg. maybe it could be

  • container.kubeaudit.io/[container name].[override identifier]
  • pod.kubeaudit.io/[override identifier]
  • namespace.kubeaudit.io/[override identifier]

Or, since in most cases the label applies to the resource it's specified on, we could have kubeaudit.io/[override identifier] as the format and only have the container case be different (since container resources don't allow labels we put it on the pod) container.kubeaudit.io/[container name].[override identifier]

cailyn-codes
cailyn-codes previously approved these changes Feb 24, 2023
Copy link

@cailyn-codes cailyn-codes 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 question, otherwise LGTM

@dani-santos-code
Copy link
Contributor Author

@genevieveluyt

Excellent point and great suggestions! Thank you! ❤️

I like kubeaudit.io/[override identifier] for pod and namespace resources, which allow for adding labels, but container.kubeaudit.io/[container name].[override identifier] for container annotations!

applied your feedback here: 896f2b6

more specifically: https://github.com/Shopify/kubeaudit/blob/896f2b6e50553209a65958a9a8edfc07f02f50c8/pkg/override/override.go

auditors/apparmor/fixtures/apparmor-bad-value-override.yml Outdated Show resolved Hide resolved
printer.go Outdated
@@ -48,6 +49,13 @@ func WithColor(color bool) PrintOption {
}
}

// WithDeprecationWarning allows us to send a message about deprecations to end users
func WithDeprecationWarning(warning string) PrintOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the other print options are format options that the user can specify (eg. if they're using kubeaudit as a package; as a cli we pass on flags to the printer) so it's a bit weird to have an option that isn't intended to be used by an end user. Maybe we can just print the warning to StdErr in root.go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true! doesn't make sense indeed. let's go with stdErr in root. Addressed here: 211c9a8

Copy link
Contributor

@genevieveluyt genevieveluyt left a comment

Choose a reason for hiding this comment

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

Looks great thank you! 🎉 You can bump the VERSION file if you want to put out a release right away

@dani-santos-code dani-santos-code merged commit e4c7f8a into main Feb 28, 2023
@dani-santos-code dani-santos-code deleted the ds/remove-tainted-annotations branch February 28, 2023 14:36
@dani-santos-code dani-santos-code changed the title remove refs to old annotations support for new override labels Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants