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 Cluster/RoleBinding replace semantics #337

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

hausdorff
Copy link
Contributor

Fixes #335.

@hausdorff
Copy link
Contributor Author

I've tested this pretty extensively. Given the limited blast radius and our confidence in the change I'm going to go ahead and merge this once CI passes.

@hausdorff hausdorff force-pushed the hausdorff/rolebinding-replace branch from c22af5f to dd6b07d Compare January 4, 2019 20:15
@hausdorff hausdorff force-pushed the hausdorff/rolebinding-replace branch from dd6b07d to afeca11 Compare January 4, 2019 21:01
@hausdorff hausdorff merged commit d4f8be4 into master Jan 4, 2019
@pulumi-bot pulumi-bot deleted the hausdorff/rolebinding-replace branch January 4, 2019 21:27
@joeduffy
Copy link
Member

joeduffy commented Jan 7, 2019

I'm growingly nervous about our manual approach to maintaining these lists. /cc @lukehoban @lblackstone for feedback on this also

For example, take StorageClass:

"storage.k8s.io": versions{
"v1": kinds{
"StorageClass": properties{
".parameters",
".provisioner",
},
},
},

I assume these settings are largely driven by logic like the following: https://github.com/kubernetes/kubernetes/blob/8dbda2258776332f1b9a18c6d80fb5ce5e675c25/pkg/apis/storage/validation/validation.go#L56

If that's true, it means for this randomly selected example, we missed the reclaimPolicy. Possibly binding mode also? (I'm not certain what apivalidation.ValidateImmutableField does...)

I am concerned that, on current course and speed, we're going to play whack a mole effectively endlessly, one by one, as customers run into and report problems.

Is there an alternative approach we can take here? Maybe even invoking the validation logic to discover errors dynamically, to determine what requires a replacement, since this information doesn't appear to be available statically? Or am I just overly, and unnecessarily, concerned?

@hausdorff
Copy link
Contributor Author

I'm concerned enough that we're actually already working with SIGCLI to add this information to the OpenAPI spec, which is the only way to fix this once and for all in a way that is compatible with CRDs. This rolls up into our work to standardize on .status conditions.

FWIW, I am less concerned about having a big manual list. These conditions are static and once we have a definitive list, it should be super powerful. The concern for me, as always, is the k8s extension points.

I'll open a tracking issue to make sure we follwo up.

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.

None yet

2 participants