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

0.87 release planned 🎃 #1333

Closed
clux opened this issue Oct 31, 2023 · 12 comments
Closed

0.87 release planned 🎃 #1333

clux opened this issue Oct 31, 2023 · 12 comments

Comments

@clux
Copy link
Member

clux commented Oct 31, 2023

Quite a bit in the milestone now and seems like a good time to do another release. particularly since we got a fix in the controller scheduler (and another timeout pass-down "fix")

particular stuff worth highlighting imo:

  • socks5
  • auth plugin work (although still some WIP there in Provide cluster info to exec plugins #1331)
  • Controller::reconcile_on change (breaking, but it's for an unstable interface)
  • dep upgrades, syn upgrade caused a break on #[kube(struct)]

will likely hit it up later this evening.

@clux
Copy link
Member Author

clux commented Oct 31, 2023

ugh, failed mid release:

error[E0425]: cannot find value `connector` in this scope
   --> src/client/builder.rs:123:51
    |
123 |         let mut connector = TimeoutConnector::new(connector);
    |                                                   ^^^^^^^^^ not found in this scope

@clux
Copy link
Member Author

clux commented Oct 31, 2023

kube-core was the only one that made it through. yanked it to avoid confusing dependabot et al:

cargo yank --version 0.87.0 kube-core
    Updating crates.io index
        Yank [email protected]

@clux
Copy link
Member Author

clux commented Oct 31, 2023

Ok, we somehow uncovered a fault in CI testing here; cargo test -p kube-client --no-default-features fails. Annoying because we do actually try to do a no-default-feature build of kube (*), just not all the underlying crates...

The problem in this case is the socks5 PR removing these 2 lines: https://github.com/kube-rs/kube/pull/1311/files#diff-1ab2824dd0a6633b63d18104f91fc5e7dfb07bb14a80c956af7b8af33162813bL78-L79 which are not reinstated in the new make_generic_builder for all cases. I.e. we can't create the service anymore in the case of no tls stack. i.e.

Not decided what to do yet for the release. Either we temporarily backout the PR or fix forwards. cc @Razz4780 . In either case, not doing the release tonight.

@Razz4780
Copy link
Contributor

Ok, we somehow uncovered a fault in CI testing here; cargo test -p kube-client --no-default-features fails. Annoying because we do actually try to do a no-default-feature build of kube (*), just not all the underlying crates...

The problem in this case is the socks5 PR removing these 2 lines: https://github.com/kube-rs/kube/pull/1311/files#diff-1ab2824dd0a6633b63d18104f91fc5e7dfb07bb14a80c956af7b8af33162813bL78-L79 which are not reinstated in the new make_generic_builder for all cases. I.e. we can't create the service anymore in the case of no tls stack. i.e.

Not decided what to do yet for the release. Either we temporarily backout the PR or fix forwards. cc @Razz4780 . In either case, not doing the release tonight.

Wait, what exactly is the problem? cargo test -p kube-client --no-default-features works on main. Those two lines are still there in TryFrom<Config> implementation for GenericService. make_generic_builder accepts the base connector as an argument.

@clux
Copy link
Member Author

clux commented Oct 31, 2023

uh, i think the problem is that the make_generic_builder expects to use the base_connector in one of the feature-flagged lines below in https://github.com/kube-rs/kube/blob/main/kube-client/src/client/builder.rs#L113-L116, but if neither are true, then you don't actually have the connector variable in scope. the cargo test command i wrote does indeed work though 😕

@clux
Copy link
Member Author

clux commented Oct 31, 2023

this fails at root in main:

cargo test -p kube --no-default-features --features=client

EDIT: but more importantly, build also:

cargo build -p kube --no-default-features --features=client

clux added a commit that referenced this issue Oct 31, 2023
excessive shadowing to avoid `connector` being an undefined var for no-tls build.
for #1333

Signed-off-by: clux <[email protected]>
clux added a commit that referenced this issue Oct 31, 2023
excessive shadowing to avoid `connector` being an undefined var for no-tls build.
for #1333

Signed-off-by: clux <[email protected]>
@Razz4780
Copy link
Contributor

Ah, that's right. #1335 fixes the issue

@clux
Copy link
Member Author

clux commented Oct 31, 2023

Seems we have both arrived at the same fix: #1334
Appreciate it!

@Razz4780
Copy link
Contributor

Btw. one line in the related code seems fishy - https://github.com/kube-rs/kube/pull/1335/files#diff-1ab2824dd0a6633b63d18104f91fc5e7dfb07bb14a80c956af7b8af33162813bL118

if auth_layer.is_none() || config.cluster_url.scheme() == Some(&http::uri::Scheme::HTTPS) {
    // no tls stack situation only works on anonymous auth with http scheme
    return Err(Error::TlsRequired);
}

I haven't checked the code thoroughly, but based on the comment it seems to me that it should rather be something like

if auth_layer.is_some() || config.cluster_url.scheme() != Some(&http::uri::Scheme::HTTP) {
    // no tls stack situation only works on anonymous auth with http scheme
    return Err(Error::TlsRequired);
}

@clux
Copy link
Member Author

clux commented Oct 31, 2023

hm, good spot. i think the auth_layer check might be leftover from my initial experiments. i suspect it's a worthless check, and the scheme check is doing all the heavy lifting. the https scheme should be the only necessary condition for triggering the error (you could actually have an auth layer with basic auth without using https - and that shouldn't throw here)

clux added a commit that referenced this issue Nov 1, 2023
* Fix no-tls build for `Client`

excessive shadowing to avoid `connector` being an undefined var for no-tls build.
for #1333

Signed-off-by: clux <[email protected]>

* feature test

Signed-off-by: clux <[email protected]>

* limit TlsRequired error to no-tls when scheme is HTTPS

spotted in upstream discussion

Signed-off-by: clux <[email protected]>

* fix comment

Signed-off-by: clux <[email protected]>

---------

Signed-off-by: clux <[email protected]>
@clux clux closed this as completed in 5813ad0 Nov 1, 2023
@goenning
Copy link
Contributor

Hi @clux it seems like version 0.86 is yanked, but I could not find any notes on why. This issue only mentions about 0.87 (which is not even listed on crates.io), but what happened to 0.86?

@clux
Copy link
Member Author

clux commented Dec 11, 2023

Hey @goenning , it was yanked as a result of #1316 (comment) to try to avoid having more people being bitten by that controller bug - linked to that release.

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

No branches or pull requests

3 participants