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

Add VadlidatingAdmissionPolicy RBAC and bump k8s to v1.30 release #3564

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

hjiawei
Copy link
Contributor

@hjiawei hjiawei commented Oct 24, 2024

Description

This changeset adds necessary VadlidatingAdmissionPolicy RBACs for Tigera apiserver and bumps k8s dependency to v1.30 release. This change is tested in a Calico Enterprise cluster.

Related to: projectcalico/calico#9335.

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

This changeset adds necessary VadlidatingAdmissionPolicy RBACs
for Tigera apiserver and bumps k8s dependency to v1.30 release.
@hjiawei hjiawei marked this pull request as ready for review October 25, 2024 05:26
@hjiawei hjiawei requested a review from a team as a code owner October 25, 2024 05:26
@@ -90,13 +90,13 @@ type Endpoint struct {
HonorTimestamps *bool `json:"honorTimestamps,omitempty"`

// MetricRelabelConfigs to apply to samples before ingestion.
MetricRelabelConfigs []*v1.RelabelConfig `json:"metricRelabelings,omitempty"`
MetricRelabelConfigs []v1.RelabelConfig `json:"metricRelabelings,omitempty"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is changed in upstream PR #6479. @rene-dekker I think this might be the most clean way to adapt to the vendor change. WDYT?

Copy link
Member

@rene-dekker rene-dekker Oct 25, 2024

Choose a reason for hiding this comment

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

We effectively only use this variable in 1 place. I think it would be better to convert the field where it is used, so that we don't need to make breaking changes to our API.

Maybe in pkg/ptr/conversion.go we can add a utility function converting pointer slices to slices using generics?

Copy link
Member

Choose a reason for hiding this comment

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

Jiawei reminded me that this is not really a breaking change. Everything that is not an array containing nil values should deserialize properly. That paired with the knowledge that this feature is quite new and not used so much yet, makes me think that it is probably ok.

WDYT: @tmjd @caseydavenport

Copy link
Member

@caseydavenport caseydavenport Oct 25, 2024

Choose a reason for hiding this comment

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

I think it is OK, but I do also think it is a breaking API change.

For one, anyone importing our code using this field (doubt that's anyone!) will fail to compile when they bump the version.

Technically the nil value array is still a breaking change, although we can probably make a case that nobody is using that config since it wouldn't have worked before anyway (but our API would have accepted it)

All this to say that I'm OK with making this change to make our lives easier, even though technically it's a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Seems ok to me

@@ -165,7 +165,7 @@ func AddDeploymentWatch(c ctrlruntime.Controller, name, namespace string) error
}

func AddPeriodicReconcile(c ctrlruntime.Controller, period time.Duration, handler handler.EventHandler) error {
return c.Watch(&source.Channel{Source: createPeriodicReconcileChannel(period)}, handler)
return c.Watch(source.Channel(createPeriodicReconcileChannel(period), handler))
Copy link
Contributor Author

@hjiawei hjiawei Oct 25, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@rene-dekker rene-dekker left a comment

Choose a reason for hiding this comment

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

lgtm

@rene-dekker rene-dekker merged commit 60b5624 into tigera:master Oct 25, 2024
5 checks passed
@hjiawei hjiawei deleted the k8s-1.30 branch October 25, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants