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

fix: update matching for Kubernetes error message when server-dry-run is not supported #663

Merged
merged 6 commits into from
Jan 13, 2020
Merged

fix: update matching for Kubernetes error message when server-dry-run is not supported #663

merged 6 commits into from
Jan 13, 2020

Conversation

sco11morgan
Copy link
Contributor

What are you trying to accomplish with this PR?
Current error message contains 1does not support dry run" not "doesn't support dry-run"
It is not clear if the message previously was "doesn't support dry-run" so I left that in the matching expression.

https://github.com/kubernetes/apiserver/blob/7dc4ceb2fd3311166a78bdf6a4eec4c194364dd0/pkg/admission/plugin/webhook/errors/statuserror.go#L61

...

How is this accomplished?
updated existing regex Krane::KubernetesResource::SERVER_DRY_RUN_DISABLED_ERROR
...

What could go wrong?
...

… is not supported

current error message contains "does not support dry run"
not "doesn't support dry-run"
It is not clear if the message was previously "doesn't support dry-run" so I left that in the matching expression.

https://github.com/kubernetes/apiserver/blob/7dc4ceb2fd3311166a78bdf6a4eec4c194364dd0/pkg/admission/plugin/webhook/errors/statuserror.go#L61
@KnVerey
Copy link
Contributor

KnVerey commented Jan 9, 2020

What specifically did you see that prompted this PR? The link in the description is to a repo containing a "Generic library for building a Kubernetes aggregated API server" and the error is specifically about admission webhooks, which krane doesn't deal with. I believe the error message we're checking for comes from https://github.com/kubernetes/kubernetes/blob/16c2ae4607371f1f948c0a9c884805e699330450/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go#L698, which still uses doesn't on master.

@sco11morgan
Copy link
Contributor Author

I ran into the message
Internal error occurred: admission webhook "redacted" does not support dry run
from our clusters.

We're in the process of updating our webhooks to work with server-dry-run, but in the meantime we can't upgrade to >=0.28.0 because deploying secrets fails.

@KnVerey
Copy link
Contributor

KnVerey commented Jan 9, 2020

Thanks for the explanation. I now see that the code in kubernetes/apiserver (despite that repo's description) is just synced from kubernetes/kubernetes, so the error message in question can also be found here: https://github.com/kubernetes/kubernetes/blob/8b0c36d6204029f6085567706d6e7f689a497346/staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/errors/statuserror.go#L61.

@@ -16,7 +16,7 @@ class KubernetesResource
TIMEOUT = 5.minutes
LOG_LINE_COUNT = 250
SERVER_DRY_RUN_DISABLED_ERROR =
/(unknown flag: --server-dry-run)|(doesn't support dry-run)|(dryRun alpha feature is disabled)/
/(unknown flag: --server-dry-run)|(doesn't support dry-run)|(does not support dry run)|(dryRun alpha feature is disabled)/
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @sco11morgan for bring it to us.
We can make one regex for doesn't and does not, something like:
/(unknown flag: --server-dry-run)|(does[\s]*n[o|']t support dry-run)|(dryRun alpha feature is disabled)/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Thx

@KnVerey
Copy link
Contributor

KnVerey commented Jan 10, 2020

Tests passed, but the diff contains a couple linter errors:

Offenses:
--
  |  
  | lib/krane/kubernetes_resource.rb:19:121: C: Metrics/LineLength: Line is too long. [128/120]
  | /(unknown flag: --server-dry-run)\|(doesn't support dry-run)\|(does not support dry run)\|(dryRun alpha feature is disabled)/
  | ^^^^^^^^
  | test/unit/krane/kubernetes_resource_test.rb:266:93: C: Layout/TrailingWhitespace: Trailing whitespace detected.
  | refute(resource.validation_failed?, "Failed to ignore server dry run responses matching:

Copy link
Contributor

@jessie-JNing jessie-JNing left a comment

Choose a reason for hiding this comment

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

Change looks good to me. Please fix the lint before merge it!

Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

Linting passed locally

@dturn dturn merged commit d772651 into Shopify:master Jan 13, 2020
@dturn dturn temporarily deployed to rubygems January 22, 2020 19:12 Inactive
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.

4 participants