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

chore: upgrading to 0.30 api, creating v1 or v1beta1 VAP/VAPB #3472

Merged
merged 22 commits into from
Aug 8, 2024

Conversation

JaydipGabani
Copy link
Contributor

Signed-off-by: Jaydip Gabani [email protected]
(cherry picked from commit 34275e7a564bf4f617bd232795d4426628cb7ece)

What this PR does / why we need it: In 1.30 if beta APIs are not enabled, VAP and VAPB generation will fail. This PR looks at available api version and creates VAP/VAPB

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

Signed-off-by: Jaydip Gabani <[email protected]>
(cherry picked from commit 34275e7a564bf4f617bd232795d4426628cb7ece)
@JaydipGabani JaydipGabani requested a review from a team as a code owner August 2, 2024 18:15
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Mainly thoughts about how to handle multiple versions.

required:
- key
- operator
type: object
type: array
x-kubernetes-list-type: atomic
Copy link
Contributor

Choose a reason for hiding this comment

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

Is adding this backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maxsmythe I think so, this page says atomic can apply to any list. It is coming through bumping k8s api - https://github.com/kubernetes/apimachinery/blob/v0.30.3/pkg/apis/meta/v1/types.go#L1250

Copy link
Contributor

@maxsmythe maxsmythe Aug 6, 2024

Choose a reason for hiding this comment

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

Backwards compatibility means that applying this tag will not change merge behavior.

In other words, we should be sure that kubectl apply logic prior to this tag being added and post this tag being added are unchanged.

Because this potentially affects backwards compatibility, we should be certain that this is the case. We should not rely on speculation. In other words, we should do more research than just inferring from a vague statement in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found By default, Server-Side Apply treats custom resources as unstructured data. All keys are treated the same as struct fields, and all lists are considered atomic., here. So before this change, the list was being treated as atomic for patchStratagy=merge. wdyt?

How do I test for backward compatibility for this change?

I tested with deploying CRDs before this change and creating mutators, then I upgraded the CRDs with this change and applied mutators But I think this may be wrong way to test for backward compat, I am not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also see this If listType is missing, the API server interprets a patchStrategy=merge marker as a listType=map and the corresponding patchMergeKey marker as a listMapKey. on the same page, so now I am cofused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the response - atomic is the default - https://kubernetes.slack.com/archives/C0123CNN8F3/p1722991724774759?thread_ts=1722991068.800879&cid=C0123CNN8F3.

@maxsmythe let me know if this resolves your concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am pasting the response for reference here about this change -

atomic is the default
that was making it explicit to avoid linting errors about implicit list type
the containing labelselector struct is already annotated as +structType=atomic, which generators should turn into an atomic annotation on the parent selector field, so no list type annotations on the child fields should be relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding more context:

This x-kubernetes-list-type: atomic is getting added for list fields under metav1.labelSelector and metav1.namespaceSelector for mutators. This change is coming from here, where marker listType=atomic is getting added to matchExpression and values field. controller-runtime is looking at this markers and adding the field to manifets.

Copy link
Contributor

Choose a reason for hiding this comment

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

"By default, Server-Side Apply treats custom resources as unstructured data. All keys are treated the same as struct fields, and all lists are considered atomic." Sounds conclusive to me (from your first reply)

go.mod Outdated Show resolved Hide resolved
}
return reconcile.Result{}, err
}
if currentVapBinding == nil {
newVapBinding = transformedVapBinding.DeepCopy()
err = r.scheme.Convert(transformedVapBinding, newVapBinding, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will convert honor the GVK set on the unstructured object? It may be better/simpler to have newVapBinding be of Object type (the generic interface that matches all K8s objects, and set it to an empty instance of the appropriate versioned object via a function.

ex:

newVapBinding, err := vapBindingForVersion(gvk) // do err check
r.scheme.Convert(transformedVapBinding, newVapBinding, nil)


func vapBindingForVersion(gvk scheme.GroupVersion) (runtime.Object, error) {
   switch gvk.Version {
   case "v1":
      return &v1.VapBinding{}
   ....
   default:
     return nil, errors.New("unrecognized version")
   }
}

Doing it this way will avoid uncertainties around how the type system handles unstructured and excess version juggling.

} else {
newVapBinding = currentVapBinding.DeepCopy()
newVapBinding.Spec = transformedVapBinding.Spec
unstructuredObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&transformedVapBinding.Spec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets just separate this out to a separate function and use typecasting rather than resorting to unstructured.

createErr := &v1beta1.CreateCRDError{Code: ErrCreateCode, Message: err.Error()}
status.Status.Errors = append(status.Status.Errors, createErr)
err := r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not transform to vap object", status, err)
return reconcile.Result{}, err
}
if currentVap == nil {
newVap = transformedVap.DeepCopy()
err = r.scheme.Convert(transformedVap, newVap, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same concern about convert/unstructured. We can probably just use typed objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using typed conversion.

Makefile Outdated
@@ -172,8 +173,7 @@ e2e-bootstrap: e2e-dependencies
if [ $$(${GITHUB_WORKSPACE}/bin/kind get clusters) ]; then ${GITHUB_WORKSPACE}/bin/kind delete cluster; fi

# Create a new kind cluster
# TODO(ritazh): remove KIND_CLUSTER_FILE when vap feature is GA
if [ $$(echo $(KUBERNETES_VERSION) | cut -d'.' -f2) -lt 28 ]; then ${GITHUB_WORKSPACE}/bin/kind create cluster --image $(KIND_NODE_VERSION) --wait 5m; else ${GITHUB_WORKSPACE}/bin/kind create cluster --config $(KIND_CLUSTER_FILE) --image $(KIND_NODE_VERSION) --wait 5m; fi
if [ $$(echo $(KUBERNETES_VERSION) | cut -d'.' -f2) -lt 28 ]; then ${GITHUB_WORKSPACE}/bin/kind create cluster --image $(KIND_NODE_VERSION) --wait 5m; else ${GITHUB_WORKSPACE}/bin/kind create cluster --image $(KIND_NODE_VERSION) --wait 5m; fi
Copy link
Member

Choose a reason for hiding this comment

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

we still need the kind_cluster_file for versions before vap GAed in 1.30

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yip, adding the file back.

go.mod Outdated
go 1.21
go 1.22.0

toolchain go1.22.2
Copy link
Member

Choose a reason for hiding this comment

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

will this downgrade Go version? do we need GOTOOLCHAIN set to local for release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed toolchain go1.22.2

…ng a bug in IsVAPApiEnabled()

Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
vapMux.Lock()
defer vapMux.Unlock()

if VapAPIEnabled != nil {
return *VapAPIEnabled
return *VapAPIEnabled, GroupVersion
Copy link
Member

Choose a reason for hiding this comment

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

Who sets GroupVersion? what if it's never set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GroupVersion is set alongside VapAPIEnabled=true whenever VAP api with v1 or v1beta is found. If its never set, GroupVersion will be nil, but in that case *VapAPIEnabled will always be false.

Copy link
Member

Choose a reason for hiding this comment

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

since GroupVersion can be nil, please check for nil for the returned value from the caller function

Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
(cherry picked from commit 34275e7a564bf4f617bd232795d4426628cb7ece)
…ng a bug in IsVAPApiEnabled()

Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
Signed-off-by: Jaydip Gabani <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2024

Codecov Report

Attention: Patch coverage is 48.94895% with 170 lines in your changes missing coverage. Please review.

Project coverage is 48.07%. Comparing base (3350319) to head (5b3ffa0).
Report is 118 commits behind head on master.

Files Patch % Lines
pkg/controller/constraint/constraint_controller.go 0.00% 112 Missing ⚠️
...onstrainttemplate/constrainttemplate_controller.go 70.27% 27 Missing and 6 partials ⚠️
pkg/controller/pubsub/pubsub_config_controller.go 0.00% 15 Missing ⚠️
pkg/controller/mutators/core/adder.go 52.38% 10 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (5b3ffa0). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (3350319) HEAD (5b3ffa0)
unittests 2 1
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3472      +/-   ##
==========================================
- Coverage   54.49%   48.07%   -6.43%     
==========================================
  Files         134      219      +85     
  Lines       12329    15165    +2836     
==========================================
+ Hits         6719     7290     +571     
- Misses       5116     7059    +1943     
- Partials      494      816     +322     
Flag Coverage Δ
unittests 48.07% <48.94%> (-6.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani JaydipGabani requested a review from ritazh August 6, 2024 03:32
@sozercan sozercan added this to the v3.17.0 milestone Aug 6, 2024
@@ -609,35 +628,128 @@ func (c *ConstraintsCache) reportTotalConstraints(ctx context.Context, reporter
}
}

func IsVapAPIEnabled() bool {
func IsVapAPIEnabled() (bool, *schema.GroupVersion) {
vapMux.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to have an RWMutex here to enable parallelism in the future.

This is especially true because this function is being called by two different reconcilers (constraint/constraint template).

General pattern:

  • acquire read lock
  • check if VapAPIEnabled has been assigned a value, if so, return it
  • Otherwise, drop read lock and acquire a write lock
  • check if VapAPIEnabled has been assigned a value, if so, return it (this is necessary b/c a write could have happened in the time the lock was released)
  • continue as written

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added read lock

}
}

func (r *ReconcileConstraint) getRunTimeVAPBinding(gvk *schema.GroupVersion, transformedVapBinding *admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding, currentVapBinding client.Object) (client.Object, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this does not need to be a member function, it can be a standalone function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified to standalone function.

}
}

func (r *ReconcileConstraintTemplate) getRunTimeVAP(gvk *schema.GroupVersion, transformedVap *admissionregistrationv1beta1.ValidatingAdmissionPolicy, currentVap client.Object) (client.Object, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can be standalone function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified to standalone function.

return v1beta1VAP.DeepCopy(), nil
}

func v1beta1ToV1(v1beta1Obj *admissionregistrationv1beta1.ValidatingAdmissionPolicy) (*admissionregistrationv1.ValidatingAdmissionPolicy, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use scheme.Convert here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Direct conversion from v1beta1.ValidatingAdmissionPolicy to v1.ValidatingAdmissionPolicy using r.scheme.Convert results in error -
converting (v1beta1.ValidatingAdmissionPolicy) to (v1.ValidatingAdmissionPolicy): unknown conversion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oof, sounds like a bug in K8s code. Interim solution LGTM

Copy link
Member

@ritazh ritazh Aug 7, 2024

Choose a reason for hiding this comment

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

@JaydipGabani do you want to open an issue for this on k/k? and add a comment to remove it after it's fixed in k/k?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue - kubernetes/kubernetes#126582

return v1beta1VAPBinding.DeepCopy(), nil
}

func v1beta1ToV1(v1beta1Obj *admissionregistrationv1beta1.ValidatingAdmissionPolicyBinding) (*admissionregistrationv1.ValidatingAdmissionPolicyBinding, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use scheme.Convert() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same happens with the binding as well, using r.scheme.Convert results in error in direct conversion.

@JaydipGabani JaydipGabani requested a review from maxsmythe August 6, 2024 20:09
@@ -629,6 +629,13 @@ func (c *ConstraintsCache) reportTotalConstraints(ctx context.Context, reporter
}

func IsVapAPIEnabled() (bool, *schema.GroupVersion) {
vapMux.RLock()
if VapAPIEnabled != nil {
vapMux.RUnlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

For strict correctness, you should only access the variable while it's under a read lock. Suggest using intermediate variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added local variables for strict correctness.

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM after nit fix and after the backwards compatibility question is resolved.

@JaydipGabani JaydipGabani requested a review from maxsmythe August 7, 2024 17:28
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Code LGTM, did not review tests, but defer to ritazh@/sozercan@ for those.

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

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

LGTM

transformedVapBinding, err := transform.ConstraintToBinding(instance, VAPEnforcementActions)
if err != nil {
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
if err2 := r.writer.Update(ctx, status); err2 != nil {
log.Error(err2, "could not report transform vapbinding error status")
log.Error(err2, "could not report transform vapbinding error", "vapName", vapBindingName)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.Error(err2, "could not report transform vapbinding error", "vapName", vapBindingName)
log.Error(err2, "could not report transform vapbinding error", "vapBindingName", vapBindingName)

if err != nil {
status.Status.Errors = append(status.Status.Errors, constraintstatusv1beta1.Error{Message: err.Error()})
if err2 := r.writer.Update(ctx, status); err2 != nil {
log.Error(err2, "could not get VAP object with runtime group version", "vapName", vapBindingName)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log.Error(err2, "could not get VAP object with runtime group version", "vapName", vapBindingName)
log.Error(err2, "could not get VAP object with runtime group version", "vapBindingName", vapBindingName)

return false, nil
}

groupVersion := schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
groupVersion := schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1"}
groupVersion := schema.GroupVersion{Group: admissionregistrationv1beta1.GroupName, Version: "v1"}

*VapAPIEnabled = false
return false

groupVersion = schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
groupVersion = schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"}
groupVersion = schema.GroupVersion{Group: admissionregistrationv1beta1.GroupName, Version: "v1beta1"}

}
log.Info("IsVapAPIEnabled true")

log.Info("IsVapAPIEnabled", "error", err)
Copy link
Member

Choose a reason for hiding this comment

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

why are we logging an error as info?

suffix := "VapShouldBeCreatedV1"

logger.Info("Running test: Vap should be created with v1")
constraint.GroupVersion = &schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
constraint.GroupVersion = &schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1"}
constraint.GroupVersion = &schema.GroupVersion{Group: admissionregistrationv1beta1.GroupName, Version: "v1"}

Copy link
Member

Choose a reason for hiding this comment

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

hmm why didnt the test catch this?

Signed-off-by: Jaydip Gabani <[email protected]>
@JaydipGabani JaydipGabani merged commit 36b75d7 into open-policy-agent:master Aug 8, 2024
21 checks passed
@JaydipGabani JaydipGabani deleted the k130 branch August 8, 2024 00:27
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.

5 participants