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

(delta/sds): Mark secret as ready if server says the resource doesn't exist #33362

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

keithmattix
Copy link
Contributor

As discussed in Slack, a delta removal should complete cluster warming

Commit Message: (fix/sds): Mark cluster as ready after
Additional Description:
Risk Level: Low
Testing: integration
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

… exist

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Small nits but otherwise LGTM, thanks!
Assigning Harvey for senior-maintainer review.
/assign @htuch

test/integration/ads_integration_test.cc Outdated Show resolved Hide resolved
test/integration/ads_integration_test.cc Outdated Show resolved Hide resolved
@htuch
Copy link
Member

htuch commented Apr 7, 2024

Does this need runtime protection? Seems pretty minor corner case, so I'm inclined to say no, but just putting it out there.

@keithmattix
Copy link
Contributor Author

Does this need runtime protection? Seems pretty minor corner case, so I'm inclined to say no, but just putting it out there.

Runtime protection in what sense?

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
@htuch
Copy link
Member

htuch commented Apr 9, 2024

As per https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#breaking-change-policy. I think it's fine TBH in this case, but for next behavior change it's worth familiarising.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Can you update the version history to be clear this changed?

@keithmattix
Copy link
Contributor Author

IIUC, when you take both of my PRs together, the only change to the user is that an SDS removal of a secret that doesn't exist yet is NACK'd instead of ACK'd. Based on the breaking change policy (thanks for the link!), the first PR was the breaking change but this PR reverts to the status quo with respect to the init target being marked as ready. Sorry if I'm misunderstanding something

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

OK, makes sense, thanks.

@htuch htuch merged commit 8f0e1ea into envoyproxy:main Apr 10, 2024
53 checks passed
@keithmattix keithmattix deleted the sds/delta-mark-secret-as-ready branch April 17, 2024 19:50
alyssawilk pushed a commit to alyssawilk/envoy that referenced this pull request Apr 29, 2024
… exist (envoyproxy#33362)

As discussed in Slack, a delta removal should complete cluster warming

Risk Level: Low
Testing: integration

Signed-off-by: Keith Mattix II <keithmattix@microsoft.com>
keithmattix added a commit to istio/community that referenced this pull request Jul 18, 2024
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback!

PRs:
- envoyproxy/envoy#35074
- envoyproxy/envoy#33857
- envoyproxy/envoy#33362
- envoyproxy/envoy#32961
- istio/proxy#5617
- istio/proxy#5514
istio-testing pushed a commit to istio/community that referenced this pull request Jul 19, 2024
Istio has very few maintainers here and though I'm still learning, I've contributed a handful of upstream envoy PRs as well as 2 recent changes to istio-proxy's telemetry. If existing maintainers have other areas they'd like to see more contributions in, I welcome the feedback!

PRs:
- envoyproxy/envoy#35074
- envoyproxy/envoy#33857
- envoyproxy/envoy#33362
- envoyproxy/envoy#32961
- istio/proxy#5617
- istio/proxy#5514
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

3 participants