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

Sanitize namespace flag consistently #220

Merged
merged 3 commits into from
Jan 3, 2025
Merged

Conversation

hoffm
Copy link
Contributor

@hoffm hoffm commented Dec 19, 2024

Problem

This plugin accepts a namespace var and creates a K8s namespace using that value. K8s namespaces must contain only lowercase alphanumeric characters and dashes, and begin with an alphanumeric character (reference).

To handle the possibility that users will provide values for namespace that are invalid as K8s namespace names, the plugin sanitizes the value before creating the namespace. This ensures that namespace creation does not fail due to an invalid name.

However, when the plugin calls kubectl rollout, it passes the unsanitized, user-provided namespace value. As a result, if the namespace var required sanitization to be a valid K8s namespace name, the rollout command will specify a namespace that does not exist, and will fail with the message:

namespaces "$NAMESPACE_VAR" not found

Fix

I see two options here:

  1. Reject namespace values that are not valid K8s namespaces.
  2. Sanitize the user-inputted value for use in the rollout command.

I've implemented (2) in checkParams.

I prefer approach because it allows the most flexibility for users. For instance, a common usage pattern is to pass the git branch name as the drone-gke namespace value. Branch names can have capital letters and other characters not allowed in K8s branch names. Option (2) allows users to continue the practice of using branch names without having to restrict the characters they can use in their git branches.

PR Form

If this is a change to the core functionality, did you make a corresponding PR to drone-eks?

https://github.com/nytimes/drone-eks/pull/75

  • yes
  • no
  • n/a

Did you update the tests?

  • yes
  • no
  • n/a

Did you update the docs?

I'm thinking of this as a bug fix, so I didn't think an update to the docs was warranted. Let me know if there's a change I should make.

  • yes
  • no
  • n/a

@hoffm hoffm requested a review from a team as a code owner December 19, 2024 13:53
@hoffm hoffm changed the title Set namespace flag after normalizing it Set namespace flag after sanitizing it Dec 19, 2024
@tonglil
Copy link
Member

tonglil commented Dec 20, 2024

Thanks Michael, good find! I agree with you, the namespace var should be sanitized once and used consistently.

Do you mind refactoring the code so the sanitization and c.Set happens in the checkParams func as you described? That way c.String("namespace") will have the sanitized string everywhere it's called as checkParams happens early on.

Also, once this PR is approved, thank you in advance for importing the same changes into drone-eks!! 🫡

@hoffm
Copy link
Contributor Author

hoffm commented Dec 23, 2024

Do you mind refactoring the code so the sanitization and c.Set happens in the checkParams func as you described? That way c.String("namespace") will have the sanitized string everywhere it's called as checkParams happens early on.

@tonglil Done!

@hoffm hoffm changed the title Set namespace flag after sanitizing it Sanitize namespace flag consistently Dec 23, 2024
@tonglil tonglil merged commit cb8ba78 into nytimes:main Jan 3, 2025
1 check passed
@tonglil
Copy link
Member

tonglil commented Jan 3, 2025

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants