Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Check the correctness and consistency of immutability constraints in all of our resources #1448

Closed
3 tasks done
danyinggu opened this issue Jul 14, 2020 · 5 comments · Fixed by #1523
Closed
3 tasks done
Assignees
Labels
area/sources kind/bug Something isn't working kind/feature-request New feature or request priority/0 Fix now and possibly cherry picked into the most recent release. storypoint/5
Milestone

Comments

@danyinggu
Copy link
Contributor

danyinggu commented Jul 14, 2020

Problem
Related to #1441, #1442 and #1433.
When promoting the sources related types to v1, we found that the checkImmutableFields methods between validations are somewhat inconsistent and incomplete (also for the UTs). We need to check them again in the following sprint to make sure they are correct.

Exit Criteria

  • Make sure the checkImmutability is called in validation logic for all versions of sources (v1, v1beta1 and v1alpha1). @bharattkukreja

  • Make sure in our code base, the immutable fields that are included in the checkImmutableFields function are correct, complete and consistent among sources, need to add the missing fields if needed. @danyinggu
    As mentioned in PR Update immutable fields for sources in v1alpha1 and v1beta1 #1504, we add immutability check ClusterNameAnnotation and AutoscalingClass Annotation (relates to Autoscaling annotation should be made immutable #665) for all versions of resources. We also add more fields to be immutable in all of the versions. (see the PR for details)

  • Based on 1 and 2, make sure the immutable fields cannot be mutated when using kubectl edit and mutable fields can be mutated. @bharattkukreja

cc @nachocano

@danyinggu danyinggu added this to the Backlog milestone Jul 14, 2020
@nachocano nachocano added priority/1 Blocks current release defined by release/* label or blocks current milestone release/2 and removed priority/1 Blocks current release defined by release/* label or blocks current milestone release/2 labels Jul 14, 2020
@nachocano
Copy link
Member

We need to do this in order to be actually immutable in higher versions: knative/eventing#3523, knative/eventing#3522

@nachocano
Copy link
Member

I think that needs to be done in v1beta1 and v1...

This has just become a high priority bug

@nachocano nachocano added kind/bug Something isn't working priority/0 Fix now and possibly cherry picked into the most recent release. release/2 labels Jul 16, 2020
@capri-xiyue
Copy link
Contributor

capri-xiyue commented Jul 17, 2020

I think that needs to be done in v1beta1 and v1...

This has just become a high priority bug

Wondering whether we also need to do that in v1alpha1. I checked the codebase, we write the function CheckImmutableFields but it never gets called in validation logic.

@nachocano nachocano removed this from the Backlog milestone Jul 21, 2020
@danyinggu
Copy link
Contributor Author

danyinggu commented Jul 21, 2020

@bharattkukreja : Just synced with @nachocano offline, the scope of this issue is:

  1. Make sure the checkImmutability is called in validation logic for all versions of sources (v1, v1beta1 and v1alpha1).
  2. Make sure in our code base, the immutable fields that are included in the checkImmutableFields function are correct, complete and consistent among sources, need to add the missing fields if needed.
  3. Based on 1 and 2, make sure the immutable fields cannot be mutated when using kubectl edit and mutable fields can be mutated.

@danyinggu
Copy link
Contributor Author

danyinggu commented Jul 21, 2020

I already worked on the UTs to cover all fields in our current code base as the UT does not cover some fields check in our current version in the code base, will submit a PR, and @bharattkukreja will take over this issue. He might need to add more fields if missing and add UTs to them (see point 2 in the above comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/sources kind/bug Something isn't working kind/feature-request New feature or request priority/0 Fix now and possibly cherry picked into the most recent release. storypoint/5
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants